From a8e356b3d280e981ee4bf2bb52c751bc1d2ffd62 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 27 May 2026 22:15:20 +1000 Subject: [PATCH] feat: ENI hot-plug Sprint 3a scaffolding (#257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lays the Sprint 3a Tier 0 groundwork from docs/development/feature/enihotplug-v1.md so the live QMP pipeline in 3b can plug in without further plumbing changes. - types.ENIRequests: per-VM PCIe slot allocator state (free-list + eniID→slot reverse map), mirrors EBSRequests pattern. - instancetypes.MaxENIsForType / HotPlugENISlotsForType: instance-type → ENI cap lookup with explicit-type-then-family-then-default precedence. - vm.DeviceController interface + QMPDeviceController (real) + StubDeviceController (in-memory): unit-testable QMP device_add / device_del / netdev_add / netdev_del surface. QueryPCI body lands in Sprint 3b alongside the live response parser. - buildBaseVMConfig now accepts the instance type and emits two distinct PCIe root-port pools at boot: hotplug-ebs{1..11} (fixed, /dev/sd[f-p]) and hotplug-eni{1..N} (per-type, N = MaxENIs - 1). - volumes.go bus id renamed hotplug%d → hotplug-ebs%d; query-block QDev regex narrowed to hotplug-ebs(\d+) so future virtio-net devices on hotplug-eni ports cannot collide with the EBS ordering. - restore + ResetNodeLocalState reset ENIRequests.Mu alongside the EBS mutex so restored VMs start with a fresh allocator. Closes mulga-cs-enihotplug-3a. --- spinifex/instancetypes/eni_limits.go | 70 ++++++ spinifex/instancetypes/eni_limits_test.go | 61 ++++++ spinifex/types/eni.go | 18 ++ spinifex/vm/device_map.go | 15 +- spinifex/vm/device_map_test.go | 11 +- spinifex/vm/lifecycle.go | 55 ++++- spinifex/vm/lifecycle_test.go | 53 ++++- spinifex/vm/qmp_device.go | 246 ++++++++++++++++++++++ spinifex/vm/qmp_device_test.go | 116 ++++++++++ spinifex/vm/restore.go | 1 + spinifex/vm/vm.go | 2 + spinifex/vm/volumes.go | 7 +- 12 files changed, 632 insertions(+), 23 deletions(-) create mode 100644 spinifex/instancetypes/eni_limits.go create mode 100644 spinifex/instancetypes/eni_limits_test.go create mode 100644 spinifex/types/eni.go create mode 100644 spinifex/vm/qmp_device.go create mode 100644 spinifex/vm/qmp_device_test.go diff --git a/spinifex/instancetypes/eni_limits.go b/spinifex/instancetypes/eni_limits.go new file mode 100644 index 000000000..cfe0aa34e --- /dev/null +++ b/spinifex/instancetypes/eni_limits.go @@ -0,0 +1,70 @@ +package instancetypes + +import "strings" + +// defaultMaxENIs is the fallback ENI cap for instance types not enumerated +// in maxENIsByType / maxENIsByFamily. Matches AWS's "small general purpose" +// envelope; deliberately conservative so undersized hosts cannot exhaust +// host taps from a single VM. +const defaultMaxENIs = 4 + +// maxENIsByType overrides maxENIsByFamily for size-specific caps where AWS +// docs publish a different number from the family default. +var maxENIsByType = map[string]int{ + "m5.large": 3, + "m5.xlarge": 3, + "m5.2xlarge": 4, + "m5.4xlarge": 8, +} + +// maxENIsByFamily is consulted when no type-specific entry exists. Sized to +// match AWS's published per-family ENI cap for representative members of +// each family; coarse on purpose for v1 — refine when a customer hits the +// cap. Keys are the dot-stripped family prefix (e.g. "t3", "m5"). +var maxENIsByFamily = map[string]int{ + "t2": 3, + "t3": 3, + "t3a": 3, + "t4g": 3, + "m5": 15, // covers m5.8xlarge and larger; smaller sizes overridden above + "m5a": 15, + "m6i": 15, + "m6a": 15, + "m6g": 8, + "m7i": 15, + "m7a": 15, + "m7g": 8, + "m8i": 15, + "m8a": 15, + "m8g": 8, +} + +// MaxENIsForType returns the total ENI cap for the given instance type +// (primary + secondaries). Lookup precedence: explicit type → family → +// defaultMaxENIs. The value is the hardware-imposed AWS limit, not a +// per-host quota; the host's hot-plug slot pool is sized as +// MaxENIsForType - 1 (the primary ENI consumes no hot-plug slot). +func MaxENIsForType(instanceType string) int { + if n, ok := maxENIsByType[instanceType]; ok { + return n + } + family, _, found := strings.Cut(instanceType, ".") + if found { + if n, ok := maxENIsByFamily[family]; ok { + return n + } + } + return defaultMaxENIs +} + +// HotPlugENISlotsForType returns the number of PCIe root ports that must be +// pre-allocated at VM start for runtime ENI hot-plug. Equals +// MaxENIsForType - 1 since the primary ENI is wired at boot via the fixed +// netdev path and does not consume a hot-plug slot. Always ≥ 0. +func HotPlugENISlotsForType(instanceType string) int { + n := MaxENIsForType(instanceType) - 1 + if n < 0 { + return 0 + } + return n +} diff --git a/spinifex/instancetypes/eni_limits_test.go b/spinifex/instancetypes/eni_limits_test.go new file mode 100644 index 000000000..7f5705142 --- /dev/null +++ b/spinifex/instancetypes/eni_limits_test.go @@ -0,0 +1,61 @@ +package instancetypes + +import "testing" + +func TestMaxENIsForType(t *testing.T) { + tests := []struct { + instanceType string + wantMax int + wantHotPlug int + }{ + // Explicit type overrides + {"m5.large", 3, 2}, + {"m5.xlarge", 3, 2}, + {"m5.2xlarge", 4, 3}, + {"m5.4xlarge", 8, 7}, + + // Family fallback (larger m5 sizes hit the family default of 15) + {"m5.8xlarge", 15, 14}, + {"m5.16xlarge", 15, 14}, + + // Burstable families + {"t3.nano", 3, 2}, + {"t3.micro", 3, 2}, + {"t3a.2xlarge", 3, 2}, + {"t4g.medium", 3, 2}, + + // ARM general-purpose family + {"m6g.large", 8, 7}, + + // x86 general-purpose family default + {"m7i.4xlarge", 15, 14}, + + // Unknown family → default + {"x9unknown.large", defaultMaxENIs, defaultMaxENIs - 1}, + + // Malformed input → default + {"", defaultMaxENIs, defaultMaxENIs - 1}, + {"nodot", defaultMaxENIs, defaultMaxENIs - 1}, + } + + for _, tt := range tests { + t.Run(tt.instanceType, func(t *testing.T) { + if got := MaxENIsForType(tt.instanceType); got != tt.wantMax { + t.Errorf("MaxENIsForType(%q) = %d, want %d", tt.instanceType, got, tt.wantMax) + } + if got := HotPlugENISlotsForType(tt.instanceType); got != tt.wantHotPlug { + t.Errorf("HotPlugENISlotsForType(%q) = %d, want %d", tt.instanceType, got, tt.wantHotPlug) + } + }) + } +} + +func TestHotPlugENISlotsForType_NeverNegative(t *testing.T) { + // Defensive: if a future maxENIsByType entry drops to 0, slot count + // must clamp at 0 rather than overflow into negative territory. + maxENIsByType["test.zero"] = 0 + defer delete(maxENIsByType, "test.zero") + if got := HotPlugENISlotsForType("test.zero"); got != 0 { + t.Errorf("HotPlugENISlotsForType(zero-cap) = %d, want 0", got) + } +} diff --git a/spinifex/types/eni.go b/spinifex/types/eni.go new file mode 100644 index 000000000..27e3aff9f --- /dev/null +++ b/spinifex/types/eni.go @@ -0,0 +1,18 @@ +package types + +import "sync" + +// ENIRequests tracks the PCIe hot-plug slot allocator for a single VM's +// hot-plugged ENIs. AvailableSlots is the free-list of slot indices (1-based, +// matching the pcie-root-port id=hotplug-eni{N} pre-allocated at VM start). +// AttachedByENIID maps eniID → assigned slot so the detach path can free the +// slot without re-scanning, and so the post-restart reconciler can rebuild +// the in-memory state from KV truth. +// +// Boot-time ENIs (primary + ExtraENIs) use fixed slot addresses configured +// at VM start and do NOT consume the hot-plug slot pool. +type ENIRequests struct { + Mu sync.Mutex `json:"-"` + AvailableSlots []int `json:"available_slots"` + AttachedByENIID map[string]int `json:"attached_by_eni_id"` +} diff --git a/spinifex/vm/device_map.go b/spinifex/vm/device_map.go index 1f9942351..7e7fc3036 100644 --- a/spinifex/vm/device_map.go +++ b/spinifex/vm/device_map.go @@ -17,13 +17,16 @@ import ( // Example: "/machine/peripheral-anon/device[0]/virtio-backend" → 0 var pciAddrRegexp = regexp.MustCompile(`device\[(\d+)\]`) -// hotplugPortRegexp extracts the hotplug port number from a hot-plugged QDev path. -// Example: "/machine/peripheral/vdisk-vol-xxx/hotplug3/virtio-backend" or +// hotplugPortRegexp extracts the EBS hot-plug port number from a QDev path. +// Example: "/machine/peripheral/vdisk-vol-xxx/hotplug-ebs3/virtio-backend" or // // "/machine/peripheral/vdisk-vol-xxx/virtio-backend" (bus assigned by QEMU) // // The chassis number from the QEMU command line determines PCI slot order. -var hotplugPortRegexp = regexp.MustCompile(`hotplug(\d+)`) +// ENI hot-plug ports (`hotplug-eni{N}`) are explicitly excluded — virtio-net +// devices are not surfaced through query-block, but the explicit suffix +// guards against future query callers conflating the two pools. +var hotplugPortRegexp = regexp.MustCompile(`hotplug-ebs(\d+)`) // queryGuestDeviceMap uses QMP query-block to build a map from QEMU device ID // (e.g. "os", "cloudinit", "vdisk-vol-xxx") to the guest device path @@ -232,9 +235,9 @@ func extractPeripheralName(qdev string) string { return "" } -// extractHotplugPort extracts the hotplug port number from a QDev path. -// For example: "/machine/peripheral/vdisk-vol-xxx/hotplug3/virtio-backend" → 3 -// Returns -1 if no hotplug port is found. +// extractHotplugPort extracts the EBS hot-plug port number from a QDev path. +// For example: "/machine/peripheral/vdisk-vol-xxx/hotplug-ebs3/virtio-backend" → 3 +// Returns -1 if no EBS hot-plug port is found. func extractHotplugPort(qdev string) int { matches := hotplugPortRegexp.FindStringSubmatch(qdev) if len(matches) < 2 { diff --git a/spinifex/vm/device_map_test.go b/spinifex/vm/device_map_test.go index 7ad4a463a..54d366c5d 100644 --- a/spinifex/vm/device_map_test.go +++ b/spinifex/vm/device_map_test.go @@ -28,7 +28,7 @@ func TestExtractPCIIndex(t *testing.T) { }, { name: "hotplug device with higher index", - qdev: "/machine/peripheral/hotplug1/device[12]/virtio-backend", + qdev: "/machine/peripheral/hotplug-ebs1/device[12]/virtio-backend", want: 12, }, { @@ -299,8 +299,8 @@ func TestExtractHotplugPort(t *testing.T) { want int }{ { - name: "hotplug port 3", - qdev: "/machine/peripheral/vdisk-vol-xxx/hotplug3/virtio-backend", + name: "hotplug-ebs port 3", + qdev: "/machine/peripheral/vdisk-vol-xxx/hotplug-ebs3/virtio-backend", want: 3, }, { @@ -313,6 +313,11 @@ func TestExtractHotplugPort(t *testing.T) { qdev: "/machine/peripheral-anon/device[0]/virtio-backend", want: -1, }, + { + name: "hotplug-eni port ignored (not a block device port)", + qdev: "/machine/peripheral/net-eni-1/hotplug-eni1/virtio-backend", + want: -1, + }, } for _, tt := range tests { diff --git a/spinifex/vm/lifecycle.go b/spinifex/vm/lifecycle.go index 8abf850e7..139d8ce40 100644 --- a/spinifex/vm/lifecycle.go +++ b/spinifex/vm/lifecycle.go @@ -14,6 +14,7 @@ import ( "syscall" "time" + "github.com/mulgadc/spinifex/spinifex/instancetypes" "github.com/mulgadc/spinifex/spinifex/qmp" "github.com/mulgadc/spinifex/spinifex/types" "github.com/mulgadc/spinifex/spinifex/utils" @@ -207,7 +208,8 @@ func (m *Manager) startQEMU(instance *VM) error { instance.Config.PIDFile = pidFile instance.Config.ConsoleLogPath = consoleLogPath } else { - instance.Config = buildBaseVMConfig(instance.ID, pidFile, consoleLogPath, serialSocket, spec.Architecture, instance.BootMode, spec.VCPUs, spec.MemoryMiB) + instance.Config = buildBaseVMConfig(instance.ID, instance.InstanceType, pidFile, consoleLogPath, serialSocket, spec.Architecture, instance.BootMode, spec.VCPUs, spec.MemoryMiB) + m.initENIRequests(instance) instance.EBSRequests.Mu.Lock() drives, iothreads, devices, err := buildDrives(instance.EBSRequests.Requests, spec.VCPUs, instance.Config.MachineType) @@ -633,11 +635,26 @@ func sendQMPCommand(q *qmp.QMPClient, cmd qmp.QMPCommand, instanceID string) (*q } } +// EBSHotPlugSlotCount is the fixed number of PCIe root ports pre-allocated +// for EBS hot-plug. Matches the /dev/sd[f-p] AWS device-letter range that +// AttachVolume targets. Cannot grow without QEMU restart (Linux PCIe +// hot-plug requires pre-allocated ports), so the count is deliberately +// equal to the AWS device-letter ceiling. +const EBSHotPlugSlotCount = 11 + // buildBaseVMConfig creates a vm.Config with base QEMU settings and PCIe // hotplug root ports. bootMode is the AMI's boot mode string ("bios" | "uefi" // | "uefi-preferred"); "uefi" and "uefi-preferred" flip cfg.UseUEFI. Any other // value (including "") defaults to BIOS. -func buildBaseVMConfig(instanceID, pidFile, consoleLogPath, serialSocket, architecture, bootMode string, vCPUs, memoryMiB int) Config { +// +// PCIe root ports are pre-allocated in two pools: +// +// - hotplug-ebs{1..EBSHotPlugSlotCount} on chassis 1..EBSHotPlugSlotCount +// for AttachVolume targets (/dev/sd[f-p]). +// - hotplug-eni{1..N} on chassis EBSHotPlugSlotCount+1.. where N is +// instancetypes.HotPlugENISlotsForType(instanceType) — the AWS-published +// ENI cap for the type minus the primary ENI, which is wired at boot. +func buildBaseVMConfig(instanceID, instanceType, pidFile, consoleLogPath, serialSocket, architecture, bootMode string, vCPUs, memoryMiB int) Config { cfg := Config{ Name: instanceID, PIDFile: pidFile, @@ -650,17 +667,45 @@ func buildBaseVMConfig(instanceID, pidFile, consoleLogPath, serialSocket, archit Memory: memoryMiB, CPUCount: vCPUs, Architecture: architecture, + InstanceType: instanceType, UseUEFI: bootMode == "uefi" || bootMode == "uefi-preferred", } - // 11 PCIe root ports for /dev/sd[f-p] hotplug slots, starting at chassis 1. - for i := 1; i <= 11; i++ { + + for i := 1; i <= EBSHotPlugSlotCount; i++ { cfg.Devices = append(cfg.Devices, Device{ - Value: fmt.Sprintf("pcie-root-port,id=hotplug%d,chassis=%d,slot=0", i, i), + Value: fmt.Sprintf("pcie-root-port,id=hotplug-ebs%d,chassis=%d,slot=0", i, i), }) } + + eniSlots := instancetypes.HotPlugENISlotsForType(instanceType) + for i := 1; i <= eniSlots; i++ { + chassis := EBSHotPlugSlotCount + i + cfg.Devices = append(cfg.Devices, Device{ + Value: fmt.Sprintf("pcie-root-port,id=hotplug-eni%d,chassis=%d,slot=0", i, chassis), + }) + } + return cfg } +// initENIRequests resets the per-VM ENI hot-plug slot allocator before the +// VM boots. The free-list mirrors the hotplug-eni{1..N} root ports emitted +// by buildBaseVMConfig for the instance type. AttachedByENIID is preserved +// across restart so the reconciler (Sprint 3d) can match KV truth against +// the in-memory map; on a cold boot it starts empty. +func (m *Manager) initENIRequests(instance *VM) { + eniSlots := instancetypes.HotPlugENISlotsForType(instance.InstanceType) + instance.ENIRequests.Mu.Lock() + defer instance.ENIRequests.Mu.Unlock() + instance.ENIRequests.AvailableSlots = make([]int, 0, eniSlots) + for i := 1; i <= eniSlots; i++ { + instance.ENIRequests.AvailableSlots = append(instance.ENIRequests.AvailableSlots, i) + } + if instance.ENIRequests.AttachedByENIID == nil { + instance.ENIRequests.AttachedByENIID = make(map[string]int) + } +} + // buildDrives converts EBS volume requests into QEMU drive, iothread, and // device configurations. Returns an error if any volume is missing its // NBDURI. EFI volumes are emitted as pflash unit=1 (per-VM EFI variable diff --git a/spinifex/vm/lifecycle_test.go b/spinifex/vm/lifecycle_test.go index e84a4dbfa..0c215ef2f 100644 --- a/spinifex/vm/lifecycle_test.go +++ b/spinifex/vm/lifecycle_test.go @@ -16,13 +16,16 @@ import ( ) // TestBuildBaseVMConfig pins the non-pass-through invariants buildBaseVMConfig -// must enforce: KVM + no-graphic, q35 + host CPU regardless of arch, and 11 +// must enforce: KVM + no-graphic, q35 + host CPU regardless of arch, and // pre-allocated PCIe root ports for hot-plug (Linux's PCIe hot-plug requires // pre-allocated ports — removing one silently breaks AttachVolume/AttachENI). +// +// Default instanceType ("") falls through to instancetypes.defaultMaxENIs (4) +// → 3 hot-plug-eni slots, alongside the fixed 11 hot-plug-ebs slots. func TestBuildBaseVMConfig(t *testing.T) { for _, arch := range []string{"x86_64", "arm64"} { t.Run(arch, func(t *testing.T) { - cfg := buildBaseVMConfig("i-x", "/tmp/x.pid", "/tmp/x.log", "/tmp/x.sock", arch, "", 2, 4096) + cfg := buildBaseVMConfig("i-x", "", "/tmp/x.pid", "/tmp/x.log", "/tmp/x.sock", arch, "", 2, 4096) assert.True(t, cfg.EnableKVM) assert.True(t, cfg.NoGraphic) @@ -30,11 +33,47 @@ func TestBuildBaseVMConfig(t *testing.T) { assert.Equal(t, "host", cfg.CPUType) assert.False(t, cfg.UseUEFI, "empty bootMode must default to BIOS") - require.Len(t, cfg.Devices, 11, "PCIe hot-plug requires 11 pre-allocated root ports") - for i, dev := range cfg.Devices { - expected := fmt.Sprintf("pcie-root-port,id=hotplug%d,chassis=%d,slot=0", i+1, i+1) - assert.Equal(t, expected, dev.Value) + // 11 EBS slots + 3 ENI slots (default fallback) = 14 root ports. + require.Len(t, cfg.Devices, 14) + for i := range EBSHotPlugSlotCount { + expected := fmt.Sprintf("pcie-root-port,id=hotplug-ebs%d,chassis=%d,slot=0", i+1, i+1) + assert.Equal(t, expected, cfg.Devices[i].Value) } + for i := range 3 { + idx := EBSHotPlugSlotCount + i + expected := fmt.Sprintf("pcie-root-port,id=hotplug-eni%d,chassis=%d,slot=0", i+1, EBSHotPlugSlotCount+i+1) + assert.Equal(t, expected, cfg.Devices[idx].Value) + } + }) + } +} + +// TestBuildBaseVMConfig_ENISlotCountPerType pins the per-instance-type ENI +// hot-plug pool sizing. The slot count must equal MaxENIs - 1; the chassis +// numbers must continue from EBSHotPlugSlotCount+1 without colliding. +func TestBuildBaseVMConfig_ENISlotCountPerType(t *testing.T) { + tests := []struct { + instanceType string + wantENISlots int + }{ + {"t3.nano", 2}, + {"t3a.2xlarge", 2}, + {"m5.large", 2}, + {"m5.2xlarge", 3}, + {"m5.4xlarge", 7}, + {"m5.8xlarge", 14}, + {"", 3}, // unknown → defaultMaxENIs (4) - 1 + } + for _, tc := range tests { + t.Run(tc.instanceType, func(t *testing.T) { + cfg := buildBaseVMConfig("i-x", tc.instanceType, "/tmp/x.pid", "/tmp/x.log", "/tmp/x.sock", "x86_64", "", 2, 4096) + require.Len(t, cfg.Devices, EBSHotPlugSlotCount+tc.wantENISlots) + for i := range tc.wantENISlots { + idx := EBSHotPlugSlotCount + i + expected := fmt.Sprintf("pcie-root-port,id=hotplug-eni%d,chassis=%d,slot=0", i+1, EBSHotPlugSlotCount+i+1) + assert.Equal(t, expected, cfg.Devices[idx].Value) + } + assert.Equal(t, tc.instanceType, cfg.InstanceType) }) } } @@ -55,7 +94,7 @@ func TestBuildBaseVMConfig_BootMode(t *testing.T) { } for _, tc := range tests { t.Run(tc.bootMode, func(t *testing.T) { - cfg := buildBaseVMConfig("i-x", "/tmp/x.pid", "/tmp/x.log", "/tmp/x.sock", "x86_64", tc.bootMode, 2, 4096) + cfg := buildBaseVMConfig("i-x", "", "/tmp/x.pid", "/tmp/x.log", "/tmp/x.sock", "x86_64", tc.bootMode, 2, 4096) assert.Equal(t, tc.wantUseUEFI, cfg.UseUEFI) }) } diff --git a/spinifex/vm/qmp_device.go b/spinifex/vm/qmp_device.go new file mode 100644 index 000000000..3804d17b1 --- /dev/null +++ b/spinifex/vm/qmp_device.go @@ -0,0 +1,246 @@ +package vm + +import ( + "fmt" + "maps" + "sync" + + "github.com/mulgadc/spinifex/spinifex/qmp" +) + +// DeviceController abstracts the QEMU Machine Protocol surface the ENI +// hot-plug pipeline uses: device_add / device_del / netdev_add / netdev_del / +// query-pci. Sprint 3a lands the interface and the in-memory stub so the +// downstream pipeline (Sprint 3b) can be unit-tested without a live QEMU. +// +// The production backend (QMPDeviceController) wraps the existing +// sendQMPCommand helper so wire-level serialization continues to live on the +// underlying *qmp.QMPClient mutex. The stub backend (StubDeviceController) +// keeps an in-memory map of attached devices/netdevs for assertions and lets +// tests pre-program failure responses per execute name. +type DeviceController interface { + DeviceAdd(args map[string]any) error + DeviceDel(deviceID string) error + NetdevAdd(args map[string]any) error + NetdevDel(netdevID string) error + QueryPCI() ([]PCIDevice, error) +} + +// PCIDevice is the trimmed projection of QEMU's query-pci response that the +// hot-plug pipeline consumes. Full response parsing lands in Sprint 3b; the +// stub backend populates QDevID only, which is the field the pipeline checks +// to confirm device materialization. +type PCIDevice struct { + Bus int `json:"bus"` + Slot int `json:"slot"` + QDevID string `json:"qdev_id,omitempty"` +} + +// QMPDeviceController is the production DeviceController bound to a live +// QEMU instance. Construct one per VM via NewQMPDeviceController and reuse +// it across the hot-plug / hot-unplug pipeline for the same VM. +type QMPDeviceController struct { + client *qmp.QMPClient + instanceID string +} + +var _ DeviceController = (*QMPDeviceController)(nil) + +func NewQMPDeviceController(client *qmp.QMPClient, instanceID string) *QMPDeviceController { + return &QMPDeviceController{client: client, instanceID: instanceID} +} + +func (c *QMPDeviceController) DeviceAdd(args map[string]any) error { + _, err := sendQMPCommand(c.client, qmp.QMPCommand{Execute: "device_add", Arguments: args}, c.instanceID) + return err +} + +func (c *QMPDeviceController) DeviceDel(deviceID string) error { + _, err := sendQMPCommand(c.client, qmp.QMPCommand{ + Execute: "device_del", + Arguments: map[string]any{"id": deviceID}, + }, c.instanceID) + return err +} + +func (c *QMPDeviceController) NetdevAdd(args map[string]any) error { + _, err := sendQMPCommand(c.client, qmp.QMPCommand{Execute: "netdev_add", Arguments: args}, c.instanceID) + return err +} + +func (c *QMPDeviceController) NetdevDel(netdevID string) error { + _, err := sendQMPCommand(c.client, qmp.QMPCommand{ + Execute: "netdev_del", + Arguments: map[string]any{"id": netdevID}, + }, c.instanceID) + return err +} + +// QueryPCI is unwired in Sprint 3a; the live query-pci response parser +// lands alongside the hot-plug pipeline in Sprint 3b. Callers (the stub +// backend below + future eni_hotplug.go) get the contract today. +func (c *QMPDeviceController) QueryPCI() ([]PCIDevice, error) { + return nil, fmt.Errorf("QueryPCI: live query-pci wiring lands in Sprint 3b (enihotplug)") +} + +// StubDeviceController is an in-memory DeviceController for unit tests. It +// records every call in order, enforces duplicate-id and unknown-id errors +// the way real QEMU does, and lets tests inject a one-shot failure per +// execute name via FailNext. +type StubDeviceController struct { + mu sync.Mutex + devices map[string]map[string]any + netdevs map[string]map[string]any + calls []StubCall + failNext map[string]error +} + +// StubCall records one DeviceController invocation. Args is a shallow copy +// of the args map passed in (or {"id": …} for the *Del methods) so tests +// can assert on payload without worrying about caller mutation. +type StubCall struct { + Execute string + Args map[string]any +} + +func NewStubDeviceController() *StubDeviceController { + return &StubDeviceController{ + devices: make(map[string]map[string]any), + netdevs: make(map[string]map[string]any), + failNext: make(map[string]error), + } +} + +var _ DeviceController = (*StubDeviceController)(nil) + +// SetFailNext primes the stub to return err on the next call matching +// execute (one of "device_add", "device_del", "netdev_add", "netdev_del", +// "query-pci"). Cleared after the call fires. +func (s *StubDeviceController) SetFailNext(execute string, err error) { + s.mu.Lock() + defer s.mu.Unlock() + s.failNext[execute] = err +} + +// Calls returns a snapshot of recorded calls in invocation order. Safe for +// concurrent use. +func (s *StubDeviceController) Calls() []StubCall { + s.mu.Lock() + defer s.mu.Unlock() + out := make([]StubCall, len(s.calls)) + copy(out, s.calls) + return out +} + +// HasDevice reports whether deviceID is currently attached in the stub's +// in-memory state. +func (s *StubDeviceController) HasDevice(deviceID string) bool { + s.mu.Lock() + defer s.mu.Unlock() + _, ok := s.devices[deviceID] + return ok +} + +// HasNetdev reports whether netdevID is currently attached in the stub's +// in-memory state. +func (s *StubDeviceController) HasNetdev(netdevID string) bool { + s.mu.Lock() + defer s.mu.Unlock() + _, ok := s.netdevs[netdevID] + return ok +} + +func (s *StubDeviceController) consumeFailure(execute string) error { + if err, ok := s.failNext[execute]; ok { + delete(s.failNext, execute) + return err + } + return nil +} + +func cloneArgs(in map[string]any) map[string]any { + if in == nil { + return nil + } + out := make(map[string]any, len(in)) + maps.Copy(out, in) + return out +} + +func (s *StubDeviceController) DeviceAdd(args map[string]any) error { + s.mu.Lock() + defer s.mu.Unlock() + s.calls = append(s.calls, StubCall{Execute: "device_add", Args: cloneArgs(args)}) + if err := s.consumeFailure("device_add"); err != nil { + return err + } + id, _ := args["id"].(string) + if id == "" { + return fmt.Errorf("stub device_add: missing id") + } + if _, exists := s.devices[id]; exists { + return fmt.Errorf("stub device_add: duplicate id %q", id) + } + s.devices[id] = cloneArgs(args) + return nil +} + +func (s *StubDeviceController) DeviceDel(deviceID string) error { + s.mu.Lock() + defer s.mu.Unlock() + s.calls = append(s.calls, StubCall{Execute: "device_del", Args: map[string]any{"id": deviceID}}) + if err := s.consumeFailure("device_del"); err != nil { + return err + } + if _, ok := s.devices[deviceID]; !ok { + return fmt.Errorf("stub device_del: device %q not found", deviceID) + } + delete(s.devices, deviceID) + return nil +} + +func (s *StubDeviceController) NetdevAdd(args map[string]any) error { + s.mu.Lock() + defer s.mu.Unlock() + s.calls = append(s.calls, StubCall{Execute: "netdev_add", Args: cloneArgs(args)}) + if err := s.consumeFailure("netdev_add"); err != nil { + return err + } + id, _ := args["id"].(string) + if id == "" { + return fmt.Errorf("stub netdev_add: missing id") + } + if _, exists := s.netdevs[id]; exists { + return fmt.Errorf("stub netdev_add: duplicate id %q", id) + } + s.netdevs[id] = cloneArgs(args) + return nil +} + +func (s *StubDeviceController) NetdevDel(netdevID string) error { + s.mu.Lock() + defer s.mu.Unlock() + s.calls = append(s.calls, StubCall{Execute: "netdev_del", Args: map[string]any{"id": netdevID}}) + if err := s.consumeFailure("netdev_del"); err != nil { + return err + } + if _, ok := s.netdevs[netdevID]; !ok { + return fmt.Errorf("stub netdev_del: netdev %q not found", netdevID) + } + delete(s.netdevs, netdevID) + return nil +} + +func (s *StubDeviceController) QueryPCI() ([]PCIDevice, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.calls = append(s.calls, StubCall{Execute: "query-pci"}) + if err := s.consumeFailure("query-pci"); err != nil { + return nil, err + } + out := make([]PCIDevice, 0, len(s.devices)) + for id := range s.devices { + out = append(out, PCIDevice{QDevID: id}) + } + return out, nil +} diff --git a/spinifex/vm/qmp_device_test.go b/spinifex/vm/qmp_device_test.go new file mode 100644 index 000000000..cb48d5d40 --- /dev/null +++ b/spinifex/vm/qmp_device_test.go @@ -0,0 +1,116 @@ +package vm + +import ( + "errors" + "testing" + + "github.com/mulgadc/spinifex/spinifex/qmp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestStubDeviceController_AddDelRoundTrip(t *testing.T) { + s := NewStubDeviceController() + + require.NoError(t, s.NetdevAdd(map[string]any{"id": "hostnet-eni-1", "type": "tap"})) + require.True(t, s.HasNetdev("hostnet-eni-1")) + + require.NoError(t, s.DeviceAdd(map[string]any{"id": "net-eni-1", "driver": "virtio-net-pci"})) + require.True(t, s.HasDevice("net-eni-1")) + + pci, err := s.QueryPCI() + require.NoError(t, err) + require.Len(t, pci, 1) + assert.Equal(t, "net-eni-1", pci[0].QDevID) + + require.NoError(t, s.DeviceDel("net-eni-1")) + require.False(t, s.HasDevice("net-eni-1")) + + require.NoError(t, s.NetdevDel("hostnet-eni-1")) + require.False(t, s.HasNetdev("hostnet-eni-1")) + + executes := []string{} + for _, c := range s.Calls() { + executes = append(executes, c.Execute) + } + assert.Equal(t, []string{ + "netdev_add", "device_add", "query-pci", "device_del", "netdev_del", + }, executes) +} + +func TestStubDeviceController_DuplicateAddRejected(t *testing.T) { + s := NewStubDeviceController() + require.NoError(t, s.DeviceAdd(map[string]any{"id": "net-eni-1"})) + + err := s.DeviceAdd(map[string]any{"id": "net-eni-1"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate id") + + require.NoError(t, s.NetdevAdd(map[string]any{"id": "hostnet-eni-1"})) + err = s.NetdevAdd(map[string]any{"id": "hostnet-eni-1"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate id") +} + +func TestStubDeviceController_MissingIDRejected(t *testing.T) { + s := NewStubDeviceController() + require.Error(t, s.DeviceAdd(map[string]any{"driver": "virtio-net-pci"})) + require.Error(t, s.NetdevAdd(map[string]any{"type": "tap"})) +} + +func TestStubDeviceController_DelUnknownRejected(t *testing.T) { + s := NewStubDeviceController() + require.Error(t, s.DeviceDel("net-eni-missing")) + require.Error(t, s.NetdevDel("hostnet-eni-missing")) +} + +func TestStubDeviceController_FailNextIsOneShot(t *testing.T) { + s := NewStubDeviceController() + injected := errors.New("simulated wire failure") + s.SetFailNext("device_add", injected) + + err := s.DeviceAdd(map[string]any{"id": "net-eni-1"}) + require.ErrorIs(t, err, injected) + assert.False(t, s.HasDevice("net-eni-1"), "failed DeviceAdd must not persist state") + + // One-shot: second call succeeds. + require.NoError(t, s.DeviceAdd(map[string]any{"id": "net-eni-1"})) + assert.True(t, s.HasDevice("net-eni-1")) +} + +func TestStubDeviceController_CallsSnapshotIsCopy(t *testing.T) { + s := NewStubDeviceController() + require.NoError(t, s.DeviceAdd(map[string]any{"id": "net-eni-1"})) + + snap := s.Calls() + snap[0].Execute = "mutated" + + again := s.Calls() + require.Len(t, again, 1) + assert.Equal(t, "device_add", again[0].Execute, "Calls() must return independent copies") +} + +func TestQMPDeviceController_QueryPCIUnwiredUntil3b(t *testing.T) { + c := NewQMPDeviceController(&qmp.QMPClient{}, "i-test") + _, err := c.QueryPCI() + require.Error(t, err) + assert.Contains(t, err.Error(), "Sprint 3b") +} + +func TestQMPDeviceController_DispatchesExpectedCommands(t *testing.T) { + rec := &qmpRecorder{} + client, cancel := newMockQMPClient(t, func(cmd qmp.QMPCommand) map[string]any { + rec.record(cmd) + return map[string]any{"return": map[string]any{}} + }) + defer cancel() + + c := NewQMPDeviceController(client, "i-test") + + require.NoError(t, c.NetdevAdd(map[string]any{"id": "hostnet-eni-1", "type": "tap"})) + require.NoError(t, c.DeviceAdd(map[string]any{"id": "net-eni-1", "driver": "virtio-net-pci"})) + require.NoError(t, c.DeviceDel("net-eni-1")) + require.NoError(t, c.NetdevDel("hostnet-eni-1")) + + assert.Equal(t, []string{"netdev_add", "device_add", "device_del", "netdev_del"}, rec.executes()) +} diff --git a/spinifex/vm/restore.go b/spinifex/vm/restore.go index 1e1370ef4..aef3790dd 100644 --- a/spinifex/vm/restore.go +++ b/spinifex/vm/restore.go @@ -88,6 +88,7 @@ func (m *Manager) classifyRestoredInstances() []*VM { for _, instance := range m.Snapshot() { instance.EBSRequests.Mu = sync.Mutex{} + instance.ENIRequests.Mu = sync.Mutex{} instance.QMPClient = &qmp.QMPClient{} if instance.Status == StateTerminated { diff --git a/spinifex/vm/vm.go b/spinifex/vm/vm.go index 208f92637..fa854c3d4 100644 --- a/spinifex/vm/vm.go +++ b/spinifex/vm/vm.go @@ -41,6 +41,7 @@ type VM struct { Config Config `json:"config"` EBSRequests types.EBSRequests `json:"ebs_requests"` + ENIRequests types.ENIRequests `json:"eni_requests"` QMPClient *qmp.QMPClient `json:"-"` @@ -158,6 +159,7 @@ func (v *VM) ResetNodeLocalState() { v.MetadataServerAddress = "" v.QMPClient = &qmp.QMPClient{} v.EBSRequests.Mu = sync.Mutex{} + v.ENIRequests.Mu = sync.Mutex{} } // IsTerminationProtected reports whether the instance was launched with diff --git a/spinifex/vm/volumes.go b/spinifex/vm/volumes.go index 7a567c230..dcfed50a7 100644 --- a/spinifex/vm/volumes.go +++ b/spinifex/vm/volumes.go @@ -119,12 +119,15 @@ func (m *Manager) AttachVolume(id, volumeID, device string) (AttachVolumeResult, return AttachVolumeResult{}, fmt.Errorf("QMP blockdev-add: %w", err) } - // /dev/sdf -> hotplug1, /dev/sdg -> hotplug2, etc. + // /dev/sdf -> hotplug-ebs1, /dev/sdg -> hotplug-ebs2, etc. The id prefix + // matches the pcie-root-port pre-allocation in buildBaseVMConfig; the + // "-ebs" suffix disambiguates these ports from hotplug-eni{N} (Sprint 3a + // ENI hot-plug pool). hotplugBus := "" if len(device) > 0 { letter := device[len(device)-1] if letter >= 'f' && letter <= 'p' { - hotplugBus = fmt.Sprintf("hotplug%d", letter-'f'+1) + hotplugBus = fmt.Sprintf("hotplug-ebs%d", letter-'f'+1) } }