From ee1ee4f0331ff4a58a09c9c8771a6dd49b64fe17 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Thu, 13 Aug 2026 11:50:34 +1000 Subject: [PATCH] fix(preflight): report services still executing a replaced binary (#781) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Overwriting /usr/local/bin/spx while spinifex.target is running does not move the services onto it. Each keeps executing the replaced inode until it restarts, so a node serves a mixture of two builds, and because the handlers are NATS queue-group workers the skew shows up as roughly one request in three behaving like the old build — an intermittent fault rather than a broken node. The ansible deploy path already detects and remediates this, but only there: an out-of-band binary copy leaves nothing to report it, and on env19 that state went unnoticed for a full day. spx admin preflight now reports it, so an operator has a command to run and a script has a non-zero exit to gate on. Detection follows the deploy path's: a (deleted) marker on /proc//exe, a target path that no longer resolves, or a path that resolves to a different inode than the one being executed. --- cmd/spinifex/cmd/admin_preflight.go | 5 + docs/admin/update/README.md | 6 ++ spinifex/preflight/preflight.go | 3 +- spinifex/preflight/services.go | 136 ++++++++++++++++++++++++ spinifex/preflight/services_test.go | 157 ++++++++++++++++++++++++++++ 5 files changed, 306 insertions(+), 1 deletion(-) create mode 100644 spinifex/preflight/services.go create mode 100644 spinifex/preflight/services_test.go diff --git a/cmd/spinifex/cmd/admin_preflight.go b/cmd/spinifex/cmd/admin_preflight.go index 91e2121bf..65b999fe2 100644 --- a/cmd/spinifex/cmd/admin_preflight.go +++ b/cmd/spinifex/cmd/admin_preflight.go @@ -19,6 +19,11 @@ has the privileged helper scripts and sudoers grants that build depends on — setup.sh and setup-ovn.sh install them, and a binary-swap deploy (make deploy or a manual binary copy) skips those steps. +It also reports any running service still executing a replaced binary. +Overwriting /usr/local/bin/spx while spinifex.target is up does not move the +services onto it: each keeps executing the replaced inode until it restarts, +so a node can serve a mixture of two builds indefinitely. + Checks compare content, not mere existence, so a present-but-stale asset is caught too. Exits non-zero if any asset is Missing, Stale, or Ungranted.`, RunE: runAdminPreflight, diff --git a/docs/admin/update/README.md b/docs/admin/update/README.md index 2e30974d3..fb3fccd23 100644 --- a/docs/admin/update/README.md +++ b/docs/admin/update/README.md @@ -174,6 +174,12 @@ This is easy to miss, because the request handlers are NATS queue-group workers Check for it with: +```bash +sudo spx admin preflight +``` + +Any unit reported `Stale` with kind `service` is running a replaced binary. The check covers every `.service` unit this build ships, and exits non-zero when it finds one, so it also works as a gate in a script. To see the raw state instead: + ```bash for u in spinifex-daemon spinifex-awsgw spinifex-viperblock spinifex-vpcd spinifex-ui spinifex-predastore; do pid=$(systemctl show -p MainPID --value "$u") diff --git a/spinifex/preflight/preflight.go b/spinifex/preflight/preflight.go index 206db4552..cd393821e 100644 --- a/spinifex/preflight/preflight.go +++ b/spinifex/preflight/preflight.go @@ -49,7 +49,7 @@ func (s Status) String() string { // Result is the outcome of checking one managed asset against this build's manifest. type Result struct { Path string - Kind string // "helper" | "sudoers-grant" + Kind string // "helper" | "sudoers-grant" | "service" Status Status Detail string } @@ -80,6 +80,7 @@ func CheckHostAt(root string) []Result { results = append(results, checkHelper(root, path)) } results = append(results, checkSudoersGrant(root)) + results = append(results, checkRunningBinaries(root)...) return results } diff --git a/spinifex/preflight/services.go b/spinifex/preflight/services.go new file mode 100644 index 000000000..884ae281c --- /dev/null +++ b/spinifex/preflight/services.go @@ -0,0 +1,136 @@ +package preflight + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "sort" + "strconv" + "strings" + "syscall" + + "github.com/mulgadc/spinifex/spinifex/systemd" +) + +// deletedSuffix is what the kernel appends to /proc//exe once the +// executable's directory entry is gone, which is what a binary swap under a +// running service leaves behind. +const deletedSuffix = " (deleted)" + +// mainPIDFor is overridable so tests never shell out to a real systemd. +var mainPIDFor = func(unit string) (int, error) { + out, err := exec.Command("systemctl", "show", "-p", "MainPID", "--value", unit).Output() + if err != nil { + return 0, err + } + return strconv.Atoi(strings.TrimSpace(string(out))) +} + +// serviceUnits returns the .service units this build ships, sorted so the +// report is stable. Timers and the target have no MainPID of their own. +func serviceUnits() []string { + names := make([]string, 0, len(systemd.Units)) + for name := range systemd.Units { + if strings.HasSuffix(name, ".service") { + names = append(names, name) + } + } + sort.Strings(names) + return names +} + +// checkRunningBinaries reports units whose running process is executing a +// binary that is no longer the one on disk. Replacing a binary under a running +// service does not move the service onto it: it keeps executing the replaced +// inode until it restarts, so a node can serve a mixture of two builds. +func checkRunningBinaries(root string) []Result { + var results []Result + for _, unit := range serviceUnits() { + pid, err := mainPIDFor(unit) + if err != nil { + // A unit this build ships but this host does not run is normal: + // services are per-node, and systemctl reports nothing for a unit + // that was never installed. + continue + } + if pid == 0 { + continue + } + results = append(results, checkUnitBinary(root, unit, pid)) + } + return results +} + +func checkUnitBinary(root, unit string, pid int) Result { + exeLink := filepath.Join(root, "proc", strconv.Itoa(pid), "exe") + + target, err := os.Readlink(exeLink) + if err != nil { + return Result{ + Path: unit, + Kind: "service", + Status: Missing, + Detail: fmt.Sprintf("pid %d: cannot read its executable: %v", pid, err), + } + } + + if deleted, ok := strings.CutSuffix(target, deletedSuffix); ok { + return Result{ + Path: unit, + Kind: "service", + Status: Stale, + Detail: fmt.Sprintf("pid %d is running a deleted %s — restart spinifex.target", pid, deleted), + } + } + + // A binary removed outright leaves a link to a path that no longer + // resolves, which is the same skew reported without the marker. + onDisk, err := inodeOf(filepath.Join(root, target)) + if err != nil { + return Result{ + Path: unit, + Kind: "service", + Status: Stale, + Detail: fmt.Sprintf("pid %d is running %s, which is no longer on disk — restart spinifex.target", pid, target), + } + } + + // The marker is absent when the running inode still has another link, so + // compare inodes too: the path resolving to a different file than the one + // being executed is the same skew, unsignposted. + running, err := inodeOf(exeLink) + if err != nil { + return Result{ + Path: unit, + Kind: "service", + Status: Missing, + Detail: fmt.Sprintf("pid %d: cannot stat its executable: %v", pid, err), + } + } + if running != onDisk { + return Result{ + Path: unit, + Kind: "service", + Status: Stale, + Detail: fmt.Sprintf("pid %d is running an older copy of %s — restart spinifex.target", pid, target), + } + } + + return Result{Path: unit, Kind: "service", Status: OK} +} + +// inodeOf resolves a path to the inode behind it, following symlinks so +// /proc//exe reports the executable rather than the link. Overridable so +// tests can model a swap without a second real file. +var inodeOf = func(path string) (uint64, error) { + info, err := os.Stat(path) + if err != nil { + return 0, err + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0, fmt.Errorf("stat %s: no inode available on this platform", path) + } + return st.Ino, nil +} diff --git a/spinifex/preflight/services_test.go b/spinifex/preflight/services_test.go new file mode 100644 index 000000000..53f4f531e --- /dev/null +++ b/spinifex/preflight/services_test.go @@ -0,0 +1,157 @@ +package preflight + +import ( + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeProc builds a root containing a binary and a /proc//exe symlink to +// it, standing in for the parts of procfs the check reads. +func fakeProc(t *testing.T, pid int, binPath string, target string) string { + t.Helper() + root := t.TempDir() + + full := filepath.Join(root, binPath) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) + require.NoError(t, os.WriteFile(full, []byte("binary contents"), 0o600)) + + procDir := filepath.Join(root, "proc", strconv.Itoa(pid)) + require.NoError(t, os.MkdirAll(procDir, 0o750)) + + // The link records a host-absolute path; the check resolves it under root. + if target == "" { + target = "/" + binPath + } + require.NoError(t, os.Symlink(target, filepath.Join(procDir, "exe"))) + return root +} + +// stubInodes makes both stat calls resolve through the fake root, and lets a +// test model a swapped binary by giving the two paths different inodes. +func stubInodes(t *testing.T, root string, override map[string]uint64) { + t.Helper() + orig := inodeOf + t.Cleanup(func() { inodeOf = orig }) + + inodeOf = func(path string) (uint64, error) { + if ino, ok := override[path]; ok { + return ino, nil + } + // Everything not overridden resolves as one file, so an unswapped + // binary compares equal. + if _, err := os.Lstat(path); err != nil { + return 0, err + } + return 1, nil + } +} + +// stubMainPID reports the given pid for one unit and "no such unit" for the rest. +func stubMainPID(t *testing.T, unit string, pid int) { + t.Helper() + orig := mainPIDFor + t.Cleanup(func() { mainPIDFor = orig }) + + mainPIDFor = func(u string) (int, error) { + if u == unit { + return pid, nil + } + return 0, nil + } +} + +func TestCheckRunningBinaries(t *testing.T) { + const ( + unit = "spinifex-daemon.service" + pid = 4242 + binPath = "usr/local/bin/spx" + ) + + t.Run("running the binary that is on disk is OK", func(t *testing.T) { + root := fakeProc(t, pid, binPath, "") + stubInodes(t, root, nil) + stubMainPID(t, unit, pid) + + results := checkRunningBinaries(root) + require.Len(t, results, 1) + assert.Equal(t, unit, results[0].Path) + assert.Equal(t, "service", results[0].Kind) + assert.Equal(t, OK, results[0].Status) + }) + + t.Run("a deleted binary is reported stale", func(t *testing.T) { + root := fakeProc(t, pid, binPath, "/usr/local/bin/spx"+deletedSuffix) + stubMainPID(t, unit, pid) + + results := checkRunningBinaries(root) + require.Len(t, results, 1) + assert.Equal(t, Stale, results[0].Status) + assert.Contains(t, results[0].Detail, "deleted") + assert.Contains(t, results[0].Detail, "restart spinifex.target") + }) + + t.Run("a binary removed outright is reported stale", func(t *testing.T) { + root := fakeProc(t, pid, binPath, "") + require.NoError(t, os.Remove(filepath.Join(root, binPath))) + stubInodes(t, root, nil) + stubMainPID(t, unit, pid) + + results := checkRunningBinaries(root) + require.Len(t, results, 1) + assert.Equal(t, Stale, results[0].Status) + assert.Contains(t, results[0].Detail, "no longer on disk") + }) + + t.Run("a replaced binary at the same path is reported stale", func(t *testing.T) { + root := fakeProc(t, pid, binPath, "") + + // The path resolves to a new inode while the process still executes the + // old one — a swap that leaves no (deleted) marker behind. + stubInodes(t, root, map[string]uint64{ + filepath.Join(root, binPath): 200, + filepath.Join(root, "proc", strconv.Itoa(pid), "exe"): 100, + }) + stubMainPID(t, unit, pid) + + results := checkRunningBinaries(root) + require.Len(t, results, 1) + assert.Equal(t, Stale, results[0].Status) + assert.Contains(t, results[0].Detail, "older copy") + }) + + t.Run("a unit that is not running is skipped", func(t *testing.T) { + root := fakeProc(t, pid, binPath, "") + stubMainPID(t, "no-unit-runs.service", pid) + + assert.Empty(t, checkRunningBinaries(root)) + }) + + t.Run("an unreadable proc entry is reported rather than passed", func(t *testing.T) { + root := t.TempDir() + stubMainPID(t, unit, pid) + + results := checkRunningBinaries(root) + require.Len(t, results, 1) + assert.Equal(t, Missing, results[0].Status) + assert.Contains(t, results[0].Detail, "cannot read its executable") + }) +} + +// TestServiceUnitsAreServicesOnly guards the enumeration: a timer or the +// target has no MainPID of its own, so including one would report a spurious +// problem on every node. +func TestServiceUnitsAreServicesOnly(t *testing.T) { + units := serviceUnits() + require.NotEmpty(t, units) + + for _, u := range units { + assert.Equal(t, ".service", filepath.Ext(u), "%s is not a .service unit", u) + } + assert.Contains(t, units, "spinifex-daemon.service") + assert.NotContains(t, units, "spinifex.target") +}