## What this PR does
Makes PostgreSQL point-in-time recovery (PITR) a genuinely supported,
tested capability for the new `backups.cozystack.io` API. Fixes#2774.
Was **stacked on #3313** (`fix/3300-cnpg-barman-plugin`) for the
`recoveryTime` → CNPG `bootstrap.recovery.recoveryTarget.targetTime`
plumbing; #3313 has since merged and this branch is rebased on `main`.
This PR adds the reliability + verification layer #2774 asked for.
### Changes
- **Precise failure reason for an unreachable target.** The restore
deadline (`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, if `recoveryTime` was set and the recovery Cluster never
went healthy, the CNPG driver reads the `full-recovery` pod logs and —
when several distinct attempts exited with PostgreSQL's `FATAL: recovery
ended before configured recovery target was reached` — fails the
RestoreJob with `RecoveryConverged=False` / `Ready=False` (reason
`RecoveryTargetUnreachable`) and an actionable message, instead of a
bare generic timeout. Timing is unchanged; only the reason/message
differ. (For a faster rejection, set a shorter `restoreTimeoutSeconds`.)
Design points:
- **We deliberately do NOT trip early off the FATAL.** A valid near-now
target emits that same FATAL transiently — repeatedly, for many
attempts, on a slow archiver — before it converges, so any earlier trip
would wrongly reject a recoverable restore (observed twice in CI). The
FATAL is used only to *explain* a failure the deadline has already
declared.
- **Health wins unconditionally** — the deadline/classification check
precedes the healthy check, but a recovery that converges within the
window reaches success and is never classified as failed (a live dev7
restore did exactly this).
- **Counting distinct FATAL-ing attempts** (not pod failures) tolerates
a one-off archive-lag miss on a near-now target and ignores transient
recovery-pod crashes (a node blip / brief API-server unreachability
never emits that FATAL).
- Adds the `pods/log` RBAC grant the log read needs (distinct
subresource from `pods`); a `Forbidden` is logged loudly and surfaced in
the failure message rather than silently disabling the classification.
- **e2e** (`examples/backups/postgres/run-all.sh`, wired into the
Chainsaw `postgres-2-backup-roundtrip` suite):
- Step 45 — write `before`, capture server timestamp (µs precision),
write `after`, restore to that timestamp, assert `before` present and
`after` absent.
- Step 46 — a `recoveryTime` an hour ahead must fail with reason
`RecoveryTargetUnreachable` once the deadline elapses (asserts the
*reason*, so it proves the classification fired, not just that a timeout
occurred).
- Chainsaw budget raised 25m → 40m to cover three sequential restores +
the post-deadline classification.
- **Docs + example**: uncommented `45-restorejob-pitr.yaml`; a
`docs/operations/backup-classes.md` PITR section (recoverable window,
discovering earliest/latest restorable time from the backup catalog
since `firstRecoverabilityPoint` is unreliable under the plugin,
archive-lag tolerance, idempotency under GitOps).
### Verification
- `go build` / `go vet` clean; unit tests pass
(`logIndicatesRecoveryTargetUnreachable`, `recoveryUnreachableFromLogs`,
`recoveryPodsToInspect`, nil-Clientset guard, a reconcile-level
success-path test asserting `Succeeded` + `RecoveryConverged=True`, and
— added per review —
`TestReconcileCNPGRestore_DeadlineClassifiesRecoveryTargetUnreachable`,
a reconcile-level test driving the deadline branch through all three
sub-paths: FATAL found → reason `RecoveryTargetUnreachable`, FATAL
absent → generic timeout, `Forbidden` log read → generic timeout +
missing-`pods/log`-RBAC note. It fails if the call-site reason is
reverted to the generic one).
- Reviewed via adversarial branch-review; fixes include a critical
missing-RBAC dead-on-arrival, a classification false-positive on
transient recovery-pod crashes, and a sub-second e2e flake.
- **dev7**: backup + WAL archiving + recovery convergence verified live
on the #3313 build. The load-bearing assumption of the classification —
CNPG **accumulates** failed `full-recovery` pods 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.md` against the
diff. The only user-facing surface is the new
`docs/operations/backup-classes.md` PITR section; the website keeps a
hand-mirrored copy per docs version.
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up: PITR docs mirror warranted (not yet opened; same pending
mirror as #3313's backup-classes changes)
- [x] No other downstream repository is affected by this change
```release-note
feat(backups): PostgreSQL RestoreJob now supports point-in-time recovery (PITR). Set spec.options.recoveryTime (RFC3339) to restore to an exact instant. A recoveryTime past the latest archived WAL still runs to the restore deadline (spec.options.restoreTimeoutSeconds, default 30m), but the failure is now reported with the precise reason RecoveryTargetUnreachable and an actionable message instead of a generic timeout; set a shorter restoreTimeoutSeconds for a faster rejection.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added PostgreSQL point-in-time recovery (PITR) support via RFC3339
`recoveryTime`.
* Introduced a PITR RestoreJob example, plus helper variables and an
unreachable-target scenario.
* **Bug Fixes**
* Improved restore deadline handling with deterministic, log-based
fail-fast classification for unreachable recovery targets, including
clearer conditions.
* **Documentation**
* Added detailed PITR operational documentation (recovery windows,
failure behavior, and GitOps/idempotent behavior).
* Expanded README and example comments for PITR verification and failure
expectations.
* **Tests**
* Extended unit tests and end-to-end chainsaw workflow to cover PITR
success and unreachable failures.
* **Chores**
* Updated RBAC to allow pod log reads; refined cleanup and run scripts
for additional RestoreJob artifacts.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Address review on the S3 request-checksum fix:
- Extend the fix to the platform-managed useSystemBucket=true flow, whose
ObjectStore is built in Go by applyClusterPluginBackup (SSA-applied), not
chart-rendered. Add InstanceSidecarConfiguration to cnpgtypes.ObjectStoreSpec
and set AWS_REQUEST_CHECKSUM_CALCULATION=when_required on it via
barmanSidecarConfiguration(). The platform's own default system bucket is
SeaweedFS, a non-AWS S3 gateway of the same class as Ceph RGW. Unit test in
cnpgstrategy_controller_test.go asserts the applied ObjectStore carries it.
- Factor the duplicated instanceSidecarConfiguration block (3x across two
charts) into a shared cozy-lib helper (cozy-lib.barman.checksumSidecarConfiguration).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
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 <androndo@gmail.com>
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) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
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) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Two defects of the same class — the WAL-archive S3 prefix (serverName)
changing or defaulting differently between components:
Recovery default regression (review blocker): native CNPG defaulted an
absent externalClusters serverName to the entry name (= bootstrap.oldName),
but the barman-cloud plugin defaults it to the NEW cluster's name, so a
legacy-flow restore with bootstrap.serverName unset would read the wrong
S3 prefix (hard failure, or silently restoring another cluster's data).
The chart now always emits serverName, defaulting to bootstrap.oldName.
Live-cluster serverName flip (found by the gated postgres backup e2e on a
real cluster): the chart renders spec.plugins without serverName
(effective prefix = cluster name) while a strategy template may name a
different one; the driver's ForceOwnership patch then flipped the prefix
mid-stream, splitting the WAL archive across two prefixes — the WAL
holding the backup's start was lost between them and the restore failed
with "WAL not found". The driver now preserves the live Cluster's
effective serverName (explicit parameter, or the cluster name when the
plugin is attached without one) and only applies the strategy's value to
a Cluster with no plugin attached; both charts pin serverName explicitly,
and the example strategy no longer sets one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Review follow-up (cozystack#3300): ensureCNPGBackup stamps spec.method=plugin +
spec.pluginConfiguration on the postgresql.cnpg.io/Backup it creates — the field
that routes a driver-initiated run through the barman-cloud plugin. If it
regressed to the default (barmanObjectStore) method, CNPG would run the legacy
method against a Cluster that no longer has one and every platform backup would
fail. Add TestEnsureCNPGBackup_UsesPluginMethod asserting method, plugin name,
and cluster ref. (The chart ScheduledBackup path was already covered; the driver
path was not.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Review follow-ups (cozystack#3300):
- Lifecycle/leak: applyClusterPluginBackup now sets an ownerReference from the
driver-created barmancloud.cnpg.io/ObjectStore to its cnpg.io/Cluster, so
Kubernetes garbage-collects it when the Cluster is deleted (tenant app
teardown or the RestoreJob purge). In the platform useSystemBucket flow the
chart does not render this ObjectStore, so previously it was orphaned in the
tenant namespace. Drop the now-accurate-only 'delete' verb on objectstores
from the driver RBAC (GC handles removal; the driver never deletes it) and
correct the RBAC comment that falsely claimed the purge deletes it.
- Diagnostics/comments: the RestoreJob WAL-archive-deadline failure message and
several doc comments still told operators to check spec.backup.barmanObjectStore
/ externalClusters[].barmanObjectStore, fields no longer emitted after the
migration. Point them at the barman-cloud plugin (spec.plugins + ObjectStore,
externalClusters[].plugin) instead so they don't mislead during an incident.
- Test: TestApplyClusterPluginBackup_PatchesExistingCluster now asserts the
ObjectStore carries an ownerReference to the Cluster (kind/name/uid).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Address review feedback on the repeat in-place restore fix:
- Make the freshness discriminator strict (creationTimestamp > StartedAt) so
an exact tie resolves to "not fresh" and the caller purges, matching the
function's stated conservative default; a fresh Cluster is always created
well after StartedAt so this never misclassifies the status-write-race path.
- Document why a wall-clock timestamp comparison was chosen over recording the
purged Cluster's UID on the TargetPurged condition (the UID approach leans on
the same status write the race path assumes can fail).
- Add a reconcile-level regression test that drives reconcileCNPGRestore end to
end with a fake client: a stale leftover recovery Cluster from a prior restore
is purged, and a Cluster this restore just re-created is not re-purged. This
covers the actual call site where #3311 lived, not just the pure helpers.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
A repeat in-place PostgreSQL RestoreJob into an already-restored target
silently no-opped: the destructive purge was skipped because the live
Cluster still carried spec.bootstrap.recovery left over from the previous
restore, and the re-purge guard mistook that for a freshly-recovered
Cluster produced by this restore's own purge. The job then reported
Succeeded while the PVC, disk, and data were never touched.
Tell the two apart by the Cluster's creationTimestamp relative to the
RestoreJob's StartedAt: only a recovery Cluster created at/after StartedAt
is treated as this restore's own and skipped; one that predates StartedAt
is leftover from an earlier completed restore and is purged so the restore
re-bootstraps from the backup.
Signed-off-by: Ivan Okhotnikov <xorokhotnikov@gmail.com>
Andrey Kolkov