PMM-14738 Validate PMM HA replica counts and publish the supported range

.github/workflows/pmm-ha-pr-checks.yaml

@@ -4,6 +4,8 @@ on:
     paths:
       - 'charts/pmm-ha/**'
       - 'charts/pmm-ha-dependencies/**'
+      # Without this a PR that only edits the assertions below runs no job at all.
+      - '.github/workflows/pmm-ha-pr-checks.yaml'
 
 jobs:
   lint-test:
@@ -19,9 +21,9 @@ jobs:
             charts/pmm-ha-dependencies
 
       - name: Set up Helm
-        uses: azure/setup-helm@v4.2.0
+        uses: azure/setup-helm@v5.0.0
         with:
-          version: v3.15.4
+          version: v3.21.0
 
       - uses: azure/setup-kubectl@v4
 
@@ -70,6 +72,15 @@ jobs:
           assert_fails "$collision" --set nodeExporter.mode=openshift --set-string prometheus-node-exporter.enabled=false
           assert_fails 'nodeExporter.mode must be "internal" or "openshift"' --set nodeExporter.mode=bogus
 
+      # Value validation, peer addressing and the HAProxy server-template, asserted with
+      # helm unittest: no cluster, no dependency on Helm's template ordering, and runnable
+      # locally with `make test-pmm-ha`. Not gated on list-changed, so a PR that only edits
+      # these suites still runs them.
+      - name: Run helm unittest
+        run: |
+          make helm-unittest
+          make test-pmm-ha
+
       - name: Run chart-testing (list-changed)
         id: list-changed
         run: |

Makefile

@@ -25,7 +25,7 @@ helm-unittest:
 	$(HELM) plugin install https://github.com/helm-unittest/helm-unittest.git
 
 .PHONY: test
-test: test-pxc-operator test-pxc-db
+test: test-pxc-operator test-pxc-db test-pmm-ha
 
 .PHONY: test-pxc-operator
 test-pxc-operator:
@@ -34,3 +34,7 @@ test-pxc-operator:
 .PHONY: test-pxc-db
 test-pxc-db:
 	$(HELM) unittest charts/pxc-db
+
+.PHONY: test-pmm-ha
+test-pmm-ha:
+	$(HELM) unittest charts/pmm-ha

charts/pmm-ha/Chart.yaml

@@ -2,7 +2,7 @@ apiVersion: v2
 name: pmm-ha
 description: A Helm chart for Percona Monitoring and Management (PMM)
 type: application
-version: 1.6.2
+version: 1.7.0
 appVersion: "3.9.1"
 home: https://github.com/percona/pmm
 maintainers:

charts/pmm-ha/README.md

