Skip to content

Ship default resource requests/limits for cron and consumers #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
henzigo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Ship default resource requests/limits for cron and consumers #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
610a21f
Ship default resource requests/limits for cron and consumers
henzigo Aug 19, 2026
d74fdc4
Address review: document QoS trade-off and null opt-out, test opt-out…
henzigo Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions charts/shopsys-app/tests/component_schema_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ values:
templates:
- templates/deployment-storefront.yaml
- templates/deployment-webserver-php-fpm.yaml
- templates/deployment-consumer.yaml
tests:
- it: propagates scheduling and volume keys into the pod spec
template: templates/deployment-storefront.yaml
Expand Down Expand Up @@ -107,6 +108,45 @@ tests:
- name: my-pull-secret
- name: other-secret

- it: ships default consumer resources and lets an instance override them
template: templates/deployment-consumer.yaml
set:
consumers:
instances:
- name: email
transports: email_transport
- name: heavy
transports: heavy_transport
resources:
requests:
cpu: "500m"
asserts:
- equal:
path: spec.template.spec.containers[0].resources
value:
limits:
memory: 1Gi
requests:
cpu: "50m"
memory: 300Mi
documentIndex: 0
- equal:
path: spec.template.spec.containers[0].resources.requests.cpu
value: "500m"
documentIndex: 1

- it: lets an instance opt out of resources entirely (legacy BestEffort)
template: templates/deployment-consumer.yaml
set:
consumers:
instances:
- name: besteffort
transports: besteffort_transport
resources: null
asserts:
- notExists:
path: spec.template.spec.containers[0].resources

- it: propagates pod annotations and labels
template: templates/deployment-webserver-php-fpm.yaml
set:
Expand Down
36 changes: 36 additions & 0 deletions charts/shopsys-app/tests/cron_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,39 @@ tests:
- notMatchRegex:
path: data.cron
pattern: 'project_secret_env'

- it: ships default resource requests and limits

@henzigo henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

template: templates/deployment-cron.yaml
asserts:
- equal:
path: spec.template.spec.containers[0].resources
value:
limits:
memory: 1Gi
requests:
cpu: "100m"
memory: 300Mi

- it: deep-merges resource overrides with the defaults
template: templates/deployment-cron.yaml
set:
cron:
resources:
requests:
cpu: "10m"
asserts:
- equal:
path: spec.template.spec.containers[0].resources.requests.cpu
value: "10m"
- equal:
path: spec.template.spec.containers[0].resources.limits.memory
value: 1Gi

- it: drops the resources block entirely when set to null (legacy BestEffort opt-out)
template: templates/deployment-cron.yaml
set:
cron:
resources: null
asserts:
- notExists:
path: spec.template.spec.containers[0].resources
18 changes: 16 additions & 2 deletions charts/shopsys-app/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,14 @@ cron:
maxSurge: 1
maxUnavailable: 0
type: RollingUpdate
resources: {}
# Conservative defaults (same PHP application image as the webserver); tune per project
# from real metrics. The legacy package shipped no requests/limits (BestEffort pods).
resources:
limits:
memory: 1Gi

@henzigo henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

requests:
cpu: "100m"
memory: 300Mi
securityContext:
runAsUser: 0
podAnnotations: {}
Expand Down Expand Up @@ -308,7 +315,14 @@ consumers:
timeLimit: 300
terminationGracePeriodSeconds: 300
progressDeadlineSeconds: 600
resources: {}
# Conservative defaults (same PHP application image as the webserver); override per
# instance or per environment. The legacy package shipped no requests/limits.
resources:

@henzigo henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

limits:
memory: 1Gi
requests:
cpu: "50m"
memory: 300Mi
podAnnotations: {}
podLabels: {}
nodeSelector: {}
Expand Down
9 changes: 9 additions & 0 deletions docs/migrating-from-shopsys-deployment.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,12 @@ Intentional differences of the phase-1 rewrite; everything else is a 1:1 port.
env vars (works with any registry — GCR/GAR via username `_json_key` and the service
account JSON as the password); the GitLab-flavored `CI_REGISTRY`/`DEPLOY_REGISTER_*`
variables keep working as a fallback.
21. **Default resources for cron and consumers**: the legacy package shipped cron and
consumer pods with no requests/limits (`BestEffort` QoS). The chart now defaults to

@henzigo henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 henzigo Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

