fix(dataprotection): surface terminating namespace blocker

Author: weicao

controllers/dataprotection/backup_controller.go

@@ -21,6 +21,7 @@ package dataprotection
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 	"strings"
@@ -71,6 +72,14 @@ type BackupReconciler struct {
 	clock      clock.RealClock
 }
 
+type backupNamespaceTerminatingError struct {
+	namespace string
+}
+
+func (e *backupNamespaceTerminatingError) Error() string {
+	return fmt.Sprintf("backup namespace %q is terminating; cannot create worker resources to delete backup files, delete the Backup and wait until it is gone before deleting the namespace", e.namespace)
+}
+
 // +kubebuilder:rbac:groups=dataprotection.kubeblocks.io,resources=backups,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=dataprotection.kubeblocks.io,resources=backups/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=dataprotection.kubeblocks.io,resources=backups/finalizers,verbs=update
@@ -248,8 +257,30 @@ func (r *BackupReconciler) deleteBackupFiles(reqCtx intctrlutil.RequestCtx, back
 	case dpbackup.DeletionStatusDeleting,
 		dpbackup.DeletionStatusUnknown:
 		// wait for the deletion job completed
+		return r.recordBackupDeletionBlocker(reqCtx, backup, err)
+	}
+	return err
+}
+
+func (r *BackupReconciler) recordBackupDeletionBlocker(
+	reqCtx intctrlutil.RequestCtx,
+	backup *dpv1alpha1.Backup,
+	err error,
+) error {
+	var terminatingErr *backupNamespaceTerminatingError
+	if !errors.As(err, &terminatingErr) {
 		return err
 	}
+
+	failureReason := err.Error()
+	if backup.Status.FailureReason == failureReason {
+		return err
+	}
+	backupPatch := client.MergeFrom(backup.DeepCopy())
+	backup.Status.FailureReason = failureReason
+	if patchErr := r.Status().Patch(reqCtx.Ctx, backup, backupPatch); patchErr != nil {
+		return patchErr
+	}
 	return err
 }
 
@@ -259,7 +290,7 @@ func (r *BackupReconciler) ensureWorkerServiceAccountForBackupDeletion(reqCtx in
 		return "", fmt.Errorf("failed to get backup namespace %q before deleting backup files: %w", namespace, err)
 	}
 	if !ns.DeletionTimestamp.IsZero() {
-		return "", fmt.Errorf("backup namespace %q is terminating; cannot create worker resources to delete backup files, delete the Backup and wait until it is gone before deleting the namespace", namespace)
+		return "", &backupNamespaceTerminatingError{namespace: namespace}
 	}
 	// TODO: update the mcMgr param
 	return EnsureWorkerServiceAccount(reqCtx, r.Client, namespace, nil)

controllers/dataprotection/backup_controller_test.go

@@ -22,6 +22,7 @@ package dataprotection
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"slices"
 	"strconv"
@@ -35,6 +36,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -46,6 +48,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	kbappsv1 "github.com/apecloud/kubeblocks/apis/apps/v1"
 	dpv1alpha1 "github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1"
@@ -70,6 +73,178 @@ func (c *getErrorClient) Get(context.Context, client.ObjectKey, client.Object, .
 	return fmt.Errorf("get failed")
 }
 
