docs(backups): document PostgreSQL point-in-time recovery (PITR)#629
docs(backups): document PostgreSQL point-in-time recovery (PITR)#629Andrey Kolkov (androndo) wants to merge 1 commit into
Conversation
Mirror the new "Point-in-time recovery (PostgreSQL)" section from the monorepo docs/operations/backup-classes.md into the website copies (next + v1.6): how to set spec.options.recoveryTime on a RestoreJob, the recoverable window, how to discover the earliest/latest restorable time from the backup catalog, the RecoveryTargetUnreachable failure behavior, and idempotency under GitOps. Follows up cozystack/cozystack#3383 (PITR support, issue #2774), which added the section to the monorepo docs the website mirrors. Signed-off-by: Andrey Kolkov <androndo@gmail.com>
✅ Deploy Preview for cozystack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds PostgreSQL point-in-time recovery documentation for ChangesPostgreSQL PITR documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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 `@content/en/docs/next/operations/services/backup-classes.md`:
- Around line 247-249: Update the recovery-window backup query in
content/en/docs/next/operations/services/backup-classes.md:247-249 and
content/en/docs/v1.6/operations/services/backup-classes.md:247-249 to filter for
status.phase == "completed" before sorting by stoppedAt and selecting the oldest
backup. Apply the same completed-backup filter in both document copies.
- Line 255: The restore guidance around the upper-bound WAL description must not
imply that near-now targets are always safe. Update the sentence in
content/en/docs/next/operations/services/backup-classes.md:255 and
content/en/docs/v1.6/operations/services/backup-classes.md:255 to make near-now
restores conditional on confirming the required WAL has been archived and is
reachable before the restore deadline; preserve the existing barman-cloud
verification guidance.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f6f26296-e49b-4626-819b-77cc2bbf27c3
📒 Files selected for processing (2)
content/en/docs/next/operations/services/backup-classes.mdcontent/en/docs/v1.6/operations/services/backup-classes.md
| kubectl -n <ns> get backups.postgresql.cnpg.io \ | ||
| --sort-by=.status.stoppedAt \ | ||
| -o custom-columns=NAME:.metadata.name,PHASE:.status.phase,START:.status.startedAt,STOP:.status.stoppedAt,BEGINWAL:.status.beginWal,ENDWAL:.status.endWal |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "File locations:"
git ls-files | rg 'backup-classes\.md$' || true
echo
echo "Relevant sections (next):"
sed -n '220,270p' content/en/docs/next/operations/services/backup-classes.md
echo
echo "Relevant sections (v1.6):"
sed -n '220,270p' content/en/docs/v1.6/operations/services/backup-classes.md
echo
echo "All kubectl backups.postgresql.cnpg.io commands:"
rg -n "kubectl .*backups\.postgresql\.cnpg\.io|status\.phase|stoppedAt" content/en/docs content/en/docs/next content/en/docs/v1.6 2>/dev/null || trueRepository: cozystack/website
Length of output: 28526
Add a completed-backup filter to the recovery-window query.
The query is introduced with “Each completed base backup records its WAL range…”, but it lists all cnpg.io/Backup statuses. Add a selector or post-filter for status.phase == "completed" before sorting/picking the oldest stoppedAt, and apply the same change in the versioned doc copy.
📍 Affects 2 files
content/en/docs/next/operations/services/backup-classes.md#L247-L249(this comment)content/en/docs/v1.6/operations/services/backup-classes.md#L247-L249
🤖 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 `@content/en/docs/next/operations/services/backup-classes.md` around lines 247
- 249, Update the recovery-window backup query in
content/en/docs/next/operations/services/backup-classes.md:247-249 and
content/en/docs/v1.6/operations/services/backup-classes.md:247-249 to filter for
status.phase == "completed" before sorting by stoppedAt and selecting the oldest
backup. Apply the same completed-backup filter in both document copies.
| kubectl -n <ns> get backup.backups.cozystack.io <name> -o jsonpath='{.spec.driverMetadata}' | ||
| ``` | ||
|
|
||
| The upper bound — the latest archived WAL — is whatever the source has shipped so far; with archiving healthy, any time up to a few seconds ago is safe. To confirm the archive is current, check that the source cluster's WAL is flowing to S3 (the `barman-cloud` sidecar logs an upload per segment) before restoring to a near-now target. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'content/en/docs/(next|v1\.6)/operations/services/backup-classes\.md$' || true
for f in content/en/docs/next/operations/services/backup-classes.md content/en/docs/v1.6/operations/services/backup-classes.md; do
if [ -f "$f" ]; then
echo
echo "=== $f ==="
sed -n '220,275p' "$f" | cat -n | sed 's/^/ /'
fi
done
echo
echo "Context search around 'up to a few seconds ago' and near-now:"
rg -n "up to a few seconds ago|near-now|archived WAL|latest archived WAL|barman-cloud|archives current|archive is current" content/en/docs/next content/en/docs/v1.6 -S || trueRepository: cozystack/website
Length of output: 26317
Avoid implying near-now restores are always safe.
Archive lag is not fixed; the preceding text already warns that if archiving is stalled a near-now target can exceed the restore deadline, and that only targets still reachable before the deadline are eventually reached. Keep the sentence conditional or narrow it to targets that can be confirmed as archived.
content/en/docs/next/operations/services/backup-classes.md#L255content/en/docs/v1.6/operations/services/backup-classes.md#L255
📍 Affects 2 files
content/en/docs/next/operations/services/backup-classes.md#L255-L255(this comment)content/en/docs/v1.6/operations/services/backup-classes.md#L255-L255
🤖 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 `@content/en/docs/next/operations/services/backup-classes.md` at line 255, The
restore guidance around the upper-bound WAL description must not imply that
near-now targets are always safe. Update the sentence in
content/en/docs/next/operations/services/backup-classes.md:255 and
content/en/docs/v1.6/operations/services/backup-classes.md:255 to make near-now
restores conditional on confirming the required WAL has been archived and is
reachable before the restore deadline; preserve the existing barman-cloud
verification guidance.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
This is an unusually accurate piece of documentation. I verified every concrete claim against the cozystack/cozystack source (fresh clone, default branch): the condition types (RecoveryConverged, Ready, TargetPurged), the reasons (RecoveryTargetUnreachable, RestoreFailed), the status.phase: Failed value, the 30m default deadline (cnpgDefaultRestoreDeadline), the FATAL string the driver classifies on, the recoveryTime → CNPG recoveryTarget.targetTime mapping, the deadline-only classification logic, the referenced example (examples/backups/postgres/45-restorejob-pitr.yaml + run-all.sh), and the .status vs .spec placement of the fields in the discovery queries — all match. One minor factual inaccuracy and two nits, none blocking.
Findings
[MINOR] Idempotency section — the resume/render ordering is inverted (content/en/docs/next/operations/services/backup-classes.md, "Idempotency under GitOps"; same in v1.6)
"…suspends the target's HelmRelease across the purge so Flux cannot race the bootstrap swap, and resumes it once the recovery cluster is rendered."
The source does the opposite: the driver resumes the HelmRelease before the render, and the resume is what enables the render. In internal/backupcontroller/cnpgstrategy_controller.go the order is: suspend HR → patch the Postgres app (bootstrap.recovery) → purge the Cluster + PVCs → wait until cnpgClusterFullyGone → then resume the HR. The in-code comment is explicit:
"…only resume once the Cluster + PVCs are fully gone so the next render lands
bootstrap.recoveryon an empty namespace."
So Helm renders the recovery cluster after the resume, not before it. The section's actual conclusion ("an in-progress restore is safe to reconcile … re-attaches to the recovering cluster rather than deleting it and starting over") is correct; only the mechanism clause is reversed. Suggested wording:
"…and resumes it once the old cluster and its PVCs are fully gone, so Helm's next render lands
bootstrap.recoveryon an empty namespace."
[NIT] Recovery-window query lists all backup phases despite the "completed" framing (Discovering the earliest / latest restorable time; agrees with the existing CodeRabbit comment)
The prose says "Each completed base backup records its WAL range…" and "the oldest STOP is the window's lower bound", but the kubectl get backups.postgresql.cnpg.io --sort-by=.status.stoppedAt command returns every backup regardless of phase. Note that a server-side --field-selector on status.phase is not supported for this CRD, so the fix is either a one-line note to read the PHASE column (already in the output) and pick the oldest Completed row, or a client-side filter. Low value since the PHASE column is already shown, but the framing and the command are slightly out of step.
[NIT] "any time up to a few seconds ago is safe" — already adequately qualified (line ~255; re: the CodeRabbit stability comment)
I do not consider this blocking. The sentence is explicitly scoped to "with archiving healthy", and the immediately preceding paragraph already spells out the stalled-archiver caveat ("If archiving is badly behind … a near-now target can exceed even the default 30m window; restore to a point you can confirm is archived"). If you want to be extra defensive you could soften it, but the doc is internally consistent as written.
Caveats / notes (no action needed)
- Phase 5b (upgrade / fresh-install impact): N/A — the diff touches only
content/en/docs/**, nopackages/. No runtime, chart, RBAC, or migration surface. recoveryTimeandrestoreTimeoutSecondsare honored by the CNPG driver (typed inCNPGRestoreOptions) but are not enumerated in theRestoreJobCRD schema —spec.optionsis an opaquex-kubernetes-preserve-unknown-fieldsobject. This is not a doc defect (the fields work as documented), just context if a schema-driven reader wonders why they are not in the CRD.- The two doc copies (
nextandv1.6) are byte-identical in the added block except for theSee alsoversion path — good. - Claim that a
recoveryTimebefore the earliest base backup surfaces as genericRestoreFailed(notRecoveryTargetUnreachable) is consistent with the code's FATAL-match-or-generic-fallback structure and mirrors the monorepo's owndocs/operations/backup-classes.md. The underlying "PostgreSQL never emits that FATAL in this case" is runtime behavior not verifiable from source alone, but nothing in the code contradicts it.
2 participants
Mirrors the new Point-in-time recovery (PostgreSQL) section from the monorepo
docs/operations/backup-classes.mdinto the website copies (nextandv1.6).Follows up cozystack/cozystack#3383 (PITR support for the
backups.cozystack.ioRestoreJob API, fixes cozystack/cozystack#2774), which added the section to the monorepo docs that this site mirrors. #3313's earlierbackup-classes.mdmirror landed in #622; this is the PITR delta on top of it.The section covers:
spec.options.recoveryTime(RFC3339) on aRestoreJob, with a worked example;firstRecoverabilityPointis not reliably populated under the barman-cloud plugin);RecoveryTargetUnreachablereason for a target past the archive;Added to
nextandv1.6(the release line that already carries the barman-cloud plugin backup docs). Not added tov1.5— PITR is a new feature onmain, not part of 1.5. If 1.6 has already been cut without this feature, drop thev1.6file.Summary by CodeRabbit
RestoreJob.