prevent backup deletion when errors occur

Signed-off-by: Priyansh Choudhary <im1706@gmail.com>

Author: priyansh17

pkg/controller/backup_deletion_controller.go

@@ -268,6 +268,10 @@ 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
@@ -357,7 +361,7 @@ 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())

pkg/controller/backup_deletion_controller_test.go

@@ -670,15 +670,14 @@ 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)
-		// DeleteBackup (removing backup data from object storage) is still called because
-		// that step is not gated by the errs slice; only the Kubernetes Backup CR deletion is.
-		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 still exist and be marked Processed with errors
 		res := &velerov1api.DeleteBackupRequest{}