fix(plugin): guard mig smi output parsing against malformed lines - #2246
fix(plugin): guard mig smi output parsing against malformed lines#2246AdityaShome wants to merge 5 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AdityaShome 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 |
|
Welcome @AdityaShome! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesMIG UUID parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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 |
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 175-179: Update GetMigUUIDFromSmiOutput to validate that the
parsed field is the UUID field and that its trimmed value is non-empty and
starts with “MIG-” before returning it; otherwise skip the malformed line and
continue scanning. Add a regression test covering a line with enough
colon-separated parts but no “(UUID” field, verifying it is ignored.
- Around line 164-167: Update the device-index parsing flow around strings.Split
and strconv.Atoi so non-numeric indexes are logged at a non-fatal level and
skipped instead of reaching klog.Fatal or terminating the process. Preserve
processing of valid device indexes, and add a regression test covering a line
such as “Device x” to verify parsing continues without exiting.
🪄 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: c87df672-830c-4c95-b0ab-66670bc8f0bd
📒 Files selected for processing (2)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/util_test.go
|
Signed-off-by: AdityaShome <shomeaditya65@gmail.com>
Signed-off-by: AdityaShome <shomeaditya65@gmail.com>
Signed-off-by: AdityaShome <shomeaditya65@gmail.com>
4d601fb to
6292517
Compare
|
Last 3 commits are just adding signoff by rebase. |
mesutoezdil
left a comment
There was a problem hiding this comment.
pls rename the title to the conventional format, sth like "fix(plugin): guard mig smi output parsing against malformed lines".
Signed-off-by: AdityaShome <shomeaditya65@gmail.com>
15116bf to
af6bbd1
Compare
|
It now skips appending and logs an error when the MIG UUID lookup returns empty. |
8b7d73c to
89319cd
Compare
|
Made |
|
/assign @mesutoezdil |
|
A small follow up, so it does not go stale. |
|
ok error propagation looks good now, getmiguuidfromindex returns err, getcontainerdevicestrarray passes it up, allocate fails hard instead of quietly under allocating. nice that u tested on real hw too. only thing left, pr conflicts w master now. #2378 refactored mig alloc in the same files a few days ago. pls rebase and make sure ur fix still applies clean on top. |
| klog.Infoln("inspecting", val) | ||
| num := strings.Split(val, "Device")[1] | ||
| num = strings.Split(num, ":")[0] | ||
| deviceParts := strings.Split(val, "Device") |
There was a problem hiding this comment.
getmiguuidfromsmioutput and getmiguuidfromindex are gone in master now. #2378 replaced this whole nvidia-smi fallback with nvml miginstancemanager. pls check if the bug still exists in new code.
There was a problem hiding this comment.
Great work on the new MIG design overall, while going through the code I found two smaller issues:
- gpuUUIDToIndex and nvmlBusyGPUs (mig_startup.go) call nvml.Init() with no matching nvml.Shutdown() unlike every other NVML entry point in the codebase. gpuUUIDToIndex runs on every Allocate() and every 5s via the reconciler, so this leaks indefinitely on nodes with active MIG allocations.
- migmgr.go's MigInstanceManager never calls nvml.Init() itself it silently relies on nvmlBusyGPUs() having already leaked one at startup. Fixing 1 by adding a proper Shutdown() would break this.
Planning two follow-up PRs for these.
|
This is being closed because it does not comply with the contribution guidelines. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
GetMigUUIDFromSmiOutput(nvidia-smi fallback for MIG UUID lookup) indexed intostrings.Splitresults with no bounds checking, so any MIG-block line not matching the exact expected shape caused anindex out of rangepanic instead of being skipped turning a recoverable NVML error into a device plugin crash. Same general pattern as the scheduler-side fix in #2088 (unguarded indexing while parsing a MIG index/UUID string) a different component and code path, not otherwise related.This PR bounds-checks both splits and skips the malformed line instead of indexing blindly.
Not raised issue as it is a small fix.
Special notes for your reviewer:
nvidia-smi -Linput missing the expected"Device"keyword), then fixed it. Regression tests added covering the happy path and both previously-panicking malformed cases.go build/go vet/go test -race. Diff: 10+/4- (fix) + 40+ (tests).Does this PR introduce a user-facing change?:
AI assistance disclosure: AI assistance was used to help trace the fallback path, reproduce the panic and suggest an initial fix. I manually investigated the issue, validated the root cause, refined and implemented the solution, verified the behavior, added the tests and reviewed all changes before opening this PR.
Summary by CodeRabbit
Bug Fixes
Tests