Repository navigation
Conversation
The DaemonSet name used getSanitizedKernelVersion, which strips architecture and page size. The node selector and the image tag keep the raw kernel, so x86_64 and aarch64 updated one object. Keep those tokens in the name. After the new objects exist, delete a precompiled DaemonSet whose name is no longer desired. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // those tokens, but the node selector and the image tag keep the raw kernel, | ||
| // so kernels that sanitize together still need different objects. | ||
| func kernelSuffixForPrecompiledDaemonSet(kernelVersion string) string { | ||
| return strings.Trim(strings.ToLower(strings.ReplaceAll(kernelVersion, "_", ".")), ".") |
There was a problem hiding this comment.
🔴 Distinct kernels share one driver DaemonSet
When raw kernels differ by _ versus ., kernelSuffixForPrecompiledDaemonSet can assign both the same name. The last update replaces the first kernel's node selector and image, leaving its nodes without a driver.
Learn more
Precompiled driver DaemonSets use their names as object identities. The new suffix maps both underscores and literal dots to dots, but the node selector and image tag continue to use the raw release. A second raw kernel with the same rewritten suffix therefore updates the first DaemonSet rather than creating another, and map iteration determines which nodes get a driver. The old sanitizer stripped x86_64 but did not strip x86.64, so this particular pair previously had different names.
Example: Nodes with 5.14.0-427.37.1.el9_4.x86_64 and 5.14.0-427.37.1.el9.4.x86.64 produce the same new suffix. Only the last kernel's node selector remains; two DaemonSets were expected.
Recommended fix: Use an injective encoding of raw kernel releases for the DaemonSet name, or append a stable hash of the full raw release. Keep the result within Kubernetes name limits and use the identical name builder for creation and the desired set.
Was this helpful? React with 👍 or 👎 to provide feedback.
| err := n.client.List(ctx, list, client.MatchingLabels{ | ||
| precompiledIdentificationLabelKey: precompiledIdentificationLabelValue, | ||
| }) |
There was a problem hiding this comment.
🔴 Foreign driver DaemonSets are deleted
When another namespace has an active precompiled driver DaemonSet, deleteUndesiredPrecompiledDaemonsets can delete it despite different ownership. The cluster-wide list checks only its label and name prefix, removing a running driver unrelated to this policy.
Learn more
The cleanup list has no namespace restriction, and the later deletion uses only the precompiled label, driver name prefix, and desired names. The previous cleanup does not delete active DaemonSets, whereas this new pass deletes them regardless of status. An active DaemonSet belonging to another namespace or another owner can therefore lose its running pods during an ordinary reconciliation.
Example: A DaemonSet named nvidia-driver-daemonset-5.14-old-rhel9 in a second namespace has nvidia.com/precompiled=true and serves one node. A ClusterPolicy reconciliation whose kernel map lacks that name deletes it even though the DaemonSet belongs to the second namespace.
Recommended fix: Restrict deletion to DaemonSets in n.operatorNamespace with a controller owner reference identifying n.singleton (UID as well as kind). Make the desired-name comparison only within that owned set.
Was this helpful? React with 👍 or 👎 to provide feedback.
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: NVIDIA/gpu-operator
- Branch: main
- File: .coderabbit.yaml
- Commit: e154719
- Layers: Repository YAML
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
1e4f8677-07fe-4ac1-8759-b47bb2a1cc9c
-
controllers/object_controls.gocontrollers/object_controls_test.go
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
| list := &appsv1.DaemonSetList{} | ||
| require.NoError(t, clusterPolicyController.client.List(ctx, list, client.MatchingLabels{"app": "nvidia-driver-daemonset"})) | ||
| require.Len(t, list.Items, 2) |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
set -eu printf '%s\n' '--- changed files and relevant diff ---' git diff --stat e15471941743c5588e07fcc5f1415d5815e92b55 45945b54bebc6cc5dc274389f5ab8b9f72f8b7c2 git diff --unified=80 e15471941743c5588e07fcc5f1415d5815e92b55 45945b54bebc6cc5dc274389f5ab8b9f72f8b7c2 -- controllers/object_controls.go controllers/object_controls_test.go controllers || true printf '%s\n' '--- cleanup and precompiled references ---' rg -n -F --glob '*.go' -e 'PrecompiledDriverDaemonSets' -e 'precompiled' -e 'desired' -e 'Delete' controllers | head -n 300 printf '%s\n' '--- test names and cleanup assertions ---' rg -n --glob '*_test.go' -e 'DaemonSet|reconcil|cleanup|Delete|Ready|ready|kernel' controllers/object_controls_test.go controllers || true
Repository: NVIDIA/gpu-operator
Length of output: 41596
Script executed:
set -eu printf '%s\n' '--- precompiled reconciliation and cleanup ---' nl -ba controllers/object_controls.go | sed -n '4300,4445p' printf '%s\n' '--- DaemonSet readiness paths ---' rg -n -F --glob '*.go' -e 'func DaemonSet' -e 'return gpuv1.NotReady' -e 'overallState' controllers/object_controls.go nl -ba controllers/object_controls.go | sed -n '3900,4305p' printf '%s\n' '--- complete added test ---' nl -ba controllers/object_controls_test.go | sed -n '1935,2030p' printf '%s\n' '--- precompiled-related tests across controllers ---' rg -n --glob '*_test.go' -e 'precompiledDriverDaemonsets|PrecompiledDriverDaemonSets|precompiledIdentificationLabelKey|commonDriverDaemonsetName|kernelVersionMap|cleanupStalePrecompiledDaemonsets|deleteUndesiredPrecompiledDaemonsets' controllers || test "$?" -eq 1
Repository: NVIDIA/gpu-operator
Length of output: 32661
Script executed:
set -eu nl -ba controllers/object_controls.go | sed -n '4740,4810p' nl -ba controllers/object_controls_test.go | sed -n '640,715p'
Repository: NVIDIA/gpu-operator
Length of output: 6182
Script executed:
set -eu rg -n -F --glob '*.go' -e 'func (n ClusterPolicyController) cleanupUnusedDriverDaemonSets' -e 'cleanupUnusedDriverDaemonSets(' -e 'podCount' controllers/object_controls.go controllers/*_test.go nl -ba controllers/object_controls.go | sed -n '4550,4750p'
Repository: NVIDIA/gpu-operator
Length of output: 9249
Keep the active legacy DaemonSet until its replacement is ready.
precompiledDriverDaemonsets deletes non-desired DaemonSets when replacement reconciliation returns gpuv1.NotReady without an error. This can remove the active legacy DaemonSet before the replacement can serve its nodes.
Gate cleanup on overallState == gpuv1.Ready. Add a regression test that checks the legacy DaemonSet remains while the replacement is not ready, then is removed after readiness on a later reconciliation.
Suggested cleanup guard
-
- if len(errs) == 0 && len(desired) > 0 { + if len(errs) == 0 && overallState == gpuv1.Ready && len(desired) > 0 {
Source: Path instructions
|
Please respond to Devin's review comments. And also include #3099 within this PR. |
getNodePools keyed each precompiled pool on the sanitized kernel. That string drops x86_64, aarch64, and _64k, so an x86 node and an arm node, or a 4k node and a 64k node, became one pool. The selector and image tag then used only the first node's raw kernel. Key the pool map on the raw kernel as well, and sort equal names by that kernel. Nodes that share a raw kernel still share one pool. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
A kernel that uses '_' and one that already uses '.' in that spot were rewritten to the same DaemonSet name. A kernel that contains '_' now gets a short hash of the raw string. Kernels with no '_' keep the previous name. Cleanup lists only the operator namespace. One failed delete does not skip the remaining stale DaemonSets. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
SebTardif
changed the title
Done. The node-pool change from #3099 is on this branch, and that pull is closed. A kernel that uses |
Description
driver.usePrecompiled=trueon a ClusterPolicy builds the driver DaemonSet name withgetSanitizedKernelVersion. That helper stripsx86_64,aarch64, and_64k. The node selector and the precompiled image tag still use the rawfeature.node.kubernetes.io/kernel-version.full.Two kernels that differ only by architecture or page size share one name.
DaemonSetupdates that object, and the last loop iteration wins. The other kernel's nodes get no driver. Map iteration order is not stable, so the surviving selector can change on the next reconcile.This pull covers both paths.
On a ClusterPolicy,
driver.nvidiaDriverCRD.enableddefaults to false, sodriver.usePrecompiled=truebuilds these DaemonSets. The name keeps the architecture and page-size tokens._is rewritten to.. A kernel that contains_also gets a short hash of the raw string, sox86_64andx86.64do not share a name. Kernels with no_, such as5.4.0-generic, keep the previous name. After the new objects are created, a stale precompiled DaemonSet in the operator namespace is deleted. A DaemonSet in another namespace is left alone. One failed delete does not skip the rest.NVIDIADriver pools in
internal/state/nodepool.go(the change from #3099) key each pool on the raw kernel as well as the sanitized name. Nodes with the same raw kernel still share one pool.Fixes #3124
Fixes #3098
Checklist
make lint)make validate-generated-assets)make validate-modules)make validate-generated-assetsandmake validate-modulesdo not apply. This change does not touch API types or modules.Testing
Red, before the name change, on the real
ClusterPolicyController.step()path:The controller created
nvidia-driver-daemonset-5.14.0-427.37.1.el9.4-ubuntu22.04for5.14.0-427.37.1.el9_4.x86_64, then loggedDaemonSet is different, updatingfor5.14.0-427.37.1.el9_4.aarch64against that same name. One DaemonSet remained, with the arm64 node selector and the arm64 image. The test failed with 1 DaemonSet instead of 2.Green, after the change:
TestDriver,TestGetSanitizedKernelVersion, andTestDriverPrecompiledLibModulesUbuntu/Suseare in that package run. Those kernels (5.4.0-generic,5.4.135-generic) still produce two DaemonSets with the previous names.The node-pool change and the review follow-ups:
This is a controller test with the kernel strings already used by
TestGetSanitizedKernelVersion. It is not a live cluster capture. Those strings match a RHELuname -r, which Node Feature Discovery stores infeature.node.kubernetes.io/kernel-version.full.