Skip to content

Commit 0bf93fc

Browse files
committed
fix(backups): skip the CNPG restore purge only for this restore's Cluster
The in-place CNPG restore skips deleting the live Cluster when it carries bootstrap.recovery and was created after the RestoreJob started, taking that as "this restore's own purge and re-render produced it". The timestamp does not say which restore rendered the Cluster. A Cluster deleted while the restore waits for the WAL archive is re-rendered by the still-active HelmRelease from the previous restore's values and is created after StartedAt too; the restore then skipped its purge and reported Succeeded on the previous restore's data. The restore patch already sets bootstrap.newServerName to restoredServerName(cluster, RestoreJob UID). The chart now annotates the Cluster with it as postgres.cozystack.io/restored-server-name, whether or not it renders the barman plugin that also carries the value, and the purge is skipped only when the annotation names this RestoreJob. The mark lives on the rendered Cluster rather than in RestoreJob status, so it still covers the failed status write the skip exists for. A Cluster rendered by a chart that predates the annotation keeps the creation-time check. Fixes #3322 Assisted-by: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 54dfe39 commit 0bf93fc

5 files changed

Lines changed: 293 additions & 98 deletions

File tree

‎docs/operations/backup-classes.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,8 @@ spec:
375375
name: orders-db
376376
```
377377

378+
An application takes one restore at a time. A RestoreJob that targets an application another unfinished RestoreJob is already restoring fails with reason `ConcurrentRestore` before it changes anything, and its message names the job that goes first. It is not queued: once that job has finished, create a new RestoreJob if the restore is still wanted. A restore that has started always goes first. Between two that have not, the older one runs; creation times have one-second precision, so of two created in the same second, the one whose name sorts first runs.
379+
378380
## Point-in-time recovery (PostgreSQL)
379381

380382
A `RestoreJob` restores a `Postgres` application from a `Backup`. Omit `spec.options.recoveryTime` to recover to the latest point in the WAL archive; set it (RFC3339) to recover the database to an exact instant — a point-in-time recovery (PITR). The CNPG barman-cloud plugin restores a base backup and replays archived WAL from it: up to `recoveryTime` when one is set, so the restored cluster reflects the database as of that instant and later writes are absent, and to the end of the archive otherwise.
@@ -441,7 +443,7 @@ Recovery follows `recovery_target_timeline: latest`, which CNPG leaves as Postgr
441443

442444
### Idempotency under GitOps
443445

444-
An in-progress restore is safe to reconcile. The driver purges the target `Cluster` + PVCs exactly once per RestoreJob (guarded by the `TargetPurged` condition and a freshly-recovered check), suspends the target's HelmRelease across the purge so Flux cannot race the bootstrap swap, and resumes it once the recovery cluster is rendered. A Flux reconcile (or a controller restart) mid-restore therefore re-attaches to the recovering cluster rather than deleting it and starting over.
446+
An in-progress restore is safe to reconcile. The driver purges the target `Cluster` + PVCs exactly once per RestoreJob (guarded by the `TargetPurged` condition and by the `postgres.cozystack.io/restored-server-name` annotation, which identifies the RestoreJob whose purge produced the live recovery `Cluster`), suspends the target's HelmRelease across the purge so Flux cannot race the bootstrap swap, and resumes it once the recovery cluster is rendered. A Flux reconcile (or a controller restart) mid-restore therefore re-attaches to the recovering cluster rather than deleting it and starting over.
445447

446448
## Kafka: topic metadata only
447449

‎internal/backupcontroller/cnpgstrategy_controller.go‎

Lines changed: 59 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ const (
6767
cnpgClusterNameKey = "cnpg.io/cluster-name"
6868
cnpgS3SecretRefKey = "cnpg.io/s3-secret-ref"
6969

70+
// Annotation the postgres chart renders on the Cluster from
71+
// bootstrap.newServerName.
72+
cnpgRestoredServerNameAnnotation = "postgres.cozystack.io/restored-server-name"
73+
7074
// Polling cadence for the CNPG backup/restore lifecycle. Mirrors the
7175
// Velero strategy's defaults so behaviour is uniform across drivers.
7276
cnpgPollInterval = 5 * time.Second
@@ -420,40 +424,41 @@ func cnpgPurgeNeeded(purgedCondition, liveClusterFreshlyRecovered bool) bool {
420424

421425
// cnpgClusterFreshlyRecovered reports whether a live cnpg.io Cluster that
422426
// carries spec.bootstrap.recovery was produced by THIS RestoreJob's own purge
423-
// + chart re-render (its creationTimestamp is strictly after the job's
424-
// StartedAt) rather than being left over from an earlier, already-completed
425-
// restore.
427+
// + chart re-render rather than rendered for some other restore.
426428
//
427-
// Only the former is safe to skip re-purging. A leftover recovery Cluster from
428-
// a prior restore predates StartedAt and still holds the prior data, so it must
429-
// be purged for the new restore to re-bootstrap from the backup. The fresh
430-
// Cluster in the status-write-race case is always created after StartedAt (the
431-
// job sets StartedAt on its first reconcile, long before it purges and the
432-
// chart re-renders), so the timestamp comparison cleanly separates the two.
429+
// Only the former is safe to skip re-purging. Any other recovery Cluster holds
430+
// another restore's data and must be purged for this restore to re-bootstrap
431+
// from its backup. That includes a Cluster a concurrent RestoreJob produced,
432+
// and one the chart re-renders from an earlier restore's values after being
433+
// deleted while this restore waits for the WAL archive: both are created after
434+
// this job's StartedAt, so a timestamp alone cannot tell them apart.
433435
//
434-
// The comparison is strict (created > started), so an exact tie resolves to
435-
// "not fresh" and the caller purges. That matches the conservative default
436-
// below: the only classification that must never be wrong is calling a stale
437-
// leftover "fresh" (which reintroduces the silent no-op), and a fresh Cluster
438-
// is always created well after StartedAt (see above), never exactly at it.
436+
// The chart annotates the Cluster with bootstrap.newServerName, which the
437+
// restore patch sets to restoredServerName(cluster, RestoreJob UID). The value
438+
// lives on the object the chart renders, not in RestoreJob status, so it
439+
// survives the failed status write the skip exists for, and it names the
440+
// restore that rendered the Cluster. When the annotation is present, the
441+
// Cluster is fresh only if it carries wantMark.
439442
//
440-
// An identity-based alternative was considered - recording the purged Cluster's
441-
// UID on the TargetPurged condition and comparing UIDs on later reconciles -
442-
// which is immune to clock skew. It was rejected because it leans on the same
443-
// status write that the status-write-race path (the whole reason this skip
444-
// exists) assumes can fail, so it cannot cover that case; the timestamp
445-
// comparison needs no extra persisted state and the fresh-vs-stale gap
446-
// (a full purge + re-render cycle) always dwarfs any plausible control-plane
447-
// clock skew.
443+
// A Cluster rendered by a chart that predates the annotation has none; it then
444+
// falls back to the creation-time comparison (created strictly after
445+
// StartedAt), which still separates a Cluster from an earlier completed restore
446+
// (created before StartedAt) from this restore's own re-render.
448447
//
449448
// Returns false when freshness cannot be determined (no recovery bootstrap, or
450-
// a missing timestamp): the caller then proceeds to purge, which is the safe
451-
// default for any pre-existing, non-fresh Cluster.
452-
func cnpgClusterFreshlyRecovered(hasRecovery bool, clusterCreatedAt, restoreStartedAt *metav1.Time) bool {
453-
if !hasRecovery || clusterCreatedAt == nil || restoreStartedAt == nil {
449+
// a missing timestamp on the fallback path): the caller then purges, which is
450+
// the safe default.
451+
func cnpgClusterFreshlyRecovered(state recoveryClusterState, wantMark string, restoreStartedAt *metav1.Time) bool {
452+
if !state.hasRecovery {
453+
return false
454+
}
455+
if state.restoreMark != "" {
456+
return state.restoreMark == wantMark
457+
}
458+
if state.createdAt == nil || restoreStartedAt == nil {
454459
return false
455460
}
456-
return clusterCreatedAt.After(restoreStartedAt.Time)
461+
return state.createdAt.After(restoreStartedAt.Time)
457462
}
458463

459464
// cnpgClusterGVR addresses Clusters on the dynamic client, for reads that must
@@ -821,17 +826,16 @@ func (r *RestoreJobReconciler) reconcileCNPGRestore(ctx context.Context, restore
821826
// check the live Cluster for bootstrap.recovery: if present, the chart
822827
// has already re-rendered after a previous purge, and we must NOT delete
823828
// it again.
824-
hasRecovery, clusterCreatedAt, err := r.recoveryBootstrapClusterState(ctx, target.Namespace, clusterName)
829+
clusterState, err := r.recoveryBootstrapClusterState(ctx, target.Namespace, clusterName)
825830
if err != nil {
826831
return ctrl.Result{}, err
827832
}
828833
purgedCondition := apimeta.IsStatusConditionTrue(restoreJob.Status.Conditions, restoreCondTargetPurged)
829834
// Only skip the destructive purge for a recovery Cluster THIS restore just
830-
// produced (created at/after StartedAt). A recovery Cluster left over from
831-
// an earlier completed restore predates StartedAt and still holds the old
832-
// data, so it must be purged - otherwise a repeat in-place restore silently
833-
// no-ops.
834-
freshlyRecovered := cnpgClusterFreshlyRecovered(hasRecovery, clusterCreatedAt, restoreJob.Status.StartedAt)
835+
// produced. A recovery Cluster rendered for any other restore still holds
836+
// that restore's data, so it must be purged - otherwise a repeat in-place
837+
// restore silently no-ops.
838+
freshlyRecovered := cnpgClusterFreshlyRecovered(clusterState, restoredServerName(clusterName, restoreJob.UID), restoreJob.Status.StartedAt)
835839
if cnpgPurgeNeeded(purgedCondition, freshlyRecovered) {
836840
// Gate the destructive flow on the source cluster having shipped
837841
// the backup's required WALs to object storage. archive_command runs
@@ -983,7 +987,7 @@ func (r *RestoreJobReconciler) reconcileCNPGRestore(ctx context.Context, restore
983987
// Getting the order wrong is not just a cosmetic mislabel - a false Failed
984988
// can be resubmitted, and the next RestoreJob's purge guard would then
985989
// delete the already-recovered Cluster + PVCs to start over.
986-
if hasRecovery {
990+
if clusterState.hasRecovery {
987991
healthy, herr := r.cnpgClusterHealthy(ctx, target.Namespace, clusterName)
988992
if herr != nil {
989993
return ctrl.Result{}, herr
@@ -1790,22 +1794,37 @@ func (r *RestoreJobReconciler) readPodContainerLog(ctx context.Context, namespac
17901794
// drops the change and the cluster ends up with the original initdb spec.
17911795
// Holding here forces the caller to requeue until the old CR is fully GC'd
17921796
// and the chart re-creates a fresh one.
1793-
func (r *RestoreJobReconciler) recoveryBootstrapClusterState(ctx context.Context, namespace, clusterName string) (hasRecovery bool, createdAt *metav1.Time, err error) {
1797+
func (r *RestoreJobReconciler) recoveryBootstrapClusterState(ctx context.Context, namespace, clusterName string) (recoveryClusterState, error) {
17941798
cluster := &cnpgtypes.Cluster{}
17951799
if err := r.Get(ctx, types.NamespacedName{Namespace: namespace, Name: clusterName}, cluster); err != nil {
17961800
if apierrors.IsNotFound(err) {
1797-
return false, nil, nil
1801+
return recoveryClusterState{}, nil
17981802
}
1799-
return false, nil, err
1803+
return recoveryClusterState{}, err
18001804
}
18011805
if !cluster.DeletionTimestamp.IsZero() {
1802-
return false, nil, nil
1806+
return recoveryClusterState{}, nil
18031807
}
18041808
if cluster.Spec.Bootstrap == nil || cluster.Spec.Bootstrap.Recovery == nil {
1805-
return false, nil, nil
1809+
return recoveryClusterState{}, nil
18061810
}
18071811
created := cluster.CreationTimestamp
1808-
return true, &created, nil
1812+
return recoveryClusterState{
1813+
hasRecovery: true,
1814+
createdAt: &created,
1815+
restoreMark: cluster.Annotations[cnpgRestoredServerNameAnnotation],
1816+
}, nil
1817+
}
1818+
1819+
// recoveryClusterState is what the purge decision reads off the live Cluster.
1820+
// The zero value is a Cluster that is absent, terminating, or not bootstrapped
1821+
// from a recovery.
1822+
type recoveryClusterState struct {
1823+
hasRecovery bool
1824+
createdAt *metav1.Time
1825+
// restoreMark is cnpgRestoredServerNameAnnotation, or "" on a Cluster the
1826+
// chart rendered before it set the annotation.
1827+
restoreMark string
18091828
}
18101829

18111830
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)