fix(vz): stop the VM even when CanRequestStop is false - #5436
Conversation
| if !l.machine.CanStop() { | ||
| return errors.New("vz: the VM cannot be stopped") | ||
| } | ||
| if err := l.machine.Stop(); err != nil { |
There was a problem hiding this comment.
During initial implementation i remember doing direct stop is forceful close and may corrupt the disk. Can you confirm via the docs if its okay to use this.
There was a problem hiding this comment.
Warning
This is a destructive operation. It stops the VM without giving the guest a chance to stop cleanly.
There was a problem hiding this comment.
You're right, thanks! Fixed: It is now only reached after both RequestStop() and shutdown -h now over SSH failed or timed out.
The reason I kept it: Stop() is called from startRoutinesAndWait() on the way out, and the host agent process exits right after it, so with vz the VM is destroyed by the process exit anyway — that is exactly what happens today when Stop() returns the CanRequestStop is not supported error. Calling it explicitly only makes the teardown ordered and lets us wait for the VM to be gone. If you prefer, I can drop the forced stop and return an error instead.
`LimaVzDriver.Stop()` returned an error without stopping the VM when `CanRequestStop()` reported false, and it never removed the driver PID file. When launchd sends SIGTERM to the host agent of a LaunchDaemon instance, the host agent exits with `vz: CanRequestStop is not supported` and leaves `vz.pid` behind. Within the same boot this is harmless, because `ReadPIDFile` removes the PID file of a dead process, but after a reboot the recorded PID may have been reused by an unrelated process. The instance then shows up as `Broken` with "vz driver is running but host agent is not", and launchd with `KeepAlive` crash-loops `limactl start`. Ask the guest to shut down over SSH whenever it could not be asked through the Virtualization framework, not only for the macOS guests that ignore `RequestStop()`. Only when the guest ignores every request to shut down by itself, fall back to `VirtualMachine.Stop()`. That call is destructive, but `Stop()` is called on the way out of the host agent, and with vz the VM runs inside that process, so the VM is destroyed either way; stopping it explicitly keeps the teardown ordered and lets the driver wait for the VM to actually be gone. Always remove the PID file when the driver stops, as the qemu and krunkit drivers already do. Related to lima-vm#5087 Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
ae57244 to
6a1847b
Compare
3 participants
Problem
LimaVzDriver.Stop()returnsvz: CanRequestStop is not supportedwithout stopping the VM whenCanRequestStop()is false, and it never removes the driver PID file. The qemu and krunkit drivers do remove theirs.On an instance registered as a LaunchDaemon (
limactl autostart enable --condition=boot), launchd sends SIGTERM to the host agent,Stop()fails, andvz.pidis left behind. Within the same boot that is harmless, becausestore.ReadPIDFile()removes the PID file of a dead process, but after a reboot the recorded PID may have been reused by an unrelated process. The instance is then reported asBrokenwithvz driver is running but host agent is not, and launchd withKeepAlivecrash-loopslimactl start.This is bug 1 of #5087. It overlaps with the vz part of the draft PR #5088.
Changes
RequestStop().Stop()falls back toVirtualMachine.Stop().waitForStopped(), which also stops its ticker. The timeout is unchanged at 30 seconds.On the forced stop
VirtualMachine.Stop()is destructive, as the Apple documentation warns. It is used only afterRequestStop()andshutdown -h nowover SSH have both failed or timed out, and the alternative is not a clean shutdown:Stop()is called fromstartRoutinesAndWait()on the way out of the host agent, and the host agent process exits right afterwards. With vz the VM runs inside that process, so it is destroyed by the process exit anyway, which is exactly what happens today whenStop()returns theCanRequestStop is not supportederror.server.Stop()on the next line.Stopping the VM explicitly makes that teardown ordered and lets the driver wait for the VM to actually be gone, instead of leaving it to process termination.
If you would rather not have the forced stop at all, I can drop it and return an error instead; the PID file cleanup and the SSH fallback are what fix the reported bug.
Not in this PR
The LaunchDaemon plist does not set
ExitTimeOut, so launchd sends SIGKILL 20 seconds after SIGTERM, which can cut the graceful shutdown short. That belongs to the autostart code, so it is left for a separate change.Testing
go test ./pkg/driver/vz/...covers the two extracted helpers, including that the PID file is removed and that removing a missing one is not an error.Stop()itself needs a real VM, so it is not unit tested.make golangci-lintis clean.Assisted-by: Claude Code