Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/system/db_reconciler.go (1)

451-500: Retention logic: typos and recovery-snapshot protection still need tightening

Two earlier concerns still appear here:

1. Typos in variable names – avaialableItems and avaiallableSnapshots are still misspelled; this is minor but noisy and easy to fix.

2. Recovery snapshot protection and MaxSnapshots semantics – deletion is skipped whenever DBRecovery is configured and the snapshot name matches DBRecovery.VolumeSnapshotName, regardless of whether recovery has already completed:

if r.NooBaa.Spec.DBSpec.DBRecovery != nil && r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
    // ...
}

Given you now track RecoveryStatus.Status (Pending/Running/Completed), this will keep the recovery snapshot indefinitely (and effectively allow MaxSnapshots+1 snapshots) even long after a successful recovery, as long as DBRecovery remains set in the spec.

A more precise condition would only protect the snapshot while recovery is actually pending/running, not once it’s completed. For example:

-        for _, snapshot := range snapshots[:numToDelete] {
-            if r.NooBaa.Spec.DBSpec.DBRecovery != nil && r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
+        recoveryStatus := r.NooBaa.Status.DBStatus.RecoveryStatus
+        isRecoveryActive := recoveryStatus != nil && recoveryStatus.Status != nbv1.DBRecoveryStatusCompleted
+        for _, snapshot := range snapshots[:numToDelete] {
+            if isRecoveryActive &&
+                r.NooBaa.Spec.DBSpec.DBRecovery != nil &&
+                r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
                 r.cnpgLog("skipping deletion of recovery snapshot %s", snapshot.Name)
                 avaialableItems = append(avaialableItems, snapshot)
                 continue
             }

And optionally clean up the DBRecovery spec once recovery is fully completed if you don’t want it to keep influencing retention.

Also, consider fixing the typos in the same area:

-    avaialableItems := snapshots
+    availableItems := snapshots
    ...
-        avaialableItems = avaialableItems[numToDelete:]
+        availableItems = availableItems[numToDelete:]
    ...
-                avaialableItems = append(avaialableItems, snapshot)
+                availableItems = append(availableItems, snapshot)
    ...
-    avaiallableSnapshots := []string{}
-    for _, item := range avaialableItems {
-        avaiallableSnapshots = append(avaiallableSnapshots, item.Name)
-    }
-    r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = avaiallableSnapshots
+    availableSnapshots := []string{}
+    for _, item := range availableItems {
+        availableSnapshots = append(availableSnapshots, item.Name)
+    }
+    r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = availableSnapshots

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/system/db_reconciler.go (2)

471-471: Fix typo in variable name.

avaialableItems should be availableItems.

-	avaialableItems := snapshots
+	availableItems := snapshots

Also update all references to this variable (lines 474, 480, 503).

---

478-481: Recovery snapshot protected indefinitely beyond configured retention.

The recovery snapshot is excluded from deletion whenever DBRecovery is configured, regardless of whether recovery has completed. After a successful recovery, the DBRecovery config typically remains in the spec, causing the snapshot to be retained indefinitely and preventing MaxSnapshots enforcement from working correctly.

Consider checking the recovery status to only protect snapshots during active recovery:

-		if r.NooBaa.Spec.DBSpec.DBRecovery != nil && r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
-			r.cnpgLog("skipping deletion of recovery snapshot %s", snapshot.Name)
-			avaialableItems = append(avaialableItems, snapshot)
-			continue
+		// Only protect the recovery snapshot if recovery is still in progress
+		recoveryStatus := r.NooBaa.Status.DBStatus.RecoveryStatus
+		isRecoveryActive := recoveryStatus != nil && recoveryStatus.Status != nbv1.DBRecoveryStatusCompleted
+		if isRecoveryActive && r.NooBaa.Spec.DBSpec.DBRecovery != nil && r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
+			r.cnpgLog("skipping deletion of recovery snapshot %s (recovery in progress)", snapshot.Name)
+			availableItems = append(availableItems, snapshot)
+			continue

pkg/system/db_reconciler.go (1)

361-361: Follow Go naming conventions for unexported variables.

The variable VolSnapshotAPIGroup should start with a lowercase letter since it's an unexported (local) variable.

-	VolSnapshotAPIGroup := storagesnapshotv1.GroupName
+	volSnapshotAPIGroup := storagesnapshotv1.GroupName

And update its usage on line 371:

-			APIGroup: &VolSnapshotAPIGroup,
+			APIGroup: &volSnapshotAPIGroup,

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/system/db_reconciler.go (2)

502-502: Fix typo in variable name.

The variable name avaiallableSnapshots contains a typo. This was flagged in previous reviews but remains unfixed.

-	avaiallableSnapshots := []string{}
+	availableSnapshots := []string{}
 	for _, item := range availableItems {
-		avaiallableSnapshots = append(avaiallableSnapshots, item.Name)
+		availableSnapshots = append(availableSnapshots, item.Name)
 	}
-	r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = avaiallableSnapshots
+	r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = availableSnapshots

---

478-481: Recovery snapshot protected indefinitely after completion.

The recovery snapshot is currently protected whenever DBRecovery is configured, but this doesn't check whether the recovery has already completed. After a successful recovery, the snapshot will be retained indefinitely, preventing MaxSnapshots enforcement from working correctly.

As noted in previous reviews, the protection should only apply while recovery is active:

-			if r.NooBaa.Spec.DBSpec.DBRecovery != nil && r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
+			recoveryStatus := r.NooBaa.Status.DBStatus.RecoveryStatus
+			isRecoveryActive := recoveryStatus != nil && recoveryStatus.Status != nbv1.DBRecoveryStatusCompleted
+			if isRecoveryActive && r.NooBaa.Spec.DBSpec.DBRecovery != nil && r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
 				r.cnpgLog("skipping deletion of recovery snapshot %s", snapshot.Name)
 				availableItems = append(availableItems, snapshot)
 				continue
 			}

