From 9503a04782027bf958b2f7d4293c7f640b60e7df Mon Sep 17 00:00:00 2001 From: CMGS Date: Sat, 24 Oct 2026 21:09:00 +0800 Subject: [PATCH 1/4] feat(firecracker): hot-plug behind PCIe root ports and wait for device removal Upstream Firecracker now places every post-boot PCI device behind a free PCIe root port and fails the hot-plug when machine-config sets none, which is the default. A DELETE of a device behind a root port is now graceful: it returns at once, and the device stays until the guest releases it. Give each --pci VM 8 root ports. The field stays off the wire for MMIO VMs, because Firecracker rejects root ports there. After a DELETE, poll /vm/config until the device is gone, with the same 30s bound as the Cloud Hypervisor eject wait. A NIC the guest never releases returns ErrEjectPending, as on Cloud Hypervisor. The released Firecracker build removes the device at once, so the first poll returns there. A DELETE body ({"force": true}) is not an option: that build rejects any body. --- docs/devices.md | 7 ++- docs/firecracker.md | 1 + hypervisor/firecracker/api.go | 31 +++++++++- hypervisor/firecracker/extend.go | 20 +++++-- hypervisor/firecracker/extend_test.go | 84 ++++++++++++++++++++++++--- hypervisor/firecracker/start.go | 15 +++-- 6 files changed, 136 insertions(+), 22 deletions(-) diff --git a/docs/devices.md b/docs/devices.md index 2b627f11..33e1876d 100644 --- a/docs/devices.md +++ b/docs/devices.md @@ -82,8 +82,9 @@ Flags: On Cloud Hypervisor, detach blocks until the guest acks the ACPI eject (up to 30s — Windows can take 10–20s), so when it returns the slot, the name, and the backing file are free for immediate reuse; a guest that never acks fails the -detach with an error naming the device. Firecracker detach returns as soon as -the DELETE lands; run the printed hint so the guest drops the stale node. +detach with an error naming the device. Firecracker detach also waits, with +the same 30s bound, until the device leaves the VM config: behind a PCIe root +port Firecracker removes the device only after the guest releases it. The block device (`/dev/vdX`, serial visible in `lsblk -o NAME,SERIAL`) is usable immediately after attach. The `/dev/disk/by-id/virtio-` symlink @@ -112,7 +113,7 @@ cocoon vm device detach my-vm --id mygpu ## Firecracker (`--pci`) -Firecracker VMs created with `--pci` accept `vm disk attach`/`detach` (vhost-user-fs and VFIO stay Cloud Hypervisor only); MMIO VMs are rejected. Firecracker does not notify the guest, so cocoon prints the guest-side step after each call, and `--output json` carries it as `hints`: +Firecracker VMs created with `--pci` accept `vm disk attach`/`detach` (vhost-user-fs and VFIO stay Cloud Hypervisor only); MMIO VMs are rejected. Cocoon gives each `--pci` VM 8 PCIe root ports. A Firecracker build that places hot-plugged devices behind them notifies the guest, which then finds and releases the devices itself. An older build plugs devices onto the root bus and does not notify the guest, so cocoon prints the guest-side step after each call, and `--output json` carries it as `hints`: ```bash cocoon vm disk attach my-vm --path /vols/scratch.raw --name scratch diff --git a/docs/firecracker.md b/docs/firecracker.md index 55a44e11..8336684f 100644 --- a/docs/firecracker.md +++ b/docs/firecracker.md @@ -42,6 +42,7 @@ cocoon vm clone my-snap --name clone-vm - **OCI images only**: `--fc` is mutually exclusive with `--windows`, `--shared-memory`, `--hugepages`, `--mergeable` and `--no-watchdog`, and rejects cloudimg (UEFI boot) images - **MMIO by default**: without `--pci` (fixed for the VM lifetime, inherited by snapshots) disk attach/detach, NIC resize and clone-time `--data-disk`/`--nics` are refused +- **8 hot-plugged devices per VM**: a `--pci` VM gets 8 PCIe root ports, and a Firecracker build that places hot-plugged devices behind root ports refuses a 9th device. A snapshot keeps the root ports of its VM, so clones of a snapshot made before cocoon set root ports cannot hot-plug on such a build; create the base again - **Hot-plug is ahead of the published spec**: the Firecracker swagger still lists `PUT /drives/{id}` as pre-boot only and omits the `DELETE` verbs, while the router implements post-boot PUT and DELETE for drives and network interfaces; `--pci` hot-plug relies on the router, not the spec - **io_uring required**: writable disks use the `Async` engine with no opt-out, so a restrictive seccomp profile (Docker's default) breaks them; `--no-direct-io` is ignored - **Stop waits the full timeout**: FC guests without i8042 never answer CtrlAltDel, so `vm stop` waits out `stop_timeout_seconds` before SIGKILL diff --git a/hypervisor/firecracker/api.go b/hypervisor/firecracker/api.go index 1525a2b1..064822df 100644 --- a/hypervisor/firecracker/api.go +++ b/hypervisor/firecracker/api.go @@ -6,6 +6,8 @@ import ( "fmt" "net/http" "path/filepath" + "slices" + "time" "github.com/cocoonstack/cocoon/hypervisor" "github.com/cocoonstack/cocoon/utils" @@ -25,12 +27,18 @@ const ( // hotDiskIDPrefix: Firecracker resource ids allow only [A-Za-z0-9_], so hot-attached disks cannot reuse the cocoon-disk- scheme. hotDiskIDPrefix = "cocoon_disk_" + + // pcieHotplugPorts bounds the devices a PCI VM can hot-plug; each root port takes one root-bus slot of 32. + pcieHotplugPorts = 8 + // deviceRemovalTimeout bounds the guest release of a device behind a root port; Linux pciehp waits 5 s before it acts. + deviceRemovalTimeout = 30 * time.Second ) // fcMachineConfig is the /machine-config payload. type fcMachineConfig struct { - VCPUCount int `json:"vcpu_count"` - MemSizeMiB int `json:"mem_size_mib"` + VCPUCount int `json:"vcpu_count"` + MemSizeMiB int `json:"mem_size_mib"` + PCIeHotplugPorts int `json:"pcie_hotplug_ports,omitzero"` } type fcBootSource struct { @@ -107,6 +115,14 @@ type fcVMConfig struct { NetworkInterfaces []fcNetworkInterface `json:"network-interfaces"` } +func (c *fcVMConfig) hasDrive(id string) bool { + return slices.ContainsFunc(c.Drives, func(d fcDrive) bool { return d.DriveID == id }) +} + +func (c *fcVMConfig) hasNIC(id string) bool { + return slices.ContainsFunc(c.NetworkInterfaces, func(n fcNetworkInterface) bool { return n.IfaceID == id }) +} + func putJSON[T any](ctx context.Context, hc *http.Client, endpoint string, payload T, kind string) error { _, err := utils.DoWithRetry(ctx, func() ([]byte, error) { return utils.DoJSONOnce(ctx, hc, http.MethodPut, "http://localhost"+endpoint, kind, payload) @@ -245,3 +261,14 @@ func deleteDevice(ctx context.Context, hc *http.Client, endpoint string) error { _, err := utils.DoAPIOnce(ctx, hc, http.MethodDelete, "http://localhost"+endpoint, nil, http.StatusNoContent) return err } + +// waitDeviceRemoved polls until present is false: behind a root port Firecracker drops a device only after the guest releases it. +func waitDeviceRemoved(ctx context.Context, hc *http.Client, present func(*fcVMConfig) bool) error { + return utils.WaitFor(ctx, deviceRemovalTimeout, 100*time.Millisecond, func() (bool, error) { + cfg, err := getVMConfig(ctx, hc) + if err != nil { + return false, err + } + return !present(cfg), nil + }) +} diff --git a/hypervisor/firecracker/extend.go b/hypervisor/firecracker/extend.go index e686f7c0..a8ee422d 100644 --- a/hypervisor/firecracker/extend.go +++ b/hypervisor/firecracker/extend.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net/http" - "slices" "strings" "github.com/projecteru2/core/log" @@ -17,7 +16,6 @@ import ( "github.com/cocoonstack/cocoon/types" ) -// removal returns when the VMM drops the device; the guest releases the stale PCI node itself. type fcNICOps struct { hc *http.Client } @@ -44,7 +42,13 @@ func (o fcNICOps) AddNIC(ctx context.Context, index int, nc *types.NetworkConfig } func (o fcNICOps) RemoveNIC(ctx context.Context, id string) error { - return deleteDevice(ctx, o.hc, "/network-interfaces/"+id) + if err := deleteDevice(ctx, o.hc, "/network-interfaces/"+id); err != nil { + return err + } + if err := waitDeviceRemoved(ctx, o.hc, func(cfg *fcVMConfig) bool { return cfg.hasNIC(id) }); err != nil { + return fmt.Errorf("%w: %w", hypervisor.ErrEjectPending, err) + } + return nil } // TAPQueues is one queue pair: Firecracker opens every TAP single-queue. @@ -116,11 +120,17 @@ func (fc *Firecracker) DiskDetach(ctx context.Context, vmRef, name string) error if err != nil { return err } - if !slices.ContainsFunc(cfg.Drives, func(d fcDrive) bool { return d.DriveID == id }) { + if !cfg.hasDrive(id) { log.WithFunc("firecracker.DiskDetach").Warnf(ctx, "disk %q is not attached to vm %s, nothing to detach", name, vmRef) return nil } - return deleteDevice(ctx, hc, "/drives/"+id) + if err := deleteDevice(ctx, hc, "/drives/"+id); err != nil { + return err + } + if err := waitDeviceRemoved(ctx, hc, func(cfg *fcVMConfig) bool { return cfg.hasDrive(id) }); err != nil { + return fmt.Errorf("drive %s removal initiated but the guest has not released it: %w", id, err) + } + return nil } // DiskList returns nil (not error) for stopped and MMIO VMs so inspect can omit the field. diff --git a/hypervisor/firecracker/extend_test.go b/hypervisor/firecracker/extend_test.go index 93c8f6f2..339ea876 100644 --- a/hypervisor/firecracker/extend_test.go +++ b/hypervisor/firecracker/extend_test.go @@ -3,6 +3,7 @@ package firecracker import ( "context" "encoding/json/v2" + "errors" "io" "net" "net/http" @@ -12,6 +13,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/cocoonstack/cocoon/extend/disk" "github.com/cocoonstack/cocoon/hypervisor" @@ -134,6 +136,46 @@ func TestConvergeOrphanedPause(t *testing.T) { } } +func TestRemoveNICWaitsForGuestRelease(t *testing.T) { + srv := newFCStub(t, fcVMConfig{NetworkInterfaces: []fcNetworkInterface{{IfaceID: "eth0"}, {IfaceID: "eth1"}}}) + srv.releaseAfter(2) + if err := (fcNICOps{hc: srv.client}).RemoveNIC(t.Context(), "eth1"); err != nil { + t.Fatalf("RemoveNIC: %v", err) + } + cfg, err := getVMConfig(t.Context(), srv.client) + if err != nil { + t.Fatalf("getVMConfig: %v", err) + } + if cfg.hasNIC("eth1") || !cfg.hasNIC("eth0") { + t.Fatalf("nics = %+v, want RemoveNIC to return only after eth1 left the config", cfg.NetworkInterfaces) + } +} + +func TestRemoveNICReportsAnUnreleasedDevice(t *testing.T) { + srv := newFCStub(t, fcVMConfig{NetworkInterfaces: []fcNetworkInterface{{IfaceID: "eth1"}}}) + srv.releaseAfter(-1) + ctx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) + defer cancel() + if err := (fcNICOps{hc: srv.client}).RemoveNIC(ctx, "eth1"); !errors.Is(err, hypervisor.ErrEjectPending) { + t.Fatalf("err = %v, want ErrEjectPending", err) + } +} + +func TestMachineConfigFor(t *testing.T) { + rec := &hypervisor.VMRecord{Config: types.VMConfig{CPU: 2, Memory: 1 << 30}} + mmio, err := json.Marshal(machineConfigFor(rec)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if strings.Contains(string(mmio), "pcie_hotplug_ports") || !strings.Contains(string(mmio), `"mem_size_mib":1024`) { + t.Fatalf("mmio machine-config = %s, want no root ports", mmio) + } + rec.Config.PCI = true + if got := machineConfigFor(rec).PCIeHotplugPorts; got != pcieHotplugPorts { + t.Fatalf("pci root ports = %d, want %d", got, pcieHotplugPorts) + } +} + type fcStub struct { client *http.Client mu sync.Mutex @@ -142,6 +184,9 @@ type fcStub struct { puts map[string]string dels []string resume int + + release int + pending []string } func (s *fcStub) put(path string) string { @@ -162,6 +207,12 @@ func (s *fcStub) resumes() int { return s.resume } +func (s *fcStub) releaseAfter(reads int) { + s.mu.Lock() + defer s.mu.Unlock() + s.release = reads +} + func (s *fcStub) setState(state string) { s.mu.Lock() defer s.mu.Unlock() @@ -187,8 +238,11 @@ func (s *fcStub) handleDevice(w http.ResponseWriter, r *http.Request) { case http.MethodDelete: s.dels = append(s.dels, r.URL.Path) id := path.Base(r.URL.Path) - s.cfg.NetworkInterfaces = slices.DeleteFunc(s.cfg.NetworkInterfaces, func(n fcNetworkInterface) bool { return n.IfaceID == id }) - s.cfg.Drives = slices.DeleteFunc(s.cfg.Drives, func(d fcDrive) bool { return d.DriveID == id }) + if s.release == 0 { + s.drop(id) + } else { + s.pending = append(s.pending, id) + } default: http.Error(w, r.Method, http.StatusMethodNotAllowed) return @@ -196,6 +250,26 @@ func (s *fcStub) handleDevice(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } +func (s *fcStub) drop(id string) { + s.cfg.NetworkInterfaces = slices.DeleteFunc(s.cfg.NetworkInterfaces, func(n fcNetworkInterface) bool { return n.IfaceID == id }) + s.cfg.Drives = slices.DeleteFunc(s.cfg.Drives, func(d fcDrive) bool { return d.DriveID == id }) +} + +func (s *fcStub) serveConfig(w http.ResponseWriter, _ *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + if len(s.pending) > 0 && s.release > 0 { + s.release-- + if s.release == 0 { + for _, id := range s.pending { + s.drop(id) + } + s.pending = nil + } + } + _ = json.MarshalWrite(w, s.cfg) +} + func newFCStub(t *testing.T, cfg fcVMConfig) *fcStub { t.Helper() s := &fcStub{cfg: cfg, state: "Running", puts: map[string]string{}} @@ -209,11 +283,7 @@ func newFCStub(t *testing.T, cfg fcVMConfig) *fcStub { defer s.mu.Unlock() _ = json.MarshalWrite(w, fcInstanceInfo{State: s.state}) }) - mux.HandleFunc("/vm/config", func(w http.ResponseWriter, _ *http.Request) { - s.mu.Lock() - defer s.mu.Unlock() - _ = json.MarshalWrite(w, s.cfg) - }) + mux.HandleFunc("/vm/config", s.serveConfig) mux.HandleFunc("/vm", func(w http.ResponseWriter, r *http.Request) { s.mu.Lock() defer s.mu.Unlock() diff --git a/hypervisor/firecracker/start.go b/hypervisor/firecracker/start.go index d7525198..e07937e2 100644 --- a/hypervisor/firecracker/start.go +++ b/hypervisor/firecracker/start.go @@ -42,11 +42,7 @@ func (fc *Firecracker) startOne(ctx context.Context, id string, scan *utils.Proc func (fc *Firecracker) configureVM(ctx context.Context, hc *http.Client, rec *hypervisor.VMRecord) error { logger := log.WithFunc("firecracker.configureVM") - memMiB := int(rec.Config.Memory >> 20) - if err := putMachineConfig(ctx, hc, fcMachineConfig{ - VCPUCount: rec.Config.CPU, - MemSizeMiB: memMiB, - }); err != nil { + if err := putMachineConfig(ctx, hc, machineConfigFor(rec)); err != nil { return fmt.Errorf("machine-config: %w", err) } @@ -299,3 +295,12 @@ func launchArgs(rec *hypervisor.VMRecord, sockPath, fcLog string) []string { } return args } + +// Firecracker rejects pcie_hotplug_ports above 0 on an MMIO VM. +func machineConfigFor(rec *hypervisor.VMRecord) fcMachineConfig { + cfg := fcMachineConfig{VCPUCount: rec.Config.CPU, MemSizeMiB: int(rec.Config.Memory >> 20)} + if rec.Config.PCI { + cfg.PCIeHotplugPorts = pcieHotplugPorts + } + return cfg +} From 5e858f9f4d7003d79a8e8f838403ba4037dee85f Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 25 Oct 2026 00:14:51 +0800 Subject: [PATCH 2/4] review: PR #290 walkthrough - comment budget, verb-noun buildMachineConfig, method-aware test route Trim waitDeviceRemoved and pcieHotplugPorts comments to the fact the code cannot carry, drop two comments that restate type names, capitalize own-line comments in start.go, rename machineConfigFor to buildMachineConfig, stop the removal closures shadowing cfg, and register the stub route as GET /vm/config. --- hypervisor/firecracker/api.go | 6 ++---- hypervisor/firecracker/extend.go | 4 ++-- hypervisor/firecracker/extend_test.go | 8 ++++---- hypervisor/firecracker/start.go | 10 +++++----- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/hypervisor/firecracker/api.go b/hypervisor/firecracker/api.go index 064822df..04fdc2f0 100644 --- a/hypervisor/firecracker/api.go +++ b/hypervisor/firecracker/api.go @@ -28,13 +28,12 @@ const ( // hotDiskIDPrefix: Firecracker resource ids allow only [A-Za-z0-9_], so hot-attached disks cannot reuse the cocoon-disk- scheme. hotDiskIDPrefix = "cocoon_disk_" - // pcieHotplugPorts bounds the devices a PCI VM can hot-plug; each root port takes one root-bus slot of 32. + // pcieHotplugPorts: each root port holds one hot-plugged device and takes one of the 32 root-bus slots. pcieHotplugPorts = 8 // deviceRemovalTimeout bounds the guest release of a device behind a root port; Linux pciehp waits 5 s before it acts. deviceRemovalTimeout = 30 * time.Second ) -// fcMachineConfig is the /machine-config payload. type fcMachineConfig struct { VCPUCount int `json:"vcpu_count"` MemSizeMiB int `json:"mem_size_mib"` @@ -105,7 +104,6 @@ type fcVsockOverride struct { UDSPath string `json:"uds_path"` } -// fcInstanceInfo is the GET / view. type fcInstanceInfo struct { State string `json:"state"` } @@ -262,7 +260,7 @@ func deleteDevice(ctx context.Context, hc *http.Client, endpoint string) error { return err } -// waitDeviceRemoved polls until present is false: behind a root port Firecracker drops a device only after the guest releases it. +// Behind a root port Firecracker drops a device only after the guest releases it. func waitDeviceRemoved(ctx context.Context, hc *http.Client, present func(*fcVMConfig) bool) error { return utils.WaitFor(ctx, deviceRemovalTimeout, 100*time.Millisecond, func() (bool, error) { cfg, err := getVMConfig(ctx, hc) diff --git a/hypervisor/firecracker/extend.go b/hypervisor/firecracker/extend.go index a8ee422d..641a7015 100644 --- a/hypervisor/firecracker/extend.go +++ b/hypervisor/firecracker/extend.go @@ -45,7 +45,7 @@ func (o fcNICOps) RemoveNIC(ctx context.Context, id string) error { if err := deleteDevice(ctx, o.hc, "/network-interfaces/"+id); err != nil { return err } - if err := waitDeviceRemoved(ctx, o.hc, func(cfg *fcVMConfig) bool { return cfg.hasNIC(id) }); err != nil { + if err := waitDeviceRemoved(ctx, o.hc, func(c *fcVMConfig) bool { return c.hasNIC(id) }); err != nil { return fmt.Errorf("%w: %w", hypervisor.ErrEjectPending, err) } return nil @@ -127,7 +127,7 @@ func (fc *Firecracker) DiskDetach(ctx context.Context, vmRef, name string) error if err := deleteDevice(ctx, hc, "/drives/"+id); err != nil { return err } - if err := waitDeviceRemoved(ctx, hc, func(cfg *fcVMConfig) bool { return cfg.hasDrive(id) }); err != nil { + if err := waitDeviceRemoved(ctx, hc, func(c *fcVMConfig) bool { return c.hasDrive(id) }); err != nil { return fmt.Errorf("drive %s removal initiated but the guest has not released it: %w", id, err) } return nil diff --git a/hypervisor/firecracker/extend_test.go b/hypervisor/firecracker/extend_test.go index 339ea876..ecdfec9d 100644 --- a/hypervisor/firecracker/extend_test.go +++ b/hypervisor/firecracker/extend_test.go @@ -161,9 +161,9 @@ func TestRemoveNICReportsAnUnreleasedDevice(t *testing.T) { } } -func TestMachineConfigFor(t *testing.T) { +func TestBuildMachineConfig(t *testing.T) { rec := &hypervisor.VMRecord{Config: types.VMConfig{CPU: 2, Memory: 1 << 30}} - mmio, err := json.Marshal(machineConfigFor(rec)) + mmio, err := json.Marshal(buildMachineConfig(rec)) if err != nil { t.Fatalf("marshal: %v", err) } @@ -171,7 +171,7 @@ func TestMachineConfigFor(t *testing.T) { t.Fatalf("mmio machine-config = %s, want no root ports", mmio) } rec.Config.PCI = true - if got := machineConfigFor(rec).PCIeHotplugPorts; got != pcieHotplugPorts { + if got := buildMachineConfig(rec).PCIeHotplugPorts; got != pcieHotplugPorts { t.Fatalf("pci root ports = %d, want %d", got, pcieHotplugPorts) } } @@ -283,7 +283,7 @@ func newFCStub(t *testing.T, cfg fcVMConfig) *fcStub { defer s.mu.Unlock() _ = json.MarshalWrite(w, fcInstanceInfo{State: s.state}) }) - mux.HandleFunc("/vm/config", s.serveConfig) + mux.HandleFunc("GET /vm/config", s.serveConfig) mux.HandleFunc("/vm", func(w http.ResponseWriter, r *http.Request) { s.mu.Lock() defer s.mu.Unlock() diff --git a/hypervisor/firecracker/start.go b/hypervisor/firecracker/start.go index e07937e2..beda54c1 100644 --- a/hypervisor/firecracker/start.go +++ b/hypervisor/firecracker/start.go @@ -42,7 +42,7 @@ func (fc *Firecracker) startOne(ctx context.Context, id string, scan *utils.Proc func (fc *Firecracker) configureVM(ctx context.Context, hc *http.Client, rec *hypervisor.VMRecord) error { logger := log.WithFunc("firecracker.configureVM") - if err := putMachineConfig(ctx, hc, machineConfigFor(rec)); err != nil { + if err := putMachineConfig(ctx, hc, buildMachineConfig(rec)); err != nil { return fmt.Errorf("machine-config: %w", err) } @@ -138,7 +138,7 @@ func (fc *Firecracker) launchProcessWithLeases(ctx context.Context, rec *hypervi } defer slave.Close() //nolint:errcheck - // shell out: the firecracker binary is the authoritative VMM. + // Shell out: the firecracker binary is the authoritative VMM. fcCmd := exec.Command(fc.conf.FCBinary, launchArgs(rec, sockPath, fcLog)...) //nolint:gosec fcCmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} fcCmd.Stdin = slave @@ -166,7 +166,7 @@ func (fc *Firecracker) launchProcessWithLeases(ctx context.Context, rec *hypervi <-exited return 0, nil, fmt.Errorf("start source-lease relay: %w", relayErr) default: - // the socket file outlives a failed relay and would make inspect report a console nobody serves + // The socket file outlives a failed relay and would make inspect report a console nobody serves. _ = os.Remove(hypervisor.ConsoleSockPath(rec.RunDir)) logger.Warnf(ctx, "console relay failed (console unavailable): %v", relayErr) // A failed relay kept master open to preserve ttyS0; close it once FC exits or the fd leaks forever. @@ -213,7 +213,7 @@ func (fc *Firecracker) startConsoleRelay(runDir string, master *os.File, fcPID i } } - // shell out because self-exec spawns a detached console-relay process that survives the parent. + // Shell out because self-exec spawns a detached console-relay process that survives the parent. relayCmd := exec.Command(self) //nolint:gosec relayCmd.Env = []string{ relayEnvKey + "=1", @@ -297,7 +297,7 @@ func launchArgs(rec *hypervisor.VMRecord, sockPath, fcLog string) []string { } // Firecracker rejects pcie_hotplug_ports above 0 on an MMIO VM. -func machineConfigFor(rec *hypervisor.VMRecord) fcMachineConfig { +func buildMachineConfig(rec *hypervisor.VMRecord) fcMachineConfig { cfg := fcMachineConfig{VCPUCount: rec.Config.CPU, MemSizeMiB: int(rec.Config.Memory >> 20)} if rec.Config.PCI { cfg.PCIeHotplugPorts = pcieHotplugPorts From 83a65a653abd91dd1a84ad7fa57becba51aa1346 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 25 Oct 2026 00:14:51 +0800 Subject: [PATCH 3/4] docs(firecracker): guest needs PCIe hot-plug; guest hints apply to root-bus builds only The removal wait completes only when the guest kernel runs pciehp, and the printed guest steps are needed only when Firecracker plugs devices onto the root bus. --- cmd/vm/hotplughints.go | 2 +- docs/firecracker.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/vm/hotplughints.go b/cmd/vm/hotplughints.go index e70efb93..5b696be6 100644 --- a/cmd/vm/hotplughints.go +++ b/cmd/vm/hotplughints.go @@ -34,7 +34,7 @@ func printGuestHints(hints []string) { return } fmt.Println() - fmt.Println("Run inside the guest (PCI transport: the guest discovers and releases devices itself):") + fmt.Println("Run inside the guest if Firecracker plugs devices onto the root bus (a root-port build needs no step):") fmt.Println() for _, h := range hints { fmt.Printf(" %s\n", h) diff --git a/docs/firecracker.md b/docs/firecracker.md index 8336684f..e50fc7e1 100644 --- a/docs/firecracker.md +++ b/docs/firecracker.md @@ -42,7 +42,7 @@ cocoon vm clone my-snap --name clone-vm - **OCI images only**: `--fc` is mutually exclusive with `--windows`, `--shared-memory`, `--hugepages`, `--mergeable` and `--no-watchdog`, and rejects cloudimg (UEFI boot) images - **MMIO by default**: without `--pci` (fixed for the VM lifetime, inherited by snapshots) disk attach/detach, NIC resize and clone-time `--data-disk`/`--nics` are refused -- **8 hot-plugged devices per VM**: a `--pci` VM gets 8 PCIe root ports, and a Firecracker build that places hot-plugged devices behind root ports refuses a 9th device. A snapshot keeps the root ports of its VM, so clones of a snapshot made before cocoon set root ports cannot hot-plug on such a build; create the base again +- **8 hot-plugged devices per VM**: a `--pci` VM gets 8 PCIe root ports, and a Firecracker build that places hot-plugged devices behind root ports refuses a 9th device. The guest kernel needs PCIe hot-plug (`CONFIG_HOTPLUG_PCI_PCIE`), or every detach waits 30s and fails. A snapshot keeps the root ports of its VM, so clones of a snapshot made before cocoon set root ports cannot hot-plug on such a build; create the base again - **Hot-plug is ahead of the published spec**: the Firecracker swagger still lists `PUT /drives/{id}` as pre-boot only and omits the `DELETE` verbs, while the router implements post-boot PUT and DELETE for drives and network interfaces; `--pci` hot-plug relies on the router, not the spec - **io_uring required**: writable disks use the `Async` engine with no opt-out, so a restrictive seccomp profile (Docker's default) breaks them; `--no-direct-io` is ignored - **Stop waits the full timeout**: FC guests without i8042 never answer CtrlAltDel, so `vm stop` waits out `stop_timeout_seconds` before SIGKILL From e73fd1d6d4059ce4f346777b483f553c6c4f360b Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 25 Oct 2026 01:00:47 +0800 Subject: [PATCH 4/4] cut: loc-justify round on PR #290 (firecracker api, extend, start) - createSnapshotFC and loadSnapshotFC call sendJSONOnce on their long-timeout socket client instead of hand-rolling marshal plus DoAPIOnce. Same client, no retry, same 204; only the marshal-error text changes. - putEntropy calls putJSON with struct{}{}: same retry, PUT, 204 and {} body. - LiveNICs drops the idx reset after Sscanf: a failed Sscanf never writes idx (checked for EOF, non-integer, mismatch and overflow inputs). - Drop the goroutine that closed the PTY master after a failed console relay. It only mattered in a long-lived process that launches VMs; the daemon launches none and no sibling repo imports cocoon/hypervisor, so the CLI exit closes the fd. Existing tests cover LiveNICs; the other three rest on the helper source. The relay 5s timeout stays: it bounds a hang under the ops lock. --- hypervisor/firecracker/api.go | 25 +++++-------------------- hypervisor/firecracker/extend.go | 4 +--- hypervisor/firecracker/start.go | 5 ----- 3 files changed, 6 insertions(+), 28 deletions(-) diff --git a/hypervisor/firecracker/api.go b/hypervisor/firecracker/api.go index 04fdc2f0..24714c88 100644 --- a/hypervisor/firecracker/api.go +++ b/hypervisor/firecracker/api.go @@ -162,10 +162,7 @@ func putNetworkInterface(ctx context.Context, hc *http.Client, iface fcNetworkIn } func putEntropy(ctx context.Context, hc *http.Client) error { - _, err := utils.DoWithRetry(ctx, func() ([]byte, error) { - return utils.DoAPIOnce(ctx, hc, http.MethodPut, "http://localhost/entropy", []byte("{}")) - }) - return err + return putJSON(ctx, hc, "/entropy", struct{}{}, "entropy") } func putVsock(ctx context.Context, hc *http.Client, vsock fcVsock) error { @@ -193,17 +190,11 @@ func resumeVM(ctx context.Context, hc *http.Client) error { // createSnapshotFC does not retry: a resend re-transfers multi-GiB and clobbers a partial state.json. func createSnapshotFC(ctx context.Context, sockPath, destDir string) error { - body, err := json.Marshal(fcSnapshotCreate{ + hc := utils.NewSocketHTTPClientWithTimeout(sockPath, hypervisor.VMMemTransferTimeout) + return sendJSONOnce(ctx, hc, http.MethodPut, "/snapshot/create", fcSnapshotCreate{ SnapshotPath: filepath.Join(destDir, snapshotVMStateFile), MemFilePath: filepath.Join(destDir, snapshotMemFile), - }) - if err != nil { - return fmt.Errorf("marshal snapshot/create request: %w", err) - } - hc := utils.NewSocketHTTPClientWithTimeout(sockPath, hypervisor.VMMemTransferTimeout) - _, err = utils.DoAPIOnce(ctx, hc, http.MethodPut, - "http://localhost/snapshot/create", body, http.StatusNoContent) - return err + }, "snapshot/create request") } // an empty vsockUDSOverride inherits the snapshot's path (FC < v1.16). @@ -219,14 +210,8 @@ func loadSnapshotFC(ctx context.Context, sockPath, sourceDir string, networkOver if vsockUDSOverride != "" { req.VsockOverride = &fcVsockOverride{UDSPath: vsockUDSOverride} } - body, err := json.Marshal(req) - if err != nil { - return fmt.Errorf("marshal snapshot/load request: %w", err) - } hc := utils.NewSocketHTTPClientWithTimeout(sockPath, hypervisor.VMMemTransferTimeout) - _, err = utils.DoAPIOnce(ctx, hc, http.MethodPut, - "http://localhost/snapshot/load", body, http.StatusNoContent) - return err + return sendJSONOnce(ctx, hc, http.MethodPut, "/snapshot/load", req, "snapshot/load request") } func getInstanceInfo(ctx context.Context, hc *http.Client) (*fcInstanceInfo, error) { diff --git a/hypervisor/firecracker/extend.go b/hypervisor/firecracker/extend.go index 641a7015..d79b8e86 100644 --- a/hypervisor/firecracker/extend.go +++ b/hypervisor/firecracker/extend.go @@ -28,9 +28,7 @@ func (o fcNICOps) LiveNICs(ctx context.Context) ([]hypervisor.LiveNIC, error) { live := make([]hypervisor.LiveNIC, 0, len(cfg.NetworkInterfaces)) for _, n := range cfg.NetworkInterfaces { idx := -1 - if _, err := fmt.Sscanf(n.IfaceID, ifaceIDFmt, &idx); err != nil { - idx = -1 - } + _, _ = fmt.Sscanf(n.IfaceID, ifaceIDFmt, &idx) live = append(live, hypervisor.LiveNIC{ID: n.IfaceID, Index: idx}) } return live, nil diff --git a/hypervisor/firecracker/start.go b/hypervisor/firecracker/start.go index beda54c1..338a703d 100644 --- a/hypervisor/firecracker/start.go +++ b/hypervisor/firecracker/start.go @@ -169,11 +169,6 @@ func (fc *Firecracker) launchProcessWithLeases(ctx context.Context, rec *hypervi // The socket file outlives a failed relay and would make inspect report a console nobody serves. _ = os.Remove(hypervisor.ConsoleSockPath(rec.RunDir)) logger.Warnf(ctx, "console relay failed (console unavailable): %v", relayErr) - // A failed relay kept master open to preserve ttyS0; close it once FC exits or the fd leaks forever. - go func() { - <-exited - _ = master.Close() - }() } return pid, leaseControl, nil }