fix(monitor): clamp SetDeviceSmLimit and SetDeviceMemoryLimit to maxDevices - #2362
Merged
hami-robot[bot] merged 1 commit intoAug 6, 2026
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe NVIDIA v0 and v1 device-limit setters now cap iteration at ChangesNVIDIA device-limit bounds
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/v1/spec_test.go`:
- Around line 743-749: Replace the invalid assert.DeepEqual call in the “num
larger than maxDevices does not panic” test with assert.Equal, matching the
gotest.tools/v3/assert API already imported by the file. Apply the same
correction to the other referenced assertion calls in this test file.
🪄 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: 955cbbff-40e6-4423-b341-10a8af13be11
📒 Files selected for processing (4)
pkg/monitor/nvidia/v0/spec.gopkg/monitor/nvidia/v0/spec_test.gopkg/monitor/nvidia/v1/spec.gopkg/monitor/nvidia/v1/spec_test.go
Contributor
|
@Nakshatra480 heads up, reviewers also require an AI disclosure. you can refer https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md |
Contributor
Author
|
I have updated the PR description to include the AI disclosure. |
…evices Both v0 and v1 setter loops used sr.num as the upper bound without clamping to the backing array size. A corrupt or unexpectedly large num field from shared memory would cause an index out-of-bounds panic, bringing down the vGPU monitor and silencing all GPU metrics. Cap the loop to min(sr.num, maxDevices) in all four setters, matching the same guard already applied to sr.procnum in activeProcs. Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
Nakshatra480
force-pushed
the
fix/clamp-device-limit-setter-index
branch
from
August 6, 2026 06:21
47a8a29 to
4b48c18
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. |
Contributor
|
[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 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Contributor
Author
|
@archlitchi Thanks of merging |
This was referenced Aug 6, 2026
om7057
added a commit
to om7057/HAMi
that referenced
this pull request
Aug 10, 2026
…Limit mesutoezdil caught this on Project-HAMi#2179: master's merge into this branch resolved a conflict between this branch's atomic conversion and Project-HAMi#2362 (merged on master, fixing a real scrape panic) by keeping only this branch's side, silently dropping Project-HAMi#2362's min(s.sr.num, maxDevices) clamp in both v0/spec.go and v1/spec.go. limit/smLimit are fixed [16]uint64 arrays, but the loop bound came straight from s.sr.num, an untrusted shared-memory field. Without the clamp, a num greater than 16 indexes past the array and panics, which is exactly what Project-HAMi#2362 fixed on master; the merge just needed both changes combined, not one replacing the other. Restored the clamp alongside the atomic stores this branch adds, and brought over Project-HAMi#2362's "num larger than maxDevices does not panic" subtests for both setters in v0 and v1, which the same conflict resolution had dropped along with the fix. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
om7057
added a commit
to om7057/HAMi
that referenced
this pull request
Aug 18, 2026
…Limit mesutoezdil caught this on Project-HAMi#2179: master's merge into this branch resolved a conflict between this branch's atomic conversion and Project-HAMi#2362 (merged on master, fixing a real scrape panic) by keeping only this branch's side, silently dropping Project-HAMi#2362's min(s.sr.num, maxDevices) clamp in both v0/spec.go and v1/spec.go. limit/smLimit are fixed [16]uint64 arrays, but the loop bound came straight from s.sr.num, an untrusted shared-memory field. Without the clamp, a num greater than 16 indexes past the array and panics, which is exactly what Project-HAMi#2362 fixed on master; the merge just needed both changes combined, not one replacing the other. Restored the clamp alongside the atomic stores this branch adds, and brought over Project-HAMi#2362's "num larger than maxDevices does not panic" subtests for both setters in v0 and v1, which the same conflict resolution had dropped along with the fix. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
om7057
added a commit
to om7057/HAMi
that referenced
this pull request
Aug 19, 2026
…Limit mesutoezdil caught this on Project-HAMi#2179: master's merge into this branch resolved a conflict between this branch's atomic conversion and Project-HAMi#2362 (merged on master, fixing a real scrape panic) by keeping only this branch's side, silently dropping Project-HAMi#2362's min(s.sr.num, maxDevices) clamp in both v0/spec.go and v1/spec.go. limit/smLimit are fixed [16]uint64 arrays, but the loop bound came straight from s.sr.num, an untrusted shared-memory field. Without the clamp, a num greater than 16 indexes past the array and panics, which is exactly what Project-HAMi#2362 fixed on master; the merge just needed both changes combined, not one replacing the other. Restored the clamp alongside the atomic stores this branch adds, and brought over Project-HAMi#2362's "num larger than maxDevices does not panic" subtests for both setters in v0 and v1, which the same conflict resolution had dropped along with the fix. Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com> Signed-off-by: om7057 <kulkarniom7057@gmail.com>
hami-robot Bot
pushed a commit
that referenced
this pull request
Aug 21, 2026
* fix: use atomic load/store for shared-region limit fields
DeviceMemoryLimit, SetDeviceMemoryLimit and SetDeviceSmLimit in
pkg/monitor/nvidia/{v0,v1} read and write limit/smLimit in
sharedRegionT with plain Go assignments. These are the same
shared-memory words HAMi-core's shared_region_t.limit/sm_limit map
onto, and the C side already guarantees every other field in that
struct is atomic (see the companion fix in
Project-HAMi/HAMi-core#238). These two were the only fields left as
plain reads/writes on either side.
Switches all three methods to sync/atomic.LoadUint64/StoreUint64 on
the same words. No change to sharedRegionT's field types or layout,
so the wire format is unaffected. Existing unit tests in
pkg/monitor/nvidia/{v0,v1} pass unmodified under -race.
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
* fix: read the shared num field once per Set call, not per iteration
SetDeviceSmLimit and SetDeviceMemoryLimit atomically store into
smLimit[idx]/limit[idx], but the loop condition re-read s.sr.num on
every iteration, so the store was atomic while its own loop bound was
not. Read s.sr.num once into a local before the loop in both v0 and
v1, so the whole function touches the shared region exactly once for
the count and atomically per element for the limit itself.
num/device_num is not declared _Atomic on the C side (it is written
once at shared-region creation and never changes afterward), so this
isn't adding a new atomic guarantee there was never a matching one to
add; it just stops re-reading the same shared word on every loop tick.
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
* fix: extend atomic access to recentKernel, utilizationSwitch, lastKernelTime
Folds the remaining shared-region fields from PR #2180 into this
branch, so the atomic-access fix covers all fields the C side declares
_Atomic, not just limit/smLimit.
GetRecentKernel/SetRecentKernel and GetUtilizationSwitch/
SetUtilizationSwitch now use atomic.LoadInt32/StoreInt32 in both
v0/spec.go and v1/spec.go. LastKernelTime in v1/spec.go now uses
atomic.LoadInt64 (v0 has no lastKernelTime field on the C side and
always returns 0, so it's left as is).
Added TestSpec_ConcurrentAtomicAccess (v0) and Test_ConcurrentAtomicAccess
(v1), each running 50 goroutines x 200 iterations against a shared
sharedRegionT to exercise the setters/getters concurrently under
-race.
Co-authored-by: shivv23 <shivamkumar0423@gmail.com>
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
Signed-off-by: om7057 <kulkarniom7057@gmail.com>
* fix: restore the maxDevices clamp on SetDeviceSmLimit/SetDeviceMemoryLimit
mesutoezdil caught this on #2179: master's merge into this branch
resolved a conflict between this branch's atomic conversion and #2362
(merged on master, fixing a real scrape panic) by keeping only this
branch's side, silently dropping #2362's min(s.sr.num, maxDevices)
clamp in both v0/spec.go and v1/spec.go.
limit/smLimit are fixed [16]uint64 arrays, but the loop bound came
straight from s.sr.num, an untrusted shared-memory field. Without the
clamp, a num greater than 16 indexes past the array and panics, which
is exactly what #2362 fixed on master; the merge just needed both
changes combined, not one replacing the other.
Restored the clamp alongside the atomic stores this branch adds, and
brought over #2362's "num larger than maxDevices does not panic"
subtests for both setters in v0 and v1, which the same conflict
resolution had dropped along with the fix.
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
Signed-off-by: om7057 <kulkarniom7057@gmail.com>
* fix: use range-over-int in the concurrent atomic access tests
golangci-lint's modernize check flagged the classic for i := 0; i < n;
i++ loops in TestSpec_ConcurrentAtomicAccess (v0) and
Test_ConcurrentAtomicAccess (v1) as convertible to range-over-int.
Converted both the outer goroutine-spawning loop and the inner
iteration loop in each, since the linter caught one of each pair and
the other was the same pattern.
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
Signed-off-by: om7057 <kulkarniom7057@gmail.com>
---------
Signed-off-by: Om Kulkarni <kulkarniom7057@gmail.com>
Signed-off-by: om7057 <kulkarniom7057@gmail.com>
Co-authored-by: shivv23 <shivamkumar0423@gmail.com>
Co-authored-by: limengxuan <mengxuan.li@dynamia.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SetDeviceSmLimitandSetDeviceMemoryLimitin bothpkg/monitor/nvidia/v0/spec.goandpkg/monitor/nvidia/v1/spec.goiterated up tosr.numwithout clamping to the backing array size.sr.limitandsr.smLimitare both[16]uint64, matchingmaxDevices = 16. Ifsr.numcomes in larger than 16 - due to shared-memory corruption or a version mismatch between libvgpu and the monitor, the write panics with an index out of bounds, bringing down the vGPU monitor and silencing all GPU metrics on that node.The fix caps the loop to
min(sr.num, maxDevices)in all four setters across v0 and v1, matching the same guard already applied tosr.procnuminactiveProcs()(introduced in #2282).Files changed:
pkg/monitor/nvidia/v0/spec.go:SetDeviceSmLimit,SetDeviceMemoryLimitpkg/monitor/nvidia/v1/spec.go:SetDeviceSmLimit,SetDeviceMemoryLimitpkg/monitor/nvidia/v0/spec_test.go: added"num larger than maxDevices does not panic"regression case to both setter testspkg/monitor/nvidia/v1/spec_test.go: sameWhich issue(s) this PR fixes:
Part of #2126 (LFX observability hardening - vGPU monitor stability)
Special notes for your reviewer:
sr.numis written by libvgpu at shared-memory initialisation and is read directly from the mmap'd region. It should always be ≤ 16, but the same assumption was made aboutsr.procnumbefore #2282. This applies the identical guard to the remaining unclamped loops for consistency and to prevent the monitor from going down on corrupt input.Does this PR introduce a user-facing change?
Yes - the vGPU monitor no longer panics if
sr.numexceedsmaxDevices, so all GPU metrics stay available instead of going dark.Summary by CodeRabbit
Bug Fixes
Tests
AI Disclosure:
AI assistance was used for code inspection and draft formatting. Changes and tests were manually validated.