[17.0] pillar: promptly reap powered-off kvm domains

pkg/pillar/cmd/domainmgr/domainmgr.go

@@ -1101,7 +1101,12 @@ func verifyStatus(ctx *domainContext, status *types.DomainStatus) {
 	}
 
 	domainID, domainStatus, err := hyper.Task(status).Info(status.DomainName)
-	if err != nil || domainStatus == types.HALTED {
+	// HALTING here means the guest powered off on its own but the hypervisor
+	// process is still paused holding resources (qemu -no-shutdown); treat it
+	// like HALTED so we tear it down and free the resources rather than leaving
+	// it parked. (Info only reports HALTING once the guest has actually powered
+	// off, so an in-progress shutdown is not cut short.)
+	if err != nil || domainStatus == types.HALTED || domainStatus == types.HALTING {
 		if status.Activated && configActivate {
 			if err == nil {
 				err = fmt.Errorf("unexpected state %s", domainStatus.String())
@@ -2873,6 +2878,16 @@ func waitForDomainGone(status types.DomainStatus, maxDelay time.Duration) bool {
 				state.String())
 			return true
 		}
+		if state == types.HALTING {
+			// The guest has powered off but the hypervisor process is still
+			// paused holding its resources (e.g. qemu -no-shutdown). Stop
+			// waiting and let the caller reap it (Delete) now rather than
+			// polling out the graceful-shutdown budget. Not "gone" yet: the
+			// process still has to be quit and its resources released.
+			log.Noticef("waitForDomainGone(%v) for %s: guest powered off (state %s); reaping now",
+				status.UUIDandVersion, status.DisplayName, state.String())
+			return false
+		}
 		log.Functionf("waitForDomainGone(%v) for %s state still %s waited %v",
 			status.UUIDandVersion, status.DisplayName,
 			state.String(), waited)

pkg/pillar/hypervisor/kvm.go

@@ -6,6 +6,7 @@ package hypervisor
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -16,6 +17,7 @@ import (
 	"strconv"
 	"strings"
 	"sync/atomic"
+	"syscall"
 	"text/template"
 	"time"
 
@@ -1902,24 +1904,101 @@ func (ctx KvmContext) Stop(domainName string, _ bool) error {
 	return nil
 }
 
+// qemuExitGrace is how long Delete waits for the qemu process to exit after a
+// QMP `quit`, and again after a SIGKILL, before giving up.
+const qemuExitGrace = 5 * time.Second
+
+// waitProcessGone returns true once pid is no longer a live process, or false if
+// it is still alive after timeout.
+func waitProcessGone(pid int, timeout time.Duration) bool {
+	deadline := time.Now().Add(timeout)
+	warned := false
+	for {
+		// Signal 0 probes for existence: only ESRCH means the process is
+		// gone. Other errors (EPERM, EINVAL) mean the probe failed while the
+		// process may still exist, so don't report it as gone.
+		if err := syscall.Kill(pid, 0); err != nil {
+			if errors.Is(err, syscall.ESRCH) {
+				return true
+			}
+			if !warned {
+				logrus.Warnf("waitProcessGone(%d): unexpected error probing pid: %v", pid, err)
+				warned = true
+			}
+		}
+		if !time.Now().Before(deadline) {
+			return false
+		}
+		time.Sleep(100 * time.Millisecond)
+	}
+}
+
 // Delete deletes a domain
 func (ctx KvmContext) Delete(domainName string) (result error) {
-	//Sending a stop signal to then domain before quitting. This is done to freeze the domain before quitting it.
+	// Capture the qemu pid before tearing down state, so we can guarantee the
+	// process is gone even if the QMP quit below fails or hangs.
+	pid, pidErr := procutils.GetPidFromFile(kvmStateDir + domainName + "/pid")
+
+	// Issue a QMP `stop` command (not a process signal) to pause the vCPUs,
+	// freezing the guest before we quit it.
 	_, err := os.Stat(GetQmpExecutorSocket(domainName))
 	if err == nil {
 		execStop(GetQmpExecutorSocket(domainName))
 		if err = execQuit(GetQmpExecutorSocket(domainName)); err != nil {
-			return logError("failed to execute quit command %v", err)
+			logError("failed to execute quit command %v", err)
+		}
+	}
+
+	// Backstop: make sure qemu has actually exited and released its resources
+	// (memory, assigned PCI devices). quit over QMP can fail or hang if the
+	// monitor is wedged; without a hard kill the caller would keep seeing the
+	// domain as present and never free its resources (the lf-edge/eve#5916
+	// stall). Wait briefly for a clean exit, then SIGKILL.
+	if pidErr == nil {
+		if !waitProcessGone(pid, qemuExitGrace) {
+			logrus.Warnf("Delete(%s): qemu pid %d still alive after quit; sending SIGKILL",
+				domainName, pid)
+			_ = syscall.Kill(pid, syscall.SIGKILL)
+			if !waitProcessGone(pid, qemuExitGrace) {
+				logrus.Errorf("Delete(%s): qemu pid %d survived SIGKILL", domainName, pid)
+			}
 		}
 	}
-	// we may want to wait a little bit here and actually kill qemu process if it gets wedged
+
 	if err := os.RemoveAll(kvmStateDir + domainName); err != nil {
 		return logError("failed to clean up domain state directory %s (%v)", domainName, err)
 	}
 
 	return nil
 }
 
+// decideKvmState maps the containerd task state and the QMP run-state into the
+// SwState domainmgr should act on.
+//
+// qemu runs with -no-shutdown, so when the guest completes its ACPI poweroff the
+// qemu process does not exit: it pauses in QMP run-state "shutdown" (which
+// getQemuStatus maps to types.HALTING) while still holding its memory and any
+// assigned PCI devices, and the containerd task stays RUNNING. Surfacing HALTING
+// lets domainmgr reap the paused domain promptly (quit + cleanup, which frees the
+// resources and reaches HALTED) instead of polling out the graceful-shutdown
+// timers. It is deliberately not reported as HALTED here: the process is still
+// alive and its resources are not yet released.
+func decideKvmState(ctrdState, qmpState types.SwState, qmpErr error) types.SwState {
+	// The sole caller (Info) only reaches here with ctrdState == RUNNING, so
+	// this guard is unreachable in production; it is kept for defensive
+	// programming and so the helper can be unit-tested in isolation.
+	if ctrdState != types.RUNNING {
+		return ctrdState
+	}
+	if qmpErr != nil {
+		return types.BROKEN
+	}
+	if qmpState == types.HALTING {
+		return types.HALTING
+	}
+	return ctrdState
+}
+
 // Info returns information of a domain
 func (ctx KvmContext) Info(domainName string) (int, types.SwState, error) {
 	// first we ask for the task status
@@ -1928,13 +2007,14 @@ func (ctx KvmContext) Info(domainName string) (int, types.SwState, error) {
 		return effectiveDomainID, effectiveDomainState, err
 	}
 
-	_, err = getQemuStatus(GetQmpExecutorSocket(domainName))
-	if err != nil {
-		return effectiveDomainID, types.BROKEN,
-			logError("couldn't retrieve status for domain %s: %v", domainName, err)
+	qmpState, qmpErr := getQemuStatus(GetQmpExecutorSocket(domainName))
+	state := decideKvmState(effectiveDomainState, qmpState, qmpErr)
+	if qmpErr != nil {
+		return effectiveDomainID, state,
+			logError("couldn't retrieve status for domain %s: %v", domainName, qmpErr)
 	}
 
-	return effectiveDomainID, effectiveDomainState, nil
+	return effectiveDomainID, state, nil
 }
 
 // Cleanup cleans up a domain

pkg/pillar/hypervisor/kvm_test.go

@@ -3353,3 +3353,31 @@ func TestPCIAddressAllocator(t *testing.T) {
 	g.Expect(err.Error()).To(ContainSubstring("User-defined network interface order " +
 		"disrupts the function sequence of the multifunction PCI devices 0000:06:00 and 0000:08:00"))
 }
+
+func TestDecideKvmState(t *testing.T) {
+	g := NewGomegaWithT(t)
+	qmpErr := fmt.Errorf("qmp unreachable")
+	tests := []struct {
+		name     string
+		ctrd     types.SwState
+		qmp      types.SwState
+		qmpErr   error
+		expected types.SwState
+	}{
+		{"running guest", types.RUNNING, types.RUNNING, nil, types.RUNNING},
+		// guest finished ACPI poweroff; qemu paused under -no-shutdown -> reap
+		{"guest powered off, qemu paused", types.RUNNING, types.HALTING, nil, types.HALTING},
+		// other transient QMP run-states (e.g. migration) are not a poweroff
+		{"running task, qmp paused", types.RUNNING, types.PAUSED, nil, types.RUNNING},
+		{"running task, qmp unreachable", types.RUNNING, types.UNKNOWN, qmpErr, types.BROKEN},
+		// containerd already reports a non-running task: trust it, ignore QMP
+		{"task halted", types.HALTED, types.UNKNOWN, nil, types.HALTED},
+		{"task broken", types.BROKEN, types.UNKNOWN, nil, types.BROKEN},
+		{"task installed", types.INSTALLED, types.UNKNOWN, nil, types.INSTALLED},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g.Expect(decideKvmState(tc.ctrd, tc.qmp, tc.qmpErr)).To(Equal(tc.expected))
+		})
+	}
+}