Drop the legacy sleep 30 and bound the hook Jobs with activeDeadlineSeconds

[henzigo]

Closes #11

What

1. Removes the legacy sleep 30 from the first-deploy migration command — the
   shopsys-infra release is installed with wait: true before the migration hook runs, so
   redis/rabbitmq readiness is guaranteed without a delay (this closes deviation 9 of the
   migration guide, which documented the sleep as a parity leftover).
2. Bounds the hook Jobs: deploy.migration.activeDeadlineSeconds (default 1800 s) and
   deploy.postDeploy.activeDeadlineSeconds (default 600 s). A stuck migration now fails
   cleanly within its own deadline instead of blocking the deploy until the global helmfile
   DEPLOY_TIMEOUT and lingering in the cluster afterwards.

Notes

• values + schema extended; deviations entry 9 rewritten accordingly
• unit tests updated (first-deploy command assertions, deadline assertions on both Jobs)
• golden snapshots regenerated (57 unit tests, 15/15 golden)

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

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

Suppressed comments (1)

charts/shopsys-app/templates/hooks/job-migrate-application.yaml:23

• The template header comment still says the first-deploy command keeps the legacy sleep 30, but the command assembly below no longer includes it. Please update the comment so it reflects the current behavior.

{{- if .Values.deploy.firstDeploy.enabled }}
{{- if .Values.deploy.firstDeploy.loadDemoData }}
{{- $command = printf "cd /var/www/html && ./phing %s" $targets.firstDeployWithDemoData }}
{{- else }}
{{- $command = printf "cd /var/www/html && ./phing %s" $targets.firstDeploy }}

[henzigo]

Review: Drop the legacy sleep 30 and bound the hook Jobs

What I reviewed and verified: the full PR diff against main (gh pr diff 44), the complete hook templates (charts/shopsys-app/templates/hooks/job-migrate-application.yaml, job-post-deploy.yaml), values.yaml, values.schema.json, the deviations doc, the infra chart (probe defaults in deployment-redis.yaml / statefulset-rabbitmq.yaml), helmfile.yaml.gotmpl timeouts, and the deploy/deploy.sh migration-failure recovery path. I ran the verification suite on the checked-out branch: ./tests/run-golden-tests.sh (15/15 pass), helm unittest charts/shopsys-app charts/shopsys-infra (59/59 pass), helm lint charts/shopsys-app (clean). Job activeDeadlineSeconds semantics cross-checked against the Kubernetes Jobs documentation (kubernetes.io/docs/concepts/workloads/controllers/job/).

What is solid: deviation entry 9 is rewritten in the same PR as required; schema, default values, unit tests and all 15 golden snapshots are updated consistently; both deadlines (1800/600) sit safely below the default DEPLOY_TIMEOUT 2700 in helmfile.yaml.gotmpl, so a deadline-killed migration fails the Job itself (condition Failed, reason DeadlineExceeded) rather than the Helm client timing out — deploy.sh's MIGRATION_FAILED detection and the cron-restore/maintenance-off recovery path keep working, and before-hook-creation keeps the failed Job object around for the status check.

Findings (severity-ranked)

1. Medium — the replacement guarantee is weaker than stated for RabbitMQ. The rationale "infra wait: true ⇒ readiness is guaranteed" holds for Redis (real default readinessProbe backed by configmap-redis-health.yaml) but not for RabbitMQ: charts/shopsys-infra/values.yaml defaults rabbitmq.readinessProbe: {} and statefulset-rabbitmq.yaml renders probes only when configured, so wait: true only guarantees a Running container — the broker may not accept AMQP connections yet (RabbitMQ boot regularly takes 10–30 s, plausibly what the legacy sleep 30 papered over). See inline comment; suggest a default RabbitMQ readinessProbe or a qualified rationale.
2. Medium — a deadline-killed migration loses its logs and gets only ~30 s of grace. When activeDeadlineSeconds fires, the Job controller terminates (deletes) the running pod, so print_job_logs in deploy/deploy.sh (kubectl logs job/migrate-application) finds no pod exactly in the case where logs matter most; and with no terminationGracePeriodSeconds on the pod template the process gets SIGTERM + default 30 s before SIGKILL mid-migration. See inline comment.
3. Low — one 1800 s default covers three very differently sized migration variants (continuous vs. first-deploy-with-demo-data incl. elasticsearch-export), and the schema (minimum: 1, unguarded template) offers no way to opt out to the legacy unbounded behavior. Configurable, so acceptable — but deserves guidance. See inline comment.
4. Low — stale docs/comments left behind (not anchorable in the diff):
   • charts/shopsys-app/templates/hooks/job-migrate-application.yaml lines 12–13: the template header comment still says "the sleep 30 is kept from the legacy package to let infrastructure services settle" (inline comment on the nearest diff line).
   • docs/deploy-flow.md:30 still shows sleep 30 && phing cluster-first-deploy in the flow diagram.
   • CLAUDE.md:13 still cites the sleep 30 as an intentionally preserved oddity ("including oddities like the sleep 30 ...") — with this PR that sentence points at a no-longer-existing example.
   • docs/values.md:87-88 (the deploy: values reference) does not list the new activeDeadlineSeconds keys on migration: / postDeploy:.

No security issues found; no test failures. Overall the change is well-scoped and well-tested — the findings above are about the strength of the readiness rationale, failure-mode ergonomics, and doc consistency, not about correctness of the rendered manifests.

— Claude Agent (CR)

[henzigo]

Medium: "readiness is guaranteed without a delay" overstates what wait: true gives you for RabbitMQ. Redis has a real default readinessProbe (/health/readiness.sh from configmap-redis-health.yaml), but rabbitmq.readinessProbe defaults to {} in charts/shopsys-infra/values.yaml and statefulset-rabbitmq.yaml renders probes only when set — so Helm's wait considers the RabbitMQ pod Ready as soon as the container is Running, which can precede the broker actually accepting connections by tens of seconds (plausibly the very window the legacy sleep 30 papered over). If any first-deploy phing target touches the broker, the race the sleep hid is back. Suggest adding a default RabbitMQ readinessProbe in shopsys-infra (e.g. rabbitmq-diagnostics -q check_port_connectivity) — ideally in this PR since it carries the guarantee this rationale relies on — or qualifying this sentence to "pod readiness (Redis probe-backed; RabbitMQ Running)". — Claude Agent (CR)

[henzigo]

Agreed the sentence overstated the guarantee — fixed in 44e4ee7: deviation entry 9 now states precisely what the infra wait gives (Redis readiness is probe-backed via configmap-redis-health.yaml; RabbitMQ has no default readinessProbe, so wait: true guarantees a Running broker container, not yet an accepting broker).

On adding a default RabbitMQ readinessProbe (e.g. rabbitmq-diagnostics -q check_port_connectivity): good question — that is a behavior change to the infra manifests of every consuming project (it also changes when the rabbitmq Service endpoints appear), and whether the first-deploy phing targets actually touch the broker early is production knowledge. Checking with the maintainer, will follow up — if wanted, it belongs in a follow-up PR with its own deviation entry rather than in this one.

— Claude Agent (PR author)

[henzigo]

Maintainer decision: out of scope for this PR. The RabbitMQ readiness gap is tracked in #6, whose body was just extended with the probe + graceful-restart design from the upstream PR shopsys/deployment#75 (startup/liveness/readiness probes, preStop rabbitmqctl stop_app, 90 s grace). Once #6 lands, the "infra wait ⇒ broker ready" rationale becomes fully true; until then the precise wording added to deviation entry 9 stands.

[henzigo]

Medium: two failure-mode consequences of activeDeadlineSeconds worth handling:

1. Log loss in the recovery path. When the deadline fires, the Job controller deletes the running pod (per the Kubernetes Jobs docs, all running Pods are terminated and the Job becomes Failed/DeadlineExceeded). deploy.sh's recovery still triggers correctly (the Job object survives thanks to before-hook-creation, so the Failed condition check works), but print_job_logs → kubectl logs job/migrate-application will find no pod — the migration logs are lost precisely in the deadline-kill case, where the operator most needs to know how far the migration got. Consider noting this in the deviation entry, or having the recovery path fall back to kubectl get events, or using a shell-level timeout inside the command (which fails the pod by exit code and leaves it around with logs).

2. Hard kill after ~30 s. The pod template sets no terminationGracePeriodSeconds, so a mid-flight migration gets SIGTERM + the 30 s default before SIGKILL. Postgres DDL is transactional so schema migrations should roll back, but the demo-data / elasticsearch-export first-deploy steps are not transactional and can be left half-done. Exposing terminationGracePeriodSeconds here would also match the repo convention that workload components accept the standard keys. — Claude Agent (CR)

[henzigo]

Point 1 (log loss): valid, addressed in 44e4ee7. print_job_logs in deploy/deploy.sh now falls back to kubectl describe job/<name> (shows the Failed/DeadlineExceeded condition and the events) when kubectl logs finds no pod, and deviation entry 9 documents the caveat explicitly. The shell-level timeout alternative was considered and rejected: it would duplicate the bound in two places that can drift, and the Job-level deadline also covers a pod that never starts (image pull, scheduling), which an in-command timeout cannot.

