amadan

carlos / platform Public

zdd-readiness: Task 19 fix round 1, a killed-cgroup refusal is the box's condition, retaining setup marks empty leftovers, a pinned killed parent loses no child

verdictOf lists cgroup.ErrKilledCgroupKept as NoOwnership and memberSafeWakeReason maps it to ErrFenced's approved text. SetupRetaining marks every carlos-exec@ cgroup (and child) that holds no process, claim containment groups excepted, so the first start makes it fresh. removeKilledTree checks a directory's strict reservation before walking its children. The kernel test's sidecar sequence calls its settle.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 66c92efeb695115855cbeea770f3f61191adf0ac parent c76a1d4
10 files changed, +265 −10
  • internal/activator/claimspawn_kernel_linux_test.go +2 −1
  • internal/activator/logsink.go +8 −0
  • internal/activator/logsink_test.go +15 −0
  • internal/activator/swap.go +6 −1
  • internal/activator/swap_executed_test.go +9 −0
  • internal/activator/swap_overlap_test.go +7 −0
  • internal/cgroup/cgroup.go +5 −0
  • internal/cgroup/recreate_linux.go +88 −8
  • internal/cgroup/recreate_linux_test.go +123 −0
  • internal/cgroup/recreate_other.go +2 −0
diff --git a/internal/activator/claimspawn_kernel_linux_test.go b/internal/activator/claimspawn_kernel_linux_test.go
index 9041c16fa..0041f9029 100644
--- a/internal/activator/claimspawn_kernel_linux_test.go
+++ b/internal/activator/claimspawn_kernel_linux_test.go
@@ -189,7 +189,7 @@ func killSidecarRespawn(t *testing.T, manager *cgroup.Manager, build func() *exe
child := cgroup.RoleChild("sidecar")
var last, parentIno uint64
for attempt := range 3 {
- cmd, _, err := b.spawnInCgroupAt(rt, child, false, build)
+ cmd, settle, err := b.spawnInCgroupAt(rt, child, false, build)
if err != nil {
t.Fatalf("spawn %d: %v", attempt, err)
}
@@ -211,6 +211,7 @@ func killSidecarRespawn(t *testing.T, manager *cgroup.Manager, build func() *exe
t.Fatalf("KillSidecar %d: %v", attempt, err)
}
reaped(t, fmt.Sprintf("sidecar spawn %d", attempt), done)
+ settle()
}
manager.RemoveChild(host, child)
manager.CleanupHost(host)
diff --git a/internal/activator/logsink.go b/internal/activator/logsink.go
index f07459b60..5db96ad93 100644
--- a/internal/activator/logsink.go
+++ b/internal/activator/logsink.go
@@ -67,6 +67,14 @@ func memberSafeWakeReason(err error) string {
// itself.
err = ErrFenced
}
+ if errors.Is(err, cgroup.ErrKilledCgroupKept) {
+ // A start refused because the cgroup it would spawn into took
+ // cgroup.kill and could not be made fresh (amadan #272, Task 19).
+ // Like the populated parent above it is the box's condition, and the
+ // cgroup detail is the operator's (the manager logs it), so the
+ // member gets the maintenance fence's existing text.
+ err = ErrFenced
+ }
if errors.Is(err, errWakeUnproven) || genStartUnproven(err) || errors.Is(err, errOutcomeUndelivered) {
// A wake refused while the host's last failed wake is not proven
// stopped (zero-downtime spec §7.4), or that failed wake itself: a
diff --git a/internal/activator/logsink_test.go b/internal/activator/logsink_test.go
index c95508100..344a72936 100644
--- a/internal/activator/logsink_test.go
+++ b/internal/activator/logsink_test.go
@@ -207,3 +207,18 @@ func TestMemberSafeWakeReasonHidesALegacyParentRefusal(t *testing.T) {
t.Errorf("member reason = %q, want the fence text alone", got)
}
}
+
+// Amadan #272, Task 19: a wake refused because its cgroup took cgroup.kill
+// and could not be made fresh reads to a member as the maintenance fence's
+// approved text, not as cgroup detail. Nothing of the build ran, so the
+// wake is never one that may have started (startMayHaveRun).
+func TestMemberSafeWakeReasonHidesAKilledCgroupRefusal(t *testing.T) {
+ err := fmt.Errorf("%w: /sys/fs/cgroup/x/carlos-exec@h.example.com: device or resource busy", cgroup.ErrKilledCgroupKept)
+ got := memberSafeWakeReason(err)
+ if got != memberSafeWakeReason(ErrFenced) || strings.Contains(got, "cgroup") || strings.Contains(got, "busy") {
+ t.Errorf("member reason = %q, want the fence text alone", got)
+ }
+ if startMayHaveRun(nil, err) {
+ t.Error("a start refused for a killed cgroup counted as one that may have run")
+ }
+}
diff --git a/internal/activator/swap.go b/internal/activator/swap.go
index 4a7abb793..a1223f608 100644
--- a/internal/activator/swap.go
+++ b/internal/activator/swap.go
@@ -175,7 +175,7 @@ const (
// verdictOf types an attempt-gate error. An error that is not the gate's
// (a failure of the build) is gateProceed: the gate did not refuse.
//
-// Besides the gate's own refusals, four kinds of refusal that stop a spawn
+// Besides the gate's own refusals, six kinds of refusal that stop a spawn
// before anything of the build runs are NoOwnership, so the one classifier
// reads them as abandonment, never as a failed build:
// - plan 2's strict-host gate on placed spawns (index P2-8):
@@ -189,6 +189,10 @@ const (
// parent cgroup holds a process directly (cgroup.ErrLegacyParentPopulated,
// amadan #272): it checks before it creates the child or execs, and the
// condition is the box's, not the build's;
+// - the cgroup manager refusing a start because the cgroup it would spawn
+// into took cgroup.kill and could not be made fresh
+// (cgroup.ErrKilledCgroupKept, amadan #272, Task 19): it refuses before
+// it builds the command, so nothing ran, and the condition is the box's;
// - root refusing the nested staged executable (instancectl.ErrStagedExe,
// Task 9): it validates before it mounts or starts anything.
//
@@ -212,6 +216,7 @@ func verdictOf(err error) gateVerdict {
errors.Is(err, hostbox.ErrChanged), errors.Is(err, errClaimsAdmission),
errors.Is(err, ErrStrictRequired), errors.Is(err, ErrStrictGroupInUse), errors.Is(err, ErrStrictCleanupPending),
errors.Is(err, ErrClaimIsolationUnavailable), errors.Is(err, cgroup.ErrLegacyParentPopulated),
+ errors.Is(err, cgroup.ErrKilledCgroupKept),
errors.Is(err, errUnitNoGens), errors.Is(err, errSwapNoGens), errors.Is(err, instancectl.ErrStagedExe):
return gateNoOwnership
}
diff --git a/internal/activator/swap_executed_test.go b/internal/activator/swap_executed_test.go
index 4e020cc32..e77f5794d 100644
--- a/internal/activator/swap_executed_test.go
+++ b/internal/activator/swap_executed_test.go
@@ -45,6 +45,15 @@ func TestSwapResultReportsWhetherTheAttemptsBuildExecuted(t *testing.T) {
{"hold, the host's parent holds a process", ModeHold, func(f *swapFixture) {
f.b.genErr = fmt.Errorf("h.example.com: %w: holds 1 process(es)", cgroup.ErrLegacyParentPopulated)
}, SwapAbandoned, false},
+ // Amadan #272, Task 19: the cgroup the successor would spawn into
+ // took cgroup.kill and could not be made fresh. The manager refuses
+ // before it builds the command: the box's condition, nothing ran.
+ {"overlap, a killed cgroup would not go", ModeOverlap, func(f *swapFixture) {
+ f.b.genErr = fmt.Errorf("%w: carlos-exec@h.example.com/g2: device or resource busy", cgroup.ErrKilledCgroupKept)
+ }, SwapAbandoned, false},
+ {"hold, a killed cgroup would not go", ModeHold, func(f *swapFixture) {
+ f.b.genErr = fmt.Errorf("%w: carlos-exec@h.example.com: device or resource busy", cgroup.ErrKilledCgroupKept)
+ }, SwapAbandoned, false},
{"overlap, superseded at the switch", ModeOverlap, func(f *swapFixture) {
f.a.genHealth = func(context.Context, Gen) error { f.rev.Add(1); return nil }
}, SwapAbandoned, true},
diff --git a/internal/activator/swap_overlap_test.go b/internal/activator/swap_overlap_test.go
index 797d93da7..c9cdce781 100644
--- a/internal/activator/swap_overlap_test.go
+++ b/internal/activator/swap_overlap_test.go
@@ -294,6 +294,9 @@ func TestClassifyStep(t *testing.T) {
// Amadan #272: the generation start refused before any exec because
// the host's parent holds a process directly.
{"a populated legacy parent", live, fmt.Errorf("h: %w: holds 1 process(es)", cgroup.ErrLegacyParentPopulated), stepStale},
+ // Task 19: the start's killed cgroup could not be made fresh, so the
+ // manager refused before any exec.
+ {"a killed cgroup that would not go", live, fmt.Errorf("%w: carlos-exec@h/g2: device or resource busy", cgroup.ErrKilledCgroupKept), stepStale},
// Root refused the nested staged executable before it mounted or
// started anything (Task 9): an abandonment, not a failed build.
{"root refused the staged exe", live, fmt.Errorf("start unit: %w: not blog's staged v2", instancectl.ErrStagedExe), stepStale},
@@ -347,6 +350,10 @@ func TestVerdictOf(t *testing.T) {
// Amadan #272: a populated legacy parent refuses the start before any
// exec, and no build is at fault.
{fmt.Errorf("h: %w: holds 1 process(es)", cgroup.ErrLegacyParentPopulated), gateNoOwnership},
+ // Task 19: a start whose killed cgroup could not be made fresh is
+ // refused before the command is built; no build is at fault.
+ {cgroup.ErrKilledCgroupKept, gateNoOwnership},
+ {fmt.Errorf("%w: carlos-exec@h/role-sidecar: device or resource busy", cgroup.ErrKilledCgroupKept), gateNoOwnership},
// A row that cannot run a staged generation: nothing staged spawns.
{errUnitNoGens, gateNoOwnership},
{instancectl.ErrStagedExe, gateNoOwnership},
diff --git a/internal/cgroup/cgroup.go b/internal/cgroup/cgroup.go
index 85393b496..2266b79e5 100644
--- a/internal/cgroup/cgroup.go
+++ b/internal/cgroup/cgroup.go
@@ -249,8 +249,13 @@ func setupAt(root string, logf func(string, ...any), retain bool) (*Manager, err
// 4. Sweep per-host dirs a previous agent left behind — empty after a
// restart killed their members. rmdir refuses a non-empty dir, which is
// exactly the tolerance we want.
+ // A retaining setup keeps them, and marks the empty ones instead, so a
+ // cgroup an earlier run killed and kept is made fresh before anything
+ // spawns into it (amadan #272, seedKilled).
if !retain {
m.sweepStale()
+ } else {
+ m.seedKilled()
}
return m, nil
}
diff --git a/internal/cgroup/recreate_linux.go b/internal/cgroup/recreate_linux.go
index f7c3c2480..affd524d5 100644
--- a/internal/cgroup/recreate_linux.go
+++ b/internal/cgroup/recreate_linux.go
@@ -7,6 +7,7 @@ import (
"fmt"
"os"
"path/filepath"
+ "strings"
"sync"
"golang.org/x/sys/unix"
@@ -161,8 +162,17 @@ func (m *Manager) freshenOne(host, dir string) error {
// running as the agent's uid may have made some. The kernel refuses to
// remove a cgroup with a process in it, so a removal that succeeds also
// proves it empty. A cgroup a strict handle still pins is refused rather
-// than removed under it. The walk is bounded as the cleanup walk is.
+// than removed under it, and each directory is checked before anything
+// below it is touched, so a pinned killed parent loses none of its
+// children. The walk is bounded as the cleanup walk is.
func removeKilledTree(dir string, depth int, groups *int) error {
+ id, ok, err := pathIdentity(dir)
+ if err != nil || !ok {
+ return err
+ }
+ if reservedStrict(id) {
+ return fmt.Errorf("%w: a strict handle still holds %s", ErrStrictState, filepath.Base(dir))
+ }
entries, err := os.ReadDir(dir)
if errors.Is(err, os.ErrNotExist) {
return nil
@@ -184,13 +194,6 @@ func removeKilledTree(dir string, depth int, groups *int) error {
return err
}
}
- id, ok, err := pathIdentity(dir)
- if err != nil || !ok {
- return err
- }
- if reservedStrict(id) {
- return fmt.Errorf("%w: a strict handle still holds %s", ErrStrictState, filepath.Base(dir))
- }
if err := rmdirCgroup(dir); err != nil && !errors.Is(err, unix.ENOENT) {
return err
}
@@ -223,3 +226,80 @@ func refuseKilledChild(parent *Prepared, name string) error {
}
return nil
}
+
+// seedKilled is a retaining setup's stand-in for the sweep it skips
+// (Task 19 fix round 1). The marks are this process's own, so a host
+// cgroup or a role child that an earlier run killed and kept is unknown to
+// it, and claim-entry mode (SetupRetaining) does not sweep the empty
+// carlos-exec@ directories as Setup does. Every one that holds no process
+// is marked instead, and each of its child directories that holds none, so
+// the first start into it makes it fresh. Being marked costs an empty
+// cgroup only a recreation. A directory that holds a process, or whose
+// population cannot be read, is never marked: removing it is not ours to
+// do. The claim containment groups (carlos-exec@claim-<id>.local, unique
+// to one controller) are left alone: they are the retained evidence the
+// retaining mode exists to keep, and nothing starts into them again.
+func (m *Manager) seedKilled() {
+ entries, err := os.ReadDir(m.root)
+ if err != nil {
+ return
+ }
+ seeded := 0
+ for _, e := range entries {
+ if !e.IsDir() || !IsExecName(e.Name()) || isClaimContainment(e.Name()) {
+ continue
+ }
+ dir := filepath.Join(m.root, e.Name())
+ if kids, err := os.ReadDir(dir); err == nil {
+ for _, k := range kids {
+ if k.IsDir() && seedIfEmpty(filepath.Join(dir, k.Name())) {
+ seeded++
+ }
+ }
+ }
+ if seedIfEmpty(dir) {
+ seeded++
+ }
+ }
+ if seeded > 0 {
+ m.logf("cgroup: %d retained empty cgroup(s) will be made fresh before their next spawn", seeded)
+ }
+}
+
+// seedIfEmpty marks dir when its cgroup.events reads "populated 0" (no
+// process in it or below it) and its own cgroup.procs, when present, lists
+// none. It reports whether it marked.
+func seedIfEmpty(dir string) bool {
+ b, err := os.ReadFile(filepath.Join(dir, "cgroup.events"))
+ if err != nil {
+ return false
+ }
+ populated := true
+ for _, line := range strings.Split(string(b), "\n") {
+ if f := strings.Fields(line); len(f) == 2 && f[0] == "populated" {
+ populated = f[1] != "0"
+ }
+ }
+ if populated {
+ return false
+ }
+ if procs, err := os.ReadFile(filepath.Join(dir, "cgroup.procs")); err == nil && len(strings.Fields(string(procs))) > 0 {
+ return false
+ } else if err != nil && !errors.Is(err, os.ErrNotExist) {
+ return false
+ }
+ id, ok, err := pathIdentity(dir)
+ if err != nil || !ok {
+ return false
+ }
+ markKilled(id)
+ return true
+}
+
+// isClaimContainment reports whether an exec cgroup name is a claim
+// containment group's: carlos-exec@claim-<hex>.local, as
+// internal/host's claimContainmentName makes them.
+func isClaimContainment(name string) bool {
+ host := strings.TrimPrefix(name, execPrefix)
+ return strings.HasPrefix(host, "claim-") && strings.HasSuffix(host, ".local")
+}
diff --git a/internal/cgroup/recreate_linux_test.go b/internal/cgroup/recreate_linux_test.go
index 326de0449..6962bb7cb 100644
--- a/internal/cgroup/recreate_linux_test.go
+++ b/internal/cgroup/recreate_linux_test.go
@@ -372,3 +372,126 @@ func TestAKilledStrictRoleChildIsNotPinnedAgain(t *testing.T) {
t.Fatalf("err = %v, want ErrStrictRefused", err)
}
}
+
+// A killed parent that a strict handle still pins keeps everything below it
+// too: the reservation is checked before the walk touches a child (Task 19
+// fix round 1).
+func TestAPinnedKilledParentLosesNoChild(t *testing.T) {
+ forgetKillsAfter(t)
+ const host = "pinnedparent.example.com"
+ m, parent := genManager(t, host)
+ removed := fakeCgroupRmdir(t, nil)
+ fakeStart(t, func(*exec.Cmd) error { return nil })
+ sidecar := RoleChild("sidecar")
+ if _, err := m.StartChild(host, sidecar, TierDefault, Limit{}, buildTrue); err != nil {
+ t.Fatal(err)
+ }
+ killFixture(t, m, host, "", parent)
+ var st unix.Stat_t
+ if err := unix.Lstat(parent, &st); err != nil {
+ t.Fatal(err)
+ }
+ id := strictIdentity{uint64(st.Dev), st.Ino}
+ strictReservations.Lock()
+ strictReservations.held[id] = true
+ strictReservations.Unlock()
+ t.Cleanup(func() {
+ strictReservations.Lock()
+ delete(strictReservations.held, id)
+ strictReservations.Unlock()
+ })
+ if _, err := m.StartChild(host, sidecar, TierDefault, Limit{}, buildTrue); !errors.Is(err, ErrStrictState) {
+ t.Fatalf("start under a pinned killed parent = %v, want ErrStrictState", err)
+ }
+ if len(*removed) != 0 {
+ t.Fatalf("removed %v under a pinned parent", *removed)
+ }
+ if _, err := os.Stat(filepath.Join(parent, sidecar)); err != nil {
+ t.Fatalf("the pinned parent's child went: %v", err)
+ }
+}
+
+// retainedTree builds what an earlier run left under a delegated root:
+// carlos-exec@<host> with cgroup.events and cgroup.procs, and children
+// (name to events body) with the same files, empty.
+func retainedTree(t *testing.T, root, host, events, procs string, kids map[string]string) string {
+ t.Helper()
+ dir := filepath.Join(root, ExecName(host))
+ if err := os.Mkdir(dir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ writeFixture(t, filepath.Join(dir, "cgroup.events"), events)
+ writeFixture(t, filepath.Join(dir, "cgroup.procs"), procs)
+ writeFixture(t, filepath.Join(dir, "cgroup.controllers"), "cpu io memory pids\n")
+ for kid, body := range kids {
+ k := filepath.Join(dir, kid)
+ if err := os.Mkdir(k, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ writeFixture(t, filepath.Join(k, "cgroup.events"), body)
+ writeFixture(t, filepath.Join(k, "cgroup.procs"), "")
+ }
+ return dir
+}
+
+// Claim-entry mode (SetupRetaining) skips the sweep, and the kill marks die
+// with the process that made them, so a cgroup an earlier run killed and
+// kept would be cloned into again. The retaining setup marks every empty
+// exec cgroup (and every empty child of one) instead: the first start into
+// each makes it fresh with its limits written again. One that holds a
+// process, and a claim containment group, are left exactly as they are
+// (Task 19 fix round 1).
+func TestARetainingSetupMakesEmptyLeftoversFresh(t *testing.T) {
+ forgetKillsAfter(t)
+ quietOOM(t)
+ root := newDelegatedRoot(t)
+ empty := retainedTree(t, root, "worker.example.com", "populated 0\n", "", nil)
+ busy := retainedTree(t, root, "busy.example.com", "populated 1\n", "4242\n", nil)
+ // A generation parent whose live g3 survived, beside an empty sidecar
+ // child a KillSidecar left.
+ live := retainedTree(t, root, "live.example.com", "populated 1\n", "", map[string]string{"g3": "populated 1\n", "role-sidecar": "populated 0\n"})
+ claim := retainedTree(t, root, "claim-0123abcd.local", "populated 0\n", "", nil)
+ before := map[string]uint64{empty: inodeOf(t, empty), busy: inodeOf(t, busy), live: inodeOf(t, live),
+ filepath.Join(live, "g3"): inodeOf(t, filepath.Join(live, "g3")), filepath.Join(live, "role-sidecar"): inodeOf(t, filepath.Join(live, "role-sidecar"))}
+ m, err := SetupAtRetaining(root, t.Logf)
+ if err != nil {
+ t.Fatal(err)
+ }
+ removed := fakeCgroupRmdir(t, nil)
+ fakeStart(t, func(*exec.Cmd) error { return nil })
+
+ if _, err := m.Start("worker.example.com", TierDefault, buildTrue); err != nil {
+ t.Fatal(err)
+ }
+ if inodeOf(t, empty) == before[empty] {
+ t.Fatal("the first start reused an empty retained host cgroup")
+ }
+ if got := readFixture(t, filepath.Join(empty, "memory.max")); got != "536870912" {
+ t.Fatalf("recreated memory.max = %q, want the tier", got)
+ }
+
+ if _, err := m.Start("busy.example.com", TierDefault, buildTrue); err != nil {
+ t.Fatal(err)
+ }
+ if inodeOf(t, busy) != before[busy] {
+ t.Fatal("a retained cgroup holding a process was replaced")
+ }
+
+ if _, err := m.StartChild("live.example.com", RoleChild("sidecar"), TierDefault, Limit{}, buildTrue); err != nil {
+ t.Fatal(err)
+ }
+ if inodeOf(t, filepath.Join(live, "role-sidecar")) == before[filepath.Join(live, "role-sidecar")] {
+ t.Fatal("the sidecar respawn reused an empty retained role child")
+ }
+ if inodeOf(t, live) != before[live] || inodeOf(t, filepath.Join(live, "g3")) != before[filepath.Join(live, "g3")] {
+ t.Fatal("a populated parent or its live generation was replaced")
+ }
+
+ if id, ok, err := pathIdentity(claim); err != nil || !ok || wasKilled(id) {
+ t.Fatalf("a claim containment group was marked (ok=%v err=%v)", ok, err)
+ }
+ want := map[string]bool{empty: true, filepath.Join(live, "role-sidecar"): true}
+ if len(*removed) != 2 || !want[(*removed)[0]] || !want[(*removed)[1]] {
+ t.Fatalf("removed %v, want only the empty host cgroup and the empty role child", *removed)
+ }
+}
diff --git a/internal/cgroup/recreate_other.go b/internal/cgroup/recreate_other.go
index 84e0b84c7..94d7bed58 100644
--- a/internal/cgroup/recreate_other.go
+++ b/internal/cgroup/recreate_other.go
@@ -9,3 +9,5 @@ import "os"
func (m *Manager) freshen(string, string) error { return nil }
func removeEmptyDir(path string) error { return os.Remove(path) }
+
+func (m *Manager) seedKilled() {}