diff --git a/spinifex/vm/devices.go b/spinifex/vm/devices.go index 80360189e..fe9616631 100644 --- a/spinifex/vm/devices.go +++ b/spinifex/vm/devices.go @@ -105,6 +105,11 @@ func HotplugEBSBus(port int) string { // AttachVolume's QMP device_add (rendered as a map) and buildDrives' -device // (rendered as a joined command-line string) so the two block-graph shapes // cannot diverge. +// +// werror/rerror mirror the boot drive's on-error policy (see Drive.Werror): +// a data volume has no -drive-backed BlockBackend to carry it, so the qdev +// device itself sets werror=report,rerror=report to report backend ENOSPC to +// the guest instead of letting QEMU's default werror=enospc pause the VM. func VolumeBlkDeviceArgs(volumeID, nodeName, iothreadID, bus string) []string { return []string{ fmt.Sprintf("id=%s", VolumeDeviceID(volumeID)), @@ -112,6 +117,8 @@ func VolumeBlkDeviceArgs(volumeID, nodeName, iothreadID, bus string) []string { fmt.Sprintf("iothread=%s", iothreadID), fmt.Sprintf("serial=%s", VolumeSerial(volumeID)), fmt.Sprintf("bus=%s", bus), + "werror=report", + "rerror=report", } } @@ -123,7 +130,8 @@ func VolumeBlkDevice(volumeID, nodeName, iothreadID, bus string) Device { } // VolumeBlkDeviceQMPArgs renders the same virtio-blk-pci arguments as a -// device_add argument map for AttachVolume's QMP hot-attach path. +// device_add argument map for AttachVolume's QMP hot-attach path. See +// VolumeBlkDeviceArgs for why werror/rerror are set here. func VolumeBlkDeviceQMPArgs(volumeID, nodeName, iothreadID, bus string) map[string]any { return map[string]any{ "driver": "virtio-blk-pci", @@ -132,6 +140,8 @@ func VolumeBlkDeviceQMPArgs(volumeID, nodeName, iothreadID, bus string) map[stri "iothread": iothreadID, "serial": VolumeSerial(volumeID), "bus": bus, + "werror": "report", + "rerror": "report", } } diff --git a/spinifex/vm/devices_test.go b/spinifex/vm/devices_test.go index 5c2619182..e22acb0fc 100644 --- a/spinifex/vm/devices_test.go +++ b/spinifex/vm/devices_test.go @@ -84,3 +84,20 @@ func TestRngDevice_MMIO_WithOptions(t *testing.T) { d := RngDevice("microvm,x-option-roms=off,pic=off") assert.Equal(t, "virtio-rng-device", d.Value) } + +// TestVolumeBlkDevice_OnErrorReport pins the cold-boot data-volume device +// string to werror=report,rerror=report, mirroring the boot drive's existing +// on-error policy so a full backend reports ENOSPC to the guest instead of +// pausing the VM. +func TestVolumeBlkDevice_OnErrorReport(t *testing.T) { + d := VolumeBlkDevice("vol-data-a", "nbd-vol-data-a", "ioth-vol-data-a", "hotplug-ebs3") + assert.Equal(t, "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report", d.Value) +} + +// TestVolumeBlkDeviceQMPArgs_OnErrorReport is the hotplug counterpart: the +// QMP device_add argument map AttachVolume sends must carry the same policy. +func TestVolumeBlkDeviceQMPArgs_OnErrorReport(t *testing.T) { + args := VolumeBlkDeviceQMPArgs("vol-data-a", "nbd-vol-data-a", "ioth-vol-data-a", "hotplug-ebs3") + assert.Equal(t, "report", args["werror"]) + assert.Equal(t, "report", args["rerror"]) +} diff --git a/spinifex/vm/lifecycle_test.go b/spinifex/vm/lifecycle_test.go index 063cd6070..30bafa8e8 100644 --- a/spinifex/vm/lifecycle_test.go +++ b/spinifex/vm/lifecycle_test.go @@ -171,7 +171,7 @@ func TestBuildDrives(t *testing.T) { {Value: "driver=nbd,node-name=nbd-vol-data-a,server.type=unix,server.path=/tmp/data-a.sock,export="}, }, wantDevices: []Device{ - {Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3"}, + {Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report"}, }, wantHotplugPorts: []int{3}, }, @@ -186,7 +186,7 @@ func TestBuildDrives(t *testing.T) { {Value: "driver=nbd,node-name=nbd-vol-data-b,server.type=unix,server.path=/tmp/data-b.sock,export="}, }, wantDevices: []Device{ - {Value: "virtio-blk-pci,id=vdisk-vol-data-b,drive=nbd-vol-data-b,iothread=ioth-vol-data-b,serial=voldatab,bus=hotplug-ebs1"}, + {Value: "virtio-blk-pci,id=vdisk-vol-data-b,drive=nbd-vol-data-b,iothread=ioth-vol-data-b,serial=voldatab,bus=hotplug-ebs1,werror=report,rerror=report"}, }, wantHotplugPorts: []int{1}, }, @@ -206,8 +206,8 @@ func TestBuildDrives(t *testing.T) { {Value: "driver=nbd,node-name=nbd-vol-data-d,server.type=unix,server.path=/tmp/data-d.sock,export="}, }, wantDevices: []Device{ - {Value: "virtio-blk-pci,id=vdisk-vol-data-c,drive=nbd-vol-data-c,iothread=ioth-vol-data-c,serial=voldatac,bus=hotplug-ebs1"}, - {Value: "virtio-blk-pci,id=vdisk-vol-data-d,drive=nbd-vol-data-d,iothread=ioth-vol-data-d,serial=voldatad,bus=hotplug-ebs2"}, + {Value: "virtio-blk-pci,id=vdisk-vol-data-c,drive=nbd-vol-data-c,iothread=ioth-vol-data-c,serial=voldatac,bus=hotplug-ebs1,werror=report,rerror=report"}, + {Value: "virtio-blk-pci,id=vdisk-vol-data-d,drive=nbd-vol-data-d,iothread=ioth-vol-data-d,serial=voldatad,bus=hotplug-ebs2,werror=report,rerror=report"}, }, wantHotplugPorts: []int{1, 2}, }, @@ -247,7 +247,7 @@ func TestBuildDrives(t *testing.T) { }, wantDevices: []Device{ {Value: "virtio-blk-pci,drive=os,iothread=ioth-os,num-queues=4,bootindex=1"}, - {Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3"}, + {Value: "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report"}, }, wantHotplugPorts: []int{0, 0, 3}, }, diff --git a/spinifex/vm/shutdown.go b/spinifex/vm/shutdown.go index 4dcdf5d2f..66b02d177 100644 --- a/spinifex/vm/shutdown.go +++ b/spinifex/vm/shutdown.go @@ -551,6 +551,10 @@ func (m *Manager) transitionWithPrecheck(instance *VM, target InstanceState) err return fmt.Errorf("%w: %s -> %s for instance %s (raced)", ErrInvalidTransition, current, target, instance.ID) } + // The in-memory status reached target even though persistence + // failed, so the stamp must still land: it is what lets the + // stuck-terminate backstop see and eventually bound this instance. + m.Inspect(instance, func(v *VM) { stampShuttingDownAt(v, target) }) return err } m.Inspect(instance, func(v *VM) { stampShuttingDownAt(v, target) }) diff --git a/spinifex/vm/shutdown_test.go b/spinifex/vm/shutdown_test.go index 25f5c163b..f1c9c1163 100644 --- a/spinifex/vm/shutdown_test.go +++ b/spinifex/vm/shutdown_test.go @@ -765,6 +765,59 @@ func TestTransitionWithPrecheck_PersistenceFailure_PassesThroughError(t *testing "persistence-failure branch implies status reached target before error returned") } +// TestTransitionWithPrecheck_PersistenceFailure_StampsShuttingDownAt covers +// the specific target that matters for the stuck-terminate backstop: when +// TransitionState fails but the in-memory status still reached +// StateShuttingDown, ShuttingDownAt must be stamped anyway so +// StuckTerminateReaper.Sweep can eventually see and bound this instance. +// Before the fix this never ran because the function returned before +// reaching the stamp call. +func TestTransitionWithPrecheck_PersistenceFailure_StampsShuttingDownAt(t *testing.T) { + persistErr := errors.New("no space left on device") + var m *Manager + m = NewManagerWithDeps(Deps{ + TransitionState: func(v *VM, target InstanceState) error { + // Memory mutation succeeded; only persistence (e.g. a full disk + // writing local state) failed. + m.Inspect(v, func(vv *VM) { vv.Status = target }) + return persistErr + }, + }) + + instance := &VM{ID: "i-persist-fail-shutdown", Status: StateRunning} + m.Insert(instance) + + err := m.transitionWithPrecheck(instance, StateShuttingDown) + + require.ErrorIs(t, err, persistErr, "the persistence error must still reach the caller") + assert.False(t, instance.ShuttingDownAt.IsZero(), + "ShuttingDownAt must be stamped even though the state write failed, so the reaper can see this instance") +} + +// TestTransitionWithPrecheck_Raced_DoesNotStampShuttingDownAt is the +// contrast: when TransitionState fails AND the in-memory status did not +// reach StateShuttingDown (a concurrent transition beat this call), the +// target state was never actually entered, so stamping would be a lie. +func TestTransitionWithPrecheck_Raced_DoesNotStampShuttingDownAt(t *testing.T) { + persistErr := errors.New("kv put failed") + m := NewManagerWithDeps(Deps{ + TransitionState: func(_ *VM, _ InstanceState) error { + // Return error without mutating status, matching a concurrent + // transition that beat this call. + return persistErr + }, + }) + + instance := &VM{ID: "i-raced-shutdown", Status: StateRunning} + m.Insert(instance) + + err := m.transitionWithPrecheck(instance, StateShuttingDown) + + require.ErrorIs(t, err, ErrInvalidTransition) + assert.True(t, instance.ShuttingDownAt.IsZero(), + "the raced branch never actually entered StateShuttingDown, so it must not stamp") +} + // TestTransitionWithPrecheck_InvalidInitialTransition_WrapsErrInvalidTransition // covers the static precheck rejecting an illegal transition before // TransitionState is ever called. The error must wrap diff --git a/spinifex/vm/stuck_terminate_reaper_test.go b/spinifex/vm/stuck_terminate_reaper_test.go index 921955b5e..77845e294 100644 --- a/spinifex/vm/stuck_terminate_reaper_test.go +++ b/spinifex/vm/stuck_terminate_reaper_test.go @@ -101,6 +101,58 @@ func TestStuckTerminateReaper(t *testing.T) { assert.True(t, ok, "the instance must stay in the running map") }) + t.Run("force-completes a VM whose ShuttingDownAt was only stamped by the persistence-failure path", func(t *testing.T) { + // This is the case the backstop existed to cover: on a full disk the + // state write fails, but transitionWithPrecheck must still stamp + // ShuttingDownAt so this VM is not skipped forever. + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + cleaner := &recordingInstanceCleaner{} + store := newFakeStateStore() + persistErr := errors.New("no space left on device") + var callCount int + var m *Manager + m = NewManagerWithDeps(Deps{ + NodeID: "test-node", + StateStore: store, + InstanceCleaner: cleaner, + // Only the first call (the shutting-down transition below) fails, + // modelling the disk being full at that moment; the reaper's own + // later transition to terminated succeeds normally. + TransitionState: func(v *VM, target InstanceState) error { + m.Inspect(v, func(vv *VM) { vv.Status = target }) + callCount++ + if callCount == 1 { + return persistErr + } + return nil + }, + }) + + const id = "i-wedged-persist-fail" + instance := &VM{ID: id, Status: StateRunning} + m.Insert(instance) + + err := m.transitionWithPrecheck(instance, StateShuttingDown) + require.ErrorIs(t, err, persistErr, "the persistence error must still reach the caller") + require.False(t, instance.ShuttingDownAt.IsZero(), + "the failure path must stamp ShuttingDownAt for the reaper to ever see this instance") + + // Back-date the stamp instead of sleeping so the reaper sees a + // timed-out VM without a real wait. + m.Inspect(instance, func(v *VM) { + v.ShuttingDownAt = time.Now().Add(-(stuckTerminateTimeout + time.Minute)) + }) + + reaper := m.NewStuckTerminateReaper() + reaped, err := reaper.Sweep(context.Background()) + require.NoError(t, err) + assert.Equal(t, 1, reaped, + "a VM stamped only via the persistence-failure path must still be force-completed once past the timeout") + + _, ok := m.Get(id) + assert.False(t, ok, "the finalized instance must leave the local running map") + }) + t.Run("a shutting-down instance with no timestamp is never force-completed", func(t *testing.T) { cleaner := &recordingInstanceCleaner{} reaper, _ := newStuckTerminateReaper(t, cleaner) diff --git a/spinifex/vm/vm_test.go b/spinifex/vm/vm_test.go index 559af39cc..9b5799f4c 100644 --- a/spinifex/vm/vm_test.go +++ b/spinifex/vm/vm_test.go @@ -266,7 +266,7 @@ func TestExecute_Blockdev_UnixServer(t *testing.T) { } require.Greater(t, deviceIdx, -1) assert.Greater(t, deviceIdx, blockdevIdx, "-blockdev must precede the -device referencing it") - assert.Equal(t, "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3", args[deviceIdx+1]) + assert.Equal(t, "virtio-blk-pci,id=vdisk-vol-data-a,drive=nbd-vol-data-a,iothread=ioth-vol-data-a,serial=voldataa,bus=hotplug-ebs3,werror=report,rerror=report", args[deviceIdx+1]) } // TestExecute_Blockdev_InetServer asserts the TCP NBD form renders