Add PodDisruptionBudgets and topologySpreadConstraints support by henzigo · Pull Request #48 · shopsys/helm

henzigo · 2026-08-19T02:23:31Z

Closes #8

Stacked on #47 — only the last commit belongs to this PR; GitHub retargets to main after #47 merges.

What

PodDisruptionBudgets for webserver and storefront (pdb: {enabled: true, minAvailable: 1}). Rendered only when the component is guaranteed to run 2+ replicas — autoscaling.enabled or replicas > 1. A minAvailable: 1 budget on a single-replica Deployment would block node drains forever, so single-replica setups (bare chart defaults) get no PDB.
topologySpreadConstraints as a new standard component key on every workload in both charts (webserver, storefront, cron, consumers, redis, rabbitmq), rendered by the shared shopsys.podSettings helper. Empty by default — the legacy preferredDuringScheduling anti-affinity defaults are kept untouched for parity, per the issue.

Notes

Golden impact: all 5 scenarios render both PDBs (every scenario runs with autoscaling enabled, matching legacy ENABLE_AUTOSCALING); dev scenarios keep their HPA min=max=2 and therefore also get the budgets.
Deviations entry 22; docs/values.md standard-keys table + component notes updated.
New pdb_test.yaml suite (5 cases: gating off at 1 replica, on with autoscaling, on with fixed replicas > 1, minAvailable override, explicit disable) + a topologySpreadConstraints passthrough test. 68 unit tests total.

🤖 Generated with Claude Code

Webserver and storefront run 2+ replicas in production but nothing protected them from voluntary disruptions - a node drain could evict all replicas at once. Each component now gets a minAvailable: 1 PDB, rendered only when it is guaranteed to run 2+ replicas (autoscaling enabled or replicas > 1) so a single-replica setup never ends up with a drain-blocking budget. topologySpreadConstraints joins the standard component keys (all workloads in both charts, empty by default); the legacy anti-affinity defaults stay untouched for parity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR improves workload availability and scheduling flexibility in the Helm charts by introducing values-gated PodDisruptionBudgets (PDBs) for the webserver and storefront components, and by adding topologySpreadConstraints as a standard component key rendered via the shared shopsys.podSettings helper.

Changes:

Add new PodDisruptionBudget resources for webserver and storefront, gated by replica count/autoscaling.
Add topologySpreadConstraints passthrough to the shared pod settings helper and expose the value key across components (app + infra).
Update docs, golden snapshots, and helm-unittest coverage for the new behavior.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/golden/scenarios/production-with-cloudflare/expected/first-deploy.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/production-with-cloudflare/expected/first-deploy-with-demo-data.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/production-with-cloudflare/expected/continuous.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/escaping-env/expected/first-deploy.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/escaping-env/expected/first-deploy-with-demo-data.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/escaping-env/expected/continuous.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/development-with-cloudflare/expected/first-deploy.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/development-with-cloudflare/expected/first-deploy-with-demo-data.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/development-with-cloudflare/expected/continuous.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/development-single-domain/expected/first-deploy.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/development-single-domain/expected/first-deploy-with-demo-data.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/development-single-domain/expected/continuous.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/basic-production/expected/first-deploy.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/basic-production/expected/first-deploy-with-demo-data.yaml	Golden snapshot updated to include rendered PDBs.
tests/golden/scenarios/basic-production/expected/continuous.yaml	Golden snapshot updated to include rendered PDBs.
docs/values.md	Document `topologySpreadConstraints` as a standard component key; document PDB behavior notes.
docs/migrating-from-shopsys-deployment.md	Add deviation entry documenting PDBs + topologySpreadConstraints availability.
charts/shopsys-infra/values.yaml	Expose `topologySpreadConstraints` defaults for infra components.
charts/shopsys-common/templates/_helpers.tpl	Render `topologySpreadConstraints` via shared `shopsys.podSettings`.
charts/shopsys-app/values.yaml	Add PDB configuration defaults and `topologySpreadConstraints` defaults for app components.
charts/shopsys-app/tests/pdb_test.yaml	Add helm-unittest suite for PDB rendering rules and overrides.
charts/shopsys-app/tests/component_schema_test.yaml	Add helm-unittest asserting topologySpreadConstraints passthrough.
charts/shopsys-app/templates/pdb-webserver.yaml	New PDB template for webserver.
charts/shopsys-app/templates/pdb-storefront.yaml	New PDB template for storefront.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