pkg/system/db_reconciler.go (1)

338-387: Recovery flow is sound, but minor naming convention issue.

The recovery orchestration correctly:

1. Sets recovery status to pending
2. Stops NooBaa pods and waits for termination
3. Configures Bootstrap.Recovery with the VolumeSnapshot
4. Cleans up scheduled backups to prevent conflicts

However, the local variable at line 361 should follow Go naming conventions:

-	VolSnapshotAPIGroup := storagesnapshotv1.GroupName
+	volSnapshotAPIGroup := storagesnapshotv1.GroupName
 	//setup once the pods are terminated, set recovery in the bootstrap configuration and continue to create the cluster
 	r.CNPGCluster.Spec.Bootstrap = &cnpgv1.BootstrapConfiguration{
 		Recovery: &cnpgv1.BootstrapRecovery{
 			Database: noobaaDBName,
 			Owner:    noobaaDBUser,
 			VolumeSnapshots: &cnpgv1.DataSource{
 				Storage: corev1.TypedLocalObjectReference{
 					Kind:     "VolumeSnapshot",
 					Name:     r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName,
-					APIGroup: &VolSnapshotAPIGroup,
+					APIGroup: &volSnapshotAPIGroup,
 				},
 			},
 		},
 	}

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/system/db_reconciler.go (1)

464-481: Recovery snapshot retention and typo in avaiallableSnapshots

1. The retention logic always skips deletion when DBRecovery is non-nil and the snapshot name matches, regardless of whether recovery has already completed:

if r.NooBaa.Spec.DBSpec.DBRecovery != nil &&
   r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
    ...
}

That means as long as the user leaves DBRecovery set, this snapshot will never be aged out, even after RecoveryStatus.Status becomes Completed. If the intent is to protect only while recovery is pending/running, consider tightening this to something like:

-        if r.NooBaa.Spec.DBSpec.DBRecovery != nil &&
-            r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
+        recoveryStatus := r.NooBaa.Status.DBStatus.RecoveryStatus
+        isRecoveryActive := recoveryStatus != nil &&
+            recoveryStatus.Status != nbv1.DBRecoveryStatusCompleted
+        if isRecoveryActive &&
+            r.NooBaa.Spec.DBSpec.DBRecovery != nil &&
+            r.NooBaa.Spec.DBSpec.DBRecovery.VolumeSnapshotName == snapshot.Name {
             r.cnpgLog("skipping deletion of recovery snapshot %s", snapshot.Name)
             availableItems = append(availableItems, snapshot)
             continue
         }

This keeps the snapshot during active recovery but lets normal MaxSnapshots enforcement apply once recovery is done. (This is the same concern raised in an earlier review.)

2. Minor naming nit: avaiallableSnapshots is misspelled and slightly confusing:

avaiallableSnapshots := []string{}
for _, item := range availableItems {
    avaiallableSnapshots = append(avaiallableSnapshots, item.Name)
}
r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = avaiallableSnapshots

Consider renaming to availableSnapshots:

