From fce3d17d219b571b30e1d2a0a1392525b044d50e Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Fri, 21 Aug 2026 01:15:12 +0800 Subject: [PATCH 1/7] fix: make macOS VM start idempotent --- cmd/vm/handler_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++ cmd/vm/lifecycle.go | 9 +++++++ 2 files changed, 64 insertions(+) diff --git a/cmd/vm/handler_test.go b/cmd/vm/handler_test.go index 14a1dfc..35b946a 100644 --- a/cmd/vm/handler_test.go +++ b/cmd/vm/handler_test.go @@ -1,12 +1,67 @@ package vm import ( + "os" + "os/exec" + "path/filepath" "slices" "testing" "github.com/spf13/cobra" ) +func TestStartAlreadyRunningIsIdempotent(t *testing.T) { + stateDir := t.TempDir() + vmDir := filepath.Join(stateDir, "vms", "macos-demo") + if err := os.MkdirAll(vmDir, 0o755); err != nil { + t.Fatal(err) + } + + // Give isRunning a real process whose argv[0] and arguments match the + // qemu identity check without requiring qemu or KVM in the unit test. + fakeQEMU := filepath.Join(t.TempDir(), qemuBinary) + if err := os.Symlink("/bin/sh", fakeQEMU); err != nil { + t.Fatal(err) + } + disk := filepath.Join(vmDir, "disk.qcow2") + process := exec.Command(fakeQEMU, "-c", "sleep 60", disk) + if err := process.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = process.Process.Kill() + _ = process.Wait() + }) + + rec := &record{Name: "macos-demo", Disk: disk, PID: process.Process.Pid, VNCDisp: 1} + if err := saveRec(vmDir, rec); err != nil { + t.Fatal(err) + } + + cmd := &cobra.Command{} + cmd.SetContext(t.Context()) + cmd.Flags().String("state-dir", stateDir, "") + cmd.Flags().Int("vnc", -1, "") + cmd.Flags().String("vnc-password", "", "") + if err := cmd.Flags().Set("vnc", "2"); err != nil { + t.Fatal(err) + } + if err := cmd.Flags().Set("vnc-password", "newpass"); err != nil { + t.Fatal(err) + } + + if err := NewHandler().Start(cmd, []string{"macos-demo"}); err != nil { + t.Fatalf("duplicate start must adopt the live qemu: %v", err) + } + got, err := loadRec(vmDir) + if err != nil { + t.Fatal(err) + } + if got.PID != rec.PID || got.VNCDisp != 1 { + t.Fatalf("live record changed: pid=%d vnc=%d, want pid=%d vnc=1", got.PID, got.VNCDisp, rec.PID) + } +} + func TestCloneOpenCoreBase(t *testing.T) { tests := []struct { name string diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index c3745ec..60cfbe9 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -51,6 +51,15 @@ func (h *Handler) Start(cmd *cobra.Command, args []string) error { if err != nil { return err } + // A recovery request may have started waiting while another lifecycle + // operation (notably `vm export`) held the VM lock. Re-check after + // acquiring the lock: the export may already have restarted qemu and + // its VNC proxy. Launching a second qemu would fail and launch's error + // cleanup would tear down the healthy proxy belonging to the first one. + if isRunning(r) { + fmt.Printf("%s (pid %d, already running)\n", n, r.PID) + return nil + } r.VNCDisp, r.VNCPass = vnc, vncPass if err := h.launch(cmd, dir, r); err != nil { return err From d3466c5e63a859b700b85fcc861630797b10bc57 Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Fri, 21 Aug 2026 02:07:45 +0800 Subject: [PATCH 2/7] fix: repair missing macOS VNC proxy --- cmd/vm/lifecycle.go | 13 ++++++++++--- cmd/vm/vnc.go | 10 +++++++++- cmd/vm/vnc_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index 60cfbe9..b998082 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -53,10 +53,17 @@ func (h *Handler) Start(cmd *cobra.Command, args []string) error { } // A recovery request may have started waiting while another lifecycle // operation (notably `vm export`) held the VM lock. Re-check after - // acquiring the lock: the export may already have restarted qemu and - // its VNC proxy. Launching a second qemu would fail and launch's error - // cleanup would tear down the healthy proxy belonging to the first one. + // acquiring the lock: the export may already have restarted qemu. + // Launching a second qemu would fail and launch's error cleanup would + // tear down the healthy proxy belonging to the first one. If an earlier + // failed duplicate start already removed the proxy, repair only that + // host-side process while preserving the live guest. if isRunning(r) { + if r.Netns != "" && r.VNCDisp >= 0 && !vncProxyRunning(dir) { + if err := startVNCProxy(ctx, dir, r.VNCDisp); err != nil { + return fmt.Errorf("repair vnc proxy: %w", err) + } + } fmt.Printf("%s (pid %d, already running)\n", n, r.PID) return nil } diff --git a/cmd/vm/vnc.go b/cmd/vm/vnc.go index 4588242..232d0ec 100644 --- a/cmd/vm/vnc.go +++ b/cmd/vm/vnc.go @@ -94,11 +94,19 @@ func startVNCProxy(ctx context.Context, dir string, disp int) error { return nil } +func vncProxyRunning(dir string) bool { + pid, err := utils.ReadPIDFile(filepath.Join(dir, vncProxyPID)) + if err != nil { + return false + } + return utils.VerifyProcessCmdline(pid, filepath.Base(os.Args[0]), filepath.Join(dir, vncSockName)) +} + // stopVNCProxy kills a running proxy (best-effort). Zero grace: the proxy traps SIGTERM via the root NotifyContext and would keep accepting, and SIGKILL loses nothing on a stateless pipe. func stopVNCProxy(ctx context.Context, dir string) { pidPath := filepath.Join(dir, vncProxyPID) if pid, err := utils.ReadPIDFile(pidPath); err == nil { - _ = utils.TerminateProcess(ctx, pid, filepath.Base(os.Args[0]), vncProxyOp, 0) + _ = utils.TerminateProcess(ctx, pid, filepath.Base(os.Args[0]), filepath.Join(dir, vncSockName), 0) } _ = os.Remove(pidPath) } diff --git a/cmd/vm/vnc_test.go b/cmd/vm/vnc_test.go index 3e574b0..142b942 100644 --- a/cmd/vm/vnc_test.go +++ b/cmd/vm/vnc_test.go @@ -2,7 +2,12 @@ package vm import ( "errors" + "fmt" + "os" + "os/exec" + "path/filepath" "testing" + "time" ) func TestRequireCNIVNCPassword(t *testing.T) { @@ -53,3 +58,40 @@ func TestValidateVNCPassword(t *testing.T) { }) } } + +func TestVNCProxyRunning(t *testing.T) { + dir := t.TempDir() + if vncProxyRunning(dir) { + t.Fatal("missing proxy pidfile reported as running") + } + + sock := filepath.Join(dir, vncSockName) + proxy := exec.Command(os.Args[0], "-test.run=TestVNCProxyHelperProcess", "--", vncProxyOp, sock) + proxy.Env = append(os.Environ(), "COCOON_MACOS_VNC_PROXY_HELPER=1") + if err := proxy.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = proxy.Process.Kill() + _ = proxy.Wait() + }) + if err := os.WriteFile(filepath.Join(dir, vncProxyPID), []byte(fmt.Sprintf("%d\n", proxy.Process.Pid)), 0o600); err != nil { + t.Fatal(err) + } + + deadline := time.Now().Add(2 * time.Second) + for !vncProxyRunning(dir) && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if !vncProxyRunning(dir) { + t.Fatal("matching proxy process reported as stopped") + } +} + +func TestVNCProxyHelperProcess(t *testing.T) { + if os.Getenv("COCOON_MACOS_VNC_PROXY_HELPER") != "1" { + return + } + time.Sleep(time.Minute) + os.Exit(0) +} From 0f21ff9ab271193e9a74fe09043a4ad543c5de3b Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:10:39 +0800 Subject: [PATCH 3/7] refactor: simplify lifecycle recovery checks --- cmd/vm/lifecycle.go | 9 ++------- cmd/vm/vnc.go | 5 +---- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index b998082..be152ce 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -51,13 +51,8 @@ func (h *Handler) Start(cmd *cobra.Command, args []string) error { if err != nil { return err } - // A recovery request may have started waiting while another lifecycle - // operation (notably `vm export`) held the VM lock. Re-check after - // acquiring the lock: the export may already have restarted qemu. - // Launching a second qemu would fail and launch's error cleanup would - // tear down the healthy proxy belonging to the first one. If an earlier - // failed duplicate start already removed the proxy, repair only that - // host-side process while preserving the live guest. + // Another lifecycle operation may have restarted qemu while Start waited + // for the VM lock. Adopt the live guest and repair only its VNC proxy. if isRunning(r) { if r.Netns != "" && r.VNCDisp >= 0 && !vncProxyRunning(dir) { if err := startVNCProxy(ctx, dir, r.VNCDisp); err != nil { diff --git a/cmd/vm/vnc.go b/cmd/vm/vnc.go index 232d0ec..36ced90 100644 --- a/cmd/vm/vnc.go +++ b/cmd/vm/vnc.go @@ -96,10 +96,7 @@ func startVNCProxy(ctx context.Context, dir string, disp int) error { func vncProxyRunning(dir string) bool { pid, err := utils.ReadPIDFile(filepath.Join(dir, vncProxyPID)) - if err != nil { - return false - } - return utils.VerifyProcessCmdline(pid, filepath.Base(os.Args[0]), filepath.Join(dir, vncSockName)) + return err == nil && utils.VerifyProcessCmdline(pid, filepath.Base(os.Args[0]), filepath.Join(dir, vncSockName)) } // stopVNCProxy kills a running proxy (best-effort). Zero grace: the proxy traps SIGTERM via the root NotifyContext and would keep accepting, and SIGKILL loses nothing on a stateless pipe. From 1aad9a4ef4d13491166f5f0bfa54a02f6133c21e Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Sun, 23 Aug 2026 11:11:39 +0800 Subject: [PATCH 4/7] fix: address macOS start recovery review --- cmd/vm/handler_test.go | 36 +++++++++++++++++++++++++++++++++--- cmd/vm/lifecycle.go | 9 ++++++--- cmd/vm/vnc_test.go | 15 +++++++-------- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/cmd/vm/handler_test.go b/cmd/vm/handler_test.go index 35b946a..205b48f 100644 --- a/cmd/vm/handler_test.go +++ b/cmd/vm/handler_test.go @@ -1,10 +1,12 @@ package vm import ( + "io" "os" "os/exec" "path/filepath" "slices" + "strings" "testing" "github.com/spf13/cobra" @@ -17,8 +19,7 @@ func TestStartAlreadyRunningIsIdempotent(t *testing.T) { t.Fatal(err) } - // Give isRunning a real process whose argv[0] and arguments match the - // qemu identity check without requiring qemu or KVM in the unit test. + // A real process whose argv0+args satisfy isRunning without qemu/KVM. fakeQEMU := filepath.Join(t.TempDir(), qemuBinary) if err := os.Symlink("/bin/sh", fakeQEMU); err != nil { t.Fatal(err) @@ -50,9 +51,15 @@ func TestStartAlreadyRunningIsIdempotent(t *testing.T) { t.Fatal(err) } - if err := NewHandler().Start(cmd, []string{"macos-demo"}); err != nil { + output, err := captureStdout(t, func() error { + return NewHandler().Start(cmd, []string{"macos-demo"}) + }) + if err != nil { t.Fatalf("duplicate start must adopt the live qemu: %v", err) } + if !strings.Contains(output, "supplied VNC settings ignored because live QEMU cannot be retargeted") { + t.Fatalf("duplicate start output = %q, want ignored VNC settings warning", output) + } got, err := loadRec(vmDir) if err != nil { t.Fatal(err) @@ -62,6 +69,29 @@ func TestStartAlreadyRunningIsIdempotent(t *testing.T) { } } +func captureStdout(t *testing.T, fn func() error) (string, error) { + t.Helper() + reader, writer, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + original := os.Stdout + os.Stdout = writer + callErr := fn() + os.Stdout = original + if err := writer.Close(); err != nil { + t.Fatal(err) + } + output, err := io.ReadAll(reader) + if err != nil { + t.Fatal(err) + } + if err := reader.Close(); err != nil { + t.Fatal(err) + } + return string(output), callErr +} + func TestCloneOpenCoreBase(t *testing.T) { tests := []struct { name string diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index be152ce..bb7c712 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -51,15 +51,18 @@ func (h *Handler) Start(cmd *cobra.Command, args []string) error { if err != nil { return err } - // Another lifecycle operation may have restarted qemu while Start waited - // for the VM lock. Adopt the live guest and repair only its VNC proxy. + // An op that held the lock may have restarted qemu; adopt it and only repair a dead VNC proxy. if isRunning(r) { if r.Netns != "" && r.VNCDisp >= 0 && !vncProxyRunning(dir) { if err := startVNCProxy(ctx, dir, r.VNCDisp); err != nil { return fmt.Errorf("repair vnc proxy: %w", err) } } - fmt.Printf("%s (pid %d, already running)\n", n, r.PID) + if cmd.Flags().Changed("vnc") || cmd.Flags().Changed("vnc-password") { + fmt.Printf("%s (pid %d, already running; supplied VNC settings ignored because live QEMU cannot be retargeted)\n", n, r.PID) + } else { + fmt.Printf("%s (pid %d, already running)\n", n, r.PID) + } return nil } r.VNCDisp, r.VNCPass = vnc, vncPass diff --git a/cmd/vm/vnc_test.go b/cmd/vm/vnc_test.go index 142b942..1663b8f 100644 --- a/cmd/vm/vnc_test.go +++ b/cmd/vm/vnc_test.go @@ -2,12 +2,13 @@ package vm import ( "errors" - "fmt" "os" "os/exec" "path/filepath" "testing" "time" + + "github.com/cocoonstack/cocoon/utils" ) func TestRequireCNIVNCPassword(t *testing.T) { @@ -75,16 +76,14 @@ func TestVNCProxyRunning(t *testing.T) { _ = proxy.Process.Kill() _ = proxy.Wait() }) - if err := os.WriteFile(filepath.Join(dir, vncProxyPID), []byte(fmt.Sprintf("%d\n", proxy.Process.Pid)), 0o600); err != nil { + if err := utils.WritePIDFile(filepath.Join(dir, vncProxyPID), proxy.Process.Pid); err != nil { t.Fatal(err) } - deadline := time.Now().Add(2 * time.Second) - for !vncProxyRunning(dir) && time.Now().Before(deadline) { - time.Sleep(10 * time.Millisecond) - } - if !vncProxyRunning(dir) { - t.Fatal("matching proxy process reported as stopped") + if err := utils.WaitFor(t.Context(), 2*time.Second, 10*time.Millisecond, func() (bool, error) { + return vncProxyRunning(dir), nil + }); err != nil { + t.Fatalf("matching proxy process reported as stopped: %v", err) } } From ce7261fc97856c0d9d5f0ee1de8da82b7f433384 Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Sun, 23 Aug 2026 20:44:13 +0800 Subject: [PATCH 5/7] fix: serialize and clean up OpenCore NBD injection --- qemu/inject.go | 202 +++++++++++++++++++++++++++++++++++--------- qemu/inject_test.go | 64 ++++++++++++++ 2 files changed, 226 insertions(+), 40 deletions(-) diff --git a/qemu/inject.go b/qemu/inject.go index e1c9e2b..5aeafa8 100644 --- a/qemu/inject.go +++ b/qemu/inject.go @@ -13,69 +13,173 @@ import ( "github.com/projecteru2/core/log" "howett.net/plist" + "github.com/cocoonstack/cocoon/lock/flock" "github.com/cocoonstack/cocoon/utils" ) +const ( + nbdDeviceCount = 16 + nbdLeasePath = "/run/lock/cocoon-macos-nbd.lock" + nbdCleanupTimeout = 15 * time.Second + nbdCommandTimeout = 5 * time.Second +) + +type nbdConnection struct { + device string + pid int + pidFile string + ocPath string +} + // InjectConfig mounts the OpenCore qcow2 via qemu-nbd (the only way to edit a FAT partition inside a qcow2; needs root + the nbd module) and patches config.plist with a per-VM identity. func InjectConfig(ctx context.Context, ocPath string, sm *SMBIOS) error { - _ = exec.CommandContext(ctx, "modprobe", "nbd", "max_part=8").Run() - nbd, err := connectFreeNBD(ctx, ocPath) - if err != nil { - return err - } - // cleanup stays on plain exec.Command: it must still run after ctx cancellation - defer disconnectNBD(ctx, nbd, ocPath) - waitForPart(ctx, nbd) - mnt, err := os.MkdirTemp("", "oc-efi-") - if err != nil { - return fmt.Errorf("create mount dir: %w", err) - } - defer func() { _ = os.RemoveAll(mnt) }() - var mountErr error - for _, p := range []string{nbd + "p1", nbd + "p2", nbd} { - if mountErr = exec.CommandContext(ctx, "mount", p, mnt).Run(); mountErr == nil { - break + return withNBDLease(ctx, nbdLeasePath, func() (retErr error) { + _ = exec.CommandContext(ctx, "modprobe", "nbd", "max_part=8").Run() + conn, connectErr := connectFreeNBD(ctx, ocPath) + if connectErr != nil { + return connectErr } + defer func() { retErr = errors.Join(retErr, conn.disconnect()) }() + if waitErr := waitForPart(ctx, conn.device); waitErr != nil { + return waitErr + } + mnt, err := os.MkdirTemp("", "oc-efi-") + if err != nil { + return fmt.Errorf("create mount dir: %w", err) + } + mounted := false + defer func() { retErr = errors.Join(retErr, cleanupNBDMount(mnt, mounted)) }() + var mountErr error + for _, p := range []string{conn.device + "p1", conn.device + "p2", conn.device} { + if mountErr = exec.CommandContext(ctx, "mount", p, mnt).Run(); mountErr == nil { + mounted = true + break + } + } + if mountErr != nil { + return fmt.Errorf("mount OpenCore EFI partition on %s: %w", conn.device, mountErr) + } + return patchPlist(filepath.Join(mnt, "EFI", "OC", "config.plist"), sm) + }) +} + +// withNBDLease serializes the complete connect/mount/patch/unmount/disconnect +// transaction across cocoon-macos processes. A free-device check followed by +// qemu-nbd --connect is not atomic, so choosing devices concurrently is unsafe. +func withNBDLease(ctx context.Context, path string, fn func() error) error { + l := flock.New(path) + if err := l.Lock(ctx); err != nil { + return fmt.Errorf("lock qemu-nbd lease: %w", err) } - if mountErr != nil { - return fmt.Errorf("mount OpenCore EFI partition on %s: %w", nbd, mountErr) - } - defer func() { _ = exec.Command("umount", mnt).Run() }() - return patchPlist(filepath.Join(mnt, "EFI", "OC", "config.plist"), sm) + defer func() { _ = l.Unlock(context.Background()) }() + return fn() } // waitForPart blocks until the kernel's async partition scan exposes nbdXp1 for the mount. -func waitForPart(ctx context.Context, nbd string) { - _ = utils.WaitFor(ctx, 5*time.Second, 100*time.Millisecond, func() (bool, error) { +func waitForPart(ctx context.Context, nbd string) error { + if err := utils.WaitFor(ctx, 5*time.Second, 100*time.Millisecond, func() (bool, error) { if _, err := os.Stat(nbd + "p1"); err == nil { return true, nil } _ = exec.CommandContext(ctx, "partprobe", nbd).Run() return false, nil - }) + }); err != nil { + return fmt.Errorf("wait for partition on %s: %w", nbd, err) + } + return nil +} + +func cleanupNBDMount(mnt string, mounted bool) error { + if !mounted { + if err := os.Remove(mnt); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("remove mount dir %s: %w", mnt, err) + } + return nil + } + ctx, cancel := context.WithTimeout(context.Background(), nbdCleanupTimeout) + defer cancel() + if out, err := exec.CommandContext(ctx, "umount", mnt).CombinedOutput(); err != nil { + return fmt.Errorf("unmount %s (output: %s): %w", mnt, out, err) + } + if err := os.Remove(mnt); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("remove mount dir %s: %w", mnt, err) + } + return nil } -// disconnectNBD waits out qemu-nbd's asynchronous release, or the qemu launch races in and fails with "Failed to get shared write lock". -func disconnectNBD(ctx context.Context, nbd, ocPath string) { +// disconnect waits out qemu-nbd's asynchronous release. Cleanup deliberately +// uses a fresh context because the caller is commonly unwinding after timeout. +func (c *nbdConnection) disconnect() error { logger := log.WithFunc("qemu.disconnectNBD") - _ = exec.Command("qemu-nbd", "--disconnect", nbd).Run() - if err := utils.WaitFor(ctx, 10*time.Second, 100*time.Millisecond, func() (bool, error) { - return !isFileHeld(ocPath), nil + var commandErr error + disconnectCtx, disconnectCancel := context.WithTimeout(context.Background(), nbdCommandTimeout) + if out, err := exec.CommandContext(disconnectCtx, "qemu-nbd", "--disconnect", c.device).CombinedOutput(); err != nil { + logger.Warnf(disconnectCtx, "disconnect %s (output: %s): %v", c.device, out, err) + commandErr = fmt.Errorf("disconnect %s (output: %s): %w", c.device, out, err) + } + disconnectCancel() + waitCtx, waitCancel := context.WithTimeout(context.Background(), nbdCleanupTimeout) + defer waitCancel() + if err := utils.WaitFor(waitCtx, 10*time.Second, 100*time.Millisecond, func() (bool, error) { + held, scanErr := isFileHeld(c.ocPath) + return !held, scanErr }); err != nil { - logger.Warnf(ctx, "qcow2 %s still held after nbd disconnect: %v", ocPath, err) + logger.Warnf(waitCtx, "qcow2 %s still held after nbd disconnect: %v", c.ocPath, err) + killCtx, killCancel := context.WithTimeout(context.Background(), nbdCleanupTimeout) + defer killCancel() + var cleanupErrs []error + if cleanupErr := utils.TerminateProcess(killCtx, c.pid, "qemu-nbd", c.ocPath, time.Second); cleanupErr != nil { + logger.Warnf(killCtx, "terminate qemu-nbd pid %d for %s: %v", c.pid, c.ocPath, cleanupErr) + cleanupErrs = append(cleanupErrs, cleanupErr) + } + if cleanupErr := CleanupNBDForPath(killCtx, c.ocPath); cleanupErr != nil { + logger.Warnf(killCtx, "terminate stale qemu-nbd for %s: %v", c.ocPath, cleanupErr) + cleanupErrs = append(cleanupErrs, cleanupErr) + } + commandErr = errors.Join(commandErr, err, errors.Join(cleanupErrs...)) + } + _ = os.Remove(c.pidFile) + held, err := isFileHeld(c.ocPath) + if err != nil { + return errors.Join(commandErr, err) } + if !held { + return nil + } + return commandErr } // isFileHeld matches the daemonized qemu-nbd server's cmdline — cheaper than scanning fd tables, and that server is the only holder to wait out. -func isFileHeld(ocPath string) bool { +func isFileHeld(ocPath string) (bool, error) { pids, err := utils.FindVMMByCmdline("qemu-nbd", ocPath) - return err == nil && len(pids) > 0 + return len(pids) > 0, err +} + +// CleanupNBDForPath terminates qemu-nbd processes whose command line references +// path. PID identity is verified before signaling, so an unrelated process is +// never killed even if a PID was reused. +func CleanupNBDForPath(ctx context.Context, path string) error { + pids, err := utils.FindVMMByCmdline("qemu-nbd", path) + if err != nil { + return fmt.Errorf("scan qemu-nbd processes for %s: %w", path, err) + } + var errs []error + for _, pid := range pids { + if err := utils.TerminateProcess(ctx, pid, "qemu-nbd", path, time.Second); err != nil { + errs = append(errs, fmt.Errorf("terminate qemu-nbd pid %d: %w", pid, err)) + } + } + return errors.Join(errs...) } -// connectFreeNBD claims a device by connecting: the connect itself is the exclusive operation, so a race with another VM create just advances to the next candidate. -func connectFreeNBD(ctx context.Context, ocPath string) (string, error) { +// connectFreeNBD is called while holding the host-wide NBD lease. --fork is +// required: without it qemu-nbd remains in the foreground for the lifetime of +// the mapping and the invoking CLI never reaches the mount/patch steps. +func connectFreeNBD(ctx context.Context, ocPath string) (*nbdConnection, error) { var lastErr error - for i := range 16 { + pidFile := ocPath + ".nbd.pid" + _ = os.Remove(pidFile) + for i := range nbdDeviceCount { nbd := fmt.Sprintf("/dev/nbd%d", i) if _, err := os.Stat(nbd); err != nil { continue @@ -83,16 +187,34 @@ func connectFreeNBD(ctx context.Context, ocPath string) (string, error) { if _, err := os.Stat(fmt.Sprintf("/sys/block/nbd%d/pid", i)); !os.IsNotExist(err) { continue } - out, cerr := exec.CommandContext(ctx, "qemu-nbd", "--connect="+nbd, "-f", "qcow2", ocPath).CombinedOutput() + out, cerr := exec.CommandContext(ctx, "qemu-nbd", qemuNBDConnectArgs(nbd, pidFile, ocPath)...).CombinedOutput() if cerr == nil { - return nbd, nil + pid, err := utils.ReadPIDFile(pidFile) + if err == nil { + return &nbdConnection{device: nbd, pid: pid, pidFile: pidFile, ocPath: ocPath}, nil + } + cleanupCtx, cancel := context.WithTimeout(context.Background(), nbdCleanupTimeout) + _ = exec.CommandContext(cleanupCtx, "qemu-nbd", "--disconnect", nbd).Run() + _ = CleanupNBDForPath(cleanupCtx, ocPath) + cancel() + return nil, fmt.Errorf("read qemu-nbd pid file %s: %w", pidFile, err) } lastErr = fmt.Errorf("connect qemu-nbd %s (output: %s): %w", nbd, out, cerr) + cleanupCtx, cancel := context.WithTimeout(context.Background(), nbdCleanupTimeout) + _ = CleanupNBDForPath(cleanupCtx, ocPath) + cancel() + if ctx.Err() != nil { + return nil, errors.Join(lastErr, ctx.Err()) + } } if lastErr != nil { - return "", lastErr + return nil, lastErr } - return "", errors.New("no free /dev/nbd device (is the nbd module loaded)") + return nil, errors.New("no free /dev/nbd device (is the nbd module loaded)") +} + +func qemuNBDConnectArgs(nbd, pidFile, ocPath string) []string { + return []string{"--fork", "--pid-file=" + pidFile, "--connect=" + nbd, "-f", "qcow2", ocPath} } func patchPlist(path string, sm *SMBIOS) error { diff --git a/qemu/inject_test.go b/qemu/inject_test.go index 91054b5..eff6b74 100644 --- a/qemu/inject_test.go +++ b/qemu/inject_test.go @@ -1,14 +1,78 @@ package qemu import ( + "context" "encoding/hex" "os" "path/filepath" + "slices" + "sync" + "sync/atomic" "testing" + "time" "howett.net/plist" ) +func TestQemuNBDConnectArgsForkAndTrackServer(t *testing.T) { + want := []string{"--fork", "--pid-file=/state/vm/OpenCore.qcow2.nbd.pid", "--connect=/dev/nbd3", "-f", "qcow2", "/state/vm/OpenCore.qcow2"} + if got := qemuNBDConnectArgs("/dev/nbd3", "/state/vm/OpenCore.qcow2.nbd.pid", "/state/vm/OpenCore.qcow2"); !slices.Equal(got, want) { + t.Fatalf("qemu-nbd args = %v, want %v", got, want) + } +} + +func TestWithNBDLeaseSerializesConcurrentInjectors(t *testing.T) { + const workers = 50 + lockPath := filepath.Join(t.TempDir(), "nbd.lock") + var active, maxActive atomic.Int32 + errCh := make(chan error, workers) + var wg sync.WaitGroup + for range workers { + wg.Add(1) + go func() { + defer wg.Done() + errCh <- withNBDLease(t.Context(), lockPath, func() error { + n := active.Add(1) + defer active.Add(-1) + for old := maxActive.Load(); n > old && !maxActive.CompareAndSwap(old, n); old = maxActive.Load() { + } + time.Sleep(time.Millisecond) + return nil + }) + }() + } + wg.Wait() + close(errCh) + for err := range errCh { + if err != nil { + t.Fatal(err) + } + } + if got := maxActive.Load(); got != 1 { + t.Fatalf("maximum concurrent NBD transactions = %d, want 1", got) + } +} + +func TestWithNBDLeaseHonorsCancellation(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "nbd.lock") + held := make(chan struct{}) + release := make(chan struct{}) + go func() { + _ = withNBDLease(t.Context(), lockPath, func() error { + close(held) + <-release + return nil + }) + }() + <-held + ctx, cancel := context.WithTimeout(t.Context(), 20*time.Millisecond) + defer cancel() + if err := withNBDLease(ctx, lockPath, func() error { return nil }); err == nil { + t.Fatal("waiting NBD lease ignored context cancellation") + } + close(release) +} + // sampleConfig mirrors OSX-KVM's config.plist so patchPlist round-trips a realistic input. const sampleConfig = ` From 936d09b4f082c039f04a0ae27a55dbcc6a4f1d06 Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:01:02 +0800 Subject: [PATCH 6/7] fix: avoid reusing wedged NBD devices --- qemu/inject.go | 54 ++++++++++++++++++++++++++++++++++++++++++++- qemu/inject_test.go | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/qemu/inject.go b/qemu/inject.go index 5aeafa8..2b61b42 100644 --- a/qemu/inject.go +++ b/qemu/inject.go @@ -8,6 +8,8 @@ import ( "os" "os/exec" "path/filepath" + "strconv" + "strings" "time" "github.com/projecteru2/core/log" @@ -81,7 +83,6 @@ func waitForPart(ctx context.Context, nbd string) error { if _, err := os.Stat(nbd + "p1"); err == nil { return true, nil } - _ = exec.CommandContext(ctx, "partprobe", nbd).Run() return false, nil }); err != nil { return fmt.Errorf("wait for partition on %s: %w", nbd, err) @@ -187,6 +188,13 @@ func connectFreeNBD(ctx context.Context, ocPath string) (*nbdConnection, error) if _, err := os.Stat(fmt.Sprintf("/sys/block/nbd%d/pid", i)); !os.IsNotExist(err) { continue } + referenced, err := processReferencesBlockDevice("/proc", nbd) + if err != nil { + return nil, fmt.Errorf("check whether %s is referenced by a process: %w", nbd, err) + } + if referenced { + continue + } out, cerr := exec.CommandContext(ctx, "qemu-nbd", qemuNBDConnectArgs(nbd, pidFile, ocPath)...).CombinedOutput() if cerr == nil { pid, err := utils.ReadPIDFile(pidFile) @@ -213,6 +221,50 @@ func connectFreeNBD(ctx context.Context, ocPath string) (*nbdConnection, error) return nil, errors.New("no free /dev/nbd device (is the nbd module loaded)") } +// processReferencesBlockDevice catches userspace operations that are still +// blocked on an NBD device after the kernel has already removed its sysfs pid. +// Reusing such a device can block the next qemu-nbd attach indefinitely. +func processReferencesBlockDevice(procRoot, device string) (bool, error) { + entries, err := os.ReadDir(procRoot) + if err != nil { + return false, fmt.Errorf("read %s: %w", procRoot, err) + } + for _, entry := range entries { + if !entry.IsDir() { + continue + } + if _, err := strconv.Atoi(entry.Name()); err != nil { + continue + } + cmdlinePath := filepath.Join(procRoot, entry.Name(), "cmdline") + cmdline, err := os.ReadFile(cmdlinePath) + if err != nil { + if os.IsNotExist(err) { + continue + } + return false, fmt.Errorf("read %s: %w", cmdlinePath, err) + } + for arg := range strings.SplitSeq(string(cmdline), "\x00") { + if nbdArgReferencesDevice(arg, device) { + return true, nil + } + } + } + return false, nil +} + +func nbdArgReferencesDevice(arg, device string) bool { + if arg == device || arg == "--connect="+device { + return true + } + partition := strings.TrimPrefix(arg, device+"p") + if partition == arg || partition == "" { + return false + } + _, err := strconv.Atoi(partition) + return err == nil +} + func qemuNBDConnectArgs(nbd, pidFile, ocPath string) []string { return []string{"--fork", "--pid-file=" + pidFile, "--connect=" + nbd, "-f", "qcow2", ocPath} } diff --git a/qemu/inject_test.go b/qemu/inject_test.go index eff6b74..d83c8bb 100644 --- a/qemu/inject_test.go +++ b/qemu/inject_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "slices" + "strings" "sync" "sync/atomic" "testing" @@ -73,6 +74,44 @@ func TestWithNBDLeaseHonorsCancellation(t *testing.T) { close(release) } +func TestProcessReferencesBlockDevice(t *testing.T) { + procRoot := t.TempDir() + writeCmdline := func(pid, command string, args ...string) { + t.Helper() + dir := filepath.Join(procRoot, pid) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + argv := append([]string{command}, args...) + data := append([]byte(strings.Join(argv, "\x00")), 0) + if err := os.WriteFile(filepath.Join(dir, "cmdline"), data, 0o600); err != nil { + t.Fatal(err) + } + } + + writeCmdline("101", "partprobe", "/dev/nbd0") + writeCmdline("102", "mount", "/dev/nbd1p1", "/mnt") + writeCmdline("103", "qemu-nbd", "--connect=/dev/nbd2", "disk.qcow2") + writeCmdline("104", "helper", "/dev/nbd01") + + for _, device := range []string{"/dev/nbd0", "/dev/nbd1", "/dev/nbd2"} { + busy, err := processReferencesBlockDevice(procRoot, device) + if err != nil { + t.Fatalf("processReferencesBlockDevice(%s): %v", device, err) + } + if !busy { + t.Errorf("processReferencesBlockDevice(%s) = false, want true", device) + } + } + busy, err := processReferencesBlockDevice(procRoot, "/dev/nbd3") + if err != nil { + t.Fatal(err) + } + if busy { + t.Error("unreferenced /dev/nbd3 reported busy") + } +} + // sampleConfig mirrors OSX-KVM's config.plist so patchPlist round-trips a realistic input. const sampleConfig = ` From c4d89af1c825ba2711bacb4956ccb3c9bf4af5d1 Mon Sep 17 00:00:00 2001 From: czmDeRepository <56431414+czmDeRepository@users.noreply.github.com> Date: Sun, 23 Aug 2026 20:44:13 +0800 Subject: [PATCH 7/7] fix: make macOS VM creation transactional --- cmd/vm/clone.go | 22 +++++-- cmd/vm/handler_test.go | 45 +++++++++++++- cmd/vm/lifecycle.go | 132 +++++++++++++++++++++++++++++++---------- cmd/vm/net_linux.go | 5 +- cmd/vm/net_other.go | 2 + cmd/vm/utils.go | 90 ++++++++++++++++++++++++++-- cmd/vm/utils_test.go | 72 ++++++++++++++++++++++ 7 files changed, 324 insertions(+), 44 deletions(-) diff --git a/cmd/vm/clone.go b/cmd/vm/clone.go index 1bccae6..d1adba6 100644 --- a/cmd/vm/clone.go +++ b/cmd/vm/clone.go @@ -1,6 +1,7 @@ package vm import ( + "errors" "fmt" "path/filepath" "time" @@ -18,14 +19,17 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error { if err != nil { return err } - name, _ := cmd.Flags().GetString("name") - if name == "" { - name = src + "-clone-" + time.Now().Format("150405") - } + name := requestedVMName(cmd, src+"-clone-"+time.Now().Format("150405")) + return withVMLock(cliutil.CommandContext(cmd), home.VMDir(cmd, name), func() error { + return h.clone(cmd, srcRec, name) + }) +} + +func (h *Handler) clone(cmd *cobra.Command, srcRec *record, name string) (retErr error) { netMode, _ := cmd.Flags().GetString("net") vnc, _ := cmd.Flags().GetInt("vnc") vncPass, _ := cmd.Flags().GetString("vnc-password") - if err = requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil { + if err := requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil { return err } // SRC's disk names are reserved: extra --data-disk specs must not collide and the combined count still honors the AHCI cap @@ -42,6 +46,12 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error { if err != nil { return err } + var r *record + defer func() { + if retErr != nil { + retErr = errors.Join(retErr, cleanupFailedVM(cmd, dir, r)) + } + }() ctx := cliutil.CommandContext(cmd) copied, err := copyDataDisks(dir, srcRec.DataDisks) if err != nil { @@ -51,7 +61,7 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error { if err != nil { return err } - r := &record{ + r = &record{ Name: name, Image: srcRec.Image, ImageDigest: digest, Disk: overlay, OVMFCode: srcRec.OVMFCode, OVMFVars: ovmfVars, CPUs: srcRec.CPUs, Memory: srcRec.Memory, DataDisks: append(copied, newDisks...), diff --git a/cmd/vm/handler_test.go b/cmd/vm/handler_test.go index 205b48f..44b1938 100644 --- a/cmd/vm/handler_test.go +++ b/cmd/vm/handler_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "slices" "strings" "testing" @@ -25,7 +26,7 @@ func TestStartAlreadyRunningIsIdempotent(t *testing.T) { t.Fatal(err) } disk := filepath.Join(vmDir, "disk.qcow2") - process := exec.Command(fakeQEMU, "-c", "sleep 60", disk) + process := exec.Command(fakeQEMU, "-c", "while :; do sleep 1; done", disk) if err := process.Start(); err != nil { t.Fatal(err) } @@ -92,6 +93,48 @@ func captureStdout(t *testing.T, fn func() error) (string, error) { return string(output), callErr } +func TestStartAdoptsQEMUWhenRecordPIDWasNotCommitted(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("process cmdline adoption requires /proc") + } + stateDir := t.TempDir() + vmDir := filepath.Join(stateDir, "vms", "macos-demo") + if err := os.MkdirAll(vmDir, 0o755); err != nil { + t.Fatal(err) + } + fakeQEMU := filepath.Join(t.TempDir(), qemuBinary) + if err := os.Symlink("/bin/sh", fakeQEMU); err != nil { + t.Fatal(err) + } + disk := filepath.Join(vmDir, "disk.qcow2") + process := exec.Command(fakeQEMU, "-c", "while :; do sleep 1; done", disk) + if err := process.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = process.Process.Kill() + _ = process.Wait() + }) + if err := saveRec(vmDir, &record{Name: "macos-demo", Disk: disk, VNCDisp: -1}); err != nil { + t.Fatal(err) + } + cmd := &cobra.Command{} + cmd.SetContext(t.Context()) + cmd.Flags().String("state-dir", stateDir, "") + cmd.Flags().Int("vnc", -1, "") + cmd.Flags().String("vnc-password", "", "") + if err := NewHandler().Start(cmd, []string{"macos-demo"}); err != nil { + t.Fatalf("Start must adopt the already-running QEMU: %v", err) + } + got, err := loadRec(vmDir) + if err != nil { + t.Fatal(err) + } + if got.PID != process.Process.Pid { + t.Fatalf("adopted PID = %d, want %d", got.PID, process.Process.Pid) + } +} + func TestCloneOpenCoreBase(t *testing.T) { tests := []struct { name string diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index bb7c712..56de95d 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -2,6 +2,7 @@ package vm import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -17,27 +18,39 @@ import ( ) func (h *Handler) Create(cmd *cobra.Command, args []string) error { - r, err := h.create(cmd, args[0]) - if err != nil { - return err - } - fmt.Println(r.Name) - return nil + name := requestedVMName(cmd, "macos-"+time.Now().Format("20060102-150405")) + return withVMLock(cliutil.CommandContext(cmd), home.VMDir(cmd, name), func() error { + r, err := h.create(cmd, args[0], name) + if err != nil { + return err + } + fmt.Println(r.Name) + return nil + }) } func (h *Handler) Run(cmd *cobra.Command, args []string) error { - r, err := h.create(cmd, args[0]) - if err != nil { - return err - } - if err := h.launch(cmd, home.VMDir(cmd, r.Name), r); err != nil { - // atomic create+boot: remove everything on failure or the leftover record bricks retries; start must NOT do this — its network is persisted - teardownNet(cmd, r) - _ = os.RemoveAll(home.VMDir(cmd, r.Name)) - return err + name := requestedVMName(cmd, "macos-"+time.Now().Format("20060102-150405")) + dir := home.VMDir(cmd, name) + return withVMLock(cliutil.CommandContext(cmd), dir, func() error { + r, err := h.create(cmd, args[0], name) + if err != nil { + return err + } + if err := h.launch(cmd, dir, r); err != nil { + return errors.Join(err, cleanupFailedVM(cmd, dir, r)) + } + fmt.Printf("%s (pid %d)\n", r.Name, r.PID) + return nil + }) +} + +func requestedVMName(cmd *cobra.Command, fallback string) string { + name, _ := cmd.Flags().GetString("name") + if name != "" { + return name } - fmt.Printf("%s (pid %d)\n", r.Name, r.PID) - return nil + return fallback } func (h *Handler) Start(cmd *cobra.Command, args []string) error { @@ -51,8 +64,23 @@ func (h *Handler) Start(cmd *cobra.Command, args []string) error { if err != nil { return err } - // An op that held the lock may have restarted qemu; adopt it and only repair a dead VNC proxy. - if isRunning(r) { + // Another lifecycle operation may have restarted qemu while Start waited, + // or the previous CLI may have died after daemonizing QEMU but before + // persisting its PID. Adopt that one process instead of launching a second. + running := isRunning(r) + if !running { + var adoptErr error + running, adoptErr = adoptRunningQEMU(r) + if adoptErr != nil { + return adoptErr + } + if running { + if err := saveRec(dir, r); err != nil { + return err + } + } + } + if running { if r.Netns != "" && r.VNCDisp >= 0 && !vncProxyRunning(dir) { if err := startVNCProxy(ctx, dir, r.VNCDisp); err != nil { return fmt.Errorf("repair vnc proxy: %w", err) @@ -107,16 +135,26 @@ func (h *Handler) RM(cmd *cobra.Command, args []string) error { ctx := cliutil.CommandContext(cmd) for _, n := range args { dir := home.VMDir(cmd, n) - if _, err := os.Stat(dir); os.IsNotExist(err) { - fmt.Println(n) // nothing to remove (and no dir to hold the flock in) - continue - } - // the flock stops a concurrent start relaunching qemu between terminate and RemoveAll if err := withVMLock(ctx, dir, func() error { + if _, err := os.Stat(dir); os.IsNotExist(err) { + return nil + } else if err != nil { + return fmt.Errorf("stat vm dir: %w", err) + } + // the flock stops a concurrent create/start from changing state between terminate and RemoveAll if r, err := loadRec(dir); err == nil { terminate(ctx, r, grace) stopVNCProxy(ctx, dir) teardownNet(cmd, r) + } else { + cleanupCtx, cancel := context.WithTimeout(context.Background(), vmCleanupTimeout) + defer cancel() + if cleanupErr := cleanupQEMUForPath(cleanupCtx, dir); cleanupErr != nil { + return cleanupErr + } + if cleanupErr := qemu.CleanupNBDForPath(cleanupCtx, dir); cleanupErr != nil { + return cleanupErr + } } if err := os.RemoveAll(dir); err != nil { return fmt.Errorf("remove vm dir: %w", err) @@ -130,11 +168,7 @@ func (h *Handler) RM(cmd *cobra.Command, args []string) error { return nil } -func (h *Handler) create(cmd *cobra.Command, image string) (*record, error) { - name, _ := cmd.Flags().GetString("name") - if name == "" { - name = "macos-" + time.Now().Format("20060102-150405") - } +func (h *Handler) create(cmd *cobra.Command, image, name string) (r *record, retErr error) { rawDisks, _ := cmd.Flags().GetStringArray("data-disk") diskSpecs, err := parseDataDisks(rawDisks, nil) // fail fast before any scaffolding if err != nil { @@ -154,13 +188,18 @@ func (h *Handler) create(cmd *cobra.Command, image string) (*record, error) { if err != nil { return nil, err } + defer func() { + if retErr != nil { + retErr = errors.Join(retErr, cleanupFailedVM(cmd, dir, r)) + } + }() ctx := cliutil.CommandContext(cmd) cpus, _ := cmd.Flags().GetInt("cpus") mem, _ := cmd.Flags().GetString("memory") ssh, _ := cmd.Flags().GetInt("ssh-port") tap, _ := cmd.Flags().GetString("tap") huge, _ := cmd.Flags().GetBool("hugepages") - r := &record{ + r = &record{ Name: name, Image: image, ImageDigest: digest, Disk: overlay, OVMFCode: code, OVMFVars: ovmfVars, CPUs: cpus, Memory: mem, VNCDisp: vnc, SSHPort: ssh, VNCPass: vncPass, NetMode: netMode, Tap: tap, Hugepages: huge, VMID: utils.GenerateID(), Created: time.Now().Format(time.RFC3339), @@ -179,6 +218,30 @@ func (h *Handler) create(cmd *cobra.Command, image string) (*record, error) { return r, saveRec(dir, r) } +// cleanupFailedVM makes create/run transactional. It uses an uncanceled, +// bounded context so SIGTERM-driven command cancellation still reaps helpers, +// networking and any QEMU process started before the record was committed. +func cleanupFailedVM(cmd *cobra.Command, dir string, r *record) error { + ctx, cancel := context.WithTimeout(context.Background(), vmCleanupTimeout) + defer cancel() + var errs []error + if r != nil { + terminate(ctx, r, 0) + stopVNCProxy(ctx, dir) + teardownNetContext(ctx, cmd, r) + } + if err := cleanupQEMUForPath(ctx, dir); err != nil { + errs = append(errs, err) + } + if err := qemu.CleanupNBDForPath(ctx, dir); err != nil { + errs = append(errs, err) + } + if err := os.RemoveAll(dir); err != nil { + errs = append(errs, fmt.Errorf("remove failed vm dir: %w", err)) + } + return errors.Join(errs...) +} + func (h *Handler) launch(cmd *cobra.Command, dir string, r *record) error { ctx := cliutil.CommandContext(cmd) logger := log.WithFunc("cmd.vm.launch") @@ -216,8 +279,13 @@ func (h *Handler) launch(cmd *cobra.Command, dir string, r *record) error { stopVNCProxy(ctx, dir) return fmt.Errorf("launch qemu: %w", err) } - if pid, err := utils.ReadPIDFile(pidfile); err == nil { - r.PID = pid + pid, err := utils.ReadPIDFile(pidfile) + if err != nil { + return fmt.Errorf("read qemu pid file: %w", err) + } + r.PID = pid + if !isRunning(r) { + return fmt.Errorf("qemu pid %d is not running for disk %s", r.PID, r.Disk) } if r.VNCPass != "" { if err := setVNCPassword(ctx, spec.MonSock, r.VNCPass); err != nil { diff --git a/cmd/vm/net_linux.go b/cmd/vm/net_linux.go index e51141b..6b6f5ee 100644 --- a/cmd/vm/net_linux.go +++ b/cmd/vm/net_linux.go @@ -89,10 +89,13 @@ func provisionNet(cmd *cobra.Command, r *record) (tap, netns, mac string, err er // teardownNet removes an auto-created TAP/netns. Best-effort; never touches a user-supplied --tap. func teardownNet(cmd *cobra.Command, r *record) { + teardownNetContext(cliutil.CommandContext(cmd), cmd, r) +} + +func teardownNetContext(ctx context.Context, cmd *cobra.Command, r *record) { if !r.TapOwned { return } - ctx := cliutil.CommandContext(cmd) logger := log.WithFunc("cmd.vm.teardownNet") // warn instead of failing: rm must proceed, but a leaked TAP/netns should leave a trail if provider, err := newProvider(cmd, r); err != nil { diff --git a/cmd/vm/net_other.go b/cmd/vm/net_other.go index d3982aa..618070e 100644 --- a/cmd/vm/net_other.go +++ b/cmd/vm/net_other.go @@ -17,6 +17,8 @@ func provisionNet(_ *cobra.Command, _ *record) (tap, netns, mac string, err erro func teardownNet(_ *cobra.Command, _ *record) {} +func teardownNetContext(_ context.Context, _ *cobra.Command, _ *record) {} + func quiesceNet(_ *cobra.Command, _ *record) {} func unquiesceNet(_ *cobra.Command, _ *record) {} diff --git a/cmd/vm/utils.go b/cmd/vm/utils.go index a363fa2..1cc3de0 100644 --- a/cmd/vm/utils.go +++ b/cmd/vm/utils.go @@ -14,6 +14,7 @@ import ( "github.com/spf13/cobra" "github.com/cocoonstack/cocoon-macos/home" + "github.com/cocoonstack/cocoon-macos/qemu" "github.com/cocoonstack/cocoon/cmd/cliutil" "github.com/cocoonstack/cocoon/images" "github.com/cocoonstack/cocoon/lock/flock" @@ -21,6 +22,8 @@ import ( "github.com/cocoonstack/cocoon/utils" ) +const vmCleanupTimeout = 30 * time.Second + func loadRec(dir string) (*record, error) { var r record if err := utils.ReadJSONFile(filepath.Join(dir, "vm.json"), &r); err != nil { @@ -37,13 +40,19 @@ func saveRec(dir string, r *record) error { return nil } -// withVMLock serializes concurrent lifecycle ops on one VM (vm.json is read-modify-write). +// withVMLock serializes concurrent lifecycle ops on one VM. The lock lives +// outside the VM directory so rm/retry cannot unlink the inode while another +// process is waiting on it and accidentally split mutual exclusion. func withVMLock(ctx context.Context, dir string, fn func() error) error { - l := flock.New(filepath.Join(dir, "vm.json.lock")) + lockPath := filepath.Join(filepath.Dir(dir), ".locks", filepath.Base(dir)+".lock") + if err := utils.EnsureDirs(filepath.Dir(lockPath)); err != nil { + return fmt.Errorf("create vm lock dir: %w", err) + } + l := flock.NewTransient(lockPath) if err := l.Lock(ctx); err != nil { return fmt.Errorf("lock vm: %w", err) } - defer func() { _ = l.Unlock(ctx) }() + defer func() { _ = l.Unlock(context.Background()) }() return fn() } @@ -60,14 +69,26 @@ func scaffoldVM(cmd *cobra.Command, name, image, varsSrc, varsName string) (dir, dir = home.VMDir(cmd, name) if _, statErr := os.Stat(filepath.Join(dir, "vm.json")); statErr == nil { return "", "", "", "", fmt.Errorf("vm %q already exists; rm it first or pick another --name", name) + } else if !os.IsNotExist(statErr) { + return "", "", "", "", fmt.Errorf("stat vm record: %w", statErr) } - if err = utils.EnsureDirs(dir); err != nil { + cleanupCtx, cancel := context.WithTimeout(context.Background(), vmCleanupTimeout) + defer cancel() + if err = resetIncompleteVMDir(cleanupCtx, dir); err != nil { return "", "", "", "", err } base, digest, err := resolveBase(cmd, image, name) if err != nil { return "", "", "", "", err } + if err = utils.EnsureDirs(dir); err != nil { + return "", "", "", "", err + } + defer func() { + if err != nil { + _ = os.RemoveAll(dir) + } + }() overlay = filepath.Join(dir, "disk.qcow2") if err = bakeOverlay(cliutil.CommandContext(cmd), base, overlay); err != nil { return "", "", "", "", err @@ -79,6 +100,48 @@ func scaffoldVM(cmd *cobra.Command, name, image, varsSrc, varsName string) (dir, return dir, overlay, ovmfVars, digest, nil } +// resetIncompleteVMDir removes state left before vm.json was committed. It +// refuses to touch a directory referenced by a live QEMU guest, and reaps any +// stale qemu-nbd helpers before removing their qcow2 paths. +func resetIncompleteVMDir(ctx context.Context, dir string) error { + if _, err := os.Stat(dir); os.IsNotExist(err) { + return nil + } else if err != nil { + return fmt.Errorf("stat incomplete vm dir: %w", err) + } + if _, err := os.Stat(filepath.Join(dir, "vm.json")); err == nil { + return fmt.Errorf("refuse to remove committed vm dir %s", dir) + } else if !os.IsNotExist(err) { + return fmt.Errorf("stat vm record: %w", err) + } + if pids, err := utils.FindVMMByCmdline(qemuBinary, dir); err != nil { + return fmt.Errorf("scan qemu processes for %s: %w", dir, err) + } else if len(pids) > 0 { + return fmt.Errorf("refuse to replace incomplete vm dir %s: live qemu pids %v", dir, pids) + } + if err := qemu.CleanupNBDForPath(ctx, dir); err != nil { + return fmt.Errorf("cleanup stale qemu-nbd for %s: %w", dir, err) + } + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("remove incomplete vm dir %s: %w", dir, err) + } + return nil +} + +func cleanupQEMUForPath(ctx context.Context, path string) error { + pids, err := utils.FindVMMByCmdline(qemuBinary, path) + if err != nil { + return fmt.Errorf("scan qemu processes for %s: %w", path, err) + } + var errs []error + for _, pid := range pids { + if err := utils.TerminateProcess(ctx, pid, qemuBinary, path, 0); err != nil { + errs = append(errs, fmt.Errorf("terminate qemu pid %d: %w", pid, err)) + } + } + return errors.Join(errs...) +} + // prepareNet returns the TAP ifname, netns path (CNI only), and guest MAC; user-mode and a pre-created --tap need no provisioning, every other mode goes through the per-OS provisionNet. func prepareNet(cmd *cobra.Command, r *record) (tap, netns, mac string, err error) { switch r.NetMode { @@ -111,6 +174,25 @@ func isRunning(r *record) bool { return utils.VerifyProcessCmdline(r.PID, qemuBinary, r.Disk) } +// adoptRunningQEMU repairs a record whose launch was interrupted after QEMU +// daemonized but before its PID was saved. More than one match is treated as +// corruption instead of guessing which process owns the disk. +func adoptRunningQEMU(r *record) (bool, error) { + pids, err := utils.FindVMMByCmdline(qemuBinary, r.Disk) + if err != nil { + return false, fmt.Errorf("scan qemu process for %s: %w", r.Disk, err) + } + switch len(pids) { + case 0: + return false, nil + case 1: + r.PID = pids[0] + return true, nil + default: + return false, fmt.Errorf("multiple qemu processes use disk %s: %v", r.Disk, pids) + } +} + // terminate stops the VM's qemu, verifying the cmdline before signaling; grace=0 means immediate SIGKILL. func terminate(ctx context.Context, r *record, grace time.Duration) { if r.PID > 0 { diff --git a/cmd/vm/utils_test.go b/cmd/vm/utils_test.go index 311f2e5..965c62b 100644 --- a/cmd/vm/utils_test.go +++ b/cmd/vm/utils_test.go @@ -1,12 +1,84 @@ package vm import ( + "os" + "path/filepath" + "sync" "testing" "time" "github.com/spf13/cobra" ) +func TestWithVMLockSurvivesVMDirRemoval(t *testing.T) { + dir := filepath.Join(t.TempDir(), "vms", "demo") + acquired := make(chan struct{}) + release := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + if err := withVMLock(t.Context(), dir, func() error { + close(acquired) + <-release + return nil + }); err != nil { + t.Errorf("first lock: %v", err) + } + }() + <-acquired + if err := os.RemoveAll(dir); err != nil { + t.Fatal(err) + } + second := make(chan struct{}) + wg.Add(1) + go func() { + defer wg.Done() + if err := withVMLock(t.Context(), dir, func() error { + close(second) + return nil + }); err != nil { + t.Errorf("second lock: %v", err) + } + }() + select { + case <-second: + t.Fatal("second operation acquired while first still held the VM lock") + case <-time.After(20 * time.Millisecond): + } + close(release) + wg.Wait() +} + +func TestResetIncompleteVMDir(t *testing.T) { + dir := filepath.Join(t.TempDir(), "vms", "demo") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "OpenCore.qcow2"), []byte("partial"), 0o600); err != nil { + t.Fatal(err) + } + if err := resetIncompleteVMDir(t.Context(), dir); err != nil { + t.Fatalf("reset incomplete dir: %v", err) + } + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Fatalf("incomplete directory still exists: %v", err) + } +} + +func TestResetIncompleteVMDirRefusesCommittedRecord(t *testing.T) { + dir := filepath.Join(t.TempDir(), "vms", "demo") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "vm.json"), []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + if err := resetIncompleteVMDir(t.Context(), dir); err == nil { + t.Fatal("committed VM directory was accepted as incomplete") + } +} + func TestGraceFromFlags(t *testing.T) { tests := []struct { name string