fix: preserve running guests across target restart, harden state write (#654)

* fix: preserve running guests across target restart, harden state write spinifex-shutdown.service's ExecStop fired on any spinifex.target stop, including the stop half of `systemctl restart spinifex.target`, draining every guest via QMP even though the host never went down. Gate the drain behind `systemctl is-system-running` == "stopping", which only holds once systemd is genuinely unwinding into shutdown.target (reboot/poweroff), so a target restart or plain `systemctl stop` now leaves QEMU/nbdkit running (KillMode=process) for the daemon to reconnect to on the way back up. Daemon.WriteState returned as soon as the local state file write failed, skipping the independent JetStream KV write entirely. A crash in that window left a live VM with no durable record anywhere. Attempt the KV write regardless of local disk health, then still surface the local write error to callers, mirroring finalizeTerminated's handling of the same fault class on the terminate path. * fix: gate the shutdown drain via an spx flag, not a shell script Replace the shutdown-drain-gate.sh gate with a --only-if-host-stopping flag on `spx admin node drain`. The unit now points at /usr/local/bin/spx directly (already required for the node to function) instead of a script installed under the data directory, which a partial rebuild of that directory could silently drop without ever failing ExecStop. The gate logic also becomes a plain Go function with injectable state lookup, so it gets real table-driven coverage instead of only a live reboot. hostIsStopping() drains only on a literal "stopping" from `systemctl is-system-running`. Every other recognized state (running/degraded/...) and any unrecognized output skip the drain; a fully unreachable systemctl fails toward draining, since a skipped drain on a real shutdown hard-kills guests and loses unflushed writes, while a spurious drain on a restart only costs an extra graceful stop/relaunch cycle. Run by hand without the flag, the command still drains unconditionally as it always has. * refactor: split the shutdown-gate comments to sit with the code they explain
2026-10-20 19:25:52 +00:00 · 2026-08-02 21:28:55 +10:00
parent 22171c44ab
commit 031f0a4cdd
7 changed files with 148 additions and 13 deletions
@@ -9,9 +9,13 @@ After=spinifex-nats.service spinifex-predastore.service spinifex-viperblock.serv
 Type=oneshot
 RemainAfterExit=yes
 ExecStart=/bin/true
-# ExecStop only runs when the target/host goes down, not on a single-service
-# restart — so a daemon restart still leaves guests running (DDIL).
-ExecStop=/usr/local/bin/spx admin node drain --local --timeout=120s
+# ExecStop fires on ANY target stop, including the stop half of `systemctl
+# restart spinifex.target` (a rolling deploy) -- PartOf propagates regardless
+# of whether the host is actually going down. --only-if-host-stopping drains
+# only when systemd itself is unwinding into shutdown.target; a plain restart
+# or `systemctl stop spinifex.target` falls through so QEMU/nbdkit survive
+# (KillMode=process) and the daemon reconnects to them on the way back up.
+ExecStop=/usr/local/bin/spx admin node drain --local --timeout=120s --only-if-host-stopping
 TimeoutStopSec=180

 [Install]
@@ -117,8 +117,10 @@ var nodeDrainCmd = &cobra.Command{
 	Long: `Run the GATE and DRAIN shutdown phases against the local node only: power down
 its guests via QMP and unmount their volumes (flushing the viperblock WAL) while
 every service is still running. STORAGE/PERSIST/INFRA are left to systemd's
-ordered unit teardown. This is the ExecStop of spinifex-shutdown.service, so a
-systemctl stop or host reboot drains guests before any storage service stops.`,
+ordered unit teardown. This is what spinifex-shutdown.service's ExecStop runs,
+with --only-if-host-stopping so it only drains on a genuine host
+shutdown/reboot -- not on a plain "systemctl restart/stop spinifex.target".
+Run by hand without that flag, it always drains unconditionally.`,
 	Run: runNodeDrainLocal,
 }

@@ -281,6 +283,7 @@ func init() {
 	nodeCmd.AddCommand(nodeDrainCmd)
 	nodeDrainCmd.Flags().Bool("local", false, "Drain the local node only (required)")
 	nodeDrainCmd.Flags().Duration("timeout", 120*time.Second, "Maximum time to wait per phase")
+	nodeDrainCmd.Flags().Bool("only-if-host-stopping", false, "Skip the drain unless systemd is unwinding into shutdown.target (real reboot/poweroff); unset, always drains")
 	nodeCmd.AddCommand(nodeJSProbeCmd)
 	nodeJSProbeCmd.Flags().Duration("timeout", 10*time.Second, "Maximum time to wait for the canary round-trip")

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"os/exec"
 	"strings"
 	"time"

@@ -13,6 +14,42 @@ import (
 	"github.com/spf13/cobra"
 )

+// systemIsSystemRunning is overridable in tests so hostIsStopping does not
+// need a real systemd. systemctl exits non-zero for every state other than
+// "running", so callers must inspect the output, not just the error.
+var systemIsSystemRunning = func() (string, error) {
+	out, err := exec.Command("systemctl", "is-system-running").Output()
+	return strings.TrimSpace(string(out)), err
+}
+
+// knownSystemStates are the documented systemctl is-system-running values.
+var knownSystemStates = map[string]bool{
+	"running": true, "degraded": true, "maintenance": true,
+	"stopping": true, "initializing": true, "starting": true, "offline": true,
+}
+
+// hostIsStopping reports whether systemd is genuinely unwinding into
+// shutdown.target (reboot/poweroff/halt/kexec), as opposed to a plain
+// `systemctl restart/stop spinifex.target` where the host stays up.
+func hostIsStopping() (bool, error) {
+	out, err := systemIsSystemRunning()
+
+	// systemctl did not run at all, so the state is genuinely unknown. Fail
+	// toward draining: a skipped drain on a real shutdown hard-kills guests,
+	// while a spurious drain only costs a graceful stop/relaunch cycle.
+	if out == "" {
+		return true, fmt.Errorf("systemctl is-system-running produced no output: %w", err)
+	}
+
+	// Unrecognized output is untrusted; an unknown state must never be
+	// mistaken for shutdown.
+	if !knownSystemStates[out] {
+		return false, fmt.Errorf("systemctl is-system-running returned unrecognized state %q", out)
+	}
+
+	return out == "stopping", nil
+}
+
 // runClusterShutdown orchestrates a phased, coordinated shutdown of the cluster.
 func runClusterShutdown(cmd *cobra.Command, args []string) {
 	force, _ := cmd.Flags().GetBool("force")
@@ -169,11 +206,26 @@ func runClusterShutdown(cmd *cobra.Command, args []string) {
 func runNodeDrainLocal(cmd *cobra.Command, args []string) {
 	local, _ := cmd.Flags().GetBool("local")
 	timeout, _ := cmd.Flags().GetDuration("timeout")
+	onlyIfHostStopping, _ := cmd.Flags().GetBool("only-if-host-stopping")
 	if !local {
 		fmt.Fprintln(os.Stderr, "Error: node drain currently supports only --local")
 		os.Exit(1)
 	}

+	// Unset (an operator running this by hand), the command drains
+	// unconditionally. Set, it skips anything short of a real host shutdown,
+	// since PartOf=spinifex.target fires ExecStop on restarts too.
+	if onlyIfHostStopping {
+		stopping, err := hostIsStopping()
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: could not determine host shutdown state: %v\n", err)
+		}
+		if !stopping {
+			fmt.Println("Host is not shutting down; skipping guest drain.")
+			return
+		}
+	}
+
 	cfg, nc, err := loadConfigAndConnect()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
@@ -2,6 +2,7 @@ package cmd

 import (
 	"encoding/json"
+	"errors"
 	"testing"
 	"time"

@@ -56,3 +57,39 @@ func TestDrainDHCPLeasesNoResponders(t *testing.T) {
 	assert.Equal(t, 0, released)
 	assert.Equal(t, 0, responders)
 }
+
+// TestHostIsStopping covers every systemctl is-system-running outcome the
+// shutdown-drain gate must distinguish: a real shutdown/reboot ("stopping"),
+// ordinary steady states that must never trigger a drain, unrecognized
+// output, and a fully unreachable systemctl.
+func TestHostIsStopping(t *testing.T) {
+	orig := systemIsSystemRunning
+	t.Cleanup(func() { systemIsSystemRunning = orig })
+
+	cases := []struct {
+		name         string
+		out          string
+		err          error
+		wantStopping bool
+		wantErr      bool
+	}{
+		{"stopping", "stopping", errors.New("exit status 1"), true, false},
+		{"running", "running", nil, false, false},
+		{"degraded", "degraded", errors.New("exit status 1"), false, false},
+		{"maintenance", "maintenance", errors.New("exit status 1"), false, false},
+		{"unknown output", "banana", nil, false, true},
+		{"systemctl unreachable", "", errors.New("exec: \"systemctl\": executable file not found in $PATH"), true, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			systemIsSystemRunning = func() (string, error) { return tc.out, tc.err }
+			stopping, err := hostIsStopping()
+			assert.Equal(t, tc.wantStopping, stopping)
+			if tc.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
@@ -2296,17 +2296,25 @@ func (d *Daemon) WriteState() error {
 		return fmt.Errorf("marshal state: %w", marshalErr)
 	}

+	// The KV write is independent of local disk health (JetStream, not this
+	// disk), so a local failure must not skip it — attempt both, then report
+	// the local failure. Revision only advances when the local write (the
+	// source of truth Revision() describes) actually landed.
 	path := d.localStatePath()
-	if err := WriteLocalStateBytes(path, localData); err != nil {
-		slog.Error("Local state write failed", "path", path, "error", err)
-		return fmt.Errorf("write local state: %w", err)
+	localErr := WriteLocalStateBytes(path, localData)
+	if localErr != nil {
+		slog.Error("Local state write failed", "path", path, "error", localErr)
+	} else {
+		d.stateRevision.Add(1)
 	}
-	d.stateRevision.Add(1)

 	if d.jsManager != nil {
 		d.jsManager.WriteStateBytesBestEffort(d.node, kvData, kvSyncTimeout)
 	}

+	if localErr != nil {
+		return fmt.Errorf("write local state: %w", localErr)
+	}
 	return nil
 }

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"os"
 	"path/filepath"
 	"sync"
 	"testing"
@@ -926,6 +927,35 @@ func TestStatePersistence_RoundTrip(t *testing.T) {
 	assert.Equal(t, original.Attributes.StopInstance, loaded.Attributes.StopInstance)
 }

+// TestWriteState_LocalFailureStillWritesKV guards against a launch-window data
+// loss: before the fix, WriteState returned the instant the local file write
+// failed, so the independent JetStream KV write was never attempted. A crash
+// right after would leave a live VM with no durable record anywhere. The local
+// write is forced to fail deterministically (a regular file blocking the state
+// dir) rather than relying on a real disk fault, which is not reproducible here.
+func TestWriteState_LocalFailureStillWritesKV(t *testing.T) {
+	daemon := createDaemonWithJetStream(t)
+
+	obs := &fakeKVObserver{}
+	daemon.jsManager.SetSyncObserver(obs)
+
+	daemon.vmMgr.Insert(&vm.VM{ID: "i-localfault", Status: vm.StateRunning, InstanceType: "t3.micro"})
+
+	// Block the local state directory with a regular file so WriteLocalStateBytes's
+	// MkdirAll fails deterministically, without needing root or a real disk fault.
+	blocker := filepath.Join(daemon.config.DataDir, DefaultLocalStateDir)
+	require.NoError(t, os.WriteFile(blocker, []byte("not a directory"), 0o600))
+
+	err := daemon.WriteState()
+	require.Error(t, err, "WriteState must still surface the local write failure")
+
+	successes, failures := obs.snapshot()
+	assert.Empty(t, failures)
+	assert.Len(t, successes, 1, "KV write must be attempted even when the local write fails")
+	assert.Equal(t, uint64(0), daemon.Revision(),
+		"revision tracks local persistence and must not advance on a local failure")
+}
+
 // TestRestoreInstances_StoppedInstanceMigratedToSharedKV verifies that after
 // a daemon restart, a stopped instance is migrated to shared KV and can be
 // retrieved by the ec2.start handler.
@@ -194,8 +194,9 @@ func TestOptionalNorthstarActivation(t *testing.T) {
 // TestGracefulDrainOrdering asserts the graceful-shutdown contract: the drain
 // oneshot orders After= the storage/daemon units (so a target/host stop runs its
 // ExecStop drain first, while those services are still up), the daemon keeps
-// KillMode=process (guests survive a daemon restart — DDIL reattach), and the
-// drain is wired into the target so a target/host stop triggers it.
+// KillMode=process (guests survive a daemon restart — DDIL reattach), and
+// ExecStop passes --only-if-host-stopping so PartOf=spinifex.target firing on
+// every target stop (restart included) does not drain guests that stay up.
 func TestGracefulDrainOrdering(t *testing.T) {
 	dir := unitsDir(t)

@@ -219,8 +220,8 @@ func TestGracefulDrainOrdering(t *testing.T) {
 			t.Errorf("spinifex-shutdown.service After= must include %s so the drain stops before it", dep)
 		}
 	}
-	if !hasDirective(drain, "ExecStop=/usr/local/bin/spx admin node drain --local --timeout=120s") {
-		t.Error("spinifex-shutdown.service must drain the local node on stop via ExecStop")
+	if !hasDirective(drain, "ExecStop=/usr/local/bin/spx admin node drain --local --timeout=120s --only-if-host-stopping") {
+		t.Error("spinifex-shutdown.service must run spx admin node drain --local --only-if-host-stopping on stop via ExecStop")
 	}

 	daemon := readUnit(t, dir, "spinifex-daemon.service")