Skip to content

feat(monitor): add node label to host GPU metrics #2398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ipsitapp8 wants to merge 4 commits into from
Closed

feat(monitor): add node label to host GPU metrics #2398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c079b8
feat(monitor): add node label to host GPU metrics
ipsitapp8 Aug 6, 2026
20589a5
test: track whether host metrics are found during validation
ipsitapp8 Aug 7, 2026
7d8c911
fix(monitor): rename node label to node_name on host GPU metrics
ipsitapp8 Aug 9, 2026
e205fb5
test(monitor): cover legacy and unset-NODE_NAME cases for host metrics
ipsitapp8 Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions cmd/vGPUmonitor/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ var (
hostGPUdesc = prometheus.NewDesc(
"hami_host_gpu_memory_used_bytes",
"GPU device memory usage in bytes",
[]string{"device_index", "device_uuid", "device_type"}, nil,
[]string{"node_name", "device_index", "device_uuid", "device_type"}, nil,
)

hostGPUUtilizationdesc = prometheus.NewDesc(
"hami_host_gpu_utilization_ratio",
"GPU core utilization ratio (0-100)",
[]string{"device_index", "device_uuid", "device_type"}, nil,
[]string{"node_name", "device_index", "device_uuid", "device_type"}, nil,
)

ctrvGPUdesc = prometheus.NewDesc(
Expand Down Expand Up @@ -136,12 +136,12 @@ func initLegacyDescriptors() {
legacyHostGPUdesc = prometheus.NewDesc(
"HostGPUMemoryUsage",
"GPU device memory usage",
[]string{"deviceidx", "deviceuuid", "devicetype"}, nil,
[]string{"nodeid", "deviceidx", "deviceuuid", "devicetype"}, nil,
)
legacyHostGPUUtilizationdesc = prometheus.NewDesc(
"HostCoreUtilization",
"GPU core utilization",
[]string{"deviceidx", "deviceuuid", "devicetype"}, nil,
[]string{"nodeid", "deviceidx", "deviceuuid", "devicetype"}, nil,
)
legacyCtrvGPUdesc = prometheus.NewDesc(
"vGPU_device_memory_usage_in_bytes",
Expand Down Expand Up @@ -239,6 +239,12 @@ func (cc ClusterManagerCollector) Collect(ch chan<- prometheus.Metric) {
}

@mesutoezdil mesutoezdil Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collectpodandcontainerinfo errors out when node_name is empty. this one falls back to unknown instead. why not do the same here, for consistency?

@ipsitapp8 ipsitapp8 Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the inconsistency, it's intentional, because the two functions use NODE_NAME for different things.

In collectPodAndContainerInfo the node name is a functional input: it's the selector passed to the pod lister (metrics.go:374). With an empty value the lister would match pods whose assigned-node annotation is empty, i.e. silently wrong data. Erroring out is the only correct behavior there.

In collectGPUInfo the node name is only a label value. The underlying NVML reads don't depend on it at all. Since Collect just logs the error and moves on (metrics.go:221-224), returning an error would mean an unset NODE_NAME drops hami_host_gpu_memory_used_bytes and hami_host_gpu_utilization_ratio from every scrape, metrics that work fine today without any node dependency. That would turn a label-only, backward-compatible change into a regression for anyone deploying vGPUmonitor outside the Helm chart (the chart does inject NODE_NAME in daemonsetnvidia.yaml, but manual/vendored manifests exist).

I also chose the literal "unknown" over an empty string on purpose: Prometheus treats an empty label value as absent, so on(node_name) joins would fail in a confusing way, whereas node_name="unknown" keeps the series shape stable and makes the misconfiguration visible in a query. This matches the existing precedent in pkg/version/version.go, where unset build metadata is exposed as "unknown" on hami_build_info.

The warning log at metrics.go:244 surfaces the misconfiguration, and TestHostMetricsIncludeNodeLabel covers the unset case for both the new and legacy descriptors.

If you'd rather have strict consistency and prefer that host metrics disappear when NODE_NAME isn't set, I'm happy to flip it.


func (cc ClusterManagerCollector) collectGPUInfo(ch chan<- prometheus.Metric) error {
nodeName := os.Getenv(util.NodeNameEnvName)
if nodeName == "" {
klog.Warningf("NODE_NAME env var not set, using 'unknown' for node label")
nodeName = "unknown"
}

if err := cc.initNVML(); err != nil {
return err
}
Expand All @@ -250,7 +256,7 @@ func (cc ClusterManagerCollector) collectGPUInfo(ch chan<- prometheus.Metric) er
}

for ii := range devnum {
if err := cc.collectGPUDeviceMetrics(ch, ii); err != nil {
if err := cc.collectGPUDeviceMetrics(ch, nodeName, ii); err != nil {
klog.Error("Failed to collect metrics for GPU device ", ii, ": ", err)
}
}
Expand All @@ -274,24 +280,24 @@ func (cc ClusterManagerCollector) getDeviceCount() (int, error) {
return devnum, nil
}

func (cc ClusterManagerCollector) collectGPUDeviceMetrics(ch chan<- prometheus.Metric, index int) error {
func (cc ClusterManagerCollector) collectGPUDeviceMetrics(ch chan<- prometheus.Metric, nodeName string, index int) error {
hdev, nvret := nvml.DeviceGetHandleByIndex(index)
if nvret != nvml.SUCCESS {
return fmt.Errorf("nvml DeviceGetHandleByIndex err: %s", nvml.ErrorString(nvret))
}

if err := cc.collectGPUMemoryMetrics(ch, hdev, index); err != nil {
if err := cc.collectGPUMemoryMetrics(ch, nodeName, hdev, index); err != nil {
return err
}

if err := cc.collectGPUUtilizationMetrics(ch, hdev, index); err != nil {
if err := cc.collectGPUUtilizationMetrics(ch, nodeName, hdev, index); err != nil {
return err
}

return nil
}

func (cc ClusterManagerCollector) collectGPUMemoryMetrics(ch chan<- prometheus.Metric, hdev nvml.Device, index int) error {
func (cc ClusterManagerCollector) collectGPUMemoryMetrics(ch chan<- prometheus.Metric, nodeName string, hdev nvml.Device, index int) error {
memory, ret := hdev.GetMemoryInfo()
if ret == nvml.ERROR_NOT_SUPPORTED {
klog.V(3).Infof("Memory metrics not supported for device %d (unified memory architecture), skipping", index)
Expand All @@ -317,17 +323,17 @@ func (cc ClusterManagerCollector) collectGPUMemoryMetrics(ch chan<- prometheus.M
hostGPUdesc,
prometheus.GaugeValue,
float64(memory.Used),
fmt.Sprint(index), uuid, deviceName,
nodeName, fmt.Sprint(index), uuid, deviceName,
)

sendLegacyMetric(ch, legacyHostGPUdesc, prometheus.GaugeValue, float64(memory.Used),
fmt.Sprint(index), uuid, deviceName,
nodeName, fmt.Sprint(index), uuid, deviceName,
)

return nil
}

func (cc ClusterManagerCollector) collectGPUUtilizationMetrics(ch chan<- prometheus.Metric, hdev nvml.Device, index int) error {
func (cc ClusterManagerCollector) collectGPUUtilizationMetrics(ch chan<- prometheus.Metric, nodeName string, hdev nvml.Device, index int) error {
util, nvret := hdev.GetUtilizationRates()
if nvret != nvml.SUCCESS {
return fmt.Errorf("nvml GetUtilizationRates err: %s", nvml.ErrorString(nvret))
Expand All @@ -349,11 +355,11 @@ func (cc ClusterManagerCollector) collectGPUUtilizationMetrics(ch chan<- prometh
hostGPUUtilizationdesc,
prometheus.GaugeValue,
float64(util.Gpu),
fmt.Sprint(index), uuid, deviceName,
nodeName, fmt.Sprint(index), uuid, deviceName,
)

sendLegacyMetric(ch, legacyHostGPUUtilizationdesc, prometheus.GaugeValue, float64(util.Gpu),
fmt.Sprint(index), uuid, deviceName,
nodeName, fmt.Sprint(index), uuid, deviceName,
)

return nil
Expand Down
91 changes: 91 additions & 0 deletions cmd/vGPUmonitor/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/prometheus/client_golang/prometheus"
dto "github.com/prometheus/client_model/go"
"k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes/fake"

Expand Down Expand Up @@ -68,3 +69,93 @@ func TestDescribeCollectSync(t *testing.T) {
t.Errorf("Gather failed (legacy): %v", err)
}
}

func TestHostMetricsIncludeNodeLabel(t *testing.T) {
cases := []struct {
name string
legacyMetrics bool
setNodeName bool
wantNodeName string
}{
{name: "non-legacy/env-set", legacyMetrics: false, setNodeName: true, wantNodeName: "test-node-123"},
{name: "non-legacy/env-unset", legacyMetrics: false, setNodeName: false, wantNodeName: "unknown"},
{name: "legacy/env-set", legacyMetrics: true, setNodeName: true, wantNodeName: "test-node-123"},
{name: "legacy/env-unset", legacyMetrics: true, setNodeName: false, wantNodeName: "unknown"},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
reg := prometheus.NewPedanticRegistry()

if tc.setNodeName {
t.Setenv(util.NodeNameEnvName, tc.wantNodeName)
} else {
// Empty value is equivalent to "unset" for the os.Getenv("") == ""
// fallback check in collectGPUInfo.
t.Setenv(util.NodeNameEnvName, "")
}

client := fake.NewSimpleClientset()
informerFactory := informers.NewSharedInformerFactory(client, 0)
podLister := informerFactory.Core().V1().Pods().Lister()

if tc.legacyMetrics {
initLegacyDescriptors()
}

c := &ClusterManager{
Zone: "test-zone",
LegacyMetrics: tc.legacyMetrics,
PodLister: podLister,
containerLister: &nvidia.ContainerLister{},
}
cc := ClusterManagerCollector{ClusterManager: c}

if err := reg.Register(cc); err != nil {
t.Fatalf("Failed to register: %v", err)
}

metrics, err := reg.Gather()
if err != nil {
t.Fatalf("Gather failed: %v", err)
}

// Metrics may not be present if NVML initialization fails (no GPU
// hardware); that's expected in test environments, so absence
// doesn't fail the test, but any metric that IS present must
// carry the correct node_name label.
assertHostMetricNodeLabel(t, metrics, "hami_host_gpu_memory_used_bytes", tc.wantNodeName)
assertHostMetricNodeLabel(t, metrics, "hami_host_gpu_utilization_ratio", tc.wantNodeName)
})
}
}

func assertHostMetricNodeLabel(t *testing.T, metrics []*dto.MetricFamily, metricName, wantNodeName string) {
t.Helper()

for _, mf := range metrics {
if mf.GetName() != metricName {
continue
}
for _, m := range mf.GetMetric() {
labels := m.GetLabel()
if len(labels) != 4 {
t.Errorf("%s has %d labels, expected 4", metricName, len(labels))
}

hasNode := false
for _, label := range labels {
if label.GetName() == "node_name" {
hasNode = true
if label.GetValue() != wantNodeName {
t.Errorf("node_name label = %s, want %s", label.GetValue(), wantNodeName)
}
}
}
if !hasNode {
t.Errorf("%s missing 'node_name' label", metricName)
}
}
t.Logf("%s found and validated", metricName)
}
Comment thread
archlitchi marked this conversation as resolved.
}
Loading