From 4c91959f235b3fc156e571feab03304669d37862 Mon Sep 17 00:00:00 2001 From: Priyansh Choudhary Date: Thu, 16 Apr 2026 23:05:12 +0530 Subject: [PATCH] Backport PR #9693 and #9700 to Release-1.18 (#9731) * fix: backup deletion silently succeeds when tarball download fails (#9693) * Enhance backup deletion logic to handle tarball download failures and clean up associated CSI VolumeSnapshotContents Signed-off-by: Priyansh Choudhary * added changelog Signed-off-by: Priyansh Choudhary * Refactor error handling in backup deletion Signed-off-by: Priyansh Choudhary * Refactor backup deletion logic to skip CSI snapshot cleanup on tarball download failure Signed-off-by: Priyansh Choudhary * prevent backup deletion when errors occur Signed-off-by: Priyansh Choudhary * added logger Signed-off-by: Priyansh Choudhary * Add delay to avoid race conditions during VolumeSnapshotContent deletion (#9700) * Add delay to avoid race conditions during VolumeSnapshotContent deletion Signed-off-by: Priyansh Choudhary * updated changelog Signed-off-by: Priyansh Choudhary * Updated Changelog Signed-off-by: Priyansh Choudhary * Updated changelog Signed-off-by: Priyansh Choudhary --------- Signed-off-by: Priyansh Choudhary --- changelogs/unreleased/9731-priyansh17 | 3 + .../csi/volumesnapshotcontent_action.go | 38 ++---- .../csi/volumesnapshotcontent_action_test.go | 78 +++++++++++++ pkg/controller/backup_deletion_controller.go | 39 ++++++- .../backup_deletion_controller_test.go | 108 ++++++++++++++---- 5 files changed, 215 insertions(+), 51 deletions(-) create mode 100644 changelogs/unreleased/9731-priyansh17 diff --git a/changelogs/unreleased/9731-priyansh17 b/changelogs/unreleased/9731-priyansh17 new file mode 100644 index 000000000..46f967f53 --- /dev/null +++ b/changelogs/unreleased/9731-priyansh17 @@ -0,0 +1,3 @@ +Backporting PR #9700 and #9693 +Fix issue #9699, add a 2-second gap between temporary CSI VolumeSnapshotContent create and delete operations +Enhance backup deletion logic to handle tarball download failures \ No newline at end of file diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index 7a6724df1..456127471 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action.go @@ -27,10 +27,8 @@ import ( corev1api "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/wait" crclient "sigs.k8s.io/controller-runtime/pkg/client" - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/client" plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common" "github.com/vmware-tanzu/velero/pkg/plugin/velero" @@ -44,6 +42,10 @@ type volumeSnapshotContentDeleteItemAction struct { crClient crclient.Client } +const tempVSCCreateDeleteGap = 2 * time.Second + +var sleepBetweenTempVSCCreateAndDelete = time.Sleep + // AppliesTo returns information indicating // VolumeSnapshotContentRestoreItemAction action should be invoked // while restoring VolumeSnapshotContent.snapshot.storage.k8s.io resources @@ -115,35 +117,11 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute( return errors.Wrapf(err, "fail to create VolumeSnapshotContent %s", snapCont.Name) } - // Read resource timeout from backup annotation, if not set, use default value. - timeout, err := time.ParseDuration( - input.Backup.Annotations[velerov1api.ResourceTimeoutAnnotation]) - if err != nil { - p.log.Warnf("fail to parse resource timeout annotation %s: %s", - input.Backup.Annotations[velerov1api.ResourceTimeoutAnnotation], err.Error()) - timeout = 10 * time.Minute - } - p.log.Debugf("resource timeout is set to %s", timeout.String()) - - interval := 5 * time.Second - - // Wait until VSC created and ReadyToUse is true. - if err := wait.PollUntilContextTimeout( - context.Background(), - interval, - timeout, - true, - func(ctx context.Context) (bool, error) { - return checkVSCReadiness(ctx, &snapCont, p.crClient) - }, - ); err != nil { - // Clean up the VSC we created since it can't become ready - if deleteErr := p.crClient.Delete(context.TODO(), &snapCont); deleteErr != nil && !apierrors.IsNotFound(deleteErr) { - p.log.WithError(deleteErr).Errorf("Failed to clean up VolumeSnapshotContent %s", snapCont.Name) - } - return errors.Wrapf(err, "fail to wait VolumeSnapshotContent %s becomes ready.", snapCont.Name) - } + // Add a small delay before delete to avoid create/delete race conditions in CSI controllers. + sleepBetweenTempVSCCreateAndDelete(tempVSCCreateDeleteGap) + // Delete the temp VSC immediately to trigger cloud snapshot removal. + // The CSI driver will handle the actual cloud snapshot deletion. if err := p.crClient.Delete( context.TODO(), &snapCont, diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index 7dbd6d7ff..6cf32adec 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "testing" + "time" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" "github.com/pkg/errors" @@ -37,6 +38,50 @@ import ( velerotest "github.com/vmware-tanzu/velero/pkg/test" ) +// fakeClientWithErrors wraps a real client and injects errors for specific operations. +type fakeClientWithErrors struct { + crclient.Client + getError error + patchError error + deleteError error +} + +type fakeClientWithCallTracking struct { + crclient.Client + events *[]string +} + +func (c *fakeClientWithCallTracking) Create(ctx context.Context, obj crclient.Object, opts ...crclient.CreateOption) error { + *c.events = append(*c.events, "create") + return c.Client.Create(ctx, obj, opts...) +} + +func (c *fakeClientWithCallTracking) Delete(ctx context.Context, obj crclient.Object, opts ...crclient.DeleteOption) error { + *c.events = append(*c.events, "delete") + return c.Client.Delete(ctx, obj, opts...) +} + +func (c *fakeClientWithErrors) Get(ctx context.Context, key crclient.ObjectKey, obj crclient.Object, opts ...crclient.GetOption) error { + if c.getError != nil { + return c.getError + } + return c.Client.Get(ctx, key, obj, opts...) +} + +func (c *fakeClientWithErrors) Patch(ctx context.Context, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { + if c.patchError != nil { + return c.patchError + } + return c.Client.Patch(ctx, obj, patch, opts...) +} + +func (c *fakeClientWithErrors) Delete(ctx context.Context, obj crclient.Object, opts ...crclient.DeleteOption) error { + if c.deleteError != nil { + return c.deleteError + } + return c.Client.Delete(ctx, obj, opts...) +} + func TestVSCExecute(t *testing.T) { snapshotHandleStr := "test" tests := []struct { @@ -239,6 +284,39 @@ func TestCheckVSCReadiness(t *testing.T) { } } +func TestVSCExecute_CreateSleepDeleteOrder(t *testing.T) { + snapshotHandleStr := "test" + vsc := builder.ForVolumeSnapshotContent("bar"). + ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})). + Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}). + Result() + + vscMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(vsc) + require.NoError(t, err) + + events := make([]string, 0, 3) + realClient := velerotest.NewFakeControllerRuntimeClient(t) + trackingClient := &fakeClientWithCallTracking{Client: realClient, events: &events} + + originalSleep := sleepBetweenTempVSCCreateAndDelete + t.Cleanup(func() { + sleepBetweenTempVSCCreateAndDelete = originalSleep + }) + + sleepBetweenTempVSCCreateAndDelete = func(d time.Duration) { + require.Equal(t, tempVSCCreateDeleteGap, d) + events = append(events, "sleep") + } + + p := volumeSnapshotContentDeleteItemAction{log: logrus.StandardLogger(), crClient: trackingClient} + err = p.Execute(&velero.DeleteItemActionExecuteInput{ + Item: &unstructured.Unstructured{Object: vscMap}, + Backup: builder.ForBackup("velero", "backup").Result(), + }) + require.NoError(t, err) + require.Equal(t, []string{"create", "sleep", "delete"}, events) +} + func boolPtr(b bool) *bool { return &b } diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 5a791999c..5fe29c5f1 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "strings" "time" jsonpatch "github.com/evanphx/json-patch/v5" @@ -267,8 +268,17 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque if err != nil { log.WithError(err).Errorf("Unable to download tarball for backup %s, skipping associated DeleteItemAction plugins", backup.Name) + // for backups which failed before tarball object could be uploaded we do offline cleanup log.Info("Cleaning up CSI volumesnapshots") r.deleteCSIVolumeSnapshotsIfAny(ctx, backup, log) + + // If the tarball simply does not exist (HTTP 404 / not found), the download + // failure is permanent and not retryable, so we let deletion proceed. + // For transient errors (throttling, auth failures, network issues), record + // the error to fail the deletion so it can be retried later. + if !isTarballNotFoundError(err) { + errs = append(errs, errors.Wrapf(err, "error downloading backup tarball, CSI snapshot cleanup was skipped").Error()) + } } else { defer closeAndRemoveFile(backupFile, r.logger) deleteCtx := &delete.Context{ @@ -351,11 +361,13 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque } } - if backupStore != nil { + if backupStore != nil && len(errs) == 0 { log.Info("Removing backup from backup storage") if err := backupStore.DeleteBackup(backup.Name); err != nil { errs = append(errs, err.Error()) } + } else if len(errs) > 0 { + log.Info("Skipping removal of backup from backup storage due to previous errors") } log.Info("Removing restores") @@ -691,3 +703,28 @@ func batchDeleteSnapshots(ctx context.Context, repoEnsurer *repository.Ensurer, return errs } + +// isTarballNotFoundError reports whether err indicates that the backup tarball +// does not exist in object storage (e.g. HTTP 404 / not-found). Such errors are +// permanent and not retryable, so callers should let deletion proceed (skipping +// DeleteItemAction plugins) rather than failing the entire deletion. +// +// Transient errors (throttling, auth failures, network timeouts) return false so +// the deletion is failed and can be retried once the storage is reachable again. +func isTarballNotFoundError(err error) bool { + if err == nil { + return false + } + // Lower-case once for all comparisons. + msg := strings.ToLower(err.Error()) + // Common "not found" indicators across cloud providers: + // - "not found" / "does not exist": generic, in-memory object store + // - "nosuchkey": AWS S3 + // - "blobnotfound": Azure Blob Storage + // - "objectnotexist": GCS + return strings.Contains(msg, "not found") || + strings.Contains(msg, "does not exist") || + strings.Contains(msg, "nosuchkey") || + strings.Contains(msg, "blobnotfound") || + strings.Contains(msg, "objectnotexist") +} diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index ab3687438..dfca4f137 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -25,8 +25,6 @@ import ( "reflect" "time" - snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" - "context" "github.com/sirupsen/logrus" @@ -606,7 +604,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { // Make sure snapshot was deleted assert.Equal(t, 0, td.volumeSnapshotter.SnapshotsTaken.Len()) }) - t.Run("backup is still deleted if downloading tarball fails for DeleteItemAction plugins", func(t *testing.T) { + t.Run("backup deletion fails with error when downloading tarball fails for DeleteItemAction plugins", func(t *testing.T) { backup := builder.ForBackup(velerov1api.DefaultNamespace, "foo").Result() backup.UID = "uid" backup.Spec.StorageLocation = "primary" @@ -672,6 +670,89 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { td.backupStore.On("GetBackupVolumeSnapshots", input.Spec.BackupName).Return(snapshots, nil) td.backupStore.On("GetBackupContents", input.Spec.BackupName).Return(nil, fmt.Errorf("error downloading tarball")) + + _, err := td.controller.Reconcile(t.Context(), td.req) + require.NoError(t, err) + + td.backupStore.AssertCalled(t, "GetBackupContents", input.Spec.BackupName) + // DeleteBackup (removing backup data from object storage) must NOT be called + // when there are errors, so that the deletion can be retried later. + td.backupStore.AssertNotCalled(t, "DeleteBackup", input.Spec.BackupName) + + // the dbr should still exist and be marked Processed with errors + res := &velerov1api.DeleteBackupRequest{} + err = td.fakeClient.Get(ctx, td.req.NamespacedName, res) + require.NoError(t, err, "Expected DBR to still exist after tarball download failure") + assert.Equal(t, velerov1api.DeleteBackupRequestPhaseProcessed, res.Status.Phase) + require.Len(t, res.Status.Errors, 1) + assert.Contains(t, res.Status.Errors[0], "error downloading backup tarball, CSI snapshot cleanup was skipped") + + // backup CR should NOT be deleted + err = td.fakeClient.Get(t.Context(), types.NamespacedName{ + Namespace: velerov1api.DefaultNamespace, + Name: backup.Name, + }, &velerov1api.Backup{}) + require.NoError(t, err, "Expected backup CR to still exist after tarball download failure") + }) + t.Run("backup is still deleted if downloading tarball returns a not-found error", func(t *testing.T) { + backup := builder.ForBackup(velerov1api.DefaultNamespace, "foo").Result() + backup.UID = "uid" + backup.Spec.StorageLocation = "primary" + + input := defaultTestDbr() + input.Labels = nil + + location := &velerov1api.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Spec.StorageLocation, + }, + Spec: velerov1api.BackupStorageLocationSpec{ + Provider: "objStoreProvider", + StorageType: velerov1api.StorageType{ + ObjectStorage: &velerov1api.ObjectStorageLocation{ + Bucket: "bucket", + }, + }, + }, + Status: velerov1api.BackupStorageLocationStatus{ + Phase: velerov1api.BackupStorageLocationPhaseAvailable, + }, + } + + snapshotLocation := &velerov1api.VolumeSnapshotLocation{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: "vsl-1", + }, + Spec: velerov1api.VolumeSnapshotLocationSpec{ + Provider: "provider-1", + }, + } + + td := setupBackupDeletionControllerTest(t, defaultTestDbr(), backup, location, snapshotLocation) + td.volumeSnapshotter.SnapshotsTaken.Insert("snap-1") + + snapshots := []*volume.Snapshot{ + { + Spec: volume.SnapshotSpec{ + Location: "vsl-1", + }, + Status: volume.SnapshotStatus{ + ProviderSnapshotID: "snap-1", + }, + }, + } + + pluginManager := &pluginmocks.Manager{} + pluginManager.On("GetVolumeSnapshotter", "provider-1").Return(td.volumeSnapshotter, nil) + pluginManager.On("GetDeleteItemActions").Return([]velero.DeleteItemAction{new(mocks.DeleteItemAction)}, nil) + pluginManager.On("CleanupClients") + td.controller.newPluginManager = func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager } + + td.backupStore.On("GetBackupVolumeSnapshots", input.Spec.BackupName).Return(snapshots, nil) + // Simulate a 404/not-found error (tarball has already been removed from storage) + td.backupStore.On("GetBackupContents", input.Spec.BackupName).Return(nil, fmt.Errorf("key not found")) td.backupStore.On("DeleteBackup", input.Spec.BackupName).Return(nil) _, err := td.controller.Reconcile(t.Context(), td.req) @@ -680,30 +761,17 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { td.backupStore.AssertCalled(t, "GetBackupContents", input.Spec.BackupName) td.backupStore.AssertCalled(t, "DeleteBackup", input.Spec.BackupName) - // the dbr should be deleted + // the dbr should be deleted (not-found is treated as permanent, deletion proceeds) res := &velerov1api.DeleteBackupRequest{} err = td.fakeClient.Get(ctx, td.req.NamespacedName, res) - assert.True(t, apierrors.IsNotFound(err), "Expected not found error, but actual value of error: %v", err) - if err == nil { - t.Logf("status of the dbr: %s, errors in dbr: %v", res.Status.Phase, res.Status.Errors) - } + assert.True(t, apierrors.IsNotFound(err), "Expected DBR to be deleted after not-found tarball error, but actual error: %v", err) - // backup CR should be deleted + // backup CR should be deleted because there are no errors in errs err = td.fakeClient.Get(t.Context(), types.NamespacedName{ Namespace: velerov1api.DefaultNamespace, Name: backup.Name, }, &velerov1api.Backup{}) - assert.True(t, apierrors.IsNotFound(err), "Expected not found error, but actual value of error: %v", err) - - // leaked CSI snapshot should be deleted - err = td.fakeClient.Get(t.Context(), types.NamespacedName{ - Namespace: "user-ns", - Name: "vs-1", - }, &snapshotv1api.VolumeSnapshot{}) - assert.True(t, apierrors.IsNotFound(err), "Expected not found error for the leaked CSI snapshot, but actual value of error: %v", err) - - // Make sure snapshot was deleted - assert.Equal(t, 0, td.volumeSnapshotter.SnapshotsTaken.Len()) + assert.True(t, apierrors.IsNotFound(err), "Expected backup CR to be deleted after not-found tarball error, but actual error: %v", err) }) t.Run("Expired request will be deleted if the status is processed", func(t *testing.T) { expired := time.Date(2018, 4, 3, 12, 0, 0, 0, time.UTC)