fix(chart): Helm-managed GPU device plugin (client#564) - #712
fix(chart): Helm-managed GPU device plugin (client#564)#712shujaatTracebloc wants to merge 5 commits into
Conversation
…ubectl (client#564) The GPU device plugin was applied with an imperative `kubectl apply` of an upstream manifest fetched at install time, outside any Helm release. It was not release-tracked, so it lingered through `helm uninstall` and was re-created on every installer re-run, and it added a raw.githubusercontent.com network dependency to the install path. Render it as a Helm-managed DaemonSet (templates/gpu-device-plugin.yaml) gated on gpu.devicePlugin.enabled + vendor, in kube-system, with the manifest baked into the chart (no install-time download). The Linux installer sets the value from GPU_VENDOR in lockstep with the GPU request it writes for training jobs, and no longer applies the plugin imperatively; node verification moves to after the Helm install. A bounded, best-effort adoption shim labels/annotates a pre-existing unowned DaemonSet so a re-run after `helm uninstall` adopts it instead of failing with "exists and cannot be imported". Scope: the Windows non-WSL2 fallback keeps its imperative apply. That path must verify the node advertises a GPU BEFORE it writes the GPU request, which a Helm-managed resource (rolled out during Helm install) can't satisfy in a single install; the WSL2/CDI path deliberately doesn't use the NVML plugin at all. Chart.yaml version + appVersion bumped 1.9.39 -> 1.9.40 (chart-version-guard). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… edits (client#564) The Static-analysis job's supply-chain check (gen-manifest.sh --check) fails when a hashed sub-script changes without a manifest refresh. Re-hash the three edited scripts (install-k8s.sh, lib/gpu-plugins.sh, lib/install-client-helm.sh). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… --reuse-values (client#564) The Fleet auto-upgrade E2E upgrades from the last published release (no gpu block) with --reuse-values, which does NOT merge values.yaml defaults, so .Values.gpu was nil and .Values.gpu.devicePlugin.enabled panicked with a nil pointer. Access via parenthesised indexing ((.Values.gpu).devicePlugin) and default the per-vendor image/namespace sub-keys. Adds a helm-unittest case with gpu: null. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
# Conflicts: # scripts/manifest.sha256
…ull-secrets (client#564) Address Cursor Bugbot findings on the Helm-managed GPU device plugin: - HIGH: guard gpu values with the chart's `default dict` pattern so a `helm upgrade --reuse-values` from a pre-bump release (no gpu key) can never evaluate .enabled on nil. Go-template `and` is not short-circuit, so `and $dp $dp.enabled` still panicked; coalescing to dict removes it. - MEDIUM: route both vendor images through tracebloc.image with (dig "imageRegistry" "" $g) | default <vendor> so global.imageRegistry (#585 mirror/air-gap) re-homes them and an empty imageRegistry falls back to the vendor registry (was silently becoming docker.io); add imagePullSecrets + copy the pull secret into the plugin namespace (resource-monitor pattern) so mirrored/air-gapped hosts can pull. - Split images into registry/repository/tag/digest in values + schema; add unittests for digest pinning, mirror re-home, and pull-secret presence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 41a0f8c. Configure here.
| # (client#564) rolls out as part of the Helm release above, so confirm the node | ||
| # now advertises the GPU here rather than before Helm. verify_gpu no-ops for a | ||
| # CPU-only host (GPU_VENDOR neither nvidia nor amd). | ||
| verify_gpu |
There was a problem hiding this comment.
GPU verify races plugin rollout
Medium Severity
verify_gpu now always runs after Helm on GPU hosts, but helm upgrade --install does not --wait, and the old _gpu_rollout_gate (120s DaemonSet rollout) was removed. The node poll can expire while the plugin is still pulling, so operators get GPU may still be initializing on a healthy install, or a 90s wait plus that same warning when the plugin never becomes Ready instead of the previous CPU-mode guidance.
Additional Locations (2)
Triggered by project rule: Bugbot guide — tracebloc/client
Reviewed by Cursor Bugbot for commit 41a0f8c. Configure here.
| metadata: | ||
| labels: | ||
| name: nvidia-device-plugin-ds | ||
| {{- include "tracebloc.labels" . | nindent 8 }} |
There was a problem hiding this comment.
Chart bumps restart the GPU plugin
Medium Severity
The GPU DaemonSet pod template includes tracebloc.labels, which embed helm.sh/chart and app.kubernetes.io/version. Every client chart bump changes the pod template and rolling-restarts the device plugin. Helm ownership only needs those labels on the DaemonSet metadata; resource-monitor already keeps versioned labels off the pod template for this reason.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 41a0f8c. Configure here.
| # used to apply, so an installer re-run adopts an existing DaemonSet in place | ||
| # rather than orphaning it. | ||
| name: nvidia-device-plugin-daemonset | ||
| namespace: {{ $ns }} |
There was a problem hiding this comment.
GPU plugin can roll back auto-upgrades
High Severity
Once the installer sets gpu.devicePlugin.enabled, the plugin is a Helm-tracked DaemonSet. Auto-upgrade runs helm upgrade --atomic --wait, so an ImagePullBackOff or crash on this optional nvcr.io workload times out the wait and rolls back the entire client, blocking later chart versions including security fixes. Previously a failed plugin only continued in CPU mode.
Triggered by project rule: Bugbot guide — tracebloc/client
Reviewed by Cursor Bugbot for commit 41a0f8c. Configure here.
2 participants
Problem
The GPU device plugin was applied with an imperative
kubectl applyof an upstream manifest fetched at install time, outside any Helm release. It wasn't release-tracked, so it lingered throughhelm uninstalland was re-created on every installer re-run, and it added araw.githubusercontent.comnetwork dependency to the install path. (scripts/install-k8s.ps1:2490in the issue is stale; the live Linux apply wasscripts/lib/gpu-plugins.sh→deploy_gpu_device_plugin.)Fix
Render the plugin as a Helm-managed DaemonSet —
client/templates/gpu-device-plugin.yaml, gated ongpu.devicePlugin.enabled+vendor, inkube-system, with the manifest baked into the chart (no install-time download). It's reconciled on upgrade and removed onhelm uninstall. Nohelm.sh/resource-policy: keep— sharing the release lifecycle is the point.lib/install-client-helm.shsetsgpu.devicePlugin.{enabled,vendor}fromGPU_VENDOR, in lockstep with the GPU request it already writes for training jobs.install-k8s.shno longer applies the plugin imperatively before Helm;verify_gpumoves to after the Helm install (the plugin now rolls out with the release)._adopt_orphaned_gpu_device_plugin) labels/annotates a pre-existing, Helm-unowned DaemonSet so a re-run afterhelm uninstalladopts it in place instead of failing with "exists and cannot be imported" — exactly the issue's re-run scenario.Chart.yamlversion + appVersion bumped 1.9.39 → 1.9.40 (chart-version-guard).Scoping: Linux vs Windows
Linux requests a GPU purely on
GPU_VENDORdetection, so a chart-managed plugin works cleanly. Windows can't use it here: the non-WSL2 fallback must verify the node advertises a GPU before it writes the GPU request, and a Helm-managed resource only rolls out during Helm install — so that path keeps its imperative apply for now. The WSL2/CDI path deliberately never runs the NVML plugin. Defaultgpu.devicePlugin.enabled: falsemeans Windows renders no chart DaemonSet (no double-deploy).Tests
client/tests/gpu_device_plugin_test.yaml(default renders nothing; nvidia/amd render; namespace+image override; Helm-ownership label; fail-on-missing-vendor).scripts/tests/gpu-nvidia.batsretargeted from the removed imperative-apply assertions to the chart-managed invariants;install-client-helm.bats+3 for the adoption shim.helm lint --strict(all 4 platforms),helm templaterenders, bats suites all green. (4 unrelated helm-unittest suites fail only under local helm v4 / unittest 1.1.0 vs CI-pinned v3.15.4 / 0.5.2 — same ondevelop.)Follow-ups (not in this PR)
Confirm-GpuNodecan be reordered (verify-before-request). The chart template already supports it.rocm/k8s-device-plugin:latest) to a digest for air-gapped/reproducible installs.Closes #564
🤖 Generated with Claude Code
Note
Medium Risk
Changes cluster-level
kube-systemDaemonSets and install ordering for GPU hosts; migration shim and nil-safe templates reduce breakage, but upgrades on machines with old imperative plugins need the adoption path to succeed.Overview
Moves the GPU device plugin from imperative
kubectl apply(upstream manifest download) into the Helm chart so it is upgraded with the release and removed onhelm uninstall.Adds
gpu.devicePluginvalues (default off) and a new template that renders an NVIDIA or AMD DaemonSet inkube-system(or a configured namespace), with mirror/air-gap support viaglobal.imageRegistry, optional digest pins, andimagePullSecretsmirrored into the plugin namespace when using a private registry.The Linux installer now writes
gpu.devicePlugin.enabledandvendorfromGPU_VENDOR, runs_adopt_orphaned_gpu_device_pluginbefore Helm to label/annotate legacy DaemonSets for in-place adoption, and callsverify_gpuafter Helm instead of deploying the plugin in step (c).gpu-plugins.shdrops all deploy/rollout logic and only keeps node verification.Chart version 1.9.39 → 1.9.40; new helm-unittest suite and updated bats cover the migration invariants.
Reviewed by Cursor Bugbot for commit 41a0f8c. Bugbot is set up for automated code reviews on this repo. Configure here.