fix(delete): surface DeleteItemAction plugin errors from InvokeDeleteActions (#9993)

Signed-off-by: chlins <chlins.zhang@gmail.com>

changelogs/unreleased/9993-chlins

@@ -0,0 +1 @@
+Surface DeleteItemAction plugin errors from InvokeDeleteActions so backup deletion fails and retries instead of silently orphaning data mover snapshots and other private artifacts

internal/delete/delete_item_action_handler.go

@@ -25,6 +25,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	kubeerrs "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -80,6 +81,15 @@ func InvokeDeleteActions(ctx *Context) error {
 	}
 	processdResources := sets.NewString()
 
+	// deleteErrs collects errors returned by DeleteItemAction plugins. We keep
+	// looping over the remaining items even when a plugin fails, but we must not
+	// swallow these errors: a DIA failure means the private artifacts it manages
+	// (e.g. data mover repository snapshots) may not have been deleted. If we
+	// returned nil here, the caller would proceed to delete the backup and its
+	// metadata, orphaning those artifacts forever. Returning the aggregated error
+	// makes the caller fail the deletion so it can be retried.
+	var deleteErrs []error
+
 	for resource := range backupResources {
 		groupResource := schema.ParseGroupResource(resource)
 
@@ -124,15 +134,19 @@ func InvokeDeleteActions(ctx *Context) error {
 						Item:   obj,
 						Backup: ctx.Backup,
 					})
-					// Since we want to keep looping even on errors, log them instead of just returning.
+					// Keep looping even on errors so a single failing plugin
+					// doesn't prevent the remaining items from being cleaned up,
+					// but record the error so it can be surfaced to the caller.
 					if err != nil {
 						itemLog.WithError(err).Error("plugin error")
+						deleteErrs = append(deleteErrs, errors.Wrapf(err,
+							"error executing DeleteItemAction for %s %s", groupResource.String(), obj.GetName()))
 					}
 				}
 			}
 		}
 	}
-	return nil
+	return kubeerrs.NewAggregate(deleteErrs)
 }
 
 // getApplicableActions takes resolved DeleteItemActions and filters them for a given group/resource and namespace.

internal/delete/delete_item_action_handler_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package delete
 
 import (
+	"errors"
 	"io"
 	"sort"
 	"testing"
@@ -276,3 +277,54 @@ func TestInvokeDeleteItemActionsWithNoPlugins(t *testing.T) {
 	err := InvokeDeleteActions(c)
 	require.NoError(t, err)
 }
+
+// failingAction is a DeleteItemAction that always returns an error from Execute.
+// It is used to verify that InvokeDeleteActions surfaces plugin errors instead
+// of swallowing them.
+type failingAction struct {
+	selector velero.ResourceSelector
+	err      error
+	executed int
+}
+
+func (a *failingAction) AppliesTo() (velero.ResourceSelector, error) {
+	return a.selector, nil
+}
+
+func (a *failingAction) Execute(input *velero.DeleteItemActionExecuteInput) error {
+	a.executed++
+	return a.err
+}
+
+func TestInvokeDeleteActionsReturnsPluginErrors(t *testing.T) {
+	fs := test.NewFakeFileSystem()
+	log := logrus.StandardLogger()
+
+	tarball := test.NewTarWriter(t).
+		AddItems("pods", builder.ForPod("ns-1", "pod-1").Result(), builder.ForPod("ns-2", "pod-2").Result()).
+		Done()
+
+	action := &failingAction{err: errors.New("could not delete artifact")}
+
+	h := newHarness(t)
+	h.addResource(t, test.Pods())
+
+	c := &Context{
+		Backup:          builder.ForBackup("velero", "velero").Result(),
+		BackupReader:    tarball,
+		Filesystem:      fs,
+		DiscoveryHelper: h.discoveryHelper,
+		Actions:         []velero.DeleteItemAction{action},
+		Log:             log,
+	}
+
+	err := InvokeDeleteActions(c)
+
+	// The plugin error must be surfaced so the caller can fail the deletion
+	// rather than orphaning the artifacts the plugin failed to delete.
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "could not delete artifact")
+	// The loop must keep going: the action should run for every matching item,
+	// not stop at the first failure.
+	assert.Equal(t, 2, action.executed)
+}

pkg/controller/backup_deletion_controller.go

@@ -295,7 +295,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 			err = delete.InvokeDeleteActions(deleteCtx)
 			if err != nil {
 				log.WithError(err).Error("Error invoking delete item actions")
-				err2 := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.New("error invoking delete item actions"))
+				err2 := r.patchDeleteBackupRequestWithError(ctx, dbr, errors.Wrap(err, "error invoking delete item actions"))
 				return ctrl.Result{}, err2
 			}
 		}