Wait for the driver before starting GPU operands - #2881
rajathagasthya merged 1 commit into
Conversation
📝 WalkthroughWalkthroughThe operand Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to The production readiness gate is implemented, but its regression test can accept a bypassed gate, leaving future changes able to reintroduce startup during driver unavailability without test coverage. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
controllers/object_controls_test.go-1294-1295 (1)
1294-1295: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the readiness predicates are conjunctive.
If the manifest changes
&&to||, both assertions still pass. In that state, a staletoolkit-readyfile can admit an operand while the driver module is unloaded. Assert the combined predicate.Proposed fix
- require.Contains(t, args, "/run/nvidia/validations/toolkit-ready") - require.Contains(t, args, "grep -q '^nvidia ' /proc/modules", + require.Contains(t, args, + "[ -f /run/nvidia/validations/toolkit-ready ] && grep -q '^nvidia ' /proc/modules", "stale toolkit-ready must not admit an operand while the NVIDIA driver is unloaded")Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ddd8f033-2412-43db-a81b-450bbb244c99
📒 Files selected for processing (7)
assets/gpu-feature-discovery/0500_daemonset.yamlassets/state-dcgm-exporter/0800_daemonset.yamlassets/state-dcgm/0400_dcgm.ymlassets/state-device-plugin/0500_daemonset.yamlassets/state-mig-manager/0600_daemonset.yamlassets/state-mps-control-daemon/0400_daemonset.yamlcontrollers/object_controls_test.go
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
bcea658 to
037af50
Compare
|
/ok-to-test 037af50 |
|
@sylvesterkaczmarek Thanks for the contribution! Can you please squash your commits and make sure it's signed? |
037af50 to
41de0bd
Compare
|
Done. Squashed to one commit and re-signed it; the new head is 41de0bd. |
|
/ok-to-test 41de0bd |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
controllers/object_controls_test.go-1294-1294 (1)
1294-1294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the assertion reject bypassed readiness gates.
This substring check passes for
until true || [ -f /run/nvidia/validations/toolkit-ready ] && grep -q '^nvidia ' /proc/modules; do ...; done. That command exits the loop before either readiness condition is checked. Assert that the command starts with the requireduntil [ -f ... ] && grep ...condition and reject||bypasses.This prevents a regression that starts an NVIDIA-runtime operand while the driver is unloaded. As per path instructions, flag assertions that pass when the behavior under test is broken.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 450587da-3df3-4d82-95a4-45d352e365d2
📒 Files selected for processing (1)
controllers/object_controls_test.go
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| require.NotEmpty(t, args, "toolkit-validation init container not found") | ||
| require.Contains(t, args, "[ -f /run/nvidia/validations/toolkit-ready ] && grep -q '^nvidia ' /proc/modules", | ||
| "toolkit readiness and driver readiness must both be required before starting an operand") |
There was a problem hiding this comment.
🔍 Readiness behavior remains untested
The test only matches the added shell fragment. It never executes the gate, so unsupported driver checks and incorrect shell behavior can pass.
Was this helpful? React with 👍 or 👎 to provide feedback.
41de0bd to
a721f48
Compare
|
Updated the existing squashed commit for the WSL2 case found in review. Standard Linux still requires the live |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
a721f48 to
891d8e3
Compare
|
@rajathagasthya The |
|
/cherry-pick release-26.7 |
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Signed-off-by: Rajath Agasthya <ragasthya@nvidia.com>
891d8e3 to
f8a024d
Compare
|
/ok-to-test f8a024d |
|
🤖 Backport PR created for |
4 participants
Fixes #2874
Description
Require the live NVIDIA kernel module as well as
toolkit-readybefore creating GPU Operator operand containers that use the NVIDIA runtime. This prevents a stale validation file from admitting an operand while the driver is temporarily unloaded, when the runtime would create the container without driver libraries.The guard is applied consistently to GPU Feature Discovery, Device Plugin, DCGM, DCGM Exporter, MIG Manager, and MPS Control Daemon.
Checklist
make lint)make validate-generated-assets)make validate-modules)Testing
make unit-test- passedgo test ./controllers -run TestOperandToolkitReadinessWaitsForDriverModule -count=1- passedmake fmt- passedmake validate-modules- passedmake lintreports three existingSA4023findings incmd/nvidia-validator/main.go; the same three findings reproduce on pristinemain