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

[ipsitapp8]

AI Assistance Disclosure

This PR was implemented with assistance from Kiro (Claude Sonnet 4.5) and Claude Code. The AI helped with:

• Code generation for adding the node label to metrics
• Test case creation
• Understanding the existing codebase patterns

I have reviewed all changes, understand how they work, and validated them with local tests using WSL2.

---

Problem

Scheduler metrics include a node label, but vGPUmonitor host metrics
(hami_host_gpu_memory_used_bytes, hami_host_gpu_utilization_ratio)
only have device_index, device_uuid, and device_type.

This breaks PromQL joins between allocation and runtime metrics, making
it impossible to create dual-axis panels showing allocated vs. used memory
on multi-node clusters.

Related to #2126 (Gap #2 from observability gap analysis).

Changes

• Add node_name label to hami_host_gpu_memory_used_bytes and
  hami_host_gpu_utilization_ratio descriptors
• Read NODE_NAME from env in collectGPUInfo() (matches existing
  pattern in collectPodAndContainerInfo())
• Thread nodeName through collectGPUDeviceMetrics →
  collectGPUMemoryMetrics → collectGPUUtilizationMetrics
• Update both new and legacy metric descriptors for consistency

The label is named node_name rather than node, per review feedback,
to match the naming direction proposed in #2343 (which would standardize
the scheduler's node label to node_name too, for consistency across
components).

Testing

Added TestHostMetricsIncludeNodeLabel to verify:

• Host metrics have 4 labels including node_name
• node_name label contains the correct value from NODE_NAME env

All tests pass (run via WSL2/Ubuntu):

=== RUN   TestDescribeCollectSync
--- PASS: TestDescribeCollectSync (0.00s)
=== RUN   TestHostMetricsIncludeNodeLabel
--- PASS: TestHostMetricsIncludeNodeLabel (0.00s)
PASS
ok      github.com/Project-HAMi/HAMi/cmd/vGPUmonitor    0.020s

Tests show expected NVML errors (no GPU drivers in test environment), but this
is normal—the change affects only metric label schema, not NVML interaction logic.

No GPU hardware available for live multi-node validation. If a maintainer can
test against a real cluster, I'm happy to iterate based on actual output.

Example

Before (no node label on host metrics — join fails on multi-node):

# Error: multiple matches for device_uuid across different nodes
hami_gpu_memory_allocated_bytes * on(device_uuid) hami_host_gpu_memory_used_bytes

