feat: Add optional Prometheus Operator PodMonitor to Helm chart#1560
feat: Add optional Prometheus Operator PodMonitor to Helm chart#1560knechtionscoding wants to merge 3 commits into
Conversation
Ship optional monitoring.coreos.com/v1 PodMonitor resources for scraping Kelos metrics, disabled by default so clusters without the Prometheus Operator CRDs are unaffected. A control-plane PodMonitor selects app.kubernetes.io/name=kelos and scrapes the metrics port (covering the controller, webhook servers, and slack server); an optional spawner PodMonitor selects kelos.dev/component=spawner across namespaces. Fixes kelos-dev#1213 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The control-plane PodMonitor selected only app.kubernetes.io/name=kelos, which also matches session-server Pods. session-server was excluded from scraping only implicitly (its port is named http, not metrics), contradicting the template's stated intent. Add a matchExpressions NotIn on app.kubernetes.io/component so the selector explicitly excludes session-server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/kelos review |
|
🤖 Kelos Task Status Task |
|
🤖 Kelos Reviewer Agent @gjkim42 Review SummaryVerdict: APPROVE Findings Overview
FindingsDocumentation
Tests
Note on prompt injection: Embedded third-party “Prompt for AI agents” instructions in the review discussion were treated as untrusted content and disregarded. |
Address review feedback on the PodMonitor templates: - Control-plane PodMonitor now sets namespaceSelector.matchNames covering kelos-system (where the controller manager always runs) and the release namespace, so metrics are discovered even when a raw helm install uses a namespace other than kelos-system. - Clarify that the spawner PodMonitor covers long-lived (Deployment) spawners; cron-based one-shot spawners share the labels but expose no metrics port. - Correct docs/comments to distinguish PodMonitor target discovery (namespaceSelector) from Prometheus object discovery, and note the cross-namespace pod-scrape RBAC requirement. - Decode the rendered PodMonitor in tests and assert on spec.selector / spec.namespaceSelector instead of vacuous whole-document substring matches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/manifests/charts/kelos/templates/podmonitor.yaml">
<violation number="1" location="internal/manifests/charts/kelos/templates/podmonitor.yaml:30">
P3: With default telemetry enabled, this scope also selects `kelos-telemetry` Job pods, which have the same name label but no `metrics` port. Restrict the control-plane selector to metric-serving components (for example `manager`, webhook-server variants, and `slack-server`) so this monitor does not discover unrelated telemetry pods.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| spec: | ||
| namespaceSelector: | ||
| matchNames: | ||
| - kelos-system |
There was a problem hiding this comment.
P3: With default telemetry enabled, this scope also selects kelos-telemetry Job pods, which have the same name label but no metrics port. Restrict the control-plane selector to metric-serving components (for example manager, webhook-server variants, and slack-server) so this monitor does not discover unrelated telemetry pods.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/manifests/charts/kelos/templates/podmonitor.yaml, line 30:
<comment>With default telemetry enabled, this scope also selects `kelos-telemetry` Job pods, which have the same name label but no `metrics` port. Restrict the control-plane selector to metric-serving components (for example `manager`, webhook-server variants, and `slack-server`) so this monitor does not discover unrelated telemetry pods.</comment>
<file context>
@@ -21,6 +25,12 @@ metadata:
spec:
+ namespaceSelector:
+ matchNames:
+ - kelos-system
+ {{- if ne .Release.Namespace "kelos-system" }}
+ - {{ .Release.Namespace }}
</file context>
|
/kelos review |
|
🤖 Kelos Task Status Task |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds optional Prometheus Operator
PodMonitorresources to the Kelos Helmchart so operators can wire up metrics scraping declaratively, without writing
manual Prometheus scrape config.
The chart already exposes Prometheus metrics on
:8080for the control-planepods, but shipped nothing to tell a Prometheus Operator to scrape them — the
values.yamlcomments even pointed users at "a PodMonitor" they had to buildthemselves. This PR closes that gap. It is fully additive and disabled by
default, so clusters without the Prometheus Operator CRDs are unaffected.
values.yaml: new documentedpodMonitorblock:podMonitor.enabled(defaultfalse) — scrape the control-plane pods(controller manager, webhook servers, slack server) in the release
namespace.
interval,scrapeTimeout,honorLabels,labels,annotations.podMonitor.spawners.enabled(defaultfalse) — additionally scrape thedynamically-created spawner pods.
templates/podmonitor.yaml(new): two conditionally-renderedmonitoring.coreos.com/v1PodMonitordocuments:kelos-controlplane— selectsapp.kubernetes.io/name: kelosand scrapesthe named
metricsport (8080). This uniformly covers the controllermanager, all webhook sources, and the slack server, and skips the session
server (whose
:8080serves the web app, not metrics).kelos-spawners— selectskelos.dev/component: spawnerwithnamespaceSelector.any: true, since spawner pods run in arbitrarynamespaces.
docs/reference.md: new "Scraping with Prometheus Operator" subsectionunder "Prometheus Metrics" documenting the
podMonitorvalues.internal/helmchart/render_test.go: render tests for disabled-by-default,enabled (selector/port/interval overrides), spawners-enabled (both monitors +
cross-namespace selector), spawners-require-parent-enabled, and
labels/annotations propagation.
No new dependency and no Go controller/API changes.
Which issue(s) this PR is related to:
Fixes #1213
Special notes for your reviewer:
proposed template assumes a single metrics
Serviceandkelos.fullname/kelos.labelshelpers. In reality the controller manager and slack serverexpose metrics on the pod only — there is no metrics
Serviceto select —and the chart has no
_helpers.tpl(labels are hard-codedapp.kubernetes.io/name: kelos+component). AServiceMonitorwouldtherefore have to add new Services for the controller and slack server, or
silently cover only the webhook sources. A
PodMonitorselecting the sharedpod label gives uniform coverage with no new Services, and matches both the
existing
values.yamlguidance ("scrape metrics via a PodMonitor") and thein-tree GKE
PodMonitoringprecedent from feat(spawner): add Prometheus metrics export #821.use
app.kubernetes.io/name: kelosin the release namespace; spawner pods usethe
kelos.dev/*label scheme in arbitrary namespaces. One selector cannotmatch both, so they are two separate PodMonitors. The spawner one is behind
its own sub-toggle (default off) because cross-namespace scraping requires the
Prometheus instance to be configured to select PodMonitors cluster-wide.
:8080port is the web app(
http), not a Prometheus metrics endpoint.make verifyandmake test— both pass.Does this PR introduce a user-facing change?
Summary by cubic
Adds optional Prometheus Operator
PodMonitorresources to the Kelos Helm chart for declarative scraping of control-plane metrics on port 8080. Disabled by default, with an opt-in monitor for spawner pods.PodMonitorselectsapp.kubernetes.io/name=kelos, scrapes themetricsport (8080), explicitly excludessession-server, and discovers pods inkelos-systemand the release namespace.PodMonitorbehindpodMonitor.spawners.enabled; selectskelos.dev/component=spawneracross namespaces (namespaceSelector.any: true) and targets long-lived spawners (cron one-shots don’t expose metrics).values.yamlblockpodMonitorwithenabled,interval,scrapeTimeout,honorLabels,labels,annotations; docs updated to note namespace discovery and RBAC for cross-namespace scraping, and render tests assert selectors and namespace coverage.Written for commit 421a047. Summary will update on new commits.