kubevirt: keep domain bookkeeping until Cleanup

[eriknordmark]

Description

Fixes to the KubeVirt hypervisor backend, found while investigating why an app
restart on EVE-k behaves differently from the same operation on EVE-kvm.

This branch is now based on #6257 — see "PR dependencies" below. Its first two
commits are Andrew Durbin's from that PR, included verbatim (authorship preserved)
because #6257 rewrites the same Info/StopReplicaVMI code this PR touches, and the
two cannot be reviewed or tested independently of each other. Review the four
kubevirt: commits here.

1. kubevirt: keep domain bookkeeping until Cleanup

domainmgr asks the hypervisor to shut a domain down and then polls Info()
until it reports the domain gone, before creating any replacement.
kubevirtContext.Stop() dropped the vmiList entry that Info() resolves a
domain through, so every poll after Stop failed. waitForDomainGone treats a
failed poll as "the domain is gone", so it returned on its first one-second tick
and cleared DomainId.

The effect is that domainmgr requests deletion of a VMIRS and creates the
replacement roughly a second later, while KubeVirt is still shutting the old VMI
down — virt-handler signals a graceful shutdown and only kills the VMI once the
grace period expires (30 s by default). On a lab device two virt-launcher pods
for the same app were observed running at once as a result. Clearing DomainId
also made the forced-shutdown escalation in doInactivate unreachable, since it
is guarded on DomainId being set.

The entry now stays in place in Stop and is removed in Cleanup, the
unconditional tail of doInactivate. Delete cannot own the removal: it only
runs while DomainId is still set, which is no longer true once the domain goes
away during the graceful wait, and Cleanup's IsMetaReplicaVMI path did not
remove it either — so dropping the Stop removal on its own would leak an entry
on every successful graceful shutdown. The removal is deferred, since Cleanup
can fail after the domain is already on its way out.

For contrast, the same sequence on EVE-kvm polls real domain state, backs off
1/2/4/8/16/32 s, and escalates before anything replaces the domain, because the
containerd-backed Stop it inherits keeps no such state.

2. kubevirt: name the replicaset in the delete error

A failed VMIRS delete logged only the error, so an operator could not tell which
app's replicaset failed to go away — and this path fires during every teardown that
races the Kubernetes API. The name is now included, and the trailing newline logrus
adds itself is dropped.

An earlier version of this PR also carried kubevirt: log a successful VMIRS delete correctly, fixing errors.IsNotFound(nil) being false so a successful delete
logged Stop VMI Replicaset error <nil>. That commit is gone: #6257 fixes the same
bug with a three-way switch that additionally logs the success case, so keeping
both would have double-applied it.

3. kubevirt: don't ignore the scheduling lookup error on Delete

Delete asked scheduledOnMe() which node the domain is placed on and then
discarded its error. A failed lookup reports onMe=false, which is indistinguishable
from the legitimate "scheduled on another node" case tested on the next line, so
Delete returned nil — reporting a successful teardown while deleting nothing —
whenever the Kubernetes API was briefly unavailable. Stop already checks the same
error. (Not every failure path is (false, false): replicaPodScheduledOnMe's
"Unhandled scheduling state" returns scheduledOnNone=true alongside its error, so the
check has to key on the error rather than on scheduledOnNone.)

All five Delete call sites in domainmgr log the error and continue (three of
them are behind !ctx.hvTypeKube and unreachable on EVE-k), so no caller's
control flow changes. Note this also surfaces an already-deleted ReplicaSet as a
Delete error rather than a silent success, since the lookup's Get reports
NotFound through the same path.

4. kubevirt: treat a mid-call NotFound as gone

Info confirms the replicaset exists and then fetches it again to read the guest's
phase. A workload deleted between those two calls made the second lookup fail with
NotFound, which Info reported as an error alongside a still-scheduling state —
and waitForDomainGone returns true on any Info error before it inspects the
state, so the graceful wait ended early. That is the same premature "domain is
gone" commit 1 exists to prevent, reached through a one-poll race rather than on
every shutdown. NotFound now reports absence identically wherever it is observed,
which also drops a redundant third Get of an object already known to be gone.

Raised by @naiming-zededa in review: the concern was that keeping the vmiList
entry past Stop would let a deleted replicaset reach scheduledOnMe and be
reported BROKEN. #6257's Info rework removed that specific outcome, but the
error path it named was still wrong for the reason above.

