Skip to content

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

Closed
Brijesh-Thakkar wants to merge 1 commit into
Project-HAMi:masterfrom
Brijesh-Thakkar:v0-spec-sums-all-1024-proc-slots-instead-of-bounding-by-procnum-inflating-memory-metrics
Closed

fix(vGPUmonitor): bound v0 spec aggregation loops by procnum#2328
Brijesh-Thakkar wants to merge 1 commit into
Project-HAMi:masterfrom
Brijesh-Thakkar:v0-spec-sums-all-1024-proc-slots-instead-of-bounding-by-procnum-inflating-memory-metrics

Conversation

@Brijesh-Thakkar

@Brijesh-Thakkar Brijesh-Thakkar commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
The six metric-aggregation methods in pkg/monitor/nvidia/v0/spec.go (DeviceMemoryContextSize,
DeviceMemoryModuleSize, DeviceMemoryBufferSize, DeviceMemoryOffset, DeviceMemoryTotal,
DeviceSmUtil) iterated over the full 1024-slot procs array instead of stopping at procnum.
Any slot beyond the active process count may retain stale, non-zeroed data from a previous CUDA
process, causing DeviceMemoryTotal() and the five related methods to inflate reported usage.
Inflated usage can trigger false throttling of live GPU workloads via the feedback loop in
feedback.go.

This PR adds an activeProcCount() helper that clamps procnum to [0, len(procs)] before
slicing. This is needed because procnum is read directly from raw mmap'd shared memory
(written by libvgpu.so inside the container) with no field-level validation upstream — the
existing file-size check only guarantees the buffer is the right total size, not that individual
field values are sane. A naive fix using procs[:int(procnum)] would trade the stale-data bug
for a new panic (slice bounds out of range) if procnum were ever corrupted to a negative value
or a value greater than 1024. The clamp makes out-of-range values degrade safely instead.

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

Note:

  • The v1 implementation (pkg/monitor/nvidia/v1/spec.go) already bounds these loops by
    procnum, but has the same unguarded-value issue — a corrupted procnum there would panic
    too. That's tracked separately under bug(vGPUmonitor): v1 spec aggregations ignore status field, inflating memory metrics for dead process slots #2310 (which covers a different, pre-existing v1 defect:
    a missing p.status guard). This PR intentionally does not touch v1/spec.go to keep scope
    minimal; flagging it here for whoever picks up bug(vGPUmonitor): v1 spec aggregations ignore status field, inflating memory metrics for dead process slots #2310.
  • Existing tests in spec_test.go had procnum implicitly left at its zero value, meaning they
    weren't actually exercising the bounded-iteration path before this fix — they'd have passed
    regardless of correctness. I added explicit procnum values to all affected existing cases,
    plus:
    • A new TestSpec_ActiveProcCount table covering 7 boundary values (zero, normal, exact
      capacity, negative, large negative, over-capacity, large over-capacity)
    • 6 "stale slots beyond procnum are excluded" regression cases (one per aggregation method)
    • 2 end-to-end clamp cases in TestSpec_DeviceMemoryTotal proving negative/overflow procnum
      values don't panic
  • go vet and golangci-lint both pass clean on the changed package; full v0 test suite
    (26 tests) passes with -race.
  • AI-assisted: this fix was identified by claude code - code review and then the coding part was done by claude but the methodolgy to solve it was not done by it, for that, I used gemini a bit to get better solution to it and a bit of chatgpt as well so i can get best solution

Does this PR introduce a user-facing change?:

Fix vGPUmonitor v0 spec: correctly bound process-slot aggregation by active process count 
instead of scanning all 1024 fixed slots, preventing inflated GPU memory/utilization metrics 
and false throttling caused by stale data in unused slots.

Summary by CodeRabbit

  • Bug Fixes

    • Improved NVIDIA monitoring accuracy by limiting process-based memory and utilization calculations to valid active processes.
    • Prevented stale process data from affecting reported metrics.
    • Added safeguards for negative or oversized process counts to avoid invalid calculations and runtime errors.
  • Tests

    • Expanded coverage for zero, negative, bounded, and oversized process counts.

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

