Skip to content

fix: keep backup deletion fail-closed during namespace termination #10631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
weicao wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

fix: keep backup deletion fail-closed during namespace termination #10631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7769a34
fix: make backup deletion finalization explicit
weicao Jul 13, 2026
ebef178
fix: retry backup cleanup after namespace restoration
weicao Jul 13, 2026
fc8e4f4
test: lock missing namespace recovery interval
weicao Jul 13, 2026
c66fe55
docs: update backup deletion policy reference
weicao Jul 13, 2026
99d0ef1
fix(dataprotection): keep backup deletion fail-closed
weicao Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 49 additions & 18 deletions controllers/dataprotection/backup_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package dataprotection

import (
"context"
"errors"
"fmt"
"reflect"
"strings"
Expand Down Expand Up @@ -70,6 +71,10 @@ type BackupReconciler struct {
clock clock.RealClock
}

var errBackupNamespaceTerminating = errors.New("backup namespace terminating")

const terminatingBackupNamespaceRetryInterval = 30 * time.Second

// +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
Expand Down Expand Up @@ -215,13 +220,6 @@ func (r *BackupReconciler) deleteBackupFiles(reqCtx intctrlutil.RequestCtx, back
return nil
}

deleteBackup := func() error {
// remove backup finalizers to delete it
patch := client.MergeFrom(backup.DeepCopy())
controllerutil.RemoveFinalizer(backup, dptypes.DataProtectionFinalizerName)
return r.Patch(reqCtx.Ctx, backup, patch)
}

deleter := &dpbackup.Deleter{
RequestCtx: reqCtx,
Client: r.Client,
Expand All @@ -234,31 +232,58 @@ func (r *BackupReconciler) deleteBackupFiles(reqCtx intctrlutil.RequestCtx, back
status, err := deleter.DeleteBackupFiles(backup)
switch status {
case dpbackup.DeletionStatusSucceeded:
return deleteBackup()
return r.removeBackupFinalizer(reqCtx, backup)
case dpbackup.DeletionStatusFailed:
failureReason := err.Error()
if backup.Status.FailureReason == failureReason {
return nil
}
backupPatch := client.MergeFrom(backup.DeepCopy())
backup.Status.FailureReason = failureReason
r.Recorder.Event(backup, corev1.EventTypeWarning, "DeleteBackupFilesFailed", failureReason)
return r.Status().Patch(reqCtx.Ctx, backup, backupPatch)
return r.recordDeleteBackupFilesFailure(reqCtx, backup, err.Error())
case dpbackup.DeletionStatusDeleting,
dpbackup.DeletionStatusUnknown:
if errors.Is(err, errBackupNamespaceTerminating) {
failureReason := fmt.Sprintf(
"backup namespace %q is terminating, so worker resources cannot be created to delete backup files; the Backup finalizer is retained to avoid discarding backup metadata or silently orphaning backup files: %v",
backup.Namespace, err)
if err := r.recordDeleteBackupFilesFailure(reqCtx, backup, failureReason); err != nil {
return err
}
return intctrlutil.NewRequeueError(terminatingBackupNamespaceRetryInterval,
"waiting with the Backup finalizer retained while the backup namespace is terminating")
}
// wait for the deletion job completed
return err
}
return err
}

func (r *BackupReconciler) removeBackupFinalizer(reqCtx intctrlutil.RequestCtx, backup *dpv1alpha1.Backup) error {
if !controllerutil.ContainsFinalizer(backup, dptypes.DataProtectionFinalizerName) {
return nil
}
patch := client.MergeFromWithOptions(backup.DeepCopy(), client.MergeFromWithOptimisticLock{})
controllerutil.RemoveFinalizer(backup, dptypes.DataProtectionFinalizerName)
return r.Patch(reqCtx.Ctx, backup, patch)
}

func (r *BackupReconciler) recordDeleteBackupFilesFailure(
reqCtx intctrlutil.RequestCtx,
backup *dpv1alpha1.Backup,
failureReason string) error {
if backup.Status.FailureReason == failureReason {
return nil
}
backupPatch := client.MergeFrom(backup.DeepCopy())
backup.Status.FailureReason = failureReason
if r.Recorder != nil {
r.Recorder.Event(backup, corev1.EventTypeWarning, "DeleteBackupFilesFailed", failureReason)
}
return r.Status().Patch(reqCtx.Ctx, backup, backupPatch)
}

func (r *BackupReconciler) ensureWorkerServiceAccountForBackupDeletion(reqCtx intctrlutil.RequestCtx, namespace string) (string, error) {
ns := &corev1.Namespace{}
if err := r.Client.Get(reqCtx.Ctx, types.NamespacedName{Name: namespace}, ns); err != nil {
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 "", fmt.Errorf("%w: 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", errBackupNamespaceTerminating, namespace)
}
// TODO: update the mcMgr param
return EnsureWorkerServiceAccount(reqCtx, r.Client, namespace, nil)
Expand All @@ -280,7 +305,9 @@ func (r *BackupReconciler) handleDeletingPhase(reqCtx intctrlutil.RequestCtx, ba
}

if backup.Spec.DeletionPolicy == dpv1alpha1.BackupDeletionPolicyRetain {

@leon-ape leon-ape Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not redefine Retain without a retained-artifact recovery contract

The API comment explicitly documents deleting the Backup CR while retaining repository contents as unsupported future work; the current CR is the metadata KubeBlocks uses to locate and restore those contents. This branch now removes that CR but adds no retained-artifact identity, import, or recovery path, leaving the preserved files unmanaged and unusable through KubeBlocks. This is a public behavior change, not a finalizer implementation detail. Keep the existing contract or provide a strong product/migration/recovery design and update the API documentation and tests accordingly.

@leon-ape leon-ape Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P0] Retain does not preserve snapshot-backed backup artifacts

A VolumeSnapshot created for a Backup has the Backup as its controller owner. This branch removes the Backup finalizer for Retain without removing or replacing that ownerReference. Once the Backup disappears, garbage collection puts the VolumeSnapshot into deletion. The KubeBlocks finalizer may delay that deletion, but it only leaves a terminating, unusable artifact; once the finalizer is removed, a VolumeSnapshotClass with deletionPolicy: Delete also deletes the VolumeSnapshotContent and physical snapshot.

This directly contradicts the new API promise that Retain keeps the physical snapshot. The added Retain test creates no VolumeSnapshot, so it cannot validate this lifecycle. Deleting the Backup CR cannot be considered safe until snapshot ownership and the retained-artifact lifecycle are explicitly covered.

@weicao weicao Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in exact head 99d0ef187f31ce337e1e5130018660bfde101fe1. The PR no longer removes the Backup finalizer for Retain, and the API/CRD/reference documentation is restored to the existing fail-closed contract. The new regression keeps the Backup CR and finalizer and includes a controller-owned VolumeSnapshot, proving the metadata/ownership anchor remains live. Deleting only the CR is explicitly left for a future retained-artifact identity/import/recovery and owner-migration design.

@weicao weicao Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 99d0ef187f31ce337e1e5130018660bfde101fe1 by withdrawing the unsafe Retain finalizer release rather than attempting an incomplete ownerReference rewrite. The Backup remains present with the DataProtection finalizer, so its controller-owned VolumeSnapshot is not eligible for owner garbage collection. TestDeletingRetainBackupPreservesFinalizerAndControllerOwnedSnapshot covers the Backup finalizer plus the real Backup controller ownerReference and snapshot finalizer.

r.Recorder.Event(backup, corev1.EventTypeWarning, "Retain", "can not delete the backup if deletionPolicy is Retain")
if r.Recorder != nil {
r.Recorder.Event(backup, corev1.EventTypeWarning, "Retain", "can not delete the Backup while deletionPolicy is Retain; change the policy to Delete to remove its metadata and artifacts")
}
return intctrlutil.Reconciled()
}

Expand All @@ -295,6 +322,10 @@ func (r *BackupReconciler) handleDeletingPhase(reqCtx intctrlutil.RequestCtx, ba
}

if err := r.deleteBackupFiles(reqCtx, backup); err != nil {
var requeueErr intctrlutil.RequeueError
if errors.As(err, &requeueErr) {
return intctrlutil.RequeueAfter(requeueErr.RequeueAfter(), reqCtx.Log, requeueErr.Reason())
}
return intctrlutil.RequeueWithError(err, reqCtx.Log, "")
}
return intctrlutil.Reconciled()
Expand Down
166 changes: 166 additions & 0 deletions controllers/dataprotection/backup_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,25 @@ import (
"fmt"
"slices"
"strconv"
"testing"
"time"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/stretchr/testify/require"

vsv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
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"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/record"
"k8s.io/utils/pointer"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -1806,3 +1810,165 @@ var _ = Describe("Backup Controller test", func() {
})
})
})