Known gaps (deliberately not addressed here)

• Stop's force parameter is still ignored. This matches KvmContext.Stop,
  which also discards it (_ bool); in both backends the actual forcing is done
  by the subsequent Delete. Now that the escalation is reachable, a stuck
  domain can spend an extra maxDelay in force-true before Delete runs.
  The KVM-analogous fix would be to make kubevirt's Delete the forcing step
  (delete with GracePeriodSeconds: 0), not to act on force in Stop.
• An error returned from Stop still skips the graceful wait entirely, because
  doInactivate only calls waitForDomainGone on the success branch.

PR dependencies

Depends on #6257 (eve-k-purge-lost-delete, draft). Its two commits —
evetest: add purge tests for the kube path and pillar: keep purges from leaving the old app generation behind — are the base of this branch, included verbatim at
be7740c59 (d36df5bbe and be7740c59) with Andrew Durbin's authorship and sign-off
intact. They are not proposed as new work here.

That base builds: go build -tags k ./... and the hypervisor and zedmanager tests are
clean on it, and on this branch on top of it. #6257 has force-pushed repeatedly, and one
of its earlier tips did not compile, so this PR pins a snapshot that does rather than
tracking its head continuously. Expect a rebase when #6257 settles.

Why stacked rather than independent: #6257 rewrites Info() to derive DomainId from a
live VMIRS Get instead of from vmiList, and fixes the StopReplicaVMI logging this
PR originally fixed. The two overlap in the same functions, so they conflict textually
and cannot be tested apart. When #6257 merges, this branch should be rebased and its two
commits will drop out. #6257 force-pushes frequently, so expect re-bases.

How to test and validate this PR

Automated: go build -tags k ./hypervisor/, go vet -tags k ./hypervisor/ and
go test -tags k ./hypervisor/ under pkg/pillar. Note the k build tag is
required — hypervisor/kubevirt.go is behind //go:build k, so a build without
it does not compile this file at all. TestCreateReplicaPodConfig fails in that
bare host run, on master too — it mocks the kubeconfig at the real
/run/.kube/k3s/k3s.yaml, which a non-root user cannot create; CI runs in a
container as root and is unaffected (#6290).

On an EVE-k device, restart or deactivate an app instance and watch
waitForDomainGone in the device log:

• Before: a single waiting for 1s followed by error info domain <name>, then
  a replacement VMIRS created within about a second.
• After: state still RUNNING waited … with the delay backing off, until the
  VMI is actually gone (or the budget expires and the forced path runs), and no
  replacement until then.

Cross-check that kubectl get pods -n eve-kube-app never shows two
virt-launcher pods for the same app during a restart.

Result on hardware (amd64 EVE-k)

Run on a Supermicro SYS-E300-8D EVE-k node with the patched image, using the
preceding build without these commits as the control. The exercise is an
integration test that writes ~128 KB inside a VM app's guest and then restarts
the app instance; each test run does one cycle where the restart is requested
90–240 s after the write and one where it is requested in the same millisecond.

The graceful wait now happens. Time from the restart request to the app being
back online:

image	range	cycles
control	34.4 – 100.2 s	4
patched	113.5 – 167.3 s	6

The ranges do not overlap. That is the intended effect: waitForDomainGone polls
real domain state instead of accepting the first failed Info as "gone", so the
replacement is no longer created about a second after the delete request.

Data survival is improved but not fixed. Only the same-millisecond cycle ever
loses the guest's write:

cycle shape	control	patched
restart +90…240 s	kept 2/2	kept 3/3
restart +0.00 s	lost 2/2	lost 1/3

Why, from the device log. The wait this change enables cannot currently
succeed, because Stop deletes the VMIRS and the wait then polls that same
VMIRS by name:

13:45:08.446  DomainShutdown force-false 27d4050e….6.1        <- Stop deletes the VMIRS
13:45:08.706  waitForDomainGone …: waiting for 1s
13:45:09.761  waitForDomainGone … error Failed to determine scheduled node:
              virtualmachineinstancereplicasets.kubevirt.io "ztest-…-27d40-1" not found
13:45:09.762  waitForVMI … waiting for 1s     (then 2s, 4s, 8s, 16s, 32s)
13:46:13.278  waitForVMI … giving up at state:Unknown
13:46:13.278  VMI still available
13:46:13.307  doActivate({27d4050e… 7})       <- replacement created regardless

Every tick returns NotFound, so the state never resolves, the full 1+2+4+8+16+32
= 63 s backoff runs out, and the replacement is created with the old VMI still
present — by the code's own VMI still available on the preceding line. The
identical sequence appears in a cycle that kept its data (98ea9e82…, VMIRS
ztest-…-98ea9-1, give-up at 04:38:42.782, doActivate 20 ms later), so the
overlap is now the steady state of every EVE-k app restart on this build, and
whether the guest's last write survives is a race inside that window.

So this change converts a 1-second false "domain is gone" into a 63-second wait
that ends in the same conclusion. It removes the misleading bookkeeping and the
misleading log, and it is a prerequisite for a correct wait, but on its own it
does not stop the replacement from overlapping the old VMI. A follow-up should
make the wait poll the VMI object (which outlives its ReplicaSet) rather than the
ReplicaSet that Stop has just removed.

Also visible in both cycles, and probably worth its own fix:
kubeapi.DetachOldWorkload: a node name is required — the old workload's volume
detach does not happen.

That analysis was derived against master's Info, which returned an error whenever
vmiList had no entry for the domain. #6257, now the base of this branch, changes
exactly that: Info resolves existence through a live VMIRS Get and reports HALTED
only when the object is confirmed absent. How the graceful wait behaves on the stacked
branch therefore needs re-deriving, and I have not re-measured it on hardware since the
rebase — the numbers above are from the standalone form of these commits.

The rest of the suite was unaffected: 46 passes, plus failures this node also
produces without the change (it has no assignable audio/COM hardware).

Result in the kvm→k conversion matrix (with #5971)

Separately from the hardware run above, the two substantive commits were exercised in the
7-leg EVE-kvm→EVE-k boot-disk conversion matrix, on integration image
0.0.0-newgo-allprs2-f40d7876. That build carries #5971 (the Go kube-init daemon)
and rucoder#3 on top of the conversion chain, plus #6257, #6197, #6240, #6259 and
#6190 — so it is the combination this PR will actually ship into, not a standalone build.

7/7. Six legs green in the batch (ext4-shrink, twodisk-ext4, twodisk-zfs,
zfs-grow, ext4-toofull, zfs-notail); ext4-grow lost its emulator to a QEMU AHCI
abort (prdt_warnings=4 aborts=1) while another test shared the host, and passed on
re-run with prdt_warnings=0 aborts=0. Every leg came in faster than on the two previous
7/7 images of that branch line, including twodisk-zfs, which is the historically
fragile leg.

Scope of that evidence, precisely:

• It covers keep domain bookkeeping until Cleanup and don't ignore the scheduling lookup error on Delete — content-identical in that image to the commits here
  (patch-ids 7d1df4a12b16 and c21b35662d3f).
• It does not cover name the replicaset in the delete error or treat a mid-call NotFound as gone, both committed after that image was built.
• It demonstrates no regression in the conversion path with pkg/kube: port cluster-init.sh to Go daemon #5971's bring-up. It is
  not a targeted test of the graceful-stop behaviour itself; that evidence is the
  hardware section above.

Changelog notes

On EVE-k, an app restart or deactivate no longer creates the replacement while
the previous instance is still shutting down.

PR Backports

• 17.0-stable: To be decided by the maintainers — the change is small, but the
  behaviour it alters only matters for EVE-k.
• 16.0-stable: Same.
• 14.5-stable: Same.
• 13.4-stable: Same.

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

And the last but not least:

• I've checked the boxes above, or I've provided a good reason why I didn't
  check them.

Reasons for the unchecked boxes: no documentation change — this alters
bookkeeping ownership inside one hypervisor backend, with no new knob or
user-visible surface. The patched image has been run on an amd64 EVE-k device
(see the hardware result above); arm64 is untested.

[codecov]

Codecov Report

❌ Patch coverage is 46.94960% with 200 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.96%. Comparing base (6647569) to head (d66956b).

Files with missing lines	Patch %	Lines
pkg/pillar/hypervisor/kubevirt.go	36.45%	169 Missing and 14 partials ⚠️
pkg/pillar/cmd/volumemgr/initialvolumestatus.go	57.69%	9 Missing and 2 partials ⚠️
pkg/pillar/cmd/volumemgr/volumemgr.go	0.00%	2 Missing ⚠️
pkg/pillar/hypervisor/kubevirt_identity.go	80.00%	1 Missing and 1 partial ⚠️
pkg/pillar/hypervisor/kubevirt_staleness.go	95.45%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6271      +/-   ##
==========================================
+ Coverage   24.27%   24.96%   +0.68%     
==========================================
  Files         512      524      +12     
  Lines       93831    95836    +2005     
==========================================
+ Hits        22782    23930    +1148     
- Misses      69246    69873     +627     
- Partials     1803     2033     +230     

☔ 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.

[eriknordmark]

Correcting the hardware section I posted earlier today: it claimed the guest data
loss across an app restart was gone. A third run of the same test reproduced it,
and the device log shows this change does not stop the VMI overlap it was meant
to prevent. The description now reflects that; summary of what the run actually
shows:

What the change does do. Restart-request to app-online went from 34–100 s
(4 cycles, without these commits) to 113–167 s (6 cycles, with them), ranges not
overlapping. The graceful wait now happens instead of being skipped.

Why that is not enough. Stop deletes the VMIRS, and waitForDomainGone →
waitForVMI then resolves the domain through that same ReplicaSet name, so every
poll returns NotFound:

13:45:08.446  DomainShutdown force-false 27d4050e….6.1
13:45:09.761  waitForDomainGone … error Failed to determine scheduled node:
              virtualmachineinstancereplicasets.kubevirt.io "ztest-…-27d40-1" not found
13:45:09.762  waitForVMI … waiting for 1s   (2s, 4s, 8s, 16s, 32s)
13:46:13.278  waitForVMI … giving up at state:Unknown
13:46:13.278  VMI still available
13:46:13.307  doActivate({27d4050e… 7})

The state never resolves, the 63 s backoff expires, and the replacement is created
with the old VMI still present. The same sequence appears in a cycle that kept its
data, so the overlap is the steady state of every EVE-k app restart on this build
rather than the explanation for the one failure.

Data survival. Only the cycle where the restart is issued immediately after
the guest's write can lose it: 2 of 2 lost without these commits, 1 of 3 with
them. Too few samples to claim a real reduction.

Net: the three commits are still correct in themselves — the bookkeeping and the
two error paths — and a correct wait is impossible without the first one. But the
wait needs to poll the VMI object, which outlives its ReplicaSet, not the
ReplicaSet Stop has just deleted. Happy to take that on in this PR or a
follow-up, whichever reviewers prefer.

Separately, both cycles log kubeapi.DetachOldWorkload: a node name is required,
so the old workload's volume detach is not happening either.

[naiming-zededa]

in the Info(), this will allow the function to find the 'vmis' struct, and move on, but later, in the scheduledOnMe():

	onMe, _, err := ctx.scheduledOnMe(vmis.mtype, vmis.name)
	if err != nil {
		if isK3sUnreachable(err) {
			return 0, types.UNKNOWN, nil
		}
		return 0, types.BROKEN, logError("Failed to determine scheduled node: %s", err)
	}

if it is deleted in the kuberntes cluster, it is going to return here the types.BROKEN. which is also a problem.

Mayin in the 'vmis' struct, we can add a 'pendingOnDelete' bool, and set it here. Over in the Info(), checks on this bool on error, and return Unknown still.

[eriknordmark]

types.BROKEN is gone — #6257 rewrote Info() to confirm existence with its own Get first, so a deleted VMIRS returns HALTED/zero before scheduledOnMe runs. That is also why I didn't add pendingOnDelete: the live Get answers it better than a flag, which would be wrong if the delete itself failed.

The case underneath your comment is still real though. Info now Gets the same VMIRS up to three times, and a delete landing after the first is caught by none of them — the second's NotFound is dropped by the fall-through, the third ends at return id, types.SCHEDULING, err. waitForDomainGone returns true on any Info error before it looks at the state, so the race ends the graceful wait early.

Fixed in 5fafb53: errors.IsNotFound -> 0, HALTED, nil at both lookups, with a test that fails without it. Does that cover what you meant?

[naiming-zededa]

I think this code added is fine. but I think there is still a problem on the ScheduledOnMe() in the deletion case:
the function checks the VMIRS first, if this is gone, it returns 'not found', I think the problem is that, even the VMIRS can be removed quickly, the VMI itself may still stay around fairly longer. So, this deletion case, I think we need the same check as confirmVMIRSGone() to verify.

[eriknordmark]

Heads-up on a force-push that changes what this PR contains.

This branch is now based on #6257. Its two commits are the base here, included
verbatim at df8ae7c12 with @andrewd-zededa's authorship and sign-off intact — they are
not proposed as new work. #6257 rewrites Info() to derive DomainId from a live VMIRS
Get rather than from vmiList, which is the same code this PR touches, so the two
conflict textually and cannot be reviewed or tested apart.

Consequences worth flagging rather than leaving to be discovered in the diff:

• kubevirt: log a successful VMIRS delete correctly is gone. eve-k: fix purge leaving stale VMIRS generations and volume refs #6257 fixes that same
  bug with a three-way switch that also logs the success case, so keeping mine would
  have double-applied it. Replaced by a smaller kubevirt: name the replicaset in the delete error, which is the part eve-k: fix purge leaving stale VMIRS generations and volume refs #6257 does not cover.
• eve-k: fix purge leaving stale VMIRS generations and volume refs #6257's Info already prevents the premature "domain is gone" that
  keep domain bookkeeping until Cleanup was written for: waitForDomainGone returns
  true on any Info error, master's Info errored on a vmiList miss, and eve-k: fix purge leaving stale VMIRS generations and volume refs #6257's
  returns SCHEDULING with a non-zero id instead. What my commit still buys is that
  Info can read the guest's actual phase, so the wait ends when the guest stops rather
  than always running out the graceful budget and escalating. Reviewers may reasonably
  ask whether that is worth keeping on its own.
• The device-log analysis in the description predates this base and was derived
  against master's Info. I have not re-measured the graceful wait on hardware since the
  rebase, so treat those timings as describing the standalone form of these commits.

On testing: the two substantive commits have since been exercised in the 7-leg
EVE-kvm→EVE-k conversion matrix (7/7) on an integration image that also carries #5971,
rucoder#3 and #6257 — details in the description. That is a no-regression result for
the conversion path, not a targeted test of the graceful-stop behaviour.

When #6257 merges I will rebase and its commits will drop out. It force-pushes often, so
expect further re-bases here.

[eriknordmark]

Correction to my comment above: it said the included #6257 commits are "at df8ae7c12".
That is wrong — df8ae7c12 is not an ancestor of this branch. The commits actually
included are e9887bb1c and 73111d49f, an earlier #6257 tip. I refreshed a
separate integration branch to df8ae7c12 and then quoted that SHA here by mistake. The
description now states the correct base.

On why the base is not #6257's current tip: #6257's latest pillar commit
(74a5b1b0d) does not compile. replicaVmiScheduledOnMe was split into a
vmiScheduledOnMeFromVmirs helper whose body still references vmirsName, which is not
one of its parameters:

$ go build -tags k ./hypervisor/
hypervisor/kubevirt.go:1178:6: undefined: vmirsName
hypervisor/kubevirt.go:1183:5: undefined: vmirsName
hypervisor/kubevirt.go:1189:32: undefined: vmirsName
hypervisor/kubevirt.go:1201:75: undefined: vmirsName

Reproducible on 74a5b1b0d with none of this PR's commits present; its parent
4262a6bed builds cleanly, so the break is in that pillar commit. I did rebase this
branch onto it, resolved the one conflict in StopReplicaVMI, and then reverted — a
rebase would only turn this PR's CI red for a reason that is not this PR's. The rebase is
kept locally and will be redone once #6257 settles and builds.

@andrewd-zededa flagging the build break on your side.

Nothing else about this PR changed: the branch head is still 260c5a059, and
go build/vet/test -tags k ./hypervisor/ is clean on it.

[eriknordmark]

Rebased onto #6257's be7740c59, which fixes the undefined: vmirsName break I reported
above — go build -tags k ./... and the hypervisor and zedmanager tests are clean both
on that tip and on this branch over it. Branch head is now fc0f37411.

The three kubevirt: commits are unchanged in substance. The only conflict was in
StopReplicaVMI, resolved in favour of #6257's structure: it now names the replicaset in
the success log, so this PR's commit adds the name to the error log only and drops the
stray newline.

Re-derived after the rebase, since #6257 reworked Info() again: a vmiList miss still
returns (id, SCHEDULING, nil), so without the entry Info can never report the guest's
real phase. That is what keep domain bookkeeping until Cleanup still buys, and its
description is unchanged.

The description now names be7740c59 as the pinned base. #6257 has force-pushed several
times and has already moved past this snapshot; this PR pins one that builds rather than
tracking its head, so expect one more rebase when #6257 settles.