Skip to content

fix(vGPUmonitor): skip GPU utilization gracefully when not supported - #2619

Closed
Nakshatra480 wants to merge 1 commit into
Project-HAMi:masterfrom
Nakshatra480:fix/skip-utilization-not-supported
Closed

fix(vGPUmonitor): skip GPU utilization gracefully when not supported#2619
Nakshatra480 wants to merge 1 commit into
Project-HAMi:masterfrom
Nakshatra480:fix/skip-utilization-not-supported

Conversation

@Nakshatra480

@Nakshatra480 Nakshatra480 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • collectGPUMemoryMetrics already returns nil on nvml.ERROR_NOT_SUPPORTED (unified memory architectures, Jetson, certain MIG configurations). collectGPUUtilizationMetrics treated the same NVML return code as a hard error.
  • When this happened, collectGPUDeviceMetrics returned an error after memory metrics had already been emitted to the channel, producing a misleading "Failed to collect metrics for GPU device N" log even though memory data was fine.
  • Added the same ERROR_NOT_SUPPORTED guard that collectGPUMemoryMetrics already uses.

Files changed:

  • cmd/vGPUmonitor/metrics.go: four-line guard matching the existing memory pattern

Tested: go test ./cmd/vGPUmonitor/... -count=1 passes. The NVML path requires hardware; the guard mirrors an already-tested pattern from collectGPUMemoryMetrics.

Special notes for your reviewer:
The only change is mirroring the if nvret == nvml.ERROR_NOT_SUPPORTED { return nil } guard from the memory function. No logic change for the success path.

Does this PR introduce a user-facing change?
Yes devices that previously triggered a spurious error log now silently skip utilization (with a klog.V(3) message) and continue emitting memory metrics.

Summary by CodeRabbit

  • Bug Fixes
    • GPU monitoring now continues gracefully when utilization metrics aren’t supported by a device.
    • Other GPU driver errors continue to be handled as failures.
  • Tests
    • Added coverage for unsupported metrics, driver errors, and successful utilization collection.

@hami-robot

hami-robot Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Nakshatra480
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/M label Aug 12, 2026
@coderabbitai

coderabbitai Bot commented Aug 12, 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: fcc336d1-67a0-4a3c-87cb-c273f9fef2a0

📥 Commits

Reviewing files that changed from the base of the PR and between fd85797 and 9f01fff.

📒 Files selected for processing (1)
  • cmd/vGPUmonitor/metrics_gpu_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/vGPUmonitor/metrics_gpu_test.go

📝 Walkthrough

Walkthrough

The GPU monitor treats nvml.ERROR_NOT_SUPPORTED as non-fatal during utilization collection. It logs the condition and skips the metric. Tests cover unsupported responses, other NVML errors, and successful collection.

Changes

GPU utilization handling

Layer / File(s) Summary
Unsupported utilization handling and validation
cmd/vGPUmonitor/metrics.go, cmd/vGPUmonitor/metrics_gpu_test.go
Unsupported NVML utilization responses return without an error or metric. Other NVML errors still propagate. Successful collection emits one metric with value 75.

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

Possibly related PRs

Suggested reviewers: archlitchi

Poem

I’m a rabbit with metrics to spare,
Unsupported rates vanish in air.
Errors still pass through,
Success shows seventy-five too,
While tests keep the results clear.

🚥 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 and concisely describes the main change: gracefully skipping unsupported GPU utilization collection.
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.
✨ 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 `@cmd/vGPUmonitor/metrics_gpu_test.go`:
- Around line 83-84: Update the test around sendLegacyMetric to isolate the
package-global legacyHostGPUUtilizationdesc state before asserting the metric
count, or ensure legacy emission is gated by LegacyMetrics. Preserve the
expectation of exactly one metric when legacy metrics are disabled and prevent
other tests’ descriptor initialization from affecting this collector.
🪄 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: f309f563-26cd-4b0c-865b-7719c7744e87

📥 Commits

Reviewing files that changed from the base of the PR and between e34913f and fd85797.

📒 Files selected for processing (2)
  • cmd/vGPUmonitor/metrics.go
  • cmd/vGPUmonitor/metrics_gpu_test.go

Comment thread cmd/vGPUmonitor/metrics_gpu_test.go
@github-actions github-actions Bot added the kind/bug Something isn't working label Aug 12, 2026
collectGPUMemoryMetrics already returns nil on nvml.ERROR_NOT_SUPPORTED
(unified memory devices, Jetson, MIG configurations). collectGPUUtilizationMetrics
treated the same return code as a hard error, causing collectGPUDeviceMetrics
to surface a misleading failure even though memory metrics had already been
emitted to the channel. Mirror the existing memory guard so a device that
does not support GetUtilizationRates still produces its memory metrics
without a spurious error in the logs.

Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
@Nakshatra480
Nakshatra480 force-pushed the fix/skip-utilization-not-supported branch from fd85797 to 9f01fff Compare August 12, 2026 09:25
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 62.58% <100.00%> (+0.21%) ⬆️

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

Files with missing lines Coverage Δ
cmd/vGPUmonitor/metrics.go 50.98% <100.00%> (+6.13%) ⬆️

... and 2 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

Copy link
Copy Markdown
Contributor

This is being closed because it does not comply with the contribution guidelines.

@Nakshatra480

Copy link
Copy Markdown
Contributor Author

Thanks for your feedback. I will review the contribution guidelines and ensure that my future submissions follow them. If you can, please let me know what was missing or not compliant in this contribution so I can improve.

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.

2 participants