Conversation
|
Just want to add my feedback to this. I just tested this with the following setup: Environment:
I had to build a new k8s-mig-manager based on @Arc676 repo. I then pushed it to docker.io/linkages/k8s-mig-manager:v0.14.1. Then when I deploy the gpu-operator, I set the values for the helm chart using this: driver:
enabled: false
toolkit:
enabled: false
hostPaths:
driverInstallDir: /usr/local
mig:
strategy: mixed
migManager:
enabled: true
repository: docker.io/linkages
version: v0.14.1
env:
- name: READONLY_ROOTFS
value: "true"
- name: SYSTEMD_UNAVAILABLE
value: "true"
operator:
cleanupCRD: trueI then set the This was tested on 2 different types of nodes in the same cluster: 2 x Lenovo nodes with 8 x NVIDIA RTX PRO 6000 Blackwell Server Edition GPUs Thank you @Arc676 for this patch. I hope it or a more elegant version of this gets pulled upstream. For now this solves my problem. |
|
Thanks @Arc676 for tackling this! My preference is to replace the flag-based approach in favor of something with no new public surface area. The reason being We can document that on platforms without systemd In the meantime, we should remove the unconditional |
|
Thanks for the feedback! I agree with your point regarding the public surface; adding flags was the easiest approach but both properties can be inferred from the system's behavior. Instead of requiring the user to set these flags,
Support for systems without What would be the advantage of a lazy init for the We could keep the unconditional Unless you want to separate the features, I'd implement all these changes in this PR such that Talos support is covered. I've created a new issue to track the change to the Helm chart per your suggestion. |
|
I've adapted the implementation based on the above comments:
|
|
Any update here? Not sure what's blocking the automated checks; I don't really use GitHub actions. |
|
/ok-to-test 125d1db |
@rajathagasthya, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
@Arc676 This is on my list. I'm going to take a closer look this week. Thanks for your patience! |
There was a problem hiding this comment.
@Arc676 Thanks for your patience and for reworking this! I expanded on some of my earlier suggestions in this review.
As for NVIDIA/gpu-operator#2501, I think the decoupling can be done in a non-breaking way in the entrypoint script like below:
WITH_SHUTDOWN_HOST_GPU_CLIENTS=${WITH_SHUTDOWN_HOST_GPU_CLIENTS:-$IS_HOST_DRIVER}
|
I opened #431 for the same bug before seeing this thread, and was pointed here — makes sense to land the fix in this PR rather than compete, since it's older and already hardware-tested. Happy to fold my work in (co-author, or just take the pieces), or step back if you'd rather carry it yourself. I did end up at exactly the refactor requested in the 07-08 review, so it might save a round-trip. In #431:
The diff is on #431 as a reference. Glad to push these here as commits with you as co-author, or hand them over — whatever's easiest. Just say which you prefer. |
Arc676
left a comment
Arc676
left a comment
There was a problem hiding this comment.
@lexfrei thanks for reaching out! I actually prefer the timer-based approach (I hadn't thought of that); it's a lot cleaner than opening two connections. I also hadn't implemented any unit tests, since I tested the build in the cluster directly, so that's a useful addition for the pipeline here on GitHub. I cherry-picked your commits into my branch and updated the trailers in case the code-signing checks care about authorship. Conveniently, we used similar identifiers so there were almost no conflicts.
@rajathagasthya I went with the assumption that having unit tests is better than not having them. However, the commits are still distinct, so if you would prefer to review the tests separately, I can easily reset the branch to its HEAD~2 and re-separate the two PRs. Let me know what you think makes the most sense. I think at this point, by line count, this PR mostly contains the content of the other PR. They do the same thing so I don't really mind which one you'd prefer to follow going forward.
|
@Arc676 thanks for picking up my commits from #431 — the lazy-init shape is right. One regression to flag, in func (r *Reconfigure) cleanup() {
if mgr, _ := r.getSystemdManager(); mgr != nil {
mgr.Close()
}
}
if r.systemdManager != nil {
r.systemdManager.Close()
}Reading Separately, on |
|
@lexfrei thanks for catching the The default timeout of one second is actually outdated; I adopted your implementation of the manager setup, which does not propagate the user's command line options. Hence, the timeout is actually fixed at 10 seconds. I've removed the obsolete constant. I can put back the command line parameter if needed. |
|
Checked your cleanup correction — looks right, thanks. On the timeout: I'd keep it as a fixed internal constant rather than reintroducing the CLI parameter. A connect timeout is a liveness bound, not a tuning knob — 10s is comfortably above any healthy D-Bus handshake, and one less flag keeps the public surface smaller. If someone ever shows a real environment where 10s is wrong, that's the moment to add the knob. |
@Arc676 I do not see this correction in code; there are no new commits since then. What am I missing? |
|
@rajathagasthya oops, this is embarrassing. I pushed to a different remote and didn't update the GitHub fork. Now the branch is updated (and I've set the local upstream to track this branch). |
|
All good, thanks @Arc676! We are in a code freeze right now for a GPU Operator release, so I'll hold off an approval for now, but it looks good to me. Would you also like to make the corresponding GPU Operator chart change? WITH_SHUTDOWN_HOST_GPU_CLIENTS=${WITH_SHUTDOWN_HOST_GPU_CLIENTS:-$IS_HOST_DRIVER} |
|
/ok-to-test 1a23791 |
|
Great work on this @Arc676 ! The PR looks good to me for the most part. My only ask is that we reduce the number of comments. In general, let's use comments sparingly; we reserve comments to provide context or information when it's not obvious after reading the code. |
|
Can you rebase your PR? I think that will resolve the stuck |
rajathagasthya
left a comment
rajathagasthya
left a comment
There was a problem hiding this comment.
@Arc676 Could you squash your commits into a single commit?
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
There is still a timeout race that can leak a live D-Bus connection. If the timer/context branch wins just as NewSystemConnectionContext succeeds, the goroutine can enqueue a non-nil *dbus.Conn into the buffered result channel after NewManager has returned; go-systemd requires the successful connection to be closed, and nothing drains or closes this late result. Please make result ownership explicit so a connection arriving after cancellation is closed, and add a race-oriented test.
|
@sylvesterkaczmarek that's a good catch. Although I'm not sure how one would implement a race-oriented test. I guess one option would be to copy the |
|
I would avoid copying |
Add timeout for systemd connection Autodetect systemd availability Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Treat connection timeouts as hard errors Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Move dbus envvar Add comments to constructors Assisted-By: Claude <noreply@anthropic.com> Co-authored-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch> Signed-off-by: Aleksei Sviridkin <f@lex.la> Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
go-systemd's connection setup performs a blocking read during the D-Bus auth handshake. When the system bus socket exists but no daemon answers it - as on a systemd-less host where /run/dbus/system_bus_socket is bind-mounted from the host - that read never returns and the caller hangs indefinitely. Bound the connection setup with a timeout. The dial runs in a goroutine; if it does not complete in time, the connection context is canceled, go-systemd's watcher closes the socket to unblock the stuck read, and NewManager returns a clear error instead of blocking forever. This is a backstop for the paths that do require systemd, so a misconfigured or absent host daemon surfaces an actionable error rather than a silent hang. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> Co-authored-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch> Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Remove obsolete default parameter Don't fetch manager on cleanup Co-authored-by: Aleksei Sviridkin <f@lex.la> Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Address review comments Remove or shorten verbose commenting Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Put back comment for exported function Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
|
@sylvesterkaczmarek sounds good to me. I've added a corresponding test case. Let me know what you think. @rajathagasthya I put back the comment on the |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Thanks for adding the race-oriented coverage. The late-result ownership issue is improved, but the timeout branch currently calls cancel() and then performs a blocking receive from ch before returning. If the connector does not promptly honor cancellation, NewManager can still block past the advertised timeout. I would keep the timeout return independent of connector completion, then drain and close any late non-nil connection asynchronously. A deterministic injected connector that waits until explicitly released after the timeout would pin both properties: the call returns on time, and the late connection is still closed.
Assisted-by: Microsoft Copilot:GPT5 Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
|
The blocking receive is my mistake; I don't do a lot of async. There should have been a I asked GPT and got case <-timer.C:
cancel()
go func() {
res := <-ch
if res.conn != nil {
_ = res.conn.Close()
}
}()
return nil, timeoutErrPutting aside the invalid In the interest of time, I've pushed the corrected version of GPT's suggestion. |
|
Yes. I would move ownership of the late connection into the worker rather than start a second goroutine that can wait on The cleanest shape is to make the injected connector return the result, then use an unbuffered result channel: type connectFunc func(context.Context) (*dbus.Conn, error)
ch := make(chan result)
connCtx, cancel := context.WithCancel(ctx)
go func() {
conn, err := connect(connCtx)
res := result{conn: conn, err: err}
select {
case ch <- res:
// NewManager owns conn now.
case <-connCtx.Done():
if conn != nil {
conn.Close()
}
}
}()Then the timeout branch only calls |
Co-authored-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
|
Further testing has revealed that the original assumption about leaking a In the spirit of your comment regarding explicit ownership, I've made the change you suggested regardless of this new information. While running this code locally, I've found that the race-oriented test is quite flaky, albeit not in its results. The test always passes because of the aforementioned behavior but the exact codepath is not consistent. Adding debug prints in various locations is enough to alter the timing such that the program follows a different codepath. In particular, the I couldn't find an email of yours for the coauthor trailer so I picked a random GitHub-assigned one from one of your repositories. If you'd prefer that I include different metadata (or not attribute coauthorship at all), let me know and I'll update it when I squash the commits at the end of the review process. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked the current head. The timeout path now cancels the production D-Bus connection context, the worker owns any late result, and the added test confirms cancellation disconnects the connection. My earlier late-connection leak concern is resolved.
| var err error | ||
|
|
||
| // Intercept the connection so we can check its state afterwards | ||
| conn, err = dbus.NewSystemConnectionContext(ctx) |
There was a problem hiding this comment.
Does this initiate a real systemd connection? If so, the tests will fail on a machine with no systemd. Should this be pointed at a fake socket?
There was a problem hiding this comment.
This is a good point. I'm not sure what the most meaningful solution is. For the moment, I've added a check such that the tests involving real systemd connections are skipped when the host shutdown flag is set to false (emulating the behavior in production).
I suppose we could try to use a fake socket, but I'm not sure how to do this efficiently. We'd have to either replace the connection setup function, which makes the test deviate from the production behavior, or emulate the real systemd connection's setup and Hello calls such that dbus thinks that it connected properly. This would mean maintaining a sort of minimal dbus server in the test code. Or maybe you have a different idea? I've never used dbus in my own projects so I don't know much about using it.
There was a problem hiding this comment.
For the moment, I've added a check such that the tests involving real systemd connections are skipped when the host shutdown flag is set to false (emulating the behavior in production).
Nothing in tests or CI sets that env var though, so it doesn't have an effect.
I don't know how to use a fake socket either, to be honest. I asked Claude and it is suggested the following diff (for transparency, I don't fully understand it):
diff --git a/internal/systemd/systemd_test.go b/internal/systemd/systemd_test.go
index 67c203e9..c83b6936 100644
--- a/internal/systemd/systemd_test.go
+++ b/internal/systemd/systemd_test.go
@@ -18,6 +18,8 @@
import (
"context"
+ "errors"
+ "io"
"net"
"os"
"path/filepath"
@@ -68,14 +70,18 @@ func TestNewManagerWithTimeoutUnresponsiveSocket(t *testing.T) {
// Accept connections but never write a reply, mimicking a bind-mounted host
// socket with no systemd/D-Bus daemon behind it.
+ accepted := make(chan net.Conn, 1)
go func() {
for {
conn, err := listener.Accept()
if err != nil {
return
}
- // Hold the connection open without responding.
t.Cleanup(func() { _ = conn.Close() })
+ select {
+ case accepted <- conn:
+ default:
+ }
}
}()
@@ -93,6 +99,25 @@ func TestNewManagerWithTimeoutUnresponsiveSocket(t *testing.T) {
if elapsed > 5*time.Second {
t.Errorf("connect took %s, expected it to fail fast near the %s timeout", elapsed, timeout)
}
+
+ // Cancelling the connection context must close the client socket, so the
+ // server side of the stuck handshake reads EOF.
+ var serverConn net.Conn
+ select {
+ case serverConn = <-accepted:
+ case <-time.After(2 * time.Second):
+ t.Fatal("no connection was accepted")
+ }
+ _ = serverConn.SetReadDeadline(time.Now().Add(2 * time.Second))
+ for {
+ _, err := serverConn.Read(make([]byte, 64))
+ if errors.Is(err, io.EOF) {
+ break
+ }
+ if err != nil {
+ t.Fatalf("expected EOF on the server side after the timeout, got: %v", err)
+ }
+ }
}
func TestNewManagerWithTimeoutMissingSocket(t *testing.T) {
@@ -125,55 +150,22 @@ func TestManagerCloseNil(t *testing.T) {
}
}
-func TestManagerRace(t *testing.T) {
- if os.Getenv("WITH_SHUTDOWN_HOST_GPU_CLIENTS") == "false" {
- t.Skip()
- }
-
- const timeout = 1 * time.Second
-
- var conn *dbus.Conn
-
- delayedConnect := func(ctx context.Context) (*dbus.Conn, error) {
- var err error
-
- // Intercept the connection so we can check its state afterwards
- conn, err = dbus.NewSystemConnectionContext(ctx)
+func TestNewManagerWithTimeoutLateConnector(t *testing.T) {
+ const timeout = 100 * time.Millisecond
- // Only send result after context is canceled to simulate race condition
+ // The connector only returns after the context is cancelled, so its result
+ // can never arrive before the timer fires.
+ connect := func(ctx context.Context) (*dbus.Conn, error) {
<-ctx.Done()
-
- return conn, err
+ return nil, ctx.Err()
}
- mgr, err := newManagerWithTimeout(context.Background(), timeout, delayedConnect)
+ start := time.Now()
+ mgr, err := newManagerWithTimeout(context.Background(), timeout, connect)
+ elapsed := time.Since(start)
expectTimeout(t, mgr, err)
-
- if conn.Connected() {
- t.Errorf("expected connection to be dropped after timeout")
- }
-}
-
-func TestCancelDbus(t *testing.T) {
- if os.Getenv("WITH_SHUTDOWN_HOST_GPU_CLIENTS") == "false" {
- t.Skip()
- }
-
- ctx, cancel := context.WithCancel(context.Background())
-
- conn, err := dbus.NewSystemConnectionContext(ctx)
- if err != nil {
- t.Errorf("expected successful connection, got error: %v", err)
- }
-
- if !conn.Connected() {
- t.Error("connection is not connected")
- }
-
- cancel()
-
- if conn.Connected() {
- t.Error("canceling connection context did not disconnect dbus")
+ if elapsed > 2*time.Second {
+ t.Errorf("timeout return took %s, expected it near %s", elapsed, timeout)
}
}There was a problem hiding this comment.
Nothing in tests or CI sets that env var though, so it doesn't have an effect.
True, but this is straightforward to change. We could, for example, update the test target in the Makefile to include
if which systemctl >&/dev/null && systemctl is-system-running >&/dev/null; then
export WITH_SHUTDOWN_HOST_GPU_CLIENTS=true
else
export WITH_SHUTDOWN_HOST_GPU_CLIENTS=false
fiThen the systemd tests will be skipped if the test runner doesn't use systemd.
I asked MS Copilot (GPT) to generate code for a dbus listener that would successfully reply to the Hello call that DBus uses and keep connections open. This ensures that dbus.NewSystemConnectionContext succeeds and conn.Connected() returns true afterwards. Basically, this emulates a real, successful systemd connection, except it replies to any other calls with UnknownMethod. I ran the tests on macOS to verify that they pass when systemd isn't available.
In the interest of time, I've pushed the code to this branch so you can also take a look. However, I've deliberately omitted the signoff trailer because the generated code is almost 800 lines and I haven't thoroughly reviewed the DBus implementation, nor do I want to without confirmation that it's worth keeping such a massive chunk of code around just for a unit test. Personally, I'd find this a bit excessive.
There is one more alternative approach: instead of creating a dummy socket in the Go code, we can just create a dummy DBus listener in the CI pipeline. This is a lot shorter, only adding about 50 lines and nothing that needs extra maintenance to the test. GPT suggests the following:
type TestBus struct {
cmd *exec.Cmd
address string
}
func NewTestBus(t *testing.T) *TestBus {
t.Helper()
ctx, cancel := context.WithCancel(context.Background())
cmd := exec.CommandContext(
ctx,
"dbus-daemon",
"--session",
"--nofork",
"--print-address=1",
)
stdout, err := cmd.StdoutPipe()
if err != nil {
cancel()
t.Fatal(err)
}
if err := cmd.Start(); err != nil {
cancel()
t.Fatal(err)
}
r := bufio.NewReader(stdout)
address, err := r.ReadString('\n')
if err != nil {
cancel()
_ = cmd.Wait()
t.Fatal(err)
}
bus := &TestBus{
cmd: cmd,
address: strings.TrimSpace(address),
}
t.Setenv("DBUS_SYSTEM_BUS_ADDRESS", bus.address)
t.Cleanup(func() {
cancel()
_ = cmd.Wait()
})
return bus
}This also works and is much shorter, but introduces a different dependency to the tests: this doesn't run on macOS anymore because there is no dbus-daemon. Of course, most systems on which the tests are likely to run will probably have this binary. But the same could be said about systemd itself. For example, Talos has neither systemd nor the dbus-daemon binary. I suppose it's slightly safer to assume that dbus-daemon is present since even systems without systemd might still have DBus for other purposes.
If I had to choose, I suppose I'd rather spawn a subshell and depend on an external binary to simulate the systemd DBus connection than add a partial DBus implementation to the test code. Let me know what you think.
There was a problem hiding this comment.
@Arc676 Just stepping back a bit — would it make sense to just remove TestManagerRace and TestCancelDbus? They exist because of the dbus connection leak concern, which you've established doesn't occur. I don't think it's worth all this machinery.
There was a problem hiding this comment.
I agree that the machinery is a bit overcomplicated. Do you think it's worth keeping some kind of test to ensure that the connection leak concern is unfounded? We've established that dbus handles the connection closure properly but is it enough to know this now and not continue testing for it in the future? I guess if we trust the dbus implementation to not change this particular detail, then we don't really need to go out of our way to verify it. I'm OK with just leaving in a comment addressing this issue and removing the tests.
| func TestCancelDbus(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| conn, err := dbus.NewSystemConnectionContext(ctx) |
Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
|
I would avoid keeping the ~800-line hand-written D-Bus server just for these tests. The production ownership/timeout logic is unchanged and my earlier leak concern is already resolved. For the remaining portability concern, I would prefer one of two smaller options:
I lean toward the first option because it keeps this PR focused on mig-parted's logic rather than maintaining a partial D-Bus implementation. |
6 participants
Motivation
Informally, this PR adds (partial) support for Talos. Closes #356.
More formally, this PR adds support for
systems with read-only filesystems andsystems that do not runsystemd.Description
The MIG manager assumes that it will be able to copy the
mig-partedbinary to the host and usesystemdto restart host-side GPU services. Neither of these is true for Talos, which is an immutable OS that doesn't runsystemd. Proper Talos support would introduce a dependency on the Talos API, but that is beyond the scope of this PR and likely falls beyond the scope of what this tool should support.This PR adds support for systems like Talos by introducing two new flags (both of which are required for Talos):1. Identifying the host as read-only: prevent the manager from attempting to copy data to the host2. Flagging the absence ofsystemd: tell the manager to skip allsystemdoperations that would otherwise cause the program to hang, since there would be no response on DBusThis PR includesnil-checks for thesystemdmanager that were not present before. In the original code, these checks are effectively unnecessary because this member is always initialized and the entire program blocks on this initialization ifsystemdis not present.Updated Description
In the interest of clarity for other users, here's an updated description of what this PR actually changes after all the discussion and how it relates to Talos.
This PR replaces the unconditional initialization of the
systemdmanager with a lazy initialization that includes a timeout. In a separate update to the GPU operator Helm chart, theWITH_SHUTDOWN_HOST_GPU_CLIENTSflag is decoupled from the flag indicating the presence of drivers on the host. The MIG manager already skipssystemdoperations whenWITH_SHUTDOWN_HOST_GPU_CLIENTS=false. Together, these two changes remove the hard dependency onsystemd, allowing the application to run on devices withoutsystemd.The decoupling of the flag also means that the MIG manager no longer tries to copy the
mig-partedbinary to the host just because host drivers are present. Note that this means thatWITH_SHUTDOWN_HOST_GPU_CLIENTSmust also be set tofalsewhen running on a read-only host. No changes in this PR are directly related to support for read-only systems.Improvements
This is the simplest possible solution to the problem described in the linked issue. All the MIG- and GPU-related operations work fine1 on Talos. We simply need to skip over the parts that can't work on Talos. The obvious alternatives or improvements over this PR are:
systemd, either by inspecting the running processes or by introducing a timeout on the DBus connection, and adjusting accordingly, instead of requiring a flagCaveats
This PR exists more for discussion than with the goal of being merged.These changes were made based on a very cursory reading and superficial understanding of the MIG manager. There is likely a cleaner and more elegant way to achieve this. However, I'll submit the patch as a proof-of-concept: by disabling the host-copies and allsystemdfeatures, the MIG manager works properly on Talos. This is, at least for us, an important starting point.Footnotes
CUDA validation yields
ERROR: init 250 result=11s. I haven't yet figured out what this means, but so far it hasn't impacted the use of the GPU. The GPU workloads still run fine, as does the CUDA validation pod. ↩