-    avaiallableSnapshots := []string{}
-    for _, item := range availableItems {
-        avaiallableSnapshots = append(avaiallableSnapshots, item.Name)
-    }
-    r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = avaiallableSnapshots
+    availableSnapshots := make([]string, 0, len(availableItems))
+    for _, item := range availableItems {
+        availableSnapshots = append(availableSnapshots, item.Name)
+    }
+    r.NooBaa.Status.DBStatus.BackupStatus.AvailableSnapshots = availableSnapshots

Also applies to: 502-507

pkg/system/db_reconciler.go (3)

153-171: Recovery bootstrap path is clean, but consider gating on “active” recovery, not just DBRecovery presence

The split between:

• “no DBRecovery → InitDB bootstrap”, and
• “DBRecovery present → reconcileClusterRecovery + Bootstrap.Recovery + DBClusterStatusRecovering / RecoveryStatus{Pending→Running→Completed}

is clear and avoids nil derefs on DBRecovery.

One semantic concern: as coded, any future deletion of the CNPG Cluster while Spec.DBSpec.DBRecovery is still set will trigger another recovery from the same snapshot, potentially discarding newer data even after a successful recovery.

You may want to gate the recovery bootstrap on an “active” recovery condition (e.g. RecoveryStatus == nil or Status != Completed) instead of just DBRecovery != nil, so a completed recovery isn’t re-triggered accidentally. For example:

-    if r.NooBaa.Spec.DBSpec.DBRecovery == nil {
+    recoveryCfg := r.NooBaa.Spec.DBSpec.DBRecovery
+    recoveryStatus := r.NooBaa.Status.DBStatus.RecoveryStatus
+    isRecoveryActive := recoveryCfg != nil &&
+        (recoveryStatus == nil || recoveryStatus.Status != nbv1.DBRecoveryStatusCompleted)
+
+    if !isRecoveryActive {
         // InitDB bootstrap
         ...
-    } else {
+    } else {
         // Recovery bootstrap
         if err := r.reconcileClusterRecovery(); err != nil {
             ...
         }
     }

This keeps the current flow but makes “do another destructive recovery” an explicit action (e.g. by clearing/adjusting recovery status/spec) instead of happening on every cluster deletion while the spec field lingers.

Also applies to: 181-197, 338-347

---

293-313: VolumeSnapshot backup configuration guards are sound; minor nit on offlineBackup naming

The additional checks ensuring DBBackup.VolumeSnapshot is non-nil and that Spec.Backup.VolumeSnapshot is initialized before assigning ClassName and Online look correct and consistent with reconcileDBBackup.

Minor readability nit: offlineBackup := false then passed as &offlineBackup to the Online field inverts semantics in the variable name. Renaming the local to online := false (or similar) would better reflect what’s stored in the spec and avoid confusion when revisiting this code.

---

511-543: VolumeSnapshot listing logic is good; TypeMeta APIVersion is slightly off and unnecessary

The Client.List call with a namespace and LabelSelectorFromValidatedSet({"cnpg.io/cluster": <name>}) followed by a name-prefix filter is a reasonable way to scope snapshots to this cluster’s scheduled backups.

Two minor nits around the list struct:

volumeSnapshots := storagesnapshotv1.VolumeSnapshotList{
    TypeMeta: metav1.TypeMeta{
        APIVersion: storagesnapshotv1.GroupName,
        Kind:       "VolumeSnapshotList",
    },
}

• storagesnapshotv1.GroupName is just the group (e.g. "snapshot.storage.k8s.io"), not the full "snapshot.storage.k8s.io/v1" APIVersion.
• For controller-runtime Client.List, setting TypeMeta is not required; the scheme already knows the correct GVK for VolumeSnapshotList.

You can simplify and avoid any APIVersion confusion by dropping the TypeMeta entirely:

-func (r *Reconciler) listVolumeSnapshotsOrderByCreate() ([]storagesnapshotv1.VolumeSnapshot, error) {
-    volumeSnapshots := storagesnapshotv1.VolumeSnapshotList{
-        TypeMeta: metav1.TypeMeta{
-            APIVersion: storagesnapshotv1.GroupName,
-            Kind:       "VolumeSnapshotList",
-        },
-    }
+func (r *Reconciler) listVolumeSnapshotsOrderByCreate() ([]storagesnapshotv1.VolumeSnapshot, error) {
+    volumeSnapshots := storagesnapshotv1.VolumeSnapshotList{}

The rest of the function (filtering by prefix and sorting by CreationTimestamp) looks good.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro