eve-k: fix purge leaving stale VMIRS generations and volume refs - #6257
Draft
andrewd-zededa wants to merge 8 commits into
Draft
eve-k: fix purge leaving stale VMIRS generations and volume refs#6257andrewd-zededa wants to merge 8 commits into
andrewd-zededa wants to merge 8 commits into
Conversation
Adds the cluster-level reproduction suite for purge scenarios that interrupt the app between issuing a purge and it completing: a plain purge as the control case, a purge issued while the device is powered off, and a purge issued while the app's designated node is down and a failover is in progress. These are what caught the kubevirt VMIRS duplication bug and, later, the zedmanager volume-ref wedge, both fixed in this branch's subsequent commits. - TestVMAppPurgeBaseline: purge a healthy, undisturbed app; exactly one VMIRS and one volume generation before and after, named/keyed for the new generation. - TestVMAppPurgeAfterPowerCycle: purge issued while the device is powered off, then powered back on - the deterministic reboot-mid- purge repro. Same end-state assertions as the baseline test. - TestVMAppPurgeDuringFailover: the app's designated node is powered off, KubeVirt reschedules the replica onto another node, and a purge is issued while the designated node is still down - this rules out gating the teardown on the designated node or on current replica placement, since a purge in this window must not wait on the dead node. - helpers_test.go: shared fixtures and assertions for all three - device/app config, VMIRS/volume/purge-counter inspection via EdgeDevice.RunShellScript and EVE's own published state, and a live-cluster-info watcher for detecting a failover without racing a stale cached read from the node the app is failing over away from. Supporting framework additions: - EdgeDevice.PowerOff/PowerOn: hard power control through the broker, distinct from HardReboot - the device does not come back on its own after PowerOff. - EdgeCluster.FindAppNodeName exported (was findAppNodeName) so the failover test's own node-matching helper can reuse it instead of duplicating the logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Durbin <andrewd@zededa.com>
…le-generation helper Groundwork for fixing app purges losing track of the previous VMIRS generation. No behaviour change except logging. - Fix an inverted IsNotFound check in StopReplicaVMI: on a successful delete, err is nil, so IsNotFound(nil) was false and the success path fell into the error branch, logging "Stop VMI Replicaset error <nil>" at error level on every successful delete. Success, NotFound, and a real API error are now distinguished correctly; the returned error is unchanged in all three cases. - Wrap the kubevirt and k8s client constructors (newKubevirtClient, newK8sClient) as package-level vars. kubecli's only built-in test hook overrides a different constructor than the one this file actually calls, so unit tests had no way to inject a fake client; this makes - Add logging at every previously-silent return in Info, replicaVmiScheduledOnMe, and podListToSchedulingState, including the branch that returns "not scheduled on this node" - today's logs can't distinguish that outcome from several others that share the same return value, which previously required a full code read to resolve during an incident. - Add a pure, side-effect-free helper (staleVMIRSGeneration / staleVMIRSNames) that identifies superseded VMIRS generations for an app by its App-Domain-Name label prefix, object name, and purge counter suffix. Not wired up to any caller yet. Tests: TestStopReplicaVMIErrorHandling, TestStaleGenerationPredicate, TestStaleVMIRSNames, TestTrailingCounter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Durbin <andrewd@zededa.com>
… a random number and replica placement Carries the DomainStatus through to Task() and uses it to make the purge-critical parts of the kube path trustworthy: what a workload's id means, and when Info is allowed to say "gone". - Task() now returns a kubevirtTask wrapper carrying the DomainStatus instead of discarding it. Every existing method not touched here (Stop, Delete, Cleanup, Start, etc.) is served unchanged via Go's method promotion; the wrapper adds kubeName()/metaType(), which derive identity straight from status instead of depending on the vmiList cache. - domainID is no longer rand.Uint32(). It's now workloadID(uid, kubeName): an FNV-1a hash of the VMIRS's real metadata.uid, falling back to hashing kubeName before the object exists (Create runs before Start creates it). Start and StartReplicaPodContiner capture the UID from their own Create response and update the cached id if it changed, logging the transition. Create() no longer dereferences an unguarded map entry on a cache miss - it falls back to the same derivation instead of risking a nil-pointer panic. - Info is rewritten to check VMIRS existence directly (a new replicaSetUID helper, via Get) instead of inferring it from replica placement. The contract: DomainId is zero if and only if the object is confirmed absent (NotFound). An unreachable API or any other existence-check failure returns the caller's last-known DomainId with UNKNOWN - never zero, never fabricated. Found-but-not-yet-scheduled- anywhere and found-with-an-unmapped-phase both return a fresh non-zero id with SCHEDULING (held). Found-and-running-on-a-different- node keeps UNKNOWN but with the fresh non-zero id rather than zero - returning SCHEDULING there instead would send verifyStatus into its rescheduling arm and make a healthy app on another node report BOOTING forever. - Two supporting fixes Info's rewrite depends on: podListToSchedulingState's second return value was hardcoded true for any non-terminating pod regardless of whether it was actually bound to a node; fixed to mean "not bound to any node anywhere" (existing test updated to match). stateMap was missing the "Scheduled" KubeVirt phase entirely (only had "Scheduling"), which fell through to the generic unmapped-phase branch. Cleanup is deliberately left untouched - it wasn't clearly required for the contract fix and touching it risked scope creep unverifiable without a live cluster. Tests: TestTaskDerivesKubeName, TestTaskMetaType, TestWorkloadID, TestInfoContract, TestInfoUnreachableKeepsLastID, TestCreateReturnsNonZero. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Durbin <andrewd@zededa.com>
…e next one Builds on the derived-identity and honest Info-return work already in this branch: domainmgr no longer lies about a domain's existence, but nothing yet stopped an old generation from surviving a purge. This adds the sweep, fixes the one already-committed bug that would have made it a no-op, and documents the resulting contract. - Setup/Start now run an unconditional sweep before creating a domain: enumerate every VMIRS (or, for a NOHYPER app, plain ReplicaSet) belonging to the app, delete every generation older than the one about to be created, and confirm each is actually gone - object and pods, not just the object, since Kubernetes' garbage collection of the pod is asynchronous and generations share MACs, veth names, and the RWO disk. A confirm-absence failure fails Start outright rather than risk creating a second generation alongside one that might still exist. Unconditional matters specifically for a reboot mid-purge: domainmgr's own state lives in /run and does not survive it, so a purge in progress across a reboot is not always re-detected as a purge on the next boot; the sweep doesn't depend on detecting one, only on enumerating what actually exists. - Fixes a real bug in the stale-generation helper committed earlier: it read a VMIRS's own ObjectMeta.Labels for the App-Domain-Name label, but CreateReplicaVMIConfig never sets that field - the label only lives on Spec.Selector.MatchLabels (confirmed against a live VMIRS's actual JSON). Without this fix the sweep would never have matched a real VMIRS. A NOHYPER app's plain ReplicaSet has no existing field to reuse (its own selector is generation-specific, not UUID-scoped), so it gets a new top-level label instead. - getVMIStatus now skips terminating VMIs when matching by GenerateName and NodeName. During a same-node replica restart, the outgoing and incoming VMI briefly share both, and picking whichever the API happened to list first - rather than the live one - is what made a confirm-absence wait latch onto the object already on its way out. - Two small refactors needed to make any of this testable: newK8sClient now returns the kubernetes.Interface it was always used through, not the concrete *kubernetes.Clientset kubernetes.NewForConfig returns, so tests can substitute a fake client. getVMIStatus/GetDomsCPUMem called kubeapi.GetKubeConfig() directly instead of going through the existing swappable client vars; wrapped as getKubeConfig for the same reason. - Documents the DomainId contract where the field lives (types/domainmgrtypes.go, previously a bare, uncommented int) and fixes a misleading comment on it in kubevirt.go. Adds a section to zedkube.md on the kube app lifecycle: the naming scheme, why generations collide when both briefly exist, the Info return contract, and the sweep - and corrects an existing inaccuracy there (it said the unknown-state escalation was 5 minutes; the actual constant is 30). Tests: TestSweepDeletesOnlyOlderGenerations, TestSweepConfirmAbsenceTimeoutFails, TestWaitForVMITargetsCorrectObject, TestStalePodReplicaSetNames. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Durbin <andrewd@zededa.com>
…omplete When a purge changes an app's desired volume, doInstall marks the old VolumeRefStatus entry pending removal and waits for volumemgr to publish a matching delete before dropping it. If volumemgr never had a live status for that entry to begin with in this boot (e.g. a reboot wiped ephemeral state before volumemgr got a chance to republish it), no delete event will ever arrive, and the app is stuck in DownloadAndVerify permanently - never even requesting the new volume. Check volumemgr's live VolumeRefStatus directly before parking the entry: if it's already absent, drop it immediately instead of waiting on a confirmation that can't happen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Durbin <andrewd@zededa.com>
assertSingleVolumeGeneration only reads volumemgr's own VolumeStatus publication, so if volumemgr's bookkeeping already considers a stale generation's volume gone while the underlying PVC was never actually deleted in Kubernetes, every existing purge test still reports a clean pass - exactly what was observed live: a stale PVC left Bound after a purge zedmanager reported complete. Adds assertNoOrphanedPVCs, which cross-checks every PVC actually present in the cluster against the PVC names volumemgr's current VolumeStatus publications would produce (VolumeStatus.GetPVCName), so a PVC unaccounted for by any of them is flagged. Wired into the baseline and after-power-cycle tests' end-state assertions; not the failover test, matching its existing note that clustered volume semantics aren't established there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Durbin <andrewd@zededa.com>
TestDoInstallDropsStaleVolumeRefWithNoLiveStatus only checked that the stale AppInstanceStatus entry was dropped, not that zedmanager's own VolumeRefConfig request to volumemgr was actually unpublished. A future change that dropped the status entry without calling MaybeRemoveVolumeRefConfig/unpublishVolumeRefConfig first would still have passed. The test now publishes a live VolumeRefConfig for the stale ref up front and asserts it is gone afterward. Also replaces the UUID fixtures with synthetic ones - they were real identifiers from the live debugging session that reproduced this bug, which should never have ended up committed. Signed-off-by: Andrew Durbin <andrewd@zededa.com>
…ition The sweep section described itself as the fix for reboot-mid-purge without saying what it doesn't cover: it reconciles VMIRS/ReplicaSet objects and their pods only, never the stale generation's PVC, so a disk can still be left behind indefinitely - as observed live, and still an open gap. It also implied the sweep runs unconditionally on every Start without noting that it only runs at all once Start is actually called. A purge issued while the device is off can leave zedmanager waiting forever on a VolumeRefStatus deletion from volumemgr that will never arrive (volumemgr's own state for that reference was wiped by the same reboot before it was ever asked about it again this boot), so domainmgr's Start for the new generation is never invoked and the sweep never gets a chance to run - independent of how correct the sweep is on its own terms. Documents both gaps and how the two fixes in this branch are complementary. Signed-off-by: Andrew Durbin <andrewd@zededa.com>
github-actions
Bot
requested review from
OhmSpectator,
eriknordmark,
rene,
rucoder and
shjala
July 31, 2026 23:30
Contributor
Author
|
I need to take another pass on this and get the verbosity on some the comments down and some verification on the new evetest tests. |
andrewd-zededa
added
stable
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6257 +/- ##
==========================================
+ Coverage 22.93% 23.60% +0.66%
==========================================
Files 510 522 +12
Lines 93473 95430 +1957
==========================================
+ Hits 21440 22525 +1085
- Misses 70292 70939 +647
- Partials 1741 1966 +225 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
milan-zededa
reviewed
Aug 3, 2026
| // the EVE API. found is false if the file does not exist yet (e.g. before | ||
| // the app's first purge). | ||
| func purgeCounter(dev *evetest.EdgeDevice, appUUID uuid.UUID) (counter int, found bool) { | ||
| path := "/persist/status/zedmanager/UuidToNum/" + appUUID.String() + ".json" |
Contributor
There was a problem hiding this comment.
Note that in my outstanding PR I made improvements to ReadPublication method provided by evetest.
It now returns true/false instead of fatal on missing file, so you could use that instead of creating a custom method (once my PR is merged): https://github.com/milan-zededa/eve/blob/c5ad210e94476a545d7f4dfe5e0b4b77d7f6d71c/evetest/edgedevice.go#L2671-L2685
Contributor
Author
There was a problem hiding this comment.
Sounds good, I'll rebase once merged
milan-zededa
reviewed
Aug 3, 2026
| // is now scheduled on a node other than excludeDevName, and returns the | ||
| // EdgeDevice matching that node name. | ||
| // | ||
| // This deliberately does NOT use EdgeCluster.FindDeviceHostingApp: that |
Contributor
There was a problem hiding this comment.
If this can be useful for other (e.g. cluster/failover) tests, maybe we should make this a method of EdgeCluster
milan-zededa
reviewed
Aug 3, 2026
| }) | ||
| } | ||
|
|
||
| // PowerOff performs a hard power-off of the device through the broker |
Contributor
There was a problem hiding this comment.
I also happen to add PowerOff/PowerOn methods in my outstanding PR :)
Contributor
Author
There was a problem hiding this comment.
Good, I'll rebase once merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes two related but independently-triggerable bugs on eve-k (kubevirt
hypervisor) where an application purge can leave the app in a broken state,
most reliably reproduced when the purge is issued while the device is
powered off (so the purge is only detected/applied across a reboot).
Bug 1 - stale VMIRS/ReplicaSet generation survives a purge (
hypervisor/kubevirt.go).Infocould report a fabricatedDomainIdof0when it genuinely couldnot confirm whether the previous generation's object existed (e.g. an
unattributed replica during a restart, or - after a reboot - domainmgr
having no
DomainConfig/DomainStatusat all, since/runis tmpfs anddoes not survive a reboot). A
0id is exactly what tells domainmgr "alreadytorn down," so the old generation's teardown was silently skipped and the
purge counter advanced anyway, leaving two VMIRS generations running side
by side indefinitely.
Fix:
Infonow derivesDomainIdfrom the VMIRS/ReplicaSet's actualmetadata.uidvia a direct existence check, and the contract is explicit:DomainIdis zero if and only if the object is confirmed absent. A newunconditional stale-generation sweep runs before every
Start, enumeratingand tearing down (and confirming actually gone - object and pods) any
generation older than the one about to be created, independent of whether
a purge was even detected this boot - which matters specifically because a
reboot mid-purge is not always re-detected as a purge on the next boot. Also
fixes a real bug in the stale-generation helper that read the wrong object
field for the app-identifying label, which would have made the sweep a
no-op.
Bug 2 - a purge can wedge zedmanager forever in
DownloadAndVerify(
cmd/zedmanager/updatestatus.go).AppInstanceStatusis rebuilt fromscratch on every zedmanager restart. When a purge changes the app's desired
volume,
doInstallmarks the oldVolumeRefStatusentry pending removaland waits for volumemgr to publish a matching delete before dropping it. If
volumemgr's own ephemeral state for that reference was also wiped by the
same reboot and it is never asked about that reference again this boot, no
such delete event will ever arrive - the app is left parked in
DownloadAndVerifypermanently, never even requesting the new volume (and,as a consequence, domainmgr's
Startfor the new generation is nevercalled, so the sweep above never gets a chance to run either).
Fix:
doInstallnow checks whether volumemgr has any live status for thestale reference at all before deciding to wait on it, and drops it
immediately if not, instead of waiting on a confirmation that can never
come.
The two fixes are complementary and independently reproducible/verifiable
(see
pkg/pillar/docs/eve-k-purge-fault-injection-runbook.md).Known open item, not fixed here: the stale-generation sweep reconciles
VMIRS/ReplicaSet objects and their pods only, not PVCs - a stale
generation's disk can still be left behind (
Boundbut unreferenced)after an otherwise-clean purge. Documented in
pkg/pillar/docs/zedkube.mdas a still-open gap.
PR dependencies
None.
How to test and validate this PR
Unit tests (Docker-based):
make -C pkg/pillar test
Covers:
pkg/pillar/hypervisor(VMIRS/ReplicaSet client mocking, thestale-generation sweep,
Info's existence-confirmation contract, terminating-VMI targeting) and
pkg/pillar/cmd/zedmanager(the stale-volume-ref drop/waitbehavior).
Cluster-level evetest (requires a live/emulated eve-k environment):
make evetest NAME=TestVMAppPurgeBaseline
make evetest NAME=TestVMAppPurgeAfterPowerCycle
make evetest NAME=TestVMAppPurgeDuringFailover
TestVMAppPurgeBaseline: control case, a plain purge of a healthy app.TestVMAppPurgeAfterPowerCycle: the deterministic repro for both bugs -power the device off, issue a purge, power it back on; asserts exactly
one VMIRS at the new generation, the purge counter advanced by exactly
one, a new volume generation, and no orphaned PVC.
TestVMAppPurgeDuringFailover: purge issued while the app's designatednode is down and a failover to another node is in progress.
Manually verified against a live 3-node eve-k cluster: reproduced the
original duplicate-VMIRS failure and the zedmanager wedge on a pre-fix
build, and confirmed both are resolved on this branch. See
pkg/pillar/docs/eve-k-purge-fault-injection-runbook.mdfor hand-runfault-injection recipes covering scenarios not practical to automate
(timing-dependent races, blocking the kube API mid-purge, etc.).
Changelog notes
Fixes a bug on eve-k (kubevirt hypervisor) where purging an application
around a device reboot could leave a duplicate/stale VM instance running
alongside the new one, or leave the purge stuck indefinitely instead of
completing. No other user-facing changes.
PR Backports
Checklist
And the last but not least:
check them.
Please, check the boxes above after submitting the PR in interactive mode.