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 1 commit
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
62 changes: 45 additions & 17 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 All @@ -31,6 +32,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8sruntime "k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -70,6 +72,8 @@ type BackupReconciler struct {
clock clock.RealClock
}

var errBackupNamespaceNotFound = errors.New("backup namespace not found")

// +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 +219,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,27 +231,53 @@ 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, errBackupNamespaceNotFound) {
failureReason := fmt.Sprintf(
"backup namespace %q no longer exists, so worker resources cannot be created to delete backup files; the finalizer is retained to avoid silently orphaning backup files; change spec.deletionPolicy to Retain to explicitly keep the files and finish deleting the Backup: %v",
backup.Namespace, err)
return r.recordDeleteBackupFilesFailure(reqCtx, backup, failureReason)
}
// 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 {
if apierrors.IsNotFound(err) {

@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] Base this state machine on a reachable API state

Backup is namespace-scoped, so the API server cannot keep returning this Backup after its Namespace becomes NotFound; namespace deletion keeps the Namespace in Terminating until namespaced finalizers are released. The new fake-client test constructs a state Kubernetes cannot expose. In the real namespace teardown path, the DeletionTimestamp branch below still returns the existing generic error, so the Backup finalizer continues to block namespace deletion. Please provide production evidence for a reachable NotFound path or handle the actual Terminating lifecycle safely.

@leon-ape leon-ape Jul 14, 2026

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.

Clarification on “handle the actual Terminating lifecycle safely”: this must not mean force-removing the Backup finalizer or force-deleting the Backup because its Namespace is being deleted. Namespace deletion is not authorization to discard backup metadata or remote data. The data-protection contract must remain fail-closed, even if that intentionally blocks Namespace finalization. The problem here is that the new recovery state machine and test are centered on an unreachable Namespace-NotFound/Backup-still-readable state; prioritizing Namespace deletion over Backup safety would turn that modeling error into a serious data-loss risk.

@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 99d0ef1873cc3762c87b38631fb8fe6029a88f79. I removed the unreachable Namespace-NotFound/Backup-readable state machine and replaced it with the real Namespace Terminating lifecycle. The controller now creates no deletion worker, keeps the Backup finalizer and metadata, records the fail-closed reason, and returns a bounded 30-second requeue. The focused regression constructs a terminating Namespace and proves no Job is created and the Backup finalizer remains. A generic Namespace lookup error is no longer reclassified as recoverable NotFound.

@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.

Correction: the exact head is 99d0ef187f31ce337e1e5130018660bfde101fe1 (the previous reply expanded the short prefix incorrectly). The implementation and test description are unchanged.

return "", fmt.Errorf("%w: failed to get backup namespace %q before deleting backup files: %v", errBackupNamespaceNotFound, namespace, err)
}
return "", fmt.Errorf("failed to get backup namespace %q before deleting backup files: %w", namespace, err)
}
if !ns.DeletionTimestamp.IsZero() {
Expand All @@ -280,7 +303,12 @@ 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.EventTypeNormal, "Retain", "retaining backup files and deleting the Backup object")
}
if err := r.removeBackupFinalizer(reqCtx, backup); err != nil {
return intctrlutil.RequeueWithError(err, reqCtx.Log, "failed to remove finalizer from retained Backup")
}
return intctrlutil.Reconciled()
}

Expand Down
122 changes: 122 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,10 +25,12 @@ 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"
Expand All @@ -40,6 +42,7 @@ import (
"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 +1809,122 @@ var _ = Describe("Backup Controller test", func() {
})
})
})

func TestDeleteBackupFilesMissingNamespaceHoldsFinalizerAndSurfacesFailure(t *testing.T) {
scheme := runtime.NewScheme()
require.NoError(t, corev1.AddToScheme(scheme))
require.NoError(t, batchv1.AddToScheme(scheme))
require.NoError(t, dpv1alpha1.AddToScheme(scheme))
now := metav1.Now()
backup := &dpv1alpha1.Backup{
ObjectMeta: metav1.ObjectMeta{
Namespace: "missing-namespace",
Name: "backup-missing-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-missing-namespace",
},
}
repoPVC := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{Namespace: backup.Namespace, Name: backup.Status.PersistentVolumeClaimName},
}
cli := fake.NewClientBuilder().WithScheme(scheme).
WithStatusSubresource(backup).
WithObjects(backup.DeepCopy(), repoPVC).
Build()
reconciler := &BackupReconciler{
Client: cli,
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}

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

require.NoError(t, err)
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, "finalizer is retained to avoid silently orphaning backup files")
require.Contains(t, current.Status.FailureReason, "deletionPolicy to Retain")
jobs := &batchv1.JobList{}
require.NoError(t, cli.List(context.Background(), jobs, client.InNamespace(backup.Namespace)))
require.Empty(t, jobs.Items)
}

func TestDeletingRetainBackupReleasesFinalizerWithoutNamespaceOrDeleteWorker(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, dpv1alpha1.AddToScheme(scheme))
now := metav1.Now()
backup := &dpv1alpha1.Backup{
ObjectMeta: metav1.ObjectMeta{
Namespace: "missing-namespace",
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",
},
}
cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(backup.DeepCopy()).Build()
reconciler := &BackupReconciler{
Client: cli,
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}

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

require.NoError(t, err)
current := &dpv1alpha1.Backup{}
err = cli.Get(context.Background(), client.ObjectKeyFromObject(backup), current)
if err == nil {
require.NotContains(t, current.Finalizers, dptypes.DataProtectionFinalizerName)
} else {
require.True(t, apierrors.IsNotFound(err), "unexpected get error: %v", err)
}
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