refactor: replace NVML util panics with error returns - #2234
refactor: replace NVML util panics with error returns#2234shellyco-code wants to merge 1 commit into
Conversation
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shellyco-code The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
github-actions
Bot
added
kind/enhancement
📝 WalkthroughWalkthroughThe change replaces panic-based NVML lookup failures with returned errors. Container-device conversion logs these errors and skips affected devices or MIG UUIDs. ChangesNVML error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go`:
- Around line 461-465: Update GetContainerDeviceStrArray so conversion failures
from GetIndexAndTypeFromUUID return an error instead of continuing with a
shortened device list; preserve skipping behavior only for device discovery
paths. Propagate this error through Allocate before calling getAllocateResponse,
ensuring failed conversion does not remove the pending allocation annotation or
mark the allocation successful.
- Around line 198-200: Update the fallback path around GetMigUUIDFromSmiOutput
to return a non-nil error when the resolved MIG UUID is empty, instead of
returning empty success. Preserve the existing successful return for non-empty
UUIDs so callers use their current error path to log and skip the device.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8061e4d-447d-4627-8ddc-fa459597a22a
📒 Files selected for processing (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go
| outStr := stdout.String() | ||
| uuid := GetMigUUIDFromSmiOutput(outStr, originuuid, idx) | ||
| return uuid | ||
| return uuid, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return an error when the fallback does not find a MIG UUID.
GetMigUUIDFromSmiOutput returns "" when the requested MIG device is absent from the output. Lines 198-200 currently report that result as success. Line 505 then appends an empty device identifier.
Return a non-nil error when migUUID == "" so the existing error path logs and skips the device.
Proposed fix
- uuid := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
- return uuid, nil
+ migUUID := GetMigUUIDFromSmiOutput(outStr, originuuid, idx)
+ if migUUID == "" {
+ return "", fmt.Errorf("nvidia-smi -L returned no MIG UUID for GPU %q at index %d", originuuid, idx)
+ }
+ return migUUID, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| outStr := stdout.String() | |
| uuid := GetMigUUIDFromSmiOutput(outStr, originuuid, idx) | |
| return uuid | |
| return uuid, nil | |
| outStr := stdout.String() | |
| migUUID := GetMigUUIDFromSmiOutput(outStr, originuuid, idx) | |
| if migUUID == "" { | |
| return "", fmt.Errorf("nvidia-smi -L returned no MIG UUID for GPU %q at index %d", originuuid, idx) | |
| } | |
| return migUUID, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go` around lines 198 -
200, Update the fallback path around GetMigUUIDFromSmiOutput to return a non-nil
error when the resolved MIG UUID is empty, instead of returning empty success.
Preserve the existing successful return for non-empty UUIDs so callers use their
current error path to log and skip the device.
| devtype, devindex, err := GetIndexAndTypeFromUUID(val.UUID) | ||
| if err != nil { | ||
| klog.Errorf("failed to get index and type from UUID %s: %v", val.UUID, err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fail allocation when conversion drops a requested device.
These continue statements shorten tmp, but GetContainerDeviceStrArray cannot report that loss. Allocate passes this result to getAllocateResponse, then removes the pending allocation annotation and marks the allocation successful after a nil response error. It does not compare the converted-device count with devreq.
Return an error from GetContainerDeviceStrArray when any requested device cannot convert. Propagate that error through Allocate. Keep device discovery-time skipping separate from allocation-time failure handling.
Also applies to: 500-505
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go` around lines 461 -
465, Update GetContainerDeviceStrArray so conversion failures from
GetIndexAndTypeFromUUID return an error instead of continuing with a shortened
device list; preserve skipping behavior only for device discovery paths.
Propagate this error through Allocate before calling getAllocateResponse,
ensuring failed conversion does not remove the pending allocation annotation or
mark the allocation successful.
|
the fix direction is ok but skipping a device in allocate silently under-allocates also the pr does not touch register.go which is what #2230 actually describes. no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes a critical node-level reliability issue where the HAMi device plugin crashes via
panic(0)if it encounters any NVML query failures (e.g., driver glitches) in utility functions.Instead of panicking and crashing the plugin,
GetIndexAndTypeFromUUIDandGetMigUUIDFromIndexnow return anerror. The caller (GetContainerDeviceStrArray) logs the error viaklog.Errorfand safely skips the faulty device, allowing the plugin to continue operating with the remaining healthy devices on the node.Which issue(s) this PR fixes:
Fixes #2230
Special notes for your reviewer:
panic(0)calls withfmt.Errorf()returns and handled them at call sites.Signed-off-by) to the commit.Does this PR introduce a user-facing change?: