Auto-exclude Velero namespace from backups instead of hard error

Velero does not support backing up its own namespace. Previously this
would fail during execution with no clear signal at request time.

This change auto-excludes the Velero namespace during backup preparation:
- When included via wildcard or empty includes (all namespaces): adds
  the Velero namespace to ExcludedNamespaces with a warning log.
- When explicitly listed alongside other namespaces: removes it from
  IncludedNamespaces, adds to ExcludedNamespaces, and logs a warning.
- When it is the only included namespace: returns a validation error
  to prevent a silent empty backup.
- Avoids duplicate entries in ExcludedNamespaces if the namespace was
  already excluded by prior logic (e.g. exclude-from-backup label).

Signed-off-by: Achin Mishra <urs.achin.007@gmail.com>

Author: achinmishra

changelogs/unreleased/9693-priyansh17

@@ -0,0 +1 @@
+Enhance backup deletion logic to handle tarball download failures

pkg/controller/backup_controller.go

@@ -600,7 +600,17 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel
 				fmt.Sprintf("Velero namespace %q is the only included namespace, but Velero does not support backing up its own namespace", veleroNs))
 		} else {
 			logger.Warnf("Velero namespace %q is being excluded from this backup because Velero does not support backing up its own namespace.", veleroNs)
-			request.Spec.ExcludedNamespaces = append(request.Spec.ExcludedNamespaces, veleroNs)
+			// Only append if not already present (e.g. from the exclude-from-backup label logic).
+			alreadyExcluded := false
+			for _, ns := range request.Spec.ExcludedNamespaces {
+				if ns == veleroNs {
+					alreadyExcluded = true
+					break
+				}
+			}
+			if !alreadyExcluded {
+				request.Spec.ExcludedNamespaces = append(request.Spec.ExcludedNamespaces, veleroNs)
+			}
 		}
 	}
 

pkg/controller/backup_controller_test.go

@@ -274,11 +274,11 @@ func TestProcessBackupVeleroNamespaceValidation(t *testing.T) {
 	defaultBackupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Phase(velerov1api.BackupStorageLocationPhaseAvailable).Result()
 
 	tests := []struct {
-		name                        string
-		backup                      *velerov1api.Backup
-		expectedIncludedNamespaces  []string
-		expectedExcludedNamespaces  []string
-		expectValidationError       bool
+		name                       string
+		backup                     *velerov1api.Backup
+		expectedIncludedNamespaces []string
+		expectedExcludedNamespaces []string
+		expectValidationError      bool
 	}{
 		{
 			name:                       "velero namespace explicitly as only IncludedNamespace returns validation error",
@@ -323,10 +323,10 @@ func TestProcessBackupVeleroNamespaceValidation(t *testing.T) {
 			expectValidationError:      true,
 		},
 		{
-			name:                        "velero plus other namespaces in includes removes velero and backs up the others",
-			backup:                      builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").Phase(velerov1api.BackupPhaseReadyToStart).IncludedNamespaces("velero", "default", "kube-system").Result(),
-			expectedIncludedNamespaces:  []string{"default", "kube-system"},
-			expectedExcludedNamespaces:  []string{"velero"},
+			name:                       "velero plus other namespaces in includes removes velero and backs up the others",
+			backup:                     builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").Phase(velerov1api.BackupPhaseReadyToStart).IncludedNamespaces("velero", "default", "kube-system").Result(),
+			expectedIncludedNamespaces: []string{"default", "kube-system"},
+			expectedExcludedNamespaces: []string{"velero"},
 		},
 	}
 

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")
+}

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,38 +670,108 @@ 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"))
-		td.backupStore.On("DeleteBackup", input.Spec.BackupName).Return(nil)
 
 		_, err := td.controller.Reconcile(t.Context(), td.req)
 		require.NoError(t, err)
 
 		td.backupStore.AssertCalled(t, "GetBackupContents", input.Spec.BackupName)
-		td.backupStore.AssertCalled(t, "DeleteBackup", 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 be deleted
+		// the dbr should still exist and be marked Processed with errors
 		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)
-		}
+		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 be deleted
+		// backup CR should NOT be deleted
 		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)
+		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"
 
-		// 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)
+		input := defaultTestDbr()
+		input.Labels = nil
 
-		// Make sure snapshot was deleted
-		assert.Equal(t, 0, td.volumeSnapshotter.SnapshotsTaken.Len())
+		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)
+		require.NoError(t, err)
+
+		td.backupStore.AssertCalled(t, "GetBackupContents", input.Spec.BackupName)
+		td.backupStore.AssertCalled(t, "DeleteBackup", input.Spec.BackupName)
+
+		// 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 DBR to be deleted after not-found tarball error, but actual error: %v", err)
+
+		// 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 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)