volumemgr: retry failed volume deletes instead of leaking them

[rene]

Description

The core problem is orphaned/leftover Longhorn volumes left behind by a failed "purge" (redeploy/reset) operation.

1. During earlier stress testing, the cluster got into a bad state and several VMs became corrupted
2. To recover, the user ran a purge on the corrupted VM. The first purge didn't complete correctly, so user retried, three purge attempts total.
3. Now, in kubectl get volume, there are three volumes that all share the same distinctive size, the size uniquely allocated to the corrupted VM, indicating these volumes belongs to this VM.

The abnormal state:

• Only one of the three is legitimate
• The other two are lingering leftovers, reported on a different node, showing degraded/unknown robustness. They were created by the repeated purge attempts.
• These orphans are not needed but are still being replicated, so they're consuming storage space on the cluster.
• Critically, they don't appear in the Longhorn UI — only the one real volume shows there — so normal tooling gives no way to see or clean them up.

Fix

maybeDeleteVolume logged a failed volume delete, set the error, and unpublished the VolumeStatus anyway ("we have no retrial mechanism for volume delete now"). Once the status was gone volumemgr had no record of the volume, so nothing retried and the underlying PVC/Longhorn volume was leaked forever.

This is how a failed+retried app purge on a kubevirt/longhorn cluster ends up with several orphaned Longhorn volumes for one app: each purge bumps the volume generation counter, so GetPVCName() yields a new PVC name (-pvc-) and a new Longhorn volume, while the old generation's delete was fire-and-forget and dropped on failure. For a replicated volume the owner node being unreachable at delete time (mid -purge, or during a node reboot for a cluster update) is enough to trigger it. The leaked volumes keep replicating and consuming space and don't show in the Longhorn UI (no PV/PVC/workload binding them).

On delete failure, keep the VolumeStatus published in the Deleting sub-state with the error and re-drive it from the gc tick via retryFailedVolumeDelete, mirroring the bounded, gc-driven pattern already used for transient cluster-volume creates (retryclustervolume.go): a pure volumeDeleteRetryActionFor decision function, a per-volume retry count in an in-memory map, and give-up after maxVolumeDeleteRetries so a permanently-undeletable volume parks terminally (unpublished, the pre-existing fallback) rather than resubmitting a worker job forever. On successful delete the retry count is cleared.

How to test and validate this PR

Unit tests were added, the issue is tricky to be reproducible on the field, since purge operation must fail to left an orphan volume on the node.

Changelog notes

Implement deletion of Longhorn orphan volumes created after failed purge operations.

PR Backports

• 17.0

Checklist

• I've provided a proper description
• I've added the proper documentation
• I've tested my PR on amd64 device
• I've tested my PR on arm64 device
• I've written the test verification instructions
• I've set the proper labels to this PR
• I've checked the boxes above, or I've provided a good reason why I didn't
  check them.

[codecov]

Codecov Report

❌ Patch coverage is 22.22222% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.00%. Comparing base (bb4d39f) to head (1784929).
⚠️ Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/pillar/cmd/volumemgr/retryvolumedelete.go	29.41%	24 Missing ⚠️
pkg/pillar/cmd/volumemgr/handlevolume.go	0.00%	9 Missing ⚠️
pkg/pillar/cmd/volumemgr/volumemgr.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6176      +/-   ##
==========================================
+ Coverage   22.62%   23.00%   +0.38%     
==========================================
  Files         506      517      +11     
  Lines       93198    94953    +1755     
==========================================
+ Hits        21082    21841     +759     
- Misses      70392    71203     +811     
- Partials     1724     1909     +185     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[europaul]

mirrors @eriknordmark 's solution for creating the volumes - LGTM

[rene]

@claude

[claude]

Claude finished @rene's task in 3m 19s —— View job

---

Code review

Reviewed the diff against origin/master. This is a clean, well-scoped fix that faithfully mirrors the established retryclustervolume.go pattern (pure decision function + bounded gc-driven retry + in-memory count). It also correctly fixes the latent nil deref (vr.Error.Error() when vr.Error == nil). The core change — keeping the VolumeStatus published in Deleting and re-driving from the gc tick instead of unpublishing-and-leaking — is sound.

I verified the concurrency-adjacent concern: re-driving while a destroy is still in flight is safe — worker.TrySubmit returns JobInProgressError, and AddWorkDestroy treats that as non-fatal (no double execution, no Fatalf). The retry budget is intentionally wall-clock-bounded (~gc interval × count), as documented.

Two non-blocking observations posted inline:

• Give-up path re-hides the orphan (retryvolumedelete.go:71) — on give-up the status is unpublished, reintroducing the exact invisibility this PR fixes (just deferred ~72 min). The sibling cvGiveUp leaves a terminal published error for operator visibility; consider mirroring that.
• Stale retry-count entry (retryvolumedelete.go:64) — the count is only cleared on success/give-up, so a volume that leaves the candidate set another way (e.g. re-referenced) keeps a lingering count that shortens a later delete's budget.

Neither blocks merge. Nice work — LGTM overall, matching @europaul's approval.
· fix-volumedelete-after-prune

[rene]

mirrors @eriknordmark 's solution for creating the volumes - LGTM

@europaul , could you please point to which PR are you referring to?

[europaul]

mirrors @eriknordmark 's solution for creating the volumes - LGTM

@europaul , could you please point to which PR are you referring to?

#6121

[rene]

@claude

[claude]

Claude finished @rene's task in 1m 12s —— View job

---

Re-review — both fixes confirmed

Verified the two follow-up points from my earlier review are now handled correctly.

• Give-up path no longer re-hides the orphan — vdGiveUp (retryvolumedelete.go:81) now keeps the VolumeStatus published in Deleting with its terminal error, only popping the stale worker result and bumping the count past the cap so later gc ticks hit vdParked and neither re-drive nor re-log. This mirrors the cluster-create give-up and keeps a permanently-undeletable volume visible in status. ✅

• Stale retry-count entry cleared — vdSkip (retryvolumedelete.go:74) now deletes the map entry, so a volume that leaves the candidate set another way (e.g. re-referenced during a redeploy) can't resume a later delete from a shortened budget. maybeDeleteVolume also clears it on the success/unpublish path (handlevolume.go:336). ✅

A couple of small things I checked while re-reading — both fine, no action needed:

• The delete-failure branch in maybeDeleteVolume returns without popping the worker result; that's intentional, since vdGiveUp later pops it via popVolumeWorkResult, and a re-driven AddWorkDestroy produces a fresh result. No result is leaked.
• While a destroy is in flight, gc ticks can increment volumeDeleteRetryCount faster than actual delete attempts (each vdRedrive re-submits and TrySubmit returns JobInProgressError), so the budget is wall-clock-bounded (~gc interval × maxVolumeDeleteRetries) rather than attempt-bounded. This is consistent with the documented retryclustervolume.go pattern and the code comments say as much.

The added unit test pins the boundary (under cap → redrive, at cap → give up once, beyond cap → parked, plus the skip / in-flight cases). LGTM.
· fix-volumedelete-after-prune

[eriknordmark]

Built fault injection + an end-to-end eden test that reproduces the failed-delete scenario this PR fixes, and used them to validate the fix both ways.

Tooling (draft PRs):

• volumemgr: build-tagged fault injection for volume delete #6190 — build-tagged (FAULT_INJECTION=y) fault gates on the delete path: /tmp/VolumeDestroy_FaultInjection_Fail fails DestroyVolume before the replicated-volume skip (so it fires for replicated Longhorn volumes too), and /tmp/DeletePVC_FaultInjection_Fail fails DeletePVC. Both are no-op stubs without the tag, so shipped images are unaffected.
• tests: add volume-delete retry e2e test eden#1217 — an evetestkit suite: TestVolumeDeleteRetryRecovers (retain→retry→recover) and TestVolumeDeleteGivesUp (bounded give-up).

Results (host EVE-k, current master + this PR):

• ✅ TestVolumeDeleteRetryRecovers — on a failed destroy the VolumeStatus is retained in SubState=Deleting with the error and re-driven off the gc tick; clearing the fault lets the retry succeed and the volume unpublishes.
• ✅ TestVolumeDeleteGivesUp — with the fault held, the volume is given up on (unpublished) after maxVolumeDeleteRetries.

Negative control — same tooling on master without this PR (branch eriknordmark/eve:volumedelete-fault-nofix):

• ❌ TestVolumeDeleteRetryRecovers fails at "was not kept in SubState=Deleting" — the pre-fix code unpublishes the failed delete immediately (the leak), so the test never observes the retained state. This confirms the test genuinely catches the bug this PR fixes, rather than passing regardless.

One caveat on the give-up test: give-up = maxVolumeDeleteRetries × the gc tick (timer.gc.vdisk/10 s), and volumemgr only creates its gc ticker at boot — so exercising give-up quickly needs timer.gc.vdisk=60 and a reboot. Noted as a follow-up in eden#1217.

[eriknordmark]

I did some fault injection testing to show that EVE master has issues in this space and that they are fixed by this PR.