fix: make helm-v1 data migration crash-resumable - #261
Open
sklarsa wants to merge 1 commit into
Open
Conversation
The migration init container was not crash-resumable. Its single go/no-go signal -- "is there a tables.d.* marker at the volume root?" -- was destroyed by the migration itself: the marker is moved into db/ before the old db/ backup is relocated to db/db. An OOMKill/eviction/node-drain between those steps makes Kubernetes re-run the init container from scratch; the re-run finds no marker, prints "Nothing to move.", exits 0, and strands the original db/ data under a timestamped temp dir. The timestamped temp name also made leftovers unrecognisable across runs. Redesign the script to be idempotent and resumable from an interruption at any point: - Durable completion sentinel (db/.helm_v1_migration_done): written LAST, checked FIRST, so the decision no longer depends on the marker surviving. - Deterministic backup name (db_helm_migration_1_bak) instead of a timestamp, so a leftover is recognisable and finishable on the next run. - In-progress marker (db/.helm_v1_migration_started) committed once db/ is claimed as the work dir, so a re-run never mistakes a half-filled work dir for a fresh pre-existing db/ and re-nests it. - Go/no-go now also resumes on a leftover backup or an in-progress marker, and a top-of-script recovery finishes a backup already sitting inside db/. - The refuse-if-db/db-exists safety is kept for genuinely ambiguous states. Behaviour on the happy paths is preserved: empty-dir marker detection via ls -d, null-glob and broken-symlink guards on both loops, quoting for names with spaces, hidden-file globs, and db/db nesting for a pre-existing db/. Verified under dash: behaviour-preservation scenarios, single-crash injection after every filesystem op (Case A and Case B), double-injection matrices, and randomized multi-injection fuzzing all converge to the correct db/ layout with no stranded data and no leftover temp/backup dirs. shellcheck -s sh clean and helm lint pass. Closes #260 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Independent verification on kind (real alpine:3.23 busybox + QuestDB)Verified this on a local kind cluster with the actual chart (the automated crash-injection matrix above runs under
LGTM. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the Helm-v1 data-migration init container (
charts/questdb/templates/init_db_migrations_configmap.yaml,migrate_to_helm_v1.sh) idempotent and crash-resumable.Closes #260.The bug (#260)
The migration used a single go/no-go signal — "is there a
tables.d.*marker at the volume root?" — but the migration itself destroys that signal partway through: theforloop moves the marker intodb/before the olddb/backup is relocated todb/db. If the init container is killed in between (OOMKill / eviction / node-drain → Kubernetes re-runs init containers from scratch), the re-run sees no marker → printsNothing to move.→ exits 0 → the originaldb/data is stranded under a timestamped temp dir and never relocated. The timestamped temp name also made leftovers unrecognisable across runs.The fix
Redesigned so that interrupting the script at any point and re-running it to completion converges to the correct
db/layout, with no stranded data and no leftover temp/backup dirs (no ~2× copy — only renames):db/.helm_v1_migration_done— written last, checked first, so the decision no longer depends on the marker surviving.db_helm_migration_1_bak(no timestamp) — a leftover is recognisable and finishable across runs.db/.helm_v1_migration_started— committed oncedb/is claimed as the work dir, so a re-run never mistakes a half-filled work dir for a fresh pre-existingdb/and re-nests it (this is the subtle Case-A resumability trap).db/.db/db-exists safety (don't silently clobber) and everything the old script already got right:ls -dempty-dir marker detection (with its# shellcheck disable=SC2086), null-glob + broken-symlink guards on both loops, quoting for names with spaces, hidden-file globs, anddb/dbnesting for a pre-existingdb/.Verification
Extracted the embedded script and drove it under
/bin/dash. Crash injection wrapsmv/mkdir/touch/rmto run the real op thenexit 99after the Nth mutation, then the unmodified script runs to completion and the finaldb/layout is diffed against the fully-migrated expected layout (with checks for no stranded data, no leftover*_bak, and root containing onlydb/).Behaviour-preservation (all exit 0, correct layout): fresh/empty volume, old→new (regular + hidden + subdir + spaces), pre-existing
db/→db/db, broken visible & hidden symlinks, empty-directory marker, and idempotent re-run (Already migrated.).Crash-injection matrix — every point converged:
db/+ full mix) — single injection after op Ndb/) — single injection after op Ndb/db/db, not strandeddb/db, root files →db/Also:
shellcheck -s shclean (only the intentional, already-suppressed SC2086 on the marker glob),helm lint charts/questdb/passes, andhelm template … | grep migrate_to_helmrenders; the helm-rendered script is byte-identical to the source and re-passes shellcheck.🤖 Generated with Claude Code