Repository navigation
Conversation
|
Hi @younsl thanks for the PR. This is definitely the direction we want to go, but we need to be very careful on how this is implemented so we don't end up having to do multiple back-forth iterations with breaking changes on the design. I'm currently reviewing and gathering my thoughts on the topic, and I'll be posting a review in the coming days. So please stay tuned! |
|
Thanks @villebro! I'll wait for your review before changing anything. |
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
934deab to
91432ae
Compare
b37a0bd to
f87663a
Compare
f87663a to
5a60e5b
Compare
|
@younsl question on the design: should backup happen before or after drain? I feel it may be safer to drain before backup, and then just rehydrate after successful completion or failed backup. |
|
Thanks @villebro, agreed. Draining first means the snapshot matches exactly what migrate sees, with no writes landing after the dump. Plan:
One question: rehydrate must use the image the schema still matches, not the new one. I'd recreate components pinned to status.lastLifecycleImage, which reuses the existing drain path. The alternative is scaling to zero instead of deleting during drain, so rehydrate restores the previous state as is. Which do you prefer? And should requiresDrain: false stay as an opt-out, or should I drop it? |
The current drain implementation deletes Deployments, HPAs, PDBs, and non-web Services, then waits for the pods to terminate. I’d prefer to retain that clean delete-and-reconcile model rather than leave zero-scaled workload resources around. But I'm not super heavily opinionated here, so I could be convinced otherwise. Also, thinking more closely about this, I’m concerned that recreating components pinned only to |
|
Thanks @villebro, that makes sense. Pushed in bb404dc:
I kept requiresDrain: false as an opt-out because the field comes from the shared BaseTaskSpec, so removing it only for backup would need a special case. Let me know if you would rather drop it. |
|
The E2E failures were in the MySQL test fixture, not in the backup code: kubectl exec connected through the Unix socket, which was not reachable on the CI runners, while the readiness probe, the backup Job, and createDatabase all connected over TCP and passed. Pushed 04277cb so the test uses TCP (-h 127.0.0.1) like the probe. Could you approve the workflows again? Thanks. |
Seed was the only lifecycle task that runs a database-tool image instead of the Superset image, and that fact was encoded as taskType == taskTypeSeed checks in the flat spec builder, the bootstrap/ConfigMap paths, and pod retention. Move it onto the task descriptor (BuildToolFlatSpec and PodRetention hooks) and extract convertToolTaskComponent from the seed helper, so another tool-image task can be added without new switch arms. No behavior change. Signed-off-by: younsl <cysl@kakao.com>
Add an optional spec.lifecycle.backup task that dumps the metastore
(pg_dump -Fc or mysqldump) to an existing PersistentVolumeClaim, or via a
custom command, immediately before a migrate or rotate run. The operator
never runs down migrations, so this gives a failed or unwanted upgrade a
restore point.
The task sits outside the checksum cascade: it runs at most once per
lifecycle run, after drain and in front of the first pending migrate or
rotate that has not started, keyed on a new status.lifecycle.settledChecksum
that only advances when a run fully completes. Editing the backup spec
therefore never re-runs another task, and a retried migrate reuses the
snapshot taken before its first attempt instead of replacing it with a
partially migrated database.
Dumps are written owner-only to a .partial file, verified, and renamed
without overwriting; files are named {parent}_{utc}_{fromTag} so name order
is chronological. On the first run a confirmed-absent database is skipped
so createDatabase can create it afterwards. A failed backup blocks the
upgrade before the database is touched. The Job runs non-root with a
defaulted fsGroup, needs no new RBAC, and receives credentials only as
secretKeyRef env vars. Backup and seed are mutually exclusive.
Lifecycle task logs now include the attempt duration.
Signed-off-by: younsl <cysl@kakao.com>
Harden the pre-upgrade backup task so an automatic backup is safe to rely on and easy to operate: - Run the backup before maintenance and drain by default (requiresDrain now defaults to false), so a slow backup adds no downtime and a failed one blocks the upgrade while the current version keeps serving. Setting requiresDrain: true keeps the previous drain-first behavior. - Verify the dump in full before committing it (pg_restore -f /dev/null instead of the TOC-only --list, which misses truncated and corrupt data blocks), re-hash it after the rename, and write a JSON manifest (image tags, Alembic revision, versions, size, SHA-256) as the commit marker. - Preflight the destination, connectivity, and the pg_dump client version, and bound lock waits with --lock-wait-timeout. - Report the result through the container termination message. The controller parses it strictly into status.lifecycle.backups (last five, newest first) and the task message, emits BackupCompleted/BackupSkipped events, and surfaces the failing step and its error output in status instead of a bare Job failure. - Add optional retention.keepLast, which prunes only manifests of this Superset (matched by UID) after a new backup is committed. - Pin the lifecycle task checksums of existing resources with a golden test so the operator upgrade does not re-run any task. - Pass an explicit -timeout to the e2e suite (E2E_TIMEOUT, default 45m); it now exceeds go test's 10 minute default. Signed-off-by: younsl <cysl@kakao.com>
Add unit tests for backup branches that were only exercised by e2e: - pendingLifecycleTasks stops at a terminally failed backup, so no migrate or init is planned after it, while a backup with retries left still gates the run. - taskTerminationMessage ignores a Pod carrying the task instance label that is not controlled by the Job. - collectBackupEnvVars passes an inline metastore URI only in Development. - settledChecksumFor returns no baseline when only the backup checksum is recorded. Signed-off-by: younsl <cysl@kakao.com>
Following review, the backup task now runs after drain by default (requiresDrain defaults to true). Components are removed before the dump starts, so no writes can land between the snapshot and migrate, and the snapshot matches the database that migrate sees. When a maintenance page is configured, users see it while the dump runs. If the backup fails for good, the upgrade stops before migrate or rotate and the instance stays drained, with the maintenance page still serving if one is configured. The operator does not bring components back on its own: rebuilding them from the last lifecycle image could miss image overrides and other workload changes made in the same edit. The user retries the backup (backup.trigger), fixes its pod spec, disables it, or reverts the image. This uses the same terminal failure path as a failed migrate, so no new controller logic is needed. Setting requiresDrain: false keeps the old behavior: the dump runs before drain while the current version keeps serving, and a failed backup leaves the instance up. Tests cover the new default, a failed backup that leaves components drained, recovery through trigger and through disabling the backup, and the opt-out path. The PostgreSQL e2e suite now enables the maintenance page and checks that it keeps serving, with the web-server Service pointing at it, while the backup is failed, and that traffic returns to the web server after recovery. Signed-off-by: younsl <cysl@kakao.com>
The MySQL backup specs talked to the test database with kubectl exec and the mysql client's default Unix socket. On the GitHub runners that socket could not be reached, so both MySQL specs failed before the backup code ran, even though the server was up: the readiness probe, the backup Job, and the createDatabase init container all connected over TCP without problems. Pass -h 127.0.0.1 to these two exec calls so they use TCP, the same way the readiness probe already does. This removes the dependency on the socket path and only changes the test fixture. Signed-off-by: younsl <cysl@kakao.com>
04277cb to
6c274a6
Compare
Summary
This PR adds an optional pre-upgrade metastore backup task, spec.lifecycle.backup. The operator never runs superset db downgrade, so restoring a pre-upgrade snapshot is the only way back from a bad upgrade. This task takes that snapshot in front of migrate and rotate, verifies it, and blocks the upgrade if it fails. It re-lands the backup part of #135 by @villebro, which was closed to be split up; its breaking parts already landed in #167, #173, and #174.
Details
Kind e2e results: 31 of 31 specs passed
kind v0.33.0 with kindest/node:v1.37.0, Go 1.27.2, postgres:17-alpine and mysql:8.4. 31 specs ran in 661.9 seconds with 0 failures.
The 14 backup specs cover first install backup and skip (PostgreSQL, MySQL), drain before backup with the maintenance page up, no backup on config-only or backup-spec-only changes, a failed backup (old pg_dump client) that blocks migrate and keeps components drained behind the maintenance page with the reason in status, snapshot reuse on a migrate retry, keepLast pruning, 0600 dumps, and the restore runbook with checksum verification. Every dump is checked against its manifest SHA-256 and read back in full.