Skip to content

Commit 0fdf3b5

Browse files
IvanHuntersgithub-actions[bot]
IvanHunters
authored and
github-actions[bot]
committed
test(backups/postgres): reconcile-level repeat-restore regression + conservative freshness tie
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 <[email protected]> (cherry picked from commit a8494d8)
1 parent c85c9f3 commit 0fdf3b5

2 files changed

Lines changed: 164 additions & 5 deletions

File tree

‎internal/backupcontroller/cnpgstrategy_controller.go‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -336,8 +336,9 @@ func cnpgPurgeNeeded(purgedCondition, liveClusterFreshlyRecovered bool) bool {
336336

337337
// cnpgClusterFreshlyRecovered reports whether a live cnpg.io Cluster that
338338
// carries spec.bootstrap.recovery was produced by THIS RestoreJob's own purge
339-
// + chart re-render (its creationTimestamp is at or after the job's StartedAt)
340-
// rather than being left over from an earlier, already-completed restore.
339+
// + chart re-render (its creationTimestamp is strictly after the job's
340+
// StartedAt) rather than being left over from an earlier, already-completed
341+
// restore.
341342
//
342343
// Only the former is safe to skip re-purging. A leftover recovery Cluster from
343344
// a prior restore predates StartedAt and still holds the prior data, so it must
@@ -346,14 +347,29 @@ func cnpgPurgeNeeded(purgedCondition, liveClusterFreshlyRecovered bool) bool {
346347
// job sets StartedAt on its first reconcile, long before it purges and the
347348
// chart re-renders), so the timestamp comparison cleanly separates the two.
348349
//
350+
// The comparison is strict (created > started), so an exact tie resolves to
351+
// "not fresh" and the caller purges. That matches the conservative default
352+
// below: the only classification that must never be wrong is calling a stale
353+
// leftover "fresh" (which reintroduces the silent no-op), and a fresh Cluster
354+
// is always created well after StartedAt (see above), never exactly at it.
355+
//
356+
// An identity-based alternative was considered - recording the purged Cluster's
357+
// UID on the TargetPurged condition and comparing UIDs on later reconciles -
358+
// which is immune to clock skew. It was rejected because it leans on the same
359+
// status write that the status-write-race path (the whole reason this skip
360+
// exists) assumes can fail, so it cannot cover that case; the timestamp
361+
// comparison needs no extra persisted state and the fresh-vs-stale gap
362+
// (a full purge + re-render cycle) always dwarfs any plausible control-plane
363+
// clock skew.
364+
//
349365
// Returns false when freshness cannot be determined (no recovery bootstrap, or
350366
// a missing timestamp): the caller then proceeds to purge, which is the safe
351367
// default for any pre-existing, non-fresh Cluster.
352368
func cnpgClusterFreshlyRecovered(hasRecovery bool, clusterCreatedAt, restoreStartedAt *metav1.Time) bool {
353369
if !hasRecovery || clusterCreatedAt == nil || restoreStartedAt == nil {
354370
return false
355371
}
356-
return !clusterCreatedAt.Time.Before(restoreStartedAt.Time)
372+
return clusterCreatedAt.After(restoreStartedAt.Time)
357373
}
358374

359375
// applyClusterBarmanObjectStore SSA-patches the live CNPG Cluster's

‎internal/backupcontroller/cnpgstrategy_controller_test.go‎

Lines changed: 145 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -641,11 +641,11 @@ func TestCNPGClusterFreshlyRecovered(t *testing.T) {
641641
want: false,
642642
},
643643
{
644-
name: "recovery cluster created exactly at start: fresh (boundary)",
644+
name: "recovery cluster created exactly at start: not fresh (conservative tie -> purge)",
645645
hasRecovery: true,
646646
createdAt: &started,
647647
startedAt: &started,
648-
want: true,
648+
want: false,
649649
},
650650
{
651651
name: "missing cluster creation timestamp: not fresh",
@@ -1598,6 +1598,149 @@ func TestReconcileCNPGRestore_MissingStrategyFailsClosedAfterDeadline(t *testing
15981598
}
15991599
}
16001600

1601+
// TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster is the
1602+
// reconcile-level regression test for #3311. The pure-function tests
1603+
// (TestCNPGPurgeNeeded / TestCNPGClusterFreshlyRecovered) lock in the truth
1604+
// table, but the bug lived at the call site: a repeat in-place restore fed the
1605+
// purge decision the wrong signal and skipped the destructive purge, so the
1606+
// job reported Succeeded against untouched data. This drives the real
1607+
// reconcileCNPGRestore path with a fake client and asserts the destructive
1608+
// purge actually fires for a stale leftover recovery Cluster - and, in the
1609+
// mirror case, that a Cluster this restore just re-created is NOT re-purged
1610+
// (the status-write-race protection the guard was originally built for).
1611+
func TestReconcileCNPGRestore_RepeatInPlacePurgesStaleRecoveryCluster(t *testing.T) {
1612+
const (
1613+
ns = "tenant"
1614+
appName = "app"
1615+
clusterName = "postgres-app"
1616+
cnpgBkName = "cnpgbk"
1617+
)
1618+
apiGroup := backupsv1alpha1.DefaultApplicationAPIGroup
1619+
strategyGroup := strategyv1alpha1.GroupVersion.Group
1620+
ctx := context.Background()
1621+
1622+
// startedAt anchors the discriminator: a stale leftover Cluster predates
1623+
// it, a freshly-recovered one postdates it.
1624+
startedAt := metav1.NewTime(time.Now())
1625+
1626+
mkBackupArtifact := func(t *testing.T) *backupsv1alpha1.Backup {
1627+
t.Helper()
1628+
snap, err := marshalCNPGBackupSnapshot(newPostgresApp(appName, ns), nil)
1629+
if err != nil {
1630+
t.Fatalf("marshal snapshot: %v", err)
1631+
}
1632+
return &backupsv1alpha1.Backup{
1633+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: "bk"},
1634+
Spec: backupsv1alpha1.BackupSpec{
1635+
ApplicationRef: corev1.TypedLocalObjectReference{APIGroup: &apiGroup, Kind: postgresAppKind, Name: appName},
1636+
StrategyRef: corev1.TypedLocalObjectReference{APIGroup: &strategyGroup, Kind: strategyv1alpha1.CNPGStrategyKind, Name: "cnpg-strategy"},
1637+
DriverMetadata: map[string]string{
1638+
cnpgServerNameKey: appName,
1639+
cnpgDestinationPathKey: "s3://bucket/" + appName + "/",
1640+
cnpgBackupNameKey: cnpgBkName,
1641+
},
1642+
},
1643+
Status: backupsv1alpha1.BackupStatus{UnderlyingResources: snap},
1644+
}
1645+
}
1646+
strategy := &strategyv1alpha1.CNPG{
1647+
ObjectMeta: metav1.ObjectMeta{Name: "cnpg-strategy"},
1648+
Spec: strategyv1alpha1.CNPGSpec{
1649+
Template: strategyv1alpha1.CNPGTemplate{
1650+
BarmanObjectStore: strategyv1alpha1.BarmanObjectStoreTemplate{DestinationPath: "s3://bucket/"},
1651+
},
1652+
},
1653+
}
1654+
// A completed cnpg.io/Backup with endWal set clears the WAL-archive gate
1655+
// so the reconcile can reach the purge step.
1656+
cnpgBackup := &cnpgtypes.Backup{
1657+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: cnpgBkName},
1658+
Spec: cnpgtypes.BackupSpec{Cluster: cnpgtypes.ClusterReference{Name: clusterName}},
1659+
Status: cnpgtypes.BackupStatus{Phase: cnpgBackupPhaseComplete, EndWal: "000000010000000000000003"},
1660+
}
1661+
mkRecoveryCluster := func(created metav1.Time) *cnpgtypes.Cluster {
1662+
return &cnpgtypes.Cluster{
1663+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: clusterName, CreationTimestamp: created},
1664+
Spec: cnpgtypes.ClusterSpec{
1665+
Bootstrap: &cnpgtypes.BootstrapConfiguration{
1666+
Recovery: &cnpgtypes.RecoverySource{Source: appName},
1667+
},
1668+
},
1669+
}
1670+
}
1671+
mkClusterPVC := func() *corev1.PersistentVolumeClaim {
1672+
return &corev1.PersistentVolumeClaim{
1673+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: clusterName + "-1", Labels: map[string]string{cnpgClusterLabel: clusterName}},
1674+
}
1675+
}
1676+
mkRestoreJob := func() *backupsv1alpha1.RestoreJob {
1677+
sa := startedAt
1678+
return &backupsv1alpha1.RestoreJob{
1679+
ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: "rj"},
1680+
Spec: backupsv1alpha1.RestoreJobSpec{BackupRef: corev1.LocalObjectReference{Name: "bk"}},
1681+
Status: backupsv1alpha1.RestoreJobStatus{StartedAt: &sa, Phase: backupsv1alpha1.RestoreJobPhaseRunning},
1682+
}
1683+
}
1684+
1685+
t.Run("stale leftover recovery cluster from a prior restore is purged", func(t *testing.T) {
1686+
backup := mkBackupArtifact(t)
1687+
// creationTimestamp an hour before StartedAt: leftover from a prior restore.
1688+
stale := metav1.NewTime(startedAt.Add(-time.Hour))
1689+
c := newCNPGStrategyTestClient(t, backup, mkRestoreJob(), strategy, cnpgBackup,
1690+
newPostgresApp(appName, ns), mkRecoveryCluster(stale), mkClusterPVC())
1691+
r := &RestoreJobReconciler{Client: c, Interface: dynamicfake.NewSimpleDynamicClient(testCNPGScheme(t))}
1692+
1693+
rj := &backupsv1alpha1.RestoreJob{}
1694+
if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: "rj"}, rj); err != nil {
1695+
t.Fatalf("get seeded RestoreJob: %v", err)
1696+
}
1697+
if _, err := r.reconcileCNPGRestore(ctx, rj, backup); err != nil {
1698+
t.Fatalf("reconcileCNPGRestore: %v", err)
1699+
}
1700+
1701+
// The stale Cluster (and its labelled PVC) must have been purged.
1702+
err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: clusterName}, &cnpgtypes.Cluster{})
1703+
if !apierrors.IsNotFound(err) {
1704+
t.Fatalf("expected stale Cluster to be purged (NotFound), got err=%v", err)
1705+
}
1706+
pvcs := &corev1.PersistentVolumeClaimList{}
1707+
if err := c.List(ctx, pvcs, client.InNamespace(ns), client.MatchingLabels{cnpgClusterLabel: clusterName}); err != nil {
1708+
t.Fatalf("list PVCs: %v", err)
1709+
}
1710+
if len(pvcs.Items) != 0 {
1711+
t.Fatalf("expected cluster PVCs purged, still have %d", len(pvcs.Items))
1712+
}
1713+
})
1714+
1715+
t.Run("freshly-recovered cluster from this restore is not re-purged", func(t *testing.T) {
1716+
backup := mkBackupArtifact(t)
1717+
// creationTimestamp a minute after StartedAt: this restore's own re-render.
1718+
fresh := metav1.NewTime(startedAt.Add(time.Minute))
1719+
c := newCNPGStrategyTestClient(t, backup, mkRestoreJob(), strategy, cnpgBackup,
1720+
newPostgresApp(appName, ns), mkRecoveryCluster(fresh))
1721+
r := &RestoreJobReconciler{Client: c, Interface: dynamicfake.NewSimpleDynamicClient(testCNPGScheme(t))}
1722+
1723+
rj := &backupsv1alpha1.RestoreJob{}
1724+
if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: "rj"}, rj); err != nil {
1725+
t.Fatalf("get seeded RestoreJob: %v", err)
1726+
}
1727+
if _, err := r.reconcileCNPGRestore(ctx, rj, backup); err != nil {
1728+
t.Fatalf("reconcileCNPGRestore: %v", err)
1729+
}
1730+
1731+
// The freshly-recovered Cluster must survive - re-purging it would
1732+
// destroy the recovery this restore just started (the status-write-race
1733+
// protection the guard was built for).
1734+
got := &cnpgtypes.Cluster{}
1735+
if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: clusterName}, got); err != nil {
1736+
t.Fatalf("expected freshly-recovered Cluster to survive, got err=%v", err)
1737+
}
1738+
if !got.DeletionTimestamp.IsZero() {
1739+
t.Fatalf("freshly-recovered Cluster must not be marked for deletion")
1740+
}
1741+
})
1742+
}
1743+
16011744
// newCNPGStrategyTestClient returns a fake client.Client wired up with
16021745
// the schemes the CNPG-strategy reconciler needs.
16031746
func newCNPGStrategyTestClient(t *testing.T, objs ...client.Object) client.Client {

0 commit comments

Comments
 (0)