diff --git a/build/systemd/spinifex-shutdown.service b/build/systemd/spinifex-shutdown.service index f4a3f2197..7099be5aa 100644 --- a/build/systemd/spinifex-shutdown.service +++ b/build/systemd/spinifex-shutdown.service @@ -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] diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index bc91f0c8d..a50284fa9 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -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") diff --git a/cmd/spinifex/cmd/cluster.go b/cmd/spinifex/cmd/cluster.go index 7e3825f94..8eeb93d19 100644 --- a/cmd/spinifex/cmd/cluster.go +++ b/cmd/spinifex/cmd/cluster.go @@ -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) diff --git a/cmd/spinifex/cmd/cluster_test.go b/cmd/spinifex/cmd/cluster_test.go index 2c4034fd6..72d55c050 100644 --- a/cmd/spinifex/cmd/cluster_test.go +++ b/cmd/spinifex/cmd/cluster_test.go @@ -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) + } + }) + } +} diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 115c2ef30..76186faff 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -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 } diff --git a/spinifex/daemon/state_test.go b/spinifex/daemon/state_test.go index a8370b334..45f6ba1a1 100644 --- a/spinifex/daemon/state_test.go +++ b/spinifex/daemon/state_test.go @@ -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. diff --git a/spinifex/systemd/units_invariants_test.go b/spinifex/systemd/units_invariants_test.go index 343adc9f9..6467c1fa9 100644 --- a/spinifex/systemd/units_invariants_test.go +++ b/spinifex/systemd/units_invariants_test.go @@ -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")