@@ -353,6 +353,8 @@ To create additional service tokens manually, see the [PMM documentation on serv
 
 | Name                              | Description                                                                                                                                    | Value                 |
 | --------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | --------------------- |
+| `replicas`                        | Number of PMM server replicas. Must be an odd number and no greater than `maxReplicas`; the chart fails the render otherwise                    | `3`                   |
+| `maxReplicas`                     | HAProxy `server-template` slots, and therefore the ceiling on `replicas`. Raising it also needs an HAProxy restart                              | `10`                  |
 | `service.name`                    | Service name that is dns name monitoring services would send data to. `monitoring-service` used by default by pmm-client in Percona operators. | `monitoring-service`  |
 | `service.type`                    | Kubernetes Service type                                                                                                                        | `ClusterIP`            |
 | `service.ports[0].port`           | https port number                                                                                                                              | `8443`                 |
@@ -393,6 +395,7 @@ To create additional service tokens manually, see the [PMM documentation on serv
 
 | Name                         | Description                                                                                                         | Value                 |
 | ---------------------------- | ------------------------------------------------------------------------------------------------------------------- | --------------------- |
+| `clickhouse.keeper.replicasCount` | Number of ClickHouse Keeper nodes. Must be odd; the chart fails the render otherwise | `3` |
 | `nameOverride`               | String to partially override common.names.fullname template with a string (will prepend the release name)           | `""`                  |
 | `extraLabels`                | Labels to add to all deployed objects                                                                               | `{}`                  |
 | `serviceAccount.create`      | Specifies whether a ServiceAccount should be created                                                                | `true`                |
@@ -783,12 +786,35 @@ haproxy:
 | MetalLB | IP from pool via `spec.loadBalancerIP` |
 ### Scaling and Monitoring
 
+#### Supported scaling range
+
+| Component | Value | Supported range | Enforced |
+|---|---|---|---|
+| PMM server | `replicas` | any odd value from `1` to `maxReplicas`; `3` (default) and `5` are what QA certifies | Yes — the chart fails the render |
+| HAProxy | `haproxy.replicaCount` | `1` up to the number of worker nodes | No — extra replicas stay `Pending` |
+| ClickHouse | `clickhouse.cluster.replicas` | `3` (default) or higher; scaling up is supported and shown below | No |
+| ClickHouse Keeper | `clickhouse.keeper.replicasCount` | any odd value; `3` is the default | Yes — the chart fails the render |
+| VictoriaMetrics | `victoriaMetrics.*.replicaCount` | defaults, or higher for larger fleets; scale up only | No |
+
+`replicas` is an availability knob, not a capacity knob: more PMM servers buy
+tolerance of more simultaneous failures, not more monitored nodes. To monitor a
+larger fleet, raise the per-component `resources` and storage rather than adding
+PMM replicas.
+
+These constraints are covered under [Known Limitations](#known-limitations).
+
+> **Upgrade note (chart 1.7.0)**
+>
+> An even `replicas` (`2` or `4`) was previously accepted and now fails the render.
+> Set an odd value in the same `helm upgrade`. That changes `PMM_HA_PEERS`, so it
+> recreates every PMM pod.
+
 #### Scaling PMM HA
 
 To scale the PMM HA deployment:
 
 ```sh
-# Scale PMM server replicas
+# Scale PMM server replicas (odd values only)
 helm upgrade pmm-ha --set replicas=5 --namespace pmm percona/pmm-ha
 
 # Scale HAProxy replicas
@@ -846,6 +872,50 @@ Common troubleshooting steps for PMM HA:
 
 ## Known Limitations
 
+### Scaling constraints
+
+- **`replicas` and `clickhouse.keeper.replicasCount` must be odd.** Both are Raft
+  ensembles, and Raft elects by majority: an even count needs more votes to elect a
+  leader without surviving more failures, and `2` survives none at all. The chart
+  rejects even values.
+- **`replicas` must not exceed `maxReplicas`** (default `10`). HAProxy renders only
+  `maxReplicas` `server-template` slots and fills them from a headless-service DNS
+  answer in arbitrary order, and it marks a backend UP only when that pod answers
+  `/v1/server/leaderHealthCheck`. A pod left without a slot is therefore invisible to
+  HAProxy, and if the Raft leader lands on it every backend is DOWN and PMM serves
+  `503`. The chart rejects this combination.
+- **Raising `maxReplicas` needs an HAProxy restart.** It is rendered into the
+  `pmm-ha-haproxy` ConfigMap, which the chart does not roll on upgrade. Routing itself
+  is DNS-based, so changing `replicas` needs no restart. Bump
+  `haproxy.podAnnotations."pmm.percona.com/config-version"` in the same `helm upgrade`
+  so the pods restart and pick up the new `server-template`:
+
+  ```sh
+  helm upgrade pmm-ha percona/pmm-ha --namespace pmm \
+    --set maxReplicas=20 \
+    --set-string 'haproxy.podAnnotations.pmm\.percona\.com/config-version=4'
+  ```
+
+  Or in `values.yaml`:
+
+  ```yaml
+  maxReplicas: 20
+  haproxy:
+    podAnnotations:
+      pmm.percona.com/config-version: "4"
+  ```
+
+  Prefer this over `kubectl rollout restart`: the bump is part of the same declarative
+  upgrade, so the restart is reproducible from the chart alone. An out-of-band restart
+  also picks up the new config, but a GitOps controller strips the `restartedAt`
+  annotation on its next sync and triggers a second, pointless rollout.
+- **`haproxy.replicaCount` cannot exceed the worker node count.** HAProxy pods use
+  required anti-affinity on `kubernetes.io/hostname`, so extra replicas stay
+  `Pending` and Helm still reports success. The same applies to the PostgreSQL
+  instances and pgBouncer.
+- **`victoriaMetrics.vmstorage.replicaCount` should not be scaled down.** Data is
+  sharded across vmstorage pods and is not migrated off a removed pod.
+
 ### Scaling Down to Single Replica
 
 When scaling down to a single PMM replica, ensure the **Raft leader is on pmm-0** before scaling. Kubernetes StatefulSets remove pods in reverse ordinal order (highest first), so:

charts/pmm-ha/templates/_helpers.tpl

@@ -303,6 +303,103 @@ Called from statefulset.yaml, which always renders.
 {{- end -}}
 {{- end -}}
 
+{{/*
+The number of HAProxy server-template slots, and therefore the ceiling on replicas.
+Shared by haproxy-configmap.yaml (which renders it) and pmm.replicas.validate (which
+enforces it) so the two can never disagree about the default.
+
+kindIs "invalid" rather than `default`, because sprig's `default` treats 0 as empty:
+with it, maxReplicas=0 would silently become 10 and the range check below could never
+see it.
+*/}}
+{{- define "pmm.maxReplicas" -}}
+{{- if kindIs "invalid" .Values.maxReplicas -}}10{{- else -}}{{- .Values.maxReplicas -}}{{- end -}}
+{{- end -}}
+
+{{/*
+Shared parity check for the chart's two Raft ensembles - PMM itself and ClickHouse
+Keeper. Raft elects a leader by majority, so an even count needs more votes to elect
+one without surviving more failures (4 tolerates a single loss, exactly like 3), and a
+count of 2 tolerates none at all.
+
+Takes a dict of:
+  name     - the values key, used verbatim in every message
+  value    - the raw value, validated before it is parsed
+  ceiling  - largest permitted value, or 0 for unbounded. The "use N instead" hint is
+             clamped to it so it never names a value a later check would reject.
+  ceilingName - the values key the ceiling comes from, so the hint can name it.
+
+The regex is deliberately strict. sprig's `int` parses base 0, so "010" would silently
+become 8; and anything wider than int64 overflows to 0. Either way the message would
+quote a number the user never typed, so both are rejected as malformed input instead.
+*/}}
+{{- define "pmm.validate.oddCount" -}}
+{{- $name := .name -}}
+{{- $raw := .value -}}
+{{- if not (regexMatch "^[1-9][0-9]{0,3}$" (toString $raw)) -}}
+{{- fail (printf "%s must be a whole number between 1 and 9999, got %v." $name $raw) -}}
+{{- end -}}
+{{- $n := int $raw -}}
+{{- if eq (mod $n 2) 0 -}}
+{{- $ceiling := int (.ceiling | default 0) -}}
+{{- $lower := sub $n 1 -}}
+{{- $upper := add $n 1 -}}
+{{- $hint := printf "Use %d or %d." $lower $upper -}}
+{{- if gt $ceiling 0 -}}
+{{- $maxOdd := $ceiling -}}
+{{- if eq (mod $ceiling 2) 0 -}}
+{{- $maxOdd = sub $ceiling 1 -}}
+{{- end -}}
+{{- if le $upper $maxOdd -}}
+{{- $hint = printf "Use %d or %d." $lower $upper -}}
+{{- else if le $lower $maxOdd -}}
+{{- $hint = printf "Use %d." $lower -}}
+{{- else -}}
+{{- $hint = printf "%s is %d, so the largest supported value is %d." (.ceilingName | default "The ceiling") $ceiling $maxOdd -}}
+{{- end -}}
+{{- end -}}
+{{- fail (printf "%s must be odd so Raft can form a quorum, got %d: an even count needs more votes to elect a leader without surviving more failures. %s" $name $n $hint) -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Fail-fast validation for the PMM replica count.
+Called from statefulset.yaml, which always renders and reaches these checks before the
+lookup in pg-user-credentials-secrets.yaml, so a plain `helm template` reports the real
+problem rather than a missing secret.
+
+HAProxy discovers PMM through a server-template with maxReplicas slots
+(haproxy-configmap.yaml), fills them from a headless-service DNS answer in arbitrary
+order, and marks a backend UP only when it answers /v1/server/leaderHealthCheck with
+200. Going above maxReplicas is therefore not merely under-routing: if the Raft leader
+lands on a pod that got no slot, every backend is DOWN and PMM serves 503.
+*/}}
+{{- define "pmm.replicas.validate" -}}
+{{- $maxRaw := include "pmm.maxReplicas" . -}}
+{{- if not (regexMatch "^([1-9][0-9]?|100)$" $maxRaw) -}}
+{{- fail (printf "maxReplicas must be a whole number between 1 and 100, got %v: it is rendered verbatim into the HAProxy server-template, and every slot is a backend server allocated at startup." $maxRaw) -}}
+{{- end -}}
+{{- $maxReplicas := int $maxRaw -}}
+{{- include "pmm.validate.oddCount" (dict "name" "replicas" "value" .Values.replicas "ceiling" $maxReplicas "ceilingName" "maxReplicas") -}}
+{{- $replicas := int .Values.replicas -}}
+{{- if gt $replicas $maxReplicas -}}
+{{- fail (printf "replicas (%d) exceeds maxReplicas (%d): HAProxy renders only %d server-template slots and fills them from DNS in arbitrary order, so a pod left without a slot is invisible to it. Because HAProxy marks a backend UP only when it answers /v1/server/leaderHealthCheck, a Raft leader on that pod leaves every backend DOWN and PMM serves 503. Lower replicas, or raise maxReplicas and bump haproxy.podAnnotations \"pmm.percona.com/config-version\" in the same upgrade so HAProxy restarts with the new server-template." $replicas $maxReplicas $maxReplicas) -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Fail-fast validation for the ClickHouse Keeper node count.
+Called from statefulset.yaml alongside the other value checks, for the same ordering
+reason described above.
+
+The parenthesised lookup matches pmm.nodeExporter.mode: without it, a nulled clickhouse
+or clickhouse.keeper key aborts with a raw Go nil-pointer error instead of the message
+this validator exists to produce.
+*/}}
+{{- define "pmm.keeper.validate" -}}
+{{- include "pmm.validate.oddCount" (dict "name" "clickhouse.keeper.replicasCount" "value" ((.Values.clickhouse).keeper).replicasCount) -}}
+{{- end -}}
+
 {{/*
 Target labels shared by both node-exporter scrape jobs. PMM's OS dashboards filter on node_name
 and node_type ("generic" is PMM's type for a bare host), so without these the node is invisible there.

charts/pmm-ha/templates/haproxy-configmap.yaml

@@ -59,5 +59,5 @@ data:
         # Use server-template for dynamic DNS-based discovery of PMM pods
         # This automatically discovers pods when scaling up/down without requiring HAProxy restart
         # HAProxy will re-resolve DNS based on 'hold valid' setting in the resolver (10s)
-        server-template pmm 1-{{ $.Values.maxReplicas | default 10 }} {{ $.Values.service.name | default "monitoring-service" }}.{{ $.Release.Namespace }}.svc.cluster.local:8443 check ssl verify none resolvers k8s init-addr last,libc,none
+        server-template pmm 1-{{ include "pmm.maxReplicas" $ }} {{ $.Values.service.name | default "monitoring-service" }}.{{ $.Release.Namespace }}.svc.cluster.local:8443 check ssl verify none resolvers k8s init-addr last,libc,none
 

charts/pmm-ha/templates/statefulset.yaml

@@ -1,4 +1,6 @@
 {{- include "pmm.nodeExporter.validate" . -}}
+{{- include "pmm.replicas.validate" . -}}
+{{- include "pmm.keeper.validate" . -}}
 apiVersion: apps/v1
 kind: StatefulSet
 metadata:

charts/pmm-ha/tests/ha-peers_test.yaml

@@ -0,0 +1,53 @@
+# PMM_HA_PEERS must address the StatefulSet's pods, which are named after pmm.fullname -
+# NOT after .Release.Name. The two are equal only when the release name already contains
+# the chart name, which is why every doc example and every ct-generated release name hides
+# a mismatch. Release "smoke" does not, so it exposes it.
+suite: pmm-ha peer addressing
+templates:
+  - templates/statefulset.yaml
+  - templates/configmap.yaml
+  - templates/clickhouse-datasource-secret.yaml
+release:
+  name: smoke
+  namespace: pmm
+tests:
+  - it: renders with a supported replica count
+    set: {replicas: 5}
+    asserts:
+      - notFailedTemplate: {}
+
+  - it: accepts every supported replica count
+    set: {replicas: 9, maxReplicas: 9}
+    asserts:
+      - notFailedTemplate: {}
+
+  - it: builds peers from pmm.fullname, not the release name
+    asserts:
+      - matchRegex:
+          path: spec.template.spec.containers[0].env[?(@.name=="PMM_HA_PEERS")].value
+          pattern: "^smoke-pmm-ha-0\\.monitoring-service\\.pmm\\.svc\\.cluster\\.local,"
+        template: templates/statefulset.yaml
+      - notMatchRegex:
+          path: spec.template.spec.containers[0].env[?(@.name=="PMM_HA_PEERS")].value
+          pattern: "(^|,)smoke-[0-9]+\\.monitoring-service"
+        template: templates/statefulset.yaml
+
+  - it: emits one peer per replica
+    set: {replicas: 5}
+    asserts:
+      - matchRegex:
+          path: spec.template.spec.containers[0].env[?(@.name=="PMM_HA_PEERS")].value
+          pattern: "smoke-pmm-ha-4\\.monitoring-service"
+        template: templates/statefulset.yaml
+
+  # A release already containing the chart name must be byte-identical to before the fix,
+  # so existing installs see no PMM_HA_PEERS change and therefore no pod recreation.
+  - it: leaves a release named pmm-ha unchanged
+    release:
+      name: pmm-ha
+      namespace: pmm
+    asserts:
+      - equal:
+          path: spec.template.spec.containers[0].env[?(@.name=="PMM_HA_PEERS")].value
+          value: "pmm-ha-0.monitoring-service.pmm.svc.cluster.local,pmm-ha-1.monitoring-service.pmm.svc.cluster.local,pmm-ha-2.monitoring-service.pmm.svc.cluster.local"
+        template: templates/statefulset.yaml

charts/pmm-ha/tests/haproxy-config_test.yaml

@@ -0,0 +1,36 @@
+# maxReplicas is the ceiling the validator enforces on replicas, so the ConfigMap that
+# consumes it must stay in step. Both sides read pmm.maxReplicas; this pins that they do.
+suite: pmm-ha HAProxy config
+templates:
+  - templates/haproxy-configmap.yaml
+release:
+  name: smoke
+  namespace: pmm
+tests:
+  - it: defaults to ten server-template slots
+    asserts:
+      - matchRegex:
+          path: data["haproxy.cfg"]
+          pattern: "server-template pmm 1-10 "
+
+  - it: follows an explicit maxReplicas
+    set: {maxReplicas: 7}
+    asserts:
+      - matchRegex:
+          path: data["haproxy.cfg"]
+          pattern: "server-template pmm 1-7 "
+
+  - it: follows maxReplicas at the ceiling
+    set: {maxReplicas: 100}
+    asserts:
+      - matchRegex:
+          path: data["haproxy.cfg"]
+          pattern: "server-template pmm 1-100 "
+
+  # Only the Raft leader answers this with 200, which is why a pod without a slot is not
+  # merely unrouted: if leadership lands there, no backend is UP at all.
+  - it: health-checks the leader endpoint
+    asserts:
+      - matchRegex:
+          path: data["haproxy.cfg"]
+          pattern: "/v1/server/leaderHealthCheck"