feat(vGPUmonitor): add missing GPU hardware health and power metrics - #2466
feat(vGPUmonitor): add missing GPU hardware health and power metrics#2466devGPP23 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: devGPP23 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 |
|
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)
📝 WalkthroughWalkthroughvGPUmonitor adds Prometheus metrics for GPU memory, temperature, power usage, and power limits. GPU collection queries NVML health values, skips unsupported queries, logs other query failures, and continues metric collection. ChangesGPU health metrics
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 |
37a72bb to
3f9bba3
Compare
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: devGP7 <gauravpatil232005@gmail.com>
3f9bba3 to
1b7ed54
Compare
|
cc/ @mesutoezdil @archlitchi @FouoF The codecov/patch check is failing because the new NVML API calls are skipped in CI without physical GPUs. vGPUmonitor/metrics.go currently doesn't use an NVML mock interface. Should I refactor the collector to support mocks to fix this coverage, or is it okay to bypass it for now? |
|
Thanks for the metrics work. This PR adds five public host metrics without behavior-level tests or user documentation. memory_total overlaps #2452, while the two _ratio metrics continue a 0–100 scale that conflicts with #2455's proposed 0–1 contract; patch coverage is also only 8.62% and failing. We should choose one public metrics schema before adding implementations, so we are closing this PR rather than publishing another competing contract. |
What this PR does:
This PR adds 5 new GPU hardware metrics to
vGPUmonitorusing NVML.This will help users build much better and more detailed Grafana dashboards for monitoring cluster health.
The new metrics are:
hami_host_gpu_temperature_celsius(Temperature)hami_host_gpu_power_usage_milliwatts(Current Power)hami_host_gpu_power_limit_milliwatts(Power Limit)hami_host_gpu_memory_total_bytes(Total Memory)hami_host_gpu_memory_utilization_ratio(Memory Bandwidth Utilization)How it works & safety:
I added explicit checks for
nvml.ERROR_NOT_SUPPORTED.If a GPU doesn't support reading temperature or power, it safely skips it without crashing the monitor.
It safely follows the exact same pattern already used for
memory.Used.Testing & Hardware Validation:: Passed local unit tests (TestDescribeCollectSync) which verifies the Prometheus descriptors.
Special Note for reviewer:
I do not currently have access to a physical NVIDIA GPU cluster. I have ensured compile-time safety and proper error handling, but I would really appreciate it if a maintainer could run this on a real GPU node to verify the final Prometheus output.
AI Assistance Disclosure:
I used an AI assistant to help research the NVML APIs and structure the code, but I have manually reviewed, checked the safety of the logic, and fully understand all the changes introduced in this PR.
Summary by CodeRabbit