Ship default resource requests/limits for cron and consumers

[henzigo]

Closes #4

What

Ships conservative default resource requests/limits for the cron and consumers components, replacing the legacy BestEffort behavior (no requests/limits at all):

Component	requests	limits
cron.resources	100m / 300Mi	1Gi memory
consumers.defaults.resources	50m / 300Mi	1Gi memory

Both run the same PHP application image as the webserver; the numbers are deliberately conservative and overridable per component, per consumer instance, and per environment overlay (same as every other component).

Notes

• Golden impact: every cron/consumer container across all 5 scenarios gains the resources block — nothing else changes.
• Deviations entry 21 added to docs/migrating-from-shopsys-deployment.md; docs/values.md updated.
• New unit tests: cron defaults + deep-merge override semantics, consumer defaults + per-instance override (62 tests total).
• The issue's "verify against production metrics of a reference project" task remains open — the defaults should be sanity-checked against real pilot metrics before/after merge; they are values, so tuning later is a one-line overlay change.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR introduces conservative default Kubernetes resource requests/limits for the cron and consumers components in the shopsys-app chart, moving these workloads away from legacy BestEffort QoS behavior while keeping overrides possible via values overlays and per-instance configuration.

Changes:

• Add default resources for cron and consumers.defaults in charts/shopsys-app/values.yaml.
• Update helm-unittest coverage to assert default resources and override/merge behavior for cron and consumer instances.
• Regenerate golden snapshots and update docs to record the intentional deviation and document the new defaults.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
charts/shopsys-app/values.yaml	Adds default resource requests/limits for cron and consumers defaults.
charts/shopsys-app/tests/cron_test.yaml	Adds unit tests asserting cron default resources and override behavior.
charts/shopsys-app/tests/component_schema_test.yaml	Adds unit test coverage for consumer default resources and per-instance override.
docs/values.md	Documents the new default resources for cron and consumers.
docs/migrating-from-shopsys-deployment.md	Records the change as an intentional deviation (entry 21).
tests/golden/scenarios/basic-production/expected/first-deploy.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/basic-production/expected/first-deploy-with-demo-data.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/basic-production/expected/continuous.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/development-single-domain/expected/first-deploy.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/development-single-domain/expected/first-deploy-with-demo-data.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/development-single-domain/expected/continuous.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/development-with-cloudflare/expected/first-deploy.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/development-with-cloudflare/expected/first-deploy-with-demo-data.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/development-with-cloudflare/expected/continuous.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/escaping-env/expected/first-deploy.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/escaping-env/expected/first-deploy-with-demo-data.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/escaping-env/expected/continuous.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/production-with-cloudflare/expected/first-deploy.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/production-with-cloudflare/expected/first-deploy-with-demo-data.yaml	Updates golden manifests to include resources blocks for cron/consumers.
tests/golden/scenarios/production-with-cloudflare/expected/continuous.yaml	Updates golden manifests to include resources blocks for cron/consumers.

---

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

[henzigo]

Review of #47 (diff vs main only — stacked chain ignored)

What I reviewed/verified in a checked-out worktree of the branch:

• ./tests/run-golden-tests.sh — 15/15 pass; snapshots for all 5 scenarios × 3 variants regenerated in the same commit as the values change (per convention).
• helm unittest charts/shopsys-app charts/shopsys-infra — 62/62 pass, including the new cron default/deep-merge tests and the consumer default/per-instance-override test.
• Merge semantics verified by actual rendering: chart-default → env-overlay deep-merge works for cron.resources (partial override keeps the untouched keys); per-instance consumer override works via mergeOverwrite (deepCopy $.Values.consumers.defaults) .; opt-out via cron.resources: null and per-instance resources: null both cleanly drop the block (the with guard handles it). One asymmetric footgun found (inline comment).
• CPU limit deliberately omitted while a CPU request is set — this is the right call (CPU limits only cause throttling; memory limits are the ones that matter) and it is consistent with the existing webserver.phpFpm, webserver.nginx, and storefront defaults, which follow the same requests-CPU/limits-memory-only shape.
• Consistency: cron and consumers were the last Deployment components still defaulting to resources: {}; this PR closes that gap without touching selectors, standard component keys, or template logic (values-only + docs + tests). Deviation entry 21 and docs/values.md updated as required.

Findings (severity-ranked):