func TestDeleteBackupFilesTerminatingNamespaceHoldsFinalizerAndSurfacesFailure(t *testing.T) {
oldServiceAccount := viper.GetString(dptypes.CfgKeyWorkerServiceAccountName)
oldClusterRole := viper.GetString(dptypes.CfgKeyWorkerClusterRoleName)
viper.Set(dptypes.CfgKeyWorkerServiceAccountName, "test-dataprotection-worker")
viper.Set(dptypes.CfgKeyWorkerClusterRoleName, "test-dataprotection-worker-role")
t.Cleanup(func() {
viper.Set(dptypes.CfgKeyWorkerServiceAccountName, oldServiceAccount)
viper.Set(dptypes.CfgKeyWorkerClusterRoleName, oldClusterRole)
})

scheme := runtime.NewScheme()
require.NoError(t, corev1.AddToScheme(scheme))
require.NoError(t, batchv1.AddToScheme(scheme))
require.NoError(t, appsv1.AddToScheme(scheme))
require.NoError(t, rbacv1.AddToScheme(scheme))
require.NoError(t, vsv1.AddToScheme(scheme))
require.NoError(t, dpv1alpha1.AddToScheme(scheme))
now := metav1.Now()
backup := &dpv1alpha1.Backup{
ObjectMeta: metav1.ObjectMeta{
Namespace: "terminating-namespace",
Name: "backup-terminating-namespace",
UID: types.UID("backup-uid"),
DeletionTimestamp: &now,
Finalizers: []string{dptypes.DataProtectionFinalizerName},
},
Spec: dpv1alpha1.BackupSpec{DeletionPolicy: dpv1alpha1.BackupDeletionPolicyDelete},
Status: dpv1alpha1.BackupStatus{
Phase: dpv1alpha1.BackupPhaseDeleting,
PersistentVolumeClaimName: "repo-pvc",
Path: "/backups/backup-terminating-namespace",
},
}
namespace := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: backup.Namespace,
DeletionTimestamp: &now,
Finalizers: []string{"kubernetes"},
},
}
repoPVC := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{Namespace: backup.Namespace, Name: backup.Status.PersistentVolumeClaimName},
}
cli := fake.NewClientBuilder().WithScheme(scheme).
WithStatusSubresource(backup).
WithObjects(namespace, backup.DeepCopy(), repoPVC).
Build()
reconciler := &BackupReconciler{
Client: cli,
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}