conservative values (cron: requests `100m`/`300Mi`, consumers: requests `50m`/`300Mi`;
both limited to `1Gi` memory) — tune them per project/environment via
`cron.resources` and `consumers.defaults.resources` (or per instance). QoS changes
from `BestEffort` to `Burstable`: with the request far below the limit nodes can
overcommit, and anything bursting past `1Gi` is now OOMKilled instead of merely
evictable — check your crons'/consumers' peak memory usage before relying on the
defaults, and set `resources: null` on a component to restore the legacy behavior.
13 changes: 11 additions & 2 deletions docs/values.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ app: # shared backend configuration

webserver: # component (see standard keys) + phpFpm/nginx sub-containers
storefront: # component + its own `env` / `secretEnv` (storefront-secret-env Secret)
cron: # component + `instances: [{name, schedule}]`
consumers: # `defaults` + `instances: [{name, transports, replicas, ...}]`
cron: # component + `instances: [{name, schedule}]`;
# default resources: requests 100m/300Mi, limits 1Gi memory
consumers: # `defaults` + `instances: [{name, transports, replicas, ...}]`;
# default resources: requests 50m/300Mi, limits 1Gi memory
redis: # infra component + `config` (redis.conf)
rabbitmq: # infra component + auth/persistence/management

Expand All @@ -100,6 +102,13 @@ entries take precedence over `envFrom` in Kubernetes — never define the same k
Lists (e.g. `security.whitelistIps`, `domains`) **replace** the base value when overridden by
an environment file — they are not merged. Maps merge deeply.

To restore the legacy BestEffort behavior (no requests/limits), set the component's
`resources: null` — for cron via `cron.resources: null`, for a single consumer instance via
`resources: null` on that instance. **Per consumer instance, null the whole `resources` map
(or replace it), never a nested key**: a nested null such as
`resources: {limits: {memory: null}}` survives the per-instance merge and renders a literal
`memory: null`, which is rejected by the Kubernetes API.

## Legacy env var → values mapping

| Legacy env var | New location |
Expand Down
12 changes: 12 additions & 0 deletions tests/golden/scenarios/basic-production/expected/continuous.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 50m
memory: 300Mi

volumeMounts:
- name: domains-urls
Expand Down Expand Up @@ -1154,6 +1160,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
12 changes: 12 additions & 0 deletions tests/golden/scenarios/basic-production/expected/first-deploy-with-demo-data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 50m
memory: 300Mi

volumeMounts:
- name: domains-urls
Expand Down Expand Up @@ -1154,6 +1160,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
12 changes: 12 additions & 0 deletions tests/golden/scenarios/basic-production/expected/first-deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 50m
memory: 300Mi

volumeMounts:
- name: domains-urls
Expand Down Expand Up @@ -1154,6 +1160,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
6 changes: 6 additions & 0 deletions tests/golden/scenarios/development-single-domain/expected/continuous.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
6 changes: 6 additions & 0 deletions tests/golden/scenarios/development-single-domain/expected/first-deploy-with-demo-data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
6 changes: 6 additions & 0 deletions tests/golden/scenarios/development-single-domain/expected/first-deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
6 changes: 6 additions & 0 deletions tests/golden/scenarios/development-with-cloudflare/expected/continuous.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
6 changes: 6 additions & 0 deletions tests/golden/scenarios/development-with-cloudflare/expected/first-deploy-with-demo-data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
6 changes: 6 additions & 0 deletions tests/golden/scenarios/development-with-cloudflare/expected/first-deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,12 @@ spec:
value: "https://s3.example.com"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
12 changes: 12 additions & 0 deletions tests/golden/scenarios/escaping-env/expected/continuous.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,12 @@ spec:
value: "479411e7"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 50m
memory: 300Mi

volumeMounts:
- name: domains-urls
Expand Down Expand Up @@ -1174,6 +1180,12 @@ spec:
value: "479411e7"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
12 changes: 12 additions & 0 deletions tests/golden/scenarios/escaping-env/expected/first-deploy-with-demo-data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,12 @@ spec:
value: "479411e7"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 50m
memory: 300Mi

volumeMounts:
- name: domains-urls
Expand Down Expand Up @@ -1174,6 +1180,12 @@ spec:
value: "479411e7"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
12 changes: 12 additions & 0 deletions tests/golden/scenarios/escaping-env/expected/first-deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,6 +1038,12 @@ spec:
value: "479411e7"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 50m
memory: 300Mi

volumeMounts:
- name: domains-urls
Expand Down Expand Up @@ -1174,6 +1180,12 @@ spec:
value: "479411e7"
- name: TRUSTED_PROXY
value: "10.0.0.0/8"
resources:
limits:
memory: 1Gi
requests:
cpu: 100m
memory: 300Mi
volumeMounts:
- name: domains-urls
mountPath: /var/www/html/config/domains_urls.yaml
Expand Down
Loading
Loading