henzigo · 2026-08-20T10:21:03Z

+{{/* Rendered only when the webserver is guaranteed to run with 2+ replicas — a PDB with
+     minAvailable: 1 on a single-replica Deployment would block node drains forever. */}}
+{{- $w := .Values.webserver }}
+{{- if and $w.pdb.enabled (or $w.autoscaling.enabled (gt (int $w.replicas) 1)) }}


Fixed in c2aeb51 — valid catch. The autoscaling arm now also requires minReplicas > 1:

{{- if and ($w.pdb).enabled (or (and $w.autoscaling.enabled (gt (int $w.autoscaling.minReplicas) 1)) (gt (int $w.replicas) 1)) }}

Verified that autoscaling.enabled=true + minReplicas=1 no longer renders the PDB, and added a unit test for exactly that case. Golden snapshots are unchanged (all scenarios run with minReplicas: 2).

henzigo · 2026-08-20T10:21:07Z

+{{/* Rendered only when the storefront is guaranteed to run with 2+ replicas — a PDB with
+     minAvailable: 1 on a single-replica Deployment would block node drains forever. */}}
+{{- $s := .Values.storefront }}
+{{- if and $s.pdb.enabled (or $s.autoscaling.enabled (gt (int $s.replicas) 1)) }}


Fixed in c2aeb51 with the same gate as pdb-webserver.yaml: the autoscaling arm now requires minReplicas > 1 (gt (int $s.autoscaling.minReplicas) 1), so a storefront that can scale down to a single pod gets no minAvailable: 1 budget. Covered by a new unit test; goldens unchanged.

henzigo · 2026-08-20T10:21:16Z

    `cron.resources` and `consumers.defaults.resources` (or per instance).
