Add PostgreSQL and MS-SQL database backup guides - #235
Merged
HiranAdikari merged 3 commits intoJul 23, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded complete PostgreSQL and Microsoft SQL Server backup guides for Harvester VMs, covering S3/IAM setup, Terraform cloud-init provisioning, scheduled backups, verification, database restoration, cross-datacenter recovery, retention, and production hardening. ChangesDatabase backup guides
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/tenancy/vm/examples/MSSQL-BACKUPS.md`:
- Around line 99-108: Remove the s3:DeleteObject action from the DbBackupObjects
runtime backup policy in the MSSQL backup example, leaving the remaining S3
permissions and resource scope unchanged.
- Around line 153-160: Update the db_backup_env template in MSSQL-BACKUPS.md to
avoid interpolating raw credentials into a sourced shell file; safely encode or
shell-escape every secret value, including db_s3_access_key, db_s3_secret_key,
and mssql_sa_password, so quotes, substitutions, backticks, and newlines cannot
alter execution. Preserve the existing configuration values while ensuring the
generated backup.env remains safe when sourced.
- Around line 171-192: Update the backup workflow around the timestamped DEST
prefix so uploads first target a temporary prefix. After all database backups
and the login artifact succeed, upload a manifest and create a final completion
marker; ensure failures cannot publish that marker. Update the restore logic to
consider only prefixes containing the completion marker, including the
corresponding flow around the additional referenced section.
- Around line 185-191: Scope the backup/restore guidance around the $SQLCMD
export and logins.sql to password-authenticated SQL login creation only. Qualify
or remove claims that users map automatically or application connectivity
remains unchanged, and state that server roles, permissions, default databases,
disabled state, and existing-login reconciliation are not preserved.
- Around line 176-183: Update the backup and restore loops in the MSSQL backup
example to preserve database names as opaque values: iterate query results
line-by-line without whitespace splitting, escape SQL identifiers using
QUOTENAME instead of interpolating names inside brackets, and use a stable
filename/manifest mapping consistently for both backup and restore.
- Around line 176-180: Update every sqlcmd invocation in the MSSQL backup
example to remove the -P password argument and provide the sa password through
the SQLCMDPASSWORD environment variable instead, including both the
database-listing and backup calls while preserving their existing commands and
options.
- Around line 173-175: Update the backup-directory setup around BK and mkdir so
/var/opt/mssql/backup is owned by mssql:mssql and has permissions allowing SQL
Server writes; preserve the existing mkdir -p behavior while applying ownership
and an appropriate mode either during creation or immediately afterward.
- Around line 328-364: The restore procedure in the documented recovery flow
must address TDE prerequisites. Update the relevant §5.1/§5.3 restore
instructions to back up the TDE certificate and private key from master and
restore them on the fresh VM before the RESTORE DATABASE loop, or explicitly
state that the guide supports non-TDE databases only.
- Around line 353-359: The restore snippets in this document must support
fresh-VM paths and existing databases: before each RESTORE DATABASE, query
RESTORE FILELISTONLY to obtain logical data and log names, set destination paths
on the VM, and include corresponding WITH MOVE clauses. Also switch existing
databases to SINGLE_USER WITH ROLLBACK IMMEDIATE before restoring, and apply
these changes to both restore snippets while retaining overwrite behavior.
- Around line 247-256: Update the db-backup.service unit content to explicitly
require and start mssql-server.service, not just order after it, and add a
suitable TimeoutStartSec for large backups. Keep the existing backup command and
network ordering unchanged.
- Around line 179-182: Update the backup command in the MSSQL backup procedure
to include WITH CHECKSUM, then validate the uploaded .bak using RESTORE
VERIFYONLY or an equivalent test-restore before the rm -f cleanup. Keep deletion
conditional on both the local backup being readable and the upload succeeding.
- Around line 118-121: Update the backup guide to separate login export from the
database backup flow: either document its server-admin credential requirements
and store that credential outside the backup environment/state path, or remove
login export and configure a least-privilege backup-only principal. Keep the
backup timer and backup credentials scoped only to the required database backup
permissions.
In `@modules/tenancy/vm/examples/POSTGRES-BACKUPS.md`:
- Around line 100-107: Remove s3:DeleteObject from the Action list in the backup
credentials example, while preserving the permissions required by the backup and
restore workflows. Do not add deletion permission elsewhere; administrative
deletion should remain separately controlled.
- Around line 163-168: Add a completion marker or manifest to the backup flow
around the pg_dumpall and per-database pg_dump commands, creating it only after
every dump and upload succeeds. Update the restore logic around the referenced
restore procedure to select only timestamp prefixes containing that marker,
excluding incomplete newest prefixes.
- Around line 166-167: Update the backup and corresponding restore logic in the
documented PostgreSQL workflow to avoid shell word-splitting and
whitespace-unsafe S3 listing parsing for database names. Use a delimiter-safe
manifest or structured encoding when enumerating names, preserve each exact
database name through dump and restore, and ensure generated S3 keys remain
reliably matchable.
- Around line 287-298: Update the verification commands following the systemd
checks to first identify the latest timestamp prefix, then run aws s3 ls against
that specific prefix so it displays roles.sql and each database dump. Keep the
existing root-only environment loading and region options intact, and ensure the
subsequent on-demand backup instruction remains unchanged.
- Around line 166-168: Update the backup loop query in the PostgreSQL backup
guide to include the postgres database so every database is uploaded, or revise
the documented contract to explicitly state that postgres is excluded; keep the
dump and S3 upload behavior unchanged.
- Around line 321-328: Update the restore block’s shell commands to enable
fail-fast behavior with set -euo pipefail and add --exit-on-error to the
pg_restore invocation. Keep the existing per-database loop and restore options
unchanged so any failed pipeline or database restore stops the run immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5ba38f4-51c0-4028-aea3-80de1df75c62
📒 Files selected for processing (2)
modules/tenancy/vm/examples/MSSQL-BACKUPS.mdmodules/tenancy/vm/examples/POSTGRES-BACKUPS.md
Comment on lines
+166
to
+167
| for db in $(psql -Atc "SELECT datname FROM pg_database WHERE datistemplate=false AND datname<>'postgres';"); do | ||
| pg_dump -Fc -d "$db" | aws s3 cp - "$DEST/$db.dump" --region "$DB_S3_REGION" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Avoid shell word-splitting and S3-key parsing for database names.
for db in $(...) breaks valid names containing whitespace, and aws s3 ls | awk | grep cannot reliably round-trip names containing whitespace or path separators. Such databases may be backed up under unusable keys or skipped during restore. Use a manifest with safely encoded database names, or a delimiter-safe structured listing.
Also applies to: 325-328
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/tenancy/vm/examples/POSTGRES-BACKUPS.md` around lines 166 - 167,
Update the backup and corresponding restore logic in the documented PostgreSQL
workflow to avoid shell word-splitting and whitespace-unsafe S3 listing parsing
for database names. Use a delimiter-safe manifest or structured encoding when
enumerating names, preserve each exact database name through dump and restore,
and ensure generated S3 keys remain reliably matchable.
Security/robustness fixes across both guides: - drop s3:DeleteObject from the runtime IAM policy (backup/restore never delete objects; retention is the S3 lifecycle rule) - deliver secrets via the systemd unit's EnvironmentFile instead of a shell `source` (no evaluation of secret values); load env with a non-eval reader in the manual snippets - write a COMPLETE marker last and restore only the latest marked prefix so a partial run is never restored - fail fast on restore (set -euo pipefail; pg_restore --exit-on-error) - add TimeoutStartSec=0 and Wants= the DB service to the backup unit MS-SQL specifics: - pass the sa password via SQLCMDPASSWORD, not -P (keeps it out of ps) - create the backup dir owned by mssql (install -d -o mssql -g mssql) - BACKUP ... WITH CHECKSUM + RESTORE VERIFYONLY before upload - scope the login-export claim to SQL-auth logins; note TDE, WITH MOVE / SINGLE_USER on restore, and a dedicated backup principal for production PostgreSQL specifics: - exclude only the postgres maintenance DB and narrow the wording; -AtXc
Contributor
Author
|
Fixed
Documented rather than re-architected (these are example guides)
|
Add a short "Configure what to back up and when" subsection to both guides: which databases (edit the SELECT/WHERE), how often (the timer OnCalendar), and retention (the S3 lifecycle Expiration). Makes the existing knobs explicit instead of only implied by the scripts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gnudeep
approved these changes
Jul 23, 2026
HiranAdikari
approved these changes
Jul 23, 2026
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
3 participants
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.
Purpose
Resolves #233. The
tenancy/vmexamples now cover VM-file backups with restic (#232), but not database backups. A database needs its engine's native tooling (logical/native backups are portable and restore into a same-or-newer engine; a raw data-directory copy is not). This adds the database companions to the restic and cluster backup guides.Goals
Provide self-service database backup + restore guides beside the
tenancy/vmmodule, matching the style ofk8s-cluster/examples/BACKUPS.mdandvm/examples/BACKUPS.md.Approach
Add two guides under
modules/tenancy/vm/examples/:POSTGRES-BACKUPS.md—pg_dumpall --roles-only+ per-databasepg_dump -Fc, streamed straight to the tenant's own S3 bucket under timestamped prefixes; in-guest systemd timer (running aspostgres, peer auth — no DB password stored) installed via thevmmodule's base64-embeddeduser_data; restore into a fresh VM, a single database, and cross-DC DR.MSSQL-BACKUPS.md— nativeBACKUP DATABASE ... TO DISK+aws s3 cp(the reliable path vsBACKUP TO URL), with server logins exported separately (SID + password hash) and contained-database auth enabled on restore; same install/verify/restore shape.Both follow the established structure: least-privilege bucket + IAM (with the multipart actions), cloud-init base64 install + timer, verify, restore (fresh VM / single database / cross-DC), a scheduling & retention reference, and a production-hardening checklist. They cross-link the restic and cluster guides.
Release note
Docs: add self-service PostgreSQL and MS-SQL database backup + restore guides for the
tenancy/vmmodule.Summary by CodeRabbit