Skip to content

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

Merged
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
Nakshatra480:fix/v1-spec-status-check
Aug 6, 2026
Merged

fix(vGPUmonitor): skip exited process slots in v1 spec metric aggregations#2314
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
Nakshatra480:fix/v1-spec-status-check

Conversation

@Nakshatra480

@Nakshatra480 Nakshatra480 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The six per-device metric accessors in pkg/monitor/nvidia/v1/spec.go (DeviceMemoryContextSize, DeviceMemoryModuleSize, DeviceMemoryBufferSize, DeviceMemoryOffset, DeviceMemoryTotal, DeviceSmUtil) were iterating over procs[:procnum] without checking p.status, so slots belonging to processes that had already exited were still included in the sum.

  • When a CUDA process exits, libvgpu sets status = 0 on its slot but does not immediately zero out the memory fields. This meant the monitor was reporting inflated memory and utilization values until the slot got reused by the next process.

  • Added a procBound() helper that clamps procnum to [0, len(procs)] before slicing, since procnum comes directly from the mmap'd shared-memory region and a corrupt value would otherwise cause a slice-bounds panic.

  • Each aggregation function now skips slots where p.status == 0, so only live process slots contribute to the reported metrics.

  • Updated existing tests to set status: 1 on active slots, and added regression cases covering dead-slot exclusion, negative procnum, and oversized procnum for all six functions.

Which issue(s) this PR fixes:

Fixes #2310

Special notes for your reviewer:

The analogous v0 bug (iterating all 1024 slots with no procnum bound at all) is tracked separately in #2281. This PR only touches the v1 layout.

Does this PR introduce a user-facing change?

Yes — hami_vgpu_memory_used_bytes and related vGPUmonitor gauges will no longer be inflated after a CUDA process exits inside a container.

Summary by CodeRabbit

  • Bug Fixes

    • Improved NVIDIA monitoring accuracy by excluding inactive process slots from context, module, buffer, offset, total memory, and SM utilization calculations.
    • Added safeguards for invalid process counts, including negative and oversized values, while preserving valid active-process data.
  • Tests

    • Expanded coverage for inactive process slots and process-count boundary conditions across NVIDIA monitoring versions.
    • Corrected a buffer-size test case label.

@hami-robot

hami-robot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Welcome @Nakshatra480! It looks like this is your first PR to Project-HAMi/HAMi 🎉

@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: 662c56a2-54bb-4826-b659-830021283baa

📥 Commits

Reviewing files that changed from the base of the PR and between a9e8cb8 and 43067d5.

📒 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 (2)
  • pkg/monitor/nvidia/v1/spec.go
  • pkg/monitor/nvidia/v1/spec_test.go

📝 Walkthrough

Walkthrough

NVIDIA v0 and v1 monitor aggregations now exclude inactive process slots. Tests cover all affected metrics, active-slot fixtures, inactive slots, and invalid process counts.

Changes

NVIDIA process aggregation updates

Layer / File(s) Summary
NVIDIA v0 aggregation updates
pkg/monitor/nvidia/v0/spec.go, pkg/monitor/nvidia/v0/spec_test.go
The six v0 aggregation methods skip inactive slots. Tests cover active slots, inactive slots, bounded iteration, and corrupt procnum values.
NVIDIA v1 aggregation updates
pkg/monitor/nvidia/v1/spec.go, pkg/monitor/nvidia/v1/spec_test.go
The six v1 aggregation methods skip inactive slots during bounded iteration. Tests cover inactive slots, procnum boundaries, active fixtures, and corrupt counts.

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

Possibly related issues

Possibly related PRs

Suggested reviewers: ouyangluwei163, mesutoezdil, imantaba

Poem

A rabbit checks each process slot,
Skips inactive data on the spot.
It bounds the count from low to high,
While stale metrics pass it by.
Tests confirm the totals are right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The v0 implementation and tests are outside issue #2310, which explicitly limits scope to v1 and tracks v0 separately. Remove the v0 changes or link the dedicated v0 issue and update the PR scope and title.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary v1 change: excluding exited process slots from metric aggregation.
Linked Issues check ✅ Passed The PR implements all six v1 status checks required by issue #2310 and adds safe process-count bounds.
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.

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 61.10% <100.00%> (+0.24%) ⬆️

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 96.25% <100.00%> (+0.66%) ⬆️
pkg/monitor/nvidia/v1/spec.go 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mesutoezdil

mesutoezdil commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

v0/spec.go has same bug, as we see no status check and no activeprocs clamp either

dead proc slots still summed into totals.
v0 is live code, used for old cache size 1197897, see cudevshr.go line 279. why not fixed there too?

Both v0 (old 1197897-byte cache) and v1 aggregation functions iterated
activeProcs() without checking p.status, so slots whose CUDA process had
already exited were still included in memory and utilization totals.

Add a p.status == 0 guard inside every loop in both specs so only live
process slots contribute to the reported metrics.

Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
@Nakshatra480
Nakshatra480 force-pushed the fix/v1-spec-status-check branch from a9e8cb8 to 43067d5 Compare August 4, 2026 17:45
@Nakshatra480

Copy link
Copy Markdown
Contributor Author

I fixed it in v0 as well. I added the same status == 0 check to all 6 functions there too. Can you take another look when you get a chance?

@archlitchi

Copy link
Copy Markdown
Member

/assign

@archlitchi archlitchi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@hami-robot hami-robot Bot added the lgtm label Aug 6, 2026
@hami-robot

hami-robot Bot commented Aug 6, 2026

Copy link
Copy Markdown
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

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 approved label Aug 6, 2026
@hami-robot
hami-robot Bot merged commit 9182ba2 into Project-HAMi:master Aug 6, 2026
17 checks passed
@Nakshatra480

Copy link
Copy Markdown
Contributor Author

@archlitchi thanks for merging

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants