Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ Kubernetes. It is a **generic, reusable package** consumed by multiple projects.

Phase 1 (done) is a faithful 1:1 port: the deploy **order** and all **end states** of the
legacy pipeline are preserved. Phase 2 (planned) is manifest modernization — until then,
manifests intentionally keep their legacy shape (including oddities like the `sleep 30` in
the first-deploy migration command). Do not "improve" manifest content without being asked.
manifests intentionally keep their legacy shape; the few intentional exceptions are
registered in `docs/migrating-from-shopsys-deployment.md`. Do not "improve" manifest
content without being asked.

**Helm 4 only** — no Helm 3 backward compatibility (e.g. `helm plugin install` uses
`--verify=false`, which Helm 3 does not know). Keep CI's `HELM_VERSION` in sync with the
Expand Down
11 changes: 7 additions & 4 deletions charts/shopsys-app/templates/hooks/job-migrate-application.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,18 @@ it at the start of the next deploy (legacy "Delete previous migration pod" step)

The command variant is selected by deploy.firstDeploy.*:
- continuous deploy: check migrations + enable maintenance + run db-dependent build phase
- first deploy: full first deploy of the cluster (the sleep 30 is kept from the legacy
package to let infrastructure services settle)
- first deploy: full first deploy of the cluster (the legacy sleep 30 was dropped - the
shopsys-infra release is waited on before this hook runs, see the deviations doc)
- first deploy with demo data: first deploy + demo data import
*/}}
{{- if .Values.deploy.migration.enabled }}
{{- $targets := .Values.deploy.migration.targets }}
{{- $command := printf "cd /var/www/html && ./phing %s" $targets.continuous }}
{{- if .Values.deploy.firstDeploy.enabled }}
{{- if .Values.deploy.firstDeploy.loadDemoData }}
{{- $command = printf "cd /var/www/html && sleep 30 && ./phing %s" $targets.firstDeployWithDemoData }}
{{- $command = printf "cd /var/www/html && ./phing %s" $targets.firstDeployWithDemoData }}

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 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)

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.

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)

{{- else }}
{{- $command = printf "cd /var/www/html && sleep 30 && ./phing %s" $targets.firstDeploy }}
{{- $command = printf "cd /var/www/html && ./phing %s" $targets.firstDeploy }}
{{- end }}
{{- end }}
apiVersion: batch/v1
Expand All @@ -35,6 +35,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: {{ .Values.deploy.migration.activeDeadlineSeconds }}

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: 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_logskubectl 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)

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.

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)

template:
spec:
volumes:
Expand Down
1 change: 1 addition & 0 deletions charts/shopsys-app/templates/hooks/job-post-deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: {{ .Values.deploy.postDeploy.activeDeadlineSeconds }}
template:
spec:
volumes:
Expand Down
8 changes: 7 additions & 1 deletion charts/shopsys-app/tests/hooks_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ tests:
- matchRegex:
path: spec.template.spec.containers[0].command[2]
pattern: 'db-migrations-count-with-maintenance build-deploy-part-2-db-dependent'
- equal:
path: spec.activeDeadlineSeconds
value: 1800

- it: switches the command on first deploy
template: templates/hooks/job-migrate-application.yaml
Expand All @@ -35,7 +38,7 @@ tests:
asserts:
- matchRegex:
path: spec.template.spec.containers[0].command[2]
pattern: 'sleep 30 && \./phing cluster-first-deploy$'
pattern: 'cd /var/www/html && \./phing cluster-first-deploy$'

