Skip to content

fix(chart): target device plugin monitor port by name - #2979

Open
magic-peach wants to merge 1 commit into
Project-HAMi:masterfrom
magic-peach:fix-monitorservice-target-port
Open

fix(chart): target device plugin monitor port by name#2979
magic-peach wants to merge 1 commit into
Project-HAMi:masterfrom
magic-peach:fix-monitorservice-target-port

Conversation

@magic-peach

@magic-peach magic-peach commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
The device plugin monitor Service hardcodes targetPort: 9394 instead of targeting the container port by name, unlike the scheduler's equivalent monitor Service (fixed in #2895) which targets by name to avoid drift if the container port number ever changes. This applies the same fix here: adds devicePlugin.service.monitorTargetPort (default metrics) to values.yaml, uses it in monitorservice.yaml, and documents it in the chart README, mirroring #2895's pattern exactly.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Confirmed via grep that this was the only remaining hardcoded targetPort of this kind in the chart. Verified rendered output is functionally identical at default values (helm template diff shows only the port field changing from the numeric literal to the equivalent name), and helm lint passes.

Does this PR introduce a user-facing change?:
No behavior change at default values; adds a new devicePlugin.service.monitorTargetPort value for consistency with the scheduler service.


This PR was written primarily by Claude Code, an AI assistant, under my direction and review. I verified the change by comparing rendered chart output before and after, and reviewed the diff before submitting.

Summary by CodeRabbit

  • New Features

    • Added configuration for the device plugin monitor service’s target port.
    • The target port defaults to the named metrics port and can be customized through Helm values.
  • Documentation

    • Updated the Helm chart configuration reference with the new monitor target port setting.

The monitor service hardcodes targetPort to the numeric container
port, unlike the scheduler's monitor service which targets by name
to avoid drift if the container port ever changes. Apply the same
pattern here, matching the metrics port name in daemonsetnvidia.yaml.

Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
@hami-robot hami-robot Bot added the kind/bug Something isn't working label Sep 7, 2026
@hami-robot
hami-robot Bot requested review from mesutoezdil and wawa0210 September 7, 2026 15:51
@hami-robot

hami-robot Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: magic-peach
Once this PR has been reviewed and has the lgtm label, please assign dsfans2014 for approval. 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:

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

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: daeb8cea-8f95-4be3-9570-cb5214660ff7

📥 Commits

Reviewing files that changed from the base of the PR and between 1092ba2 and 40d795f.

📒 Files selected for processing (3)
  • charts/hami/README.md
  • charts/hami/templates/device-plugin/monitorservice.yaml
  • charts/hami/values.yaml

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The HAMi Helm chart adds a configurable monitor target port. The default is the named metrics port. The Service template uses this value, and the README documents the setting.

Changes

Monitor target port configuration

Layer / File(s) Summary
Configure and wire the monitor target port
charts/hami/values.yaml, charts/hami/templates/device-plugin/monitorservice.yaml, charts/hami/README.md
The chart defines devicePlugin.service.monitorTargetPort with default metrics. The monitor Service uses the value instead of fixed port 9394. The README documents the parameter.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 40d79

The monitor Service now targets the monitor container’s named metrics port, with a documented configurable default. Default deployments retain the same metrics endpoint behavior and no merge-blocking risk remains.

Poem

A rabbit set the port to metrics bright
The Service follows the named route right
Values guide the chart with care
README records the setting there
Tiny changes hop through the night

🚥 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: configuring the device plugin monitor Service to target the port by name.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant