[17.0] volumemgr: retry failed volume deletes instead of leaking them

pkg/pillar/cmd/volumemgr/handlevolume.go

@@ -293,17 +293,20 @@ func maybeDeleteVolume(ctx *volumemgrContext, status *types.VolumeStatus) {
 			if !vr.VolumeCreated {
 				readyToUnPublish = true
 			} else {
-				var err string
+				var errStr string
 				if vr.Error != nil {
-					err = vr.Error.Error()
+					errStr = vr.Error.Error()
 				} else {
-					err = fmt.Sprintf("unexpected WorkDestroy return for %s", status.Key())
+					errStr = fmt.Sprintf("unexpected WorkDestroy return for %s", status.Key())
 				}
-				log.Errorf("maybeDeleteVolume: %s", err)
-				status.SetErrorDescription(types.ErrorDescription{Error: vr.Error.Error()})
-				// we have no retrial mechanism for volume delete now
-				// so let publish error in status and log and unpublish the volume
-				readyToUnPublish = true
+				// Delete failed, keep the status published in the Deleting
+				// sub-state; retryFailedVolumeDelete re-drives it off the
+				// gc tick (bounded by maxVolumeDeleteRetries).
+				log.Errorf("maybeDeleteVolume: delete of %s failed, will retry: %s",
+					status.Key(), errStr)
+				status.SetErrorDescription(types.ErrorDescription{Error: errStr})
+				publishVolumeStatus(ctx, status)
+				return
 			}
 		}
 	} else {
@@ -312,6 +315,8 @@ func maybeDeleteVolume(ctx *volumemgrContext, status *types.VolumeStatus) {
 	if readyToUnPublish {
 		// we are not interested in result
 		_ = popVolumeWorkResult(ctx, status.Key())
+		// The volume is gone, drop any delete-retry bookkeeping for it.
+		delete(ctx.volumeDeleteRetryCount, status.Key())
 		publishVolumeStatus(ctx, status)
 		unpublishVolumeStatus(ctx, status)
 		if appDiskMetric := lookupAppDiskMetric(ctx, status.FileLocation); appDiskMetric != nil {

pkg/pillar/cmd/volumemgr/retryvolumedelete.go

@@ -0,0 +1,100 @@
+// Copyright (c) 2026 Zededa, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package volumemgr
+
+import (
+	"github.com/lf-edge/eve/pkg/pillar/types"
+)
+
+// maxVolumeDeleteRetries bounds how many times a volume whose destroy failed is
+// re-driven off the gc tick before volumemgr gives up. Deleting a PVC/Longhorn
+// volume is normally quick, but the owner node can be unreachable for a while
+// during a purge or a cluster update; this budget covers such a window (~gc
+// interval * this count) while still parking a permanently-undeletable volume
+// terminally instead of resubmitting a worker job forever.
+const maxVolumeDeleteRetries = 12
+
+// volumeDeleteRetryAction is what retryFailedVolumeDelete should do with a single
+// VolumeStatus. Split out as a pure function so the bounded-retry boundary is
+// unit-testable without pubsub/worker wiring.
+type volumeDeleteRetryAction int
+
+const (
+	vdSkip    volumeDeleteRetryAction = iota // not a parked failed delete
+	vdRedrive                                // resubmit the destroy work
+	vdGiveUp                                 // retry budget just spent; log once and park terminally
+	vdParked                                 // already given up; leave published, do nothing
+)
+
+// volumeDeleteRetryActionFor decides, from a VolumeStatus and how many times its
+// delete has already been re-driven, whether to skip it, re-drive it, give up, or
+// leave it parked. A volume is a retry candidate only while it is unreferenced
+// (RefCount 0), parked in the Deleting sub-state, and carries an error - i.e. a
+// destroy that failed rather than one still in flight. It is re-driven until it has
+// been retried maxVolumeDeleteRetries times, at which point it is given up once and
+// then left parked (still published with its error, for operator visibility) so a
+// permanently-failing delete stops consuming worker slots without disappearing.
+func volumeDeleteRetryActionFor(status *types.VolumeStatus, retryCount int) volumeDeleteRetryAction {
+	if status.RefCount != 0 ||
+		status.SubState != types.VolumeSubStateDeleting ||
+		!status.HasError() {
+		return vdSkip
+	}
+	if retryCount > maxVolumeDeleteRetries {
+		return vdParked
+	}
+	if retryCount == maxVolumeDeleteRetries {
+		return vdGiveUp
+	}
+	return vdRedrive
+}
+
+// retryFailedVolumeDelete re-drives volume deletions that previously failed and
+// were left published (by maybeDeleteVolume) in the Deleting sub-state with an
+// error, rather than being unpublished and leaked. For each such volume it
+// resubmits the destroy work; the result flows back through
+// processVolumeWorkResult -> maybeDeleteVolume, which unpublishes on success
+// (clearing the retry count) or leaves the error set for another pass. After
+// maxVolumeDeleteRetries attempts it gives up: the volume is left published in the
+// Deleting sub-state with its error (mirroring the cluster-create give-up) so a
+// permanently-undeletable, possibly-orphaned volume stays visible to the operator
+// in status instead of silently disappearing; it just stops being re-driven.
+// Called from the periodic gc handler.
+func retryFailedVolumeDelete(ctx *volumemgrContext) {
+	for _, st := range ctx.pubVolumeStatus.GetAll() {
+		status := st.(types.VolumeStatus)
+		key := status.Key()
+		switch volumeDeleteRetryActionFor(&status, ctx.volumeDeleteRetryCount[key]) {
+		case vdSkip:
+			// Not (or no longer) a parked failed delete. Drop any stale retry
+			// count so a volume that left the candidate set another way (e.g. it
+			// got re-referenced during a redeploy) doesn't resume a later delete
+			// from a shortened budget. A no-op for untracked keys.
+			delete(ctx.volumeDeleteRetryCount, key)
+			continue
+		case vdParked:
+			// Already gave up on this delete; it stays published with its error
+			// for operator visibility. Nothing more to do until it stops being a
+			// candidate (handled by vdSkip above).
+			continue
+		case vdGiveUp:
+			log.Errorf("retryFailedVolumeDelete: giving up on delete of %s (%s) "+
+				"after %d retries; leaving it published in Deleting with a terminal "+
+				"error, underlying volume may be orphaned: %s",
+				key, status.DisplayName, ctx.volumeDeleteRetryCount[key], status.Error)
+			// Consume the stale worker result but keep the VolumeStatus (and its
+			// disk metrics) published; bump the count past the cap so subsequent gc
+			// ticks see vdParked and neither re-drive nor re-log.
+			_ = popVolumeWorkResult(ctx, key)
+			ctx.volumeDeleteRetryCount[key] = maxVolumeDeleteRetries + 1
+		case vdRedrive:
+			ctx.volumeDeleteRetryCount[key]++
+			log.Noticef("retryFailedVolumeDelete: retrying delete of %s (%s) "+
+				"(attempt %d/%d): %s",
+				key, status.DisplayName, ctx.volumeDeleteRetryCount[key],
+				maxVolumeDeleteRetries, status.Error)
+			AddWorkDestroy(ctx, &status)
+		}
+	}
+}

pkg/pillar/cmd/volumemgr/retryvolumedelete_test.go

@@ -0,0 +1,72 @@
+// Copyright (c) 2026 Zededa, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package volumemgr
+
+import (
+	"testing"
+
+	"github.com/lf-edge/eve/pkg/pillar/types"
+)
+
+// parkedFailedDeleteVolume builds a VolumeStatus in the state the volume-delete
+// retry treats as a candidate: unreferenced, in the Deleting sub-state, with a
+// recorded error (a destroy that failed rather than one still in flight).
+func parkedFailedDeleteVolume() types.VolumeStatus {
+	vs := types.VolumeStatus{
+		RefCount: 0,
+		SubState: types.VolumeSubStateDeleting,
+	}
+	vs.SetErrorDescription(types.ErrorDescription{Error: "DeletePVC failed: node unreachable"})
+	return vs
+}
+
+// TestVolumeDeleteRetryActionFor pins the bounded-retry boundary: a parked failed
+// delete is re-driven until it has been retried maxVolumeDeleteRetries times, at
+// which point the retry gives up exactly once and then leaves the volume parked
+// (still published with its error) instead of resubmitting a destroy worker job or
+// re-logging every gc tick forever. Anything that is not a parked failed delete is
+// skipped.
+func TestVolumeDeleteRetryActionFor(t *testing.T) {
+	// Under the cap: re-drive.
+	for _, n := range []int{0, 1, maxVolumeDeleteRetries - 1} {
+		vs := parkedFailedDeleteVolume()
+		if got := volumeDeleteRetryActionFor(&vs, n); got != vdRedrive {
+			t.Errorf("retryCount=%d: got %d, want vdRedrive(%d)", n, got, vdRedrive)
+		}
+	}
+	// Exactly at the cap: give up (log once, park).
+	{
+		vs := parkedFailedDeleteVolume()
+		if got := volumeDeleteRetryActionFor(&vs, maxVolumeDeleteRetries); got != vdGiveUp {
+			t.Errorf("retryCount=%d: got %d, want vdGiveUp(%d)",
+				maxVolumeDeleteRetries, got, vdGiveUp)
+		}
+	}
+	// Beyond the cap: already given up, stay parked (no re-drive, no re-log).
+	for _, n := range []int{maxVolumeDeleteRetries + 1, maxVolumeDeleteRetries + 5} {
+		vs := parkedFailedDeleteVolume()
+		if got := volumeDeleteRetryActionFor(&vs, n); got != vdParked {
+			t.Errorf("retryCount=%d: got %d, want vdParked(%d)", n, got, vdParked)
+		}
+	}
+	// Not a retry candidate: skip.
+	skips := map[string]func(*types.VolumeStatus){
+		"still referenced": func(v *types.VolumeStatus) { v.RefCount = 1 },
+		"not deleting":     func(v *types.VolumeStatus) { v.SubState = types.VolumeSubStateCreated },
+	}
+	for name, mutate := range skips {
+		vs := parkedFailedDeleteVolume()
+		mutate(&vs)
+		if got := volumeDeleteRetryActionFor(&vs, 0); got != vdSkip {
+			t.Errorf("%s: got %d, want vdSkip(%d)", name, got, vdSkip)
+		}
+	}
+
+	// A fresh (never-failed) delete in flight must not be treated as a failed
+	// delete: Deleting sub-state but no error yet.
+	inFlight := types.VolumeStatus{SubState: types.VolumeSubStateDeleting}
+	if got := volumeDeleteRetryActionFor(&inFlight, 0); got != vdSkip {
+		t.Errorf("in-flight delete: got %d, want vdSkip(%d)", got, vdSkip)
+	}
+}

pkg/pillar/cmd/volumemgr/volumemgr.go

@@ -98,6 +98,21 @@ type volumemgrContext struct {
 
 	volumeConfigCreateDeferredMap map[string]*types.VolumeConfig
 
+	// deferredProtectedBlobs and deferredProtectedImages cache the blob shas
+	// and image references named by unexpired deferred-delete records, so the
+	// per-blob/per-image GC guards are O(1) lookups rather than a scan of every
+	// record. Rebuilt by refreshDeferredProtection whenever the pubDeferredDelete
+	// set changes.
+	deferredProtectedBlobs  map[string]bool
+	deferredProtectedImages map[string]bool
+
+	// volumeDeleteRetryCount tracks how many times a volume whose destroy
+	// (PVC/Longhorn volume deletion) failed has been re-driven. Key is
+	// VolumeStatus.Key(). Entries are added/incremented by
+	// retryFailedVolumeDelete off the gc tick and cleared once the volume is
+	// confirmed gone (or given up on after maxVolumeDeleteRetries).
+	volumeDeleteRetryCount map[string]int
+
 	persistType types.PersistType
 
 	capabilities *types.Capabilities
@@ -573,6 +588,7 @@ func Run(ps *pubsub.PubSub, loggerArg *logrus.Logger, logArg *base.LogObject, ar
 	subContentTreeConfig.Activate()
 
 	ctx.volumeConfigCreateDeferredMap = make(map[string]*types.VolumeConfig)
+	ctx.volumeDeleteRetryCount = make(map[string]int)
 
 	subVolumeConfig, err := ps.NewSubscription(pubsub.SubscriptionOptions{
 		CreateHandler:  handleVolumeCreate,
@@ -801,6 +817,10 @@ func Run(ps *pubsub.PubSub, loggerArg *logrus.Logger, logArg *base.LogObject, ar
 			// error (longhorn/CDI not ready yet, common right after a kvm->k
 			// conversion) so they recover once the cluster is up.
 			retryFailedClusterVolumeCreate(&ctx)
+			// Re-drive volumes whose delete (PVC/Longhorn volume) failed so a
+			// transient error - or an unreachable owner node during a purge or
+			// cluster update - does not leak the underlying storage forever.
+			retryFailedVolumeDelete(&ctx)
 			// Re-drive volumes deferred pre-create waiting on EVE-k cluster
 			// storage readiness so they proceed once longhorn/CDI come up. Only
 			// EVE-k defers here; on other HVs the disk-space deferral is already