fix(monitor): skip dirs without underscore in Update instead of panicking - #2371
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesContainer directory validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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: 1
🤖 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/monitor/nvidia/cudevshr_test.go`:
- Around line 436-446: Update the malformed-name subtests “dir without
underscore in name is skipped” and the corresponding “uid” case to create
matching pods and valid cache files, using UIDs “nodashes” and “uid”
respectively. Ensure each test reaches the malformed metadata path, assert that
Update() succeeds, and retain the expectation that l.containers remains empty.
🪄 Autofix
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: 0e40b033-25f2-49df-b8cd-58c556704b24
📒 Files selected for processing (2)
pkg/monitor/nvidia/cudevshr.gopkg/monitor/nvidia/cudevshr_test.go
…king strings.Split(entry.Name(), "_")[1] in Update() assumes every directory under the hook path follows the <podUID>_<containerName> format. A stray directory without an underscore causes an index out of range panic, which crashes vGPUmonitor and silences all GPU metrics on that node. Replace both unguarded Split calls with a single SplitN(..., 2) whose result is validated before use; directories that do not match the expected format are now skipped with a warning. Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
01846ba to
206222f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, Nakshatra480 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thanks @archlitchi for merging this pr 😊 |
2 participants
Summary
Update()inpkg/monitor/nvidia/cudevshr.gocalledstrings.Split(entry.Name(), "_")[1]without checking that the split produced at least two parts. Any directory under the hook path whose name contains no underscore, a stray directory from another tool, a partial write, or operator error causes an index out of range panic that crashes vGPUmonitor and silences all GPU metrics on that node.pkg/monitor/nvidia/cudevshr.go: replaced bothstrings.Split(...)[idx]calls with a singlestrings.SplitN(..., "_", 2); directories that don't match the expected<podUID>_<containerName>format are skipped with a warning log instead of panickingpkg/monitor/nvidia/cudevshr_test.go: added"dir without underscore in name is skipped"regression case toTest_ContainerLister_UpdateTest_ContainerLister_Updatesubtests continue to passWhich issue(s) this PR fixes:
Part of #2126 (LFX observability hardening - vGPU monitor stability)
Special notes for your reviewer:
The
strings.Split(...)[0]on line 189 was safe (always produces at least one element), but the[1]on line 216 was not. The fix consolidates both into oneSplitNcall so the validation happens once before either index is used.Does this PR introduce a user-facing change?
Yes vGPUmonitor no longer panics when a directory without an underscore appears in the hook path; GPU metrics continue to be reported normally.
AI Disclosure:
AI assistance was used for code inspection and draft formatting; all logic, test cases, and verification were manually checked and validated.
Summary by CodeRabbit