fix(vGPUmonitor): bound v0 spec metric sums to active proc slots by imantaba · Pull Request #2282 · Project-HAMi/HAMi

imantaba · 2026-08-02T13:44:50Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

In pkg/monitor/nvidia/v0/spec.go, the six per-device metric accessors (DeviceMemoryContextSize, DeviceMemoryModuleSize, DeviceMemoryBufferSize, DeviceMemoryOffset, DeviceMemoryTotal and DeviceSmUtil) iterate over the entire fixed-size procs [1024]shrregProcSlotT array instead of only the live slots bounded by procnum:

for _, p := range s.sr.procs {          // iterates all 1024 slots
	v += p.used[idx].contextSize
}

procnum is the count of active process slots written by libvgpu.so. Slots at index >= procnum are not cleared and can retain stale, non-zero values from processes that have exited. Summing over all 1024 slots folds that stale data into the reported per-device memory and SM-utilization totals, inflating the corresponding vGPUmonitor gauges (and in the worst case pushing a value far above the real device usage/limit once a slot beyond procnum holds leftover data).

The v0 layout is reachable: pkg/monitor/nvidia/cudevshr.go selects it when the shared-region file size is 1197897.

The v1 layout already bounded these loops to s.sr.procs[:int(s.sr.procnum)] (fixed in acc8d91, #1345) but the fix was never backported to v0. This PR backports it to v0.

Changes

Bound all six v0 accessor loops to the active proc slots, mirroring v1.
Route both v0 and v1 through a shared-shape activeProcs() helper that clamps procnum into [0, len(procs)] before slicing. procnum comes straight from the mmap'd shared-memory region, so a corrupt value (negative or larger than the backing array) would otherwise panic with slice bounds out of range. This also hardens the pre-existing v1 code, which had the same exposure.
Update the existing spec tests to set procnum (they previously relied on all-zero trailing slots, so they would sum procs[:0] = 0 once the bound is applied).
Add a stale-slot regression case per accessor asserting that a non-zero slot beyond procnum is excluded from the sum, plus a clamp regression test (negative and oversized procnum) for both v0 and v1.

Which issue(s) this PR fixes:
Fixes #2281

Special notes for your reviewer:

Verified red→green:

With the loops still unbounded, the stale-slot cases return 600 instead of 200; after the fix all pass.
With activeProcs() slicing procnum directly (no clamp), the clamp test panics slice bounds out of range [:-5]; with the clamp it passes.

go test ./pkg/monitor/nvidia/... -race and gofmt/go vet are clean.

The clamp addresses review feedback on this PR (thanks @mesutoezdil) — folding it in here since it hardens the exact lines this PR touches, and applying it to v1 too keeps both layouts consistent.

This change was prepared with AI assistance (per CONTRIBUTING.md); all changes were reviewed and verified by me.

Does this PR introduce a user-facing change?:

Fix vGPUmonitor over-reporting device memory and SM utilization for the v0 shared-region layout, caused by counting stale process slots.

Summary by CodeRabbit

Bug Fixes
- Improved NVIDIA monitoring reliability when process-count data is invalid or corrupted.
- Device memory and SM utilization totals now ignore inactive process slots.
- Prevented monitoring errors and crashes caused by negative or oversized process counts.
Tests
- Added coverage for invalid process counts and excluded process slots across NVIDIA monitoring versions.

The v0 shared-region accessors (DeviceMemoryContextSize, ModuleSize, BufferSize, Offset, Total and DeviceSmUtil) iterated over the full procs[1024] array instead of only the live slots bounded by procnum. Slots beyond procnum can retain stale, non-zero data from exited processes, which was summed into the reported per-device memory and SM-utilization metrics and inflated the corresponding vGPUmonitor gauges. Bound every loop to procs[:int(procnum)], mirroring the v1 layout which was already fixed in acc8d91 but never backported to v0. Add regression cases asserting a stale slot beyond procnum is excluded. Fixes Project-HAMi#2281 Signed-off-by: imantaba <itn.taba@gmail.com>

coderabbitai · 2026-08-02T13:45:09Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b0f64c7d-d811-47bc-b51f-cf4eb835a5d6

📥 Commits

Reviewing files that changed from the base of the PR and between 515be96 and 925ab88.

📒 Files selected for processing (4)

pkg/monitor/nvidia/v0/spec.go
pkg/monitor/nvidia/v0/spec_test.go
pkg/monitor/nvidia/v1/spec.go
pkg/monitor/nvidia/v1/spec_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

pkg/monitor/nvidia/v0/spec.go
pkg/monitor/nvidia/v1/spec.go
pkg/monitor/nvidia/v1/spec_test.go
pkg/monitor/nvidia/v0/spec_test.go

📝 Walkthrough

Walkthrough

NVIDIA v0 and v1 specifications now clamp procnum to valid process slots before aggregating device memory and SM utilization. Tests verify that stale, negative, and oversized process counts produce bounded results without panics.

Changes

Active process metric aggregation

Layer / File(s)	Summary
Bound process ranges in v0 and v1 aggregations `pkg/monitor/nvidia/v0/spec.go`, `pkg/monitor/nvidia/v1/spec.go`	Both specifications use `activeProcs` to clamp `procnum`. Device memory and SM utilization accessors process only valid slots.
Validate stale and invalid process counts `pkg/monitor/nvidia/v0/spec_test.go`, `pkg/monitor/nvidia/v1/spec_test.go`	Tests initialize `procnum`, exclude stale slots, and verify safe results for negative and oversized values.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: archlitchi, ouyangluwei163, dsfans2014

Poem

A rabbit checks each active slot,
And leaves stale values where they rot.
Negative counts yield no sum,
Large counts stop where arrays end.
Memory and SM totals stay bounded.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The v1 helper and corruption tests extend beyond the linked issue, which targets stale slots in the v0 specification.	Move the v1 changes to a separate issue or provide linked-issue scope covering v1 procnum validation.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary v0 metric aggregation fix.
Linked Issues check	✅ Passed	The v0 accessors now bound all six metric sums to active process slots, and tests exclude stale slots as required by [`#2281`].

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-08-02T13:49:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	`64.30% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
pkg/monitor/nvidia/v0/spec.go	`95.58% <100.00%> (+0.20%)`	⬆️
pkg/monitor/nvidia/v1/spec.go	`95.58% <100.00%> (+0.20%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mesutoezdil

thx for pr

procnum is read from the mmap'd shared-memory region and may be corrupt (negative or larger than the backing procs array). Slicing it directly would panic with 'slice bounds out of range'. Route both v0 and v1 spec accessors through an activeProcs() helper that clamps procnum into [0, len(procs)] before slicing, and add regression tests covering a negative and an oversized procnum. Signed-off-by: imantaba <itn.taba@gmail.com>

mesutoezdil · 2026-08-03T10:57:27Z

/lgtm

archlitchi

/lgtm

archlitchi

/lgtm

hami-robot · 2026-08-04T04:20:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, imantaba

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [archlitchi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 2, 2026

hami-robot Bot requested review from DSFans2014 and archlitchi August 2, 2026 13:44

hami-robot Bot added the size/L label Aug 2, 2026

coderabbitai Bot requested a review from ouyangluwei163 August 2, 2026 13:45

imantaba temporarily deployed to nvidia August 2, 2026 13:53 — with GitHub Actions Inactive

mesutoezdil reviewed Aug 2, 2026

View reviewed changes

Comment thread pkg/monitor/nvidia/v0/spec.go Outdated

imantaba force-pushed the fix/monitor-v0-procnum-stale-slots branch from 515be96 to 925ab88 Compare August 2, 2026 14:30

imantaba temporarily deployed to nvidia August 2, 2026 14:39 — with GitHub Actions Inactive

hami-robot Bot assigned mesutoezdil Aug 3, 2026

hami-robot Bot added the lgtm label Aug 3, 2026

Nakshatra480 mentioned this pull request Aug 3, 2026

bug(vGPUmonitor): v1 spec aggregations ignore status field, inflating memory metrics for dead process slots #2310

Closed

This was referenced Aug 3, 2026

bug(vGPUmonitor): ListContainers races with Update — unlocked map iteration and possible use-after-munmap #2309

Closed

fix(vGPUmonitor): skip exited process slots in v1 spec metric aggregations #2314

Merged

Brijesh-Thakkar mentioned this pull request Aug 3, 2026

bug(vGPUmonitor): v0 spec sums all 1024 proc slots instead of bounding by procnum, inflating memory metrics #2327

Closed

coderabbitai Bot mentioned this pull request Aug 3, 2026

fix(vGPUmonitor): bound v0 spec aggregation loops by procnum #2328

Closed

archlitchi approved these changes Aug 4, 2026

View reviewed changes

hami-robot Bot assigned archlitchi Aug 4, 2026

hami-robot Bot added the approved label Aug 4, 2026

archlitchi approved these changes Aug 4, 2026

View reviewed changes

hami-robot Bot merged commit 8ddd431 into Project-HAMi:master Aug 4, 2026
17 checks passed

Nakshatra480 mentioned this pull request Aug 4, 2026

fix(monitor): clamp SetDeviceSmLimit and SetDeviceMemoryLimit to maxDevices #2362

Merged

coderabbitai Bot mentioned this pull request Aug 8, 2026

Child processes after fork() re initialize with zero limits when environment variables are stripped #2488

Closed

4 tasks

coderabbitai Bot mentioned this pull request Aug 9, 2026

feat(monitor): add DeviceProcessCount metric to expose tracked GPU process count #2505

Closed

coderabbitai Bot mentioned this pull request Aug 16, 2026

Elastic Compute Allocation for Idle Workloads #2693

Closed

Navigation Menu

Uh oh!

fix(vGPUmonitor): bound v0 spec metric sums to active proc slots - #2282

fix(vGPUmonitor): bound v0 spec metric sums to active proc slots#2282
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
imantaba:fix/monitor-v0-procnum-stale-slots

imantaba commented Aug 2, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

codecov Bot commented Aug 2, 2026 •

edited

Loading

Uh oh!

mesutoezdil left a comment •

edited

Loading

Uh oh!

Uh oh!

mesutoezdil commented Aug 3, 2026

Uh oh!

archlitchi left a comment

Uh oh!

archlitchi left a comment

Uh oh!

hami-robot Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

imantaba commented Aug 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

codecov Bot commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mesutoezdil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mesutoezdil commented Aug 3, 2026

Uh oh!

archlitchi left a comment

Choose a reason for hiding this comment

Uh oh!

archlitchi left a comment

Choose a reason for hiding this comment

Uh oh!

hami-robot Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imantaba commented Aug 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 2, 2026 •

edited

Loading

codecov Bot commented Aug 2, 2026 •

edited

Loading

mesutoezdil left a comment •

edited

Loading