feat(helm): optional prometheus-nats-exporter sidecar + PodMonitor for nautobot NATS#159
feat(helm): optional prometheus-nats-exporter sidecar + PodMonitor for nautobot NATS#159dmathur0 wants to merge 12 commits into
Conversation
📝 WalkthroughWalkthroughThe Helm chart adds optional NATS exporter configuration, injects a Prometheus exporter sidecar into the NATS pod, creates a conditional PodMonitor, permits metrics ingress, supports air-gapped image mapping, and adds observability-gated integration tests. ChangesNATS metrics integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NATS
participant prom_exporter
participant PodMonitor
participant Prometheus
NATS->>prom_exporter: Serve monitoring data on localhost:8222
prom_exporter->>PodMonitor: Expose prom-metrics at /metrics
Prometheus->>PodMonitor: Discover NATS exporter target
PodMonitor->>prom_exporter: Scrape prom-metrics
prom_exporter-->>Prometheus: Return nats metrics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@deploy/helm/values.yaml`:
- Around line 142-146: Update the natsExporter configuration in values.yaml to
use a prometheus-nats-exporter release containing the JetStream jsz handling
crash fix, while preserving the existing sidecar metrics configuration and its
-jsz=all behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a8616afd-8f2b-4f0c-aefb-40d9a27a4690
📒 Files selected for processing (3)
deploy/helm/templates/monitoring.yamldeploy/helm/templates/nautobot.yamldeploy/helm/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tests/integration/test_nats_exporter.py (1)
212-216: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the complete exporter argument contract.
The test only detects removal of
-jsz=all; it would miss regressions removing-connz,-varz, or-prefix=nats, despite those flags being part of the chart’s exporter configuration. Assert all required arguments together.Proposed fix
args = container.get("args", []) - if EXPORTER_JSZ_ARG not in args: + required_args = (EXPORTER_JSZ_ARG, "-connz", "-varz", "-prefix=nats") + missing_args = [arg for arg in required_args if arg not in args] + if missing_args: problems.append( - f"'{EXPORTER_CONTAINER}' missing '{EXPORTER_JSZ_ARG}' arg " + f"'{EXPORTER_CONTAINER}' missing exporter args {missing_args} " f"(JetStream metrics would be absent); args={args}" )🤖 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 `@src/tests/integration/test_nats_exporter.py` around lines 212 - 216, Update the exporter argument validation around EXPORTER_JSZ_ARG to define and check the complete required argument set, including -jsz=all, -connz, -varz, and -prefix=nats. Report any missing required arguments while preserving the existing problem-collection behavior.
🤖 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 `@src/tests/integration/test_nats_exporter.py`:
- Around line 78-84: Update the kubectl invocation in the subprocess.run call to
include a process-level timeout, and catch subprocess.TimeoutExpired so it
follows the existing None return path. Keep the current --request-timeout
argument for API requests and preserve normal subprocess result handling.
---
Nitpick comments:
In `@src/tests/integration/test_nats_exporter.py`:
- Around line 212-216: Update the exporter argument validation around
EXPORTER_JSZ_ARG to define and check the complete required argument set,
including -jsz=all, -connz, -varz, and -prefix=nats. Report any missing required
arguments while preserving the existing problem-collection behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 663fab20-c50e-498d-97d0-08c048deb624
📒 Files selected for processing (3)
deploy/helm/templates/monitoring.yamldeploy/helm/values-observability.yamlsrc/tests/integration/test_nats_exporter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/helm/templates/monitoring.yaml
| {{- end }} | ||
|
|
||
| {{- /* Nautobot NATS exporter PodMonitor (JetStream metrics for KEDA autoscaling) */}} | ||
| {{- if and .Values.monitoring.podMonitors.enabled .Values.nautobotNats.metrics.enabled }} |
There was a problem hiding this comment.
we should also prevent exporter installation when we use external nautobot/nats
…r nautobot NATS Add an opt-in prometheus-nats-exporter sidecar to the nautobot NATS deployment (behind nautobotNats.metrics.enabled, default false) plus a matching PodMonitor (gated additionally on monitoring.podMonitors.enabled). The exporter runs with -jsz=all so it emits the JetStream nats_consumer_num_pending series, enabling KEDA render autoscaling to scale on render-queue backlog. Defaults keep both the sidecar and the PodMonitor off, so existing environments render identically. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
…porter Add src/tests/integration/test_nats_exporter.py, gated behind OBSERVABILITY=true like test_probe_labels.py: when the observability stack is deployed it asserts the nautobot NATS PodMonitor exists, scrapes the prom-metrics port, selects the nats server pod (excluding nats-box), and that the NATS Deployment carries the prom-exporter sidecar with -jsz=all. Enable nautobotNats.metrics.enabled in values-observability.yaml so the local-dev + kind observability path actually exercises the exporter + PodMonitor (both still default off in values.yaml). Also tighten the NATS PodMonitor selector with a DoesNotExist matchExpression on app.kubernetes.io/component: the nats-box pod shares the app.kubernetes.io/name label but carries a component label the server pod lacks, so without the exclusion the PodMonitor would also select nats-box (which has no prom-metrics port). Found while running the new test live against kiwi-qa01. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
Wrap the containers lookup to satisfy `ruff format --check` (CI Python Lint). Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
- Gate the NATS PodMonitor on local (non-external) Nautobot/NATS so it does not render when externalServices.nautobot.local=false (no in-cluster NATS pod/exporter to scrape), matching the NATS Deployment's own condition. - Enable nautobotNats.metrics in the air-gapped extract values so the prometheus-nats-exporter image is captured for the mirror. - Bump prometheus-nats-exporter 0.15.0 -> 0.20.1. - Harden test_nats_exporter kubectl helper with a process-level timeout (request-timeout does not bound a hung auth plugin), mapping TimeoutExpired to the existing None path. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
9dbad98 to
a068d21
Compare
There was a problem hiding this comment.
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 `@src/tests/integration/test_nats_exporter.py`:
- Around line 64-68: Add assertions in the NATS exporter contract tests around
the existing exporter checks to validate the exporter image and require the
-connz, -varz, and -prefix=nats arguments, in addition to the current container
name, port, and -jsz=all assertions. Accept the documented air-gapped image
override while rejecting other image values, and keep the expectations aligned
with the sidecar and PodMonitor templates.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 381370f6-60ab-4c70-8017-0ad2893b2ce6
📒 Files selected for processing (6)
deploy/airgapped/values-airgapped-extract.yamldeploy/helm/templates/monitoring.yamldeploy/helm/templates/nautobot.yamldeploy/helm/values-observability.yamldeploy/helm/values.yamlsrc/tests/integration/test_nats_exporter.py
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/airgapped/values-airgapped-extract.yaml
- deploy/helm/values.yaml
- deploy/helm/values-observability.yaml
- deploy/helm/templates/nautobot.yaml
Broaden the sidecar integration test beyond name/port/-jsz=all to cover the whole contract the metrics pipeline depends on: require -connz, -varz and -prefix=nats (so metrics are named nats_* for KEDA/PromQL), and assert the container runs a prometheus-nats-exporter image while accepting any registry so an air-gapped mirror still passes. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
There was a problem hiding this comment.
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 `@src/tests/integration/test_nats_exporter.py`:
- Around line 228-235: Update the image validation near EXPORTER_IMAGE_REPO to
strip both tag and digest, then compare the repository’s final path component
rather than using repo.endswith(...). Preserve support for arbitrary registries
while rejecting names such as evil-prometheus-nats-exporter.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7abdd6ad-e1d7-4e7a-9001-e53ee59aa358
📒 Files selected for processing (1)
src/tests/integration/test_nats_exporter.py
… resources) The prometheus-nats-exporter sidecar ran without a container securityContext (root, full capabilities) and with a fixed 64Mi memory limit: - Apply nv-config-manager.containerSecurityContext (runAsNonRoot, uid/gid 1000, allowPrivilegeEscalation: false, drop ALL caps) to match every other app container in the chart and satisfy restricted PodSecurity. The exporter only reads localhost:8222 and serves /metrics, so it needs no privileges. - Make resources configurable via nautobotNats.metrics.resources and raise the default memory limit to 128Mi. -jsz=all scrapes every JetStream stream/ consumer, so the transient footprint grows with consumer count; 64Mi risked OOMKill loops on larger deployments. - Extend test_nats_exporter.py to assert the sidecar runs non-root, forbids privilege escalation, and drops ALL caps so the hardening can't regress. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
repo.endswith(EXPORTER_IMAGE_REPO) also accepted bogus images such as
"evil-prometheus-nats-exporter", and rsplit(":", 1) stripped a tag but not an
@sha256: digest. Drop the digest and tag, then compare the final repository
path component so any registry / air-gapped mirror still passes while a wrong
image is rejected. (per CodeRabbit review on #159)
Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
| # Polls the NATS monitoring endpoint and exposes nats_* Prometheus metrics. | ||
| natsExporter: | ||
| repository: docker.io/natsio/prometheus-nats-exporter | ||
| tag: "0.20.1" |
There was a problem hiding this comment.
The air-gapped install still references Docker Hub because natsExporter is not registered with the installer’s image mapping. Please add docker.io/natsio/prometheus-nats-exporter:0.20.1 to both _GLOBAL_IMAGE_DEFAULTS and IMAGE_OVERRIDE_KEYS, then extend the air-gap Helm-values test to verify it is rewritten to the configured registry.
| - http://localhost:8222/ | ||
| ports: | ||
| - containerPort: {{ .Values.nautobotNats.metrics.port | default 7777 }} | ||
| name: prom-metrics |
There was a problem hiding this comment.
We also need to allow this port in
deploy/helm/templates/network-policy.yaml;
otherwise Prometheus running in the configured monitoring namespace cannot scrape the exporter.
There was a problem hiding this comment.
Good catch — and it became a real gap after #126 merged, which turned the monitoring-namespace rule into an explicit TCP port allow-list (8000/8003/9000/9090/stork/9187). The exporter serves on nautobotNats.metrics.port (default 7777), which wasn't in that list, so Prometheus in a separate monitoring namespace would be dropped.
Fixed in bec2afc: added the exporter port to the allow-list, gated on nautobotNats.metrics.enabled so it only opens when the sidecar is deployed:
- protocol: TCP
port: 9187 # CNPG metrics exporter
{{- if .Values.nautobotNats.metrics.enabled }}
- protocol: TCP
port: {{ .Values.nautobotNats.metrics.port | default 7777 }} # nautobot NATS exporter
{{- end }}| # values.yaml, so only this overlay (and real deployments that opt in) run them. | ||
| # test_nats_exporter.py asserts the PodMonitor CR + exporter sidecar exist when | ||
| # OBSERVABILITY=true. | ||
| nautobotNats: |
There was a problem hiding this comment.
shoudl we should update the kind observability run to include the exporter as well?
There was a problem hiding this comment.
It already is — the observability kind run picks up the exporter without a workflow change.
The kind-integration.yml observability path doesn't helm -f values-observability.yaml directly; it sets infrastructure.monitoring.observability_enabled=true in ci-integration-test.yaml and runs make kind-up. The installer's deployer then layers this file in via a second -f:
# deployer.py
if self.config.infrastructure.monitoring.observability_enabled:
observability_values = Path(chart) / "values-observability.yaml"
...
helm_args.extend(["-f", str(observability_values)])and helm_values.py flips monitoring.podMonitors.enabled=true on observability. So with nautobotNats.metrics.enabled: true set here, both the exporter sidecar and the NATS PodMonitor render on the observability run.
Confirmed empirically: the observability kind run (observability=true) passed test_nats_exporter.py, which is a hard failure (not a skip) when OBSERVABILITY=true and asserts the PodMonitor CR + exporter sidecar exist.
| {{- include "nv-config-manager.containerSecurityContext" . | nindent 10 }} | ||
| args: | ||
| - -port={{ .Values.nautobotNats.metrics.port | default 7777 }} | ||
| - -connz |
There was a problem hiding this comment.
KEDA only requires -jsz=all. Plain -connz emits aggregate—not per-connection—metrics, but it still makes NATS enumerate connections on every scrape - do we plan to use these extra stats for dashboards?
There was a problem hiding this comment.
Agreed — we only use -jsz=all for KEDA (nats_consumer_num_pending), and there are no connection-level dashboards planned. Dropped -connz in 1561071, keeping -varz (cheap server-health) and -jsz=all, and updated the sidecar contract test's required args to match. This avoids the per-scrape connection enumeration you flagged.
…ewrite The prometheus-nats-exporter sidecar (nautobotNats.metrics) was captured by the air-gap bundler (it renders in values-airgapped-extract.yaml), but the installer never rewrote global.images.natsExporter.repository to the mirror registry because natsExporter was missing from its image tables. On an air-gapped cluster with metrics enabled the sidecar would fall back to the chart default (docker.io/natsio/prometheus-nats-exporter) and ImagePullBackOff. Add natsExporter to _GLOBAL_IMAGE_DEFAULTS (helm_values.py) and IMAGE_OVERRIDE_KEYS (schema.py), and extend the air-gap rewrite test to assert it maps to the configured registry. (per review on #159) Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
#126 turned the monitoring-namespace NetworkPolicy rule into an explicit port allow-list, but the prometheus-nats-exporter sidecar serves on nautobotNats.metrics.port (default 7777), which wasn't included — so with networkPolicy.enabled and Prometheus in a separate monitoring namespace, the exporter scrape would be dropped and nats_* metrics (incl. the KEDA nats_consumer_num_pending signal) would silently go missing. Add the exporter port to the allow-list, gated on nautobotNats.metrics.enabled so it only opens when the sidecar is deployed. (per review on #159) Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
KEDA render autoscaling scrapes nats_consumer_num_pending, emitted by -jsz=all. -connz made NATS enumerate every connection on each scrape for per-connection stats we don't dashboard. Drop it, keeping -varz (cheap server-health) and -jsz=all, and update the sidecar contract test's required args to match. (per review on #159) Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
|
/kind test |
|
Queued Kind integration for 1561071: https://github.com/NVIDIA/nv-config-manager/actions/workflows/kind-integration.yml |
…or override Guards the invariant that the installer's registry-override tables (NV_CONFIG_MANAGER_IMAGE_KEYS + _GLOBAL_IMAGE_DEFAULTS) stay in lockstep with the chart's global.images set. A chart image missing from the tables is left at its docker.io default and ImagePullBackOffs on registry/air-gapped installs — the gap that left prometheus-nats-exporter unmirrored. Fails at test time instead of deploy time if the two ever diverge. (per review on #159) Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
3 participants
Summary
Adds an opt-in Prometheus metrics path for the in-cluster nautobot NATS, so JetStream consumer backlog is scrapeable — the prerequisite for KEDA render autoscaling on render-queue depth.
prometheus-nats-exportersidecar on the nautobot NATS deployment, behindnautobotNats.metrics.enabled(defaultfalse). Runs with-jsz=all -connz -varz -prefix=nats, polling the NATS monitoring endpoint (:8222) and exposingnats_*metrics on:7777(prom-metricsport).PodMonitorgated onmonitoring.podMonitors.enabled AND nautobotNats.metrics.enabled, selecting the NATS pod and scraping theprom-metricsport.global.images.natsExporter→docker.io/natsio/prometheus-nats-exporter:0.15.0(same version already proven in cerebro).The key flag is
-jsz=all, which emitsnats_consumer_num_pending— the series a futurerenderService.autoscalingKEDA trigger scales on.Why gate the PodMonitor on
metrics.enabledRather than a standalone
podMonitors.nats.enabledtoggle, the PodMonitor is gated onnautobotNats.metrics.enabled(in addition tomonitoring.podMonitors.enabled) so you can never render a PodMonitor pointing at aprom-metricsport that doesn't exist.Blast radius
None by default. Both knobs default to
false;helm templatewith default values renders no sidecar and no PodMonitor — existing environments are byte-identical.Test plan
helm template … --values values-ci.yaml --set nautobotNats.metrics.enabled=true→ sidecar (natsio/prometheus-nats-exporter:0.15.0,-jsz=all,prom-metrics) + PodMonitor render with correct selector/port.helm template … --values values-ci.yaml(defaults) → zero exporter/PodMonitor resources.kiwi-qa01via a values overlay (nautobotNats.metrics.enabled=true,monitoring.podMonitors.enabled=true) and confirm Prometheus scrapesnats_consumer_num_pendingwith the expected labels.Note: enablement in the
kiwi-qa01overlay (+renderService.autoscaling) and the render scale-test are follow-ups; this PR is the reusable chart plumbing only.Summary by CodeRabbit
PodMonitorscraping the exporter’sprom-metricsport (/metrics) and supporting JetStream metrics, with consistent naming and pod selection safeguards.OBSERVABILITY=true, with stricter security validation.Kind Integration
Kind integration completed with success for
38ac20ein this workflow run.