fix(redisdistributed): scope Redis alert rules to release namespace

[PiyushSingh-ZS]

Description

charts/redisdistributed/templates/alerts.yaml rendered a per-release PrometheusRule, but 6 of 7 alert expressions had no namespace selector — they evaluated redis_* metrics across every namespace in the cluster while stamping this release's namespace/service as static labels. An alert about another namespace's Redis was mislabeled as this release.

Two were outright broken in multi-tenant clusters:

• RedisTooManyMasters — count(redis_instance_info{role="master"}) > 1 counts every master in every namespace; with ≥2 scraped Redis instances it was permanently true (constant false positive).
• RedisMissingMaster — only dropped to 0 if every Redis master cluster-wide disappeared, so it never fired when a single release's master was actually gone (silent failure of a critical alert).

Fix

Added namespace="{{ .Release.Namespace }}" to every metric selector, matching what RedisDown already does:

Alert	Metric now scoped
RedisMissingMaster	redis_instance_info
RedisTooManyMasters	redis_instance_info
RedisDisconnectedSlaves	redis_connected_slaves (both count/sum)
RedisReplicationBroken	redis_connected_slaves
RedisClusterFlapping	redis_connected_slaves
RedisRejectedConnections	redis_rejected_connections_total

RedisDown was already correctly scoped and is unchanged.

Chart bumped 0.0.9 → v0.0.10, repackaged into docs/, and index.yaml regenerated.

Type of Change

• Bug fix
• New feature
• Documentation update
• Chart configuration update

Checklist

• I have performed a self-review of my code
• helm lint passes without errors
• helm template renders valid YAML with all exprs namespace-scoped
• My changes generate no new warnings
• Chart repackaged and repo index updated

Fixes #297

[arunesh-j]

Reviewed at 0d9c680. The diagnosis is correct and the direction is right. I found one issue with the change itself and two pre-existing problems that mean the fix cannot currently have any observable effect — plus a packaging hazard from the stale base.

The diagnosis is sound

Unscoped count(redis_instance_info{role="master"}) really does evaluate cluster-wide, and both consequences you call out are real: RedisTooManyMasters is permanently true once two Redis instances exist anywhere, and RedisMissingMaster only fires if every master cluster-wide disappears. Adding a namespace selector is a genuine improvement.

1. Namespace scoping is not release scoping — the two "outright broken" alerts are still broken

This is the main point about the change. Multiple releases in one namespace is the normal pattern here; the chart names everything {{ .Release.Name }}-redis-* precisely so that works. With two redisdistributed releases in the same namespace:

count(redis_instance_info{role="master", namespace="ns"}) > 1

Two releases, one master each → count = 2 → still permanently firing. And RedisMissingMaster still will not fire when one release's master is gone, because the other release's master holds the count at 1. That is the same failure you set out to fix, narrowed from cluster-wide to namespace-wide rather than resolved.

The correct pattern is already in this file — RedisDown, the one alert left unchanged:

redis_up{namespace="{{ .Release.Namespace }}", instance=~"{{ .Release.Name }}-redis-.*"} == 0

It scopes by namespace and release. The six changed alerts need the release-level selector too. Worth confirming which label actually carries release identity in your scrape config before copying instance=~ verbatim — for ServiceMonitor targets in a default Prometheus Operator setup instance is the address, so pod=~ or service= may be more reliable.

2. Pre-existing: the PrometheusRule is rejected by the API server

alerts.yaml labels all seven rules with service: {{ .Values.name }}, and name is not defined in charts/redisdistributed/values.yaml. All seven render null:

307:            service: 
319:            service: 
331:            service: 
...  (7 total)

The PrometheusRule CRD requires string label values, so on any cluster with the Prometheus Operator CRDs the object is rejected and helm install fails outright. Not introduced here, but it means none of these alerts exist to be fixed.

Same defect and same one-line fix that just landed for postgres in #305:

service: {{ .Values.name | default .Release.Name | quote }}

Since this PR already bumps the chart version and repackages, folding it in costs nothing and makes the rest of the change reachable.

3. Pre-existing: the ServiceMonitor matches no Service, so there are no redis_* metrics

ServiceMonitor selector: {'app': 'r-redisdistributed'}

Service r-redis-headless-service
   metadata.labels: {'app': 'r-redisdistributed-master', 'app.kubernetes.io/component': 'master'}
   ports: ['redis', 'metrics-port']
   -> SM match: False

The selector is {{ .Release.Name }}-redisdistributed; the only Service the chart renders is labelled …-redisdistributed-master. Zero targets, so nothing scrapes the redis-exporter sidecar (which is present, on port 2121), and every redis_* series these alerts reference is empty.

