From 8a6ac7af1c1436a20ef2117edeebe7ede4ae6804 Mon Sep 17 00:00:00 2001 From: Priyansh Choudhary Date: Wed, 15 Apr 2026 02:06:40 +0530 Subject: [PATCH] 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 --- changelogs/unreleased/9693-priyansh17 | 1 + pkg/controller/backup_deletion_controller.go | 39 ++++++- .../backup_deletion_controller_test.go | 108 ++++++++++++++---- 3 files changed, 127 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/9693-priyansh17 diff --git a/changelogs/unreleased/9693-priyansh17 b/changelogs/unreleased/9693-priyansh17 new file mode 100644 index 000000000..0023a3c18 --- /dev/null +++ b/changelogs/unreleased/9693-priyansh17 @@ -0,0 +1 @@ +Enhance backup deletion logic to handle tarball download failures \ No newline at end of file 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 58d9b0420..24cc65846 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)