Repository navigation
feat(backups): support PostgreSQL point-in-time recovery (PITR) - #3383
Conversation
github-actions
Bot
added
area/storage
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPostgreSQL ChangesPostgreSQL PITR
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run-all.sh
participant RestoreJob
participant CNPG
participant PostgreSQL
run-all.sh->>RestoreJob: submit recoveryTime restore
RestoreJob->>CNPG: configure recovery target
CNPG->>PostgreSQL: replay archived WAL
PostgreSQL-->>CNPG: recovered state or unreachable-target FATAL
CNPG-->>RestoreJob: cluster health and recovery status
RestoreJob-->>run-all.sh: Succeeded or RecoveryTargetUnreachable
Suggested labels: Suggested reviewers: 🚥 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 |
6f7db63 to
8a9af01
Compare
…i-driver (#3386) ## What this PR does `quay.io/centos/centos:stream9` is a **rolling tag** — upstream deletes the previously-pinned digest whenever it publishes a new `stream9` build. The digest re-pinned in ce4b263 (`sha256:a93e3316…`) has now itself been deleted upstream, so every `kubevirt-csi-driver` image build fails: ``` #3 ERROR: quay.io/centos/centos:stream9@sha256:a93e3316…: not found ERROR: failed to solve: … failed to resolve source metadata … not found make: *** [Makefile:43: image-kubevirt-csi-driver] Error 1 ``` This breaks the **Build packages/apps/kubernetes** job on `main` and on every open PR (e.g. observed on #3383). Re-pins to the current `stream9` manifest-list digest `sha256:3714c89f1903dac5a5e1eb6c02d84189ff6d962a0c18df01feba0a5406856926`, resolved via `docker buildx imagetools inspect quay.io/centos/centos:stream9`. > Note: this is the second recurrence of the same class of breakage (ce4b263 fixed the first). Because `stream9` is a rolling tag, a pinned digest will keep rotting; a follow-up could track it via renovate or pin a date-stamped/immutable tag instead. Out of scope here — this just unblocks CI. ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated the container build to use a refreshed CentOS Stream 9 base image. * No end-user-facing functionality or behavior changed. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
8a9af01 to
596ed77
Compare
dosubot
Bot
added
the
area/database
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enables point-in-time recovery (PITR) for PostgreSQL backups within the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements point-in-time recovery (PITR) for PostgreSQL databases managed by CNPG. It introduces a fail-fast mechanism that scans bootstrap-recovery pod logs for specific PostgreSQL FATAL errors to quickly identify unreachable recovery targets, preventing restore jobs from hanging. To support this, the controller was updated to use a Kubernetes Clientset, and RBAC permissions were expanded to allow reading pods/log. The changes also include comprehensive documentation, updated test scripts, expanded E2E chainsaw tests, and new unit tests to validate the log-parsing and pod-filtering logic. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/operations/backup-classes.md (1)
239-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQualify the
firstRecoverabilityPointobservation.“Unreliable under the barman-cloud plugin” is generalized from one dev7 observation on CNPG 1.28.1 and may become stale or misleading across versions and environments. Mark it as an environment/version-specific caveat and retain the catalog-based procedure as the recommended fallback.
🤖 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 `@docs/operations/backup-classes.md` around lines 239 - 242, Update the “Discovering the earliest / latest restorable time” section to qualify the empty firstRecoverabilityPoint observation as specific to the dev7 test cluster running CNPG 1.28.1 with the barman-cloud plugin, rather than presenting it as generally unreliable. Retain the backup-catalog procedure as the recommended fallback.
🤖 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 `@docs/operations/backup-classes.md`:
- Around line 243-248: Update the backup catalog command to include only
completed backup resources, using the appropriate phase filter before sorting
and displaying STOP. Preserve the existing columns and ordering, and ensure
operators cannot mistake failed or in-progress rows for recoverable backups.
- Around line 231-235: Revise the “Latest” and very-recent recovery guidance in
the backup documentation to avoid implying a fixed seconds-long or
wall-clock-safe margin. State that the restorable boundary depends on the most
recent WAL shipped to object storage, archive lag, segment boundaries, and the
available WAL/catalog state; direct readers to verify that state when selecting
a recoveryTime, while preserving the existing retry and unreachable-target
behavior.
In `@internal/backupcontroller/cnpgstrategy_controller.go`:
- Around line 1480-1499: Update RestoreJobReconciler.readPodContainerLog to
create a derived context with a finite timeout before calling req.Stream, using
the controller’s established timeout constant or configuration if available.
Ensure the derived context is canceled via defer and pass it to Stream so
stalled log retrieval cannot block indefinitely.
---
Nitpick comments:
In `@docs/operations/backup-classes.md`:
- Around line 239-242: Update the “Discovering the earliest / latest restorable
time” section to qualify the empty firstRecoverabilityPoint observation as
specific to the dev7 test cluster running CNPG 1.28.1 with the barman-cloud
plugin, rather than presenting it as generally unreliable. Retain the
backup-catalog procedure as the recommended fallback.
🪄 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: 75939325-a4e7-48f3-94ff-f1259a6a7b69
📒 Files selected for processing (12)
docs/operations/backup-classes.mdexamples/backups/postgres/00-helpers.shexamples/backups/postgres/35-restorejob-in-place.yamlexamples/backups/postgres/45-restorejob-pitr.yamlexamples/backups/postgres/README.mdexamples/backups/postgres/cleanup.shexamples/backups/postgres/run-all.shhack/e2e-chainsaw/postgres/chainsaw-test.yamlinternal/backupcontroller/cnpgstrategy_controller.gointernal/backupcontroller/cnpgstrategy_controller_test.gointernal/backupcontroller/restorejob_controller.gopackages/system/backupstrategy-controller/templates/rbac.yaml
| ```bash | ||
| # Completed base backups, oldest first: STOP is the earliest instant that | ||
| # backup alone can restore to; the oldest STOP is the window's lower bound. | ||
| 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
Filter the catalog output to completed backups.
The command lists every cnpg.io/Backup and only prints its phase, despite the surrounding text saying these are completed backups. A failed or in-progress resource with stoppedAt could be mistaken for the earliest recoverable point. Explicitly filter or instruct operators to discard non-completed rows before using STOP.
🤖 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 `@docs/operations/backup-classes.md` around lines 243 - 248, Update the backup
catalog command to include only completed backup resources, using the
appropriate phase filter before sorting and displaying STOP. Preserve the
existing columns and ordering, and ensure operators cannot mistake failed or
in-progress rows for recoverable backups.
| // readPodContainerLog returns the tail of a pod container's log via the | ||
| // clientset (the controller-runtime cache client cannot read the log | ||
| // subresource). | ||
| func (r *RestoreJobReconciler) readPodContainerLog(ctx context.Context, namespace, podName, container string) (string, error) { | ||
| tail := int64(cnpgRecoveryLogTailLines) | ||
| req := r.Clientset.CoreV1().Pods(namespace).GetLogs(podName, &corev1.PodLogOptions{ | ||
| Container: container, | ||
| TailLines: &tail, | ||
| }) | ||
| stream, err := req.Stream(ctx) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer stream.Close() | ||
| data, err := io.ReadAll(stream) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return string(data), nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the pod-log stream with a timeout.
req.Stream(ctx) has no per-call deadline. The reconcile ctx passed in typically carries no timeout unless the caller set one, so a stalled/slow connection to the API server while streaming a recovery pod's log can block this call - and the reconcile goroutine - indefinitely. TailLines bounds response size, not connection duration.
🔧 Proposed fix
func (r *RestoreJobReconciler) readPodContainerLog(ctx context.Context, namespace, podName, container string) (string, error) {
+ ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
+ defer cancel()
tail := int64(cnpgRecoveryLogTailLines)
req := r.Clientset.CoreV1().Pods(namespace).GetLogs(podName, &corev1.PodLogOptions{
Container: container,
TailLines: &tail,
})
stream, err := req.Stream(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // readPodContainerLog returns the tail of a pod container's log via the | |
| // clientset (the controller-runtime cache client cannot read the log | |
| // subresource). | |
| func (r *RestoreJobReconciler) readPodContainerLog(ctx context.Context, namespace, podName, container string) (string, error) { | |
| tail := int64(cnpgRecoveryLogTailLines) | |
| req := r.Clientset.CoreV1().Pods(namespace).GetLogs(podName, &corev1.PodLogOptions{ | |
| Container: container, | |
| TailLines: &tail, | |
| }) | |
| stream, err := req.Stream(ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| defer stream.Close() | |
| data, err := io.ReadAll(stream) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(data), nil | |
| } | |
| // readPodContainerLog returns the tail of a pod container's log via the | |
| // clientset (the controller-runtime cache client cannot read the log | |
| // subresource). | |
| func (r *RestoreJobReconciler) readPodContainerLog(ctx context.Context, namespace, podName, container string) (string, error) { | |
| ctx, cancel := context.WithTimeout(ctx, 10*time.Second) | |
| defer cancel() | |
| tail := int64(cnpgRecoveryLogTailLines) | |
| req := r.Clientset.CoreV1().Pods(namespace).GetLogs(podName, &corev1.PodLogOptions{ | |
| Container: container, | |
| TailLines: &tail, | |
| }) | |
| stream, err := req.Stream(ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| defer stream.Close() | |
| data, err := io.ReadAll(stream) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(data), nil | |
| } |
🤖 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 `@internal/backupcontroller/cnpgstrategy_controller.go` around lines 1480 -
1499, Update RestoreJobReconciler.readPodContainerLog to create a derived
context with a finite timeout before calling req.Stream, using the controller’s
established timeout constant or configuration if available. Ensure the derived
context is canceled via defer and pass it to Stream so stalled log retrieval
cannot block indefinitely.
5f3e094 to
f6313e2
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM with non-blocking notes
Reviewed the change end-to-end. The core premise (CNPG accumulates multiple
failed bootstrap-recovery pods rather than replacing one in place) is
independently confirmed against upstream cloudnative-pg sources: the
full-recovery bootstrap is a batch/v1 Job with RestartPolicy: Never and a
default BackoffLimit=6, i.e. up to 1 + 6 = 7 coexisting pods, which justifies
the cnpgRecoveryMaxInspectPods=5 / cnpgRecoveryUnreachableMinAttempts=3
constants. Build, vet and tests are clean; the CRD/RBAC/upgrade surface is
purely additive.
MINOR
internal/backupcontroller/cnpgstrategy_controller.go:1435-1437 — Forbidden
guard degrades silently. When readPodContainerLog returns IsForbidden
(the pods/log grant added by this PR is absent or stripped by an external
policy layer), the code logs at Info level and continues, disabling the
RecoveryTargetUnreachable fail-fast for this and all subsequent reconciles,
with no Event and no dedicated status condition/reason. Kept at MINOR because
the path still terminates via the pre-existing restoreTimeoutSeconds (not
touched by this PR), which sets Ready=False with a PITR hint
(cnpgstrategy_controller.go:490-495) — it is a fallback to prior accepted
behavior, not a new silent hang. Suggestion: after N consecutive Forbidden
reads, set a dedicated status condition or emit a Warning Event.
Makes RestoreJob.spec.options.recoveryTime a genuinely supported, tested capability for the new backups.cozystack.io API (issue #2774). The recoveryTime -> CNPG bootstrap.recovery.recoveryTarget.targetTime plumbing already existed on the barman-cloud-plugin branch; this adds the reliability and verification layer the issue asked for: - Fail-fast for an unreachable target: when a RestoreJob has recoveryTime set and the recovery Cluster is not healthy, the driver reads the CNPG full-recovery pod logs and, once several distinct attempts have exited with PostgreSQL's "recovery ended before configured recovery target was reached" FATAL, fails the RestoreJob with conditions RecoveryConverged=False / Ready=False (reason RecoveryTargetUnreachable) instead of hanging to the restore deadline. Health is checked first, so a converged recovery always wins; counting distinct FATAL-ing attempts (not pod failures) tolerates a transient archive-lag miss on a near-now target and ignores transient recovery-pod crashes. Adds the pods/log RBAC grant the log read requires. - e2e (examples/backups/postgres/run-all.sh, wired into the Chainsaw postgres-2-backup-roundtrip suite): step 45 writes a before marker, captures the server timestamp, writes an after marker, restores to that timestamp and asserts the before row survived and the after row did not; step 46 submits a recoveryTime an hour ahead and asserts it fails fast with reason RecoveryTargetUnreachable. - Working, uncommented example (45-restorejob-pitr.yaml) and a docs section (docs/operations/backup-classes.md) covering the recoverable window, how to discover the earliest/latest restorable time from the backup catalog, the archive-lag tolerance, and idempotency under GitOps. Refs #2774 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
f6313e2 to
6a1e2d0
Compare
|
IvanHunters thanks for the review! Addressed the MINOR note (head |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Solid feature with the right overall shape (additive RBAC verb, safe upgrade path, non-PITR path byte-for-byte unchanged, helper unit tests added). Two things worth addressing before merge:
-
The core new logic isn't covered by a reconcile-level test. The deadline-exceeded PITR classification branch in
reconcileCNPGRestore(internal/backupcontroller/cnpgstrategy_controller.go:924-963) is only exercised through the pure helpers andrecoveryTargetUnreachabledirectly, never throughreconcileCNPGRestoreitself. Mutation check confirms the gap: swappingRecoveryTargetUnreachableback to the genericRestoreFailedat the call site keeps the whole suite green, so the call site can be silently broken. ThereadPodLogseam already exists; a reconcile-level test covering the three sub-paths (FATAL found / not found / RBAC Forbidden) would close it. -
The "fails fast" wording in the PR body and release note contradicts the code. Classification only runs after
time.Since(StartedAt) > deadline, and the code comment plus the newdocs/operations/backup-classes.mdsay so explicitly ("we do NOT try to fail earlier"). With the defaultrestoreTimeoutSecondsthe client still waits the full timeout; only the reason/message changes, not the timing. Since the release note ships to the public changelog, it's worth rewording to match the docs.
Review of #3383 found the RecoveryTargetUnreachable classification branch in reconcileCNPGRestore was only covered through the pure helpers and recoveryTargetUnreachable directly, never through reconcileCNPGRestore itself — a mutation swapping the call-site reason back to the generic RestoreFailed kept the suite green. Add TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable: it backdates StartedAt past the restore deadline with a recoveryTime set and drives the real reconcile through the readPodLog seam, asserting all three deadline sub-paths — FATAL present (reason RecoveryTargetUnreachable on both RecoveryConverged and Ready), FATAL absent (generic timeout, no RecoveryTargetUnreachable), and a Forbidden log read (generic timeout + the missing-pods/log-RBAC note). Verified it fails when the call-site reason is reverted to the generic RestoreFailed. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
|
Thanks — both addressed. 1. Reconcile-level coverage of the classification call site. Added 2. "Fails fast" wording. You're right — classification only runs after |
github-actions
Bot
removed
the
size/XL
github-actions
Bot
added
the
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/backupcontroller/cnpgstrategy_controller_test.go (1)
2369-2384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the
RecoveryClassificationForbiddenWarning event.The PR contract states a forbidden log read emits a
Warningevent with reasonRecoveryClassificationForbidden. This subcase verifies the status message note but never drainsrecord.NewFakeRecorder(10), so a regression that drops the event would go unnoticed. Draining the recorder'sEventschannel and asserting the reason would close that gap.🤖 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 `@internal/backupcontroller/cnpgstrategy_controller_test.go` around lines 2369 - 2384, The “Forbidden log read” test should also verify the required Warning event. In the subcase using newReconciler, retain and drain the fake recorder’s Events channel after reconcileToFailed, then assert that an emitted event has reason RecoveryClassificationForbidden.
🤖 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.
Nitpick comments:
In `@internal/backupcontroller/cnpgstrategy_controller_test.go`:
- Around line 2369-2384: The “Forbidden log read” test should also verify the
required Warning event. In the subcase using newReconciler, retain and drain the
fake recorder’s Events channel after reconcileToFailed, then assert that an
emitted event has reason RecoveryClassificationForbidden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 329db766-c289-4dd0-a709-08ef27180042
📒 Files selected for processing (1)
internal/backupcontroller/cnpgstrategy_controller_test.go
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The deadline-exceeded branch in reconcileCNPGRestore fires unconditionally before the cluster-health check, so a restore that has actually converged can be reported Failed/RecoveryTargetUnreachable instead of Succeeded whenever the reconcile that observes it lands late (e.g. a controller restart spanning the restore window) — reproduced with a driving go test against this exact code path.
Findings
[CRITICAL] internal/backupcontroller/cnpgstrategy_controller.go:924-925 vs :966-980 — deadline check precedes and fully bypasses the health check; a converged restore can be misreported Failed, and the existing purge-on-retry logic can then delete the already-recovered data.
reconcileCNPGRestore checks time.Since(restoreJob.Status.StartedAt.Time) > deadline at line 925 and, if true, unconditionally runs the new classification and returns via markRestoreJobFailedReason/markRestoreJobFailed (lines 926-963) — it never falls through to hasRecovery/cnpgClusterHealthy (lines 966-980) in that same call. So if the reconcile that first observes "deadline elapsed" happens after the target Cluster has already reached cnpgClusterHealthyPhase (e.g. the backupstrategy-controller pod was down or its workqueue stalled for longer than restoreTimeoutSeconds while the recovery quietly finished converging), the RestoreJob is marked Failed even though the underlying Postgres cluster is healthy and serving.
I reproduced this with a driven unit test built on the PR's own test harness (TestReconcileCNPGRestore_HealthyClusterSucceeds / TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable patterns): StartedAt 72h in the past (past the 30m default deadline), the recovery Cluster.Status.Phase = "Cluster in healthy state", one leftover recovery pod whose log still carries the transient FATAL the PR's own body says is normal for a converging near-now target ("a valid near-now target hits the same FATAL transiently ... before it converges", and dev7 observed multiple simultaneous failed recovery pods). Result: Phase="Failed", Ready reason RecoveryTargetUnreachable, message telling the tenant to "pick a recoveryTime inside the recoverable window" — a confident, specific, and wrong diagnosis for a restore that in fact succeeded. (Test was run against this checkout, then removed; not left in the tree.)
This directly contradicts the PR's own design claim ("Health wins unconditionally ... a recovery that converges within the window reaches success and is never classified as failed") — that statement holds only while reconciliation keeps up in real time against the 5s cnpgPollInterval; it does not hold across a reconcile gap that itself exceeds the deadline, which the PR's own "Idempotency under GitOps" docs section explicitly anticipates as a supported case ("a controller restart mid-restore therefore re-attaches to the recovering cluster rather than deleting it and starting over" — true for avoiding a double-purge of the same RestoreJob, but it does not make the health-vs-deadline race safe).
Escalation to data loss: if an operator or automation reacts to the false Failed by resubmitting the restore, the new RestoreJob's purge guard (purgedCondition/freshlyRecovered at lines 786-793, cnpgPurgeNeeded) sees a recovery Cluster created before the new RestoreJob's StartedAt and treats it as "a recovery Cluster left over from an earlier completed restore" that "must be purged" — deleting the already-successfully-restored Cluster and its PVCs to start over.
The ordering itself (deadline check before health check) predates this PR, but this PR is the one that gives that pre-existing race a confident, specific, wrong reason (RecoveryTargetUnreachable) instead of the old generic timeout message, and it is precisely the code this PR extends with the classification feature. Fix: check hasRecovery/cnpgClusterHealthy before (or as a short-circuit inside) the deadline branch, so an already-converged Cluster always reports Succeeded regardless of how late the reconcile discovers it, and add the missing test corner (StartedAt far past deadline and Cluster healthy) as a regression test — none of the new tests cover this combination; all three deadline-branch sub-tests use a Cluster with Status.Phase left empty/unhealthy.
[MINOR] internal/backupcontroller/cnpgstrategy_controller.go:1485 — defer stream.Close() on the new pod-log reader ignores the Close error (golangci-lint errcheck). Low impact (read-only log stream, no cleanup depends on the error), but it's new code from this PR.
[MINOR] packages/system/backupstrategy-controller/tests/ — no helm-unittest assertion covers the new pods/log ClusterRole rule (packages/system/backupstrategy-controller/templates/rbac.yaml:29-35); the chart's test suite has zero RBAC-rule assertions at all, so this doesn't regress existing coverage, but Phase 5d's RBAC-change requirement ("an assertion on the resulting access surface") is unmet.
Claim mismatches
[PARTIAL] "Health wins unconditionally ... a recovery that converges within the window reaches success and is never classified as failed" — true only while the deadline branch is never entered before the healthy branch runs at least once; false across a reconcile gap that itself exceeds the deadline (reproduced, see Findings).
[PARTIAL] Release note: "the failure is now reported with the precise reason RecoveryTargetUnreachable ... instead of a generic timeout" — the added precision can be confidently wrong in the race above, which is a regression in diagnostic trustworthiness relative to the old generic-timeout message, not a strict improvement in every case.
[UNVERIFIABLE] "dev7: backup + WAL archiving + recovery convergence verified live" and "7 simultaneous failed recovery pods" — I have no access to the dev7 cluster to confirm; not blocking on its own since the negative path is separately covered by the CI Chainsaw e2e (step 46), which I confirmed is wired into hack/e2e-chainsaw/postgres/chainsaw-test.yaml with the budget raised 25m to 40m.
Operational risks
- A rolling upgrade of
backupstrategy-controller(including the upgrade that ships this very PR) restarts the controller pod. If that restart lands near the tail end of a live, already-converging PITR restore whoserestoreTimeoutSecondshas also elapsed by the time the pod comes back, the first post-restart reconcile can flip an already-successful restore toFailed/RecoveryTargetUnreachable. A shortrestoreTimeoutSeconds(which the PR's own docs recommend setting for "faster rejection") makes this materially more likely to be hit by ordinary restart jitter, not just extended outages.
Caveats
spec.options.recoveryTimehas no format validation at ingestion (parseCNPGRestoreOptions,internal/backupcontroller/cnpgstrategy_controller.go:1728-1737, only checks JSON well-formedness). A malformed timestamp (typo, non-RFC3339 string) flows straight through to CNPG'sbootstrap.recovery.recoveryTarget.targetTimeand burns the full restore deadline before failing with a generic reason instead of being rejected immediately with a clear message. This predates this PR (therecoveryTimeplumbing itself is from #3313, untouched by this diff), so it is not counted against this PR, but it sits squarely in the feature this PR is hardening.- The new
pods/logClusterRole rule (packages/system/backupstrategy-controller/templates/rbac.yaml:29-35) is cluster-wide with noresourceNamesscoping, so this single system controller can now read the log of any pod in any tenant namespace, not just its own recovery pods (Kubernetes RBAC cannot scopegetonpods/logto a dynamic pod name set). This is consistent with the controller's existing already-broad cluster-wide grants (podsget/list/watch,apps.cozystack.io: "*", Postgres patch/update, VM update, etc.), so it is not a new category of risk, but it does widen what this one controller can read (pod logs can incidentally contain leaked secrets). Not blocking. - Grepped for the pre-existing "deadline check precedes health check" pattern in sibling strategy controllers (
etcdstrategy_controller.go:858,foundationdbstrategy_controller.go:923,970,mariadbstrategy_controller.go:541,592) — same shape appears to recur, but these files are untouched by this PR and I did not verify each one has the same consequence; flagged only as a follow-up, not as a finding against this PR. chart_lintis clean (0 render/kubeconform errors) andshell_lintonly returned two info-level "not following sourced file" notices from shellcheck onexamples/backups/postgres/{cleanup.sh,run-all.sh}— not actionable.- Grepped the repo for other consumers hardcoding the literal
"RestoreFailed"reason string outsideinternal/backupcontroller; found none, so the new reason granularity does not appear to break any other in-tree consumer. go build/go vetclean oninternal/backupcontroller/...; the full PITR-related unit test suite (TestReconcileCNPGRestore_*,TestRecoveryTargetUnreachable_*,TestRecoveryUnreachableFromLogs,TestRecoveryPodsToInspect,TestLogIndicatesRecoveryTargetUnreachable) passes.
Recommended follow-ups
- Reorder
reconcileCNPGRestoresocnpgClusterHealthy(andhasRecovery) is checked before the deadline-exceeded branch is allowed to mark the RestoreJob Failed, and add the missing regression test (StartedAtfar past deadline and Cluster already healthy). - Add
time.Parse(time.RFC3339, ...)validation forspec.options.recoveryTimeat parse time inparseCNPGRestoreOptions, surfaced as a specificReady=Falsereason rather than a silent 30-minute wait followed by a generic failure. - Add a helm-unittest asserting the new
pods/logrule on thebackupstrategy-controllerClusterRole. - Audit the sibling strategy controllers (etcd/foundationdb/mariadb) for the same deadline-vs-health ordering.
- The PR body already tracks an unopened
cozystack/websitedocs-mirror follow-up for the new PITR section; no action needed here beyond what's already noted.
Addresses review CHANGES_REQUESTED on #3383. CRITICAL: reconcileCNPGRestore checked the restore deadline BEFORE the cluster-health check, so a reconcile that first observes "deadline elapsed" AFTER the recovery Cluster already reached a healthy state (a controller restart or stalled workqueue spanning restoreTimeoutSeconds) would mark an already-converged restore Failed / RecoveryTargetUnreachable. A false Failed can then escalate to data loss: a resubmitted RestoreJob's purge guard treats the recovered Cluster as a stale leftover and deletes it + its PVCs. Fix: check hasRecovery + cnpgClusterHealthy first and return Succeeded unconditionally when healthy, regardless of how late the reconcile runs; only a not-yet-healthy restore reaches the deadline/classification branch. Adds TestReconcileCNPGRestore_HealthyPastDeadlineSucceeds (StartedAt 72h past the deadline + healthy Cluster + recoveryTime set; the injected readPodLog fails the test if the classification path is entered, proving health short-circuits). MINOR: handle the deferred Close error on the pod-log reader (errcheck). Signed-off-by: Andrey Kolkov <[email protected]>
|
IvanHunters thanks — the CRITICAL is a real bug, fixed in [CRITICAL] health-vs-deadline ordering — reordered [MINOR] [MINOR] helm-unittest for the RFC3339 validation of Sibling controllers (etcd/fdb/mariadb) same ordering — agree it's worth auditing; tracking as a separate follow-up since those files are untouched here and each needs its own repro to confirm the same consequence. Fresh CI is running. |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
The change is a fail-safe diagnostic layer over an existing deadline-bounded restore path: it adds a precise RecoveryTargetUnreachable failure reason, a pods/log RBAC grant, and reorders the health check before the deadline check; build/vet/tests pass and the reconcile-level regression test is non-vacuous (verified by mutation). Two load-bearing assumptions couple to CNPG/PostgreSQL internals that cannot be verified in a hermetic static review, but both degrade gracefully rather than breaking, so they are Caveats, not blockers.
Findings
None blocking.
Claim mismatches
None. Spot-checks of the load-bearing self-claims held up:
- "Reconcile-level test fails if the call-site reason is reverted to the generic one" — [OK], confirmed by mutation: reverting
internal/backupcontroller/cnpgstrategy_controller.go:1002(markRestoreJobFailedReason(..., "RecoveryTargetUnreachable", msg)→markRestoreJobFailed(..., msg)) turnsTestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable/recovery-target-unreachable_FATAL...RED. - "go build / go vet clean; unit tests pass" — [OK], reproduced locally.
- "adds the pods/log RBAC grant" — [OK],
packages/system/backupstrategy-controller/templates/rbac.yaml:34-36grantspods/log: [get], a scoped additive grant (no wildcard). - "Health wins unconditionally, checked before the deadline" — [OK],
cnpgstrategy_controller.go:921health block precedes the deadline block at:939; guarded byTestReconcileCNPGRestore_HealthyPastDeadlineSucceeds(injectedreadPodLogt.Fatalf if the classification path is entered for a healthy cluster).
Caveats
- Upgrade path (Phase 5b.A): the only chart change is an additive
pods/loggrant on thebackups.cozystack.io:strategy-controllerClusterRole (rbac.yaml:34-36). No values.yaml / values.schema.json / Chart.yaml / CRD change, so nomake generateartifacts and no numbered migration are required, and none were added (correct). RBAC is strictly widened, never contracted — no running tenant loses access. During the brief upgrade window where the new controller image lands before the ClusterRole reconciles,GetLogsreturns Forbidden, and the code handles that explicitly (cnpgstrategy_controller.goForbidden branch → Warning Event + note appended to the failure message, restore still bounded by the deadline). Verified by reading the diff; not exercised on a live upgrade. - Fresh install (Phase 5b.B):
Clientsetis wired inrestorejob_controller.goSetupWithManagerviakubernetes.NewForConfig(cfg), and all call sites are nil-safe (recoveryTargetUnreachablereturns false whenClientset == nil && readPodLog == nil). chart_lint reported 0 render/kubeconform errors. Verified statically. - Upstream-string coupling (CNPG/PostgreSQL internals): the classification depends on (a) the recovery container being named
full-recovery(cnpgRecoveryContainerName,cnpgstrategy_controller.go:129) and (b) the exact FATAL substring"recovery ended before configured recovery target was reached"(:143). Both are documented as verified against CNPG 1.28.1 / postgresql:18.1 on dev7, but I could not verify them hermetically. Failure mode if either drifts on a future CNPG/PG bump is fail-safe, not breakage:recoveryPodsToInspectreturns empty / the FATAL match misses →unreachable=false→ the restore still fails at the deadline with the generic timeout reason (no hang-forever, no data loss). The negative e2e (step 46) is the drift detector, as the code comment states. - Phase 5c (config-combination matrix): not applicable — no
values.yamltoggles and no{{- if/with }}on values were introduced;rbac.yamlis a static template. - Live negative-path verification: the PR body discloses that a fully-green live step-46 run was blocked by COSI bucket-provisioning flakiness on dev7, so the
RecoveryTargetUnreachableclassification was proven only via the pure/reconcile unit tests plus the CI Chainsaw e2e, not a live dev cluster. Accepted as disclosed; the CI Chainsaw suite (hack/e2e-chainsaw/postgres/chainsaw-test.yaml, budget raised 25m→40m) is the standing guard.
Recommended follow-ups
- Live end-to-end validation of step 45/46 on a disposable dev cluster (via the separate
cozystack-pr-testskill) would close the one surface this static review could not exercise: that CNPG 1.28.1 actually names the containerfull-recoveryand emits the matched FATAL, and that the negative case reportsRecoveryTargetUnreachablewithin the 300srestoreTimeoutSeconds. Out of scope for a hermetic review (needs a live cluster + object storage). - PITR docs mirror to cozystack/website (already tracked by the author as a follow-up alongside #3313's backup-classes changes).
Mirrors the new **Point-in-time recovery (PostgreSQL)** section from the monorepo `docs/operations/backup-classes.md` into the website copies (`next` and `v1.6`). Follows up cozystack/cozystack#3383 (PITR support for the `backups.cozystack.io` RestoreJob API, fixes cozystack/cozystack#2774), which added the section to the monorepo docs that this site mirrors. #3313's earlier `backup-classes.md` mirror landed in #622; this is the PITR delta on top of it. The section covers: - setting `spec.options.recoveryTime` (RFC3339) on a `RestoreJob`, with a worked example; - the recoverable window (earliest = oldest base backup, latest = last archived WAL); - discovering the earliest/latest restorable time from the backup catalog (since `firstRecoverabilityPoint` is not reliably populated under the barman-cloud plugin); - the deadline-bounded failure behavior and the precise `RecoveryTargetUnreachable` reason for a target past the archive; - idempotency under GitOps. Added to `next` and `v1.6` (the release line that already carries the barman-cloud plugin backup docs). Not added to `v1.5` — PITR is a new feature on `main`, not part of 1.5. If 1.6 has already been cut without this feature, drop the `v1.6` file. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added guidance for PostgreSQL point-in-time recovery using `RestoreJob`. * Documented recovery-time configuration, valid recovery windows, and example manifests. * Explained how to identify the earliest and latest restorable timestamps. * Clarified failure behavior for unavailable or out-of-range recovery targets. * Documented idempotent restore behavior during GitOps reconciliation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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 <[email protected]>
2 participants
What this PR does
Makes PostgreSQL point-in-time recovery (PITR) a genuinely supported, tested capability for the new
backups.cozystack.ioAPI. Fixes #2774.Was stacked on #3313 (
fix/3300-cnpg-barman-plugin) for therecoveryTime→ CNPGbootstrap.recovery.recoveryTarget.targetTimeplumbing; #3313 has since merged and this branch is rebased onmain. This PR adds the reliability + verification layer #2774 asked for.Changes
spec.options.restoreTimeoutSeconds, default 30m) stays the sole authority for "this restore is stuck" — the client waits the full window either way. What changes is the diagnosis: once the deadline has elapsed, ifrecoveryTimewas set and the recovery Cluster never went healthy, the CNPG driver reads thefull-recoverypod logs and — when several distinct attempts exited with PostgreSQL'sFATAL: recovery ended before configured recovery target was reached— fails the RestoreJob withRecoveryConverged=False/Ready=False(reasonRecoveryTargetUnreachable) and an actionable message, instead of a bare generic timeout. Timing is unchanged; only the reason/message differ. (For a faster rejection, set a shorterrestoreTimeoutSeconds.) Design points:pods/logRBAC grant the log read needs (distinct subresource frompods); aForbiddenis logged loudly and surfaced in the failure message rather than silently disabling the classification.examples/backups/postgres/run-all.sh, wired into the Chainsawpostgres-2-backup-roundtripsuite):before, capture server timestamp (µs precision), writeafter, restore to that timestamp, assertbeforepresent andafterabsent.recoveryTimean hour ahead must fail with reasonRecoveryTargetUnreachableonce the deadline elapses (asserts the reason, so it proves the classification fired, not just that a timeout occurred).45-restorejob-pitr.yaml; adocs/operations/backup-classes.mdPITR section (recoverable window, discovering earliest/latest restorable time from the backup catalog sincefirstRecoverabilityPointis unreliable under the plugin, archive-lag tolerance, idempotency under GitOps).Verification
go build/go vetclean; unit tests pass (logIndicatesRecoveryTargetUnreachable,recoveryUnreachableFromLogs,recoveryPodsToInspect, nil-Clientset guard, a reconcile-level success-path test assertingSucceeded+RecoveryConverged=True, and — added per review —TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable, a reconcile-level test driving the deadline branch through all three sub-paths: FATAL found → reasonRecoveryTargetUnreachable, FATAL absent → generic timeout,Forbiddenlog read → generic timeout + missing-pods/log-RBAC note. It fails if the call-site reason is reverted to the generic one).full-recoverypods rather than reusing one — was confirmed (observed 7 simultaneous failed recovery pods). A fully-green live step-46 run was blocked by intermittent COSI bucket-provisioning flakiness on dev7; the negative path is covered by the CI Chainsaw e2e.Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff. The only user-facing surface is the newdocs/operations/backup-classes.mdPITR section; the website keeps a hand-mirrored copy per docs version.Summary by CodeRabbit
recoveryTime.