diff --git a/pkg/pillar/cmd/domainmgr/domainmgr.go b/pkg/pillar/cmd/domainmgr/domainmgr.go index d798541afab..cd092b73b83 100644 --- a/pkg/pillar/cmd/domainmgr/domainmgr.go +++ b/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) diff --git a/pkg/pillar/hypervisor/kvm.go b/pkg/pillar/hypervisor/kvm.go index 95f796e4514..f803f74ec01 100644 --- a/pkg/pillar/hypervisor/kvm.go +++ b/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,17 +1904,67 @@ 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) } @@ -1920,6 +1972,33 @@ func (ctx KvmContext) Delete(domainName string) (result error) { 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 diff --git a/pkg/pillar/hypervisor/kvm_test.go b/pkg/pillar/hypervisor/kvm_test.go index 30479fcfd72..cbcf08e4276 100644 --- a/pkg/pillar/hypervisor/kvm_test.go +++ b/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)) + }) + } +}