From 30786ee4d82198b5e64c558b4e70b53c5925b5cd Mon Sep 17 00:00:00 2001 From: Andrii Dema Date: Fri, 3 Oct 2025 15:16:11 +0300 Subject: [PATCH 1/5] K8SPS-535: fix more `delete-backup` finalizer issues https://perconadev.atlassian.net/browse/K8SPS-535 --- cmd/sidecar/handler/backup/handler.go | 2 +- pkg/controller/psbackup/controller.go | 33 ++++++++++++++++----------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/cmd/sidecar/handler/backup/handler.go b/cmd/sidecar/handler/backup/handler.go index 42f5f86e2..073b120d4 100644 --- a/cmd/sidecar/handler/backup/handler.go +++ b/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") } diff --git a/pkg/controller/psbackup/controller.go b/pkg/controller/psbackup/controller.go index be32b4992..99a1d19f5 100644 --- a/pkg/controller/psbackup/controller.go +++ b/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.Client.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.Client.Update(ctx, c) + }) + if err != nil { return errors.Wrap(err, "failed to update finalizers for backup") } @@ -524,6 +527,7 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, cr *apiv1.PerconaServerMySQLBackup) (bool, error) { if cr.Status.State != apiv1.BackupSucceeded { + fmt.Println("TEST 4", cr.Name) return true, nil } @@ -557,6 +561,7 @@ func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, c if err := r.Client.Create(ctx, job); err != nil { return false, errors.Wrapf(err, "create job %s/%s", job.Namespace, job.Name) } + fmt.Println("TEST 1", cr.Name) return false, nil } @@ -573,6 +578,7 @@ func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, c complete = true } } + fmt.Println("TEST 2", cr.Name, complete) return complete, nil } @@ -586,6 +592,7 @@ func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, c if err := sc.DeleteBackup(ctx, cr.Name, *backupConf); err != nil { return false, errors.Wrap(err, "delete backup") } + fmt.Println("TEST 3", cr.Name) return true, nil } From 3d424a71139d273872100e9d27ff8c1a3050f93b Mon Sep 17 00:00:00 2001 From: Andrii Dema Date: Fri, 3 Oct 2025 15:17:04 +0300 Subject: [PATCH 2/5] remove debug log --- pkg/controller/psbackup/controller.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/controller/psbackup/controller.go b/pkg/controller/psbackup/controller.go index 99a1d19f5..9ef5fb636 100644 --- a/pkg/controller/psbackup/controller.go +++ b/pkg/controller/psbackup/controller.go @@ -527,7 +527,6 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, cr *apiv1.PerconaServerMySQLBackup) (bool, error) { if cr.Status.State != apiv1.BackupSucceeded { - fmt.Println("TEST 4", cr.Name) return true, nil } @@ -561,7 +560,6 @@ func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, c if err := r.Client.Create(ctx, job); err != nil { return false, errors.Wrapf(err, "create job %s/%s", job.Namespace, job.Name) } - fmt.Println("TEST 1", cr.Name) return false, nil } @@ -578,7 +576,6 @@ func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, c complete = true } } - fmt.Println("TEST 2", cr.Name, complete) return complete, nil } @@ -592,7 +589,6 @@ func (r *PerconaServerMySQLBackupReconciler) deleteBackup(ctx context.Context, c if err := sc.DeleteBackup(ctx, cr.Name, *backupConf); err != nil { return false, errors.Wrap(err, "delete backup") } - fmt.Println("TEST 3", cr.Name) return true, nil } From ee033892029a623ae34106d73f1bb30c403450b5 Mon Sep 17 00:00:00 2001 From: Andrii Dema Date: Fri, 3 Oct 2025 16:36:43 +0300 Subject: [PATCH 3/5] fix unit-test --- cmd/sidecar/handler/backup/delete_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/sidecar/handler/backup/delete_test.go b/cmd/sidecar/handler/backup/delete_test.go index 9e3c83172..65adc342f 100644 --- a/cmd/sidecar/handler/backup/delete_test.go +++ b/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") From d28352f864c2bafc55c4f44214005856e6c1b259 Mon Sep 17 00:00:00 2001 From: Andrii Dema Date: Fri, 3 Oct 2025 17:05:18 +0300 Subject: [PATCH 4/5] fix lint --- pkg/controller/psbackup/controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/psbackup/controller.go b/pkg/controller/psbackup/controller.go index 9ef5fb636..cf8fc51c4 100644 --- a/pkg/controller/psbackup/controller.go +++ b/pkg/controller/psbackup/controller.go @@ -505,7 +505,7 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context err := k8sretry.OnError(k8sretry.DefaultRetry, func(error) bool { return true }, func() error { c := new(apiv1.PerconaServerMySQLBackup) - if err := r.Client.Get(ctx, client.ObjectKeyFromObject(cr), c); err != nil { + if err := r.Get(ctx, client.ObjectKeyFromObject(cr), c); err != nil { if k8serrors.IsNotFound(err) { return nil } @@ -516,7 +516,7 @@ func (r *PerconaServerMySQLBackupReconciler) checkFinalizers(ctx context.Context if len(c.Finalizers) == 0 { c.Finalizers = nil } - return r.Client.Update(ctx, c) + return r.Update(ctx, c) }) if err != nil { return errors.Wrap(err, "failed to update finalizers for backup") From 032e2bbca22d8739fc41408fcef6da068985ac82 Mon Sep 17 00:00:00 2001 From: Andrii Dema Date: Fri, 3 Oct 2025 17:43:41 +0300 Subject: [PATCH 5/5] fix unit-test --- pkg/controller/psbackup/controller_test.go | 41 ++++++++++++---------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/pkg/controller/psbackup/controller_test.go b/pkg/controller/psbackup/controller_test.go index 9c762301f..7520d909f 100644 --- a/pkg/controller/psbackup/controller_test.go +++ b/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) }) }