+type statusPatchCountingClient struct {
+	client.Client
+	statusPatchCount int
+	statusPatchErr   error
+}
+
+func (c *statusPatchCountingClient) Status() client.SubResourceWriter {
+	return &statusPatchCountingWriter{
+		SubResourceWriter: c.Client.Status(),
+		count:             &c.statusPatchCount,
+		patchErr:          c.statusPatchErr,
+	}
+}
+
+type statusPatchCountingWriter struct {
+	client.SubResourceWriter
+	count    *int
+	patchErr error
+}
+
+func (w *statusPatchCountingWriter) Patch(
+	ctx context.Context,
+	obj client.Object,
+	patch client.Patch,
+	opts ...client.SubResourcePatchOption,
+) error {
+	*w.count = *w.count + 1
+	if w.patchErr != nil {
+		return w.patchErr
+	}
+	return w.SubResourceWriter.Patch(ctx, obj, patch, opts...)
+}
+
+func TestDeleteBackupFilesRecordsTerminatingNamespaceBlockerWithoutStoppingRetry(t *testing.T) {
+	g := NewWithT(t)
+	scheme := runtime.NewScheme()
+	g.Expect(corev1.AddToScheme(scheme)).To(Succeed())
+	g.Expect(batchv1.AddToScheme(scheme)).To(Succeed())
+	g.Expect(rbacv1.AddToScheme(scheme)).To(Succeed())
+	g.Expect(dpv1alpha1.AddToScheme(scheme)).To(Succeed())
+
+	now := metav1.Now()
+	namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{
+		Name:              "terminating-backup",
+		DeletionTimestamp: &now,
+		Finalizers:        []string{"test.kubeblocks.io/finalizer"},
+	}}
+	backup := &dpv1alpha1.Backup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "backup",
+			Namespace:         namespace.Name,
+			UID:               types.UID("backup-uid"),
+			DeletionTimestamp: &now,
+			Finalizers:        []string{dptypes.DataProtectionFinalizerName},
+		},
+		Spec: dpv1alpha1.BackupSpec{DeletionPolicy: dpv1alpha1.BackupDeletionPolicyDelete},
+		Status: dpv1alpha1.BackupStatus{
+			Phase:          dpv1alpha1.BackupPhaseDeleting,
+			BackupRepoName: "repo",
+			Path:           "/backups/backup",
+		},
+	}
+	repo := &dpv1alpha1.BackupRepo{ObjectMeta: metav1.ObjectMeta{Name: backup.Status.BackupRepoName}}
+	baseClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithStatusSubresource(&dpv1alpha1.Backup{}).
+		WithObjects(namespace, backup, repo).
+		Build()
+	countingClient := &statusPatchCountingClient{Client: baseClient}
+	reconciler := &BackupReconciler{Client: countingClient, Scheme: scheme}
+	reqCtx := intctrlutil.RequestCtx{Ctx: context.Background()}
+
+	err := reconciler.deleteBackupFiles(reqCtx, backup.DeepCopy())
+	var terminatingErr *backupNamespaceTerminatingError
+	g.Expect(errors.As(err, &terminatingErr)).To(BeTrue())
+	g.Expect(err.Error()).To(ContainSubstring("is terminating; cannot create worker resources to delete backup files"))
+	g.Expect(countingClient.statusPatchCount).To(Equal(1))
+
+	stored := &dpv1alpha1.Backup{}
+	g.Expect(baseClient.Get(reqCtx.Ctx, client.ObjectKeyFromObject(backup), stored)).To(Succeed())
+	g.Expect(stored.Status.FailureReason).To(Equal(err.Error()))
+	g.Expect(controllerutil.ContainsFinalizer(stored, dptypes.DataProtectionFinalizerName)).To(BeTrue())
+
+	jobs := &batchv1.JobList{}
+	g.Expect(baseClient.List(reqCtx.Ctx, jobs, client.InNamespace(namespace.Name))).To(Succeed())
+	g.Expect(jobs.Items).To(BeEmpty())
+	serviceAccounts := &corev1.ServiceAccountList{}
+	g.Expect(baseClient.List(reqCtx.Ctx, serviceAccounts, client.InNamespace(namespace.Name))).To(Succeed())
+	g.Expect(serviceAccounts.Items).To(BeEmpty())
+	roleBindings := &rbacv1.RoleBindingList{}
+	g.Expect(baseClient.List(reqCtx.Ctx, roleBindings, client.InNamespace(namespace.Name))).To(Succeed())
+	g.Expect(roleBindings.Items).To(BeEmpty())
+
+	err = reconciler.deleteBackupFiles(reqCtx, stored.DeepCopy())
+	g.Expect(errors.As(err, &terminatingErr)).To(BeTrue())
+	g.Expect(countingClient.statusPatchCount).To(Equal(1))
+}
+
+func TestDeleteBackupFilesDoesNotRecordOrdinaryDeletingError(t *testing.T) {
+	g := NewWithT(t)
+	scheme := runtime.NewScheme()
+	g.Expect(batchv1.AddToScheme(scheme)).To(Succeed())
+	g.Expect(dpv1alpha1.AddToScheme(scheme)).To(Succeed())
+
+	backup := &dpv1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{
+		Name:       "backup",
+		Namespace:  "default",
+		UID:        types.UID("backup-uid"),
+		Finalizers: []string{dptypes.DataProtectionFinalizerName},
+	}}
+	baseClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithStatusSubresource(&dpv1alpha1.Backup{}).
+		WithObjects(backup).
+		Build()
+	countingClient := &statusPatchCountingClient{Client: baseClient}
+	reconciler := &BackupReconciler{
+		Client: &getErrorClient{Client: countingClient},
+		Scheme: scheme,
+	}
+
+	err := reconciler.deleteBackupFiles(intctrlutil.RequestCtx{Ctx: context.Background()}, backup.DeepCopy())
+	g.Expect(err).To(MatchError("get failed"))
+	g.Expect(countingClient.statusPatchCount).To(Equal(0))
+	g.Expect(backup.Status.FailureReason).To(BeEmpty())
+}
+
+func TestDeleteBackupFilesReturnsStatusPatchError(t *testing.T) {
+	g := NewWithT(t)
+	scheme := runtime.NewScheme()
+	g.Expect(corev1.AddToScheme(scheme)).To(Succeed())
+	g.Expect(batchv1.AddToScheme(scheme)).To(Succeed())
+	g.Expect(dpv1alpha1.AddToScheme(scheme)).To(Succeed())
+
+	now := metav1.Now()
+	namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{
+		Name:              "terminating-backup",
+		DeletionTimestamp: &now,
+		Finalizers:        []string{"test.kubeblocks.io/finalizer"},
+	}}
+	backup := &dpv1alpha1.Backup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "backup",
+			Namespace:  namespace.Name,
+			UID:        types.UID("backup-uid"),
+			Finalizers: []string{dptypes.DataProtectionFinalizerName},
+		},
+		Status: dpv1alpha1.BackupStatus{
+			Phase:          dpv1alpha1.BackupPhaseDeleting,
+			BackupRepoName: "repo",
+			Path:           "/backups/backup",
+		},
+	}
+	repo := &dpv1alpha1.BackupRepo{ObjectMeta: metav1.ObjectMeta{Name: backup.Status.BackupRepoName}}
+	baseClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithStatusSubresource(&dpv1alpha1.Backup{}).
+		WithObjects(namespace, backup, repo).
+		Build()
+	patchErr := errors.New("status patch failed")
+	countingClient := &statusPatchCountingClient{Client: baseClient, statusPatchErr: patchErr}
+	reconciler := &BackupReconciler{Client: countingClient, Scheme: scheme}
+
+	err := reconciler.deleteBackupFiles(intctrlutil.RequestCtx{Ctx: context.Background()}, backup.DeepCopy())
+	g.Expect(err).To(MatchError(patchErr))
+	g.Expect(countingClient.statusPatchCount).To(Equal(1))
+
+	stored := &dpv1alpha1.Backup{}
+	g.Expect(baseClient.Get(context.Background(), client.ObjectKeyFromObject(backup), stored)).To(Succeed())
+	g.Expect(stored.Status.FailureReason).To(BeEmpty())
+}
+
 func TestSyncJobActions(t *testing.T) {
 	tests := []struct {
 		name           string