From 03ef0addb6e996064b1262a489ed8b10d8294680 Mon Sep 17 00:00:00 2001 From: Oksana Grishchenko <91597950+oksana-grishchenko@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:01:21 +0200 Subject: [PATCH] EVEREST-1141 Backup storage files deletion fix (#428) Co-authored-by: Mayank Shah --- controllers/common/consts.go | 4 +- controllers/common/helper.go | 58 +++++++++--------- controllers/databasecluster_controller.go | 21 +++++-- .../databaseclusterbackup_controller.go | 61 ++++++++++++++++--- controllers/providers/pg/provider.go | 2 +- controllers/providers/psmdb/provider.go | 28 ++++++++- controllers/providers/pxc/provider.go | 2 +- e2e-tests/tests/core/pg/10-assert.yaml | 2 +- .../tests/core/psmdb/10-create-cluster.yaml | 2 +- e2e-tests/tests/core/pxc/10-assert.yaml | 2 +- .../tests/features/dbbackup_pg/10-assert.yaml | 2 +- .../features/dbbackup_psmdb/10-assert.yaml | 2 +- .../features/dbbackup_pxc/10-assert.yaml | 2 +- .../monitoringconfig_pg/01-assert.yaml | 2 +- .../monitoringconfig_psmdb/01-assert.yaml | 2 +- .../monitoringconfig_pxc/01-assert.yaml | 2 +- .../multinamespace_pg/90-delete-clusters.yaml | 2 + .../90-delete-cluster.yaml | 2 + .../multinamespace_pxc/90-delete-cluster.yaml | 2 + 19 files changed, 142 insertions(+), 58 deletions(-) diff --git a/controllers/common/consts.go b/controllers/common/consts.go index 21feb5fc2..11eecd03e 100644 --- a/controllers/common/consts.go +++ b/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" diff --git a/controllers/common/helper.go b/controllers/common/helper.go index d05607f52..b8f58abb6 100644 --- a/controllers/common/helper.go +++ b/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. diff --git a/controllers/databasecluster_controller.go b/controllers/databasecluster_controller.go index 872f76dae..81ae16616 100644 --- a/controllers/databasecluster_controller.go +++ b/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 { diff --git a/controllers/databaseclusterbackup_controller.go b/controllers/databaseclusterbackup_controller.go index 8ab0efecb..183584a6d 100644 --- a/controllers/databaseclusterbackup_controller.go +++ b/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) } diff --git a/controllers/providers/pg/provider.go b/controllers/providers/pg/provider.go index 3124318b8..74818d525 100644 --- a/controllers/providers/pg/provider.go +++ b/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. diff --git a/controllers/providers/psmdb/provider.go b/controllers/providers/psmdb/provider.go index 23134e5a3..4840864c8 100644 --- a/controllers/providers/psmdb/provider.go +++ b/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. diff --git a/controllers/providers/pxc/provider.go b/controllers/providers/pxc/provider.go index 9f75e21d4..d4e3b8135 100644 --- a/controllers/providers/pxc/provider.go +++ b/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. diff --git a/e2e-tests/tests/core/pg/10-assert.yaml b/e2e-tests/tests/core/pg/10-assert.yaml index 0068fc3d9..1e5fe3aee 100644 --- a/e2e-tests/tests/core/pg/10-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/core/psmdb/10-create-cluster.yaml b/e2e-tests/tests/core/psmdb/10-create-cluster.yaml index 4c19340d8..0518124b5 100644 --- a/e2e-tests/tests/core/psmdb/10-create-cluster.yaml +++ b/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: diff --git a/e2e-tests/tests/core/pxc/10-assert.yaml b/e2e-tests/tests/core/pxc/10-assert.yaml index 6aeae5ab8..596d737da 100644 --- a/e2e-tests/tests/core/pxc/10-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/dbbackup_pg/10-assert.yaml b/e2e-tests/tests/features/dbbackup_pg/10-assert.yaml index c76238080..357e23de4 100644 --- a/e2e-tests/tests/features/dbbackup_pg/10-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/dbbackup_psmdb/10-assert.yaml b/e2e-tests/tests/features/dbbackup_psmdb/10-assert.yaml index 1670d33a7..d107ed5df 100644 --- a/e2e-tests/tests/features/dbbackup_psmdb/10-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/dbbackup_pxc/10-assert.yaml b/e2e-tests/tests/features/dbbackup_pxc/10-assert.yaml index 1f793b324..8e902bd02 100644 --- a/e2e-tests/tests/features/dbbackup_pxc/10-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/monitoringconfig_pg/01-assert.yaml b/e2e-tests/tests/features/monitoringconfig_pg/01-assert.yaml index 17e84937d..94c583eeb 100644 --- a/e2e-tests/tests/features/monitoringconfig_pg/01-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/monitoringconfig_psmdb/01-assert.yaml b/e2e-tests/tests/features/monitoringconfig_psmdb/01-assert.yaml index b747a49ff..5d359011a 100644 --- a/e2e-tests/tests/features/monitoringconfig_psmdb/01-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/monitoringconfig_pxc/01-assert.yaml b/e2e-tests/tests/features/monitoringconfig_pxc/01-assert.yaml index d418acb2f..f4e63be7d 100644 --- a/e2e-tests/tests/features/monitoringconfig_pxc/01-assert.yaml +++ b/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: diff --git a/e2e-tests/tests/features/multinamespace_pg/90-delete-clusters.yaml b/e2e-tests/tests/features/multinamespace_pg/90-delete-clusters.yaml index 976d6fa44..2632318a4 100644 --- a/e2e-tests/tests/features/multinamespace_pg/90-delete-clusters.yaml +++ b/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 diff --git a/e2e-tests/tests/features/multinamespace_psmdb/90-delete-cluster.yaml b/e2e-tests/tests/features/multinamespace_psmdb/90-delete-cluster.yaml index f2646a0b0..c3a27b887 100644 --- a/e2e-tests/tests/features/multinamespace_psmdb/90-delete-cluster.yaml +++ b/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 diff --git a/e2e-tests/tests/features/multinamespace_pxc/90-delete-cluster.yaml b/e2e-tests/tests/features/multinamespace_pxc/90-delete-cluster.yaml index 5263d9860..bdb7fe740 100644 --- a/e2e-tests/tests/features/multinamespace_pxc/90-delete-cluster.yaml +++ b/e2e-tests/tests/features/multinamespace_pxc/90-delete-cluster.yaml @@ -2,6 +2,8 @@ apiVersion: kuttl.dev/v1 kind: TestStep commands: - command: kubectl -n $NAMESPACE delete db test-pxc-cluster + - command: kubectl patch db test-pxc-cluster -n $NAMESPACE -p '{"metadata":{"finalizers":null}}' --type merge + ignoreFailure: true - command: kubectl patch pxc test-pxc-cluster -n $NAMESPACE -p '{"metadata":{"finalizers":null}}' --type merge ignoreFailure: true - command: kubectl delete monitoringconfig test-mc -n $NAMESPACE