K8SPS-535: fix more delete-backup finalizer issues

cmd/sidecar/handler/backup/delete_test.go

@@ -24,13 +24,13 @@ type fakeStorageClient struct {
 }
 
 func (c *fakeStorageClient) ListObjects(_ context.Context, destination string) ([]string, error) {
-	if destination == "non-existing-backup" {
+	if destination == "non-existing-backup/" {
 		return nil, nil
 	}
 	switch destination {
-	case "non-existing-backup":
+	case "non-existing-backup/":
 		return nil, nil
-	case "existing-backup":
+	case "existing-backup/":
 		return []string{"some-dest/backup1", "some-dest/backup2"}, nil
 	default:
 		return nil, errors.New("fake list objects error")

cmd/sidecar/handler/backup/handler.go

@@ -64,7 +64,7 @@ func (h *Handler) backupExists(ctx context.Context, cfg *xb.BackupConfig) (bool,
 	if err != nil {
 		return false, errors.Wrap(err, "new storage")
 	}
-	objects, err := storage.ListObjects(ctx, cfg.Destination)
+	objects, err := storage.ListObjects(ctx, cfg.Destination+"/")
 	if err != nil {
 		return false, errors.Wrap(err, "list objects")
 	}

pkg/controller/psbackup/controller.go

@@ -478,20 +478,13 @@ func (r *PerconaServerMySQLBackupReconciler) runPostFinishTasks(
 }
 
 func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context, cr *apiv1.PerconaServerMySQLBackup) error {
-	if cr.DeletionTimestamp == nil || cr.Status.State == apiv1.BackupStarting || cr.Status.State == apiv1.BackupRunning {
+	if cr.DeletionTimestamp == nil || cr.Status.State == apiv1.BackupStarting || cr.Status.State == apiv1.BackupRunning || len(cr.Finalizers) == 0 {
 		return nil
 	}
 	log := logf.FromContext(ctx).WithName("checkFinalizers")
 
 	finalizers := sets.NewString()
 
-	switch cr.Status.State {
-	case apiv1.BackupError, apiv1.BackupNew:
-		if cr.Finalizers == nil {
-			return nil
-		}
-	}
-
 	for _, finalizer := range cr.GetFinalizers() {
 		var err error
 		switch finalizer {
@@ -510,12 +503,22 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context
 		}
 	}
 
-	cr.Finalizers = finalizers.List()
-	if len(cr.Finalizers) == 0 {
-		cr.Finalizers = nil
-	}
+	err := k8sretry.OnError(k8sretry.DefaultRetry, func(error) bool { return true }, func() error {
+		c := new(apiv1.PerconaServerMySQLBackup)
+		if err := r.Get(ctx, client.ObjectKeyFromObject(cr), c); err != nil {
+			if k8serrors.IsNotFound(err) {
+				return nil
+			}
+			return errors.Wrapf(err, "get %v", client.ObjectKeyFromObject(cr).String())
+		}
 
-	if err := r.Update(ctx, cr); err != nil {
+		c.Finalizers = finalizers.List()
+		if len(c.Finalizers) == 0 {
+			c.Finalizers = nil
+		}
+		return r.Update(ctx, c)
+	})
+	if err != nil {
 		return errors.Wrap(err, "failed to update finalizers for backup")
 	}
 

pkg/controller/psbackup/controller_test.go

@@ -281,19 +281,13 @@ func TestStateDescCleanup(t *testing.T) {
 }
 
 func TestCheckFinalizers(t *testing.T) {
-	ctx := context.Background()
 	scheme := runtime.NewScheme()
-	if err := clientgoscheme.AddToScheme(scheme); err != nil {
-		t.Fatal(err, "failed to add client-go scheme")
-	}
-	if err := apiv1.AddToScheme(scheme); err != nil {
-		t.Fatal(err, "failed to add apis scheme")
-	}
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, apiv1.AddToScheme(scheme))
 	namespace := "some-namespace"
+
 	cr, err := readDefaultCRBackup("some-name", namespace)
-	if err != nil {
-		t.Fatal(err, "failed to read default backup")
-	}
+	require.NoError(t, err)
 
 	tests := []struct {
 		name               string
@@ -414,9 +408,10 @@ func TestCheckFinalizers(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			tt.cr.Status.Storage = storage
+			cr := tt.cr.DeepCopy()
+			cr.Status.Storage = storage
 
-			job := xtrabackup.GetDeleteJob(new(apiv1.PerconaServerMySQL), tt.cr, new(xtrabackup.BackupConfig))
+			job := xtrabackup.GetDeleteJob(new(apiv1.PerconaServerMySQL), cr, new(xtrabackup.BackupConfig))
 			cond := batchv1.JobCondition{
 				Type:   batchv1.JobComplete,
 				Status: corev1.ConditionTrue,
@@ -426,25 +421,33 @@ func TestCheckFinalizers(t *testing.T) {
 			}
 			job.Status.Conditions = append(job.Status.Conditions, cond)
 
-			cb := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.cr, sec, job)
+			cb := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cr, sec, job)
 			r := PerconaServerMySQLBackupReconciler{
 				Client:        cb.Build(),
 				Scheme:        scheme,
 				ServerVersion: &platform.ServerVersion{Platform: platform.PlatformKubernetes},
 			}
-			err := r.Delete(ctx, tt.cr)
-			if err != nil {
+
+			require.NoError(t, r.Delete(t.Context(), cr))
+
+			// Getting backup with deletion timestamp
+			if err := r.Get(t.Context(), client.ObjectKeyFromObject(cr), cr); err != nil {
+				if k8serrors.IsNotFound(err) && len(tt.expectedFinalizers) == 0 {
+					return
+				}
 				t.Fatal(err)
 			}
-			cr := new(apiv1.PerconaServerMySQLBackup)
-			if err := r.Get(ctx, types.NamespacedName{Name: tt.cr.Name, Namespace: tt.cr.Namespace}, cr); err != nil {
-				if k8serrors.IsNotFound(err) && len(cr.Finalizers) == 0 {
+
+			assert.NoError(t, r.checkFinalizers(t.Context(), cr))
+
+			// Getting backup with updated finalizers
+			if err := r.Get(t.Context(), client.ObjectKeyFromObject(cr), cr); err != nil {
+				if k8serrors.IsNotFound(err) && len(tt.expectedFinalizers) == 0 {
 					return
 				}
 				t.Fatal(err)
 			}
 
-			r.checkFinalizers(ctx, cr)
 			assert.Equal(t, tt.expectedFinalizers, cr.Finalizers)
 		})
 	}