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/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..e50fc7e1 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. 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 diff --git a/hypervisor/firecracker/api.go b/hypervisor/firecracker/api.go index 1525a2b1..24714c88 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,17 @@ 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: 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"` + VCPUCount int `json:"vcpu_count"` + MemSizeMiB int `json:"mem_size_mib"` + PCIeHotplugPorts int `json:"pcie_hotplug_ports,omitzero"` } type fcBootSource struct { @@ -97,7 +104,6 @@ type fcVsockOverride struct { UDSPath string `json:"uds_path"` } -// fcInstanceInfo is the GET / view. type fcInstanceInfo struct { State string `json:"state"` } @@ -107,6 +113,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) @@ -148,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 { @@ -179,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). @@ -205,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) { @@ -245,3 +244,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 } + +// 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..d79b8e86 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 } @@ -30,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 @@ -44,7 +40,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(c *fcVMConfig) bool { return c.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 +118,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(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 } // 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..ecdfec9d 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 TestBuildMachineConfig(t *testing.T) { + rec := &hypervisor.VMRecord{Config: types.VMConfig{CPU: 2, Memory: 1 << 30}} + mmio, err := json.Marshal(buildMachineConfig(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 := buildMachineConfig(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("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 d7525198..338a703d 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, buildMachineConfig(rec)); err != nil { return fmt.Errorf("machine-config: %w", err) } @@ -142,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 @@ -170,14 +166,9 @@ 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. - go func() { - <-exited - _ = master.Close() - }() } return pid, leaseControl, nil } @@ -217,7 +208,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", @@ -299,3 +290,12 @@ func launchArgs(rec *hypervisor.VMRecord, sockPath, fcLog string) []string { } return args } + +// Firecracker rejects pcie_hotplug_ports above 0 on an MMIO VM. +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 + } + return cfg +}