Skip to content

fix(argo-rollouts): honor namespaceOverride in ServiceMonitor - #4095

Open
PhuocOng wants to merge 2 commits into
argoproj:mainfrom
PhuocOng:fix/rollouts-servicemonitor-namespace
Open

PhuocOng wants to merge 2 commits into
argoproj:mainfrom
PhuocOng:fix/rollouts-servicemonitor-namespace

Conversation

@PhuocOng

@PhuocOng PhuocOng commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #4094.

The metrics Service uses the argo-rollouts.namespace helper, but the ServiceMonitor target selector uses .Release.Namespace. When namespaceOverride differs from the release namespace, the monitor cannot select the Service it is meant to scrape.

Use the same namespace helper for spec.namespaceSelector.matchNames. The monitor's own configurable namespace, labels and endpoint port are unchanged. No new values, images or dependencies.

Validation (Helm 4.2.3, chart-testing 3.14.0, kind / Kubernetes 1.35.0):

  • Four local structured render cases cover with/without namespaceOverride and with/without a separate monitor namespace. Both override cases fail before the fix; all four pass afterward, including matching Service labels and ports.
  • All three existing chart-testing install configurations pass. Repository lint, changelog lint, generated-docs diff and ArtifactHub lint pass.
  • With the official ServiceMonitor CRD installed, a live chart install places the monitor in a separate monitoring namespace, selects the overridden workload namespace and exposes a reachable metrics endpoint. This checks resources and endpoint reachability, not a full Prometheus Operator scrape loop.
  • Default resources are unchanged apart from chart-version labels.

Upstream Linting and Testing passed on commit d86229fd09153b380a1d6411f22b6fab58d01fb5, including all three install configurations. All seven PR checks passed.

Prepared with GitHub Copilot assistance.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • I wrote this PR with the help of a LLM but I fully vetted the work before raising this PR for review
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • I have created a separate pull request for each chart according to pull requests
  • My build is green (troubleshooting builds).

@PhuocOng
PhuocOng marked this pull request as ready for review September 24, 2026 17:40
tico24
tico24 previously approved these changes Sep 29, 2026

@tico24 tico24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#4104 also uses 2.43.3 and is set to auto-merge, so could you bump this to 2.43.4?

@tico24
tico24 force-pushed the fix/rollouts-servicemonitor-namespace branch from d86229f to 3826328 Compare October 14, 2026 07:36
tico24
tico24 previously approved these changes Oct 14, 2026
@tico24
tico24 enabled auto-merge (squash) October 14, 2026 07:40
Signed-off-by: Peter Ong <peterong@microsoft.com>
@tico24
tico24 force-pushed the fix/rollouts-servicemonitor-namespace branch from 3826328 to 4181b1a Compare October 26, 2026 09:08
tico24
tico24 previously approved these changes Oct 26, 2026
argo-helm now bumps chart versions after merge (argoproj#4176). This puts the version and artifacthub.io/changes in Chart.yaml back to main's, and moves this PR's changelog entry into charts/argo-rollouts/changes/honor-namespaceoverride-in-servicemonitor.yaml with the same bump level.

Signed-off-by: Tim Collins <tim@thecollins.team>

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[argo-rollouts] ServiceMonitor ignores namespaceOverride for metrics targets

2 participants