Skip to content

Commit 84c4346

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 68d8a78 commit 84c4346

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
@@ -69,6 +69,10 @@ const (
6969
cnpgS3SecretRefKey = "cnpg.io/s3-secret-ref"
7070
cnpgMajorVersionKey = "cnpg.io/major-version"
7171

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

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

468473
// cnpgClusterGVR addresses Clusters on the dynamic client, for reads that must
@@ -850,17 +855,16 @@ func (r *RestoreJobReconciler) reconcileCNPGRestore(ctx context.Context, restore
850855
// check the live Cluster for bootstrap.recovery: if present, the chart
851856
// has already re-rendered after a previous purge, and we must NOT delete
852857
// it again.
853-
hasRecovery, clusterCreatedAt, err := r.recoveryBootstrapClusterState(ctx, target.Namespace, clusterName)
858+
clusterState, err := r.recoveryBootstrapClusterState(ctx, target.Namespace, clusterName)
854859
if err != nil {
855860
return ctrl.Result{}, err
856861
}
857862
purgedCondition := apimeta.IsStatusConditionTrue(restoreJob.Status.Conditions, restoreCondTargetPurged)
858863
// Only skip the destructive purge for a recovery Cluster THIS restore just
859-
// produced (created at/after StartedAt). A recovery Cluster left over from
860-
// an earlier completed restore predates StartedAt and still holds the old
861-
// data, so it must be purged - otherwise a repeat in-place restore silently
862-
// no-ops.
863-
freshlyRecovered := cnpgClusterFreshlyRecovered(hasRecovery, clusterCreatedAt, restoreJob.Status.StartedAt)
864+
// produced. A recovery Cluster rendered for any other restore still holds
865+
// that restore's data, so it must be purged - otherwise a repeat in-place
866+
// restore silently no-ops.
867+
freshlyRecovered := cnpgClusterFreshlyRecovered(clusterState, restoredServerName(clusterName, restoreJob.UID), restoreJob.Status.StartedAt)
864868
if cnpgPurgeNeeded(purgedCondition, freshlyRecovered) {
865869
// A physical restore only opens on the major version that wrote the
866870
// data files. Across majors the recovery pods loop on "database files
@@ -1035,7 +1039,7 @@ func (r *RestoreJobReconciler) reconcileCNPGRestore(ctx context.Context, restore
10351039
// Getting the order wrong is not just a cosmetic mislabel - a false Failed
10361040
// can be resubmitted, and the next RestoreJob's purge guard would then
10371041
// delete the already-recovered Cluster + PVCs to start over.
1038-
if hasRecovery {
1042+
if clusterState.hasRecovery {
10391043
healthy, herr := r.cnpgClusterHealthy(ctx, target.Namespace, clusterName)
10401044
if herr != nil {
10411045
return ctrl.Result{}, herr
@@ -1899,22 +1903,37 @@ func (r *RestoreJobReconciler) readPodContainerLog(ctx context.Context, namespac
18991903
// drops the change and the cluster ends up with the original initdb spec.
19001904
// Holding here forces the caller to requeue until the old CR is fully GC'd
19011905
// and the chart re-creates a fresh one.
1902-
func (r *RestoreJobReconciler) recoveryBootstrapClusterState(ctx context.Context, namespace, clusterName string) (hasRecovery bool, createdAt *metav1.Time, err error) {
1906+
func (r *RestoreJobReconciler) recoveryBootstrapClusterState(ctx context.Context, namespace, clusterName string) (recoveryClusterState, error) {
19031907
cluster := &cnpgtypes.Cluster{}
19041908
if err := r.Get(ctx, types.NamespacedName{Namespace: namespace, Name: clusterName}, cluster); err != nil {
19051909
if apierrors.IsNotFound(err) {
1906-
return false, nil, nil
1910+
return recoveryClusterState{}, nil
19071911
}
1908-
return false, nil, err
1912+
return recoveryClusterState{}, err
19091913
}
19101914
if !cluster.DeletionTimestamp.IsZero() {
1911-
return false, nil, nil
1915+
return recoveryClusterState{}, nil
19121916
}
19131917
if cluster.Spec.Bootstrap == nil || cluster.Spec.Bootstrap.Recovery == nil {
1914-
return false, nil, nil
1918+
return recoveryClusterState{}, nil
19151919
}
19161920
created := cluster.CreationTimestamp
1917-
return true, &created, nil
1921+
return recoveryClusterState{
1922+
hasRecovery: true,
1923+
createdAt: &created,
1924+
restoreMark: cluster.Annotations[cnpgRestoredServerNameAnnotation],
1925+
}, nil
1926+
}
1927+
1928+
// recoveryClusterState is what the purge decision reads off the live Cluster.
1929+
// The zero value is a Cluster that is absent, terminating, or not bootstrapped
1930+
// from a recovery.
1931+
type recoveryClusterState struct {
1932+
hasRecovery bool
1933+
createdAt *metav1.Time
1934+
// restoreMark is cnpgRestoredServerNameAnnotation, or "" on a Cluster the
1935+
// chart rendered before it set the annotation.
1936+
restoreMark string
19181937
}
19191938

19201939
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)