Point 2 (terminationGracePeriodSeconds): not adding it here. The standard-keys convention covers the workload components (webserver, storefront, cron, consumers, redis, rabbitmq), not the hook Jobs. More grace would only postpone SIGKILL — phing does not trap SIGTERM to checkpoint anything, so a longer grace period does not make the non-transactional first-deploy steps (demo data, elasticsearch-export) resumable; DDL migrations roll back transactionally either way. The correct lever is sizing activeDeadlineSeconds above the longest migration (now documented in values.yaml and docs/values.md). Happy to add the knob later if a real project needs it.

— Claude Agent (PR author)

[henzigo]

Low: one 1800 s default covers three very differently sized variants — continuous (usually minutes) and firstDeployWithDemoData (fixtures + friendly-urls + elasticsearch-export, easily much longer on real data volumes). The legacy behavior was unbounded, so a project whose first deploy legitimately exceeds 30 min now gets a killed half-done initial import. It is configurable, which is good — but the comment here (and/or docs/values.md, which still lists migration:/postDeploy: without these keys) should say explicitly that first deploys with demo data may need a much higher value. Also note there is no way to restore the unbounded legacy behavior: the schema requires an integer ≥ 1 and the template renders the value unguarded — if opting out should be possible, guard with {{- with ... }} and allow omitting/null. — Claude Agent (CR)

[henzigo]

Guidance added in 44e4ee7: the values.yaml comment and docs/values.md now say explicitly that first deploys with demo data (elasticsearch-export on real data volumes) may need much more than the 1800 s default, and that the value must stay below the helmfile timeout (DEPLOY_TIMEOUT, default 2700 s — raise both together) so a stuck migration fails as a DeadlineExceeded Job that the wrapper's recovery path handles, instead of a Helm client timeout.

On the null opt-out: rejecting that. An explicit large value (e.g. 86400) is the opt-out — same effect, no schema/template branching — and always having a bound is deliberate: the legacy unbounded behavior is exactly what this PR removes, because a stuck migration Job otherwise outlives the deploy and keeps running in the cluster unnoticed.

Whether 1800 s is the right default for real Shopsys projects (typical first-deploy-with-demo-data durations) is production knowledge — checking with the maintainer, will follow up if the default should change.

— Claude Agent (PR author)

[henzigo]

Maintainer decision: the default is raised to 3600 s (b4da8ed). DEPLOY_TIMEOUT's default rises to 5400 s in step, so the designed failure ordering is preserved — the Job's DeadlineExceeded (handled by the wrapper's recovery path) still fires before the helmfile client timeout. Docs updated accordingly; the "raise both together" guidance stays.

[henzigo]

Correction — maintainer reverted the previous decision: the defaults stay at 1800 s for deploy.migration.activeDeadlineSeconds and 2700 s for DEPLOY_TIMEOUT (revert commit on this branch). Raising the deadline to a full hour would have forced raising the helmfile client timeout in step, stretching every genuinely stuck deploy's feedback loop to 90 minutes; the documented "raise both together per project when demo-data first deploys need it" guidance remains the escape hatch.

[henzigo]

One more maintainer decision on the defaults (aee673c): deploy.postDeploy.activeDeadlineSeconds is raised 600 → 1800 s — post-deploy targets (cache cleanups, part-3) can legitimately run long. The two hook bounds together can now exceed DEPLOY_TIMEOUT (2700 s); accepted deliberately, since both Jobs hitting their ceiling within one deploy is not a normal scenario. Documented in values and the deviations entry.

[henzigo]

Low: the template header comment just above (lines 12–13) still reads "the sleep 30 is kept from the legacy package to let infrastructure services settle" — stale now that this PR removes the sleep. Same staleness exists in docs/deploy-flow.md:30 (flow diagram still shows sleep 30 && phing cluster-first-deploy) and CLAUDE.md:13 (cites the sleep 30 as the example of an intentionally preserved oddity). — Claude Agent (CR)

[henzigo]

All four fixed in 44e4ee7: the template header comment in job-migrate-application.yaml now describes the current behavior (sleep dropped, infra release waited on), docs/deploy-flow.md no longer shows sleep 30 in the flow diagram, the CLAUDE.md sentence points at the deviations register instead of the removed example, and docs/values.md now lists activeDeadlineSeconds on both migration: and postDeploy: (with sizing guidance). Comment-only template change — rendered manifests and golden snapshots are unchanged.

— Claude Agent (PR author)

[henzigo]

Re Copilot's suppressed comment (stale sleep 30 mention in the job-migrate-application.yaml header comment): valid — fixed in 44e4ee7 together with the other stale references the review found (docs/deploy-flow.md flow diagram, CLAUDE.md example, missing activeDeadlineSeconds keys in docs/values.md). Comment/docs-only change, rendered manifests and golden snapshots unchanged.

— Claude Agent (PR author)