fix(chart): declare metrics port on vGPU monitor container - #1990
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ChangesMetrics Port Declaration
Estimated code review effort: 1 (Trivial) | ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request exposes a named port metrics (port 9394) in the NVIDIA device plugin daemonset and updates the monitor service's targetPort to reference this named port. The reviewer noted that changing targetPort to a named port could break metrics collection for other daemonsets (such as Hygon DCU or Cambricon MLU) that share this service but do not have the named port declared. It is recommended to revert targetPort to the numeric value 9394 to maintain compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
aea16ae to
9621c73
Compare
Declare the named 'metrics' containerPort (9394) on the vgpu-monitor container. The monitor already serves Prometheus metrics on :9394, but the port was never declared on the container spec, so it was not discoverable (e.g. for PodMonitor/ServiceMonitor selectors referencing a named port). The monitor Service keeps a numeric targetPort (9394) so it continues to route to any selected pod serving on that port, without requiring every selected pod to declare a container port of that name. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
9621c73 to
567bd06
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, ntheanh201 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
The
vgpu-monitorcontainer in the device-plugin DaemonSet serves Prometheusmetrics on
:9394(the--metrics-bind-addressdefault incmd/vGPUmonitor/main.go),and the
hami-device-plugin-monitorService forwards to that port. However, theport was never declared in the container's
ports:spec.This means the metrics endpoint is not discoverable from the Pod spec (e.g. for
PodMonitor/ServiceMonitorselectors that reference a named port).This PR declares the named
metricscontainerPort (9394) on thevgpu-monitorcontainer, matching the convention already used by the scheduler Deployment.
The monitor Service is intentionally left unchanged: it keeps a numeric
targetPort: 9394. A numeric targetPort routes to any selected pod serving onthat port, whereas a named targetPort would require every pod the Service
selects to declare a container port of that name. Keeping it numeric makes this
change fully non-breaking.
This replaces #1085, whose branch had drifted too far from
masterto rebase.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Verified with
helm template; the container now renders ametricsport and theService
targetPortremains the numeric9394(unchanged frommaster).Does this PR introduce a user-facing change?:
AI assistance disclosure: this change was prepared with the help of Claude Code (per CONTRIBUTING.md).
Summary by CodeRabbit