Skip to content

Commit 47512ae

Browse files
androndoandrondo
committed
fix(backups): keep the CNPG purge for unmarked Clusters this restore did not render
A recovery Cluster without the restored-server-name annotation fell back to the creation-time comparison. On the current chart the annotation is absent when bootstrap.newServerName is empty, i.e. a manually configured recovery: re-rendered during the WAL-archive wait it is created after StartedAt, the purge was skipped and the restore could report success on the manual recovery source. Use the creation-time fallback only when the application's bootstrap.newServerName already carries this restore's server name, which keeps it for Clusters rendered by a chart that predates the annotation. Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent c9aafd5 commit 47512ae

2 files changed

Lines changed: 65 additions & 12 deletions

File tree

‎internal/backupcontroller/cnpgstrategy_controller.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -440,22 +440,24 @@ func cnpgPurgeNeeded(purgedCondition, liveClusterFreshlyRecovered bool) bool {
440440
// restore that rendered the Cluster. When the annotation is present, the
441441
// Cluster is fresh only if it carries wantMark.
442442
//
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.
443+
// A Cluster without the annotation was rendered either by a chart that
444+
// predates it or from values with no bootstrap.newServerName (a manually
445+
// configured recovery). It is fresh only if appMark, the application's current
446+
// bootstrap.newServerName, is wantMark - this restore has patched the values -
447+
// and it was created strictly after StartedAt, which separates a Cluster from
448+
// an earlier completed restore from this restore's own re-render.
447449
//
448450
// Returns false when freshness cannot be determined (no recovery bootstrap, or
449451
// a missing timestamp on the fallback path): the caller then purges, which is
450452
// the safe default.
451-
func cnpgClusterFreshlyRecovered(state recoveryClusterState, wantMark string, restoreStartedAt *metav1.Time) bool {
453+
func cnpgClusterFreshlyRecovered(state recoveryClusterState, wantMark, appMark string, restoreStartedAt *metav1.Time) bool {
452454
if !state.hasRecovery {
453455
return false
454456
}
455457
if state.restoreMark != "" {
456458
return state.restoreMark == wantMark
457459
}
458-
if state.createdAt == nil || restoreStartedAt == nil {
460+
if appMark != wantMark || state.createdAt == nil || restoreStartedAt == nil {
459461
return false
460462
}
461463
return state.createdAt.After(restoreStartedAt.Time)
@@ -835,7 +837,7 @@ func (r *RestoreJobReconciler) reconcileCNPGRestore(ctx context.Context, restore
835837
// produced. A recovery Cluster rendered for any other restore still holds
836838
// that restore's data, so it must be purged - otherwise a repeat in-place
837839
// restore silently no-ops.
838-
freshlyRecovered := cnpgClusterFreshlyRecovered(clusterState, restoredServerName(clusterName, restoreJob.UID), restoreJob.Status.StartedAt)
840+
freshlyRecovered := cnpgClusterFreshlyRecovered(clusterState, restoredServerName(clusterName, restoreJob.UID), targetApp.Spec.Bootstrap.NewServerName, restoreJob.Status.StartedAt)
839841
if cnpgPurgeNeeded(purgedCondition, freshlyRecovered) {
840842
// Gate the destructive flow on the source cluster having shipped
841843
// the backup's required WALs to object storage. archive_command runs

‎internal/backupcontroller/cnpgstrategy_controller_test.go‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,7 @@ func TestCNPGClusterFreshlyRecovered(t *testing.T) {
653653
cases := []struct {
654654
name string
655655
state recoveryClusterState
656+
appMark string
656657
startedAt *metav1.Time
657658
want bool
658659
}{
@@ -689,37 +690,56 @@ func TestCNPGClusterFreshlyRecovered(t *testing.T) {
689690
{
690691
name: "unmarked, created after start (our own re-render by an older chart): fresh",
691692
state: recoveryClusterState{hasRecovery: true, createdAt: &after},
693+
appMark: ours,
692694
startedAt: &started,
693695
want: true,
694696
},
697+
{
698+
name: "unmarked, created after start, values not patched by this restore (manual recovery re-rendered): not fresh",
699+
state: recoveryClusterState{hasRecovery: true, createdAt: &after},
700+
appMark: "",
701+
startedAt: &started,
702+
want: false,
703+
},
704+
{
705+
name: "unmarked, created after start, values patched by another restore: not fresh",
706+
state: recoveryClusterState{hasRecovery: true, createdAt: &after},
707+
appMark: other,
708+
startedAt: &started,
709+
want: false,
710+
},
695711
{
696712
name: "unmarked, created before start (leftover from previous restore): not fresh",
697713
state: recoveryClusterState{hasRecovery: true, createdAt: &before},
714+
appMark: ours,
698715
startedAt: &started,
699716
want: false,
700717
},
701718
{
702719
name: "unmarked, created exactly at start: not fresh (conservative tie -> purge)",
703720
state: recoveryClusterState{hasRecovery: true, createdAt: &started},
721+
appMark: ours,
704722
startedAt: &started,
705723
want: false,
706724
},
707725
{
708726
name: "unmarked, missing cluster creation timestamp: not fresh",
709727
state: recoveryClusterState{hasRecovery: true},
728+
appMark: ours,
710729
startedAt: &started,
711730
want: false,
712731
},
713732
{
714733
name: "unmarked, missing job start timestamp: not fresh",
715734
state: recoveryClusterState{hasRecovery: true, createdAt: &after},
735+
appMark: ours,
716736
startedAt: nil,
717737
want: false,
718738
},
719739
}
720740
for _, tc := range cases {
721741
t.Run(tc.name, func(t *testing.T) {
722-
got := cnpgClusterFreshlyRecovered(tc.state, ours, tc.startedAt)
742+
got := cnpgClusterFreshlyRecovered(tc.state, ours, tc.appMark, tc.startedAt)
723743
if got != tc.want {
724744
t.Errorf("got %v want %v", got, tc.want)
725745
}
@@ -2279,12 +2299,16 @@ func TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster(t *testing
22792299
}
22802300
})
22812301

2282-
t.Run("freshly-recovered cluster from this restore is not re-purged", func(t *testing.T) {
2302+
t.Run("unmarked cluster this restore's values rendered is not re-purged", func(t *testing.T) {
22832303
backup := mkBackupArtifact(t)
2284-
// creationTimestamp a minute after StartedAt: this restore's own re-render.
2304+
// creationTimestamp a minute after StartedAt: this restore's own
2305+
// re-render by a chart that does not annotate the Cluster.
22852306
fresh := metav1.NewTime(startedAt.Add(time.Minute))
2286-
c := newCNPGStrategyTestClient(t, backup, mkRestoreJob(), strategy, cnpgBackup,
2287-
newPostgresApp(appName, ns), mkRecoveryCluster(fresh))
2307+
rjSeed := mkRestoreJob()
2308+
app := newPostgresApp(appName, ns)
2309+
app.Spec.Bootstrap.NewServerName = restoredServerName(clusterName, rjSeed.UID)
2310+
c := newCNPGStrategyTestClient(t, backup, rjSeed, strategy, cnpgBackup,
2311+
app, mkRecoveryCluster(fresh))
22882312
r := &RestoreJobReconciler{Client: c, Interface: dynamicfake.NewSimpleDynamicClient(testCNPGScheme(t)), Recorder: record.NewFakeRecorder(10)}
22892313

22902314
rj := &backupsv1alpha1.RestoreJob{}
@@ -2307,6 +2331,33 @@ func TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster(t *testing
23072331
}
23082332
})
23092333

2334+
// A manually configured recovery (bootstrap.enabled without
2335+
// newServerName) renders no annotation. Re-rendered after StartedAt but
2336+
// before this restore patches the values, it holds the manual recovery
2337+
// source, not this restore's backup.
2338+
t.Run("unmarked cluster from values this restore has not patched is purged even when created after start", func(t *testing.T) {
2339+
backup := mkBackupArtifact(t)
2340+
fresh := metav1.NewTime(startedAt.Add(time.Minute))
2341+
app := newPostgresApp(appName, ns)
2342+
app.Spec.Bootstrap.Enabled = true
2343+
app.Spec.Bootstrap.OldName = "manual-source"
2344+
c := newCNPGStrategyTestClient(t, backup, mkRestoreJob(), strategy, cnpgBackup,
2345+
app, mkRecoveryCluster(fresh), mkClusterPVC())
2346+
r := &RestoreJobReconciler{Client: c, Interface: dynamicfake.NewSimpleDynamicClient(testCNPGScheme(t)), Recorder: record.NewFakeRecorder(10)}
2347+
2348+
rj := &backupsv1alpha1.RestoreJob{}
2349+
if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: "rj"}, rj); err != nil {
2350+
t.Fatalf("get seeded RestoreJob: %v", err)
2351+
}
2352+
if _, err := r.reconcileCNPGRestore(ctx, rj, backup); err != nil {
2353+
t.Fatalf("reconcileCNPGRestore: %v", err)
2354+
}
2355+
err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: clusterName}, &cnpgtypes.Cluster{})
2356+
if !apierrors.IsNotFound(err) {
2357+
t.Fatalf("expected the manual recovery Cluster to be purged (NotFound), got err=%v", err)
2358+
}
2359+
})
2360+
23102361
t.Run("recovery cluster marked by another restore is purged even when created after start", func(t *testing.T) {
23112362
backup := mkBackupArtifact(t)
23122363
fresh := metav1.NewTime(startedAt.Add(time.Minute))

0 commit comments

Comments
 (0)