EVEREST-1141 Backup storage files deletion fix (#428)

Co-authored-by: Mayank Shah <mayank.shah@percona.com>

controllers/common/consts.go

@@ -59,8 +59,8 @@ const (
 	// EverestSecretsPrefix is the prefix for secrets created by Everest.
 	EverestSecretsPrefix = "everest-secrets-"
 
-	// DBBackupCleanupFinalizer is the finalizer for cleaning up DatabaseClusterBackup.
-	DBBackupCleanupFinalizer = "everest.percona.com/dbb-cleanup"
+	// UpstreamClusterCleanupFinalizer is the finalizer for cleaning up the upstream cluster.
+	UpstreamClusterCleanupFinalizer = "everest.percona.com/upstream-cluster-cleanup"
 
 	// ForegroundDeletionFinalizer is the finalizer that ensures foreground deletion for the resource.
 	ForegroundDeletionFinalizer = "foregroundDeletion"

controllers/common/helper.go

@@ -621,49 +621,47 @@ func GetBackupStorageIndexInPGBackrestRepo(
 	return -1
 }
 
-// HandleDBBackupsCleanup handles the cleanup of the dbbackup objects.
+// HandleUpstreamClusterCleanup handles the cleanup of the psdmb objects.
 // Returns true if cleanup is complete.
-func HandleDBBackupsCleanup(
+func HandleUpstreamClusterCleanup(
 	ctx context.Context,
 	c client.Client,
 	database *everestv1alpha1.DatabaseCluster,
+	upstream client.Object,
 ) (bool, error) {
-	if controllerutil.ContainsFinalizer(database, DBBackupCleanupFinalizer) {
-		if done, err := deleteBackupsForDatabase(ctx, c, database.GetName(), database.GetNamespace()); err != nil {
+	if controllerutil.ContainsFinalizer(database, UpstreamClusterCleanupFinalizer) { //nolint:nestif
+		// first check that all dbb are deleted since the upstream backups may need the upstream cluster to be present.
+		backupList, err := ListDatabaseClusterBackups(ctx, c, database.GetName(), database.GetNamespace())
+		if err != nil {
 			return false, err
-		} else if !done {
+		}
+		if len(backupList.Items) != 0 {
 			return false, nil
 		}
-		controllerutil.RemoveFinalizer(database, DBBackupCleanupFinalizer)
-		return true, c.Update(ctx, database)
-	}
-	return true, nil
-}
 
-// Delete all dbbackups for the given database.
-// Returns true if no dbbackups are found.
-func deleteBackupsForDatabase(
-	ctx context.Context,
-	c client.Client,
-	dbName, dbNs string,
-) (bool, error) {
-	backupList, err := ListDatabaseClusterBackups(ctx, c, dbName, dbNs)
-	if err != nil {
-		return false, err
-	}
-	if len(backupList.Items) == 0 {
-		return true, nil
-	}
-	for _, backup := range backupList.Items {
-		if !backup.GetDeletionTimestamp().IsZero() {
-			// Already deleting, continue to next.
-			continue
+		err = c.Get(ctx, types.NamespacedName{
+			Name:      database.Name,
+			Namespace: database.Namespace,
+		}, upstream)
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				controllerutil.RemoveFinalizer(database, UpstreamClusterCleanupFinalizer)
+				return true, c.Update(ctx, database)
+			}
+			return false, err
 		}
-		if err := c.Delete(ctx, &backup); err != nil {
+
+		err = c.Delete(ctx, upstream)
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				return true, nil
+			}
 			return false, err
 		}
+		controllerutil.RemoveFinalizer(database, UpstreamClusterCleanupFinalizer)
+		return true, c.Update(ctx, database)
 	}
-	return false, nil
+	return true, nil
 }
 
 // IsOwnedBy checks if the child object is owned by the parent object.

controllers/databasecluster_controller.go

@@ -70,7 +70,7 @@ const (
 )
 
 var everestFinalizers = []string{
-	common.DBBackupCleanupFinalizer,
+	common.UpstreamClusterCleanupFinalizer,
 	common.ForegroundDeletionFinalizer,
 }
 
@@ -164,9 +164,6 @@ func (r *DatabaseClusterReconciler) reconcileDB(
 		if err := applier.DataSource(); err != nil {
 			return err
 		}
-		if err := controllerutil.SetControllerReference(db, p.DBObject(), r.Client.Scheme()); err != nil {
-			return err
-		}
 		return nil
 	}
 	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, p.DBObject(), mutate); err != nil {
@@ -659,6 +656,22 @@ func (r *DatabaseClusterReconciler) initWatchers(controller *builder.Builder) {
 		}),
 		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
 	)
+
+	controller.Watches(
+		&psmdbv1.PerconaServerMongoDB{},
+		&handler.EnqueueRequestForObject{},
+		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
+	)
+	controller.Watches(
+		&pgv2.PerconaPGCluster{},
+		&handler.EnqueueRequestForObject{},
+		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
+	)
+	controller.Watches(
+		&pxcv1.PerconaXtraDBCluster{},
+		&handler.EnqueueRequestForObject{},
+		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
+	)
 }
 
 func (r *DatabaseClusterReconciler) databaseClustersInObjectNamespace(ctx context.Context, obj client.Object) []reconcile.Request {

controllers/databaseclusterbackup_controller.go

@@ -55,6 +55,7 @@ import (
 )
 
 const (
+	databaseClusterKind       = "DatabaseCluster"
 	databaseClusterBackupKind = "DatabaseClusterBackup"
 	everestAPIVersion         = "everest.percona.com/v1alpha1"
 
@@ -118,16 +119,6 @@ func (r *DatabaseClusterBackupReconciler) Reconcile(ctx context.Context, req ctr
 		return reconcile.Result{}, err
 	}
 
-	if len(backup.ObjectMeta.Labels) == 0 {
-		backup.ObjectMeta.Labels = map[string]string{
-			databaseClusterNameLabel: backup.Spec.DBClusterName,
-			fmt.Sprintf(backupStorageNameLabelTmpl, backup.Spec.BackupStorageName): backupStorageLabelValue,
-		}
-		if err := r.Update(ctx, backup); err != nil {
-			return reconcile.Result{}, err
-		}
-	}
-
 	// DBBackups are always deleted in foreground.
 	if backup.GetDeletionTimestamp().IsZero() &&
 		controllerutil.AddFinalizer(backup, common.ForegroundDeletionFinalizer) {
@@ -146,6 +137,10 @@ func (r *DatabaseClusterBackupReconciler) Reconcile(ctx context.Context, req ctr
 		return reconcile.Result{}, err
 	}
 
+	if err := r.reconcileMeta(ctx, backup, cluster); err != nil {
+		return reconcile.Result{}, err
+	}
+
 	storage := &everestv1alpha1.BackupStorage{}
 	err = r.Get(ctx, types.NamespacedName{Name: backup.Spec.BackupStorageName, Namespace: r.systemNamespace}, storage)
 	if err != nil {
@@ -194,6 +189,32 @@ func (r *DatabaseClusterBackupReconciler) Reconcile(ctx context.Context, req ctr
 	return ctrl.Result{Requeue: requeue}, nil
 }
 
+func (r *DatabaseClusterBackupReconciler) reconcileMeta(
+	ctx context.Context,
+	backup *everestv1alpha1.DatabaseClusterBackup,
+	cluster *everestv1alpha1.DatabaseCluster,
+) error {
+	var needUpdate bool
+	if len(backup.ObjectMeta.Labels) == 0 {
+		backup.ObjectMeta.Labels = map[string]string{
+			databaseClusterNameLabel: backup.Spec.DBClusterName,
+			fmt.Sprintf(backupStorageNameLabelTmpl, backup.Spec.BackupStorageName): backupStorageLabelValue,
+		}
+		needUpdate = true
+	}
+	if metav1.GetControllerOf(backup) == nil {
+		if err := controllerutil.SetControllerReference(cluster, backup, r.Client.Scheme()); err != nil {
+			return err
+		}
+		needUpdate = true
+	}
+
+	if needUpdate {
+		return r.Update(ctx, backup)
+	}
+	return nil
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *DatabaseClusterBackupReconciler) SetupWithManager(mgr ctrl.Manager, systemNamespace string) error {
 	unstructuredResource := &unstructured.Unstructured{}
@@ -381,6 +402,9 @@ func (r *DatabaseClusterBackupReconciler) tryCreatePG(ctx context.Context, obj c
 		databaseClusterNameLabel:                      pgBackup.Spec.PGCluster,
 		fmt.Sprintf(backupStorageNameLabelTmpl, name): backupStorageLabelValue,
 	}
+	if err = controllerutil.SetControllerReference(cluster, backup, r.Scheme); err != nil {
+		return err
+	}
 
 	return r.Create(ctx, backup)
 }
@@ -423,6 +447,15 @@ func (r *DatabaseClusterBackupReconciler) tryCreatePXC(ctx context.Context, obj
 		databaseClusterNameLabel: pxcBackup.Spec.PXCCluster,
 		fmt.Sprintf(backupStorageNameLabelTmpl, pxcBackup.Spec.StorageName): backupStorageLabelValue,
 	}
+	cluster := &everestv1alpha1.DatabaseCluster{}
+	err = r.Get(ctx, types.NamespacedName{Name: pxcBackup.Spec.PXCCluster, Namespace: pxcBackup.Namespace}, cluster)
+	if err != nil {
+		return err
+	}
+	if err = controllerutil.SetControllerReference(cluster, backup, r.Scheme); err != nil {
+		return err
+	}
+
 	return r.Create(ctx, backup)
 }
 
@@ -475,6 +508,14 @@ func (r *DatabaseClusterBackupReconciler) tryCreatePSMDB(ctx context.Context, ob
 		databaseClusterNameLabel: psmdbBackup.Spec.ClusterName,
 		fmt.Sprintf(backupStorageNameLabelTmpl, psmdbBackup.Spec.StorageName): backupStorageLabelValue,
 	}
+	cluster := &everestv1alpha1.DatabaseCluster{}
+	err = r.Get(ctx, types.NamespacedName{Name: psmdbBackup.Spec.ClusterName, Namespace: psmdbBackup.Namespace}, cluster)
+	if err != nil {
+		return err
+	}
+	if err = controllerutil.SetControllerReference(cluster, backup, r.Scheme); err != nil {
+		return err
+	}
 	return r.Create(ctx, backup)
 }
 

controllers/providers/pg/provider.go

@@ -134,7 +134,7 @@ func (p *Provider) Status(ctx context.Context) (everestv1alpha1.DatabaseClusterS
 
 // Cleanup runs the cleanup routines and returns true if the cleanup is done.
 func (p *Provider) Cleanup(ctx context.Context, database *everestv1alpha1.DatabaseCluster) (bool, error) {
-	return common.HandleDBBackupsCleanup(ctx, p.C, database)
+	return common.HandleUpstreamClusterCleanup(ctx, p.C, database, &pgv2.PerconaPGCluster{})
 }
 
 // DBObject returns the PerconaPGCluster object.

controllers/providers/psmdb/provider.go

@@ -153,7 +153,33 @@ func (p *Provider) Status(ctx context.Context) (everestv1alpha1.DatabaseClusterS
 
 // Cleanup runs the cleanup routines and returns true if the cleanup is done.
 func (p *Provider) Cleanup(ctx context.Context, database *everestv1alpha1.DatabaseCluster) (bool, error) {
-	return common.HandleDBBackupsCleanup(ctx, p.C, database)
+	// as a first step of the psmdb cleanup we need to ensure that psmdb pitr is disabled, otherwise the
+	// "failed to delete backup: unable to delete the last backup while PITR is on"
+	// appears when trying to delete the last backup.
+	err := ensurePSMDBPitrDisabled(ctx, p.C, database)
+	if err != nil {
+		return false, err
+	}
+	return common.HandleUpstreamClusterCleanup(ctx, p.C, database, &psmdbv1.PerconaServerMongoDB{})
+}
+
+func ensurePSMDBPitrDisabled(ctx context.Context,
+	c client.Client,
+	database *everestv1alpha1.DatabaseCluster,
+) error {
+	psmdb := &psmdbv1.PerconaServerMongoDB{}
+	err := c.Get(ctx, types.NamespacedName{
+		Name:      database.Name,
+		Namespace: database.Namespace,
+	}, psmdb)
+	if err != nil && !k8serrors.IsNotFound(err) {
+		return err
+	}
+	if !psmdb.Spec.Backup.PITR.Enabled {
+		return nil
+	}
+	psmdb.Spec.Backup.PITR.Enabled = false
+	return client.IgnoreNotFound(c.Update(ctx, psmdb))
 }
 
 // DBObject returns the PerconaServerMongoDB object.

controllers/providers/pxc/provider.go

@@ -225,7 +225,7 @@ func (p *Provider) Status(ctx context.Context) (everestv1alpha1.DatabaseClusterS
 
 // Cleanup runs the cleanup routines and returns true if the cleanup is done.
 func (p *Provider) Cleanup(ctx context.Context, database *everestv1alpha1.DatabaseCluster) (bool, error) {
-	return common.HandleDBBackupsCleanup(ctx, p.C, database)
+	return common.HandleUpstreamClusterCleanup(ctx, p.C, database, &pxcv1.PerconaXtraDBCluster{})
 }
 
 // DBObject returns the PerconaXtraDBCluster object.

e2e-tests/tests/core/pg/10-assert.yaml

@@ -9,7 +9,7 @@ kind: DatabaseCluster
 metadata:
   name: test-pg-cluster
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   engine:

e2e-tests/tests/core/psmdb/10-create-cluster.yaml

@@ -6,7 +6,7 @@ kind: DatabaseCluster
 metadata:
   name: test-psmdb-cluster
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   engine:

e2e-tests/tests/core/pxc/10-assert.yaml

@@ -9,7 +9,7 @@ kind: DatabaseCluster
 metadata:
   name: test-pxc-cluster
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   engine:

e2e-tests/tests/features/dbbackup_pg/10-assert.yaml

@@ -7,7 +7,7 @@ kind: DatabaseCluster
 metadata:
   name: test-pg-cluster
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   engine:

e2e-tests/tests/features/dbbackup_psmdb/10-assert.yaml

@@ -7,7 +7,7 @@ kind: DatabaseCluster
 metadata:
   name: test-psmdb-cluster
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   engine:

e2e-tests/tests/features/dbbackup_pxc/10-assert.yaml

@@ -7,7 +7,7 @@ kind: DatabaseCluster
 metadata:
   name: test-pxc-cluster
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   engine:

e2e-tests/tests/features/monitoringconfig_pg/01-assert.yaml

@@ -7,7 +7,7 @@ kind: DatabaseCluster
 metadata:
   name: pg-mc
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   monitoring:

e2e-tests/tests/features/monitoringconfig_psmdb/01-assert.yaml

@@ -7,7 +7,7 @@ kind: DatabaseCluster
 metadata:
   name: mongo-mc
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   monitoring:

e2e-tests/tests/features/monitoringconfig_pxc/01-assert.yaml

@@ -7,7 +7,7 @@ kind: DatabaseCluster
 metadata:
   name: pxc-mc
   finalizers:
-  - everest.percona.com/dbb-cleanup
+  - everest.percona.com/upstream-cluster-cleanup
   - foregroundDeletion
 spec:
   monitoring:

e2e-tests/tests/features/multinamespace_pg/90-delete-clusters.yaml

@@ -2,6 +2,8 @@ apiVersion: kuttl.dev/v1
 kind: TestStep
 commands:
   - command: kubectl -n $NAMESPACE delete db test-pg-cluster
+  - command: kubectl patch db test-pg-cluster -n $NAMESPACE -p '{"metadata":{"finalizers":null}}' --type merge
+    ignoreFailure: true
   - command: kubectl patch postgresclusters test-pg-cluster -n $NAMESPACE -p '{"metadata":{"finalizers":null}}' --type merge
     ignoreFailure: true
   - command: kubectl delete monitoringconfig test-mc -n $NAMESPACE

e2e-tests/tests/features/multinamespace_psmdb/90-delete-cluster.yaml

@@ -2,6 +2,8 @@ apiVersion: kuttl.dev/v1
 kind: TestStep
 commands:
   - command: kubectl -n $NAMESPACE delete db test-psmdb-cluster
+  - command: kubectl patch db test-psmdb-cluster -n $NAMESPACE -p '{"metadata":{"finalizers":null}}' --type merge
+    ignoreFailure: true
   - command: kubectl patch psmdb test-psmdb-cluster -n $NAMESPACE -p '{"metadata":{"finalizers":null}}' --type merge
     ignoreFailure: true
   - command: kubectl delete monitoringconfig test-mc -n $NAMESPACE