Nightly pg_dump into the postgres volume, with backup health reporting (#1443) - #1444
Merged
Conversation
…1443) Volume-level snapshots of a live postgres data directory are only probably restorable — they look like a power cut to postgres. CSE IT recommended the standard mitigation: have the database write its own consistent dump into the same volume, so every snapshot they take carries a known-good restore point. A db-backup sidecar in both compose files runs scripts/pg_backup.sh hourly; the script is a single pass that writes one dated `pg_dump | gzip` per UTC day to pg_backups/ inside the data volume, prunes past 14 days (never the newest dump), and writes a status file — on failure too, so a broken backup is distinguishable from one that hasn't run yet. Two details are load-bearing. The sidecar's entrypoint is overridden so the postgres image's own entrypoint can't start a second server on that PGDATA. And the scheduling loop lives in the compose file rather than in the script, because `docker compose up -d` only recreates containers whose config changed — a loop inside the bind-mounted script would keep running stale code after every deploy, with no shell access to restart it. Django only ever reads the status, via a volume mounted read-only: /version.json gains backup_ok/last_backup_at/backup_age_hours/backup_count, the admin dashboard gets a superuser callout shown only when stale or failing, and Data Health gets a detail panel. This reporting isn't polish — the dumps sit in a Docker volume on a host with no shell, inside a PGDATA the website container can't traverse, so without it there's no way to observe whether backups happen at all. Host cron was considered and rejected: no shell on grabthar/docker-test2 means a permanent CSE IT dependency for every schedule change and no visibility. A per-deploy pre-migration dump was also rejected — its marginal recovery over the nightly dump is usually zero rows, and it would cost a second dump code path. Tested, because an untested backup is not a backup. Two Docker harnesses prove dump → destroy volume → restore: scripts/test_backup_restore.sh on a synthetic schema built to break a naive dump (unicode, embedded quotes/newlines, NULLs, bytea, a 540 KB row, views, FKs, sequences), and test_backup_restore_django.sh on the real schema, asserting Django accepts the restored database via `migrate --check` and `manage.py check`. Both also pin the initdb-refuses-a- non-empty-directory gotcha that docs/BACKUPS.md warns about. Unit tests cover the status reader's failure modes — it runs inside each_context, so it must degrade to "unknown" rather than take the admin down. Restore runbook, including what to hand CSE IT for a production restore: docs/BACKUPS.md
The prune path had no coverage, including the guard that keeps the newest dump regardless of age — the one that stops a long-failing backup from ending with pruning deleting the last good restore point. Both are now exercised against a scratch volume with artificially aged files.
- Add AdminBackupWarningTests (website/tests/test_backup_status.py): 4 integration tests hitting /admin/ and /admin/data-health/ through a real superuser/editor request, mirroring AdminLoggingWarningTests for the sibling LOG_TO_FILE feature. The existing tests only proved get_backup_status() returns the right dict in isolation, never that MakeabilityLabAdminSite.each_context actually wires BACKUP_STATUS into the rendered templates or that the superuser gate holds at the HTTP layer. - Fix scripts/test_backup_restore_django.sh's "m2m authors restored in order" assertion: it sorted both sides before comparing, so it would have passed even if the restore lost SortedManyToManyField's sort_value ordering (the two test emails happen to also be alphabetical). Seed the through-table out of alphabetical order and compare unsorted, so the assertion can actually fail. - Note the python3 host dependency in scripts/test_backup_restore.sh's header comment (used by its status.json-parsing helpers, unlike the rest of the script which runs entirely inside containers). - Make the db-backup sidecar's entrypoint loop respond to `docker compose stop`/`down` immediately instead of always eating the full 10s stop grace period: bash only processes a trapped signal between foreground commands, so a SIGTERM arriving during the hourly/retry `sleep` sat unhandled until the sleep finished. Add `trap 'exit 0' TERM INT` and background+wait the sleep so the trap fires right away. Verified live: docker stop went from ~10s to 0.12s. Verified end to end in the 1443 worktree: - `manage.py test website.tests.test_backup_status website.tests.test_version_endpoint --settings=makeabilitylab.settings_test` — 24/24 new/touched tests pass (2 unrelated pre-existing failures in test_logging_config, same root cause noted in the PR: local image predates #1439's concurrent-log-handler dependency). - `scripts/test_backup_restore.sh` — 35/35 pass. - `scripts/test_backup_restore_django.sh` — 15/15 pass, including the reworked ordering assertion. - Brought up the real db-backup sidecar via docker-compose-local-dev.yml and confirmed live: correct status.json written, and `docker stop` returns in 0.12s post-fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) — Sonnet 5, claude-sonnet-5
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.
Closes #1443.
A filesystem-level snapshot of a live Postgres data directory isn't guaranteed to be transaction-consistent — restoring one behaves like recovering from a hard power cut. UW CSE IT flagged this and recommended the standard mitigation: have the database write its own consistent dump on a schedule, into the same volume they snapshot, so every snapshot automatically carries a known-good restore point.
What this adds
A
db-backupsidecar service (in both compose files) runsscripts/pg_backup.shhourly. The script is a single pass: if today's dump doesn't exist it makes one, prunes anything past retention, and writes a status file./var/lib/postgresql/data/pg_backups/makeability-YYYY-MM-DD.sql.gz, mode 0600, atomic temp+rename,gzip -tverified before promotion.Django only ever reads the status, through a read-only mount:
/version.jsongainsbackup_ok,last_backup_at,backup_age_hours,backup_countThat reporting isn't polish. The dumps sit in a Docker volume on a host with no shell, inside a
PGDATAthe website container (uid 48) can't even traverse — without the status file there's no way to observe whether backups happen at all. Same reasoning aslog_to_filein #1283.Two things that are load-bearing
entrypointis overridden. Left alone, the postgres image's own entrypoint would try to start a second server on thatPGDATA.docker compose up -donly recreates a container whose config changed — a loop inside the bind-mounted script would keep running stale code after every deploy, with no shell access to restart it.Decisions
grabthar/docker-test2, so host cron would be a permanent CSE IT dependency for every schedule or retention change, with no visibility into whether it was still running.DJANGO_ENVgating so it wouldn't fire on every push to-test. The real control for destructive one-shots is what we already do: make them idempotent and disable them once run (Re-standardize legacy talk/poster/pub filenames that were never renamed #1401, Store original uploaded filename and show it (admin-only) for talks/posters/publications #1391).pg_dumpall --globals-only. The only role isadmin, recreated fromPOSTGRES_USERon any fresh container.Testing
An untested backup is not a backup, so the restore path is proven rather than assumed. Two Docker harnesses both genuinely destroy the volume and restore onto an empty one:
scripts/test_backup_restore.shbytea, a 540 KB row, views, foreign keys, sequences. Plus retention pruning, the never-delete-the-last-dump guard, failure reporting, and same-day idempotency.scripts/test_backup_restore_django.shmigrate, seeded through the ORM includingSortedManyToManyFieldthrough-tables and rich text. Asserts Django accepts the restored database:migrate --checkandmanage.py checkboth pass.Both also pin the
initdb-refuses-a-non-empty-directory behavior thatdocs/BACKUPS.mdwarns about, so that caveat can't silently go stale. Neither touches your real stack, database, or volumes.Django-side unit tests (
website/tests/test_backup_status.py, 28 with the/version.jsonadditions) cover the status reader's failure modes — missing file, truncated JSON, non-object JSON, unparseable timestamps, missing keys. That code runs insideeach_context, so it must degrade to "unknown" rather than take the whole admin down.I also brought up the real compose sidecar and confirmed the full chain end to end: sidecar writes → uid 48 reads through the read-only mount →
get_backup_status()reports healthy.Full suite: 763 tests, 2 failures, both in
test_logging_configand both pre-existing — the local image predates #1439'sconcurrent-log-handlerdependency. They fail identically on untouchedmasterwith the same image, and pass on this branch once the package is installed.Notes for review
.pa11yci.jsonhas no admin URLs. The callout reuses the existing AA-verified.ml-log-warningpalette rather than introducing new colors, and the panel uses<th scope="row">. Happy to add screenshots if you want them before merge.backup-statusvolume is deliberately not external and not something CSE IT needs to back up — it holds only the status file and is regenerated every pass. The dumps live indb-data, which is the volume that gets snapshotted.Person.email, which is deliberately withheld from the public API. They're written 0600 into a Docker volume and must never be moved under a web-served path.docs/BACKUPS.mdhas the restore runbook, including the section to hand CSE IT for a production restore (which needs someone with Docker access on the host — we don't have it).🤖 Generated with Claude Code — Opus 5 (1M context), claude-opus-5[1m]