mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 17:11:30 +00:00
fix(preflight): report services still executing a replaced binary (#781)
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/<pid>/exe, a target path that no longer resolves, or a path that resolves to a different inode than the one being executed.
This commit is contained in:
Josh Siviour
@@ -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,
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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/<pid>/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/<pid>/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
|
||||
}
|
||||
@@ -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/<pid>/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")
|
||||
}
|
||||
Reference in New Issue
Block a user