After (label added, key aligned with the node_name direction proposed in #2343):

hami_host_gpu_memory_used_bytes{node_name="<node>"}

Note: scheduler metrics (hami_gpu_memory_allocated_bytes) still use the
label key node on master — #2343, which would have renamed it to
node_name, was closed without merging. Until that lands, a same-key join
across components needs a bridge:

hami_gpu_memory_allocated_bytes
  * on(node, device_uuid) group_left()
  label_replace(hami_host_gpu_memory_used_bytes, "node", "$1", "node_name", "(.*)")

Breaking Change?

No. This adds a label, which is backward-compatible. Existing queries that don't reference node_name will continue to work.

Summary by CodeRabbit

• New Features

  • Host GPU metrics now include the node name for improved identification.
  • Legacy host metrics include a nodeid label.
  • Metrics use the configured node name, or unknown when unavailable.

• Bug Fixes

  • Improved labeling consistency across GPU device, memory, utilization, and legacy metrics.

[hami-robot]

Welcome @ipsitapp8! It looks like this is your first PR to Project-HAMi/HAMi 🎉

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 97bbb307-e1b0-4b19-bbb1-b9138a672b3f

📥 Commits

Reviewing files that changed from the base of the PR and between 7d8c911 and e205fb5.

📒 Files selected for processing (1)

• cmd/vGPUmonitor/metrics_test.go

---

📝 Walkthrough

Walkthrough

Host GPU metrics read node identity from NODE_NAME, use "unknown" when it is unset, and propagate the value through current and legacy memory and utilization metrics. Tests cover configured and unset node names.

Changes

Host GPU node labels

Layer / File(s)	Summary
Resolve and emit node labels cmd/vGPUmonitor/metrics.go	Current metrics include node_name. Legacy metrics include nodeid. Collection helpers receive the resolved NODE_NAME value or "unknown".
Validate host metric labels cmd/vGPUmonitor/metrics_test.go	Table-driven tests cover current and legacy collectors with configured and unset node names. A helper validates four labels and accepts absent metric families.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• [Feature/Metrics] Instrument vGPUmonitor metrics with native node_name and pod labels #2152 — The issue requests node_name labeling for host vGPUmonitor metrics.
• feat(monitor): Standardized OpenTelemetry and Dynamic Cluster Node Labeling #2510 — The issue also modifies cmd/vGPUmonitor/metrics.go to add a node_name label.

Possibly related PRs

• Project-HAMi/HAMi#2153 — It updates the same metric collection path and related label tests.
• Project-HAMi/HAMi#2170 — It aligns GPU Prometheus descriptors with the node_name label.
• Project-HAMi/HAMi#2512 — It changes the same host GPU metric label propagation path.

Suggested reviewers: archlitchi, wawa0210

Poem

A rabbit labels GPUs bright,
With node_name set just right.
When the node name is away,
"unknown" marks the field today.
Four labels hop in line,
Metrics stay aligned.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a node label to host GPU metrics.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/metrics_test.go`:
- Around line 101-125: Update the metric-validation loop in the relevant test to
track whether hami_host_gpu_memory_used_bytes and
hami_host_gpu_utilization_ratio are each encountered during reg.Gather(). After
processing the gathered metrics, fail the test if either expected metric family
was not found, while preserving the existing label validations.

🪄 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: 44f66fac-87a1-4b0b-a7fa-fe0ad0118826

📥 Commits

Reviewing files that changed from the base of the PR and between e6d0902 and 2c079b8.

📒 Files selected for processing (2)

• cmd/vGPUmonitor/metrics.go
• cmd/vGPUmonitor/metrics_test.go

[archlitchi]

/assign

[codecov]

Codecov Report

❌ Patch coverage is 18.75000% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/vGPUmonitor/metrics.go	18.75%	12 Missing and 1 partial ⚠️

Flag	Coverage Δ
unittests	61.72% <18.75%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/vGPUmonitor/metrics.go	30.06% <18.75%> (-0.07%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ipsitapp8
Once this PR has been reviewed and has the lgtm label, please ask for approval from archlitchi. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

🧹 Nitpick comments (1)

cmd/vGPUmonitor/metrics_test.go (1)

75-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the legacy path and the unset-node fallback.

TestHostMetricsIncludeNodeLabel tests only LegacyMetrics: false with NODE_NAME set. The change also updates legacy descriptors and maps an unset NODE_NAME to "unknown". Add cases for both LegacyMetrics values and both node-name states. Apply the label assertions to each case.

🤖 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/metrics_test.go` around lines 75 - 89, Expand
TestHostMetricsIncludeNodeLabel into cases covering both LegacyMetrics values
and both set and unset NODE_NAME states, including the unset fallback to
"unknown". Construct each case with the appropriate ClusterManager configuration
and apply the existing node-label assertions to every case.

🤖 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.

Nitpick comments:
In `@cmd/vGPUmonitor/metrics_test.go`:
- Around line 75-89: Expand TestHostMetricsIncludeNodeLabel into cases covering
both LegacyMetrics values and both set and unset NODE_NAME states, including the
unset fallback to "unknown". Construct each case with the appropriate
ClusterManager configuration and apply the existing node-label assertions to
every case.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cd3e6b8e-5c68-4280-9ba6-f0b022215d01

📥 Commits

Reviewing files that changed from the base of the PR and between 20589a5 and 7d8c911.

📒 Files selected for processing (2)

• cmd/vGPUmonitor/metrics.go
• cmd/vGPUmonitor/metrics_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• cmd/vGPUmonitor/metrics.go

[ipsitapp8]

@archlitchi The latest commit (e205fb5) is pushed, addressing the node_name label rename and adding table-driven test coverage for legacy/unset-NODE_NAME cases. The CI Workflow and CodeQL runs are stuck in action_required and need a maintainer to approve them before Compile/lint/Unit test can run. Could you approve the workflow run when you get a chance?

[mesutoezdil]

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

[ipsitapp8]

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.

[mesutoezdil]

Answers must be written by human being.

You can view the relevant rule here.
https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#contribution-gates

[ipsitapp8]

@mesutoezdil The mistake was from my side! however I understood the workflow and wrote the reply by myself.. If you want you want give a chance for this
collectPodAndContainerInfo uses the node name to filter data so node name must be exact while the other one uses nodename as a label on data and if any machine is misconfigured, it would return an error and all metrics of the GPU will be lost even when the GPU is working fine.