diff --git a/pkg/pillar/cmd/volumemgr/handlevolume.go b/pkg/pillar/cmd/volumemgr/handlevolume.go index 9e2a5c2118b..f3a09aa939b 100644 --- a/pkg/pillar/cmd/volumemgr/handlevolume.go +++ b/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 { diff --git a/pkg/pillar/cmd/volumemgr/retryvolumedelete.go b/pkg/pillar/cmd/volumemgr/retryvolumedelete.go new file mode 100644 index 00000000000..eec845532fb --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/pillar/cmd/volumemgr/retryvolumedelete_test.go b/pkg/pillar/cmd/volumemgr/retryvolumedelete_test.go new file mode 100644 index 00000000000..c3eb5ac5349 --- /dev/null +++ b/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) + } +} diff --git a/pkg/pillar/cmd/volumemgr/volumemgr.go b/pkg/pillar/cmd/volumemgr/volumemgr.go index 4cc0c3a7897..755fd51529f 100644 --- a/pkg/pillar/cmd/volumemgr/volumemgr.go +++ b/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