The six metric-aggregation methods in pkg/monitor/nvidia/v0/spec.go
iterated over the full 1024-slot procs array instead of stopping at
procnum. Any slot beyond the active process count may retain stale,
non-zeroed data from a previous CUDA process, causing DeviceMemoryTotal
and the five related methods to inflate reported usage.

Added an activeProcCount() helper that clamps procnum to [0, len(procs)]
before slicing, since procnum is read from raw mmap'd shared memory with
no upstream validation and could otherwise be negative or >1024, causing
a panic rather than just wrong data.

The v1 implementation already bounds these loops by procnum but has the
same unguarded-value issue; that's tracked separately under #2310 and
intentionally left out of scope here.

Fixes #2327
Copilot AI review requested due to automatic review settings August 3, 2026 21:15
@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 3, 2026
@hami-robot
hami-robot Bot requested review from DSFans2014 and lengrongfu August 3, 2026 21:15
@hami-robot

hami-robot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Brijesh-Thakkar
Once this PR has been reviewed and has the lgtm label, please assign dsfans2014 for approval. For more information see the Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@hami-robot hami-robot Bot added the size/L label Aug 3, 2026
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

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: 251d29c7-a95d-4582-b591-46ab6d5031cc

📥 Commits

Reviewing files that changed from the base of the PR and between 0345bd8 and 82a7aee.

📒 Files selected for processing (2)
  • pkg/monitor/nvidia/v0/spec.go
  • pkg/monitor/nvidia/v0/spec_test.go

📝 Walkthrough

Walkthrough

The v0 NVIDIA monitor now clamps procnum to the process-array bounds. Device memory and SM utilization metrics aggregate only active process slots. Tests cover normal, stale, negative, and oversized process counts.

Changes

v0 process bounds

Layer / File(s) Summary
Bounded metric aggregation
pkg/monitor/nvidia/v0/spec.go
Adds activeProcCount and uses it when aggregating device memory and SM utilization metrics.
Aggregation validation
pkg/monitor/nvidia/v0/spec_test.go
Tests clamping, stale-slot exclusion, negative counts, and oversized counts without panics.

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

Possibly related issues

  • Issue 2281: Covers related v0 monitoring aggregation and process-slot handling.
  • Issue 2310: Concerns related monitoring aggregation behavior in v1, but uses a different inactive-slot condition.

Possibly related PRs

Suggested reviewers: copilot

Poem

A rabbit counts the slots in line,
And leaves stale data out of time.
Negative counts become zero,
Big counts stop at the array’s door.
Metrics hop safely, neat and bright.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 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 v0 aggregation fix and the procnum bound.
Linked Issues check ✅ Passed The changes implement the six bounded v0 aggregation methods and clamp invalid procnum values as required by issue #2327.
Out of Scope Changes check ✅ Passed The implementation and regression tests stay within issue #2327 scope and do not modify the out-of-scope v1 implementation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes vGPUmonitor’s NVIDIA v0 shared-memory spec metric aggregation to only consider active process slots (bounded by procnum) rather than iterating all 1024 fixed slots, preventing stale-slot data from inflating memory/utilization metrics and triggering false throttling.

Changes:

  • Add Spec.activeProcCount() to clamp procnum to [0, len(procs)] before slicing.
  • Update all six v0 aggregation methods to iterate over procs[:activeProcCount()].
  • Strengthen unit tests with explicit procnum values, boundary clamp coverage, and “stale slots excluded” regressions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/monitor/nvidia/v0/spec.go Clamp procnum and bound all aggregation loops to active proc slots to avoid stale-data inflation and slice panics.
pkg/monitor/nvidia/v0/spec_test.go Add boundary tests for clamping and regression coverage ensuring stale slots beyond procnum are excluded.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@archlitchi

Copy link
Copy Markdown
Member

Done in #2282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants