feat: add hami_gpu_device_health metric to scheduler - #2540
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: manoj-1407 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)
📝 WalkthroughWalkthroughThe scheduler adds the ChangesScheduler metrics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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:
|
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
left two small questions inline. also this pr has a conflict with master right now, needs a rebase before merge.
| "Realized MIG instance identity and scheduler placement", | ||
| []string{"node", "device_uuid", "device_index", "mig_uuid", "profile", "gpu_instance_id", "compute_instance_id", "placement_start", "placement_size"}, nil, | ||
| ) | ||
| nodeGPUDeviceHealthDesc := prometheus.NewDesc( |
There was a problem hiding this comment.
name says gpu but this loop runs for every backend, not only gpu. same pattern as the other metrics near it. just checking this is on purpose.
There was a problem hiding this comment.
Yes, intentional — same pattern as hami_gpu_memory_limit_bytes, hami_gpu_core_limit_ratio, and the other descriptors in that loop. The gpu prefix follows the existing naming convention in this file rather than reflecting the backend type at runtime. The loop is backend-agnostic by design.
| } | ||
| } | ||
|
|
||
| healthVal := float64(0) |
There was a problem hiding this comment.
is this health value live at scrape time, or a cached value from an older check? matters for alert delay if someone pages off this metric.
There was a problem hiding this comment.
it's the value from the most recent CheckHealth() call, written into DeviceUsage.Health by the device plugin and cached in nodeManager. so it reflects last known state, not a live poll at scrape time — lag depends on the device plugin's check interval. happy to add a note to the metric description if that's useful.
DeviceUsage.Health is used internally to skip unhealthy devices in Fit()
but was never emitted as a Prometheus metric. Operators have no way to
alert on an unhealthy GPU without inspecting node annotations manually.
Add hami_gpu_device_health gauge (1=healthy, 0=unhealthy) in
collectNodeMetrics, with label set {node, device_uuid, device_index,
device_type} consistent with hami_gpu_memory_limit_bytes.
Signed-off-by: G. Manoj Kumar <manojkumar148700@gmail.com>
Signed-off-by: manoj-1407 <manojkumar148700@gmail.com>
f2aff49 to
693649f
Compare
|
This is being closed because it does not comply with the contribution guidelines. |
|
Hi @mesutoezdil , fair call. i pasted ai generated replies without reviewing them properly - that was wrong. I do understand the code though. Would you be open to reopening if I engage properly going forward? |
2 participants
DeviceUsage.Healthalready exists — device plugins set it throughCheckHealth(), and the scheduler uses it internally to skip unhealthy devices when scoring. But it's never actually exposed anywhere. Right now if a GPU goes unhealthy, the only way to notice is digging through node annotations by hand — Prometheus has zero visibility into it.This adds
hami_gpu_device_healthas a gauge (1 = healthy, 0 = unhealthy) insidecollectNodeMetrics, with labelsnode,device_uuid,device_index,device_type— same set ashami_gpu_memory_limit_bytes, so it joins cleanly with the metrics that already exist.What this gets you: alert on
hami_gpu_device_health == 0and page someone, build a Grafana panel showing device status per node, or line up unhealthy-device events against spikes in scheduling failures — which right now you basically can't do without going device by device.Used the same 0/1 gauge pattern
hami_mig_device_infoalready uses for boolean state, so nothing new stylistically. No legacy metric added since there wasn't a health metric in legacy mode to begin with.One note for whoever reviews —
nodeGPUDeviceHealthDescis declared as a local variable inside the function, matching every other descriptor in there. None of them are package-level, so I kept it consistent.Test added:
TestCollectNodeMetricsDeviceHealth, one healthy device and one unhealthy device, checks both come out with the right value. Followed the same structure asTestAMDCoreAllocatedRatioNormalizationalready in that file.Summary by CodeRabbit
New Features
1for healthy devices and0for unhealthy devices, with node, UUID, index, and device type details.Tests