From c825e3c136bc40bcd9bb6d5b180546c3029beeb2 Mon Sep 17 00:00:00 2001 From: Chlins Zhang Date: Tue, 14 Jul 2026 05:51:54 +0800 Subject: [PATCH] fix(delete): surface DeleteItemAction plugin errors from InvokeDeleteActions (#9993) Signed-off-by: chlins --- changelogs/unreleased/9993-chlins | 1 + internal/delete/delete_item_action_handler.go | 18 ++++++- .../delete/delete_item_action_handler_test.go | 52 +++++++++++++++++++ pkg/controller/backup_deletion_controller.go | 2 +- 4 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/9993-chlins diff --git a/changelogs/unreleased/9993-chlins b/changelogs/unreleased/9993-chlins new file mode 100644 index 000000000..79c5236bb --- /dev/null +++ b/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 diff --git a/internal/delete/delete_item_action_handler.go b/internal/delete/delete_item_action_handler.go index 4837d0243..2a16044ee 100644 --- a/internal/delete/delete_item_action_handler.go +++ b/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. diff --git a/internal/delete/delete_item_action_handler_test.go b/internal/delete/delete_item_action_handler_test.go index 6743cd1f9..b7d4e6e6a 100644 --- a/internal/delete/delete_item_action_handler_test.go +++ b/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) +} diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index ccac5cd85..cd74a3a27 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/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 } }