-
Notifications
You must be signed in to change notification settings - Fork 94
Add support for using DNSNames instead of raw IPs for IMEX daemons #433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0a2bc66 to
03b41e2
Compare
| // might be pointless. Terminate us. | ||
| return fmt.Errorf("error (re)starting IMEX daemon: %w", err) | ||
| if err := processManager.EnsureStarted(); err != nil { | ||
| return fmt.Errorf("failed to ensure IMEX daemon is started: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I write next is probably premature before we have talked synchronously. But I already want to leave a thought / questions here that I have on my mind right now.
Does the daemon watch for node config file changes?
So far, I thought that here (after an update) we have to
- send a signal to the child process to either make it "re-initiate all TCP connections" or "re-read the config" (and have it decide itself if it wants to (re-)initiate some TCP connections (I choose this wording because as the first step of a TCP connect() there's DNS (re)resolution (hopefully bypassing any cache (I've never nested parentheses in normal text so deeply so I thought I add another layer just for fun))))
- or (as before) bluntly restart the child.
And zooming out a little further, in your thinking you are probably carefully distinguishing between these cases:
- 'growing' (new DNS name in pool)
- 'shrinking' (DNS name removal from pool)
- 'changing' (old DNS name pointing to a different IP address)
- 'not changing' (nothing changed)
And of course these are not all equally difficult to support. I assume your experiment / poccing focused on just one of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About:
I thought that here (after an update) we have to send a signal to the child process to either make it "re-initiate all TCP connections"
Answered and discussed here: https://github.com/NVIDIA/k8s-dra-driver-gpu/pull/433/files#r2318598969
I assume your experiment / poccing focused on just one of them?
Given the test shared by @klueska I believe it's fair to say we are confirming here mainly
- 'changing' (old DNS name pointing to a different IP address)
and maybe implicitly also
- 'not changing' (nothing changed)'
32d6a21 to
7db9d06
Compare
|
In its current form, this only works if the rescheduled pods reland on the same node... |
16cea5a to
ff87ce4
Compare
The latest change fixes this issue. It does this by ensuring a consistent And it make sense -- normally DNS names do provide such a global mapping — I was just abusing things by only maintaining a local mapping without any regard for how that mapping was done on other nodes. |
fb0b257 to
948b741
Compare
972cd01 to
44d2462
Compare
0bc7f08 to
be1170e
Compare
Signed-off-by: Kevin Klues <[email protected]>
Signed-off-by: Kevin Klues <[email protected]>
Without this the IMEX daemons were getting confused if the same DNS name was used to by different nodes to point to differernt IMEX daemons in the ensemble. Signed-off-by: Kevin Klues <[email protected]>
be1170e to
9715887
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting! After lots of discussion and iteration: let's get this in 🚀
This patch lays the foundation for eventually resolving the following issues:
It does this by leveraging the new "hostnames" featured introduced in newer versions of the IMEX daemon. This new "hostnames" feature allows one to list a set of DNS resolvable names in the
nodes_config.cfgfile read by an IMEX daemon. In doing so, we are able to provide a level-of-indirection between each individual IMEX daemon and how it connects to other IMEX daemons in the same IMEX domain.So long as all of the IMEX daemons agree on the same set of DNS names in their
nodes_config.cfgfiles, we can dynamically change which IMEX daemon each of these names resolves to, thus providing natural support for things like (1) failing over worker pods to other nodes, (2) replacing nodes on node failures, and (2) expanding workloads to include more or fewer workers over time.The only feature directly enabled by this PR is #348. The others are not directly addressed, but this PR does lay the foundation for resolving each of them in the future.
Fixes: #348
Test ran to verify its working as expected: