diff --git a/build/systemd/spinifex-viperblock.service b/build/systemd/spinifex-viperblock.service index c262b23d2..3306e9616 100644 --- a/build/systemd/spinifex-viperblock.service +++ b/build/systemd/spinifex-viperblock.service @@ -1,4 +1,4 @@ -# spinifex-unit-version: 1 +# spinifex-unit-version: 2 [Unit] Description=Spinifex Viperblock (Block Storage) PartOf=spinifex.target @@ -51,7 +51,8 @@ ProtectKernelTunables=yes ProtectKernelModules=yes ProtectKernelLogs=yes ProtectControlGroups=yes -ProtectProc=invisible +# ProtectProc must stay default: hidepid=invisible hides spinifex-daemon's +# QEMU /proc entries (different uid) orphan nbdkit reaping must see. SystemCallArchitectures=native MemoryDenyWriteExecute=yes RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK diff --git a/spinifex/services/viperblockd/nbdkit_recovery.go b/spinifex/services/viperblockd/nbdkit_recovery.go index e9e6c42b2..899bc4cb6 100644 --- a/spinifex/services/viperblockd/nbdkit_recovery.go +++ b/spinifex/services/viperblockd/nbdkit_recovery.go @@ -2,7 +2,8 @@ package viperblockd // Rebuilds cfg.MountedVolumes from nbdkit processes that survived a // viperblockd restart, so findMountedVolume can find them again (see -// orphan_scan.go for the equivalent problem/fix on the QEMU side). +// orphan_scan.go for the equivalent problem/fix on the QEMU side), and reaps +// the two classes of surviving nbdkit that recovery itself cannot adopt. import ( "bytes" @@ -11,8 +12,10 @@ import ( "log/slog" "os" "path/filepath" + "slices" "strconv" "strings" + "syscall" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/nats-io/nats.go" @@ -170,10 +173,11 @@ func recoverMountedVolumes(ctx context.Context, cfg *Config, nc *nats.Conn, proc } // Two live nbdkits for one volume is the double-mount hazard itself: - // adopt the first and leave the second visible in the log. + // adopt the first and reap the rest if nothing is using them. if _, ok := findMountedVolume(cfg, disc.Volume); ok { slog.Warn("recovery: volume already recovered from another nbdkit process, skipping", "pid", disc.PID, "volume", disc.Volume) + reapOrphanedNbdkit(procRoot, disc, reapReasonDuplicate) continue } @@ -181,6 +185,7 @@ func recoverMountedVolumes(ctx context.Context, cfg *Config, nc *nats.Conn, proc if err != nil { slog.Error("recovery: failed to rebuild mounted volume registry entry, skipping", "pid", disc.PID, "volume", disc.Volume, "err", err) + reapOrphanedNbdkit(procRoot, disc, reapReasonUnadoptable) continue } @@ -192,3 +197,164 @@ func recoverMountedVolumes(ctx context.Context, cfg *Config, nc *nats.Conn, proc "pid", mv.PID, "volume", mv.Name, "socket", mv.Socket, "port", mv.Port) } } + +// reapReason names which of recovery's two failure-to-adopt paths produced +// an unclaimed nbdkit process, carried only for logging. +type reapReason string + +const ( + reapReasonUnadoptable reapReason = "unadoptable" + reapReasonDuplicate reapReason = "duplicate" +) + +// reapOrphanedNbdkit decides whether disc -- a corroborated nbdkit process +// recovery could not fold into MountedVolumes -- may be safely signalled. +// Reaping the wrong process destroys a running guest's data path, so disc is +// only ever signalled once a scan of every other process on the host finds no +// reference to its NBD endpoint. A referencing process, or a scan that could +// not complete, both leave it running: the caller gets a loud log either way. +func reapOrphanedNbdkit(procRoot string, disc discoveredNbdkit, reason reapReason) { + if hidden, herr := procHidepidInvisible(procRoot); hidden { + slog.Warn("recovery: proc mount hides other users' process entries; a live referencer could be invisible to this scan, leaving unclaimed nbdkit running", + "pid", disc.PID, "volume", disc.Volume, "reason", reason, "err", herr) + return + } + + referencingPID, referenced, err := endpointReferencer(procRoot, disc) + if err != nil { + slog.Warn("recovery: could not scan for processes using an unclaimed nbdkit's endpoint, leaving it running", + "pid", disc.PID, "volume", disc.Volume, "reason", reason, "err", err) + return + } + if referenced { + slog.Error("recovery: unclaimed nbdkit process is still referenced by a live process; left running deliberately, needs operator attention", + "pid", disc.PID, "volume", disc.Volume, "reason", reason, "referencing_pid", referencingPID) + return + } + + proc, err := os.FindProcess(disc.PID) + if err != nil { + slog.Error("recovery: failed to locate unclaimed nbdkit process to reap", + "pid", disc.PID, "volume", disc.Volume, "reason", reason, "err", err) + return + } + if err := proc.Signal(syscall.SIGTERM); err != nil { + slog.Error("recovery: failed to signal unclaimed nbdkit process", + "pid", disc.PID, "volume", disc.Volume, "reason", reason, "err", err) + return + } + + if disc.Socket != "" { + if err := os.Remove(disc.Socket); err != nil && !os.IsNotExist(err) { + slog.Warn("recovery: failed to remove socket file for reaped nbdkit process", + "pid", disc.PID, "volume", disc.Volume, "socket", disc.Socket, "err", err) + } + } + + slog.Info("recovery: reaped unclaimed nbdkit process with no live referencer", + "pid", disc.PID, "volume", disc.Volume, "reason", reason) +} + +// procHidepidInvisible reports whether procRoot's mount hides another uid's +// /proc/ entry outright (hidepid=invisible/2), so a referencer never +// reaches os.ReadDir. Missing mountinfo reports false; any other read error cannot be ruled out. +func procHidepidInvisible(procRoot string) (bool, error) { + data, err := os.ReadFile(filepath.Join(procRoot, "self", "mountinfo")) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return true, err + } + + for line := range strings.SplitSeq(string(data), "\n") { + fields := strings.Fields(line) + sep := slices.Index(fields, "-") + if sep < 0 || sep+3 >= len(fields) || len(fields) < 6 { + continue + } + if fields[4] != "/proc" || fields[sep+1] != "proc" { + continue + } + opts := fields[5] + "," + fields[sep+3] + if strings.Contains(opts, "hidepid=invisible") || strings.Contains(opts, "hidepid=2") { + return true, nil + } + } + return false, nil +} + +// endpointReferencer reports whether any process other than disc's own PID +// has disc's NBD endpoint on its own /proc//cmdline: the socket path for +// a unix mount, or the 127.0.0.1: pair for a TCP one. That is how a +// guest's QEMU process is handed its drive backend at launch, and it is the +// only positive evidence available that an unclaimed nbdkit still serves a +// live guest. A non-nil error means the scan itself could not be completed, +// not that nothing was found -- callers must treat that as unknown, not safe. +func endpointReferencer(procRoot string, disc discoveredNbdkit) (pid int, found bool, err error) { + entries, err := os.ReadDir(procRoot) + if err != nil { + return 0, false, err + } + + for _, entry := range entries { + candidate, cerr := strconv.Atoi(entry.Name()) + if cerr != nil || candidate <= 0 || candidate == disc.PID { + continue + } + + cmdline, rerr := os.ReadFile(filepath.Join(procRoot, entry.Name(), "cmdline")) + if rerr != nil { + // The process exited between ReadDir and this read: genuinely + // not a referencer. Any other error (EACCES under a hidepid + // /proc, most importantly) means the scan cannot rule this PID + // out, so it must not be silently read as absence of evidence. + if os.IsNotExist(rerr) { + continue + } + return 0, false, rerr + } + + if cmdlineReferencesEndpoint(cmdline, disc) { + return candidate, true, nil + } + } + return 0, false, nil +} + +// cmdlineReferencesEndpoint checks a NUL-separated /proc//cmdline for +// disc's endpoint. A socket mount's path appears verbatim in whichever +// argument named it (QEMU's nbd+unix:///?socket= file= or its +// server.path= -blockdev option). A TCP mount's 127.0.0.1: pair +// may appear joined in one argument (a boot/EFI drive's nbd://host:port) or +// split across server.host=/server.port= within the same comma-joined +// -blockdev argument, so both shapes are checked. +func cmdlineReferencesEndpoint(cmdline []byte, disc discoveredNbdkit) bool { + args := bytes.Split(bytes.TrimRight(cmdline, "\x00"), []byte{0}) + + if disc.Socket != "" { + needle := []byte(disc.Socket) + for _, arg := range args { + if bytes.Contains(arg, needle) { + return true + } + } + return false + } + + if disc.Port <= 0 { + return false + } + joined := fmt.Appendf(nil, "127.0.0.1:%d", disc.Port) + serverPort := fmt.Appendf(nil, "server.port=%d", disc.Port) + serverHost := []byte("server.host=127.0.0.1") + for _, arg := range args { + if bytes.Contains(arg, joined) { + return true + } + if bytes.Contains(arg, serverPort) && bytes.Contains(arg, serverHost) { + return true + } + } + return false +} diff --git a/spinifex/services/viperblockd/nbdkit_recovery_test.go b/spinifex/services/viperblockd/nbdkit_recovery_test.go index 2538b8844..1cccb9dba 100644 --- a/spinifex/services/viperblockd/nbdkit_recovery_test.go +++ b/spinifex/services/viperblockd/nbdkit_recovery_test.go @@ -10,6 +10,7 @@ import ( "fmt" "net" "os" + "os/exec" "path/filepath" "strconv" "strings" @@ -17,6 +18,7 @@ import ( "time" "github.com/mulgadc/spinifex/spinifex/ebsprovider" + "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/viperblock/viperblock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -66,6 +68,34 @@ func writeProcFixture(t *testing.T, procRoot string, pid int, comm string, argv require.NoError(t, os.WriteFile(filepath.Join(dir, "cmdline"), []byte(strings.Join(argv, "\x00")+"\x00"), 0644)) } +// spawnLongRunningProcess starts a real, short-lived-by-signal process so +// reap tests can send it a genuine SIGTERM and observe a genuine exit, +// instead of pretending a fabricated /proc entry is a live process (SIGTERM +// only ever reaches the real kernel process table, never procRoot). +func spawnLongRunningProcess(t *testing.T) int { + t.Helper() + cmd := exec.Command("sleep", "60") + require.NoError(t, cmd.Start()) + pid := cmd.Process.Pid + done := make(chan struct{}) + go func() { _ = cmd.Wait(); close(done) }() + t.Cleanup(func() { + _ = cmd.Process.Kill() + <-done + }) + return pid +} + +// writeReferencerFixture writes a /proc//{comm,cmdline} pair for a +// process whose argv carries volumeSocket's endpoint the way QEMU's -blockdev +// does, so endpointReferencer has something positive to find. comm is +// cosmetic only -- endpointReferencer does not filter by it, matching the +// "any process on the host may be a legitimate referencer" fail-safe design. +func writeReferencerFixture(t *testing.T, procRoot string, pid int, blockdevArg string) { + t.Helper() + writeProcFixture(t, procRoot, pid, "qemu-system-x86_64", []string{"qemu-system-x86_64", "-blockdev", blockdevArg}) +} + // --- argv parsing --- func TestParseNbdkitCmdline_SocketTransport(t *testing.T) { @@ -330,3 +360,324 @@ func TestRecoverMountedVolumes_DuplicateVolumeAdoptedOnce(t *testing.T) { } assert.Equal(t, 1, count, "two nbdkits for one volume must produce one registry entry, not two") } + +// --- orphan reaping --- + +// TestReapOrphanedNbdkit_NoReferencer_SignalsAndRemovesSocket proves the +// baseline case the bead asks for: an unclaimed nbdkit with nothing on the +// host referencing its endpoint is signalled and its socket cleaned up. +func TestReapOrphanedNbdkit_NoReferencer_SignalsAndRemovesSocket(t *testing.T) { + pid := spawnLongRunningProcess(t) + socket := filepath.Join(t.TempDir(), "vol-reap-unreferenced.sock") + listenUnix(t, socket) + procRoot := t.TempDir() // no other processes at all: nothing to find + + logs := captureLogs(t) + disc := discoveredNbdkit{PID: pid, Volume: "vol-reap-unreferenced1", Socket: socket} + reapOrphanedNbdkit(procRoot, disc, reapReasonUnadoptable) + + require.Eventually(t, func() bool { return !utils.ProcessAlive(pid) }, 2*time.Second, 20*time.Millisecond, + "an unreferenced orphan must be signalled and exit") + _, err := os.Stat(socket) + assert.True(t, os.IsNotExist(err), "the reaped orphan's socket file must be removed") + assert.Contains(t, logs.String(), "reaped unclaimed nbdkit process") + assert.Contains(t, logs.String(), fmt.Sprintf("pid=%d", pid)) + assert.Contains(t, logs.String(), "volume=vol-reap-unreferenced1") + assert.Contains(t, logs.String(), "reason=unadoptable") +} + +// TestReapOrphanedNbdkit_LiveReferencer_LeftRunningAndLogged proves the +// safety rule: an unclaimed nbdkit whose endpoint is still named on another +// process's cmdline must never be signalled, however unadoptable it looked. +func TestReapOrphanedNbdkit_LiveReferencer_LeftRunningAndLogged(t *testing.T) { + pid := spawnLongRunningProcess(t) + socket := filepath.Join(t.TempDir(), "vol-reap-referenced.sock") + listenUnix(t, socket) + + procRoot := t.TempDir() + const referencerPID = 909090 + writeReferencerFixture(t, procRoot, referencerPID, + fmt.Sprintf("driver=nbd,node-name=vol0,server.type=unix,server.path=%s,export=", socket)) + + logs := captureLogs(t) + disc := discoveredNbdkit{PID: pid, Volume: "vol-reap-referenced1", Socket: socket} + reapOrphanedNbdkit(procRoot, disc, reapReasonDuplicate) + + // Give a real signal every chance to have landed before asserting it did not. + time.Sleep(100 * time.Millisecond) + assert.True(t, utils.ProcessAlive(pid), "a referenced orphan must never be signalled") + _, err := os.Stat(socket) + assert.NoError(t, err, "a live orphan's socket must not be removed") + assert.Contains(t, logs.String(), "left running deliberately") + assert.Contains(t, logs.String(), fmt.Sprintf("pid=%d", pid)) + assert.Contains(t, logs.String(), "volume=vol-reap-referenced1") + assert.Contains(t, logs.String(), fmt.Sprintf("referencing_pid=%d", referencerPID)) +} + +// TestReapOrphanedNbdkit_TCPEndpointReferencedAcrossSplitBlockdevArgs proves +// endpointReferencer recognizes a TCP endpoint even when QEMU's -blockdev +// splits host and port into server.host=/server.port= within one argument, +// not just the joined host:port form a boot/EFI drive uses. +func TestReapOrphanedNbdkit_TCPEndpointReferencedAcrossSplitBlockdevArgs(t *testing.T) { + pid := spawnLongRunningProcess(t) + procRoot := t.TempDir() + const referencerPID = 909091 + writeReferencerFixture(t, procRoot, referencerPID, + "driver=nbd,node-name=vol0,server.type=inet,server.host=127.0.0.1,server.port=10899,export=") + + disc := discoveredNbdkit{PID: pid, Volume: "vol-reap-tcp1", Port: 10899} + reapOrphanedNbdkit(procRoot, disc, reapReasonUnadoptable) + + time.Sleep(100 * time.Millisecond) + assert.True(t, utils.ProcessAlive(pid), "a TCP endpoint split across server.host=/server.port= must still be recognized as referenced") +} + +// unreadableProcDir creates procRoot/ with a cmdline file inside, then +// strips all permissions from the directory itself, so os.ReadFile on the +// cmdline path inside it fails with a permission error rather than +// os.ErrNotExist -- standing in for an /proc mounted with hidepid, where a +// guest owned by another uid is invisible to viperblockd entirely. Skips the +// calling test when running as root, since root ignores directory permission +// bits and the failure this simulates would not occur. +func unreadableProcDir(t *testing.T, procRoot string, pid int) { + t.Helper() + if os.Geteuid() == 0 { + t.Skip("cannot simulate a permission-denied /proc entry while running as root") + } + dir := filepath.Join(procRoot, strconv.Itoa(pid)) + require.NoError(t, os.MkdirAll(dir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "cmdline"), []byte("x\x00"), 0644)) + require.NoError(t, os.Chmod(dir, 0000)) + t.Cleanup(func() { _ = os.Chmod(dir, 0755) }) // let t.TempDir() clean up +} + +// TestEndpointReferencer_MissingCandidateCmdline_NotAnError proves a +// candidate that exited between ReadDir and the cmdline read (ENOENT) is +// correctly read as "not a referencer", not as an inconclusive scan. +func TestEndpointReferencer_MissingCandidateCmdline_NotAnError(t *testing.T) { + procRoot := t.TempDir() + // A numbered entry with no cmdline file at all: the process vanished + // between the directory listing and this read. + require.NoError(t, os.MkdirAll(filepath.Join(procRoot, "424243"), 0755)) + + disc := discoveredNbdkit{PID: 424242, Volume: "vol-enoent1", Socket: filepath.Join(t.TempDir(), "x.sock")} + pid, found, err := endpointReferencer(procRoot, disc) + + require.NoError(t, err, "a vanished candidate must not make the scan inconclusive") + assert.False(t, found) + assert.Zero(t, pid) +} + +// TestEndpointReferencer_UnreadableCandidate_ReturnsError proves the hole the +// safety argument depends on is closed: a candidate this scan could not read +// for any reason other than ENOENT must surface as an error, not as "no +// referencer found", since a read failure is not evidence of absence. +func TestEndpointReferencer_UnreadableCandidate_ReturnsError(t *testing.T) { + procRoot := t.TempDir() + unreadableProcDir(t, procRoot, 555555) + + disc := discoveredNbdkit{PID: 424242, Volume: "vol-eacces1", Socket: filepath.Join(t.TempDir(), "x.sock")} + _, found, err := endpointReferencer(procRoot, disc) + + require.Error(t, err, "a permission-denied candidate must make the scan inconclusive, not silently pass") + assert.False(t, os.IsNotExist(err), "the error must not be mistaken for ENOENT") + assert.False(t, found) +} + +// TestReapOrphanedNbdkit_UnreadableCandidate_LeftRunningAndLogged is the +// end-to-end version of the fix: an unclaimed nbdkit is left running, not +// reaped, when the referencer scan itself could not be completed -- the same +// fail-safe outcome as an unreadable procRoot, just triggered by one +// unreadable candidate entry among otherwise-readable ones. +func TestReapOrphanedNbdkit_UnreadableCandidate_LeftRunningAndLogged(t *testing.T) { + pid := spawnLongRunningProcess(t) + socket := filepath.Join(t.TempDir(), "vol-reap-eacces.sock") + listenUnix(t, socket) + + procRoot := t.TempDir() + unreadableProcDir(t, procRoot, 555556) + + logs := captureLogs(t) + disc := discoveredNbdkit{PID: pid, Volume: "vol-reap-eacces1", Socket: socket} + reapOrphanedNbdkit(procRoot, disc, reapReasonUnadoptable) + + time.Sleep(100 * time.Millisecond) + assert.True(t, utils.ProcessAlive(pid), "an inconclusive scan must never result in a signal") + _, err := os.Stat(socket) + assert.NoError(t, err, "an orphan left running because the scan was inconclusive must keep its socket") + assert.Contains(t, logs.String(), "could not scan for processes") + assert.Contains(t, logs.String(), fmt.Sprintf("pid=%d", pid)) + assert.Contains(t, logs.String(), "volume=vol-reap-eacces1") +} + +// writeMountinfoFixture writes procRoot/self/mountinfo with a single "/proc" +// entry carrying opts, standing in for the real /proc/self/mountinfo that +// procHidepidInvisible parses on a live node. +func writeMountinfoFixture(t *testing.T, procRoot, opts string) { + t.Helper() + dir := filepath.Join(procRoot, "self") + require.NoError(t, os.MkdirAll(dir, 0755)) + line := fmt.Sprintf("36 35 0:29 / /proc %s shared:1 - proc proc rw\n", opts) + require.NoError(t, os.WriteFile(filepath.Join(dir, "mountinfo"), []byte(line), 0644)) +} + +// TestProcHidepidInvisible_DetectsNamedAndLegacyValues proves the mountinfo +// parser recognizes both the named hidepid=invisible option and its legacy +// numeric equivalent, and does not false-positive on an unrelated mount. +func TestProcHidepidInvisible_DetectsNamedAndLegacyValues(t *testing.T) { + for _, tc := range []struct { + name string + opts string + want bool + }{ + {"named invisible", "rw,nosuid,nodev,noexec,relatime,hidepid=invisible", true}, + {"legacy numeric 2", "rw,nosuid,nodev,noexec,relatime,hidepid=2", true}, + {"hidepid off", "rw,nosuid,nodev,noexec,relatime,hidepid=off", false}, + {"hidepid noaccess (EACCES-style, already handled)", "rw,nosuid,nodev,noexec,relatime,hidepid=1", false}, + {"no hidepid option at all", "rw,nosuid,nodev,noexec,relatime", false}, + } { + t.Run(tc.name, func(t *testing.T) { + procRoot := t.TempDir() + writeMountinfoFixture(t, procRoot, tc.opts) + + got, err := procHidepidInvisible(procRoot) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +// TestProcHidepidInvisible_MissingMountinfoIsNotEvidence proves a fabricated +// procRoot with no self/mountinfo at all (every other test's fixture shape) +// is read as "not detected", not as an error -- so it never regresses tests +// written before this fail-safe existed. +func TestProcHidepidInvisible_MissingMountinfoIsNotEvidence(t *testing.T) { + got, err := procHidepidInvisible(t.TempDir()) + require.NoError(t, err) + assert.False(t, got) +} + +// TestReapOrphanedNbdkit_HidepidInvisible_DeclinesEvenOnACleanScan is the +// bug this fix closes: under hidepid=invisible a live referencer's +// /proc/ directory entry is simply absent, not unreadable, so +// endpointReferencer would return a clean "not found" with no error at all. +// reapOrphanedNbdkit must detect the hazardous mount itself and decline +// before trusting that clean result, exactly as it declines on EACCES. +func TestReapOrphanedNbdkit_HidepidInvisible_DeclinesEvenOnACleanScan(t *testing.T) { + pid := spawnLongRunningProcess(t) + socket := filepath.Join(t.TempDir(), "vol-reap-hidepid.sock") + listenUnix(t, socket) + + procRoot := t.TempDir() // no referencer entries at all: a clean scan + writeMountinfoFixture(t, procRoot, "rw,nosuid,nodev,noexec,relatime,hidepid=invisible") + + logs := captureLogs(t) + disc := discoveredNbdkit{PID: pid, Volume: "vol-reap-hidepid1", Socket: socket} + reapOrphanedNbdkit(procRoot, disc, reapReasonUnadoptable) + + time.Sleep(100 * time.Millisecond) + assert.True(t, utils.ProcessAlive(pid), "hidepid=invisible must never be read as a clean 'no referencer' scan") + _, err := os.Stat(socket) + assert.NoError(t, err, "an orphan left running because entries could be hidden must keep its socket") + assert.Contains(t, logs.String(), "hides other users' process entries") + assert.Contains(t, logs.String(), fmt.Sprintf("pid=%d", pid)) + assert.Contains(t, logs.String(), "volume=vol-reap-hidepid1") +} + +// TestRecoverMountedVolumes_DuplicateVolume_UnusedDuplicateReaped extends +// TestRecoverMountedVolumes_DuplicateVolumeAdoptedOnce: the duplicate +// recovery declines to adopt must not just be skipped but reaped, while the +// adopted process is never touched, matching the bead's acceptance that no +// volume is ever served by two processes. +func TestRecoverMountedVolumes_DuplicateVolume_UnusedDuplicateReaped(t *testing.T) { + _, natsURL := setupEmbeddedNATS(t) + cfg := setupTestConfig(t, natsURL) + cfg.constructVB = fileBackedConstructVB(t) + nc := startProviderSubjects(t, cfg, natsURL) + + const volumeName = "vol-duplicatereap1" + procRoot := t.TempDir() + socketDir := t.TempDir() + + // Both candidates are real processes: scanNbdkitProcs' adoption order + // follows os.ReadDir's lexical sort of the PID directory names, which is + // not something this test controls, so either could end up "adopted". + // Using two real spawned PIDs means whichever one recovery declines to + // adopt is always safe to actually SIGTERM. + pids := map[int]string{} + for _, name := range []string{"a", "b"} { + pid := spawnLongRunningProcess(t) + socket := filepath.Join(socketDir, name+".sock") + listenUnix(t, socket) + writeProcFixture(t, procRoot, pid, "nbdkit", nbdkitArgv(cfg.BaseDir, socket, 0, volumeName)) + pids[pid] = socket + } + + recoverMountedVolumes(context.Background(), cfg, nc, procRoot) + + mv, ok := findMountedVolume(cfg, volumeName) + require.True(t, ok) + t.Cleanup(func() { mv.VB.StopWALSyncer() }) + require.Contains(t, pids, mv.PID, "the adopted PID must be one of the two candidates") + + var duplicatePID int + var duplicateSocket string + for pid, socket := range pids { + if pid != mv.PID { + duplicatePID, duplicateSocket = pid, socket + } + } + + require.Eventually(t, func() bool { return !utils.ProcessAlive(duplicatePID) }, 2*time.Second, 20*time.Millisecond, + "the unused duplicate must be reaped") + assert.True(t, utils.ProcessAlive(mv.PID), "the adopted process must never be signalled") + _, err := os.Stat(duplicateSocket) + assert.True(t, os.IsNotExist(err), "the reaped duplicate's socket file must be removed") +} + +// TestRecoverMountedVolumes_UnadoptableReapedAcrossRestarts covers the +// restart-then-stop sequence the bead asks for, not a single mount/unmount +// lifetime: a backend recovery cannot adopt (its volume no longer exists) is +// reaped on the first restart's recovery pass, and a second pass -- standing +// in for the field case's further stop/start cycles -- finds nothing left to +// reap or log, proving the orphan does not survive the way it did before +// this fix. +func TestRecoverMountedVolumes_UnadoptableReapedAcrossRestarts(t *testing.T) { + _, natsURL := setupEmbeddedNATS(t) + cfg := setupTestConfig(t, natsURL) + cfg.S3Host = fastFailingS3Host(t) // forces buildVB to fail: the volume is unadoptable + nc := startProviderSubjects(t, cfg, natsURL) + + const volumeName = "vol-unadoptable-restart1" + procRoot := t.TempDir() + pid := spawnLongRunningProcess(t) + socket := filepath.Join(t.TempDir(), "o.sock") // short: unix socket paths cap at ~108 bytes + listenUnix(t, socket) + writeProcFixture(t, procRoot, pid, "nbdkit", nbdkitArgv(cfg.BaseDir, socket, 0, volumeName)) + + // First restart's recovery pass: the volume cannot be rebuilt, and + // nothing references its endpoint, so it must be reaped. + logs := captureLogs(t) + recoverMountedVolumes(context.Background(), cfg, nc, procRoot) + + require.Eventually(t, func() bool { return !utils.ProcessAlive(pid) }, 2*time.Second, 20*time.Millisecond, + "an unadoptable backend with no referencer must be reaped on its first recovery pass") + assert.Contains(t, logs.String(), "reaped unclaimed nbdkit process") + _, ok := findMountedVolume(cfg, volumeName) + assert.False(t, ok, "a backend recovery could not rebuild must never appear in MountedVolumes") + + // The kernel's real /proc would no longer list a PID that has exited; + // mirror that so the second pass sees exactly what a real restart would. + require.NoError(t, os.RemoveAll(filepath.Join(procRoot, strconv.Itoa(pid)))) + + // Second restart's recovery pass (standing in for further stop/start + // cycles): nothing survived, so there is nothing left to discover, reap + // or log -- unlike the field case, which was still there after six. + logs2 := captureLogs(t) + recoverMountedVolumes(context.Background(), cfg, nc, procRoot) + + assert.NotContains(t, logs2.String(), "volume="+volumeName, + "a reaped orphan must not be rediscovered or re-logged on a later restart") + _, ok = findMountedVolume(cfg, volumeName) + assert.False(t, ok) +} diff --git a/spinifex/systemd/units_gen.go b/spinifex/systemd/units_gen.go index 74e285ae6..ec4697882 100644 --- a/spinifex/systemd/units_gen.go +++ b/spinifex/systemd/units_gen.go @@ -20,7 +20,7 @@ var Units = map[string]string{ "spinifex-shutdown.service": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex graceful guest drain\nPartOf=spinifex.target\n# After= all services so it stops FIRST on target/host shutdown: the drain runs\n# while nats/predastore/viperblock/daemon are still up (QMP, unmount, WAL flush).\nAfter=spinifex-nats.service spinifex-predastore.service spinifex-viperblock.service spinifex-daemon.service\n\n[Service]\nType=oneshot\nRemainAfterExit=yes\nExecStart=/bin/true\n# ExecStop fires on ANY target stop, including the stop half of `systemctl\n# restart spinifex.target` (a rolling deploy) -- PartOf propagates regardless\n# of whether the host is actually going down. --unless-restarting drains on a\n# real shutdown/reboot and on a plain `systemctl stop spinifex.target`; it\n# skips only when a restart of the stack is already queued, so QEMU/nbdkit\n# survive (KillMode=process) and the daemon reconnects on the way back up.\nExecStop=/usr/local/bin/spx admin node drain --local --timeout=120s --unless-restarting\nTimeoutStopSec=180\n\n[Install]\nWantedBy=spinifex.target\n", "spinifex-system.slice": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex control/infra tier (NATS, predastore, viperblock, daemon, awsgw, vpcd, ui)\nBefore=slices.target\n\n[Slice]\n# RG-3: child of spinifex.slice (spinifex-system -> spinifex), inherits the\n# parent ceiling, no extra limit. Holds the 7 control/infra services. Guest QEMU\n# moves to the sibling spinifex-guests.slice (per-instance placement lands in\n# 0002 RG-7); until then the daemon-forked guests stay under spinifex-daemon.service.\n", "spinifex-ui.service": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex Web UI\nPartOf=spinifex.target\nAfter=spinifex-awsgw.service\n\n[Service]\nEnvironmentFile=-/etc/spinifex/telemetry.env\nType=simple\nSlice=spinifex-system.slice\nUser=spinifex-ui\nGroup=spinifex\nExecStart=/usr/local/bin/spx service spinifex-ui start\nRestart=on-failure\nRestartSec=5\n# server.Shutdown() uses an internal 30s context (WriteTimeout=120s covers\n# long streaming proxy responses, e.g. console passthrough); add margin above it.\nTimeoutStopSec=40\nOOMScoreAdjust=-500\n# Per-service ceiling bounds a leak in this service alone rather than\n# tripping the slice-wide cap, which would pick an arbitrary victim.\nMemoryHigh=10%\nMemoryMax=20%\nEnvironment=SPINIFEX_UI_TLS_CERT=/etc/spinifex/server.pem\nEnvironment=SPINIFEX_UI_TLS_KEY=/etc/spinifex/server.key\nEnvironment=SPINIFEX_UI_BASE_DIR=/var/lib/spinifex/spinifex-ui/\n\n# Filesystem access\nNoNewPrivileges=yes\nProtectSystem=strict\nPrivateDevices=yes\nReadOnlyPaths=/etc/spinifex/server.pem /etc/spinifex/server.key\nReadWritePaths=/var/lib/spinifex/spinifex-ui /var/log/spinifex\n\n# Hardening\nProtectHome=yes\nPrivateTmp=yes\nRestrictSUIDSGID=yes\nLockPersonality=yes\nRestrictRealtime=yes\nRestrictNamespaces=yes\nProtectClock=yes\nProtectKernelTunables=yes\nProtectKernelModules=yes\nProtectKernelLogs=yes\nProtectControlGroups=yes\nProtectProc=invisible\nSystemCallArchitectures=native\nMemoryDenyWriteExecute=yes\nRestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK\nCapabilityBoundingSet=\nProtectHostname=yes\nKeyringMode=private\nRemoveIPC=yes\nLimitCORE=0\nLimitNOFILE=65536\nLimitNPROC=4096\n\n[Install]\nWantedBy=spinifex.target\n", - "spinifex-viperblock.service": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex Viperblock (Block Storage)\nPartOf=spinifex.target\nAfter=spinifex-nats.service spinifex-predastore.service\nWants=spinifex-nats.service\n\n[Service]\nEnvironmentFile=-/etc/spinifex/telemetry.env\nType=simple\nSlice=spinifex-system.slice\nUser=spinifex-viperblock\nGroup=spinifex\nExecStartPre=/var/lib/spinifex/wait-for-nats.sh\nExecStart=/usr/local/bin/spx service viperblock start\nRestart=on-failure\nRestartSec=5\nOOMScoreAdjust=-500\n# Per-service ceiling bounds a leak in this service alone rather than\n# tripping the slice-wide cap, which would pick an arbitrary victim.\nMemoryHigh=15%\nMemoryMax=25%\n# KillMode=process: nbdkit still serving a guest survives a restart here, the\n# way guest QEMU survives a daemon restart. Tearing one out mid-write corrupts\n# that guest's filesystem; idle nbdkit is reaped explicitly on shutdown.\nKillMode=process\n# One worst-case nbdkit reap: killProcessGracePeriod (120s, utils.KillProcess)\n# plus its own shutdownDrainTimeout (20s, viperblock/nbd) and headroom — must\n# strictly exceed 120s now shutdownVolumes reaps concurrently, not serially.\nTimeoutStopSec=150\nEnvironmentFile=/etc/spinifex/systemd.env\nEnvironment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml\nEnvironment=SPINIFEX_BASE_DIR=/var/lib/spinifex/viperblock/\n\n# Filesystem access\nNoNewPrivileges=yes\nProtectSystem=strict\nPrivateDevices=yes\nReadOnlyPaths=/etc/spinifex/spinifex.toml /etc/spinifex/systemd.env /etc/spinifex/ca.pem\nReadWritePaths=/var/lib/spinifex/viperblock /var/log/spinifex /run/spinifex/nbd\n\n# Hardening\nProtectHome=yes\nPrivateTmp=yes\nRestrictSUIDSGID=yes\nLockPersonality=yes\nRestrictRealtime=yes\nRestrictNamespaces=yes\nProtectClock=yes\nProtectKernelTunables=yes\nProtectKernelModules=yes\nProtectKernelLogs=yes\nProtectControlGroups=yes\nProtectProc=invisible\nSystemCallArchitectures=native\nMemoryDenyWriteExecute=yes\nRestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK\nCapabilityBoundingSet=\nProtectHostname=yes\nKeyringMode=private\nRemoveIPC=yes\nLimitCORE=0\nLimitNOFILE=65536\nLimitNPROC=4096\n\n[Install]\nWantedBy=spinifex.target\n", + "spinifex-viperblock.service": "# spinifex-unit-version: 2\n[Unit]\nDescription=Spinifex Viperblock (Block Storage)\nPartOf=spinifex.target\nAfter=spinifex-nats.service spinifex-predastore.service\nWants=spinifex-nats.service\n\n[Service]\nEnvironmentFile=-/etc/spinifex/telemetry.env\nType=simple\nSlice=spinifex-system.slice\nUser=spinifex-viperblock\nGroup=spinifex\nExecStartPre=/var/lib/spinifex/wait-for-nats.sh\nExecStart=/usr/local/bin/spx service viperblock start\nRestart=on-failure\nRestartSec=5\nOOMScoreAdjust=-500\n# Per-service ceiling bounds a leak in this service alone rather than\n# tripping the slice-wide cap, which would pick an arbitrary victim.\nMemoryHigh=15%\nMemoryMax=25%\n# KillMode=process: nbdkit still serving a guest survives a restart here, the\n# way guest QEMU survives a daemon restart. Tearing one out mid-write corrupts\n# that guest's filesystem; idle nbdkit is reaped explicitly on shutdown.\nKillMode=process\n# One worst-case nbdkit reap: killProcessGracePeriod (120s, utils.KillProcess)\n# plus its own shutdownDrainTimeout (20s, viperblock/nbd) and headroom — must\n# strictly exceed 120s now shutdownVolumes reaps concurrently, not serially.\nTimeoutStopSec=150\nEnvironmentFile=/etc/spinifex/systemd.env\nEnvironment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml\nEnvironment=SPINIFEX_BASE_DIR=/var/lib/spinifex/viperblock/\n\n# Filesystem access\nNoNewPrivileges=yes\nProtectSystem=strict\nPrivateDevices=yes\nReadOnlyPaths=/etc/spinifex/spinifex.toml /etc/spinifex/systemd.env /etc/spinifex/ca.pem\nReadWritePaths=/var/lib/spinifex/viperblock /var/log/spinifex /run/spinifex/nbd\n\n# Hardening\nProtectHome=yes\nPrivateTmp=yes\nRestrictSUIDSGID=yes\nLockPersonality=yes\nRestrictRealtime=yes\nRestrictNamespaces=yes\nProtectClock=yes\nProtectKernelTunables=yes\nProtectKernelModules=yes\nProtectKernelLogs=yes\nProtectControlGroups=yes\n# ProtectProc must stay default: hidepid=invisible hides spinifex-daemon's\n# QEMU /proc entries (different uid) orphan nbdkit reaping must see.\nSystemCallArchitectures=native\nMemoryDenyWriteExecute=yes\nRestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK\nCapabilityBoundingSet=\nProtectHostname=yes\nKeyringMode=private\nRemoveIPC=yes\nLimitCORE=0\nLimitNOFILE=65536\nLimitNPROC=4096\n\n[Install]\nWantedBy=spinifex.target\n", "spinifex-vpcd.service": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex VPC Daemon (OVN Networking)\nPartOf=spinifex.target\nAfter=spinifex-nats.service spinifex-daemon.service systemd-networkd.service openvswitch-switch.service ovn-controller.service\nWants=spinifex-nats.service ovn-controller.service\n\n[Service]\nEnvironmentFile=-/etc/spinifex/telemetry.env\nType=simple\nSlice=spinifex-system.slice\nUser=spinifex-vpcd\nGroup=spinifex\nExecStartPre=/var/lib/spinifex/wait-for-nats.sh\nExecStart=/usr/local/bin/spx service vpcd start\nRestart=on-failure\nRestartSec=5\n# Waits for an in-flight OVN drift-reconcile pass to return before exiting;\n# no sub-timeout bounds that call today, so this covers a normal pass.\nTimeoutStopSec=20\nOOMScoreAdjust=-500\n# Per-service ceiling: bounds a leak here instead of the slice-wide cap.\nMemoryHigh=10%\nMemoryMax=20%\nEnvironment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml\nEnvironment=SPINIFEX_BASE_DIR=/var/lib/spinifex/vpcd/\n\n# RG-9: network tier. Per-tap IMDS removed the in-process setns, so vpcd no\n# longer enters a per-VPC netns: CAP_SYS_ADMIN is dropped and the filesystem\n# sandbox is restored.\n#\n# RG-10: these are ambient so ip/iptables/sysctl/arping inherit them on exec and\n# vpcd holds no sudoers rules at all. NoNewPrivileges stays off pending the ADR\n# update that retires the last escalation path.\nAmbientCapabilities=CAP_NET_ADMIN CAP_NET_RAW CAP_NET_BIND_SERVICE CAP_SETUID CAP_SETGID\nCapabilityBoundingSet=CAP_NET_ADMIN CAP_NET_RAW CAP_NET_BIND_SERVICE CAP_SETUID CAP_SETGID\nNoNewPrivileges=no\nProtectSystem=full\nPrivateDevices=yes\nReadOnlyPaths=/etc/spinifex/spinifex.toml /etc/spinifex/server.pem /etc/spinifex/server.key /etc/spinifex/ca.pem\nReadWritePaths=/var/lib/spinifex/vpcd /var/log/spinifex\n\n# RG-9: hardening baseline\nProtectHome=yes\nPrivateTmp=yes\nRestrictSUIDSGID=yes\nLockPersonality=yes\nRestrictRealtime=yes\nRestrictNamespaces=yes\nProtectClock=yes\nProtectKernelTunables=yes\nProtectKernelModules=yes\nProtectKernelLogs=yes\nProtectControlGroups=yes\nProtectProc=invisible\nSystemCallArchitectures=native\nMemoryDenyWriteExecute=yes\n# AF_PACKET: raw L2 BOOTP for the in-process DHCP client before the iface has an IP\nRestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK AF_PACKET\nProtectHostname=yes\nKeyringMode=private\nRemoveIPC=yes\nLimitCORE=0\nLimitNOFILE=65536\nLimitNPROC=4096\n\n[Install]\nWantedBy=spinifex.target\n", "spinifex.slice": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex resource-bounded slice (host ceiling)\nBefore=slices.target\n\n[Slice]\n# RG-1: no CPUWeight — slice roots are siblings; capping below the default would\n# let user.slice out-prioritize the VM fleet. Operator CPU priority lives on\n# system.slice (system.slice.d/spinifex-reserve.conf).\n# RG-1/RG-5: throttle+reclaim at High, hard wall at Max (% of physical RAM).\nMemoryHigh=80%\nMemoryMax=90%\n# RG-5: spill to the full swap device before the MemoryMax wall; matched to swap size.\nMemorySwapMax=8G\n", "spinifex.target": "# spinifex-unit-version: 1\n[Unit]\nDescription=Spinifex Infrastructure Platform\nAfter=network-online.target\nWants=network-online.target\nWants=spinifex-nats.service spinifex-predastore.service spinifex-northstar.service spinifex-viperblock.service spinifex-daemon.service spinifex-awsgw.service spinifex-vpcd.service spinifex-ui.service spinifex-qmp-collector.service spinifex-shutdown.service spinifex-nats-watchdog.timer openvswitch-switch.service ovn-controller.service\n\n[Install]\nWantedBy=multi-user.target\n", diff --git a/spinifex/systemd/units_invariants_test.go b/spinifex/systemd/units_invariants_test.go index d94f446eb..16a221680 100644 --- a/spinifex/systemd/units_invariants_test.go +++ b/spinifex/systemd/units_invariants_test.go @@ -431,10 +431,13 @@ var unitBodyHashes = map[string]map[int]string{ "spinifex-shutdown.service": {1: "bcdc455916f35aa7494b2fe25e691339e8f1e22f031dfd9fd95203a9aa4bdaa4"}, "spinifex-system.slice": {1: "ca450c2b28a8b13dd767957fa9469bd74bd222d7abed79945e83d564d5ce16dd"}, "spinifex-ui.service": {1: "9d5ec3785bf730405f23ec9df79675bf65ecbca9df47c15f5a4b4639393afbc5"}, - "spinifex-viperblock.service": {1: "5c8cdd2004abf8e5725cbce0200565b9b29be7ab3210a4e0a2cdfe37ec5facb9"}, - "spinifex-vpcd.service": {1: "1b722640310145767cd34e87f4804852e63f46d2145ed20f8cc5f5400ebc5965"}, - "spinifex.slice": {1: "f73d9343e0e1bedd647835c8bb0c80fb3a3bd66474661234ecac23a4caafc24f"}, - "spinifex.target": {1: "0ffba9faee5a477f8ff7466a6bccb4dc7e04f5cf92a405553c242e6548402078"}, + "spinifex-viperblock.service": { + 1: "5c8cdd2004abf8e5725cbce0200565b9b29be7ab3210a4e0a2cdfe37ec5facb9", + 2: "5d691bf5ce4a5a636d5dcc07f7bfa36d0da9eb0d7fb26079f89358ca0e5440cd", + }, + "spinifex-vpcd.service": {1: "1b722640310145767cd34e87f4804852e63f46d2145ed20f8cc5f5400ebc5965"}, + "spinifex.slice": {1: "f73d9343e0e1bedd647835c8bb0c80fb3a3bd66474661234ecac23a4caafc24f"}, + "spinifex.target": {1: "0ffba9faee5a477f8ff7466a6bccb4dc7e04f5cf92a405553c242e6548402078"}, } // TestUnitBodyPinnedToVersion asserts each unit's body hash matches the