Support Read-Only, systemd-less Systems#382
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}| } | ||
|
|
||
| systemdManager, err := systemd.NewManager(ctx) | ||
| systemdManager, err := systemd.NewManager(ctx, time.Duration(opts.SystemdTimeout)*time.Second) |
There was a problem hiding this comment.
To expand on my earlier suggestion of lazy init of systemd manager:
- We could remove the unconditional init in this
New()method, move it to a cached getter method which all the callers that use systemd (shutdownHostGPUClients,hostStartSystemdServicesandpersistConfig) invoke. Each reconfiguration callsNew()anyway, so latency doesn't change. - On Talos,
shutdownHostGPUClientsandhostStartSystemdServicesare never invoked whenWithShutdownHostGPUClients = true.persistConfigonly runs ifHOST_MIG_MANAGER_STATE_FILEalready exists on the host (the default value is/etc/systemd/system/nvidia-mig-manager.service.d/override.confwhich is not applicable to Talos). - Right now, you're treating connection timeout as "systemd not present" and ignoring it. On systems that have systemd, connection timeouts should be hard failures and shouldn't be equated to systemd not being present.
I think the code could look something like this:
// Cached getter
func (r *Reconfigure) getSystemdManager() (*systemd.Manager, error) {
if r.systemdManager != nil {
return r.systemdManager, nil
}
m, err := systemd.NewManager(r.ctx) // connect timeout lives inside NewManager
if err != nil {
return nil, fmt.Errorf("failed to connect to systemd (if the host does not run systemd, set WITH_SHUTDOWN_HOST_GPU_CLIENTS=false): %w", err)
}
r.systemdManager = m
return m, nil
}
// Caller
func (r *Reconfigure) shutdownHostGPUClients() error {
m, err := r.getSystemdManager()
if err != nil {
return err
}
stoppedServices, err := m.StopSystemdServices(services)
...
}There was a problem hiding this comment.
Regarding point 3: this is true. Using the timeout was the simplest way to check for systemd without introducing much additional logic. However, it occurs to me that the systemd dependency is actually tied to features behind the WithShutdownHostGPUClients flag. This means that mig-parted doesn't need a systemd detection mechanism if we instead rely on the flag.
I've implemented the lazy-init as you suggested and removed the soft-fail for timeouts. I've kept the timeout check itself since this prevents the program from hanging in the event of an actual DBus connection issue. The new implementation thus assumes that WithShutdownHostGPUClients will be set only when systemd is running. Otherwise, the reconfiguration will always fail due to connection timeout. With the flags set properly, none of the systemd-dependent calls can be reached with the flag is disabled, so the initializer is never called.
As a side note for anyone using this patch on their own setup: you have to patch the operator manually to use this feature branch now.
Patch Details
The new changes make this build of mig-parted dependent on the decoupling of the flags in the chart. Until the chart issue is resolved, mig-parted can only be used in this form if the entrypoint script is patched in the operator image (which can be done using a volume mount and a custom ConfigMap).
Custom entrypoint:
apiVersion: v1
kind: ConfigMap
metadata:
name: nvidia-mig-manager-custom-entrypoint
data:
configmap.yaml: |
apiVersion: v1
kind: ConfigMap
metadata:
name: nvidia-mig-manager-entrypoint
namespace: "FILLED BY THE OPERATOR"
labels:
app: nvidia-mig-manager
data:
entrypoint.sh: |
#!/bin/sh
until [ -f /run/nvidia/validations/driver-ready ]
do
echo "waiting for the driver validations to be ready..."
sleep 5
done
set -o allexport
cat /run/nvidia/validations/driver-ready
. /run/nvidia/validations/driver-ready
# manually export additional envs required by mig-manager
export WITH_SHUTDOWN_HOST_GPU_CLIENTS=${WITH_SHUTDOWN_HOST_GPU_CLIENTS:-$IS_HOST_DRIVER}
echo "WITH_SHUTDOWN_HOST_GPU_CLIENTS=$WITH_SHUTDOWN_HOST_GPU_CLIENTS"
echo "Starting nvidia-mig-manager"
exec nvidia-mig-managerMounting into the operator (just showing the altered bits):
apiVersion: apps/v1
kind: Deployment
metadata:
name: gpu-operator
spec:
template:
spec:
volumes:
- name: mig-entrypoint
configMap:
name: nvidia-mig-manager-custom-entrypoint
containers:
- name: gpu-operator
volumeMounts:
- name: mig-entrypoint
mountPath: /opt/gpu-operator/state-mig-manager/0420_configmap.yaml
subPath: configmap.yamlAdd 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>
|
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. |
Signed-off-by: Alessandro Vinciguerra <alessandro.vinciguerra@postfinance.ch>
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.
| } | ||
|
|
||
| systemdManager, err := systemd.NewManager(ctx) | ||
| systemdManager, err := systemd.NewManager(ctx, time.Duration(opts.SystemdTimeout)*time.Second) |
There was a problem hiding this comment.
Regarding point 3: this is true. Using the timeout was the simplest way to check for systemd without introducing much additional logic. However, it occurs to me that the systemd dependency is actually tied to features behind the WithShutdownHostGPUClients flag. This means that mig-parted doesn't need a systemd detection mechanism if we instead rely on the flag.
I've implemented the lazy-init as you suggested and removed the soft-fail for timeouts. I've kept the timeout check itself since this prevents the program from hanging in the event of an actual DBus connection issue. The new implementation thus assumes that WithShutdownHostGPUClients will be set only when systemd is running. Otherwise, the reconfiguration will always fail due to connection timeout. With the flags set properly, none of the systemd-dependent calls can be reached with the flag is disabled, so the initializer is never called.
As a side note for anyone using this patch on their own setup: you have to patch the operator manually to use this feature branch now.
Patch Details
The new changes make this build of mig-parted dependent on the decoupling of the flags in the chart. Until the chart issue is resolved, mig-parted can only be used in this form if the entrypoint script is patched in the operator image (which can be done using a volume mount and a custom ConfigMap).
Custom entrypoint:
apiVersion: v1
kind: ConfigMap
metadata:
name: nvidia-mig-manager-custom-entrypoint
data:
configmap.yaml: |
apiVersion: v1
kind: ConfigMap
metadata:
name: nvidia-mig-manager-entrypoint
namespace: "FILLED BY THE OPERATOR"
labels:
app: nvidia-mig-manager
data:
entrypoint.sh: |
#!/bin/sh
until [ -f /run/nvidia/validations/driver-ready ]
do
echo "waiting for the driver validations to be ready..."
sleep 5
done
set -o allexport
cat /run/nvidia/validations/driver-ready
. /run/nvidia/validations/driver-ready
# manually export additional envs required by mig-manager
export WITH_SHUTDOWN_HOST_GPU_CLIENTS=${WITH_SHUTDOWN_HOST_GPU_CLIENTS:-$IS_HOST_DRIVER}
echo "WITH_SHUTDOWN_HOST_GPU_CLIENTS=$WITH_SHUTDOWN_HOST_GPU_CLIENTS"
echo "Starting nvidia-mig-manager"
exec nvidia-mig-managerMounting into the operator (just showing the altered bits):
apiVersion: apps/v1
kind: Deployment
metadata:
name: gpu-operator
spec:
template:
spec:
volumes:
- name: mig-entrypoint
configMap:
name: nvidia-mig-manager-custom-entrypoint
containers:
- name: gpu-operator
volumeMounts:
- name: mig-entrypoint
mountPath: /opt/gpu-operator/state-mig-manager/0420_configmap.yaml
subPath: configmap.yamlMove 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>
|
@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. |
Motivation
Informally, this PR adds (partial) support for Talos. Closes #356.
More formally, this PR adds support for systems with read-only filesystems and systems that do not run
systemd.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):
systemd: tell the manager to skip allsystemdoperations that would otherwise cause the program to hang, since there would be no response on DBusThis PR includes
nil-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.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 all
systemdfeatures, 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. ↩