1. Medium — 1Gi memory limit is the risky half of the change (inline on values.yaml): crons/consumers that previously could burst arbitrarily as BestEffort will now be OOMKilled above 1Gi. The PR honestly flags the open "verify against production metrics" task — I'd treat that as a gate for tagging a release consumed by real projects.
2. Low — per-instance nested null renders an invalid manifest (inline on values.yaml): resources: null on a consumer instance opts out cleanly, but resources: {limits: {memory: null}} survives mergeOverwrite and renders a literal memory: null, which fails K8s schema validation (verified with kubeconform 1.31.0). Chart-level cron.resources nulls are fine (Helm's coalesce strips them). Worth one sentence of docs.
3. Low — deviation entry 21 omits the QoS trade-off (inline on the docs): BestEffort → Burstable with request (300Mi) far below limit (1Gi) means node overcommit and eviction-ordering changes; the entry lists the numbers but not this implication.
4. Nit — no test for the documented opt-out path (inline on cron_test.yaml).

Out-of-diff observation (not actionable here): the migrate-application and post-deploy hook Jobs keep resources: {} defaults even though they run the heaviest PHP workloads (demo data import, elasticsearch export). If leaving hooks BestEffort is deliberate (safer for first deploys), fine — otherwise it may deserve a follow-up issue.

No correctness, security, or convention violations found beyond the above; the change is well-scoped, well-tested, and honestly documented.

— Claude Agent (CR)

[henzigo]

Medium: This 1Gi memory limit is the actual behavior-risk half of the deviation from legacy BestEffort. The requests are harmless (scheduling hints), but a limit converts "could burst" into "gets OOMKilled": Shopsys cron jobs include memory-heavy work (feed generation, image processing, elasticsearch export), and PHP CLI commonly runs with memory_limit=-1, so nothing inside the container will stop a job before the kernel does. A cron that today peaks at, say, 1.2Gi on a pilot project will silently start dying after upgrading to this chart version.

The PR description already flags the open "verify against production metrics" task — good. Two suggestions:

• Treat that verification as a gate before a release that real projects consume, since the failure mode (nightly cron OOMKilled) is quiet and delayed.
• Consider adding one sentence to deviation 21 telling upgraders to check their crons' peak memory usage before adopting the defaults.

Not blocking — the value is overridable per environment and the trade-off is a judgment call.

— Claude Agent (CR)

[henzigo]

Agreed that the 1Gi limit is the risky half. Two-part response in d74fdc4:

• Deviation entry 21 now spells out the upgrade check: QoS moves from BestEffort to Burstable, bursts past 1Gi are OOMKilled instead of merely evictable, and upgraders should check their crons'/consumers' peak memory before relying on the defaults (with resources: null documented as the escape hatch back to legacy behavior).
• Whether 1Gi actually covers real production peaks (feed generation, image processing, ES export) cannot be verified from this repo — that needs production metrics. Escalating the "verify against metrics before tagging a release" gate to the maintainer as an open question rather than guessing.

[henzigo]

Low (verified by rendering): the per-instance override path has an asymmetric null footgun. Setting resources: null on a consumer instance cleanly opts that instance out (the with $consumer.resources guard drops the block → BestEffort), but a nested null like:

consumers:
  instances:
    - name: heavy
      transports: t
      resources:
        limits:
          memory: null

survives mergeOverwrite (Helm's null-stripping coalesce doesn't reach inside list items) and renders a literal memory: null, which fails K8s schema validation (kubeconform 1.31.0: Deployment consumer-... is invalid ... resources/limits/memory). The chart-level cron.resources path is fine — Helm's coalesce strips the null there (renders limits: {}, verified).

Nothing to fix in the template; just consider documenting in docs/values.md that removing limits for a single consumer instance requires nulling/replacing the whole resources map, not a nested key.

— Claude Agent (CR)

[henzigo]

Reproduced the footgun by rendering: per-instance resources: null cleanly drops the block, while a nested resources: {limits: {memory: null}} survives mergeOverwrite and renders a literal memory: null. Documented in docs/values.md (d74fdc4): to remove limits for a single consumer instance, null or replace the whole resources map, never a nested key. No template change — the chart-level path is unaffected (Helm's coalesce strips nulls there), and guarding against nested nulls in the template would add complexity for a case the docs now cover.

[henzigo]

Low: the entry records the numbers but not the operational implication of the deviation: pods move from BestEffort to Burstable QoS, and with the memory request (300Mi) far below the limit (1Gi) nodes can overcommit — under memory pressure these pods remain early eviction candidates whenever usage exceeds the request, and anything bursting past 1Gi is now OOMKilled instead of merely evictable. One added sentence ("QoS changes from BestEffort to Burstable; check your crons' peak memory before relying on the 1Gi default") would make the upgrade checklist complete, since this file is exactly where upgraders look for behavior changes.

— Claude Agent (CR)

[henzigo]

Done in d74fdc4 — deviation entry 21 now states the QoS change (BestEffort → Burstable), the overcommit implication of request 300Mi << limit 1Gi, that bursts past 1Gi are OOMKilled instead of merely evictable, the advice to check peak memory before relying on the defaults, and the resources: null opt-out.

[henzigo]

Nit: the default and deep-merge cases are covered, but the opt-out escape hatch is not — a test setting cron.resources: null and asserting the container has no resources key (and/or the equivalent whole-map resources: null on a consumer instance in component_schema_test.yaml) would pin the behavior projects will rely on to restore legacy BestEffort. I verified by rendering that both currently work; a test keeps them working.

— Claude Agent (CR)

[henzigo]

Added in d74fdc4: cron_test.yaml pins cron.resources: null → no resources key on the container, and component_schema_test.yaml pins the per-instance equivalent (resources: null on a consumer instance). Both pass (64/64); golden snapshots untouched since no rendered output changed.