cpick-kv-computedomain-crd-sync-main - #2864
Conversation
getDriverAdditionalConfigs placed the lib-modules volume and mount inside the "if !cr.Spec.UsePrecompiledDrivers()" branch, guarded by a check for UsePrecompiledDrivers. That inner condition can never hold there, so an NVIDIADriver with usePrecompiled: true on sles or sl-micro rendered a driver pod without /run/host/lib/modules. Move the mount out to its own block after the non-precompiled branch. The comment above it goes as well, since the guard already says precompiled and SUSE and the volume literals already say host module tree. The equivalent code in the ClusterPolicy path sits before the early return for precompiled drivers and works as intended, so only the NVIDIADriver path was affected. That is also why this went unnoticed: the tests added with the mount only covered the ClusterPolicy path. Add a table-driven test that renders the DaemonSet and asserts the volume on the pod spec and the mount on nvidia-driver-ctr. Going through the render catches a template change that stopped threading AdditionalConfigs into the pod as well as the Go-side bug. Fixes #2776 Signed-off-by: Abrar Shivani <ashivani@nvidia.com> (cherry picked from commit 77d1673)
Signed-off-by: Karthikeyan Valliyurnatt <kvalliyurnat@nvidia.com> (cherry picked from commit bae2eb6)
[release-26.7] Fix state tracking when NVIDIA driver DaemonSet cleanup fails
[release-26.7] Mount host /lib/modules for precompiled drivers on SUSE
When devicePlugin.enabled is set to false in the ClusterPolicy, the nvidia-node-status-exporter still monitors the device_plugin_devices_total metric which reports 0 (since no device plugin pods are running). This triggers a false positive GPUOperatorNodeDeploymentFailed alert. Fix: The operator now injects a DEVICE_PLUGIN_ENABLED env var into the node-status-exporter daemonset based on the ClusterPolicy. When set to "false", the exporter skips device plugin validation entirely, so the metric is never emitted and the alert does not fire. Fixes: #2237 Signed-off-by: Harshal Patil <12152047+harche@users.noreply.github.com> (cherry picked from commit 9b079ef)
[release-26.7] Skip device plugin alert when devicePlugin is disabled in ClusterPolicy
Signed-off-by: dentinyhao <dentinyhao@gmail.com> (cherry picked from commit 62de6b5)
[release-26.7] Stop defaulting deprecated cdi.default in ClusterPolicy
(cherry picked from commit bf1ce04) Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com> Co-authored-by: Tariq Ibrahim <tibrahim@nvidia.com>
(cherry picked from commit 08c40bc) Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
📝 WalkthroughWalkthroughThe change propagates device-plugin enablement to the node status exporter. Disabled plugins skip validation and report Merge Risk: 🟡 Moderate · up to Disabled device-plugin configurations can incorrectly validate and alert when user environment settings override the controller value, and cudaNvSci image builds can fail after required CMake modules are omitted. The affected runtime and transition behavior also needs direct regression coverage before merge. ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
![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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
tests/e2e/suites/clusterpolicy_test.go-346-347 (1)
346-347: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for the disabled validator behavior.
This assertion only checks the DaemonSet template. It still passes if
NodeMetrics.RunignoresDEVICE_PLUGIN_ENABLED, startswatchDevicePluginValidation, or leavesgpu_operator_node_device_plugin_devices_totalat0. Add regression coverage that asserts the disabled path reports-1and does not start device-plugin validation.As per path instructions, “Flag assertions that would still pass if the behavior under test were broken” and “new operand or CRD behaviour that ships with no regression test.”
Source: Path instructions
tests/e2e/suites/clusterpolicy_test.go-359-364 (1)
359-364: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise a false-to-true transition before asserting re-enable behavior.
If the device plugin is already enabled,
EnableDevicePluginis a no-op. This test then passes even if reconciliation cannot replace an existingDEVICE_PLUGIN_ENABLED=falsevalue. Disable the device plugin and observefalsebefore enabling it and assertingtrue.As per path instructions, “Flag assertions that would still pass if the behavior under test were broken.”
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: f208dde7-53ab-43c1-b263-2011f3a8d00c
⛔ Files ignored due to path filters (5)
bundle/manifests/nvidia.com_clusterpolicies.yamlis excluded by!bundle/manifests/nvidia.com_*.yamlbundle/manifests/resource.nvidia.com_computedomains.yamlis excluded by!bundle/manifests/resource.nvidia.com_*.yamlconfig/crd/bases/nvidia.com_clusterpolicies.yamlis excluded by!config/crd/bases/**deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yamlis excluded by!deployments/gpu-operator/crds/**deployments/gpu-operator/crds/resource.nvidia.com_computedomains.yamlis excluded by!deployments/gpu-operator/crds/**
📒 Files selected for processing (12)
api/nvidia/v1/clusterpolicy_types.goassets/state-node-status-exporter/0800_prometheus_rule_openshift.yamlcmd/nvidia-validator/metrics.gocontrollers/object_controls.gocontrollers/state_manager.gocontrollers/state_manager_test.gocontrollers/transforms_test.godocker/Dockerfileinternal/state/driver_test.gointernal/state/driver_volumes.gotests/e2e/helpers/clusterpolicy.gotests/e2e/suites/clusterpolicy_test.go
💤 Files with no reviewable changes (1)
- api/nvidia/v1/clusterpolicy_types.go
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| if !config.DevicePlugin.IsEnabled() { | ||
| devicePluginEnabled = "false" | ||
| } | ||
| setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), DevicePluginEnabledEnvName, devicePluginEnabled) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent NodeStatusExporter.Env from overriding DEVICE_PLUGIN_ENABLED.
NodeStatusExporter.Env is applied after this assignment. A ClusterPolicy that sets DEVICE_PLUGIN_ENABLED=true overrides the controller value when DevicePlugin.Enabled=false. The validator then starts device-plugin validation and can emit the false alert condition. Apply this reserved variable after user environment variables, or reject the reserved name.
Proposed fix
- setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), DevicePluginEnabledEnvName, devicePluginEnabled)
-
// set/append environment variables for exporter container
if len(config.NodeStatusExporter.Env) > 0 {
for _, env := range config.NodeStatusExporter.Env {
setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), env.Name, env.Value)
}
}
+ setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), DevicePluginEnabledEnvName, devicePluginEnabled)|
|
||
| RUN curl -L https://codeload.github.com/NVIDIA/cuda-samples/tar.gz/refs/tags/v${CUDA_SAMPLES_VERSION} | \ | ||
| tar -xzvf - --strip-components=1 --wildcards */${SAMPLE_NAME}/* --wildcards */Common/* --wildcards */cmake/* && \ | ||
| tar -xzvf - --strip-components=1 --wildcards */${SAMPLE_NAME}/* --wildcards */Common/* && \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge NVIDIA/gpu-operator /tmp/coderabbit-repo-knowledge/nvidia-gpu-operator-eed98df4/conventions
Length of output: 1663
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- docker/Dockerfile (relevant range) ---'
nl -ba docker/Dockerfile | sed -n '1,90p'
printf '%s\n' '--- relevant diff ---'
git diff -- docker/Dockerfile
printf '%s\n' '--- archive-related references ---'
rg -n -C 3 'SAMPLE_NAME|cmake/Modules|cudaNvSci|cuda-samples' docker .github Makefile README.md 2>/dev/null || true
printf '%s\n' '--- upstream v12.9 sample contract ---'
curl -fsSL https://raw.githubusercontent.com/NVIDIA/cuda-samples/v12.9/Samples/4_CUDA_Libraries/cudaNvSci/CMakeLists.txt | sed -n '1,180p'
printf '%s\n' '--- upstream v12.9 module paths ---'
curl -fsSL https://api.github.com/repos/NVIDIA/cuda-samples/git/trees/v12.9?recursive=1 |
jq -r '.tree[].path' | grep -E '(^|/)(cmake/Modules/FindNVSCI\.cmake|Samples/4_CUDA_Libraries/cudaNvSci/CMakeLists\.txt)$'Repository: NVIDIA/gpu-operator
Length of output: 3538
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- docker/Dockerfile ---'
sed -n '1,82p' docker/Dockerfile
printf '%s\n' '--- exact diff ---'
git diff --no-ext-diff --unified=12 -- docker/Dockerfile
printf '%s\n' '--- v12.9 archive paths used by the extraction ---'
curl -fsSL https://codeload.github.com/NVIDIA/cuda-samples/tar.gz/refs/tags/v12.9 |
tar -tzf - |
grep -E '(^|/)(Samples/4_CUDA_Libraries/cudaNvSci/CMakeLists\.txt|Common/|cmake/Modules/FindNVSCI\.cmake)' |
head -20Repository: NVIDIA/gpu-operator
Length of output: 4053
Preserve cmake/Modules for cudaNvSci.
When SAMPLE_NAME=cudaNvSci, its CMakeLists.txt loads FindNVSCI.cmake from cmake/Modules. The extraction omits this file, so CMake can omit the target and the later cp ${SAMPLE_NAME} command can fail. Restore */cmake/* extraction or restrict supported SAMPLE_NAME values, and test each value.
7 participants
Description
Checklist
make lint)make validate-generated-assets)make validate-modules)Testing