refactor(vGPUmonitor): use nvml.Interface - #2629
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Kanika0306 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe vGPU monitor now receives an injected ChangesNVML interface injection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main.go`:
- Line 105: Update start to create a single nvml.Interface instance, then pass
that shared interface to both watchAndFeedback and initMetrics instead of
constructing NVML objects independently. Adjust the affected function signatures
and call sites while preserving existing monitoring and metrics behavior.
In `@cmd/vGPUmonitor/nvml_degraded_test.go`:
- Line 1: Add the repository’s standard Apache 2.0 license header to the top of
the new Go file, before the package declaration in the main package.
🪄 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: 7a3363dc-c421-4e3d-aa71-8ce12b653819
📒 Files selected for processing (5)
cmd/vGPUmonitor/feedback.gocmd/vGPUmonitor/main.gocmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/nvml_degraded_test.gocommit_message.txt
| @@ -0,0 +1,48 @@ | |||
| package main | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the Apache 2.0 license header.
This new Go file has no required license header. Copy the standard Apache 2.0 header from an existing repository Go file.
As per coding guidelines, "**/*.go: All Go files require an Apache 2.0 license header."
🤖 Prompt for 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.
In `@cmd/vGPUmonitor/nvml_degraded_test.go` at line 1, Add the repository’s
standard Apache 2.0 license header to the top of the new Go file, before the
package declaration in the main package.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
This PR refactors cmd/vGPUmonitor to stop using package-level NVML CGo calls directly and instead use an injected nvml.Interface, aligning vGPUmonitor with the interface-based NVML dependency-injection pattern used elsewhere in HAMi (per issue #2627). It also adds tests to ensure the “degraded mode” behavior works when no NVML interface is provided.
Changes:
- Inject
nvml.Interfaceinto the vGPUmonitor GPU metrics collection path and update NVML init/device discovery to use the interface. - Update
watchAndFeedbackto accept annvml.Interfaceand support a nil-interface degraded mode. - Add unit tests covering nil-NVML degraded behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| commit_message.txt | Documents the refactor and the added degraded-mode unit tests. |
| cmd/vGPUmonitor/nvml_degraded_test.go | Adds focused tests ensuring nil-NVML degraded paths return promptly and without error. |
| cmd/vGPUmonitor/metrics.go | Refactors NVML usage to go through injected nvml.Interface and adds a nil guard to skip physical GPU metrics when NVML isn’t configured. |
| cmd/vGPUmonitor/main.go | Wires a real NVML interface (nvml.New()) into metrics and feedback paths. |
| cmd/vGPUmonitor/feedback.go | Updates watchAndFeedback to use injected NVML and adds degraded-mode behavior when NVML is nil. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| wg.Go(func() { | ||
| for { | ||
| if err := watchAndFeedback(ctx, containerLister, lockChannel); err != nil { | ||
| if err := watchAndFeedback(ctx, containerLister, nvmlgo.New(), lockChannel); err != nil { | ||
| // if err is temporary closed, wait for lock file to be removed |
0744b23 to
cd4b363
Compare
| return fmt.Errorf("nvml Init err: %s", nvml.ErrorString(nvret)) | ||
| func (cc ClusterManagerCollector) initNVML(nvmllib nvml.Interface) error { | ||
| nvret := nvmllib.Init() | ||
| if !errors.Is(nvret, nvml.SUCCESS) { |
There was a problem hiding this comment.
pr body and coderabbit both say tests were added or updated, but no _test.go file is in this diff. where are they?
There was a problem hiding this comment.
Thanks for catching that! The test additions were previously omitted from the commit diff. I have now added unit tests in metrics_test.go (TestCollectGPUInfo_NilNVML) and feedback_test.go (TestWatchAndFeedback_NilNVML) covering NewClusterManager, collectGPUInfo, and watchAndFeedback under a nil NVML interface / degraded mode
| return 0, fmt.Errorf("nvml GetDeviceCount err: %s", nvml.ErrorString(nvret)) | ||
| func (cc ClusterManagerCollector) getDeviceCount(nvmllib nvml.Interface) (int, error) { | ||
| devnum, nvret := nvmllib.DeviceGetCount() | ||
| if !errors.Is(nvret, nvml.SUCCESS) { |
There was a problem hiding this comment.
does errors.Is(nvret, nvml.SUCCESS) behave any differently than nvret == nvml.SUCCESS here? why pick errors.Is?
There was a problem hiding this comment.
nvret is of type nvml.Return, which satisfies the standard Go error interface (Error() string). Using errors.Is(nvret, nvml.SUCCESS) follows the established convention across HAMi (e.g. pkg/device/nvidia/links.go:127,159), providing idiomatic Go error checking for types implementing error and supporting unwrapping.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
f47146d to
5a900e3
Compare
cd938ff to
80ec41b
Compare
80ec41b to
6bae21b
Compare
|
Can you explain what is the key difference of nvml.Interface and NVML CGo and why we should refactor? |
6bae21b to
a90b40a
Compare
…ics collection This refactor decouples the vGPUmonitor metrics collector from package-level NVML calls by accepting an nvml.Interface value through NewClusterManager and propagating it through the internal collect* helpers. Changes: - cmd/vGPUmonitor/metrics.go: store nvml.Interface on ClusterManager; thread it through collectGPUInfo, initNVML, getDeviceCount, collectGPUDeviceMetrics, and collectGPUUtilizationMetrics; guard all collect helpers against nil ClusterManager / PodLister; wrap NVML returns with errors.Is for idiomatic error comparison; guard NewClusterManager informer setup behind a nil check. - cmd/vGPUmonitor/main.go: pass nvml.New() to NewClusterManager and initMetrics so production code still uses the real library. - cmd/vGPUmonitor/feedback.go: alias klog import for clarity; downgrade debug-only klog.V(5) calls to klog.Infof for visible feedback in logs. - cmd/vGPUmonitor/metrics_test.go: add TestCollectGPUInfo_NilNVML, TestNewClusterManager, TestInitMetrics, TestCollectGPUInfo_Success, and TestCollectGPUInfo_ErrorPaths using nvml/mock. - cmd/vGPUmonitor/feedback_test.go: add TestWatchAndFeedback_WithNVMLSuccess, TestWatchAndFeedback_WithNVMLError, TestWatchAndFeedback_MigLockSignal. Signed-off-by: Kanika0306 <kanikakatare0306@gmail.com>
a90b40a to
36ef0cc
Compare
Here's a version formatted to read naturally as a PR reply comment (less "generated report," more like an actual contributor explaining their reasoning): Good question — happy to explain. The core difference: the old code called NVML functions directly as package-level globals ( Why that matters:
No functional change intended for the production path — |
|
/assign |
|
This is being closed because it does not comply with the contribution guidelines. |
could you let me know why it does not because all the test cases pass here |
pls read the contributions rules. |
What does this PR do?
Refactors
cmd/vGPUmonitorto replace package-level NVML CGo calls with the injectednvml.Interface.This removes the direct dependency on package-level NVML symbols such as
nvml.SUCCESSandnvml.ERROR_NOT_SUPPORTEDand makesvGPUmonitorconsistent with the interface-based NVML usage in the project.Changes
nvml.Interface.vGPUmonitorto use the injected NVML interface.Why?
Using
nvml.Interfacekeeps the implementation consistent with the project's dependency-injection approach and makes the code easier to test and maintain.It also avoids relying on package-level NVML CGo symbols that are not available through the interface-based implementation.
Testing
cmd/vGPUmonitor.make verify.Related Issue
Closes #2627
AI Assistance Disclosure
I consulted ChatGPT to understand the codebase and the NVML interface usage. The implementation was reviewed and understood by me, and I verified the changes and tests locally.
Summary by CodeRabbit
New Features
Bug Fixes
Tests