From aa25dced2bdd9b9d0551d721844e69df522a7948 Mon Sep 17 00:00:00 2001 From: Justin Klein Date: Wed, 7 Oct 2026 11:39:25 +0000 Subject: [PATCH] stalwart: per-group distribution and PDB, and the selector-match CI assurance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stalwart: the six new per-group values in the nodes table, a "High availability, per group" section, and four traps - distribution is a promise and the price is a Pending pod (including the delete-the-Node-object inversion and the ceiling as a rule rather than a number), the opposite defaults of the two node-inclusion policies, nodeAffinityPolicy being pruned silently below Kubernetes 1.26, and the collision abort. Also: the install-test renders neither object, and the render-only overlay that covers them instead. CI-CD: the new selector-match job as step 4, install-test renumbered to 6, and helm-dependencies.sh now in six places - with the 503 that took both render tests red while this was written. Chart-Style-Guide: §11 gains the per-group PDB, the distribution-rule convention, the chart-filled labelSelector, the explicit nodeAffinityPolicy with its two separately-argued consequences, the collision abort, and the reminder not to add updateStrategy or podManagementPolicy for this. Lessons-Learned: two sections - the two defaults that pointed in opposite directions (where a review's own authoritative table was the thing that was wrong), and the duplicate the API rejected while the apply path merged it silently - plus four entries in the recurring shape. --- CI-CD.md | 29 +++++++++++++++--- Chart-Style-Guide.md | 6 ++++ Lessons-Learned.md | 30 +++++++++++++++++++ stalwart.md | 71 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 4 deletions(-) diff --git a/CI-CD.md b/CI-CD.md index 7e88d94..6ac439a 100644 --- a/CI-CD.md +++ b/CI-CD.md @@ -108,7 +108,28 @@ Two details worth knowing before touching it: - **The resources paths come from `values.schema.json`, not `values.yaml`.** It is JSON, so `python3` reads it without pyyaml. A block counts when it is `#/definitions/resources` or carries `limitFactor`/`requests` itself, which keeps `minecraft-server`'s JVM-sizing `minecraft.resources` out. A chart whose templates render a resources block while its schema declares none **fails** with exactly that message: the schema is out of sync. - **A chart that never reaches the helper is reported, not counted as passing.** `gotenberg` sets every computable limit explicitly, so nothing is multiplied and nothing is proven. Such charts appear under "helper not reached" in the job summary rather than as a green tick for a code path never entered. -### 4. `trivy-config` (reporting only) +### 4. `selector-match` (every chart, every PR) + +The second job that ignores the changed-chart matrix, and for the same reason as `quantity-render`. `ci/run-selector-match-test.sh` renders **every** chart with the fixtures the install-test uses and checks that every label selector in the render really matches the labels of a pod template **in that same render**. Two kinds of object are covered, and both fail *silently* when their selector matches nothing: + +| Object | What a non-matching selector does | Who notices today | +|---|---|---| +| `topologySpreadConstraints` | the constraint is a condition over **zero** pods and is therefore trivially satisfied: nothing is distributed, no event, no log line | nobody — and with `whenUnsatisfiable: DoNotSchedule` the object reads like a guarantee | +| `PodDisruptionBudget` | the budget does its arithmetic over zero pods and never blocks an eviction, so `kubectl drain` takes every replica at once | nobody — `kubectl get pdb` shows an object that looks like protection | + +Neither is a render error, neither fails the install-test, and neither shows up in the diff of the PR that causes it — the chart that breaks it is usually a chart that **renamed a label somewhere else**. Charts answer half of that by keeping the selector and the pod labels in one helper (`stalwart.groupLabels`, `keycloak`'s `app` label); the other half is that one definition only helps while nobody writes a second one, and that is the half a test can hold. Introduced with the per-group distribution rule in [stalwart](stalwart), where the trivially-satisfied case became reachable by design rather than by accident. + +A spread constraint is checked against **its own** pod template (the failure guarded against is a constraint that does not count the pods it sits on); a `PodDisruptionBudget` against any pod template in the render, since it has no owning template. + +Three things to know before touching it: + +- **Helm is the YAML parser, through the one-template chart `ci/yaml2json`** (`{{ .Values | toJson }}`). The runners have neither pyyaml nor `yq` — the same constraint `ci/run-trivy-config.sh` and `ci/run-quantity-render-test.sh` work under — and a YAML reader written in `awk` was the obvious alternative and the wrong one: its failure mode is a parser that quietly reads nothing, finds no selector, reports no mismatch and leaves the test green for a chart it never looked at. The rendered manifest stream is therefore handed to that chart as a **values file** and comes back as JSON for `python3`. The one textual step is the document split on a line that is exactly `---` in column one, and getting *that* wrong does not produce a false pass: the fragment stops being valid YAML and `helm` exits 1. `ci/yaml2json` lives under `ci/`, so it has no publish workflow and no version-bump obligation — the chart-discovery loops look one level below the repo root and `ci` has no `Chart.yaml`. +- **What is deliberately not checked, named so the gap is not mistaken for coverage.** `podAffinity`/`podAntiAffinity` are out of scope: their semantics are relative to pods that exist, so a term matching nothing is a legitimate configuration rather than a mistake. A selector expressed **only** through `matchExpressions` is **reported as "not checked" and never counted as passing** — evaluating set-based requirements means reimplementing label-selector semantics, i.e. a second parser by another name. +- **Two anti-vacuum guards, because this test can be green over nothing.** Both objects render only above a replica threshold and most charts' CI fixtures sit below it. `ci/selector-match/.yaml` is a render-only overlay applied **last**, whose only job is to push a chart over that threshold — and a chart that *has* one and still renders no selector **fails**, because that overlay existing is the whole claim that something was checked. On top of that, a run in which **no selector at all** was checked fails. Today `stalwart` is exercised through its overlay (two replicas in both node groups, plus an `externalServices.valkey.host` without which the chart's own "cluster with no in-memory store" abort fires) and `keycloak` through its ordinary install-test fixture, which already runs two replicas. + +Measured: 30 charts, **5 selectors verified in ~14 s**, no cluster. The fail path was exercised rather than assumed — renaming `node-group` to `nodegroup` in a hand-written copy of the PDB selector produced two `FAIL` lines naming both the selector and the labels actually present, plus a non-zero exit; the same was done for the spread constraint's selector, and the `matchExpressions`-only and overlay-without-selector paths were probed too. + +### 5. `trivy-config` (reporting only) Renders each chart in the scan matrix and lists what `trivy config` finds. **It never fails the job** (`TRIVY_CONFIG_MODE: report`). Deliberately not a gate yet: the repo has several documented deviations, and a check that goes red on those every time is one people learn to scroll past. Switching the mode to `gate` is the intended step once the seccomp gap is closed. @@ -116,7 +137,7 @@ Trivy is installed from its release tarball, not via `aquasecurity/setup-trivy` Exceptions live in `ci/trivy/trivyignore.yaml`, each with a mandatory `statement` — the script refuses to run without one and prints every suppressed finding together with its reason. **The trusted-registry list for AVD-KSV-0125 is a separate file on purpose**, `ci/trivy/data/ksv0125.yaml`: it is *configured* rather than suppressed, so the check keeps being able to say something about an image from a genuinely foreign source. A new registry therefore means an entry there, not an ignore — `quay.io` joined it with [keycloak](keycloak), the first chart in the repo using it. Note what the check does *not* forgive: an unqualified Docker Hub image puts the organisation in the registry position and is still reported, which is why every chart writes its image fully qualified. -### 5. `install-test` (per changed, non-excluded chart) +### 6. `install-test` (per changed, non-excluded chart) `ci/run-install-test.sh ` against a throwaway [k3d](https://k3d.io) cluster: apply the stub `OnePasswordItem` CRD plus the chart's fixtures, `helm install` with the CI values, and wait for every Deployment and StatefulSet to roll out. Since probes and hardening are active, "Ready" means the app actually answers. On failure the job dumps pod descriptions, events and container logs. @@ -288,13 +309,13 @@ All three are recorded on [`Gemessenes-Verhalten`](https://git.jk-dev.de/workflo Not a workflow but the prerequisite of every helm command in this repo: it fetches a chart's declared dependencies into `charts/`, and returns silently for the charts that declare none (all but one). -It runs in **five** places, because every helm command exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in the shared publish block, and inside `ci/run-install-test.sh`, `ci/run-quantity-render-test.sh` and `ci/run-trivy-config.sh`. The second of those is now across a repository boundary: the block calls this script from the calling repository's checkout, which makes the script part of the contract — renaming or deleting it breaks publishing from a place that no longer reads our diffs. One of those call sites is the render test that covers **every** chart on **every** PR — so a dependency-carrying chart can turn a third-party Helm repository's bad day into a red pipeline on an unrelated PR. +It runs in **six** places, because every helm command exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in the shared publish block, and inside `ci/run-install-test.sh`, `ci/run-quantity-render-test.sh`, `ci/run-selector-match-test.sh` and `ci/run-trivy-config.sh`. The second of those is now across a repository boundary: the block calls this script from the calling repository's checkout, which makes the script part of the contract — renaming or deleting it breaks publishing from a place that no longer reads our diffs. Two of those call sites are the render tests that cover **every** chart on **every** PR — so a dependency-carrying chart can turn a third-party Helm repository's bad day into a red pipeline on an unrelated PR, now from either of them. Measured while writing the selector-match test: the registry answered `503` for several minutes and both tests went red on `stalwart`, `ytdlp-web-player` and `paperless-ngx` with nothing in the diff to blame. It runs `helm dependency update`, not `build`: `Chart.lock` is not committed, because Renovate rewrites `Chart.yaml` and cannot regenerate the lock, so a committed one would arrive stale. ## A PR that touches only `ci/` -Both matrices follow the changed **chart directories**, so a PR that only changes the harness exercises nothing by itself. The Trivy scan has a fixed-sample fallback for that case; the install-test has none — bump the affected chart's patch version to pull it into the matrix, or run the [all-charts install-test](#the-all-charts-install-test-on-demand) once the change is on `main`. The quantity render test is the exception to the rule: it always runs over everything. +Both matrices follow the changed **chart directories**, so a PR that only changes the harness exercises nothing by itself. The Trivy scan has a fixed-sample fallback for that case; the install-test has none — bump the affected chart's patch version to pull it into the matrix, or run the [all-charts install-test](#the-all-charts-install-test-on-demand) once the change is on `main`. The quantity render test and the selector-match test are the two exceptions to the rule: both always run over everything. ## What a Gitea runner does differently diff --git a/Chart-Style-Guide.md b/Chart-Style-Guide.md index 861618f..9b2398f 100644 --- a/Chart-Style-Guide.md +++ b/Chart-Style-Guide.md @@ -121,6 +121,12 @@ Measured on `urlaubsverwaltung` (helm 3.19.0, Kubernetes 1.35): `helm get manife - Implemented via a per-chart `.replicas` helper using an explicit nil check (`kindIs "invalid"`), because Helm's `default` would swallow an explicit `0`. - **A chart whose default is more than one replica ships a `PodDisruptionBudget`.** `policy/v1`, named `--pdb`, `minAvailable` as a value (default `1`), selector on the workload's `app` label. [keycloak](keycloak) is the first and set the convention. It is a rule rather than a per-chart taste because of *which* failure a second replica is usually bought against: a node drain is the most common reason a pod moves — it is what a node upgrade does — and without a budget the drain is free to take every replica at once. The second replica then costs money and buys nothing. - **And it is rendered only while the *computed* replica count is `>= 2`.** `minAvailable: 1` with exactly one replica forbids every voluntary eviction, so `kubectl drain` blocks forever on a pod nothing may move — and the message names the PDB, not the replica count, which sends the reader to the wrong file. The same applies to `scaleDown: true`, where there are no pods and the object would only describe a promise about nothing. So the condition reads the `.replicas` helper rather than the raw value, and `NOTES.txt` prints which of the two happened instead of leaving it to be inferred from `kubectl get pdb`. +- **A chart whose replicas are a property of a *group* ships one PDB per group, with that group's own count.** [stalwart](stalwart) is the first — one node group per cluster role, one StatefulSet each. The `>= 2` condition then applies per group (with `full: 2` and `mx: 1` exactly one object exists), the name and the selector carry the group, and `NOTES.txt` lists the outcome per group rather than once for the release. +- **A budget without a distribution rule is half an answer.** A PDB keeps a drain from taking every replica at once; it says nothing about where the replicas were put in the first place, and two pods on one node are not redundancy. So where replicas are meant to survive the loss of a node, the chart also offers a **switch that renders the `topologySpreadConstraints` entry itself** — `maxSkew: 1` on `kubernetes.io/hostname`, `whenUnsatisfiable: DoNotSchedule` so the distribution is a promise rather than an effort, plus a generic pass-through (`affinity`, `nodeSelector`, `tolerations`, `topologySpreadConstraints`) for what the switch does not cover. `stalwart`'s `nodes..spreadAcrossNodes` (default `true`, same computed `>= 2` gate) is the reference. +- **The `labelSelector` is filled by the chart, never written by the operator.** A spread constraint whose selector matches no pod is **trivially satisfied** — a condition over zero pods — so it distributes nothing with a green rollout, no event and no log line; and with `DoNotSchedule` the object then reads like a guarantee. A PDB whose selector matches nothing never blocks an eviction while looking like protection. One helper therefore defines the pod template's labels, the spread selector and the PDB selector (`stalwart.groupLabels`; the precedent for one-definition-because-a-mismatch-is-not-a-render-error is `stalwart.env.dbPassword`), and **`ci/run-selector-match-test.sh` holds every rendered selector against the rendered pod labels on every PR** — see [the selector-match job](CI-CD#4-selector-match-every-chart-every-pr). One definition only helps while nobody writes a second one. +- **Set `nodeAffinityPolicy: Ignore` explicitly, and argue it separately from the replica ceiling.** The two node-inclusion policies have **opposite** defaults: `nodeAffinityPolicy` defaults to `Honor`, `nodeTaintsPolicy` to `Ignore`. Under `Honor` a narrowing `nodeSelector` shrinks the set of counted domains until both replicas fit into one — skew 0, co-location, **no `Pending`, no event, and a correct selector**, so no check can see it. `Ignore` turns the same case into skew 2 and a `Pending` pod. A render abort is the wrong fix: it would also forbid a `nodeSelector` spanning five nodes, which is legal. Two things belong in `values.yaml` with it, and separately: the field is **beta** and is pruned without a word below Kubernetes 1.26, which brings the gap straight back; and the replica ceiling — **as many replicas per group as there are schedulable workers**, because `nodeTaintsPolicy ≙ Ignore` keeps control-plane domains at 0 — comes from the *other* field and existed before this one was set. Write the ceiling as a rule, never as a number. +- **A user entry colliding with the chart's own on `(topologyKey, whenUnsatisfiable)` aborts the render, naming both values.** `whenUnsatisfiable` defaults to `DoNotSchedule`, so an omitted one still collides. The alternatives are an API error pointing at a list index, and a silent merge over `patchMergeKey: topologyKey` that drops one `labelSelector` and goes green. +- **Do not add `updateStrategy` or `podManagementPolicy` for this.** The Kubernetes defaults — `RollingUpdate` with `partition: 0`, and `OrderedReady` — already update one pod at a time and wait for Ready. A values key that restates a default is a key someone will later believe does something. ## 12. Values that must not lie diff --git a/Lessons-Learned.md b/Lessons-Learned.md index e1e777a..17ecf93 100644 --- a/Lessons-Learned.md +++ b/Lessons-Learned.md @@ -462,6 +462,32 @@ The second finding is the one that cost the detour, and it is not about Keycloak **Now:** when a counter moves in the same round as a change, do not argue from runs made earlier or elsewhere — **reproduce the old state on the current machine and measure it there.** For a chart that means disabling the new values in the fixture and checking that the pod template hash returns to the previous one, which turns "roughly the same spec" into a fact. Budget for more than one run per side: a 1-in-3 behaviour is invisible in a single run and looks like a regression in two. And where a green path reports a number nobody can explain, say so in the file that carries the number and name the harness change that would answer it — the next person to see that restart should not have to re-derive this. +## The two defaults that pointed in opposite directions + +Both halves of [stalwart](stalwart)'s distribution rule are node-inclusion policies on the same `topologySpreadConstraints` entry, so they read as a pair. They are not, and the API says so in two lines that sit a few fields apart: + +- `nodeAffinityPolicy` — *"If this value is nil, the behavior is equivalent to the **Honor** policy"* +- `nodeTaintsPolicy` — *"If this value is nil, the behavior is equivalent to the **Ignore** policy"* + +A review of the plan had written down, as the price of `whenUnsatisfiable: DoNotSchedule`, that a `nodeSelector` or `tolerations` narrowing the eligible nodes would leave a replica permanently `Pending`. For `tolerations` that is right. For `nodeSelector` it is **wrong in the dangerous direction**: under the `Honor` default only nodes matching the pod's affinity count as domains, so narrowing a 2-replica group onto one node leaves exactly one eligible domain, both pods land in it, the skew is 0 and the constraint is **satisfied**. No `Pending`, no event, and — the part that closes every remaining escape — the `labelSelector` in the object is perfectly correct, so the CI selector check cannot see it either. The promise is gone and every instrument reads normal. + +Two things were wrong at once, and the second is the more useful one. The first: a documented symptom that does not occur is worse than no documentation, because somebody will wait for it. The second: **the review that found the error had itself been written as an authoritative-sounding table**, and the correction came only from reading the API definition for both fields rather than for the one under discussion. + +The fix is one field, `nodeAffinityPolicy: Ignore`, which makes every node count: the same narrowing becomes `2,0,0`, skew 2, `Pending`. A render abort was the obvious alternative and is the wrong one — it would also have forbidden a `nodeSelector` spanning five nodes, which is legal and works. And the field must be documented **apart** from the replica ceiling it sits next to: the ceiling (*as many replicas per group as there are schedulable workers*) comes from `nodeTaintsPolicy ≙ Ignore` and existed before any of this, so presenting the two as one block invites the next reader to pay a price to a decision that did not cause it. + +**Now:** when a feature has two switches that look like siblings, **read the default of each one separately** — and when a protection rests on a field, check whether the field can be *absent*. `nodeAffinityPolicy` is beta: on by default from Kubernetes 1.26, GA with `NodeInclusionPolicyInPodTopologySpread` in 1.33, and **pruned without a word** on anything older, which restores the exact gap it was set against. That sentence belongs at the value with the minimum version, never assumed as known. See [§11](Chart-Style-Guide#11-scaling). + +## The duplicate that the API rejected and the apply path merged + +Two `topologySpreadConstraints` entries with the same `topologyKey` *and* the same `whenUnsatisfiable` are a duplicate — and `whenUnsatisfiable` **defaults to `DoNotSchedule`**, so a user entry on `kubernetes.io/hostname` that simply omits the field collides with a chart-rendered one. What happens next depends on how the manifest reaches the cluster, and the two answers differ in kind: + +- `kubectl create` fails validation with a message naming a **list index**, not a value. The reader is sent to a rendered manifest to count entries instead of to the two values that produced them. +- an **apply**-based path merges the two over `patchMergeKey: topologyKey` — **silently**. One `labelSelector` is dropped, one constraint survives, and the result is green. + +The second is the one that matters here, because a dropped `labelSelector` is exactly the trivially-satisfied case: a condition over zero pods, distributing nothing, under an object that reads like a guarantee. + +**Now:** where a chart renders an entry into a list the user can also append to, the chart **aborts the render on a collision and names both values** — the switch that produced its own entry and the list the user wrote. The abort fires only while the chart's entry is actually rendered, so it cannot trip a configuration that has no duplicate. The generalisation beyond this field: **a list merged by a key is a namespace, and two writers into one namespace need a referee at render time** — the API's own answer is either an index or a silent merge, and neither names a value anybody set. + ## The recurring shape - **Green is a claim about the assertions that ran, not about the system.** @@ -495,4 +521,8 @@ The second finding is the one that cost the detour, and it is not about Keycloak - **The strictest file mode is not the correct one.** A mounted secret is owned by `root`, `fsGroup` supplies only the group, and an application that treats `EACCES` like "absent" turns a permission mistake into a feature that quietly switched itself off. - **A counter that moved alongside a change is not evidence until the control case ran on the same machine.** Earlier green runs are a different environment, and an intermittent behaviour needs several runs per side before either answer is honest. - **Ask where the consumer of a file runs, not only where the application runs.** A chart with a pre-build step has two consumers of its volumes at two different times, and a mount that reaches only the serving one fails without a symptom. +- **Two switches that look like siblings can have opposite defaults.** Read the default of each one on its own, and keep the consequences of each in its own paragraph — a price documented next to the wrong decision is paid by whoever reads that decision later. +- **A protection that rests on a field can be lost by the field being absent.** Beta fields are pruned without a word on an older API server, so the question is not only "is the value right" but "does this version keep it at all" — with the minimum version written at the value. +- **A documented symptom that does not occur is worse than no documentation**, because somebody will wait for it instead of looking. +- **A list merged by a key is a namespace, and two writers into one namespace need a referee at render time.** The API's own answers are an error naming a list index and a silent merge that drops one half; neither names a value anybody set. - **When the fix needs a privilege, ask whether two cheap steps can replace one privileged one.** Splitting a pre-build into a seeding container plus a build container kept the hardening baseline intact across the whole pod, where the shorter single-container version would have bought a documented deviation *and* a suppression that silences the container that actually serves traffic. diff --git a/stalwart.md b/stalwart.md index 811bcaf..773a813 100644 --- a/stalwart.md +++ b/stalwart.md @@ -232,11 +232,32 @@ A **map**, not a list, so that an override never depends on a position. The key |---|---|---|---| | `nodes..replicas` | `1` | `0` | `0` allowed; `scaleDown` wins | | `nodes..exposedPorts` | `[smtp, submissions, imaps, http]` | `[smtp, http]` | Keys of `ports`; **must include `http`** (all probes target it) | +| `nodes..spreadAcrossNodes` | `true` | `true` | One pod of this group per node, **enforced**. Rendered only from two *computed* replicas upwards — [what it costs](#distribution-is-a-promise-and-the-price-is-a-pending-pod) | +| `nodes..podDisruptionBudget.enabled` | `true` | `true` | A PDB **per group**, rendered only from two *computed* replicas upwards | +| `nodes..podDisruptionBudget.minAvailable` | `1` | `1` | `minAvailable`, not `maxUnavailable`: the promise is about the service | +| `nodes..affinity` | `{}` | `{}` | Passed verbatim into this group's pod spec. **Narrowing it below the replica count blocks the group** — [why that is the good outcome](#the-two-node-inclusion-policies-have-opposite-defaults) | +| `nodes..nodeSelector` | `{}` | `{}` | As above | +| `nodes..tolerations` | `[]` | `[]` | As above. Tolerating a taint does **not** raise the replica ceiling | +| `nodes..topologySpreadConstraints` | `[]` | `[]` | Appended after the chart's own and really AND-ed with it. A collision on `kubernetes.io/hostname` **aborts the render** — [why](#a-colliding-spread-constraint-aborts-the-render) | | `nodes..resources.requests` | `200m` / `512Mi` / `64Mi` | `100m` / `256Mi` / `64Mi` | Limits computed by `limitFactor` | | `nodes..env` | `[]` | `[]` | Additive; `value`, `secretKeyRef`, `configMapKeyRef` | **There is no `nodes..tasks`.** Which tasks a role runs is a property of the `ClusterRole` object in the database and the chart cannot write it; a values key that looked like it set them and did not would be worse than its absence. +#### High availability, per group + +`spreadAcrossNodes: true` renders one `topologySpreadConstraints` entry for the group — `maxSkew: 1` on `kubernetes.io/hostname`, `whenUnsatisfiable: DoNotSchedule`, `nodeAffinityPolicy: Ignore` — with a `labelSelector` the **chart** fills from its own pod labels. `podDisruptionBudget.enabled: true` renders `-stalwart--pdb` with the same selector. They are two halves of one answer: the constraint decides that the pods sit on different nodes, the budget decides that only one of those nodes may be emptied at a time. + +**The distribution is only ever *within* a group.** The selector names the release **and** the node group, so `full` pods are spread against `full` pods and `mx` pods against `mx` pods. A `full` and an `mx` pod on the same node is deliberately fine — they are different roles, not redundant copies. There is **no** cross-group condition, and that was decided rather than overlooked. + +**Both objects are conditional on the *computed* replica count** (so `scaleDown: true` removes both). With `full: 2` and `mx: 1` exactly one PDB and one spread constraint exist. `NOTES.txt` prints the outcome **per group** — enforced / not rendered / off, and for the PDB its `minAvailable` or the reason it is absent — so it never has to be inferred from `kubectl get pdb`. + +**The selector is the chart's, never yours.** `stalwart.groupLabels` is the single definition behind the pod template's labels, the spread constraint's `labelSelector` and the PDB's selector, and the reason is that a selector matching no pod fails silently in *both* objects: the constraint becomes a condition over zero pods and is trivially satisfied, the budget never blocks an eviction while looking like protection. `ci/run-selector-match-test.sh` holds all three against each other on every pull request — see [the selector-match job](CI-CD#4-selector-match-every-chart-every-pr). + +**Scaling a group from 1 to 2 replaces the running pod.** The constraint lives in the **pod template**, so the change is a rollout and not merely an addition. Expect a restart of the node that is already up, in the same round. + +**And one precondition that is not in `values.yaml`:** this works because the StatefulSets have **no `volumeClaimTemplates`**. If the open question about `/var/lib/stalwart` ever turns into a PVC per group, a node-local or topology-bound storage class pins each pod to wherever its volume was first provisioned — distribution is then decided at provisioning time rather than by this rule, and `DoNotSchedule` can keep a returning pod `Pending` for good. **The rule holds only while this chart stays PVC-free.** + ### `networkPolicy` | Value | Default | Meaning | @@ -492,6 +513,54 @@ But read from the source: **`/healthz/ready` does not query the database.** It r Both endpoints are unauthenticated and carry no network restriction. They do pass the configurable `allowedEndpoints` expression (default `200`, i.e. everything allowed) whose loopback escape hatch does **not** cover a node IP — so an operator who later writes a restrictive expression breaks every probe. +### Distribution is a promise, and the price is a `Pending` pod + +`whenUnsatisfiable` is `DoNotSchedule`, not `ScheduleAnyway`, because the requirement was stated as a guarantee: `full` pods run on different nodes, `mx` pods run on different nodes. `ScheduleAnyway` gives no guarantee at all — with one worker at 97 % requested CPU the scheduler may put both replicas of a group on the one remaining node, entirely within the rule and with the redundancy gone. + +The price is a pod that stays `Pending` rather than one that lands somewhere useless, **and a `Pending` pod with no scheduling event reads like a different problem than a distribution rule.** Two cases: + +- **Two of three workers gone.** The surviving replica keeps running; the second does **not** join the survivor — but only *while the `Node` objects still exist*. A `NotReady` node still counts as an (empty) domain, so placing another pod would raise the skew to 2. **Delete the `Node` objects and the domains disappear with them, and the pod schedules.** Same mechanism, opposite outcome, one step apart — worth knowing before a node is "temporarily" left in the API. +- **A narrowing `nodeSelector` or `affinity`.** `DoNotSchedule` plus a node restriction below the group's replica count is a combination that blocks itself. That is intended and loud — see the next trap for why it took an explicit field to make it loud. + +Where the 97 %-CPU node actually bites: **at the third replica, not the second.** With two replicas on three workers the distribution is `1,1,0` → skew 1 → allowed, and the tight CPU produces no `Pending` yet. Only from three replicas per group upwards must every worker accept one. + +**The replica ceiling is a rule, not a number: as many replicas per group as there are schedulable workers.** The chart leaves `nodeTaintsPolicy` at its API default of `Ignore`, so **every** node counts as a domain — control-plane nodes included, where no Stalwart pod ever runs. Those domains hold the minimum at 0 permanently, so with `maxSkew: 1` no domain may hold more than one pod. Add a worker and the ceiling rises with it; remove one and it falls. Past that point a group needs `spreadAcrossNodes: false`, or an own entry with `ScheduleAnyway`. **Do not write today's count down anywhere** — it would be wrong at the next cluster rebuild and nobody would come back for it. + +### The two node-inclusion policies have opposite defaults + +Straight from the API definition, and this asymmetry is the whole reason the chart sets one of them explicitly: + +- `nodeAffinityPolicy` — *"If this value is nil, the behavior is equivalent to the **Honor** policy"*, and Honor means only nodes matching the pod's `nodeAffinity`/`nodeSelector` are counted as domains. +- `nodeTaintsPolicy` — *"If this value is nil, the behavior is equivalent to the **Ignore** policy"*, i.e. all nodes are counted. + +Under the `Honor` default, a `nodeSelector` narrowing a 2-replica group onto **one** node leaves exactly one eligible domain, both pods land in it, and the skew is 0 — **the constraint is satisfied.** Both replicas co-located, nothing `Pending`, no event, and the selector in the object is perfectly correct, so the CI selector check cannot see it either. The chart therefore sets **`nodeAffinityPolicy: Ignore`**, under which every node counts: the same narrowing gives `2,0,0`, skew 2, and the second pod is `Pending`. The silent half becomes the loud one. + +A render abort was the alternative and was rejected, because it would also have forbidden a `nodeSelector` spanning five nodes, which is legal and works (`1,1,0,0,0` → skew 1). The field forbids only the case that is actually broken. + +**And the two fields are not a package.** The replica ceiling above comes from `nodeTaintsPolicy ≙ Ignore` and existed before any of this; `nodeAffinityPolicy: Ignore` does **not** add it, it only closes the silent gap. They are argued separately in `values.yaml` for exactly that reason. + +### `nodeAffinityPolicy` can be dropped without a word + +The field is **beta** — on by default from Kubernetes **1.26**, graduated to GA with the feature gate `NodeInclusionPolicyInPodTopologySpread` in **1.33**. On an older API server, or one with the gate switched off, it is **pruned silently**, and the gap of the previous trap comes straight back: a narrowing `nodeSelector` again yields co-location with no `Pending` and no message. The object still looks correct in `kubectl get sts -o yaml`, minus one line. + +So **1.26 is the minimum version on which this chart can make the promise at all.** Check it against the live object rather than against the render: + +```bash +kubectl -n get sts -l stalwart.jk-dev.de/release=-stalwart \ + -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.spec.template.spec.topologySpreadConstraints[*].nodeAffinityPolicy}{"\n"}{end}' +``` + +An empty column where `Ignore` should be is the whole symptom. **Not measured** against a pre-1.26 server — this is the documented pruning behaviour for a field behind a disabled gate, not something tried here. + +### A colliding spread constraint aborts the render + +Kubernetes keys a spread constraint on `(topologyKey, whenUnsatisfiable)`, and `whenUnsatisfiable` **defaults to `DoNotSchedule`** — so an entry in `nodes..topologySpreadConstraints` on `kubernetes.io/hostname` *without* an explicit `whenUnsatisfiable` collides with the chart's own. The chart aborts the render naming both `nodes..spreadAcrossNodes` and `nodes..topologySpreadConstraints`, because both ways the collision would otherwise resolve are worse: + +- `kubectl create` rejects the duplicate with a message naming a **list index** rather than a value; +- an apply-based path **merges** the two entries over `patchMergeKey: topologyKey` — *silently* — so one `labelSelector` is dropped and the result is green. + +Entries on any **other** key (`topology.kubernetes.io/zone`, for instance) are appended and really do apply as an AND. To own the per-node distribution yourself, set `spreadAcrossNodes: false` and keep your entry — remembering that `nodeAffinityPolicy: Ignore` is then yours to repeat. The abort fires only while the chart's own entry is actually rendered, so it cannot trip a single-replica group. + ## Upgrades and backup - **A plain app-version bump** is a chart patch and needs no action here; Renovate opens it and `ci/renovate-bump-chart.sh` bumps the version. Chart pages carry no version numbers — `Chart.yaml` is the source. @@ -513,6 +582,8 @@ What it does prove is the chart's own responsibility, and the first item is why 3. the rendered `config.json` parses **as Stalwart reads it** — the store opens, so `@type`, the integer timeout and the `authSecret` indirection are all accepted, which is the one check that the password really arrives without ever being in the ConfigMap; 4. the Secret key names match, the node ports can be allocated, the quantities are valid, and the NetworkPolicies do not cut off the probes. +**The install-test renders neither the PDB nor the spread constraint.** `ci/stalwart/test-values.yaml` runs `full: 1` / `mx: 0`, so both objects stay below their threshold — nothing about the HA objects is exercised in a cluster, and nothing claims otherwise. What covers them instead is a **render-only** overlay, `ci/selector-match/stalwart.yaml`, which raises both groups to two replicas and sets an `externalServices.valkey.host` (without it the chart's own "cluster with no in-memory store" abort fires). `ci/run-selector-match-test.sh` then checks that each group's PDB selector and spread `labelSelector` match that group's own pod labels — and **fails** if the overlay stops producing a selector, because an overlay that quietly reaches nothing would make the check vacuum-green. See [CI/CD](CI-CD). + **No Valkey in the fixtures, deliberately.** The in-memory store is a database object, so a Valkey in the namespace would be reachable and unused — making the test look as though it covered clustering while covering none of it. The test runs a single node, which needs no in-memory store. The subchart path is covered by a local render only; a known gap. ## See also