Skip to content

fix(monitor): clamp DeviceNum to maxDevices to prevent scrape-path panic - #2400

Merged
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
Nakshatra480:fix/clamp-device-num-getter
Aug 6, 2026
Merged

fix(monitor): clamp DeviceNum to maxDevices to prevent scrape-path panic#2400
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
Nakshatra480:fix/clamp-device-num-getter

Conversation

@Nakshatra480

@Nakshatra480 Nakshatra480 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

DeviceNum() in v0/spec.go and v1/spec.go returns int(s.sr.num) , an
unclamped raw read from the shared-memory region. collectContainerMetrics in
cmd/vGPUmonitor/metrics.go loops for i := range c.Info.DeviceNum() and
calls DeviceUUID(i) and DeviceMemoryLimit(i), which index into [16]uuid
and [16]uint64 arrays respectively. When sr.num exceeds 16 due to a
version mismatch or a partial libvgpu write, every Prometheus scrape causes an
index out of range panic that crashes vGPUmonitor and silences all GPU metrics
on that node.

  • pkg/monitor/nvidia/v0/spec.go: DeviceNum() now returns min(int(s.sr.num), maxDevices)
  • pkg/monitor/nvidia/v1/spec.go: same
  • pkg/monitor/nvidia/v0/spec_test.go: added "num larger than maxDevices is clamped" to TestSpec_DeviceNum
  • pkg/monitor/nvidia/v1/spec_test.go: added the same to Test_DeviceNum; also tightened existing table entries to inline form
  • All tests pass: go test ./pkg/monitor/nvidia/... -count=1

This applies the same min(_, maxDevices) guard already used in activeProcs()
(via #2282) and the setter loops (via #2362) to the getter that drives the
scrape loop.

Which issue(s) this PR fixes:
Part of #2126 (LFX observability hardening - vGPU monitor stability)

Special notes for your reviewer:
DeviceUUID(i) and DeviceMemoryLimit(i) are called immediately after the
DeviceNum() loop bound, both access fixed-size [16] arrays so i >= 16
is an out-of-bounds panic, not a silent wrong value.

Does this PR introduce a user-facing change?
Yes vGPUmonitor no longer panics when sr.num exceeds 16; per-device GPU
metrics continue to be reported correctly.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected GPU device count reporting when the detected count exceeds the supported maximum.
    • Device counts are now safely limited to the maximum supported value, preventing invalid or unexpectedly large counts in monitoring results.
    • Applied consistently across supported NVIDIA monitoring versions.
  • Tests

    • Added coverage to verify that excessive device counts are properly capped.

AI Disclosure:
AI assistance was used for code inspection and draft formatting.

@hami-robot
hami-robot Bot requested review from FouoF and lengrongfu August 6, 2026 07:22
@github-actions github-actions Bot added the kind/bug Something isn't working label Aug 6, 2026
@hami-robot hami-robot Bot added the size/S label Aug 6, 2026
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Spec.DeviceNum now caps shared-region device counts at maxDevices in NVIDIA v0 and v1. Tests cover oversized and high-bit counts.

Changes

NVIDIA device count bounds

Layer / File(s) Summary
Cap device counts and verify bounds
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
DeviceNum returns no more than maxDevices in both versions. Tests cover oversized and high-bit counts.

Estimated code review effort: 2 (Simple) | ~5 minutes

Possibly related PRs

  • Project-HAMi/HAMi#2362 — Modifies NVIDIA v0/v1 device-count handling and tests to clamp oversized shared-region counts.

Suggested reviewers: lengrongfu, fouof, dsfans2014

Poem

A rabbit checks each count,
High bits meet a sturdy mount.
V0 and v1 stay in line,
maxDevices marks the design.
Tests confirm the bound is sound.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the DeviceNum clamping fix and its purpose of preventing scrape-path panics.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/v0/spec.go`:
- Line 81: Clamp the uint64 device count before converting it to int in both
DeviceNum implementations: update pkg/monitor/nvidia/v0/spec.go lines 81-81 and
pkg/monitor/nvidia/v1/spec.go lines 95-95 to apply min against
uint64(maxDevices), then narrow the result to int.
🪄 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: 39b388cc-b553-4322-863c-f301c0ae1ade

📥 Commits

Reviewing files that changed from the base of the PR and between 96207fc and da80db8.

📒 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

Comment thread pkg/monitor/nvidia/v0/spec.go Outdated
DeviceNum() returns int(s.sr.num) — a raw read from the shared-memory
region. collectContainerMetrics loops over DeviceNum() and calls
DeviceUUID(i) and DeviceMemoryLimit(i), which index into [16]uuid and
[16]uint64 arrays. A corrupt sr.num field (version mismatch or partial
write) larger than 16 causes an out-of-bounds panic on every Prometheus
scrape, silencing all GPU metrics on that node.

Apply the same min(_, maxDevices) clamp already used in the setter loops
and activeProcs() to DeviceNum() in both v0 and v1.

Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
@Nakshatra480
Nakshatra480 force-pushed the fix/clamp-device-num-getter branch from da80db8 to b2cb1a2 Compare August 6, 2026 07:28
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

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.

@coderabbitai
coderabbitai Bot requested a review from DSFans2014 August 6, 2026 07:28
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 62.04% <100.00%> (+0.09%) ⬆️

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.45% <100.00%> (ø)
pkg/monitor/nvidia/v1/spec.go 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

@Eshiv-Pandey

Copy link
Copy Markdown
Contributor

@Nakshatra480 just add ai disclosure. you can take ref from contributing.md

@Nakshatra480

Copy link
Copy Markdown
Contributor Author

@Nakshatra480 just add ai disclosure. you can take ref from contributing.md

Done

@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 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 4db733c into Project-HAMi:master Aug 6, 2026
18 checks passed
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.

3 participants