feat(firecracker): hot-plug behind PCIe root ports and wait for device removal

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)

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-<name>` 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

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

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
+	})
+}

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.

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,15 +238,38 @@ 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
 	}
 	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()