From 6ade74db800590d312ff0fe3f9dd01f226ef2a7e Mon Sep 17 00:00:00 2001 From: Justin Klein Date: Wed, 7 Oct 2026 11:24:37 +0000 Subject: [PATCH] feat(stalwart): spread each node group over nodes and give each one a PDB A two-replica node group was redundancy on paper: nothing kept both pods off one node, and no PodDisruptionBudget kept a drain from taking both at once. Both objects are now rendered per node group, gated on that group's COMPUTED replica count being >= 2. The distribution is a promise, not an effort: maxSkew 1 on kubernetes.io/hostname with whenUnsatisfiable DoNotSchedule, selecting only this group's own pods, so an "mx" and a "full" pod may share a node. The selector comes from the chart (stalwart.groupLabels), never from the operator - a spread constraint whose selector matches no pod is trivially satisfied, distributes nothing and reports nothing. nodeAffinityPolicy: Ignore is set explicitly, and that is the load-bearing detail. Its API default is Honor, under which a narrowing nodeSelector shrinks the set of counted domains until both replicas fit into one: skew 0, both pods co-located, no Pending, no event, and a selector that is correct - so no check can see it. With Ignore the same narrowing gives skew 2 and the second pod is Pending. A render abort was the alternative and would also have forbidden a nodeSelector spanning five nodes, which is legal. A user entry colliding with the chart's own on (topologyKey, whenUnsatisfiable) aborts the render naming both values, because the API rejects the duplicate with a message about a list index and an apply-based path merges the two over patchMergeKey: topologyKey silently, dropping one selector. Minor rather than major, proved with a diff rather than argued: with the shipped defaults (full: 1, mx: 0) and with ci/stalwart/test-values.yaml, helm template renders byte-for-byte what 1.0.1+upv0.16.24 rendered - both diffs empty. New in CI, over every chart on every PR: ci/run-selector-match-test.sh holds every rendered PodDisruptionBudget selector and every topologySpreadConstraints labelSelector against the labels of the pod templates in the same render. Helm itself is the YAML parser (ci/yaml2json), because the runners have neither pyyaml nor yq and a hand-written parser could read nothing and report nothing. podAffinity/podAntiAffinity are out of scope; matchExpressions-only selectors are reported as not checked rather than counted as passing. Two anti-vacuum guards: a chart with an overlay under ci/selector-match/ that renders no selector fails, and a run that checks no selector at all fails. Fixes #96 --- .github/workflows/validate-charts.yml | 30 ++ README.md | 26 +- ci/run-selector-match-test.sh | 461 ++++++++++++++++++++++++++ ci/selector-match/stalwart.yaml | 38 +++ ci/yaml2json/Chart.yaml | 24 ++ ci/yaml2json/templates/json.yaml | 1 + ci/yaml2json/values.yaml | 4 + stalwart/Chart.yaml | 9 +- stalwart/templates/NOTES.txt | 76 +++++ stalwart/templates/_helpers.tpl | 125 +++++++ stalwart/templates/stalwart.pdb.yaml | 62 ++++ stalwart/templates/stalwart.sfs.yaml | 48 ++- stalwart/values.schema.json | 79 +++++ stalwart/values.yaml | 150 +++++++++ 14 files changed, 1125 insertions(+), 8 deletions(-) create mode 100755 ci/run-selector-match-test.sh create mode 100644 ci/selector-match/stalwart.yaml create mode 100644 ci/yaml2json/Chart.yaml create mode 100644 ci/yaml2json/templates/json.yaml create mode 100644 ci/yaml2json/values.yaml create mode 100644 stalwart/templates/stalwart.pdb.yaml diff --git a/.github/workflows/validate-charts.yml b/.github/workflows/validate-charts.yml index 81c7ba2..e692ca1 100644 --- a/.github/workflows/validate-charts.yml +++ b/.github/workflows/validate-charts.yml @@ -173,6 +173,36 @@ jobs: - name: "Render every chart with bare integer quantities" run: ci/run-quantity-render-test.sh + selector-match: + name: "Selector match" + runs-on: ubuntu-latest + # The second job that ignores the changed-chart matrix and runs over *every* + # chart (Gitea #96), and for the same reason as quantity-render above: the + # mistake arrives by copy into a chart whose own diff nobody reads for it. + # + # What it asserts: every PodDisruptionBudget selector and every + # topologySpreadConstraints labelSelector a chart renders really matches the + # labels of a pod template in the SAME render. Both objects fail SILENTLY + # when their selector matches nothing - the spread constraint becomes a + # condition over zero pods and is trivially satisfied, the budget does its + # arithmetic over zero pods and never blocks an eviction - so neither a + # render error nor the install-test can catch it, and with + # whenUnsatisfiable: DoNotSchedule the object reads like a guarantee while + # distributing nothing. + # + # It needs no cluster and no Trivy: ~14 seconds for the whole repository, + # depends on no other job, and finishes well inside build-chart. The YAML is + # parsed by Helm itself through ci/yaml2json, because the runners have + # neither pyyaml nor yq and a hand-written parser would be able to read + # nothing and report nothing - see the script's header. + steps: + - name: "Checkout Repository" + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + - name: "Install Helm" + uses: azure/setup-helm@9bc31f4ebc9c6b171d7bfbaa5d006ae7abdb4310 # v5 + - name: "Check every rendered selector against the rendered pod labels" + run: ci/run-selector-match-test.sh + trivy-config: name: "Trivy config ${{ matrix.chart }}" needs: detect-changed-charts diff --git a/README.md b/README.md index d9273d5..2be8fd2 100644 --- a/README.md +++ b/README.md @@ -68,8 +68,8 @@ Every chart has a wiki page of its own — `https://git.jk-dev.de/jlab-cluster/h Note: the registry **accepts a re-upload of an already published version** (HTTP 201) and replaces the stored artifact, so a republish of an existing version is still silent — see #160. That is also what makes a manual `workflow_dispatch` of any `publish-.yml` a harmless way to prove the shared block works. - `change-app-version.yml` — manually dispatched app-version bump; the chart must be listed in its `chart-name` choice list. -- `ci/helm-dependencies.sh ` — not a workflow but the step they all share: fetches a chart's declared dependencies into `charts/`, and returns silently for the charts that declare none (all but three: `ytdlp-web-player`, `stalwart`, `paperless-ngx`). It runs in **five** places, because every helm command this repo builds on exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in the shared publish block (which is why that block names this script as a prerequisite of its callers — see above), and inside `ci/run-install-test.sh`, `ci/run-quantity-render-test.sh` and `ci/run-trivy-config.sh`. See "Chart dependencies" in the style guidelines for what that costs. -- `validate-charts.yml` — PR pipeline: detects which charts a PR touches and, for exactly those, runs `helm lint --strict`, `helm package`, and a chart-version bump check (`template-chart` is exempt from the version check). PRs without chart changes skip the build matrix. After the build stage, an **install-test** job (the same changed-charts matrix, minus the charts listed in `ci/excluded-charts.txt`) installs each changed chart into an ephemeral [k3d](https://k3d.io) cluster on the runner and requires every workload to become Ready — see below. Both matrices follow the changed chart directories, so a PR that only changes the harness under `ci/` 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 after the merge. The **resource-quantity render test** is the exception to that rule: it runs over every chart on every PR — see below. +- `ci/helm-dependencies.sh ` — not a workflow but the step they all share: fetches a chart's declared dependencies into `charts/`, and returns silently for the charts that declare none (all but three: `ytdlp-web-player`, `stalwart`, `paperless-ngx`). It runs in **six** places, because every helm command this repo builds on exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in the shared publish block (which is why that block names this script as a prerequisite of its callers — see above), 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`. See "Chart dependencies" in the style guidelines for what that costs. +- `validate-charts.yml` — PR pipeline: detects which charts a PR touches and, for exactly those, runs `helm lint --strict`, `helm package`, and a chart-version bump check (`template-chart` is exempt from the version check). PRs without chart changes skip the build matrix. After the build stage, an **install-test** job (the same changed-charts matrix, minus the charts listed in `ci/excluded-charts.txt`) installs each changed chart into an ephemeral [k3d](https://k3d.io) cluster on the runner and requires every workload to become Ready — see below. Both matrices follow the changed chart directories, so a PR that only changes the harness under `ci/` 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 after the merge. The **resource-quantity render test** and the **selector-match render test** are the two exceptions to that rule: both run over every chart on every PR — see below. - `install-test.yml` — reusable workflow (`workflow_call`) holding the install-test **job** itself: the runner-label expression, the pinned `kubectl` with its skew check, `envsubst`, the containerised-job detection, the per-leg k3d cluster and the `freesailarr` VPN credentials. Its one input is a JSON array of chart names; its callers decide *which* charts are tested and whether the job runs at all, never *how*. Two callers: `validate-charts.yml` and `install-test-all-charts.yml`. **It is not copied for the second caller** — a second copy is a second chance to test differently, unnoticed, the same argument that gives `ci/excluded-charts.txt` exactly one parser. **It stays in this repository, and that is a decision, not an oversight** (Gitea #66, decision 4). It is reusable in form but not in substance: it is welded to the `ci/` fixtures, to the `ci/excluded-charts.txt` parser and to this repository's chart matrix. A building block that only one repository can use is not shared, it is merely moved somewhere else — and moving it would buy nothing while putting a tag between us and a file we change often. `publish-helm-chart.yml` was the opposite case: it takes a chart name and a registry owner and knows nothing about this repository, which is exactly why it could go. This note exists so the question is not asked again in half a year. @@ -142,6 +142,25 @@ 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 (same runner constraint as `ci/run-trivy-config.sh`), and every chart-owned key must appear there anyway. A block counts when it is `#/definitions/resources` or carries `limitFactor`/`requests` itself, which is what keeps `minecraft-server`'s JVM-sizing `minecraft.resources` out. A chart whose templates render a resources block while its schema declares none **fails** the test with exactly that message: the schema is then out of sync with the chart. - **A chart that never reaches the helper is reported, not counted as passing.** `gotenberg` sets every computable limit explicitly in its fallback defaults, and since a `null` never deletes a default there, no override can free them — nothing is multiplied, so nothing is proven. Those charts appear under "helper not reached" in the job summary rather than as a green tick for a code path that was never entered. +### Selector-match render test (all charts, every PR) + +`ci/run-selector-match-test.sh [chart …]` (no argument = all charts) renders every chart with the same fixtures the install-test uses and checks that **every label selector the render contains really matches the labels of a pod template in that same render**. Two kinds of object are covered, and both of them 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 the first half of that by keeping the selector and the pod labels in one helper (`stalwart.groupLabels`, `keycloak`'s `app` label) — but one definition only helps while nobody writes a second one, and that is the half a test can hold. Introduced with Gitea #96, where the chart-owned spread constraint made the trivially-satisfied case reachable by design rather than by accident. + +Three things to know before touching it: + +- **Helm is the YAML parser, through the one-template chart `ci/yaml2json`.** The runners have neither `pyyaml` nor `yq` (measured; 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. So the rendered manifest stream is handed to that chart as a **values file** and comes back as JSON, which `python3` reads from the standard library. 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. +- **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. And a selector expressed **only** through `matchExpressions` is **reported as "not checked", never counted as passing** — evaluating set-based requirements means reimplementing label-selector semantics, which is a second parser by another name. +- **Two anti-vacuum guards, because this test can be green over nothing.** Both objects are rendered 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**, since that overlay existing is the whole claim that something was checked. On top of that the run fails if **no selector at all** was checked anywhere. 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, and a non-zero exit; the same was done for the spread constraint's selector. + ### Install-test (PR pipeline stage 2) For every chart a PR changes, `ci/run-install-test.sh ` runs against a throwaway k3d cluster: it applies the stub `OnePasswordItem` CRD plus the chart's fixtures, `helm install`s the chart with its CI values, and waits for all Deployments/StatefulSets to roll out (per-chart timeout). Since probes and security hardening are active on all workloads, "Ready" means the app actually answers. On failure the job dumps pod descriptions, events and container logs. @@ -389,6 +408,9 @@ These guidelines are **binding for every current and future chart** in this repo - Implemented via a per-chart `.replicas` helper in `_helpers.tpl` (reference in `template-chart`); the helper uses 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 matching the workload's `app` label. `keycloak` (Gitea #83) is the first and sets the convention. The reason it is a rule and not a per-chart taste: 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, so the second replica buys nothing against the failure it was added for. - **And it is rendered only while the effective 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 describe a promise about nothing. So the condition is on the **computed** replica count (the `.replicas` helper), not on the raw value, and `NOTES.txt` prints which of the two happened rather than leaving it to be inferred from `kubectl get pdb`. +- **A chart whose replicas are a property of a *group* gets one PDB per group, with that group's own count** (Gitea #96; `stalwart` is the first, one node group per cluster role). The condition above then applies per group — with `full: 2` and `mx: 1` exactly one object is rendered — and `NOTES.txt` lists the outcome per group rather than once for the release. The name carries the group (`---pdb`), and so does the selector. +- **A budget without a distribution rule is half an answer.** The PDB keeps a drain from taking every replica at once; it says nothing about where the replicas were put in the first place. Where a chart's replicas are meant to survive the loss of a node, it also offers a **per-group switch that renders the `topologySpreadConstraints` entry itself, with a selector the chart fills from its own pod labels** — never one the operator writes, because a spread constraint whose selector matches no pod is trivially satisfied and distributes nothing, with a green rollout and no message. `stalwart` sets the convention: `nodes..spreadAcrossNodes` (default `true`, gated on the same computed `>= 2`), `maxSkew: 1` on `kubernetes.io/hostname`, `whenUnsatisfiable: DoNotSchedule` so the distribution is a promise rather than an effort, and a generic per-group pass-through (`affinity`, `nodeSelector`, `tolerations`, `topologySpreadConstraints`) for everything the switch does not cover. Three traps come with it and all three belong in the chart's `values.yaml`: `nodeAffinityPolicy: Ignore` must be set **explicitly** (its API default is `Honor`, under which a narrowing `nodeSelector` shrinks the set of counted domains until both replicas fit in one, giving skew 0 — co-location with no `Pending`, no event, and a selector that is *correct*, so no CI check can see it); that field is **beta** and is pruned without a word below Kubernetes 1.26, which brings that exact gap back; and the replica ceiling is **a rule, not a number** — as many replicas per group as there are schedulable workers, because `nodeTaintsPolicy` defaults to `Ignore` and control-plane domains therefore hold the minimum at 0 permanently. That ceiling comes from `nodeTaintsPolicy` and is **not** a consequence of setting `nodeAffinityPolicy`; the two are documented separately for that reason. A user entry colliding with the chart's own on `(topologyKey, whenUnsatisfiable)` **aborts the render naming both values**, because the two alternatives are an API error pointing at a list index and a silent merge over `patchMergeKey: topologyKey` that drops one selector and goes green. +- **Both selectors are held against the rendered pod labels on every PR** by `ci/run-selector-match-test.sh` — see "Selector-match render test" above. Needed because the shared-helper rule above is a convention, and a convention is the thing a copy breaks. ### 12. Values that must not lie (#68, #93, #99, #215) diff --git a/ci/run-selector-match-test.sh b/ci/run-selector-match-test.sh new file mode 100755 index 0000000..692bb9b --- /dev/null +++ b/ci/run-selector-match-test.sh @@ -0,0 +1,461 @@ +#!/usr/bin/env bash +# Selector-match render test: every label selector a chart renders is held +# against the labels of the pod templates in the SAME render. +# +# Usage: ci/run-selector-match-test.sh [chart ...] (no argument = all charts) +# +# WHAT IT GUARDS AGAINST, AND WHY A GREEN ROLLOUT DOES NOT (Gitea #96) +# +# Two kinds of object in this repository select pods by label, and BOTH of them +# fail silently when the selector matches nothing: +# +# topologySpreadConstraints - a constraint whose labelSelector matches no pod +# is TRIVIALLY SATISFIED. It is a condition over zero pods, so every +# placement meets it: nothing is distributed, the rollout is green, no event +# is emitted and no log line appears. With whenUnsatisfiable: DoNotSchedule +# this is worse rather than better, because the object then reads like a +# guarantee that two replicas sit on different nodes. +# PodDisruptionBudget - a budget whose selector matches no pod does its +# arithmetic over zero pods and never blocks an eviction. `kubectl drain` +# takes the whole group at once while the object sits in the namespace +# looking like protection. +# +# Neither is a render error, neither is an install-test failure, and neither is +# visible in a diff: the chart that breaks it is usually a chart that RENAMED a +# label somewhere else. Charts keep their selector and their pod labels in one +# helper for that reason (stalwart.groupLabels, keycloak's app label) - but one +# definition only helps while nobody writes a second one, and that is the part a +# test can hold. +# +# Over ALL charts and not the changed ones, for the same reason as +# ci/run-quantity-render-test.sh: the mistake arrives by copy into a new chart, +# whose own diff nobody reads for it, and running over everything means there is +# no list to maintain - adding a chart enrols it. +# +# HOW THE YAML IS PARSED, AND WHY NOT WITH A PARSER WRITTEN HERE +# +# The runners have neither pyyaml nor yq - measured, the same constraint +# ci/run-trivy-config.sh and ci/run-quantity-render-test.sh work under. Writing +# a YAML reader in awk or sed was the obvious alternative and is the wrong one: +# its failure mode is a parser that quietly reads nothing, finds no selector, +# reports no mismatch, and leaves this test green for a chart it never looked +# at - the exact class of signal this repository removes rather than adds. +# +# So HELM is the parser, which is a YAML implementation that is already +# installed, already trusted, and already the one that produced the manifests. +# ci/yaml2json is a one-template chart that prints its values back as JSON; the +# rendered manifest stream is handed to it as a VALUES FILE and comes back as +# JSON that python3 reads with its standard library. What IS done textually is +# only the document split, on a line that is exactly "---" in column one, and +# the consequence of getting that wrong is not a false pass: the fragment stops +# being valid YAML and helm exits 1. +# +# WHAT IS DELIBERATELY NOT CHECKED +# +# - podAffinity / podAntiAffinity. They carry labelSelectors too, but their +# semantics are "relative to pods that exist", and an anti-affinity term +# matching nothing is a legitimate configuration rather than a mistake. +# Out of scope, and named so that the gap is not mistaken for coverage. +# - selectors expressed ONLY through matchExpressions. Evaluating a set-based +# requirement means reimplementing label-selector semantics, which is a +# second parser by another name. Those are REPORTED as "not checked" and +# are never counted as passing, because a green tick for a code path nobody +# entered is worse than an honest gap (README, "Two rules for every check +# added here"). +# +# AND THE ANTI-VACUUM GUARDS, because this test can be green over nothing. A +# chart renders a PDB or a spread constraint only above a replica threshold, and +# every chart's CI fixtures sit below it. Two rules therefore apply: +# +# 1. ci/selector-match/.yaml is an overlay applied last, whose only job +# is to push a chart over that threshold. A chart that HAS one and still +# renders no selector FAILS - the overlay exists for exactly that, so an +# overlay that stopped working is a finding and not a shrug. +# 2. If no selector at all was checked across the whole run, the run fails. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +CI_DIR="$ROOT/ci" +YAML2JSON="$CI_DIR/yaml2json" +OVERLAY_DIR="$CI_DIR/selector-match" +RELEASE="selector" + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# --- Which charts ----------------------------------------------------------- +CHARTS=() +if [ "$#" -gt 0 ]; then + CHARTS=("$@") +else + for DIR in "$ROOT"/*/; do + [ -f "$DIR/Chart.yaml" ] || continue + CHARTS+=("$(basename "$DIR")") + done +fi +if [ "${#CHARTS[@]}" -eq 0 ]; then + echo "ERROR: no chart directories found under $ROOT" >&2 + exit 1 +fi + +FAILED="" +CHECKED="" +NOT_EXERCISED="" +WITH_GAPS="" +TOTAL_SELECTORS=0 +SUMMARY_ROWS="" + +for CHART in "${CHARTS[@]}"; do + if [ ! -f "$ROOT/$CHART/Chart.yaml" ]; then + echo "ERROR: '$CHART' is not a chart directory" >&2 + exit 1 + fi + + # Same reason as in ci/run-quantity-render-test.sh: a chart with a declared + # dependency cannot be rendered at all while charts/ is empty, and this job + # runs over EVERY chart on EVERY pull request - so without this line a PR + # touching nothing but a README would go red on the charts that have one. + "$ROOT/ci/helm-dependencies.sh" "$ROOT/$CHART" + + # Values: the same precedence the other render tests use, so a chart's CI + # values are maintained in one place and satisfy its required() values. + VALUES_ARGS=() + VALUES_LABEL="chart defaults" + if [ -f "$CI_DIR/$CHART/test-values.yaml" ]; then + VALUES_ARGS=(-f "$CI_DIR/$CHART/test-values.yaml") + VALUES_LABEL="ci/$CHART/test-values.yaml" + elif [ -f "$CI_DIR/trivy/values/$CHART.yaml" ]; then + VALUES_ARGS=(-f "$CI_DIR/trivy/values/$CHART.yaml") + VALUES_LABEL="ci/trivy/values/$CHART.yaml" + fi + HAS_OVERLAY="" + if [ -f "$OVERLAY_DIR/$CHART.yaml" ]; then + # Last, so it wins over the install-test values. + VALUES_ARGS+=(-f "$OVERLAY_DIR/$CHART.yaml") + VALUES_LABEL="$VALUES_LABEL + ci/selector-match/$CHART.yaml" + HAS_OVERLAY=1 + fi + + echo "::group::$CHART ($VALUES_LABEL)" + + RENDER="$WORK/$CHART.manifest.yaml" + if ! helm template "$RELEASE" "$ROOT/$CHART" \ + ${VALUES_ARGS[@]+"${VALUES_ARGS[@]}"} >"$RENDER" 2>"$WORK/err"; then + sed 's/^/ /' "$WORK/err" + echo "::endgroup::" + echo "::error title=selector-match $CHART::$CHART does not render with its CI values, so no selector could be checked. Reproduce: helm template $RELEASE $CHART $(printf '%s ' "${VALUES_ARGS[@]+${VALUES_ARGS[@]}}")" + FAILED="$FAILED $CHART" + continue + fi + + # --- The document split, and the only textual step in this script ---------- + # One YAML sequence item per rendered document, content indented by four and + # the item marker on a line of its own - so a document whose first line is a + # comment ("# Source: ...") needs no special case. A wrong split does not + # produce a false pass: the result stops being parsable and helm exits 1. + DOCS="$WORK/$CHART.docs.yaml" + awk ' + BEGIN { print "docs:"; started = 0 } + /^---[ \t]*$/ { print " -"; started = 1; next } + { + if (!started) { print " -"; started = 1 } + if (length($0) == 0) print ""; else print " " $0 + } + ' "$RENDER" >"$DOCS" + + JSON="$WORK/$CHART.json" + if ! helm template y2j "$YAML2JSON" -f "$DOCS" 2>"$WORK/err" \ + | awk '/^\{/ { print; exit }' >"$JSON"; then + sed 's/^/ /' "$WORK/err" + echo "::endgroup::" + echo "::error title=selector-match $CHART::the rendered manifests of $CHART could not be re-read as YAML by ci/yaml2json. That is a parse failure, not a selector finding: either the render contains a document this script's '---' split cut in the wrong place, or the manifest is not valid YAML. Helm's own message is in the job log." + FAILED="$FAILED $CHART" + continue + fi + if [ ! -s "$JSON" ]; then + echo "::endgroup::" + echo "::error title=selector-match $CHART::ci/yaml2json produced no JSON for $CHART - the manifests were not re-read, so nothing was checked." + FAILED="$FAILED $CHART" + continue + fi + + # --- The comparison -------------------------------------------------------- + # Inline, as in ci/run-quantity-render-test.sh, and on the standard library + # only - json, because the YAML is already gone by this point. It writes one + # "VERDICT|detail" line per selector and never decides the exit code itself: + # the loop below does, so a verdict can never be lost to a swallowed status. + set +e + python3 - "$CHART" "$JSON" >"$WORK/$CHART.out" 2>"$WORK/$CHART.pyerr" <<'PY' +import json, sys + +chart, path = sys.argv[1], sys.argv[2] +docs = json.load(open(path)).get("docs") or [] + +# A pod template is any value of a key named "template" that carries a spec with +# containers. Found by walking rather than by enumerating kinds, so a CronJob's +# spec.jobTemplate.spec.template is covered without a special case, and so is +# whatever workload kind a chart adds next. +def pod_templates(node, path="$"): + out = [] + if isinstance(node, dict): + for key, value in node.items(): + here = "%s.%s" % (path, key) + if (key == "template" and isinstance(value, dict) + and isinstance(value.get("spec"), dict) + and "containers" in value["spec"]): + out.append((here, value)) + out.extend(pod_templates(value, here)) + elif isinstance(node, list): + for i, value in enumerate(node): + out.extend(pod_templates(value, "%s[%d]" % (path, i))) + return out + + +def labels_of(template): + meta = template.get("metadata") + labels = meta.get("labels") if isinstance(meta, dict) else None + return labels if isinstance(labels, dict) else {} + + +def name_of(doc): + meta = doc.get("metadata") + if isinstance(meta, dict) and meta.get("name"): + return str(meta["name"]) + return "" % doc.get("kind", "object") + + +def matches(selector, labels): + """matchLabels only. Every key must be present with the same value; the + comparison is on the string form, because YAML 'true' and the string + "true" are the same label value to Kubernetes.""" + for key, value in selector.items(): + if key not in labels or str(labels[key]) != str(value): + return False + return True + + +def classify(selector, where): + """(matchLabels dict, None) when it can be evaluated, (None, reason) when + it cannot. A selector this script cannot evaluate is REPORTED, never + counted as a pass.""" + if selector is None: + return None, ("%s has NO selector at all. For a topology spread " + "constraint that means it matches no pod and is " + "therefore vacuously satisfied; for a PodDisruptionBudget " + "it is rejected by the API server" % where) + if not isinstance(selector, dict): + return None, "%s selector is not a mapping (%r)" % (where, selector) + match_labels = selector.get("matchLabels") + if isinstance(match_labels, dict) and match_labels: + if selector.get("matchExpressions"): + # matchLabels AND matchExpressions: the matchLabels half is still + # a necessary condition, so checking it is sound - it just is not + # the whole selector. Said out loud rather than passed off as full + # coverage. + return match_labels, None + return match_labels, None + if selector.get("matchExpressions"): + return None, ("%s selects only through matchExpressions, which this " + "test does not evaluate - doing so would mean " + "reimplementing label-selector semantics, i.e. a second " + "parser. Reported, not counted as passing" % where) + if match_labels == {} or selector == {}: + return {}, None # the empty selector matches every pod + return None, "%s selector has neither matchLabels nor matchExpressions" % where + + +# Rooted at "/" rather than at "$", because a chart with one +# workload per node group renders several documents whose internal path is +# identical - and then every finding would name the same spot. +all_templates = [] +for doc in docs: + if isinstance(doc, dict): + root = "%s/%s" % (doc.get("kind", "?"), name_of(doc)) + all_templates.extend(pod_templates(doc, root)) + +lines = [] + +# 1. Every topologySpreadConstraints entry, against ITS OWN pod template. That +# is the strict and the correct comparison: the failure being guarded +# against is a constraint that does not count the very pods it sits on. +for where, template in all_templates: + spread = template.get("spec", {}).get("topologySpreadConstraints") + if not isinstance(spread, list): + continue + labels = labels_of(template) + for i, constraint in enumerate(spread): + if not isinstance(constraint, dict): + lines.append("FAIL|%s: topologySpreadConstraints[%d] at %s is not a mapping" + % (chart, i, where)) + continue + key = constraint.get("topologyKey", "") + tag = "spread[%d] on %s at %s" % (i, key, where) + match_labels, reason = classify(constraint.get("labelSelector"), tag) + if match_labels is None: + if constraint.get("labelSelector") is None: + lines.append("FAIL|%s: %s" % (chart, reason)) + else: + lines.append("SKIP|%s: %s" % (chart, reason)) + continue + if matches(match_labels, labels): + lines.append("OK|%s: %s matches its own pod template's labels (%s)" + % (chart, tag, ", ".join("%s=%s" % kv for kv in sorted(match_labels.items())) or "empty selector, matches every pod")) + else: + lines.append("FAIL|%s: %s selects %s but the pod template it is " + "attached to carries %s - the constraint therefore " + "matches NONE of its own pods, is trivially satisfied, " + "and distributes nothing while the rollout stays green. " + "Render the chart and compare the selector with the pod " + "template's labels; the usual cause is a label renamed " + "in one of the two places" + % (chart, tag, + ", ".join("%s=%s" % kv for kv in sorted(match_labels.items())), + ", ".join("%s=%s" % kv for kv in sorted(labels.items())) or "no labels at all")) + +# 2. Every PodDisruptionBudget, against ALL pod templates of the render: a +# budget has no owning template, it only has to select some pod that exists. +pdbs = [d for d in docs if isinstance(d, dict) and d.get("kind") == "PodDisruptionBudget"] +for doc in pdbs: + tag = "PodDisruptionBudget %s" % name_of(doc) + spec = doc.get("spec") if isinstance(doc.get("spec"), dict) else {} + match_labels, reason = classify(spec.get("selector"), tag) + if match_labels is None: + lines.append(("FAIL|%s: %s" if spec.get("selector") is None else "SKIP|%s: %s") + % (chart, reason)) + continue + if not all_templates: + lines.append("FAIL|%s: %s is rendered but this render contains no pod " + "template at all, so its selector cannot match anything" + % (chart, tag)) + continue + hits = [w for w, t in all_templates if matches(match_labels, labels_of(t))] + if hits: + lines.append("OK|%s: %s matches %d pod template(s): %s" + % (chart, tag, len(hits), ", ".join(hits))) + else: + known = sorted({"%s=%s" % kv for _, t in all_templates for kv in labels_of(t).items()}) + lines.append("FAIL|%s: %s selects %s, which matches NONE of the %d pod " + "template(s) in the same render. A PDB that matches no pod " + "never blocks an eviction, so a node drain takes every " + "replica at once while the object looks like protection. " + "Labels actually present on the pod templates: %s" + % (chart, tag, + ", ".join("%s=%s" % kv for kv in sorted(match_labels.items())), + len(all_templates), "; ".join(known) or "none")) + +for line in lines: + print(line) +PY + RC=$? + set -e + if [ "$RC" -gt 1 ]; then + sed 's/^/ /' "$WORK/$CHART.pyerr" + echo "::endgroup::" + echo "::error title=selector-match $CHART::the selector comparison itself failed for $CHART (python exit $RC). Treated as a failure, never as a pass." + FAILED="$FAILED $CHART" + continue + fi + + CHART_FAIL="" + CHART_OK=0 + CHART_SKIP=0 + while IFS='|' read -r VERDICT DETAIL; do + case "$VERDICT" in + OK) + CHART_OK=$((CHART_OK + 1)) + echo " ok $DETAIL" + ;; + SKIP) + CHART_SKIP=$((CHART_SKIP + 1)) + echo " gap $DETAIL" + ;; + FAIL) + CHART_FAIL=1 + echo " FAIL $DETAIL" + echo "::error title=selector-match $CHART::$DETAIL" + ;; + *) + [ -n "$VERDICT" ] && echo " $VERDICT $DETAIL" + ;; + esac + done <"$WORK/$CHART.out" + echo "::endgroup::" + + TOTAL_SELECTORS=$((TOTAL_SELECTORS + CHART_OK)) + + if [ -n "$CHART_FAIL" ]; then + FAILED="$FAILED $CHART" + SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | **failed** | $CHART_OK | $CHART_SKIP |"$'\n' + echo "$CHART: FAILED" + elif [ "$CHART_OK" -gt 0 ]; then + CHECKED="$CHECKED $CHART" + [ "$CHART_SKIP" -gt 0 ] && WITH_GAPS="$WITH_GAPS $CHART" + SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | ok | $CHART_OK | $CHART_SKIP |"$'\n' + echo "$CHART: ok ($CHART_OK selector(s) matched, $CHART_SKIP not checked)" + elif [ -n "$HAS_OVERLAY" ]; then + # The overlay's ONE job is to push this chart over the threshold at which + # it renders a selector. If it did not, this test is vacuum-green for the + # chart it was written for, which is the failure the overlay prevents. + FAILED="$FAILED $CHART" + SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | **failed** | 0 | $CHART_SKIP |"$'\n' + echo "::error title=selector-match $CHART::$CHART has an overlay at ci/selector-match/$CHART.yaml but rendered no PodDisruptionBudget and no topologySpreadConstraints, so nothing was checked. That overlay exists only to make those objects render - either it no longer reaches the threshold (replica count, a feature switch) or the chart stopped rendering them. A green tick here would be a tick for a check that ran over nothing." + echo "$CHART: FAILED (overlay present, no selector rendered)" + else + NOT_EXERCISED="$NOT_EXERCISED $CHART" + [ "$CHART_SKIP" -gt 0 ] && WITH_GAPS="$WITH_GAPS $CHART" + echo "$CHART: no PodDisruptionBudget and no topologySpreadConstraints rendered - nothing to check." + fi +done + +# --- Report ------------------------------------------------------------------ +count() { set -- $1; echo "$#"; } +listing() { set -- $1; [ "$#" -eq 0 ] && { echo "–"; return; }; printf '`%s`' "$1"; shift; for C in "$@"; do printf ', `%s`' "$C"; done; echo; } +SUMMARY="${GITHUB_STEP_SUMMARY:-/dev/null}" +{ + echo "### Selector-match render test (Gitea #96)" + echo + echo "Every \`PodDisruptionBudget\` selector and every \`topologySpreadConstraints\` label" + echo "selector in a chart's render, held against the labels of the pod templates in the" + echo "**same** render. A selector that matches no pod is not a render error and not an" + echo "install-test failure: the spread constraint is then trivially satisfied and the" + echo "budget never blocks an eviction, both silently." + echo + echo "| Result | Count | Charts |" + echo "|---|---|---|" + echo "| Selectors checked and matching | $(count "$CHECKED") | $(listing "$CHECKED") |" + echo "| No such object rendered | $(count "$NOT_EXERCISED") | $(listing "$NOT_EXERCISED") |" + echo "| Failed | $(count "$FAILED") | $(listing "$FAILED") |" + echo + echo "$TOTAL_SELECTORS selector(s) verified in total." + if [ -n "$WITH_GAPS" ]; then + echo + echo "Charts with selectors this test does **not** evaluate (\`matchExpressions\` only — reported, never counted as passing): $(listing "$WITH_GAPS")." + fi + if [ -n "$SUMMARY_ROWS" ]; then + echo + echo "| Chart | Result | Checked | Not checked |" + echo "| --- | --- | --- | --- |" + printf '%s' "$SUMMARY_ROWS" + fi +} >>"$SUMMARY" + +echo "===================== selector match test ======================" +echo "checked: $(count "$CHECKED")${CHECKED:+ -$CHECKED}" +echo "not exercised: $(count "$NOT_EXERCISED")${NOT_EXERCISED:+ -$NOT_EXERCISED}" +echo "failed: $(count "$FAILED")${FAILED:+ -$FAILED}" +echo "selectors verified: $TOTAL_SELECTORS" +echo "================================================================" + +if [ -n "$FAILED" ]; then + echo "ERROR: selectors do not match the pod labels they are meant to select in:$FAILED" >&2 + exit 1 +fi + +# The second anti-vacuum guard. Reaching zero means every chart fell into "no +# such object rendered", which is indistinguishable from a broken harness - and +# it is the state this test would silently decay into if the overlays were ever +# dropped. +if [ "$TOTAL_SELECTORS" -eq 0 ]; then + echo "ERROR: not a single selector was checked in this run. Either no chart renders a PodDisruptionBudget or a topologySpreadConstraints any more, or the overlays under ci/selector-match/ no longer push one over its replica threshold. A pass over zero assertions is not a pass." >&2 + exit 1 +fi diff --git a/ci/selector-match/stalwart.yaml b/ci/selector-match/stalwart.yaml new file mode 100644 index 0000000..8c1103b --- /dev/null +++ b/ci/selector-match/stalwart.yaml @@ -0,0 +1,38 @@ +# Overlay for ci/run-selector-match-test.sh, applied ON TOP of +# ci/stalwart/test-values.yaml and nowhere else - not by the install-test, not by +# the Trivy scan, not by the quantity render test. +# +# WHY IT EXISTS. Both objects this test looks at are rendered only while a node +# group's COMPUTED replica count is >= 2, and the install-test fixture runs +# full: 1 / mx: 0 deliberately (a second pod would need a real in-memory store, +# and a running "mx" pod would fail-open into a second full node against an +# empty database). So without this file stalwart renders no PodDisruptionBudget +# and no topologySpreadConstraints, and the test would be green for the chart it +# was written for, having looked at nothing. ci/run-selector-match-test.sh +# therefore FAILS a chart that has an overlay and still renders no selector. +# +# This is a RENDER-ONLY overlay. Nothing here is installed anywhere, so the two +# reasons the install-test keeps mx at 0 do not apply. +nodes: + full: + replicas: 2 + exposedPorts: [smtp, submissions, imaps, http] + mx: + # Two as well, so BOTH groups' selectors are checked. The per-group split is + # the whole point of this chart's version of the rule: one object per group, + # each selecting only its own pods. A single group would leave the case where + # a group's selector accidentally matches the OTHER group's pods untested. + replicas: 2 + exposedPorts: [smtp, http] + +externalServices: + valkey: + # REQUIRED HERE, and not cosmetic: with more than one Stalwart pod and no + # in-memory store address the chart ABORTS THE RENDER ("this release runs N + # Stalwart pods, which is a cluster, but no in-memory store has an + # address"). Nothing connects to this name - the render never resolves it - + # but without it this overlay would turn the whole job red instead of + # exercising it. valkeySubchart.enabled: true would be the other way out and + # is deliberately not taken: it would add a network fetch of the published + # valkey chart to a test that otherwise needs none. + host: valkey-selector-match-fixture diff --git a/ci/yaml2json/Chart.yaml b/ci/yaml2json/Chart.yaml new file mode 100644 index 0000000..1af11a6 --- /dev/null +++ b/ci/yaml2json/Chart.yaml @@ -0,0 +1,24 @@ +apiVersion: v2 +# NOT A CHART THIS REPOSITORY PUBLISHES. It is a YAML parser, and Helm is the +# only one the runners have. +# +# ci/run-selector-match-test.sh has to read label selectors out of rendered +# manifests, and the runners carry neither pyyaml nor yq - measured, and the +# same constraint ci/run-trivy-config.sh and ci/run-quantity-render-test.sh +# work under. The alternative to this chart is a YAML parser written in awk or +# sed, and the failure mode of that is the one this repository removes rather +# than adds: a parser that quietly reads nothing finds no selector, reports no +# mismatch, and the test is green for a chart it never looked at. +# +# So the manifest fragment is handed to Helm as a VALUES FILE, and the one +# template below prints it back as JSON, which python3 reads with its standard +# library. A fragment Helm cannot parse makes helm exit 1, which is loud. +# +# It lives under ci/ and therefore has no publish workflow, no version-bump +# requirement and no entry in change-app-version.yml: the chart-discovery loops +# in this repository look for a Chart.yaml one level below the repo root, and +# "ci" has none. The version below is meaningless for the same reason. +name: yaml2json +description: Internal CI helper - reprints a values file as JSON so python3 can read YAML without pyyaml +type: application +version: 0.0.0 diff --git a/ci/yaml2json/templates/json.yaml b/ci/yaml2json/templates/json.yaml new file mode 100644 index 0000000..369ff59 --- /dev/null +++ b/ci/yaml2json/templates/json.yaml @@ -0,0 +1 @@ +{{ .Values | toJson }} diff --git a/ci/yaml2json/values.yaml b/ci/yaml2json/values.yaml new file mode 100644 index 0000000..ac40e1d --- /dev/null +++ b/ci/yaml2json/values.yaml @@ -0,0 +1,4 @@ +# Deliberately empty. Every value this chart renders comes from the -f file the +# caller passes, which is one document out of a rendered manifest stream. A +# default here would be merged into that document and would show up as a key +# the chart under test never rendered. diff --git a/stalwart/Chart.yaml b/stalwart/Chart.yaml index db7e8ac..5af11c9 100644 --- a/stalwart/Chart.yaml +++ b/stalwart/Chart.yaml @@ -18,7 +18,14 @@ type: application # all three of latest / v0.16 / v0.16.24 resolved to one digest pushed seconds # apart, so either floating name would change the running image with no commit # in this repository and nothing to report it. -version: 1.0.1+upv0.16.24 +# MINOR, not major, and the evidence is a diff rather than a judgement: the +# per-group spread constraint and the per-group PodDisruptionBudget of Gitea #96 +# are both gated on the COMPUTED replica count being >= 2, and the chart ships +# full: 1 / mx: 0. `helm template` against the previous version therefore renders +# byte-for-byte identically for the defaults AND for ci/stalwart/test-values.yaml +# - measured, both empty diffs. New optional values with unchanged defaults is +# exactly the minor case (README, "Versioning"). +version: 1.1.0+upv0.16.24 appVersion: "v0.16.24" maintainers: diff --git a/stalwart/templates/NOTES.txt b/stalwart/templates/NOTES.txt index fe2bf0d..050ab4f 100644 --- a/stalwart/templates/NOTES.txt +++ b/stalwart/templates/NOTES.txt @@ -217,6 +217,82 @@ WHAT A GREEN POD PROVES HERE, AND WHAT IT DOES NOT the node IP, and so every pod on a node shares one bucket. The defaults cost 6 requests/min/pod and leave room for about 16 pods per node. Tightening them produces 429s, and a 429 on liveness is a restart loop. +{{- $anySpread := false }} +{{- range $group, $spec := $nodes }} +{{- $r := int (include "stalwart.replicas" (dict "scaleDown" $.Values.scaleDown "replicas" $spec.replicas)) }} +{{- $s := fromYaml (include "stalwart.groupSettings" (dict "group" $group "given" $spec)) }} +{{- if and $s.spreadAcrossNodes (ge $r 2) }}{{ $anySpread = true }}{{ end }} +{{- end }} + +DISTRIBUTION AND DISRUPTION BUDGET — WHAT EACH GROUP ACTUALLY GOT + Both objects are conditional, so this is the list rather than something to + infer from `kubectl get pdb` and an `-o yaml`. +{{- range $group, $spec := $nodes }} +{{- $r := int (include "stalwart.replicas" (dict "scaleDown" $.Values.scaleDown "replicas" $spec.replicas)) }} +{{- $s := fromYaml (include "stalwart.groupSettings" (dict "group" $group "given" $spec)) }} + + {{ $group }} — {{ $r }} effective replica(s) +{{- if and $s.spreadAcrossNodes (ge $r 2) }} + SPREAD ENFORCED. maxSkew 1 on kubernetes.io/hostname, whenUnsatisfiable + DoNotSchedule, nodeAffinityPolicy Ignore, selecting this group's + own pods only — a pod of "{{ $group }}" sharing a node with a pod of + another group stays fine. This is a PROMISE, so the price is a Pending pod + rather than a co-located one: if fewer nodes are available than + this group has replicas, the surplus replica does not schedule and + there is NO scheduling event to read. Check with + kubectl -n {{ $.Release.Namespace }} get pod -l stalwart.jk-dev.de/node-group={{ $group }} -o wide +{{- else if $s.spreadAcrossNodes }} + SPREAD NOT RENDERED, because this group has {{ $r }} effective replica(s) and the + rule needs two. {{ if eq $r 0 }}With no pod at all there is nothing to + distribute{{ else }}With one pod every placement satisfies it, so the object + would describe nothing{{ end }}. Raise nodes.{{ $group }}.replicas to 2 to get it — + and note that doing so changes the POD TEMPLATE, so {{ if eq $r 0 }}the group + starts with the constraint already in place{{ else }}the pod running now is + replaced in the same rollout round, not merely joined{{ end }}. +{{- else }} + SPREAD OFF — nodes.{{ $group }}.spreadAcrossNodes is false. Nothing keeps two + pods of this group off the same node, so its replicas are + redundancy on paper only unless your own + nodes.{{ $group }}.topologySpreadConstraints or affinity does the job. +{{- end }} +{{- if and $s.podDisruptionBudget.enabled (ge $r 2) }} + PDB {{ $release }}-{{ $group }}-pdb, minAvailable {{ $s.podDisruptionBudget.minAvailable | int64 }}. A node drain cannot take + all {{ $r }} replicas of this group at once. +{{- else if $s.podDisruptionBudget.enabled }} + PDB NONE, because the effective replica count is {{ $r }} and a budget needs + two. Deliberate: {{ if eq $r 0 }}with no pod at all the object would only + describe a promise about nothing{{ else }}minAvailable 1 with a single replica + forbids every voluntary eviction, so `kubectl drain` would block + forever on a pod nothing may move — and the error would name the + PDB rather than the replica count{{ end }}. +{{- else }} + PDB NONE — nodes.{{ $group }}.podDisruptionBudget.enabled is false. A node + drain may take every replica of this group at once. +{{- end }} +{{- end }} +{{- if $anySpread }} + + ONE VERSION CAVEAT, AND IT FAILS SILENTLY: nodeAffinityPolicy is a beta field + — on by default from Kubernetes 1.26, GA with the feature gate + NodeInclusionPolicyInPodTopologySpread in 1.33. An older API server, or one + with the gate off, PRUNES IT WITHOUT A WORD, and then a nodeSelector or + affinity that narrows this group onto a single node no longer produces a + Pending pod but two CO-LOCATED ones, with the constraint reported as + satisfied. Compare the live object against what was rendered: + kubectl -n {{ .Release.Namespace }} get sts -l stalwart.jk-dev.de/release={{ $release }} \ + -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. + + AND A CEILING THAT IS A RULE, NOT A NUMBER: this chart leaves + nodeTaintsPolicy at its API default of Ignore, so every node in the cluster + counts as a domain — control-plane nodes included, where no Stalwart pod ever + runs. They hold the minimum at 0 permanently, so with maxSkew 1 a group can + never place more replicas than there are SCHEDULABLE WORKERS. It moves with + the cluster: add a worker and it rises, remove one and it falls. Past that + point a group needs spreadAcrossNodes: false or an own constraint with + ScheduleAnyway. This ceiling comes from nodeTaintsPolicy and existed before + nodeAffinityPolicy was set — the two fields are not a package. +{{- end }} {{- if .Values.ingress.enabled }} WEB UI AND JMAP diff --git a/stalwart/templates/_helpers.tpl b/stalwart/templates/_helpers.tpl index 3790233..89b05c9 100644 --- a/stalwart/templates/_helpers.tpl +++ b/stalwart/templates/_helpers.tpl @@ -354,6 +354,131 @@ accept SMTP behind the SAME Service object. {{- printf "stalwart.jk-dev.de/port-%s" . -}} {{- end -}} +{{/* +The labels that identify ONE node group's pods - and the ONLY definition of +them. Input: dict "root" "group" . Output: YAML. + +Three places consume this: the pod template's labels, the labelSelector of the +chart's own topology spread constraint, and the selector of the group's +PodDisruptionBudget. All three have to agree, and the failure of a disagreement +is silent in both of the latter two: + + - a spread constraint whose labelSelector matches no pod is TRIVIALLY + SATISFIED. It distributes nothing, the rollout is green, and nothing is + logged. With whenUnsatisfiable: DoNotSchedule that is worse rather than + better, because the object then looks like a guarantee. + - a PDB whose selector matches no pod reports ALLOWED DISRUPTIONS equal to + its own minAvailable arithmetic over zero pods and never blocks an + eviction, so a drain takes every replica at once - the thing the object was + added to prevent. + +One definition is the precedent set by stalwart.env.dbPassword in this file, +for exactly the same reason: a mismatch between two copies is not a render +error. ci/run-selector-match-test.sh is the second half of that - it holds +every rendered selector in this repository against the labels of the pod +templates in the same render, because one definition only helps while nobody +writes a second one. +*/}} +{{- define "stalwart.groupLabels" -}} +stalwart.jk-dev.de/release: {{ include "stalwart.fullname" .root }} +stalwart.jk-dev.de/node-group: {{ .group }} +{{- end -}} + +{{/* +The effective scheduling and disruption settings of one node group (#99 shape). +Input: dict "group" "given" . + +Only the two switches the chart interprets carry defaults. affinity, +nodeSelector, tolerations and topologySpreadConstraints are pass-through: the +chart never reads their contents, Kubernetes owns their schema, and a default +would be a value this chart cannot reason about. They therefore arrive here +unchanged, including absent. +*/}} +{{- define "stalwart.groupSettings" -}} +{{- $defaults := dict + "spreadAcrossNodes" true + "podDisruptionBudget" (dict "enabled" true "minAvailable" 1) -}} +{{- include "stalwart.defaultedDict" (dict "defaults" $defaults "given" .given "path" (printf "nodes.%s" .group)) -}} +{{- end -}} + +{{/* +The topologySpreadConstraints of one node group: the chart's own entry followed +by the user's, or nothing at all. +Input: dict "root" "group" "settings" + "replicas" . + +WHY THE CHART-OWNED ENTRY NEEDS >= 2 REPLICAS. With one pod the constraint is +satisfied by any placement, so it describes nothing; and the group that ships +with replicas: 0 ("mx") would carry an object about pods that do not exist. The +condition is on the COMPUTED count, so scaleDown: true removes it as well. The +side effect is the one that made this a minor rather than a major change: a +default install (full: 1, mx: 0) renders byte-for-byte what it rendered before. + +WHY nodeAffinityPolicy: Ignore IS SET EXPLICITLY, AND IT IS NOT A STYLE CHOICE. +The two node-inclusion policies have OPPOSITE defaults, straight from the API +definition: nodeAffinityPolicy defaults to Honor, nodeTaintsPolicy to Ignore. +Under Honor, only nodes matching the pod's nodeAffinity/nodeSelector count as +domains - so a nodeSelector narrowing a 2-replica group onto ONE node leaves one +eligible domain, both pods land in it, the skew is 0, and the constraint is +satisfied. Both replicas are co-located, nothing is Pending, no event is +emitted, and the selector in the object is correct - so the CI selector check +cannot see it either. With Ignore every node counts, the same narrowing gives +2,0,0, the skew is 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. + +Note what this field does NOT add: the ceiling on replicas per group comes from +nodeTaintsPolicy being Ignore, and it existed before this value did. values.yaml +argues the two separately for that reason. + +WHY A COLLISION ABORTS. Kubernetes keys a spread constraint on +(topologyKey, whenUnsatisfiable), whose API default is DoNotSchedule - so a +user entry on kubernetes.io/hostname without an explicit whenUnsatisfiable +collides with the chart's own. Both ways that collision resolves are worse than +an abort: `kubectl create` fails validation with a message naming a LIST INDEX +instead of a value, and an apply-based path merges the two entries over +patchMergeKey: topologyKey - SILENTLY, dropping one labelSelector and leaving a +green result. The abort names both values instead, and it only fires while the +chart's own entry is actually rendered, since without it there is no duplicate. +*/}} +{{- define "stalwart.spreadConstraints" -}} +{{- $root := .root -}} +{{- $group := .group -}} +{{- $settings := .settings -}} +{{- $own := list -}} +{{- if and $settings.spreadAcrossNodes (ge (int .replicas) 2) -}} +{{- $own = list (dict + "maxSkew" 1 + "topologyKey" "kubernetes.io/hostname" + "whenUnsatisfiable" "DoNotSchedule" + "nodeAffinityPolicy" "Ignore" + "labelSelector" (dict "matchLabels" (fromYaml (include "stalwart.groupLabels" (dict "root" $root "group" $group))))) -}} +{{- end -}} +{{- $given := $settings.topologySpreadConstraints -}} +{{- if kindIs "invalid" $given -}} +{{- $given = list -}} +{{- end -}} +{{- if not (kindIs "slice" $given) -}} +{{- fail (printf "nodes.%s.topologySpreadConstraints must be a list of topology spread constraints or null, got %s (%v)" $group (kindOf $given) $given) -}} +{{- end -}} +{{- if $own -}} +{{- range $i, $constraint := $given -}} +{{- if not (kindIs "map" $constraint) -}} +{{- fail (printf "nodes.%s.topologySpreadConstraints[%d] must be a mapping, got %s (%v)" $group $i (kindOf $constraint) $constraint) -}} +{{- end -}} +{{- $when := $constraint.whenUnsatisfiable | default "DoNotSchedule" -}} +{{- if and (eq ($constraint.topologyKey | default "") "kubernetes.io/hostname") (eq $when "DoNotSchedule") -}} +{{- fail (printf "nodes.%s.topologySpreadConstraints[%d] collides with the constraint nodes.%s.spreadAcrossNodes renders: both are keyed on topologyKey kubernetes.io/hostname with whenUnsatisfiable DoNotSchedule (the API default, so an omitted whenUnsatisfiable counts), and Kubernetes keys a spread constraint on exactly that pair. Two entries sharing it are rejected by `kubectl create` with a message naming a list index rather than a value, and on an apply-based path they are MERGED over patchMergeKey: topologyKey - silently, so one labelSelector is dropped and the result is green. Pick one: set nodes.%s.spreadAcrossNodes: false and keep your entry (it then owns the per-node distribution of this group, and the chart's nodeAffinityPolicy: Ignore is yours to repeat), or drop your entry and keep the chart's. Entries on any OTHER topologyKey - topology.kubernetes.io/zone, for instance - are appended and really do apply as an AND" $group $i $group $group) -}} +{{- end -}} +{{- end -}} +{{- end -}} +{{- $all := concat $own $given -}} +{{- if $all -}} +{{- toYaml $all -}} +{{- end -}} +{{- end -}} + {{/* The address of the in-memory store, and the render abort that goes with it. Input: the root context. Returns the host, or "" when none is needed. diff --git a/stalwart/templates/stalwart.pdb.yaml b/stalwart/templates/stalwart.pdb.yaml new file mode 100644 index 0000000..83c6862 --- /dev/null +++ b/stalwart/templates/stalwart.pdb.yaml @@ -0,0 +1,62 @@ +{{- /* +ONE PodDisruptionBudget PER NODE GROUP, and that is the whole difference from +the precedent. keycloak.pdb.yaml (Gitea #83, README guideline 11) set the +convention this file follows; what it did not have to deal with is a chart whose +replica count is a property of a GROUP rather than of the release. So the +condition is evaluated per group, and with full: 2 / mx: 1 exactly one object is +rendered. + +WHY IT EXISTS: with two replicas in a group a node drain is otherwise free to +take both at once - and a drain is the most common reason a pod moves, because +it is what a node upgrade does. Without this object the second replica buys +nothing against the failure it was added for. The chart's topology spread +constraint and this budget are two halves of one answer: the constraint decides +that the two pods sit on different nodes, the budget decides that only one of +those nodes may be emptied at a time. + +minAvailable rather than maxUnavailable, as in keycloak: the promise wanted is +about the service - at least one node of this group keeps answering throughout +the drain - and not about the size of the disruption. + +AND WHY IT IS NOT RENDERED BELOW TWO EFFECTIVE REPLICAS. This is the trap that +makes a PDB worth a condition rather than an unconditional object, quoted from +the precedent because it has not changed: minAvailable: 1 with exactly ONE +replica forbids every voluntary eviction, so `kubectl drain` blocks forever on a +pod nothing is allowed to move - and the message then names the PDB and not the +replica count, which sends the reader to the wrong file. The same holds for +scaleDown: true, where the group has no pods at all and the object would only +describe a promise about nothing. + +The count is therefore the COMPUTED one (stalwart.replicas), not the raw +nodes..replicas, which is what makes scaleDown: true remove the budget +rather than leave it behind. NOTES.txt prints, per group, which of the cases +applied, so an operator does not have to infer it from `kubectl get pdb`. + +THE SELECTOR COMES FROM stalwart.groupLabels, the same definition the pod +template's labels come from. A PDB whose selector matches no pod does not fail: +it reports its arithmetic over zero pods, never blocks an eviction, and a drain +takes the whole group while the object sits there looking like protection. +ci/run-selector-match-test.sh holds this selector against the rendered pod +templates on every pull request for exactly that reason. +*/ -}} +{{- $root := . -}} +{{- range $group, $spec := fromYaml (include "stalwart.nodes" .) }} +{{- $name := printf "%s-%s" (include "stalwart.fullname" $root) $group }} +{{- $replicas := int (include "stalwart.replicas" (dict "scaleDown" $root.Values.scaleDown "replicas" $spec.replicas)) }} +{{- $settings := fromYaml (include "stalwart.groupSettings" (dict "group" $group "given" $spec)) }} +{{- if and $settings.podDisruptionBudget.enabled (ge $replicas 2) }} +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: {{ $name }}-pdb + labels: + app: {{ $name }}-sfs + {{- include "stalwart.groupLabels" (dict "root" $root "group" $group) | nindent 4 }} +spec: + minAvailable: {{ $settings.podDisruptionBudget.minAvailable | int64 }} + selector: + matchLabels: + {{- include "stalwart.groupLabels" (dict "root" $root "group" $group) | nindent 6 }} +{{- end }} +{{- end }} diff --git a/stalwart/templates/stalwart.sfs.yaml b/stalwart/templates/stalwart.sfs.yaml index f0b0e6a..753cc25 100644 --- a/stalwart/templates/stalwart.sfs.yaml +++ b/stalwart/templates/stalwart.sfs.yaml @@ -23,6 +23,8 @@ are emptyDirs. That is also why there is no fsGroup in the pod context. {{- $valkeyHost := include "stalwart.valkeyHost" . -}} {{- range $group, $spec := fromYaml (include "stalwart.nodes" .) }} {{- $name := printf "%s-%s" (include "stalwart.fullname" $root) $group }} +{{- $replicas := int (include "stalwart.replicas" (dict "scaleDown" $root.Values.scaleDown "replicas" $spec.replicas)) }} +{{- $settings := fromYaml (include "stalwart.groupSettings" (dict "group" $group "given" $spec)) }} --- {{ if $valkeyHost -}} # In-memory store this release expects: {{ $valkeyHost }}:{{ $root.Values.externalServices.valkey.port }} @@ -36,10 +38,9 @@ metadata: name: {{ $name }}-sfs labels: app: {{ $name }}-sfs - stalwart.jk-dev.de/release: {{ include "stalwart.fullname" $root }} - stalwart.jk-dev.de/node-group: {{ $group }} + {{- include "stalwart.groupLabels" (dict "root" $root "group" $group) | nindent 4 }} spec: - replicas: {{ include "stalwart.replicas" (dict "scaleDown" $root.Values.scaleDown "replicas" $spec.replicas) }} + replicas: {{ $replicas }} serviceName: {{ $name }}-headless selector: matchLabels: @@ -51,8 +52,15 @@ spec: app: {{ $name }}-sfs # Scopes every per-port Service to THIS release, so a second stalwart # release in the same namespace does not get its pods selected. - stalwart.jk-dev.de/release: {{ include "stalwart.fullname" $root }} - stalwart.jk-dev.de/node-group: {{ $group }} + {{- /* These same two labels are also what the group's + PodDisruptionBudget and the chart's topology spread constraint + select on, which is why they come from ONE definition: a + selector matching no pod fails silently in both of those objects + - see the helper's comment. Kept as a rendered YAML comment above + rather than folded in here so that the manifest this chart + produced before Gitea #96 and the one it produces now are + byte-identical for the default values. */}} + {{- include "stalwart.groupLabels" (dict "root" $root "group" $group) | nindent 8 }} {{- /* One label per port this group may be reached on. The per-port Services select on these: NodePorts are cluster-globally unique, so "full" and "mx" can only accept SMTP behind the SAME @@ -81,6 +89,36 @@ spec: automountServiceAccountToken: false securityContext: {{- include "stalwart.podSecurityContext" $root.Values.securityContext | nindent 8 }} + {{- /* SCHEDULING. The first three are pass-through: the chart never reads + their contents, Kubernetes owns their schema, and an empty block + renders nothing at all rather than an empty field. + + MIND THE COMBINATION WITH THE SPREAD CONSTRAINT BELOW. A + nodeSelector or an affinity that narrows the eligible nodes below + the group's replica count turns the constraint into a permanent + Pending - which is the INTENDED, loud outcome (see values.yaml), + but it reads like a different problem if you have not been told. */}} + {{- with $settings.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with $settings.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with $settings.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- /* The chart's own per-node constraint plus any the user appended, or + nothing while the group is below two effective replicas. The helper + aborts the render on a collision instead of letting two entries + with the same (topologyKey, whenUnsatisfiable) be merged silently. */}} + {{- $spread := include "stalwart.spreadConstraints" (dict "root" $root "group" $group "settings" $settings "replicas" $replicas) }} + {{- if $spread }} + topologySpreadConstraints: + {{- $spread | nindent 8 }} + {{- end }} initContainers: # config.json is copied out of the ConfigMap into the emptyDir rather # than mounted from it, because STALWART WRITES THIS FILE ITSELF: on diff --git a/stalwart/values.schema.json b/stalwart/values.schema.json index 7398770..44d1879 100644 --- a/stalwart/values.schema.json +++ b/stalwart/values.schema.json @@ -191,6 +191,24 @@ "exposedPorts": { "$ref": "#/definitions/exposedPorts" }, + "spreadAcrossNodes": { + "$ref": "#/definitions/spreadAcrossNodes" + }, + "podDisruptionBudget": { + "$ref": "#/definitions/podDisruptionBudget" + }, + "affinity": { + "$ref": "#/definitions/podSpecObject" + }, + "nodeSelector": { + "$ref": "#/definitions/podSpecObject" + }, + "tolerations": { + "$ref": "#/definitions/podSpecArray" + }, + "topologySpreadConstraints": { + "$ref": "#/definitions/topologySpreadConstraints" + }, "resources": { "$ref": "#/definitions/resources" }, @@ -212,6 +230,24 @@ "exposedPorts": { "$ref": "#/definitions/exposedPorts" }, + "spreadAcrossNodes": { + "$ref": "#/definitions/spreadAcrossNodes" + }, + "podDisruptionBudget": { + "$ref": "#/definitions/podDisruptionBudget" + }, + "affinity": { + "$ref": "#/definitions/podSpecObject" + }, + "nodeSelector": { + "$ref": "#/definitions/podSpecObject" + }, + "tolerations": { + "$ref": "#/definitions/podSpecArray" + }, + "topologySpreadConstraints": { + "$ref": "#/definitions/topologySpreadConstraints" + }, "resources": { "$ref": "#/definitions/resources" }, @@ -233,6 +269,24 @@ "exposedPorts": { "$ref": "#/definitions/exposedPorts" }, + "spreadAcrossNodes": { + "$ref": "#/definitions/spreadAcrossNodes" + }, + "podDisruptionBudget": { + "$ref": "#/definitions/podDisruptionBudget" + }, + "affinity": { + "$ref": "#/definitions/podSpecObject" + }, + "nodeSelector": { + "$ref": "#/definitions/podSpecObject" + }, + "tolerations": { + "$ref": "#/definitions/podSpecArray" + }, + "topologySpreadConstraints": { + "$ref": "#/definitions/topologySpreadConstraints" + }, "resources": { "$ref": "#/definitions/resources" }, @@ -410,6 +464,31 @@ "minLength": 1 } }, + "spreadAcrossNodes": { + "description": "Renders a topologySpreadConstraints entry for THIS group on topologyKey kubernetes.io/hostname, maxSkew 1, whenUnsatisfiable DoNotSchedule, nodeAffinityPolicy Ignore, with a labelSelector the chart fills from its own pod labels - the user never writes it, because a selector that matches no pod is trivially satisfied, distributes nothing and reports nothing. Rendered only while the group's COMPUTED replica count is >= 2, so scaleDown: true removes it as well. DoNotSchedule makes it a promise rather than an effort, and the price is a Pending pod: when two of three workers are gone (while their Node objects still exist), or when a nodeSelector/affinity narrows the eligible nodes below the replica count. nodeAffinityPolicy is beta - on by default from Kubernetes 1.26, GA with the NodeInclusionPolicyInPodTopologySpread gate in 1.33 - and is pruned SILENTLY on anything older, which brings back exactly the co-location it was set against. The replica ceiling (as many per group as there are schedulable workers, because control-plane domains hold the minimum at 0) comes from nodeTaintsPolicy and is NOT a consequence of that field. The whole rule holds only while this chart stays PVC-free - see open measurement 1 on #65.", + "type": "boolean" + }, + "podDisruptionBudget": { + "description": "One PodDisruptionBudget per node group, rendered only while that group's COMPUTED replica count is >= 2 (so scaleDown: true renders none). The condition is the point, not tidiness: 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 rather than the replica count. minAvailable rather than maxUnavailable, as in keycloak: the promise is about the service. The selector comes from the same definition as the pod template's labels, because a PDB matching no pod never blocks an eviction while looking like protection.", + "type": ["object", "null"], + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "minAvailable": { "type": "integer", "minimum": 1 } + } + }, + "podSpecObject": { + "description": "Passed verbatim into this group's pod spec (affinity, nodeSelector). Loosely typed on purpose, as with the probes and the security contexts: Kubernetes owns this schema, the chart reads none of the contents, and a stricter copy here would reject valid configurations rather than catch mistakes. Declaring the key is not optional even so, because the level above carries additionalProperties: false. Empty renders no field at all. Mind that narrowing the eligible nodes below a group's replica count while spreadAcrossNodes is on leaves a replica permanently Pending - intended, and loud by design.", + "type": ["object", "null"] + }, + "podSpecArray": { + "description": "Passed verbatim into this group's pod spec (tolerations). Loosely typed for the same reason as podSpecObject. Note that tolerating a taint does not raise the replica ceiling: with nodeTaintsPolicy at its default of Ignore an untolerated node already counts as an empty domain.", + "type": ["array", "null"] + }, + "topologySpreadConstraints": { + "description": "Additional constraints for this group, APPENDED after the chart's own and really AND-ed with it, so a second key such as topology.kubernetes.io/zone works as expected. Loosely typed like the other pass-throughs. An entry on kubernetes.io/hostname with whenUnsatisfiable DoNotSchedule - or omitted, which IS DoNotSchedule - aborts the render naming both this value and spreadAcrossNodes: Kubernetes keys a constraint on that pair, kubectl create rejects the duplicate with a message about a list index instead of a value, and an apply-based path merges the two over patchMergeKey: topologyKey silently, dropping one labelSelector for a green result.", + "type": ["array", "null"] + }, "externalService": { "type": ["object", "null"], "additionalProperties": false, diff --git a/stalwart/values.yaml b/stalwart/values.yaml index 1dcb080..37ce67b 100644 --- a/stalwart/values.yaml +++ b/stalwart/values.yaml @@ -300,6 +300,137 @@ nodes: # Every group needs http, including mx: it is the port all three probes # use. It is not published for mx. exposedPorts: [smtp, submissions, imaps, http] + + # ----------------------------------------------------------------------- + # Distribute THIS GROUP's pods over distinct nodes (Gitea #96) + # ----------------------------------------------------------------------- + # Renders a topologySpreadConstraints entry on topologyKey + # kubernetes.io/hostname with maxSkew: 1 and a labelSelector the CHART + # fills in — the two labels it puts on its own pods anyway. You never + # write that selector, and that is the point: a spread constraint whose + # selector matches no pod is TRIVIALLY SATISFIED. It distributes nothing, + # the rollout is green, and nothing anywhere says so. + # + # ONLY WITHIN THIS 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. + # + # RENDERED ONLY FROM TWO EFFECTIVE REPLICAS UPWARDS. With one pod any + # placement satisfies it, so it would describe nothing; the count used is + # the computed one, so scaleDown: true removes it too. + # + # whenUnsatisfiable IS DoNotSchedule, i.e. THIS IS A PROMISE AND NOT AN + # EFFORT — and the price is a pod that stays Pending rather than one that + # lands somewhere useless. ScheduleAnyway was the original proposal and was + # rejected: with one worker at 97 % requested CPU the scheduler may put both + # replicas of a group on the same remaining node, entirely within the rule + # and with the redundancy gone. The cases where the promise costs something: + # + # - TWO OF THREE WORKERS GONE: the second replica stays Pending instead + # of joining the survivor — but only WHILE THE Node OBJECTS STILL + # EXIST. A NotReady node still counts as an (empty) domain, so the + # third replica 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. + # - A NARROWING nodeSelector/affinity BELOW: see nodeSelector further + # down. That combination blocks itself, deliberately and loudly. + # + # A Pending pod with no scheduling event reads like a different problem + # than a distribution rule, so NOTES.txt names this rule whenever it is + # rendered. + # + # THE FIELD THAT CAN BE DROPPED WITHOUT A WORD: the constraint carries + # nodeAffinityPolicy: Ignore, and that field is BETA — on by default since + # Kubernetes 1.26, graduated to GA with the feature gate + # NodeInclusionPolicyInPodTopologySpread in 1.33. On an API server older + # than 1.26, or one with the gate switched off, the field is pruned + # SILENTLY, and the gap it was set against comes straight back (the + # nodeSelector case below). Nothing reports it: the object still looks + # correct in `kubectl get sts -o yaml`, minus one line. So 1.26 is the + # minimum version this chart can make the promise on at all. NOT MEASURED + # against an older server — this is the documented pruning behaviour for a + # field behind a disabled gate, not something tried here. Check the + # rendered StatefulSet against the live one if the server is near that + # boundary. + # + # WHAT IS *NOT* A CONSEQUENCE OF THAT FIELD, because the two + # node-inclusion policies are not a package: the ceiling on replicas + # described at nodeSelector below comes from nodeTaintsPolicy, which this + # chart leaves at its own default. It was there before nodeAffinityPolicy + # was set and is not its price. + # + # ONE PRECONDITION, AND IT IS NOT IN THIS FILE: this works because the + # StatefulSet has NO volumeClaimTemplates. Open measurement 1 on #65 asks + # whether anything under /var/lib/stalwart has to survive a restart; if + # that answer ever becomes yes, a PVC per group appears, and with a + # node-local or topology-bound storage class the pod is pinned 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. This rule holds only while the chart + # stays PVC-free. + # + # AND THE COST OF TURNING IT ON BY SCALING UP: going from 1 to 2 replicas + # changes the POD TEMPLATE (the constraint appears in it), so the existing + # pod is replaced in the same rollout round rather than merely joined by a + # second one. Expect a restart of the running node, not just an addition. + spreadAcrossNodes: true + + # A PodDisruptionBudget for this group, rendered only while the group's + # COMPUTED replica count is >= 2 — the spread constraint's companion: that + # one decides the two pods sit on different nodes, this one decides only + # one of those nodes may be emptied at a time. + # + # The condition is not tidiness. 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 rather than the + # replica count, which sends the reader to the wrong file. Precedent and + # full argument: keycloak (README, guideline 11). + podDisruptionBudget: + enabled: true + # minAvailable, not maxUnavailable: the promise is about the service — + # at least one pod of this group keeps answering through a drain. + minAvailable: 1 + + # Generic pass-through into this group's pod spec. Rendered verbatim with + # toYaml; the chart reads none of their contents, which is why they are + # loosely typed in values.schema.json — Kubernetes owns their schema. An + # empty value renders no field at all. + # + # THE ONE COMBINATION THAT BLOCKS ITSELF, and it does so on purpose: + # narrowing the eligible nodes below this group's replica count while + # spreadAcrossNodes is on leaves a replica permanently Pending. With + # nodeAffinityPolicy: Ignore on the chart's constraint, a nodeSelector + # down to one node gives 2,0,0 — skew 2 — rather than the one eligible + # domain and skew 0 it would give under the API default of Honor. The + # second shape is what we do NOT want: both pods co-located, nothing + # Pending, no event, and a selector that is perfectly correct, so no check + # in CI can see it either. Loud beats silent; that is the whole trade. + # + # AND THE CEILING, WHICH IS A RULE AND NOT A NUMBER. The chart leaves + # nodeTaintsPolicy at its API default of Ignore, so EVERY node counts as a + # domain — control-plane nodes included, and no Stalwart pod ever runs + # there. Those domains hold the minimum at 0 permanently, so with + # maxSkew: 1 a group can never place more replicas than there are + # SCHEDULABLE WORKERS. Add a worker and the ceiling rises with it; remove + # one and it falls. Do not write today's count down here — it would be + # wrong at the next cluster rebuild and nobody would come back for it. + # Raising a group past that ceiling needs spreadAcrossNodes: false, or an + # own topologySpreadConstraints entry with ScheduleAnyway. + affinity: {} + nodeSelector: {} + tolerations: [] + # Appended AFTER the chart's own constraint and really AND-ed with it, so + # a second key — topology.kubernetes.io/zone, say — works as expected. + # + # An entry on kubernetes.io/hostname with whenUnsatisfiable DoNotSchedule + # (or omitted, which IS DoNotSchedule) ABORTS THE RENDER naming both this + # value and spreadAcrossNodes. Kubernetes keys a constraint on exactly + # that pair: `kubectl create` rejects the duplicate with a message about a + # list index instead of a value, and an apply-based path merges the two + # over patchMergeKey: topologyKey SILENTLY — one labelSelector is dropped + # and the result is green. A named abort is better than either. + topologySpreadConstraints: [] resources: requests: cpu: 200m @@ -324,6 +455,25 @@ nodes: # fail-open above), which is the opposite of what this group is for. replicas: 0 exposedPorts: [smtp, http] + # Same six keys as nodes.full, same defaults, and argued in full there — + # including the three things that are easy to get wrong: that + # nodeAffinityPolicy can be pruned silently below Kubernetes 1.26, that the + # replica ceiling is "as many as there are schedulable workers" rather than + # a number, and that the ceiling comes from nodeTaintsPolicy and not from + # the field this chart sets. + # + # Neither object is rendered for this group while replicas is 0, and the + # condition is on the computed count: raising it to 2 (after the "mx" + # ClusterRole exists — see above) brings both the spread constraint and the + # PodDisruptionBudget with it, independently of what "full" does. + spreadAcrossNodes: true + podDisruptionBudget: + enabled: true + minAvailable: 1 + affinity: {} + nodeSelector: {} + tolerations: [] + topologySpreadConstraints: [] resources: requests: cpu: 100m