[DO NOT MERGE] eve-k: reclaim the purged generation's PVC - #6318
eriknordmark wants to merge 4 commits into
Conversation
On EVE-k a purge deletes the stale generation's VMIRS and pod but leaves its PVC behind, so the Longhorn volume underneath is never reclaimed and the space stays consumed for the life of the device. Extend volumemgr's periodic garbage-collection pass to cover PVC-backed volumes: a PVC with no currently-published VolumeStatus is collected, mirroring what the pass already does for file-backed volumes. Runs only on EVE-k. Originally part of lf-edge#6257, which merged without it. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestVMAppPurgeAfterPowerCycle checks the old generation's disk is gone on the local-hypervisor path but skips the equivalent check on Kubevirt, so an interrupted purge that strands the old generation's PVC leaves the test green. Enable the check on both paths. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A purge must not gate its teardown on the app's designated node, nor on wherever a replica happens to be scheduled: neither signal is both durable and liveness-aware, so either one deadlocks a purge issued while the designated node is down. Add a three-node cluster test for that case. The app is deployed with a preferred designated node, that node is powered off so KubeVirt reschedules the replica elsewhere, and the purge is issued while it is still down; the surviving workload must be exactly one VMIRS, named for the new generation. Originally part of lf-edge#6257, which merged without it. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A PVC can stay Pending forever. Its ProvisioningFailed events alternate "volume not found" and "volume already exists" - the provisioner created the Longhorn backend volume once, lost track of that success in its own cache, and keeps retrying a name it no longer recognizes. This hits TestVMAppPurgeDuringFailover at first-ever PVC creation, timing it out before it reaches the failover step it tests. Confirmed live with no EVE or pillar change involved: deleting the csi-provisioner pod forces a fresh leader election and cache, and the next retry succeeds. waitForAppRunningMitigatingPVCStall wraps the app-running wait with a background watcher that restarts csi-provisioner once, and only after a PVC has shown the failure signature continuously for two minutes. A PVC that clears on its own before then passes through untouched, and the wait's own timeout and failure behavior are unchanged. It is wired into the failover test alone, the one test observed to fail this way, because restarting a cluster-wide Longhorn pod is not something to do from every test that creates a volume. Marked REMOVE ME: it works around infra, not anything this suite tests. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6318 +/- ##
==========================================
+ Coverage 24.67% 25.04% +0.37%
==========================================
Files 514 524 +10
Lines 94151 95880 +1729
==========================================
+ Hits 23231 24014 +783
- Misses 69057 69814 +757
- Partials 1863 2052 +189 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closing: every part of this PR now lives elsewhere. The The two evetest commits with no counterpart there — |
Description
Draft, for comparison — not competing for merge. @andrewd-zededa is preparing
the production version of this work, which handles multi-node clusters and makes
the lifecycle consistent across the designated node and the rest of the cluster.
This branch is the single-node reference implementation that has been running on
a local integration branch, opened so the two can be read side by side. Prefer
his where they disagree.
The leak
On EVE-k a purge deletes the stale generation's VMIRS and its pod, but not its
PVC. The Longhorn volume underneath is never reclaimed, so the space stays
consumed for the life of the device. #6257 fixed the duplicate-generation bug
and merged;
sweepStaleGenerationsthere deletes the workload objects and stopsshort of the disk.
An ordinary purge does not leak — it cleans up its own PVC. The orphan appears
when a purge is interrupted such that
sweepStaleGenerationsis what reaps thestale generation, so accumulation tracks interruption frequency rather than purge
frequency.
The fix
volumemgr's periodic garbage-collection pass already collects file- and
dataset-backed volumes with no currently-published
VolumeStatus. Extend it toPVC-backed ones on the same rule, running only on EVE-k. A volume this node
still tracks is never collected, including one it tracks only as a failover
candidate for an app running elsewhere in the cluster.
Commits
volumemgr: reclaim orphaned PVCs on EVE-k— the fix, plus unit tests forthe reap decision (which is separable from the Kubernetes delete call, so it
is testable without one).
evetest: assert the purged PVC is reclaimed—TestVMAppPurgeAfterPowerCyclechecks the old generation's disk is gone on the local-hypervisor path but
skips the equivalent on Kubevirt, so the leak leaves it green. Enables the
check on both paths.
evetest: purge with the designated node down— a three-node clustertest: app deployed with a preferred designated node, that node powered off so
KubeVirt reschedules the replica, purge issued while it is still down. This is
the case where gating the teardown on the designated node — or on wherever a
replica currently happens to be scheduled — deadlocks. It is also the area
Andrew's version addresses properly; treat it as a statement of the problem
rather than as the answer.
evetest: work around a Longhorn CSI-provisioner stall— marked REMOVE MEin its own file. Works around infra, not anything this suite tests. Drop this
commit if you would rather the test flake than carry it.
PR dependencies
None. #6257 has merged; this applies on top of it.
How to test and validate this PR
Unit —
make -C pkg/pillar test. The reap decision is covered directly:a published volume is never reaped, an unknown one is, a replicated one is not,
and one unparseable candidate is skipped rather than stopping the pass.
Negative control run locally: neutering the "still published" guard makes
TestVolumesToReapSkipsLiveVolumefail on its assertion, and it passes againonce restored — so the test can actually fail.
e2e —
EVETEST_HYPERVISOR=kubevirt make evetest NAME=TestVMAppPurgeAfterPowerCycle.Without the fix this now fails on the assertion enabled by commit 2:
-pvc-0is generation 0 — the old generation's disk, the objectsweepStaleGenerationsleaves behind. With the fix the same scenario passes.That A/B was run locally, one image per leg differing only in the three
cmd/volumemgrfiles, both legs driven from the tree where the assertion islive: power-cycle × no-reclaim failed, power-cycle × reclaim passed, and the
plain-purge baseline passed on both — which is what establishes that a healthy
purge does not leak and that the assertion is able to fire.
TestVMAppPurgeDuringFailoverneeds a three-node cluster and has not been runagainst this branch.
Not verified here: the leak rate under repeated interrupted purges, and
whether accumulation causes Longhorn storage pressure or downstream volume
failures.
Changelog notes
Fixes a bug on eve-k (kubevirt hypervisor) where a purge interrupted by a reboot
could leave the old application generation's disk behind indefinitely, consuming
storage for the life of the device. Adds automated tests for purge storage
reclaim.
PR Backports
Checklist
And the last but not least:
check them.
Notes on the unchecked boxes:
pkg/pillar/docs/volumemgr.mdis untouched — sayso if the GC pass gaining a third backend warrants a line there. The amd64 e2e
evidence is the A/B described above rather than a run of this exact branch;
arm64 has not been exercised. Labels are left for whoever picks this up, given
it is a draft.