PMM-14989 Retain state on client pod restart - #954
Open
ademidoff wants to merge 21 commits into
Open
Conversation
…ashboard PMM-13860: Add PMM_HA_NAMESPACE to statefulset
PMM-15309 Add read-only ClickHouse data source user
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: ademidoff <81549+ademidoff@users.noreply.github.com>
The pmm-client StatefulSet added in PMM-14665 (#913) has no override in ci/ci-values.yaml, so CI runs 3 Client pods at 100m CPU each. On the single-node Kind cluster that leaves nothing for the third PMM replica: pmm-ha-<release>-2 stays Pending with "Insufficient cpu" and the install hits the 600s timeout. Cap the Clients at 1 and shrink their requests. Also fix clickhouse.keeper.replicas, a key the chart never reads - it is replicasCount - so CI has been running 3 Keepers instead of 1 since January. Together this frees ~450m of requests; the stuck replica needs 200m. Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151: make PG and ClickHouse operators watch all namespaces
The pmm-ha-dependencies chart installed the PostgreSQL (Percona PGO) and
ClickHouse (Altinity) operators scoped to their own namespace, so they did
not reconcile PerconaPGCluster / ClickHouseInstallation CRs created in other
namespaces. Installing pmm-ha into a second namespace (e.g. a DR restore
target) left PostgreSQL and ClickHouse with no pods, while only
VictoriaMetrics came up — its operator is already cluster-scoped.
- pg-operator: set watchAllNamespaces=true. This renders WATCH_NAMESPACE=""
(all namespaces) and switches the operator RBAC from a namespaced
Role/RoleBinding to ClusterRole/ClusterRoleBinding.
- altinity-clickhouse-operator: RBAC is already cluster-wide, but the
operator only watched its own namespace; set WATCH_NAMESPACES=.* so it
watches every namespace. (An empty watch list, documented as "all", did
not take effect in 0.25.4.)
Lets a DR/secondary namespace be added without manual operator edits.
Validated by upgrading pmm-operators with these values: the PG and
ClickHouse operators then reconciled CRs in a second namespace, with no
disruption to the existing namespace.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151: fix PMM-HA peer DNS to use the StatefulSet name, not the release name
The pmm.haPeers helper built the HA memberlist peer FQDNs from .Release.Name
(<release>-<ordinal>.monitoring-service...), but the PMM pods are named after the
StatefulSet (pmm.fullname): <release>-pmm-ha-<ordinal>. These only coincide when the
release is named "pmm-ha". With any other release name (e.g. a DR install named
"pmm-dr"), every peer name resolved to a non-existent host, the memberlist could not
form, and pmm-managed panicked on startup ("cannot start high availability service:
failed to join memberlist cluster ... no such host") — so PMM-HA never came up.
Build the peers from include "pmm.fullname" so they match the actual pod hostnames for
any release name. No change for a release named "pmm-ha" (fullname == release name).
Part of multi-namespace support: lets pmm-ha be installed under any release name (e.g. a
secondary/DR namespace) with a working HA cluster.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151: fix HAProxy wait-for-pmm-ready to use the StatefulSet name, not the release name
The HAProxy init script (haproxy-init-configmap.yaml) probes each PMM instance at
<release>-<ordinal>.monitoring-service..., but the PMM pods are named after the StatefulSet
(pmm.fullname): <release>-pmm-ha-<ordinal>. As with pmm.haPeers, these only coincide when the
release is named "pmm-ha". With any other release name the probe targets non-existent hosts
(HTTPS status 000), wait-for-pmm-ready never passes, and HAProxy never starts — even though PMM
itself is healthy.
Build pmm_host from include "pmm.fullname" so it matches the actual pod hostnames for any
release name. Companion to the pmm.haPeers fix; completes multi-namespace support for PMM-HA.
Signed-off-by: theTibi <tkorocz@gmail.com>
* Enhance multi-namespace support for ClickHouse and PostgreSQL operators in PMM HA dependencies
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151 Match the ClickHouse scrape jobs to the CR names
The ClickHouse and Keeper scrape jobs matched the pod's chi/chk label against
pmm.fullname, but clickhouse-cluster.yaml and clickhouse-keeper.yaml name those CRs
from Release.Name. The two are equal only when the release name already contains the
chart name, so with the distinct release name this branch enables (e.g. "pmm-2" ->
CHI "pmm-2", fullname "pmm-2-pmm-ha") neither job matched anything and ClickHouse and
Keeper metrics were silently dropped.
Match on Release.Name, which is what the label actually carries. This is the same
naming split the haPeers fix addresses, in the opposite direction - the rest of the
ClickHouse wiring (sticky Service selector, pmm.clickhouse.nodes,
pmm.clickhouse.keeper.nodes) already uses Release.Name consistently.
Renaming the CRs to pmm.fullname instead would recreate the ClickHouse cluster on
existing releases, so the regexes move rather than the resources.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151 Document the privilege change and the operator upgrade path
- Privileges: the cluster-wide RBAC requirement applies to every install, not only
multi-namespace ones, and fails outright where cluster-scoped RBAC cannot be created.
Spell out the single-namespace opt-out instead of alluding to it.
- Existing installs: the cluster-wide watch is a values default, so operators installed
before this version stay namespace-scoped until helm upgrade. Add the upgrade command,
note that reinstalling the chart into the new namespace fails on VM CRD ownership, and
warn that installing pmm-ha first is not recoverable by retrying - PMM boots without
PostgreSQL and Grafana then keeps the default admin password.
Signed-off-by: theTibi <tkorocz@gmail.com>
* Fix helm upgrade command for pmm-ha-operators
* PMM-15151 Address review feedback on multi-namespace support
Templates:
- Escape the interpolated vmagent scrape-config regexes with regexQuoteMeta.
They are fully-anchored RE2 and Helm permits "." in release names, so
release "pmm.ha" rendered regex 'pmm.ha' and could keep a different CHI's
pods. Applied to all seven, not only the two this PR already touched.
- Move the chart-maintainer notes out of the inlineScrapeConfig block scalar
into {{- /* */}}, matching _helpers.tpl. They were shipping inside the
VMAgent CR, so helm upgrade rewrote the spec and made the operator
regenerate the scrape-config Secret and reload every vmagent replica.
- Bump haproxy config-version to "4". The init-script ConfigMap fix in this
PR is not covered by any checksum, so without the bump helm upgrade
reported success while running pods kept the pre-upgrade script.
Docs:
- Rewrite the multi-namespace section around the real rule: one pmm-ha per
namespace (six namespaced objects have fixed names), each with a distinct
release name. Note that every instance's kube-state-metrics reads
cluster-wide, so namespaces are not a tenancy boundary. Drop the install
example, which could not render because pmm-secret must pre-exist, and
which duplicated the worked example in the pmm-ha chart README.
- Use kubectl rollout status instead of a readiness wait when upgrading the
operators. helm upgrade runs without --wait and this is an env-only
change, so the readiness wait matched the still-Ready pre-upgrade pod.
- Correct the "order matters" box. An unreconciled ClickHouseInstallation
parks the PMM pod at Init:1/2 on wait-for-clickhouse, which loops with no
timeout, so Grafana never initializes and the default admin password
state described previously cannot occur.
- Warn that uninstalling this chart stops reconciliation in every namespace,
and that retained CRDs block reinstalling it into a different namespace.
- Rename the remaining pmm-operators references to pmm-ha-operators.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151 Fix instance discovery and tenancy caveats in docs
Address the remaining review comments on the multi-namespace section:
- The pre-uninstall check used `helm list -A -f pmm-ha`, but `-f` is a regex
over the release name, not the chart. It misses any instance not named
"pmm-ha" - which is exactly the convention this document establishes - and
returns a plausible non-empty list, so the reader concludes there are no
other instances and proceeds with the uninstall. Filter on the chart column
instead, anchored with `[0-9]` so it does not also match
pmm-ha-dependencies. Same fix in prose form in NOTES.txt.
- Drop the forward reference to the worked two-namespace example, which lives
in #865 and is not on this branch. The host port 9100 constraint is now
stated inline, and only the pmm-secret pointer goes to the pmm-ha README,
where that section actually exists. The two PRs can now merge in either
order.
- Extend the "namespaces are not a tenancy boundary" note to cover RBAC. Each
instance's ServiceAccount holds cluster-wide read/delete on Secrets, so
instances are not isolated from each other's credentials. The ClusterRole
predates this PR and is unchanged here; what changed is that this section
now recommends the configuration that makes the grant reachable.
Docs only - no templates, values or RBAC touched.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151 Address Copilot review feedback
- Upgrade docs: tell existing installs to use their own release name (earlier
docs used pmm-operators, so `helm upgrade pmm-ha-operators` aborts with
`release: not found` for exactly the audience that section targets).
- Correct the rollout explanation: the upgrade also swaps pg-operator's
namespaced Role/RoleBinding for cluster-scoped ones, and both the pg and
ClickHouse operators use `strategy: Recreate`, so the false green comes from
the pre-upgrade pod still being Ready while Terminating, not from an overlap
window. Only victoria-metrics-operator rolls with overlap.
- NOTES.txt: derive the SCOPE message from the values instead of always
claiming cluster-wide, so a narrowed install (pg-operator.watchAllNamespaces
false, or WATCH_NAMESPACES other than ".*") is reported as such.
- Bump pmm-ha-dependencies to 1.1.0: chart-releaser runs with skip_existing
and 1.0.0 is already published, so the changed defaults and RBAC would
silently never be published.
- CI: add a render check for the release-name-sensitive names this PR fixes.
`ct install` generates a pmm-ha-prefixed release name, so pmm.fullname
equals Release.Name there and the whole bug class is invisible. Renders as
release `pmm-2` and asserts the peer list, HAProxy hostnames, CH/Keeper
scrape selectors and CHI name, plus `pmm.ha` -> `regex: 'pmm\.ha'`.
Needs --dry-run=server because pg-user-credentials-secrets.yaml does an
unconditional lookup+fail on pmm-secret.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15151 Clarify the release-name requirement for multi-namespace installs
The multi-namespace section said only that a distinct release name "flows
into the correct pod/peer DNS", which reads as if the name itself has to
follow a pattern. QA hit this and asked whether a release name not
containing the chart name (e.g. "pmm-shruti") is supposed to work.
It is: pod and peer DNS are derived from pmm.fullname, not Release.Name,
so any name works and uniqueness in the cluster is the only requirement.
Say that explicitly, with an example.
Signed-off-by: theTibi <tkorocz@gmail.com>
---------
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15149: support installing PMM-HA into multiple namespaces The cluster-wide monitoring sub-charts (kube-state-metrics, prometheus-node-exporter) are already conditional on main, which makes it possible to run a second PMM-HA instance in another namespace (e.g. a DR / restore target or an isolated test instance) by disabling them and using a distinct Helm release name. This closes the remaining gaps so that path is clean and documented: - vmagent: gate the kube-state-metrics and node-exporter scrape jobs on the same `kube-state-metrics.enabled` / `prometheus-node-exporter.enabled` toggles. They were rendered unconditionally, so disabling the sub-charts left dangling scrape configs with no targets. Other jobs (kubelet, cadvisor, kube-apiserver, and the PMM components) are untouched. - README: add an "Installing into multiple namespaces" section - operators installed once (cluster-wide), a distinct release name per instance, and monitoring sub-charts disabled on secondary instances (node-exporter uses hostNetwork:9100, so only one set can run per node), plus per-namespace prerequisites (pmm-secret, backup-storage access). - .gitignore: ignore local-only value overrides (values-dev.yaml, values-*.local.yaml) so per-environment files containing real bucket/account/IRSA details are never committed. Backward compatible: both monitoring sub-charts default to enabled, so existing single-namespace installs are unchanged. Verified with `helm template` (default keeps the jobs; disabled drops only the two gated jobs) and `helm lint`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: theTibi <tkorocz@gmail.com> * Apply suggestion from @ademidoff Co-authored-by: Alex Demidoff <a@demidoff.me> Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149: correct multi-namespace monitoring docs (review feedback) Disabling kube-state-metrics / prometheus-node-exporter on secondary instances does NOT reuse the first instance's agents: each vmagent's KSM/node-exporter scrape jobs are scoped to namespaces.names=[.Release.Namespace], so a secondary instance collects no kube-state-metrics or node-exporter metrics at all (those live only in the first instance's VictoriaMetrics). Reword the section + code-block comment to say so; note kubelet/cadvisor (role: node) still provide basic node/container metrics, and that this is fine for a DR target. Addresses review comments on PR #865. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149: document namespace creation in multi-namespace install example Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149 Gate the kube-state-metrics scrape job via a bundledEnabled helper Mirrors pmm.nodeExporter.bundledEnabled from PMM-15070 for kube-state-metrics, so both gates follow Helm's `condition:` semantics: only a boolean false disables the subchart, while null / non-bool `enabled` values leave it enabled (Helm renders the subchart in those cases, so the scrape job must stay too). Replaces the hand-rolled `index .Values "kube-state-metrics" "enabled"` form. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149 Note OpenShift node-exporter mode in the multi-namespace section With nodeExporter.mode=openshift the scrape job targets the platform exporter in openshift-monitoring rather than the release namespace, so every instance still collects node metrics - only kube-state-metrics stays first-instance-only. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149 Point the multi-namespace docs at the dependencies chart The operator side (install once, cluster-wide watch) and the distinct-release-name requirement are documented in pmm-ha-dependencies' "Multi-namespace support" section (PMM-15151), so link there instead of restating them. This section keeps only the per-instance settings that belong to the pmm-ha chart. Also splits the monitoring sub-chart guidance, which conflated two different cases: - prometheus-node-exporter must be disabled on additional instances - it uses host networking on port 9100, so a second DaemonSet's pods cannot start. - kube-state-metrics is optional. Its resources are named after the release (pmm-dr-kube-state-metrics), so a second instance can run its own copy without colliding, and collect real object-state metrics rather than none. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149 Widen the local value-override ignore pattern values-dev.yaml matched only that exact basename, so the per-environment files this is meant to catch - values-dev-demo2.yaml, values-dev-eks.yaml - were still tracked. values-dev*.yaml covers them; the charts' own values.yaml is unaffected. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15149 Note the required node-exporter toggle in OpenShift mode Signed-off-by: theTibi <tkorocz@gmail.com> --------- Signed-off-by: theTibi <tkorocz@gmail.com> Co-authored-by: Alex Demidoff <a@demidoff.me>
* PMM-14403 Finalize the HA resource limitations
Sizes the chart's defaults for ~100 monitored nodes at 30-day retention and
gives every container explicit requests and limits. Several components had
none at all: the PostgreSQL instances, pgBouncer, the PMM client sidecar,
kube-state-metrics and node-exporter all ran as BestEffort, making the pods
that hold PMM's inventory, Grafana state and alert rules the first thing the
kubelet evicts under node pressure. HAProxy carries every client
remote_write, QAN payload and UI request, and inherited a request but no
limit from its subchart.
Adds dataRetentionDays, which drives both vmstorage's -retentionPeriod and
PMM's PMM_DATA_RETENTION. The two were previously independent: vmstorage kept
90 days while Query Analytics silently expired on PMM's own 30-day default.
An explicit victoriaMetrics.vmstorage.retentionPeriod still wins, so installs
already pinning it keep their behaviour.
ClickHouse storage moves from 20Gi to 50Gi per replica. Each replica holds a
full copy of Query Analytics, and 20Gi does not cover 100 services at 30-day
retention once ClickHouse's own system.* tables are accounted for. This is a
PVC expansion on upgrade and needs a storage class with allowVolumeExpansion.
Documents the model behind the numbers in docs/SIZING.md, along with ready
profiles for 500 and 1000 nodes in examples/. The sizing constants there are
measured from a running cluster rather than estimated, and the document shows
how to re-measure them against your own workload.
Verified with helm lint and helm template against the default values, both
example profiles and ci/ci-values.yaml.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-14403 Make dataRetentionDays opt-in so upgrades never shorten retention
Defaulting dataRetentionDays to 30 shortened metrics retention from 90 to 30
days on any install that had taken the chart default, deleting TSDB data that
does not come back. The commit message claimed only installs explicitly
pinning victoriaMetrics.vmstorage.retentionPeriod were affected, which was
wrong.
Two things about how retention works in HA make the single-value design worse
than it looked:
* PMM_DATA_RETENTION governs Query Analytics only. The built-in
VictoriaMetrics that would otherwise consume it does not run in an HA
deployment - supervisord starts only qan-api2, which prunes ClickHouse.
Metrics retention belongs entirely to the operator.
* Setting PMM_DATA_RETENTION locks the setting. PMM's ChangeSettings
returns FailedPrecondition for any retention change while the variable is
present, so the chart was silently removing a control users had in the UI.
dataRetentionDays is now empty by default. Left empty the chart does not touch
either mechanism: vmstorage keeps victoriaMetrics.vmstorage.retentionPeriod,
restored to its original "90d", and PMM_DATA_RETENTION is not emitted, so
Query Analytics stays on PMM's own setting and remains adjustable from the UI.
Upgrades are a no-op. Set it and it drives both, taking precedence over
vmstorage.retentionPeriod, which is what a declaratively managed deployment
wants.
Documents the split in SIZING.md, including that the default 50Gi vmstorage
volume holds roughly 30 days at 100 nodes rather than the 90 the default
retention asks for. Neither default is changed to resolve that, because one
choice deletes data and the other forces a PVC expansion on every install.
Verified across six configurations: defaults, dataRetentionDays at 30/45/90,
an explicitly pinned retentionPeriod, and both set together.
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-14403 Keep the CI install within the single-node kind budget
ct install failed with "0/1 nodes are available: 1 Insufficient cpu" - two of
the three PMM pods stayed Pending. ci-values.yaml shrinks resources to fit the
single-node kind cluster, but it did not cover the components this branch gave
requests to for the first time, so those requests landed at their production
defaults:
vmagent 2 x 250m (never overridden, previously 2 x 100m)
kube-state-metrics 100m (previously BestEffort)
node-exporter 50m (previously BestEffort)
PG PMM sidecar 100m (previously BestEffort)
That is +550m against a base branch that was already at 2.75 of roughly 4
available CPU.
Adds CI overrides for those four, plus a 5Gi ClickHouse volume now that the
default is 50Gi. Also fixes clickhouse.keeper.replicas, which is not a key the
chart reads - the value is replicasCount, so CI has been running three Keepers
where it meant to run one.
Request totals rendered from ci-values.yaml:
base branch (CI green) 2.75 CPU 7.28 Gi
this branch, before 3.30 CPU 8.09 Gi
this branch, after 2.49 CPU 6.06 Gi
Signed-off-by: theTibi <tkorocz@gmail.com>
---------
Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15394 Let HAProxy start without waiting for every PMM replica The HAProxy pod ran an init container that required every PMM replica to answer HTTP 200 before HAProxy could start, and exited 1 after five minutes - so the pod CrashLoopBackOffs at Init:0/1 and HAProxy never starts at all. Every fresh install races this, and the chart guarantees it will. The StatefulSet uses the default OrderedReady policy, so pmm-ha-1 is created only after pmm-ha-0 is Ready and pmm-ha-2 only after pmm-ha-1. For most of the five-minute budget the later replicas do not exist, their names do not resolve, and curl returns 000. PMM Server is a multi-GB image that also runs database migrations on first boot, so three replicas started one after another routinely exceed the budget. PMM-15202 is this failure, closed as Not a Bug. The gate was also redundant. haproxy.cfg carries "init-addr last,libc,none", so a backend whose name does not resolve is held with no address rather than aborting startup, and "http-check expect status 200" against /v1/server/leaderHealthCheck brings it UP only once it is genuinely serving. HAProxy running before any PMM pod exists is the designed behaviour: it returns 503 until a backend is up, which beats not running. The init container added no capability - it only turned "HAProxy up, no backends yet" into "no HAProxy". Verified on a live 3-replica cluster. Before the change HAProxy was already serving with seven of ten server-template slots in MAINT (resolution), i.e. unresolved names. After removing the init container, deleting a follower and restarting HAProxy in the same moment brought all three pods to 1/1 Running in under ten seconds with zero restarts, and PMM stayed reachable throughout. The chart's own init script, run unmodified against two absent replicas, exits 1 while three healthy replicas are serving. Removing it also drops the alpine:latest image and its "apk add --no-cache curl" at every HAProxy start, which required egress to the Alpine mirrors. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15394 Drop the CI assertions on the deleted HAProxy init script PMM-15151 (#868) added a lint-test step asserting that the wait-for-pmm.sh host loop renders pmm_host="<fullname>-$i..." - an assertion on the very file this branch deletes, so it failed the moment PMM-HA-GA was merged in. Git saw no conflict because the assertion and the deletion live in different files. HAProxy no longer builds per-pod names at all: it resolves backends through the headless service with server-template + init-addr, so there is nothing left to assert. The PMM_HA_PEERS half of the check, which does still build per-pod names from pmm.fullname, is kept as-is. Signed-off-by: theTibi <tkorocz@gmail.com> --------- Signed-off-by: theTibi <tkorocz@gmail.com>
* PMM-15393 Let HAProxy scale beyond the worker-node count The required podAntiAffinity on kubernetes.io/hostname capped haproxy.replicaCount at the number of worker nodes: every replica beyond it stayed Pending forever while Helm still reported the release as successful. Replace it with a soft topology spread constraint (maxSkew: 1, whenUnsatisfiable: ScheduleAnyway), which keeps one pod per node whenever there are enough nodes and co-locates instead of failing to schedule when there are not. Add a PodDisruptionBudget so a node drain cannot take out every HAProxy replica sharing a node. PostgreSQL and pgBouncer keep their required anti-affinity: co-locating Patroni members costs quorum on a single node loss. Signed-off-by: theTibi <tkorocz@gmail.com> * PMM-15393 Use maxUnavailable in the HAProxy PodDisruptionBudget minAvailable: 1 renders at every replica count, and at replicaCount 1 it gives allowedDisruptions = 0: kubectl drain hangs, the cluster-autoscaler cannot reclaim the node and managed node-pool upgrades stall. replicaCount 1 is reachable - it is the haproxy subchart's own default. maxUnavailable: 1 expresses the intent at every count. At the chart default of 3 replicas it keeps 2 serving instead of permitting two simultaneous evictions, and it serializes eviction of co-located replicas now that the soft topology spread allows them to share a node. A single replica stays drainable. Signed-off-by: theTibi <tkorocz@gmail.com> --------- Signed-off-by: theTibi <tkorocz@gmail.com>
The 'QAN for PMM Server' toggle in Settings cannot control the bundled PostgreSQL cluster: in HA mode PMM's database is a separate Percona PG Operator cluster whose pods register themselves in PMM as ordinary PostgreSQL services through the pmm-client sidecar. QAN data therefore kept flowing regardless of that setting. Turn query collection off at the source instead. The operator always emits '--query-source=<querySource>' and appends spec.pmm.postgresParams last, so repeating the flag wins: postgres_exporter is still registered and metrics keep flowing, but no QAN agent is created. pg-db could not pass the value through - the postgresParams conditional in cluster.yaml has been an empty if/end block since it was added in K8SPG-654 (pg 2.7.0), so the documented pmm.postgresParams value was silently dropped. Fill in the missing line. Also pin PMM_ENABLE_INTERNAL_PG_QAN=0 in pmmEnv so pmm-managed rejects attempts to switch the toggle on rather than failing later in the transaction. Signed-off-by: theTibi <tkorocz@gmail.com>
The PMM Client pods came with a `pmmClient` section in values.yaml, but the hand-maintained parameter table in the chart README was never extended to match, so none of the eleven parameters were documented for anyone reading the README rather than the values file. The rows mirror the `@param` annotations one to one, and the section is placed where it sits in values.yaml, between the PMM Server parameters and the secrets.
ademidoff
requested review from
4nte and
JiriCtvrtka
and removed request for
a team
September 7, 2026 05:51
ademidoff
changed the title
The PMM Client pods came with a `pmmClient` section in values.yaml, but the hand-maintained parameter table in the chart README was never extended to match, so none of the eleven parameters were documented for anyone reading the README rather than the values file. The rows mirror the `@param` annotations one to one, and the section is placed where it sits in values.yaml, between the PMM Server parameters and the secrets.
ademidoff
force-pushed
the
PMM-14989-retain-state-on-client-pod-restart
branch
from
September 7, 2026 15:08
f37bb12 to
6d80155
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new README row for pmmClient.forceRegistration omits a critical warning about destructive behavior, which could lead to accidental loss of configured Services.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the pmm-ha Helm chart documentation to include the previously undocumented pmmClient values in charts/pmm-ha/README.md, aligning the README’s parameter table with the pmmClient section already present in values.yaml.
Changes:
- Added a new “PMM Client” parameters table documenting
pmmClient.*configuration keys in the README. - Documented defaults for
pmmClientreplicas, image settings, storage, resources, and scheduling-related fields.
File summaries
| File | Description |
|---|---|
| charts/pmm-ha/README.md | Adds README parameter-table coverage for pmmClient configuration options. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… improve clarity and idempotency
…github.com/percona/percona-helm-charts into PMM-14989-retain-state-on-client-pod-restart
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.
Ticket: https://perconadev.atlassian.net/browse/PMM-14989
Based on
PMM-HA-GA(#927), notmain. Please check the base branch is right before merging.The problem
A PMM Client loses every database Service added to it when its pod restarts or its image is
upgraded, even with a PV and PVC in place.
pmm-agent setupruns on every container start andregisters the Node again, and PMM Server answers that by dropping the existing Node together with
every Service on it.
The client pods on
PMM-HA-GAalready avoid the destructive case: the init container incharts/pmm-ha/templates/pmm-client-statefulset.yamlregisters only when the pmm-agentconfiguration file is absent, so a restarted pod keeps its identity and its Services. What that
gate cannot do is recover a pod whose Node PMM Server no longer knows, after a server reinstall or
a restore from an older backup. It sees a configuration file, skips setup, and the Agent then loops
on "No Agent with ID" while both probes keep passing, so the pod reads Ready and monitors nothing.
What this PR carries
The documentation half of the ticket. The
pmmClientsection incharts/pmm-ha/values.yamlwasnever reflected in the hand-maintained parameter table in
charts/pmm-ha/README.md, leaving alleleven parameters undocumented for anyone reading the README rather than the values file.
forceRegistrationis the one that matters most: turning it on makes PMM Server remove the Nodewith every Service configured on it.
The rows mirror the
@paramannotations one to one, so the table and the values file cannot driftapart. The section is placed where it sits in
values.yaml, between the PMM Server parameters andthe secrets.
No template, value or default is changed here, and no chart
versionbump, since the PMM chartsaccumulate changes on the branch and are bumped once in a dedicated release PR.
Still to come on this branch
The behavioural half, which is blocked outside this repo. percona/pmm#5890 makes
pmm-agent setupidempotent under PMM-15260: an Agent that already holds an ID keeps its Node, and an Agent whose ID
the server no longer knows registers again instead of stranding. Its feature build is
Percona-Lab/pmm-submodules#4558.
Once that ships in a client image, the file gate above can be dropped in favour of an unconditional
pmm-agent setup, which is what closes the remaining failure mode. The two have to move together:pmmClient.image.tagis pinned to a released image, and an older agent without the force flagcrashloops on
AlreadyExistsif the gate is removed underneath it.Verification
ct lintagainstcharts/pmm-ha, in a container pinned to the versions.github/workflows/pmm-ha-pr-checks.yamlresolves: passes. The three[INFO] Fail:lines in thatoutput come from the pre-existing
pmm-secretlookupincharts/pmm-ha/templates/pg-user-credentials-secrets.yaml, whichhelm lintdowngrades to INFO.They are unrelated to this change and appear on an unmodified tree too.
Note on scope
This branch originally also trimmed
pmmClientincharts/pmm-ha/ci/ci-values.yamlfor thesingle-node Kind run. #946 landed the same change first, so that commit was dropped rather than
rebased into a conflict, and only the README work remains here.