result, err := reconciler.handleDeletingPhase(intctrlutil.RequestCtx{Ctx: context.Background()}, backup)

require.NoError(t, err)
require.Equal(t, 30*time.Second, result.RequeueAfter,
"a terminating Namespace has no state transition that can make worker creation safe, so retries must be bounded")
require.Greater(t, result.RequeueAfter, time.Second, "persistent namespace absence must not cause a hot loop")
current := &dpv1alpha1.Backup{}
require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(backup), current))
require.Contains(t, current.Finalizers, dptypes.DataProtectionFinalizerName)
require.Contains(t, current.Status.FailureReason, "is terminating")
require.Contains(t, current.Status.FailureReason, "finalizer is retained")
jobs := &batchv1.JobList{}
require.NoError(t, cli.List(context.Background(), jobs, client.InNamespace(backup.Namespace)))
require.Empty(t, jobs.Items)
}

func TestDeletingRetainBackupPreservesFinalizerAndControllerOwnedSnapshot(t *testing.T) {
scheme := runtime.NewScheme()
require.NoError(t, corev1.AddToScheme(scheme))
require.NoError(t, batchv1.AddToScheme(scheme))
require.NoError(t, appsv1.AddToScheme(scheme))
require.NoError(t, vsv1.AddToScheme(scheme))
require.NoError(t, dpv1alpha1.AddToScheme(scheme))
now := metav1.Now()
controller := true
backup := &dpv1alpha1.Backup{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "retained-backup",
UID: types.UID("retained-backup-uid"),
ResourceVersion: "1",
DeletionTimestamp: &now,
Finalizers: []string{dptypes.DataProtectionFinalizerName},
},
Spec: dpv1alpha1.BackupSpec{DeletionPolicy: dpv1alpha1.BackupDeletionPolicyRetain},
Status: dpv1alpha1.BackupStatus{
Phase: dpv1alpha1.BackupPhaseDeleting,
PersistentVolumeClaimName: "repo-pvc",
Path: "/backups/retained-backup",
},
}
snapshot := &vsv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Namespace: backup.Namespace,
Name: "retained-backup-data-0",
Finalizers: []string{dptypes.DataProtectionFinalizerName},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: dpv1alpha1.GroupVersion.String(),
Kind: "Backup",
Name: backup.Name,
UID: backup.UID,
Controller: &controller,
}},
},
}
cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(backup.DeepCopy(), snapshot).Build()
reconciler := &BackupReconciler{
Client: cli,
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}

result, err := reconciler.handleDeletingPhase(intctrlutil.RequestCtx{Ctx: context.Background()}, backup)

require.NoError(t, err)
require.False(t, result.Requeue)
require.Zero(t, result.RequeueAfter)
current := &dpv1alpha1.Backup{}
require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(backup), current))
require.Contains(t, current.Finalizers, dptypes.DataProtectionFinalizerName,
"Retain must keep the Backup metadata that anchors repository and snapshot artifacts")
currentSnapshot := &vsv1.VolumeSnapshot{}
require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(snapshot), currentSnapshot))
require.Contains(t, currentSnapshot.Finalizers, dptypes.DataProtectionFinalizerName)
owner := metav1.GetControllerOf(currentSnapshot)
require.NotNil(t, owner)
require.Equal(t, backup.UID, owner.UID)
jobs := &batchv1.JobList{}
require.NoError(t, cli.List(context.Background(), jobs, client.InNamespace(backup.Namespace)))
require.Empty(t, jobs.Items)
}

func TestRemoveBackupFinalizerRejectsStaleDeletionPolicyObservation(t *testing.T) {
scheme := runtime.NewScheme()
require.NoError(t, dpv1alpha1.AddToScheme(scheme))
backup := &dpv1alpha1.Backup{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "policy-race",
Finalizers: []string{dptypes.DataProtectionFinalizerName},
},
Spec: dpv1alpha1.BackupSpec{DeletionPolicy: dpv1alpha1.BackupDeletionPolicyRetain},
}
cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(backup).Build()
stale := &dpv1alpha1.Backup{}
require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(backup), stale))
current := stale.DeepCopy()
current.Spec.DeletionPolicy = dpv1alpha1.BackupDeletionPolicyDelete
require.NoError(t, cli.Update(context.Background(), current))
reconciler := &BackupReconciler{Client: cli}

err := reconciler.removeBackupFinalizer(intctrlutil.RequestCtx{Ctx: context.Background()}, stale)

require.Error(t, err)
require.NoError(t, cli.Get(context.Background(), client.ObjectKeyFromObject(backup), current))
require.Equal(t, dpv1alpha1.BackupDeletionPolicyDelete, current.Spec.DeletionPolicy)
require.Contains(t, current.Finalizers, dptypes.DataProtectionFinalizerName)
}
Loading