- it: loads demo data on first deploy when requested
template: templates/hooks/job-migrate-application.yaml
Expand Down Expand Up @@ -77,6 +80,9 @@ tests:
- matchRegex:
path: spec.template.spec.containers[0].command[2]
pattern: 'set -e'
- equal:
path: spec.activeDeadlineSeconds
value: 1800
- matchRegex:
path: spec.template.spec.containers[0].command[2]
pattern: './phing maintenance-off\n'
Expand Down
14 changes: 14 additions & 0 deletions charts/shopsys-app/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,20 @@
"enabled": { "type": "boolean" },
"loadDemoData": { "type": "boolean" }
}
},
"migration": {
"type": "object",
"properties": {
"enabled": { "type": "boolean" },
"activeDeadlineSeconds": { "type": "integer", "minimum": 1 }
}
},
"postDeploy": {
"type": "object",
"properties": {
"enabled": { "type": "boolean" },
"activeDeadlineSeconds": { "type": "integer", "minimum": 1 }
}
}
}
},
Expand Down
9 changes: 9 additions & 0 deletions charts/shopsys-app/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,18 @@ deploy:
continuous: "-verbose db-migrations-count-with-maintenance build-deploy-part-2-db-dependent"
firstDeploy: "cluster-first-deploy"
firstDeployWithDemoData: "cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"
# Hard bound for the migration Job (seconds); must exceed the longest migration -
# first deploys with demo data (elasticsearch-export) may need a much higher value.
# Keep it below the helmfile timeout (DEPLOY_TIMEOUT, default 2700) so a stuck
# migration fails as a DeadlineExceeded Job instead of a Helm client timeout.
activeDeadlineSeconds: 1800
resources: {}
postDeploy:
enabled: true
# Hard bound for the post-deploy Job (seconds). Together with the migration bound this
# can exceed DEPLOY_TIMEOUT (2700 s) - acceptable, both hitting their ceiling in one
# deploy is not a normal scenario.
activeDeadlineSeconds: 1800
resources: {}
hooks:
# kubectlImage is the image used by the cron-suspend hook Job; pinned tag on purpose
Expand Down
6 changes: 5 additions & 1 deletion deploy/deploy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,11 @@ print_job_logs() {
local job="$1" title="$2"
if kubectl -n "${NAMESPACE}" get "job/${job}" > /dev/null 2>&1; then
section_start "${job//-/_}_logs" "${title}"
kubectl logs "job/${job}" --namespace="${NAMESPACE}" || true
if ! kubectl logs "job/${job}" --namespace="${NAMESPACE}" 2> /dev/null; then
# A deadline-killed Job (activeDeadlineSeconds) has its pod deleted, so no
# logs exist - fall back to the Job description (conditions + events)
kubectl describe "job/${job}" --namespace="${NAMESPACE}" || true
fi
section_end "${job//-/_}_logs"
fi
}
Expand Down
2 changes: 1 addition & 1 deletion docs/deploy-flow.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ For the legacy behavior itself see [original-deployment.md](original-deployment.
│ ├─ hook pre-install,pre-upgrade (w10): migrate-application Job
│ │ continuous: phing db-migrations-count-with-maintenance
│ │ build-deploy-part-2-db-dependent
│ │ first: sleep 30 && phing cluster-first-deploy [+ demo targets]
│ │ first: phing cluster-first-deploy [+ demo targets]
│ │ └─ FAILS → whole apply aborts, old release stays
│ ├─ manifests apply + rollout wait (webserver, storefront, cron on the
│ │ new image – the `date` label forces a new pod, consumers, ingresses,
Expand Down
14 changes: 12 additions & 2 deletions docs/migrating-from-shopsys-deployment.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,18 @@ Intentional differences of the phase-1 rewrite; everything else is a 1:1 port.
`domains-urls-hook` copy because pre-install hooks run before regular resources exist.
8. **Namespace** is created by the wrapper/helmfile, not applied as a manifest. It is named
`<project.name>-<environment>` (replaces the "PROJECT_NAME must contain a dash" rule).
9. **`sleep 30`** in the first-deploy migration command is kept verbatim for parity even
though the infra release already guarantees readiness.
9. **The legacy `sleep 30`** in the first-deploy migration command is removed — the
shopsys-infra release is installed with `wait: true` before the migration hook runs
(Redis readiness is probe-backed; RabbitMQ has no default readinessProbe, so the wait
guarantees a Running broker container, not yet an accepting broker). The migration and
post-deploy Jobs are additionally bounded by `deploy.migration.activeDeadlineSeconds`
(default 1800 s — first deploys with demo data may need much more, see
[values.md](values.md)) and `deploy.postDeploy.activeDeadlineSeconds` (default 1800 s;
the two bounds together can exceed `DEPLOY_TIMEOUT` — acceptable, both hitting their
ceiling in one deploy is not a normal scenario).
When a deadline fires, the Job controller deletes the running pod — the Job fails as
`DeadlineExceeded` (the wrapper's recovery path still triggers), but the pod logs are
gone; the wrapper then falls back to `kubectl describe job` output.
10. **DISPLAY_FINAL_CONFIGURATION** prints one `helmfile template` output instead of two
kustomize sections.
11. **`orchestration/kubernetes/` file overrides and the composer `merge` step are gone** —
Expand Down
12 changes: 10 additions & 2 deletions docs/values.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,9 @@ rabbitmq: # infra component + auth/persistence/management
deploy:
timestamp: "" # injected by the wrapper (forces a new cron pod)
firstDeploy: { enabled: false, loadDemoData: false }
migration: { enabled, targets: {continuous, firstDeploy, firstDeployWithDemoData}, resources }
postDeploy: { enabled, resources }
migration: { enabled, targets: {continuous, firstDeploy, firstDeployWithDemoData},
resources, activeDeadlineSeconds } # hard Job bound, default 1800 s
postDeploy: { enabled, resources, activeDeadlineSeconds } # hard Job bound, default 1800 s
hooks: { kubectlImage, serviceAccountName }

extraManifests: [] # raw manifests (rendered through tpl) — escape hatch
Expand All @@ -100,6 +101,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.

`deploy.migration.activeDeadlineSeconds` must exceed the longest migration variant of the
project — first deploys with demo data (`elasticsearch-export` on real data volumes) may
need a much higher value than the 1800 s default. Keep it below the helmfile timeout
(`DEPLOY_TIMEOUT`, default 2700 s, raise both together) so a stuck migration fails as a
`DeadlineExceeded` Job — which the wrapper's recovery path handles — instead of the Helm
client timing out.

## Legacy env var → values mapping

| Legacy env var | New location |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down Expand Up @@ -1810,6 +1813,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand All @@ -1751,7 +1754,7 @@ spec:
containers:
- name: migrate-application
image: "v1.0.0"
command: ["sh", "-c", "cd /var/www/html && sleep 30 && ./phing cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"]
command: ["sh", "-c", "cd /var/www/html && ./phing cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"]
envFrom:
- secretRef:
name: app-secret-env-hook
Expand Down Expand Up @@ -1810,6 +1813,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand All @@ -1751,7 +1754,7 @@ spec:
containers:
- name: migrate-application
image: "v1.0.0"
command: ["sh", "-c", "cd /var/www/html && sleep 30 && ./phing cluster-first-deploy"]
command: ["sh", "-c", "cd /var/www/html && ./phing cluster-first-deploy"]
envFrom:
- secretRef:
name: app-secret-env-hook
Expand Down Expand Up @@ -1810,6 +1813,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down Expand Up @@ -1587,6 +1590,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand All @@ -1528,7 +1531,7 @@ spec:
containers:
- name: migrate-application
image: "v1.0.0"
command: ["sh", "-c", "cd /var/www/html && sleep 30 && ./phing cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"]
command: ["sh", "-c", "cd /var/www/html && ./phing cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"]
envFrom:
- secretRef:
name: app-secret-env-hook
Expand Down Expand Up @@ -1587,6 +1590,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand All @@ -1528,7 +1531,7 @@ spec:
containers:
- name: migrate-application
image: "v1.0.0"
command: ["sh", "-c", "cd /var/www/html && sleep 30 && ./phing cluster-first-deploy"]
command: ["sh", "-c", "cd /var/www/html && ./phing cluster-first-deploy"]
envFrom:
- secretRef:
name: app-secret-env-hook
Expand Down Expand Up @@ -1587,6 +1590,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1573,6 +1573,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down Expand Up @@ -1641,6 +1644,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1573,6 +1573,9 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
# Bound the migration - a stuck Job otherwise blocks the deploy until the global
# helmfile timeout and stays running in the cluster
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand All @@ -1582,7 +1585,7 @@ spec:
containers:
- name: migrate-application
image: "v1.0.0"
command: ["sh", "-c", "cd /var/www/html && sleep 30 && ./phing cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"]
command: ["sh", "-c", "cd /var/www/html && ./phing cluster-first-deploy db-fixtures-demo plugin-demo-data-load friendly-urls-generate domains-urls-replace elasticsearch-export"]
envFrom:
- secretRef:
name: app-secret-env-hook
Expand Down Expand Up @@ -1641,6 +1644,7 @@ metadata:
helm.sh/hook-delete-policy: before-hook-creation
spec:
backoffLimit: 0
activeDeadlineSeconds: 1800
template:
spec:
volumes:
Expand Down
Loading
Loading