K8SPS-257: fix backup deletion in error state (#383)

https://jira.percona.com/browse/K8SPS-257

Author: pooknull

pkg/controller/psbackup/controller.go

@@ -337,8 +337,9 @@ func (r *PerconaServerMySQLBackupReconciler) getBackupSource(ctx context.Context
 	return source, nil
 }
 
-func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context,
-	cr *apiv1alpha1.PerconaServerMySQLBackup) {
+const finalizerDeleteBackup = "delete-backup"
+
+func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context, cr *apiv1alpha1.PerconaServerMySQLBackup) {
 	if cr.DeletionTimestamp == nil || cr.Status.State == apiv1alpha1.BackupStarting || cr.Status.State == apiv1alpha1.BackupRunning {
 		return
 	}
@@ -350,7 +351,8 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context
 		}
 	}()
 
-	if cr.Status.State == apiv1alpha1.BackupNew {
+	switch cr.Status.State {
+	case apiv1alpha1.BackupError, apiv1alpha1.BackupNew:
 		cr.Finalizers = nil
 		return
 	}
@@ -359,7 +361,7 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context
 	for _, finalizer := range cr.GetFinalizers() {
 		var err error
 		switch finalizer {
-		case "delete-backup":
+		case finalizerDeleteBackup:
 			var ok bool
 			ok, err = r.deleteBackup(ctx, cr)
 			if !ok {
@@ -403,11 +405,11 @@ func (r *PerconaServerMySQLBackupReconciler) backupConfig(ctx context.Context, c
 		}
 		accessKey, ok := s.Data[secret.CredentialsAWSAccessKey]
 		if !ok {
-			return nil, errors.Errorf("no credentials for Azure in secret %s", nn.Name)
+			return nil, errors.Errorf("no credentials for S3 in secret %s", nn.Name)
 		}
 		secretKey, ok := s.Data[secret.CredentialsAWSSecretKey]
 		if !ok {
-			return nil, errors.Errorf("no credentials for Azure in secret %s", nn.Name)
+			return nil, errors.Errorf("no credentials for S3 in secret %s", nn.Name)
 		}
 		conf.S3.Bucket = s3.Bucket.Bucket()
 		conf.S3.Region = s3.Region
@@ -424,11 +426,11 @@ func (r *PerconaServerMySQLBackupReconciler) backupConfig(ctx context.Context, c
 		}
 		accessKey, ok := s.Data[secret.CredentialsGCSAccessKey]
 		if !ok {
-			return nil, errors.Errorf("no credentials for Azure in secret %s", nn.Name)
+			return nil, errors.Errorf("no credentials for GCS in secret %s", nn.Name)
 		}
 		secretKey, ok := s.Data[secret.CredentialsGCSSecretKey]
 		if !ok {
-			return nil, errors.Errorf("no credentials for Azure in secret %s", nn.Name)
+			return nil, errors.Errorf("no credentials for GCS in secret %s", nn.Name)
 		}
 		conf.GCS.Bucket = gcs.Bucket.Bucket()
 		conf.GCS.EndpointURL = gcs.EndpointURL

pkg/controller/psbackup/controller_test.go

@@ -5,8 +5,13 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"testing"
+	"time"
 
+	batchv1 "k8s.io/api/batch/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/yaml"
@@ -16,6 +21,8 @@ import (
 
 	apiv1alpha1 "github.com/percona/percona-server-mysql-operator/api/v1alpha1"
 	"github.com/percona/percona-server-mysql-operator/pkg/platform"
+	"github.com/percona/percona-server-mysql-operator/pkg/secret"
+	"github.com/percona/percona-server-mysql-operator/pkg/xtrabackup"
 )
 
 func TestBackupStatusErrStateDesc(t *testing.T) {
@@ -125,6 +132,168 @@ func TestBackupStatusErrStateDesc(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 := apiv1alpha1.AddToScheme(scheme); err != nil {
+		t.Fatal(err, "failed to add apis scheme")
+	}
+	namespace := "some-namespace"
+	cr, err := readDefaultCRBackup("some-name", namespace)
+	if err != nil {
+		t.Fatal(err, "failed to read default backup")
+	}
+
+	tests := []struct {
+		name               string
+		cr                 *apiv1alpha1.PerconaServerMySQLBackup
+		expectedFinalizers []string
+		finalizerJobFail   bool
+	}{
+		{
+			name: "without finalizers",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{}
+				cr.Status.State = apiv1alpha1.BackupError
+			}),
+			expectedFinalizers: nil,
+		},
+		{
+			name: "with finalizer and starting state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupStarting
+			}),
+			expectedFinalizers: []string{finalizerDeleteBackup},
+		},
+		{
+			name: "with finalizer and running state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupRunning
+			}),
+			expectedFinalizers: []string{finalizerDeleteBackup},
+		},
+		{
+			name: "with finalizer and error state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupError
+			}),
+			expectedFinalizers: nil,
+		},
+		{
+			name: "with finalizer and new state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupNew
+			}),
+			expectedFinalizers: nil,
+		},
+		{
+			name: "with failing finalizer and succeeded state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupSucceeded
+			}),
+			finalizerJobFail:   true,
+			expectedFinalizers: []string{finalizerDeleteBackup},
+		},
+		{
+			name: "with successful finalizer and succeeded state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupSucceeded
+			}),
+			expectedFinalizers: []string{},
+		},
+		{
+			name: "with successful finalizer, unknown finalizer and succeeded state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup, "unknown-finalizer"}
+				cr.Status.State = apiv1alpha1.BackupSucceeded
+			}),
+			expectedFinalizers: []string{"unknown-finalizer"},
+		},
+		{
+			name: "with failing finalizer and failed state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupFailed
+			}),
+			finalizerJobFail:   true,
+			expectedFinalizers: []string{finalizerDeleteBackup},
+		},
+		{
+			name: "with successful finalizer and failed state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup}
+				cr.Status.State = apiv1alpha1.BackupFailed
+			}),
+			expectedFinalizers: []string{},
+		},
+		{
+			name: "with successful finalizer, unknown finalizer and failed state",
+			cr: updateResource(cr.DeepCopy(), func(cr *apiv1alpha1.PerconaServerMySQLBackup) {
+				cr.Finalizers = []string{finalizerDeleteBackup, "unknown-finalizer"}
+				cr.Status.State = apiv1alpha1.BackupFailed
+			}),
+			expectedFinalizers: []string{"unknown-finalizer"},
+		},
+	}
+
+	sec := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "some-secret",
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{
+			secret.CredentialsAWSAccessKey: []byte("access-key"),
+			secret.CredentialsAWSSecretKey: []byte("secret-key"),
+		},
+	}
+	storage := &apiv1alpha1.BackupStorageSpec{
+		Type: apiv1alpha1.BackupStorageS3,
+		S3: &apiv1alpha1.BackupStorageS3Spec{
+			Bucket:            "some-bucket",
+			CredentialsSecret: "some-secret",
+		},
+		Labels: make(map[string]string),
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.cr.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+			tt.cr.Status.Storage = storage
+
+			job := xtrabackup.GetDeleteJob(tt.cr, new(xtrabackup.BackupConfig))
+			cond := batchv1.JobCondition{
+				Type:   batchv1.JobComplete,
+				Status: corev1.ConditionTrue,
+			}
+			if tt.finalizerJobFail {
+				cond.Type = batchv1.JobFailed
+			}
+			job.Status.Conditions = append(job.Status.Conditions, cond)
+
+			cb := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.cr, sec, job)
+			r := PerconaServerMySQLBackupReconciler{
+				Client:        cb.Build(),
+				Scheme:        scheme,
+				ServerVersion: &platform.ServerVersion{Platform: platform.PlatformKubernetes},
+			}
+
+			r.checkFinalizers(ctx, tt.cr)
+			if !reflect.DeepEqual(tt.cr.Finalizers, tt.expectedFinalizers) {
+				t.Fatalf("expected finalizers %v, got %v", tt.expectedFinalizers, tt.cr.Finalizers)
+			}
+		})
+	}
+}
+
 func readDefaultCR(name, namespace string) (*apiv1alpha1.PerconaServerMySQL, error) {
 	data, err := os.ReadFile(filepath.Join("..", "..", "..", "deploy", "cr.yaml"))
 	if err != nil {