PMM-14738 Validate PMM HA replica counts and publish the supported range - #936
Open
theTibi wants to merge 6 commits into
Open
PMM-14738 Validate PMM HA replica counts and publish the supported range#936theTibi wants to merge 6 commits into
theTibi wants to merge 6 commits into
Conversation
theTibi
requested review from
a team,
DhruthiKV,
egegunes,
eleo007,
gkech,
hors,
jvpasinatto,
mayankshah1607,
nmarukovich,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
August 27, 2026 15:37
theTibi
requested review from
4nte and
ademidoff
and removed request for
a team
August 27, 2026 15:37
This was referenced Aug 27, 2026
Raft elects by majority, so an even replica count needs more votes to elect a leader without surviving more failures, and replicas=2 survives none. HAProxy renders only maxReplicas server-template slots and marks a backend UP only when it answers /v1/server/leaderHealthCheck, so a replica above that ceiling is invisible to HAProxy - and if the Raft leader lands there, no backend is UP at all. Neither was checked or documented. pmm.replicas.validate and pmm.keeper.validate reject both, along with malformed input: sprig's int parses base 0, so "010" would have validated as 8, and anything wider than int64 overflows to 0. Also fixes pmm.haPeers and the HAProxy readiness probe, which built pod hostnames from .Release.Name while the StatefulSet is named after pmm.fullname. The two are equal only when the release name already contains the chart name, so any other release never formed a quorum. Release "pmm-ha" renders identically, so existing installs see no change. maxReplicas now has a single definition, pmm.maxReplicas, shared by the validator and the ConfigMap that consumes it. ci-values.yaml set clickhouse.keeper.replicas, a key nothing reads, so CI ran three Keepers instead of one. Covered by helm unittest suites under charts/pmm-ha/tests: 28 cases, no cluster required, runnable with make test-pmm-ha. Signed-off-by: theTibi <tkorocz@gmail.com>
The helm-unittest plugin.yaml now carries a platformHooks field that Helm 3.15 cannot parse, so the plugin installs but is unusable and make helm-unittest exits 1. test.yaml already runs its unittest jobs on azure/setup-helm@v5.0.0 with Helm v3.21.0; match it rather than pinning the plugin to an older release, since the Makefile target is shared with the pxc suites. Signed-off-by: theTibi <tkorocz@gmail.com>
percona#868 (PMM-15151) and percona#919 (PMM-13858) carry the same .Release.Name -> pmm.fullname fix for the Raft peer list. All three were already functionally identical; this PR differed only in where the $fullname assignment sat and in the wording of the comment, which made _helpers.tpl conflict four ways for no functional reason. Identical text on both sides of a merge resolves cleanly, so the three PRs no longer collide here whichever one lands first. tests/ha-peers_test.yaml still covers the fix. Signed-off-by: theTibi <tkorocz@gmail.com>
percona#937 (PMM-15394) removes templates/haproxy-init-configmap.yaml and the haproxy.initContainers readiness gate. This PR referenced it in three places, so merging both would have failed the ha-peers suite and left stale docs: - tests/ha-peers_test.yaml: dropped the template from the suite and the data["wait-for-pmm.sh"] assertion. - values.yaml, README.md: the HAProxy ConfigMap note now describes the one ConfigMap left after percona#937 - haproxy.cfg, which carries maxReplicas. After percona#937 only one value is embedded in an HAProxy ConfigMap, so the guidance gets simpler rather than weaker. Signed-off-by: theTibi <tkorocz@gmail.com>
percona#938 (PMM-15393) removes the required podAntiAffinity that capped haproxy.replicaCount at the worker-node count, so stating that cap here becomes wrong the moment percona#938 lands - and it conflicted with percona#938, which leaves this line at its original text and documents the new behaviour beside it. Restores the base wording. Signed-off-by: theTibi <tkorocz@gmail.com>
This was referenced Aug 28, 2026
Two conflicts, both from work that landed on PMM-HA-GA after this branch forked: - charts/pmm-ha/Chart.yaml: percona#937 bumped 1.6.1 -> 1.6.2 and appVersion to "3.9.1" while this branch bumped to 1.7.0. Kept 1.7.0 (still ahead of 1.6.2, and the render-breaking even-replicas check justifies the minor) with appVersion "3.9.1". - charts/pmm-ha/templates/haproxy-init-configmap.yaml: percona#937 (PMM-15394) deleted the file; this branch had fixed its pmm_host loop to use pmm.fullname. Took the deletion - the fix has nothing left to apply to. Nothing references the ConfigMap, the init container or wait-for-pmm.sh any more. Silent, non-conflicting overlaps checked by hand: - _helpers.tpl pmm.haPeers: percona#868 (PMM-15151) landed the same .Release.Name -> pmm.fullname fix, and this branch had already adopted that block verbatim, so the merge is a no-op there. The peer-addressing suite stays as a regression guard. - README.md: dropped the upgrade bullet announcing the PMM_HA_PEERS change. It ships in 1.6.2 via percona#868, so attributing it to 1.7.0 is now wrong. The even-replicas breaking change is unchanged. - ci/ci-values.yaml: percona#946 (PMM-15441) rewrote the file and already sets clickhouse.keeper.replicasCount, so this branch's fix for the ignored clickhouse.keeper.replicas key is subsumed. - The new examples/ and docs/SIZING.md from percona#935 set no PMM replicas value, so the new validators do not reject them. helm unittest: 3 suites, 27 tests passed. Renders verified against the merged tree - replicas=4, replicas=12 and clickhouse.keeper.replicasCount=2 all fail with the intended messages. Signed-off-by: theTibi <tkorocz@gmail.com>
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
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.
PMM-14738
Adds the validation and documentation for PMM HA scaling that the chart never had, plus the test suite that guards it.
Nothing checked the two values that can break a cluster
Rendered at
replicas1 through 12, everything was accepted silently:replicasreplicas=2survives none — one pod restart stops the cluster.maxReplicas. HAProxy rendersserver-template pmm 1-<maxReplicas>and fills the slots from a headless-service DNS answer in arbitrary order, then marks a backend UP only when it answers/v1/server/leaderHealthCheck. A pod without a slot is invisible to HAProxy — and if the Raft leader lands there, every backend is DOWN and PMM serves 503. Not a degraded pod: an outage.pmm.replicas.validateandpmm.keeper.validatenow reject both, plus malformed input. The regex is strict on purpose: sprig'sintparses base 0, so"010"would have validated as 8, and anything wider than int64 overflows to 0 — either way the error would quote a number nobody typed.Suggestions are clamped to the ceiling, so the error never names a value the next check rejects:
clickhouse.keeper.replicasCountis a Raft ensemble too.values.yamlhas always said it "should be odd"; that is now enforceable rather than advisory.Smaller fixes
maxReplicashad its| default 10duplicated between the validator andhaproxy-configmap.yaml. Both now readpmm.maxReplicas, so they cannot drift — and becausedefaulttreats 0 as empty,maxReplicas: 0used to become 10 silently.paths:filter did not include the workflow file, so a PR editing only its assertions ran no job.azure/setup-helmv4.2.0 / Helm 3.15.4 -> v5.0.0 / Helm 3.21.0, which is what thehelm-unittestplugin needs to load.Tests
charts/pmm-ha/tests/— 27 helm-unittest cases, no cluster required, runnable withmake test-pmm-ha:Scoping a suite to
templates/statefulset.yamlskips thelookupinpg-user-credentials-secrets.yamlentirely, so these do not depend on Helm's template ordering the way ahelm templateassertion would.Mutation-tested against the merged tree — every regression is caught:
pmm.haPeersto.Release.Nameserver-templateslot count to a literal10pmm.validate.oddCountalways rejectmaxReplicasceiling checkMerged with PMM-HA-GA
Merging the current base branch dropped two changes this PR originally carried, because equivalent work landed first:
PMM_HA_PEERSfix.pmm.haPeersbuilt peer hostnames from.Release.Namewhile the StatefulSet is named afterpmm.fullname, and the two are equal only when the release name already contains the chart name — sohelm install pmm-prod percona/pmm-hagave Raft peers that never resolved. PMM-15151: make PG and ClickHouse operators watch all namespaces #868 (PMM-15151) shipped the same fix, so there is nothing left to change here.tests/ha-peers_test.yamlstays as the regression guard, including a case asserting that a release namedpmm-hastill renders byte-identically.ci/ci-values.yaml. This PR removedclickhouse.keeper.replicas, a key nothing reads (the template readsreplicasCount). PMM-15441 Fit the pmm-ha CI install back onto the Kind node #946 (PMM-15441) rewrote the file and already setsreplicasCount.templates/haproxy-init-configmap.yamlwas deleted by #937 (PMM-15394), and this PR'spmm.fullnamefix to its host loop went with it. The chart version conflict resolved to1.7.0, keepingappVersion "3.9.1"from #937.Upgrade impact
Breaking for anyone running an even
replicas.2or4was accepted before and now fails the render; those users must set an odd value in the samehelm upgrade, which recreates the pods. The README carries an upgrade note.Chart
1.6.2->1.7.0. Reviewers: a case can be made for2.0.0— a^1.6.2consumer will otherwise adopt a render-breaking change automatically. Happy to bump if you prefer.Related