feat(backup): add GCS database backup and restore - #673
Conversation
samxu01
left a comment
There was a problem hiding this comment.
Review verdict: changes requested — one Critical in the runbook, chart is approve-ready (as comment; formal review events 422 from this bot identity).
Sam's three checkpoints, verified against the diff: (2) spot-schedulability — no committed values combination tolerates workload-tier=spot:NoSchedule, so the taint blocks backup pods from spot nodes; values-dev pins pool: dev with only the dev toleration and the ADR-015 comment. ✅ (3) no secret material or project ids — PROJECT_ID placeholder gone, SA email values-gated via with, bucket defaults emptied, creds stay in the existing ESO secret. ✅ Retention logic is correct (lexicographic sort of backup-YYYYMMDD-HHMMSS is chronological; empty-list guarded; head -n -N is fine on the Debian-based gcloud image). Both scripts are set -euo pipefail, which fixes the previously masked pg_dump | gzip failure.
Critical
docs/runbooks/db-backup-restore.md— Mongo restore drill:mongorestore --uri="...商/commonly_restore" --archive=/tmp/backup.archive.gz --gzip --dropdoes not restore intocommonly_restore. With--archive, mongorestore ignores the URI's database path and restores into the namespaces recorded in the archive — i.e.commonly.*. Combined with--drop, running this drill against the in-cluster MongoDB drops and overwrites the livecommonlydatabase. Fix: add explicit namespace mapping —--nsInclude="commonly.*" --nsFrom="commonly.*" --nsTo="commonly_restore.*"(derive the source db name from the configuredMONGO_URI) — and add a warning line stating why the mapping is load-bearing. The PostgreSQL drill is correct as written (plain-format dump has no\connect, so it loads into the connectedcommonly_restoreDB).
Nits (non-blocking)
- Runbook hardcodes
MONGO_IMAGE="mongo:latest"/POSTGRES_IMAGE="postgres:15"two paragraphs after saying "the same image configured in Helm" — read them fromvalues.yaml(mongodb.image.*/postgresql.image.*) instead. PG_RESTORE_USER="postgres"is hardcoded; the backup job authenticates as{{ .Values.postgresql.username }}with thepostgres-passwordsecret — use that same user orcreatedbmay fail on auth.
Fix the Critical (and ideally the nits — they're all in the same doc), and I'll approve.
samxu01
left a comment
There was a problem hiding this comment.
Re-review verdict: APPROVE — ready to merge (as comment; formal review events 422 from this bot identity).
Verified 781c01ed: the Mongo restore drill now derives MONGO_SOURCE_DB from the live mongo-uri Secret (query-string stripped, validated against ^[A-Za-z0-9_-]+$ with a hard exit on mismatch) and applies --nsInclude/--nsFrom/--nsTo into commonly_restore.*, with the load-bearing warning explaining that archive namespaces override the URI path. The destroy-the-live-DB path is closed. Both nits are fixed the right way — restore image and PG user are read from the deployed CronJob via JSONPath, so the drill can't drift from the chart. Chart templates are unchanged from my previous pass (spot-unschedulable, no secrets/project ids, retention verified). Nothing further.
Summary
Root cause
The dormant backup jobs used database images that do not contain gsutil, so their first upload would fail. They also hardcoded a project placeholder, had no retention policy, and could schedule without protection from overlap or spot preemption.
Validation
This is the repository half of #665. Bucket creation, IAM/Workload Identity binding, operator-private values, and enablement remain intentionally post-merge operations.