fix(truvami-monitoring)!: make Alertmanager optional and default it off - #70
Open
michaelbeutler wants to merge 4 commits into
Open
fix(truvami-monitoring)!: make Alertmanager optional and default it off#70michaelbeutler wants to merge 4 commits into
michaelbeutler wants to merge 4 commits into
Conversation
Alerting is handled by Grafana-managed alert rules, so this chart should not
be standing up an Alertmanager at all. It was doing so anyway.
Both truvami-dev and truvami-prod set `alertmanager.enabled: false`, yet all
four Alertmanager pods were running in prod. The Alertmanager CR lived in
templates/prometheus.yaml with no `if` guard, so the flag only ever gated the
AlertmanagerConfig and the receiver Secrets - never the Alertmanager itself.
That left the alerting path broken in a way that looked fine from the outside:
- Prometheus reported activeAlertmanagers: [] with ~200 dropped. Its
spec.alerting entry addresses Alertmanager by *Service* name, and no
template in this chart has ever created that Service.
- The Alertmanager CR referenced configSecret
"<fullname>-alertmanager-config", a Secret this chart also never creates.
prometheus-operator therefore fell back to its built-in default config,
whose root route sends everything to the "null" receiver.
- alertmanagerConfigSelector was unset, so the AlertmanagerConfig this chart
does render was never selected.
- The watchdog route matched alertname="Watchdog" exactly, but
rule-watchdog.yaml emits "TruvamiWatchdog", so the dead-man's-switch
heartbeat could never reach watchdog.webhookUrl.
Net effect in prod: alertmanager_alerts_received_total{status="firing"} = 0 on
both pods for all time, while 771 alerts were firing. A six-week Thanos
compactor halt and a 34-day block-upload outage both passed unnoticed.
Changes:
- Gate the Alertmanager CR on alertmanager.enabled, and omit the Prometheus
spec.alerting block when it is off, so Prometheus is never left pointed at
a Service that was not rendered.
- Default alertmanager.enabled to false. Grafana owns alerting.
- Add templates/alertmanager-service.yaml so the Prometheus reference
resolves when Alertmanager IS enabled. It selects on the `alertmanager`
label that prometheus-operator stamps with the CR name, so it matches only
this release - unlike the operator's namespace-wide alertmanager-operated.
- Replace the dangling configSecret with alertmanagerConfiguration pointing
at this chart's own AlertmanagerConfig, plus a configurable
alertmanagerConfigSelector.
- Fix the watchdog route matcher to TruvamiWatchdog.
- Expose alertmanager.replicas/portName/retention/configSelectorLabels.
BREAKING CHANGE: upgrading to 0.5.0 removes the Alertmanager StatefulSet and
its pods wherever alertmanager.enabled is false - which is both truvami-dev and
truvami-prod today. Those Alertmanagers have never delivered an alert, so
nothing is lost, but the pods will disappear. Set alertmanager.enabled: true to
keep them. Note ArgoCD prune is disabled on these apps, so the existing
Alertmanager CRs must be deleted by hand after rollout.
Verified: helm lint clean; server-side dry-run of the rendered Alertmanager and
Service against the live CRDs accepted; rendering prod values against 0.5.0
differs from 0.4.0 only by the removed Alertmanager CR, the removed alerting
block, and chart labels (61 -> 60 objects).
📝 WalkthroughWalkthroughThe chart release is updated to 0.5.0. Alertmanager is disabled by default, gains configurable runtime settings, renders conditionally with a Service, and is wired conditionally into Prometheus. The watchdog route now matches ChangesAlertmanager integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant Prometheus
participant AlertmanagerService
participant Alertmanager
Helm->>Prometheus: Render alerting targets when enabled
Helm->>AlertmanagerService: Render Service when enabled
Prometheus->>AlertmanagerService: Send alerts through configured port
AlertmanagerService->>Alertmanager: Forward traffic to Alertmanager pods
Alertmanager->>Alertmanager: Match TruvamiWatchdog route
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
📊 Chart DiagramVisual representation of Kubernetes resources in changed charts: truvami-monitoring (Before/After)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/truvami-monitoring/templates/prometheus.yaml`:
- Around line 142-144: Update the alertmanagerConfigSelector rendering to
preserve an explicitly configured empty alertmanager.configSelectorLabels map
instead of applying the default label selector. Distinguish an omitted value
from an explicit {} using the chart’s values/template logic, while retaining the
existing default selector when the key is absent and allowing the empty selector
to select every AlertmanagerConfig.
In `@charts/truvami-monitoring/values.yaml`:
- Around line 384-386: Update the Alertmanager upgrade comment near the
`enabled` configuration to state that, when ArgoCD pruning is disabled, setting
`enabled: false` stops rendering but does not remove the existing Alertmanager
CR or its owned pods, so operators must clean them up manually. Remove the
inaccurate claim that upgrading to chart 0.5.0 automatically removes them.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72ff751e-ee8c-4d32-8053-03b659aeca49
⛔ Files ignored due to path filters (2)
diagrams/truvami-monitoring.pngis excluded by!**/*.pngdiagrams/truvami-monitoring.previous.pngis excluded by!**/*.png
📒 Files selected for processing (6)
charts/truvami-monitoring/Chart.yamlcharts/truvami-monitoring/README.mdcharts/truvami-monitoring/templates/alertmanager-config.yamlcharts/truvami-monitoring/templates/alertmanager-service.yamlcharts/truvami-monitoring/templates/prometheus.yamlcharts/truvami-monitoring/values.yaml
…lector
Review follow-up. Two issues, both confirmed against the branch.
1. An explicitly configured `alertmanager.configSelectorLabels: {}` was silently
replaced by the default label, contradicting the documented behaviour that
`{}` selects every AlertmanagerConfig in the namespace.
`default` could not fix this - it treats {} as empty - and neither could
`hasKey`, because values.yaml declared the key, so it was always present.
Worse, because Helm merges map values, a non-empty default in values.yaml
could be added to but never reduced back to {}: a user setting
`{team: platform}` got the default label merged in alongside it.
The declared default is now null, with the real default expressed in the
template. Only null means "not configured", and Helm replaces a null base
with a user map wholesale. Verified via values files (not --set, which
merges and masks the bug):
omitted -> matchLabels: {truvami.com/alertmanager-config: "true"}
{} -> matchLabels: {}
{team: ...} -> matchLabels: {team: platform} (no default merged in)
2. The `alertmanager.enabled` comment claimed upgrading to 0.5.0 removes the
existing Alertmanager pods. It does not. The chart only stops rendering
them, and both dev-truvami-monitoring and prod-truvami-monitoring run with
`syncPolicy.automated.prune: false` (verified live), so ArgoCD leaves the
Alertmanager CR and its pods in place. The comment now says so and gives the
manual cleanup command.
Validated: helm lint clean; server-side dry-run of the empty-selector render
accepted by the live CRD; prod values render byte-identical to the previous
commit (these fixes are inert where alertmanager.enabled is false).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/truvami-monitoring/README.md`:
- Around line 20-21: Update the alertmanager.configSelectorLabels entry in the
chart README table to document its type as a map rather than a string, while
preserving the existing default value nil.
In `@charts/truvami-monitoring/templates/prometheus.yaml`:
- Around line 152-157: Update the alertmanagerConfigSelector logic around
$configSelector to accept only map values, including an explicitly empty {} map,
and call Helm fail for strings, lists, or any other non-map value before
rendering matchLabels. Preserve the existing default selector behavior when the
value is unset or invalid.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d55346b3-aaee-4c5d-b69c-493160dac215
📒 Files selected for processing (3)
charts/truvami-monitoring/README.mdcharts/truvami-monitoring/templates/prometheus.yamlcharts/truvami-monitoring/values.yaml
Comment on lines
+20
to
+21
| | alertmanager.configSelectorLabels | string | `nil` | | | ||
| | alertmanager.enabled | bool | `false` | | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document configSelectorLabels as a map, not a string.
The value accepts nil, {}, or a label map. Documenting it as string conflicts with values.yaml and can lead users to render an invalid matchLabels value.
Proposed documentation fix
-| alertmanager.configSelectorLabels | string | `nil` | |
+| alertmanager.configSelectorLabels | object | `nil` | |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | alertmanager.configSelectorLabels | string | `nil` | | | |
| | alertmanager.enabled | bool | `false` | | | |
| | alertmanager.configSelectorLabels | object | `nil` | | | |
| | alertmanager.enabled | bool | `false` | | |
🤖 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 `@charts/truvami-monitoring/README.md` around lines 20 - 21, Update the
alertmanager.configSelectorLabels entry in the chart README table to document
its type as a map rather than a string, while preserving the existing default
value nil.
Comment on lines
+152
to
+157
| {{- $configSelector := .Values.alertmanager.configSelectorLabels }} | ||
| {{- if kindIs "invalid" $configSelector }} | ||
| {{- $configSelector = dict "truvami.com/alertmanager-config" "true" }} | ||
| {{- end }} | ||
| alertmanagerConfigSelector: | ||
| matchLabels: {{- toYaml $configSelector | nindent 6 }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
values="$(mktemp)"
trap 'rm -f "$values"' EXIT
cat >"$values" <<'EOF'
alertmanager:
enabled: true
configSelectorLabels: invalid
EOF
if helm template review charts/truvami-monitoring -f "$values" >/dev/null 2>&1; then
echo "Non-map configSelectorLabels was accepted" >&2
exit 1
fiRepository: truvami/helm
Length of output: 150
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate the relevant chart files and show the surrounding template code.
git ls-files charts/truvami-monitoring | sed -n '1,200p'
printf '\n--- template slice ---\n'
sed -n '130,190p' charts/truvami-monitoring/templates/prometheus.yaml
printf '\n--- values references ---\n'
rg -n "configSelectorLabels|alertmanager\.configSelectorLabels|alertmanager:" charts/truvami-monitoring -nRepository: truvami/helm
Length of output: 6267
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Probe rendering behavior for several selector shapes to see what actually fails.
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cases=(
"null|alertmanager:\n enabled: true\n configSelectorLabels: null\n"
"empty-map|alertmanager:\n enabled: true\n configSelectorLabels: {}\n"
"string|alertmanager:\n enabled: true\n configSelectorLabels: invalid\n"
"list|alertmanager:\n enabled: true\n configSelectorLabels:\n - a\n - b\n"
)
for item in "${cases[@]}"; do
name="${item%%|*}"
body="${item#*|}"
file="$tmpdir/$name.yaml"
printf '%b' "$body" >"$file"
printf '\n== %s ==\n' "$name"
if out="$(helm template review charts/truvami-monitoring -f "$file" 2>&1)"; then
echo "OK"
printf '%s\n' "$out" | sed -n '1,40p'
else
echo "FAIL"
printf '%s\n' "$out" | sed -n '1,80p'
fi
doneRepository: truvami/helm
Length of output: 381
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect docs/defaults to see whether the field is meant to be a map or string.
rg -n "configSelectorLabels|matchLabels|AlertmanagerConfig" README.md charts/truvami-monitoring -g '!**/Chart.lock'Repository: truvami/helm
Length of output: 2954
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Show any chart schema or template-side validation relevant to this field.
for f in charts/truvami-monitoring/values.schema.json charts/truvami-monitoring/values.yaml charts/truvami-monitoring/templates/prometheus.yaml; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,240p' "$f" | sed -n '/configSelectorLabels/,+40p'
fi
doneRepository: truvami/helm
Length of output: 839
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "--- values.schema.json ---"
cat -n charts/truvami-monitoring/values.schema.json
echo
echo "--- values.yaml around configSelectorLabels ---"
sed -n '404,428p' charts/truvami-monitoring/values.yaml
echo
echo "--- README table entry ---"
sed -n '14,26p' charts/truvami-monitoring/README.mdRepository: truvami/helm
Length of output: 2105
Reject non-map alertmanager.configSelectorLabels values.
matchLabels must be a map; strings or lists will render an invalid Alertmanager CR. Keep {} working, but fail on anything else.
🤖 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 `@charts/truvami-monitoring/templates/prometheus.yaml` around lines 152 - 157,
Update the alertmanagerConfigSelector logic around $configSelector to accept
only map values, including an explicitly empty {} map, and call Helm fail for
strings, lists, or any other non-map value before rendering matchLabels.
Preserve the existing default selector behavior when the value is unset or
invalid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Alerting is handled by Grafana-managed alert rules, so this chart should not be standing up an Alertmanager at all. It was doing so anyway.
Both
truvami-devandtruvami-prodsetalertmanager.enabled: false, yet all four Alertmanager pods were running in prod. The Alertmanager CR lived insidetemplates/prometheus.yamlwith noifguard — the flag only ever gated theAlertmanagerConfigand the receiver Secrets, never the Alertmanager itself.That left the alerting path broken in a way that looked healthy from the outside. Four independent breaks, all in this chart:
spec.alertingaddresses Alertmanager by Service name; no template ever created that ServiceactiveAlertmanagers: []with ~200 droppedconfigSecret: <fullname>-alertmanager-config, which no template createsnullreceiveralertmanagerConfigSelectorunsetAlertmanagerConfigthis chart does render was never selectedalertname="Watchdog"exactly, butrule-watchdog.yamlemitsTruvamiWatchdogwatchdog.webhookUrlNet effect in prod:
alertmanager_alerts_received_total{status="firing"} = 0on both pods for all time, while 771 alerts were firing. A six-week Thanos compactor halt and a 34-day block-upload outage both passed unnoticed.What changed
alertmanager.enabled, and omit the Prometheusspec.alertingblock when it is off — so Prometheus is never left pointed at a Service that was not rendered.alertmanager.enabledtofalse. Grafana owns alerting.templates/alertmanager-service.yamlso the Prometheus reference resolves when Alertmanager is enabled. It selects on thealertmanagerlabel that prometheus-operator stamps with the CR name, so it matches only this release — unlike the operator's namespace-widealertmanager-operated, which would fan alerts out to every Alertmanager in the namespace.configSecretwithalertmanagerConfigurationpointing at this chart's ownAlertmanagerConfig, plus a configurablealertmanagerConfigSelector.TruvamiWatchdog.alertmanager.replicas/portName/retention/configSelectorLabels.Upgrading to
0.5.0removes the Alertmanager StatefulSet and its pods whereveralertmanager.enabledis false — which is bothtruvami-devandtruvami-prodtoday.Those Alertmanagers have never delivered a single alert, so nothing is lost, but the pods will disappear. Set
alertmanager.enabled: trueto keep them.ArgoCD
pruneis disabled on these apps, so the existing Alertmanager CRs will not be garbage-collected automatically and must be deleted by hand after rollout:Verification
helm lintclean; all pre-commit hooks pass (Helm Docs, Chart Version Check, Values Schema Check).Alertmanager+Serviceagainst the live CRDs onsbcc-cluster-7401— accepted, soalertmanagerConfiguration/alertmanagerConfigSelectorare valid for the installed operator.0.5.0differs from0.4.0by exactly: the removed Alertmanager CR, the removedalerting:block, and chart labels. 61 → 60 objects.--set alertmanager.enabled=trueproduces a complete, self-consistent alerting stack (CR + Service + Config, noconfigSecret).Rollout note
This does not deploy on merge — prod is pinned to
targetRevision: 0.4.0insbckube. Bumping that pin is a separate, deliberate step.Context
Found while diagnosing the 2026-07 monitoring outage. The root cause of that outage is separate — a label-cardinality bug in
truvami-bridge(topic="truvami-uplinks[0]@<offset>", 749,844 distinct values, 83% of head series). This PR fixes why nobody was told about it.Summary by CodeRabbit
enabledvalue.Servicefor stable internal access when enabled, and updated Prometheus to reference Alertmanager only when applicable.alertname.