Repository navigation
Conversation
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to
|
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
cmd/nvidia-validator/main.go: 1
803-803: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCarry the host validation error into the retry warning.
With
WITH_WAIT=true, this warning formats the separate container-validation error.Driver.runValidationhas the hostnvidia-smiexecution error, but the retry path does not receive it. This omits the underlying host error required by issue#3085. Pass the host error into the container retry path and include both errors in the warning.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/gpu-operator/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
36d15131-7599-43d7-a71c-706a25d4eff3
📒 Files selected for processing: 1
cmd/nvidia-validator/main.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.
|
@coderabbitai Thanks. The host error is not lost: (measured with a fake This reply was drafted with AI assistance and reviewed by me. |
|
Tip For best results, initiate chat on the files or code changes. @fjvbn2003 The supplied log shows that ✏️ Learnings added
You are interacting with an AI system. |
| args := append([]string{"/host", nvidiaSMIPath}, nvidiaSMIArgs()...) | ||
|
|
||
| return runCommand(command, args, silent) | ||
| if err := runCommand(command, args, silent); err != nil { |
There was a problem hiding this comment.
Why is it being assumed that the error code was 18?
There was a problem hiding this comment.
Good question. The code doesn't assume 18: it wraps whatever runCommand returns. With a stub host nvidia-smi exiting 9, 12, 18 and 255, the log shows each status as is, and the new tests check 9 and 18 (below).
The 18 in the description first came from my reproduction (a stub nvidia-smi that prints the mismatch message and exits 18, picked because it is NVML_ERROR_LIB_RM_VERSION_MISMATCH), and the description didn't say so. You were right to ask. I then measured a real mismatch: on a host with kernel module 580.95.05, the 610.43.03 nvidia-smi and libnvidia-ml (extracted from the installer, not installed) print Failed to initialize NVML: Driver/library version mismatch and exit 18 in 10 of 10 runs; the unmodified host gives 0. I updated the description and the commit message to say which output is measured and which is reproduced.
I also reworded the code comment so it doesn't guess the cause.
(Measured and written with help from Claude (AI).)
When the host has nvidia-smi but the user-space driver libraries and the loaded kernel module differ, for example after the driver packages were upgraded without a reboot, nvidia-smi cannot initialize NVML. Measured on a host with kernel module 580.95.05 and the 610.43.03 nvidia-smi and libnvidia-ml (extracted from the installer, not installed): nvidia-smi prints "Failed to initialize NVML: Driver/library version mismatch" and exits 18, in 10 of 10 runs. With a stub host nvidia-smi that prints the same message and exits 18, the driver-validation init container logs: No pre-installed driver detected on the host: exit status 18 ... failed to validate the driver, retrying after 5 seconds\n and then retries the driver container path forever. Both lines point away from the real problem: a driver is installed on the host, and the retry warning never says what failed (it also prints a literal "\n"). - validateHostDriver: when nvidia-smi was found on the host but failed to run, return an error that says so and names the binary. The error keeps nvidia-smi's own exit status; the comment does not guess the cause. Checked with the stub exiting 9, 12, 18 and 255. - runValidation: log "No usable pre-installed driver detected" instead of "No pre-installed driver detected". - validateDriverContainer: include the error in the retry warning and drop the trailing "\n". No change in control flow. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: YoungjuKim <[email protected]>
d6e2be4 to
beeb9ff
Compare
Add regression tests for the error reporting changed in the previous commit. - validateHostDriver now delegates to validateHostDriverInRoot, which takes the host root and the command runner as arguments. Production still passes "/host" and runCommand, so behavior is unchanged. - TestValidateHostDriverReportsHostNvidiaSMIFailure (linux only, since resolveHostNvidiaSMI is linux only) puts an executable nvidia-smi in a fake host root and makes the runner fail with a real *exec.ExitError (status 9 and 18) or a plain error. It asserts that chroot is called with the resolved host path, that the error names '/usr/bin/nvidia-smi' and keeps the original error text, and that errors.Is/errors.As still reach the original error and exit status. - TestValidateDriverContainerRetryWarningIncludesError runs validateDriverContainer with a fake nvidia-smi that exits 18 once and then succeeds, and asserts that the single retry warning contains "exit status 18" and does not end with "\n". With the previous validateHostDriver body (return the runner error as is) and the previous retry warning, all four cases fail. Wrapping with %v instead of %w also fails the errors.Is/errors.As checks. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: YoungjuKim <[email protected]>
Description
When a node has a pre-installed driver (
driver.enabled=false) andnvidia-smion the host cannot talk to the driver, for example after the driver packages were upgraded without a reboot, thedriver-validationinit container logs the following. This output is from the reproduction described under Testing, not from a real node: the fake hostnvidia-smiexits with 18, which I picked because it isNVML_ERROR_LIB_RM_VERSION_MISMATCHinnvml.h. I later measured a real mismatch: on a host with kernel module 580.95.05, the 610.43.03 nvidia-smi and libnvidia-ml (extracted from the installer, not installed) printFailed to initialize NVML: Driver/library version mismatchand exit 18 in 10 of 10 runs, so the reproduction matches. The validator logs whatever exit status nvidia-smi returns.and stays in
Initforever. Both log lines point away from the problem. A driver is installed on the host, and the retry warning never says what failed (it also prints a literal\n). #1276 and #1687 show users stuck on the same retry line without a reason.This PR:
validateHostDriver: whennvidia-smiwas found on the host but failed, returnfound '<path>' on the host, but running it failed: <err>.runValidation: logNo usable pre-installed driver detected on the host: ...instead ofNo pre-installed driver detected ....validateDriverContainer: include the error in the retry warning and drop the trailing\n.Control flow does not change. Validation still falls through to the driver container path, which is still needed when the host has a stray
nvidia-smibut the driver comes from the driver container.This overlaps with #3034 on the retry warning line. If #3034 merges first, I will rebase and move the error into the warning in
waitForDriver.Checklist
make lint) (golangci-lint not available locally;gofmt -sandgo vetwithGOOS=linuxare clean)make validate-generated-assets) (no generated files changed)make validate-modules) (no module changes)validateHostDriverchroots into/host, so it is not unit-testable without a refactor)Testing
go test ./cmd/nvidia-validator/passes.nvidia-operator-validatorservice account./hostwas a fake root whose/usr/bin/nvidia-smiprintsFailed to initialize NVML: Driver/library version mismatchand exits with 18. The run usedWITH_WAIT=true,--component driver, and a 12s timeout, and I ran it once with themainbinary and once with the patched binary, both built from the same commit (CGO_ENABLED=0 GOOS=linux).Before (
main):After:
Prepared with help from Claude (AI), including the local checks described above.
Fixes #3085
🤖 Generated with Claude Code