refactor(vGPUmonitor): inject nvml.Interface instead of CGo package-l… - #2628
refactor(vGPUmonitor): inject nvml.Interface instead of CGo package-l…#2628Kanika0306 wants to merge 1 commit into
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 |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe vGPU monitor now injects ChangesNVML interface injection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 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/feedback.go`:
- Around line 136-141: Add a nil check for nvmllib at the start of
watchAndFeedback before calling Init, returning an appropriate error when the
NVML interface is absent; preserve the existing initialization and deferred
Shutdown behavior for non-nil interfaces.
🪄 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: 4c842669-385a-40a6-8194-6812cacd8446
📒 Files selected for processing (3)
cmd/vGPUmonitor/feedback.gocmd/vGPUmonitor/main.gocmd/vGPUmonitor/metrics.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
c9c1e33 to
c0ca20e
Compare
There was a problem hiding this comment.
Pull request overview
Refactors cmd/vGPUmonitor to use nvml.Interface injection instead of package-level NVML CGo calls, aligning the monitor with the interface-based NVML usage pattern in the rest of HAMi and reducing direct coupling to CGo symbols.
Changes:
- Adds an injected
nvml.InterfacetoClusterManagerand threads it through the physical GPU metrics collection flow (with a nil guard to preserve current test behavior). - Updates
watchAndFeedbackto accept and use an injectednvml.Interface. - Initializes NVML interfaces via
nvml.New()inmain.goand replaces direct NVML return-value comparisons witherrors.Is().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/vGPUmonitor/metrics.go | Injects nvml.Interface into the metrics collector path and replaces package-level NVML calls with interface methods. |
| cmd/vGPUmonitor/main.go | Constructs nvml.Interface instances via nvml.New() and passes them into metrics/feedback initialization. |
| cmd/vGPUmonitor/feedback.go | Refactors NVML init/shutdown in the feedback loop to use the injected nvml.Interface. |
Suppressed comments (6)
cmd/vGPUmonitor/metrics.go:282
nvretis annvml.Return(error). Using%shere will likely format incorrectly; use%w/%vinstead so the error string is meaningful and can be unwrapped.
func (cc ClusterManagerCollector) getDeviceCount(nvmllib nvml.Interface) (int, error) {
devnum, nvret := nvmllib.DeviceGetCount()
if !errors.Is(nvret, nvml.SUCCESS) {
return 0, fmt.Errorf("nvml GetDeviceCount err: %s", nvret)
}
cmd/vGPUmonitor/metrics.go:290
nvretis annvml.Return(error).%sformatting is likely incorrect; prefer%w/%vfor proper error reporting/unwrapping.
func (cc ClusterManagerCollector) collectGPUDeviceMetrics(ch chan<- prometheus.Metric, nvmllib nvml.Interface, index int) error {
hdev, nvret := nvmllib.DeviceGetHandleByIndex(index)
if !errors.Is(nvret, nvml.SUCCESS) {
return fmt.Errorf("nvml DeviceGetHandleByIndex err: %s", nvret)
}
cmd/vGPUmonitor/metrics.go:316
- These NVML return values are
nvml.Return(error). Using%swill likely format incorrectly; use%w/%vso the error message is meaningful and can be unwrapped.
uuid, nvret := hdev.GetUUID()
if !errors.Is(nvret, nvml.SUCCESS) {
return fmt.Errorf("nvml GetUUID err: %s", nvret)
}
cmd/vGPUmonitor/metrics.go:343
nvretis annvml.Return(error). Formatting with%swill likely produce%!s(...); use%w/%vfor correct error output and unwrapping.
util, nvret := hdev.GetUtilizationRates()
if !errors.Is(nvret, nvml.SUCCESS) {
return fmt.Errorf("nvml GetUtilizationRates err: %s", nvret)
}
cmd/vGPUmonitor/metrics.go:348
nvretis annvml.Return(error).%sformatting is likely incorrect; use%w/%vso the error message is meaningful and can be unwrapped.
uuid, nvret := hdev.GetUUID()
if !errors.Is(nvret, nvml.SUCCESS) {
return fmt.Errorf("nvml GetUUID err: %s", nvret)
}
cmd/vGPUmonitor/metrics.go:353
nvretis annvml.Return(error). Using%shere will likely format incorrectly; use%w/%vfor correct error reporting and unwrapping.
deviceName, nvret := hdev.GetName()
if !errors.Is(nvret, nvml.SUCCESS) {
return fmt.Errorf("nvml GetName err: %s", nvret)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (cc ClusterManagerCollector) initNVML(nvmllib nvml.Interface) error { | ||
| nvret := nvmllib.Init() | ||
| if !errors.Is(nvret, nvml.SUCCESS) { | ||
| return fmt.Errorf("nvml Init err: %s", nvret) | ||
| } | ||
| return nil | ||
| } |
| func watchAndFeedback(ctx context.Context, lister *nvidia.ContainerLister, nvmllib nvml.Interface, migLockSignal <-chan bool) error { | ||
| klog.Info("Starting watchAndFeedback") | ||
| if nvret := nvml.Init(); nvret != nvml.SUCCESS { | ||
| return fmt.Errorf("failed to initialize NVML: %s", nvml.ErrorString(nvret)) | ||
| if nvret := nvmllib.Init(); !errors.Is(nvret, nvml.SUCCESS) { | ||
| return fmt.Errorf("failed to initialize NVML: %s", nvret) | ||
| } |
…evel calls
Replace direct package-level nvml.Init/Shutdown/DeviceGetCount/DeviceGetHandleByIndex
calls with an injected nvml.Interface in ClusterManager. Switch constant
comparisons (nvret != nvml.SUCCESS) to errors.Is() — the same pattern
already used in pkg/device/nvidia/links.go.
Motivation
- The package-level nvml functions and constants (nvml.SUCCESS,
nvml.ERROR_NOT_SUPPORTED) are defined via CGo (#include nvml.h).
The Windows/non-CGo Go language server cannot resolve them, producing
10 x "undefined: nvml.SUCCESS" IDE errors in metrics.go and feedback.go.
- Calling CGo package-level globals makes the collector hard to unit-test
without real GPU hardware.
Changes
- cmd/vGPUmonitor/metrics.go
- Add nvmllib nvml.Interface field to ClusterManager (unexported).
- Guard collectGPUInfo with a nil check so existing tests that omit
nvmllib continue to work without a mock.
- Thread nvmllib through initNVML/getDeviceCount/collectGPUDeviceMetrics.
- Replace nvret != nvml.SUCCESS with !errors.Is(nvret, nvml.SUCCESS).
- Replace nvml.ERROR_NOT_SUPPORTED == with errors.Is().
- Remove nvml.ErrorString() calls; nvml.Return implements error/Stringer
so direct %s formatting works.
- Update NewClusterManager signature to accept nvml.Interface.
- cmd/vGPUmonitor/feedback.go
- watchAndFeedback now accepts nvml.Interface instead of calling
package-level nvml.Init/Shutdown directly.
- cmd/vGPUmonitor/main.go
- Pass nvml.New() to both NewClusterManager and watchAndFeedback.
All existing unit tests pass (go test ./cmd/vGPUmonitor/... -short).
Signed-off-by: Kanika0306 <kanikakatare0306@gmail.com>
c0ca20e to
bc05abe
Compare
|
This is being closed because it does not comply with the contribution guidelines. |
|
ok I will adhere to the guideline and contribute |
Summary
Refactors
cmd/vGPUmonitorto use thenvml.Interfaceabstraction instead of package-level NVML CGo calls.This aligns
vGPUmonitorwith the existing NVML interface-injection pattern used elsewhere in HAMi and removes unnecessary coupling to CGo-specific package-level symbols.Changes
nvml.InterfaceintoClusterManager.watchAndFeedbackto receive annvml.Interface.nvml.New()frommain.gowhen initializing the components.nvml.Init()/nvml.Shutdown()calls with interface methods.errors.Is().nvml.ErrorString()usage where the returned value already implementserror.NewClusterManagerto accept the NVML interface.Motivation
The current implementation relies on package-level NVML CGo calls and constants, which makes the code more tightly coupled to the CGo implementation of
go-nvml.Using
nvml.Interfaceprovides a cleaner abstraction boundary and makes the GPU monitoring code easier to test with mocked NVML implementations in the future.This also follows the interface-based pattern already present in other HAMi NVIDIA device-plugin components.
Testing
Verified with:
go build ./cmd/vGPUmonitor/... go test ./cmd/vGPUmonitor/... -short -vNo functional behaviour is intended to change.
Related Issue
Closes #2627
Summary by CodeRabbit