fix: remove panic in calculateGPUPairScore - #2229
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 |
📝 WalkthroughWalkthroughThe NVIDIA GPU pair scorer now documents its heuristic, logs asymmetric P2P link data with both GPU UUIDs, and returns a zero score instead of panicking. A regression test covers both score directions. ChangesGPU P2P resilience
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
pkg/device/nvidia/calculate_score.go (1)
216-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the P2P mismatch branch.
calculateGPUPairScorereturns0when one GPU has fewer P2P links to another than vice versa, butpkg/device/nvidia/calculate_score_test.goonly defines symmetric link maps. Add a case with different bidirectional link counts and assert that the pair score is0without panicking.🤖 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/nvidia/calculate_score.go` around lines 216 - 217, Add a regression test in calculate_score_test.go targeting calculateGPUPairScore with asymmetric bidirectional P2P link counts, asserting it returns 0 and does not panic. Keep the existing symmetric-link cases unchanged and construct the smallest GPU/link-map setup that reaches the “P2PLinks mismatch” branch.
🤖 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.
Nitpick comments:
In `@pkg/device/nvidia/calculate_score.go`:
- Around line 216-217: Add a regression test in calculate_score_test.go
targeting calculateGPUPairScore with asymmetric bidirectional P2P link counts,
asserting it returns 0 and does not panic. Keep the existing symmetric-link
cases unchanged and construct the smallest GPU/link-map setup that reaches the
“P2PLinks mismatch” branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23fdd13b-9187-4246-8390-a87b5c654fc2
📒 Files selected for processing (1)
pkg/device/nvidia/calculate_score.go
There was a problem hiding this comment.
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
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/nvidia/calculate_score_test.go (1)
109-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the P2P-link mismatch logger.
When
asymmetric linkshits the mismatch branch,calculateGPUPairScorecallsklog.ErrorSwith"gpu0"and"gpu1". If that structured logging is required, add a logger assertion for both directions so a future change cannot silently remove the diagnostic.🤖 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/nvidia/calculate_score_test.go` around lines 109 - 140, Update the “asymmetric links” test around calculateGPUPairScore to assert the mismatch diagnostic emitted by klog.ErrorS, verifying both gpu0 and gpu1 are included for each direction. Use the test’s existing logging-capture or assertion mechanism, preserving the current score expectations.
🤖 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.
Nitpick comments:
In `@pkg/device/nvidia/calculate_score_test.go`:
- Around line 109-140: Update the “asymmetric links” test around
calculateGPUPairScore to assert the mismatch diagnostic emitted by klog.ErrorS,
verifying both gpu0 and gpu1 are included for each direction. Use the test’s
existing logging-capture or assertion mechanism, preserving the current score
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47cc7064-af7b-43ca-b1d3-bf4bcf47972c
📒 Files selected for processing (1)
pkg/device/nvidia/calculate_score_test.go
|
@mesutoezdil yo TAL |
|
the PR description only mentions go build ./ passing, can you also run make verify and make test locally just so we know CI won't complain? |
4 participants
What does this PR do?
Fixes a critical reliability issue where the HAMi scheduler pod crashes via panic() if it encounters asymmetrical P2P links between two GPUs (which can happen due to hardware topology issues or driver glitches).
Instead of panicking and crashing the entire scheduler, calculateGPUPairScore now logs the error via klog.ErrorS with the affected GPU UUIDs and gracefully returns a score of 0. This allows the best-effort GPU allocator to continue operating safely with the remaining devices.
Which issue(s) this PR fixes:
Fixes #2228
Testing
✅ Verified go build ./... passes.
✅ Ensured that asymmetrical link conditions result in an error log rather than a process crash.
✅ Added DCO sign-off.
PR Acceptance Criteria
Commits are signed with Signed-off-by (DCO).
Code is properly formatted.
Resolves a crash vulnerability in the scheduler.
This PR was created with the assistance of an AI coding agent
Summary by CodeRabbit