+22. **PodDisruptionBudgets for webserver and storefront**: the legacy package had none — a
+    node drain could evict all replicas at once. The chart now renders a
+    `minAvailable: 1` PDB per component whenever it runs 2+ replicas (autoscaling enabled


Updated in c2aeb51: deviations entry 22 now says "guaranteed to run 2+ replicas (autoscaling enabled with minReplicas > 1, or fixed replicas > 1)", matching the tightened template gate. The docs/values.md component note and the values.yaml comments were reworded the same way.

henzigo · 2026-08-20T10:21:21Z

+  - it: renders the PDB when autoscaling is enabled
+    template: templates/pdb-webserver.yaml
+    set:
+      webserver:
+        autoscaling:
+          enabled: true
+    asserts:
+      - hasDocuments:
+          count: 1
+      - equal:
+          path: spec.minAvailable
+          value: 1
+      - equal:
+          path: spec.selector.matchLabels.app
+          value: webserver-php-fpm
+


Added in c2aeb51: "renders no PDB when autoscaling can scale down to a single replica" (autoscaling enabled, minReplicas: 1, asserts zero documents). It fails against the old gate and passes with the tightened one. Also added a companion case for a nulled pdb: map raised in another review thread.

henzigo

Reviewed the #48 diff against its base cron-consumer-resource-defaults (stacked PR; base-branch changes excluded): the two new PDB templates, the topologySpreadConstraints passthrough in shopsys.podSettings, values/docs updates, unit tests, and the regenerated golden snapshots.

Verified locally (after helm dependency build on both charts):

./tests/run-golden-tests.sh — 15/15 pass
helm unittest charts/shopsys-app charts/shopsys-infra — 68/68 pass
helm lint both charts with baseline values — clean
kubeconform -strict -kubernetes-version 1.31.0 on all rendered golden output — 561/561 valid (confirms policy/v1 PDBs are schema-correct)
PDB selectors (app: webserver-php-fpm, app: storefront) match the Deployments' immutable selectors exactly; PDB names match the Deployment names; standard labels included.

Findings (severity-ranked):

Medium — the autoscaling arm of the PDB gate ignores minReplicas. autoscaling.enabled alone does not guarantee 2+ replicas: with minReplicas: 1 (nothing in the schema prevents it) the HPA can scale to a single pod while the minAvailable: 1 PDB still exists, blocking node drains — exactly the footgun the gate is meant to prevent. Verified: --set webserver.autoscaling.enabled=true --set webserver.autoscaling.minReplicas=1 renders the PDB. See inline comment.
Low — nil-pointer on pdb: null overlay. $w.pdb.enabled errors with nil pointer evaluating interface {}.enabled when an environment overlay sets webserver.pdb: to null/empty (verified with --set-json 'webserver.pdb=null'). The repo's own template guideline is to guard field access on possibly-missing maps. See inline comment.
Info — topologySpreadConstraints passthrough could use a docs hint. Raw passthrough is the right design (consistent with affinity), but since the user supplies the whole constraint, a one-line recommendation in docs/values.md would prevent the two classic mistakes: whenUnsatisfiable: DoNotSchedule deadlocking small/dev clusters (prefer ScheduleAnyway unless spread is a hard requirement), and omitting matchLabelKeys: [pod-template-hash] (beta and enabled by default since K8s 1.27; the chart is validated against 1.31), which makes the spread count old-ReplicaSet pods during rolling updates.

Non-findings, for the record: the choice to give cron no PDB is correct (cron is 0/1-replica and self-scales via the cron-suspend hook — a PDB would block drains); redis/rabbitmq correctly get only topologySpreadConstraints and no PDB (single-replica stateful); minAvailable renders unquoted, which handles both integers and "50%" strings correctly; values.schema.json needs no change since it does not enumerate component keys; docs (values.md, deviations entry 22), golden snapshots, and unit tests (gating on/off, override, disable, passthrough) are all updated in the same commit. Consumers instances with replicas > 1 get no PDB — acceptable as scoped, but worth a follow-up issue if consumer HA matters.

Overall a well-scoped, well-tested change; only finding 1 is worth fixing before merge.

— Claude Agent (CR)

henzigo · 2026-08-20T09:47:35Z

+{{/* Rendered only when the webserver is guaranteed to run with 2+ replicas — a PDB with
+     minAvailable: 1 on a single-replica Deployment would block node drains forever. */}}
+{{- $w := .Values.webserver }}
+{{- if and $w.pdb.enabled (or $w.autoscaling.enabled (gt (int $w.replicas) 1)) }}


The autoscaling.enabled arm assumes 2+ replicas, but that only holds when minReplicas >= 2. With webserver.autoscaling.minReplicas: 1 (allowed — the schema does not constrain it) this still renders the PDB, and once the HPA scales down to 1 pod, minAvailable: 1 blocks node drains — the exact footgun the header comment says this gate prevents. Verified: helm template --set webserver.autoscaling.enabled=true --set webserver.autoscaling.minReplicas=1 -s templates/pdb-webserver.yaml renders the budget.

Suggest tightening the guard (here and in pdb-storefront.yaml):

{{- if and $w.pdb.enabled (or (and $w.autoscaling.enabled (gt (int $w.autoscaling.minReplicas) 1)) (gt (int $w.replicas) 1)) }}

and adding a minReplicas: 1 case to pdb_test.yaml. — Claude Agent (CR)

Implemented in c2aeb51 essentially as suggested — the guard in both templates is now:

{{- if and ($w.pdb).enabled (or (and $w.autoscaling.enabled (gt (int $w.autoscaling.minReplicas) 1)) (gt (int $w.replicas) 1)) }}

(also with the parenthesized ($w.pdb) access from the sibling finding), plus the minReplicas: 1 unit test. Re-verified your repro: --set webserver.autoscaling.enabled=true --set webserver.autoscaling.minReplicas=1 now renders nothing. Docs and values comments updated to describe the actual gate.

henzigo · 2026-08-20T09:47:35Z

+{{/* Rendered only when the storefront is guaranteed to run with 2+ replicas — a PDB with
+     minAvailable: 1 on a single-replica Deployment would block node drains forever. */}}
+{{- $s := .Values.storefront }}
+{{- if and $s.pdb.enabled (or $s.autoscaling.enabled (gt (int $s.replicas) 1)) }}


$s.pdb.enabled nil-pointers when an environment overlay sets storefront.pdb: to null (verified: --set-json 'storefront.pdb=null' fails with nil pointer evaluating interface {}.enabled). Chart defaults normally deep-merge the map back in, but an explicit pdb: with commented-out children in an overlay produces a cryptic render error at deploy time. Per the repo guideline (guard field access on possibly-missing maps), ($s.pdb).enabled — with $s.pdb.minAvailable | default 1 below — keeps it robust. Same applies to pdb-webserver.yaml. — Claude Agent (CR)

Fixed in c2aeb51: both templates now use ($s.pdb).enabled / ($w.pdb).enabled, and minAvailable renders as {{ $s.pdb.minAvailable | default 1 }}. A nulled pdb: overlay now renders no PDB instead of failing with the nil-pointer error (re-verified with --set-json 'storefront.pdb=null'), and a pdb map replaced without minAvailable falls back to 1 instead of emitting an empty field. Added a unit test for the pdb: null case. I kept the autoscaling.* accesses unguarded to match the existing pattern in the HPA/Deployment templates (chart defaults define that map in full).

henzigo · 2026-08-20T09:47:35Z

 | `resources` | container resources |
 | `podAnnotations` / `podLabels` | extra pod metadata |
-| `nodeSelector` / `tolerations` / `affinity` / `priorityClassName` | scheduling |
+| `nodeSelector` / `tolerations` / `affinity` / `topologySpreadConstraints` / `priorityClassName` | scheduling |


Since topologySpreadConstraints is a raw passthrough, consider a one-line usage hint here: prefer whenUnsatisfiable: ScheduleAnyway unless spread is a hard requirement (DoNotSchedule can leave pods Pending on small/dev clusters), and add matchLabelKeys: [pod-template-hash] (beta, enabled by default since K8s 1.27; the chart is validated against 1.31) so the spread does not count old-ReplicaSet pods during rolling updates. Purely a docs suggestion — the passthrough design itself is right and consistent with affinity. — Claude Agent (CR)

Added in c2aeb51: a short note under the standard-keys table in docs/values.md recommending whenUnsatisfiable: ScheduleAnyway unless spread is a hard requirement, and matchLabelKeys: [pod-template-hash] so the spread ignores old-ReplicaSet pods during rolling updates. Kept it to one paragraph since the key is a raw passthrough by design.

Review findings on #48: autoscaling.enabled alone does not guarantee 2+ replicas - with minReplicas: 1 the HPA can scale to a single pod while the minAvailable: 1 budget still exists, blocking node drains (the exact footgun the gate exists to prevent). The autoscaling arm now also requires minReplicas > 1. Also guard the pdb map access - an overlay setting `pdb:` to null previously failed the render with a cryptic nil-pointer error; it now simply renders no PDB - and default minAvailable to 1 when the map is replaced without it. Docs (deviations entry 22, values.md, values.yaml comments) updated to match the actual gate; values.md gains a short topologySpreadConstraints usage hint (ScheduleAnyway vs DoNotSchedule, matchLabelKeys: [pod-template-hash]). Two new unit tests cover the minReplicas: 1 and pdb: null cases. Golden snapshots are unchanged (all scenarios run with minReplicas: 2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… into pdb-topology-spread # Conflicts: # docs/migrating-from-shopsys-deployment.md

Typos in the new keys (e.g. an unsupported maxUnavailable) now fail the render instead of being silently ignored. pdb accepts null - the documented overlay opt-out - and minAvailable accepts both an integer and a percentage string, matching the PDB API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

henzigo requested a lite review from Copilot August 19, 2026 02:25

Copilot started reviewing on behalf of henzigo August 19, 2026 02:25 View session

henzigo mentioned this pull request Aug 19, 2026

Run workloads under dedicated ServiceAccounts without API tokens #49

Open

Copilot AI reviewed Aug 19, 2026

View reviewed changes

henzigo commented Aug 20, 2026

View reviewed changes

henzigo and others added 3 commits August 20, 2026 12:19

Merge remote-tracking branch 'origin/cron-consumer-resource-defaults'…

d24e469

… into pdb-topology-spread # Conflicts: # docs/migrating-from-shopsys-deployment.md

Navigation Menu

Uh oh!

Add PodDisruptionBudgets and topologySpreadConstraints support - #48

Add PodDisruptionBudgets and topologySpreadConstraints support#48
henzigo wants to merge 4 commits into
cron-consumer-resource-defaultsfrom
pdb-topology-spread

henzigo commented Aug 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo left a comment

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

henzigo Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

henzigo commented Aug 19, 2026

What

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henzigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants