-
Notifications
You must be signed in to change notification settings - Fork 277
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.
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
7769a34
ebef178
fc8e4f4
c66fe55
99d0ef1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ package dataprotection | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "reflect" | ||
| "strings" | ||
|
|
@@ -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" | ||
|
|
@@ -70,6 +72,10 @@ type BackupReconciler struct { | |
| clock clock.RealClock | ||
| } | ||
|
|
||
| var errBackupNamespaceNotFound = errors.New("backup namespace not found") | ||
|
|
||
| const missingBackupNamespaceRetryInterval = 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 | ||
|
|
@@ -215,13 +221,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, | ||
|
|
@@ -234,27 +233,57 @@ 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) | ||
| if err := r.recordDeleteBackupFilesFailure(reqCtx, backup, failureReason); err != nil { | ||
| return err | ||
| } | ||
| return intctrlutil.NewRequeueError(missingBackupNamespaceRetryInterval, | ||
| "waiting for the backup namespace to be restored before deleting backup files") | ||
| } | ||
| // 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) { | ||
| 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() { | ||
|
|
@@ -280,7 +309,12 @@ func (r *BackupReconciler) handleDeletingPhase(reqCtx intctrlutil.RequestCtx, ba | |
| } | ||
|
|
||
| if backup.Spec.DeletionPolicy == dpv1alpha1.BackupDeletionPolicyRetain { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in exact head
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
| 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() | ||
| } | ||
|
|
||
|
|
@@ -295,6 +329,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() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 NamespaceTerminatinglifecycle. 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.There was a problem hiding this comment.
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.