Separately worth noting: even once matched, that Service selects only master pods, so slave-side series would still be absent.

This is a bigger fix and reasonable to split into its own PR — but it should be filed, because these alerts are inert until it lands.

Packaging

Within the PR everything is self-consistent: version 0.0.9 → 0.0.10, helm lint clean, docs/redisdistributed-v0.0.10.tgz matches source, its index digest matches, and older versions are retained.

The problem is the base. The branch sits on 6918909, four commits behind main, and regenerating docs/index.yaml from that tree loses everything published since:

DROPPED by this PR (4):
   - jupyterhub v0.0.4
   - outline v0.0.9
   - superset v0.0.8
   - wordpress v0.0.8

Those are the "make the charts installable at all" releases from 9691853 and 9c6fbf5. GitHub already reports this as CONFLICTING, so a rebase is needed regardless — the important part is that the conflict must be resolved by re-running helm repo index . --url https://helm.zop.dev from docs/, not by keeping this side of the file. Otherwise those four tarballs stay in docs/ while helm.zop.dev stops serving them.

---

Requesting changes. The core ask is #1 — add release-level scoping so the two alerts this PR targets are actually fixed — plus resolving the rebase correctly. I would strongly suggest folding in #2, since it is a one-liner in the file you are already touching and nothing here works without it.

[arunesh-j]

This alert is still permanently firing after the fix. Namespace scoping narrows the blast radius but does not scope to a release.

Two redisdistributed releases in the same namespace — the normal pattern here, since the chart names everything {{ .Release.Name }}-redis-* — give one master each, so count(...) = 2 and this stays > 1 forever. Same constant false positive described in the PR body, just namespace-wide instead of cluster-wide.

RedisDown in this same file already shows the right shape:

redis_up{namespace="{{ .Release.Namespace }}", instance=~"{{ .Release.Name }}-redis-.*"} == 0

namespace and release. Worth checking which label really carries release identity in your scrape config before copying instance=~ across — for ServiceMonitor targets under a default Prometheus Operator setup instance is the address (IP:port), so pod=~ or service= may hold up better.

[arunesh-j]

Same gap as RedisTooManyMasters below, in the other direction: with a second release in the namespace holding a healthy master, count(...) stays at 1 and this never fires when this release's master is actually gone — the silent failure the PR body describes, still present after the change.

Needs the release-level selector alongside the namespace one.

[arunesh-j]

Pre-existing, but it blocks everything in this PR. Three lines below this (line 21, outside the diff so I cannot anchor there): service: {{ .Values.name }} — and name is not defined in charts/redisdistributed/values.yaml, so it renders a null label. All seven rules, verified by rendering:

307:            service: 
319:            service: 
331:            service: 
...  (7 total)

The PrometheusRule CRD requires label values to be strings, so on any cluster with the Prometheus Operator CRDs the API server rejects the object and helm install fails outright. None of these alerts exist to be fixed.

Same defect and same one-line fix that just landed for postgres in #305 — apply to all seven occurrences:

            service: {{ .Values.name | default .Release.Name | quote }}

You are already bumping the chart version and repackaging, so folding this in costs nothing and makes the rest of the change actually reachable.

[arunesh-j]

Adding the namespace matcher to both sides here is right. Separate from this PR, but worth a look while you are in this expression: count without (instance, job) (...) only collapses to one series per logical Redis if instance and job are the only labels that differ. ServiceMonitor targets also carry pod, service, endpoint and container, which would keep each pod in its own group and make the arithmetic (1 - N - 1 > 0) never true. Worth verifying against your real label set rather than taking the upstream awesome-prometheus-alerts form as-is.

[arunesh-j]

Version bump and packaging check out: helm lint clean, docs/redisdistributed-v0.0.10.tgz matches source, its index digest matches, and v0.0.9–v0.0.6 are retained.

⚠️ But docs/index.yaml was regenerated from a stale base and drops four chart versions. This branch sits on 6918909, four commits behind main:

DROPPED by this PR (4):
   - jupyterhub v0.0.4
   - outline v0.0.9
   - superset v0.0.8
   - wordpress v0.0.8

Those are the "make the charts installable at all" releases from 9691853 and 9c6fbf5. GitHub already reports this PR as CONFLICTING, so a rebase is required anyway — the part that matters is how the docs/index.yaml conflict is resolved. Please re-run helm repo index . --url https://helm.zop.dev from docs/ rather than keeping this side of the file, otherwise those four tarballs remain in docs/ while helm.zop.dev stops serving them, and CI will not catch it.