amadan
zdd-readiness: a cgroup that took cgroup.kill is recreated before anything respawns into it
amadan #272: on the dev box a clone3(CLONE_INTO_CGROUP) into a cgroup that took cgroup.kill is SIGKILLed at once. Every kill (Cleanup and strict) marks the cgroup by identity before writing cgroup.kill; the next Start, StartChild or PrepareStrict into that path removes the killed tree under a new per-host start lock and creates it as a first start does (same limits, same parent). A removal that fails, or a cgroup a strict handle still pins, refuses the start with ErrKilledCgroupKept. A killed strict child is never re-pinned. The kernel-gated TestClaimContainmentCleanupRespawn now also runs the real KillHost and KillSidecar respawn sequences.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
11 files changed,
+835
−9
internal/activator/claimspawn_kernel_linux_test.go+132 −1internal/activator/genbackend.go+9 −0internal/cgroup/cgroup.go+38 −1internal/cgroup/cleanup_linux.go+4 −0internal/cgroup/gen.go+7 −1internal/cgroup/recreate_linux.go+225 −0internal/cgroup/recreate_linux_test.go+374 −0internal/cgroup/recreate_other.go+11 −0internal/cgroup/strict_gen_linux.go+9 −2internal/cgroup/strict_gen_linux_test.go+13 −4internal/cgroup/strict_linux.go+13 −0
diff --git a/internal/activator/claimspawn_kernel_linux_test.go b/internal/activator/claimspawn_kernel_linux_test.go| index 6aedaad67..9041c16fa 100644 |
| --- a/internal/activator/claimspawn_kernel_linux_test.go |
| +++ b/internal/activator/claimspawn_kernel_linux_test.go |
| @@ -5,12 +5,19 @@ package activator |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| "os" |
| "os/exec" |
| + "path/filepath" |
| + "strconv" |
| + "strings" |
| + "syscall" |
| "testing" |
| "time" |
| "github.com/carlosframework/platform/internal/cgroup" |
| + "github.com/carlosframework/platform/internal/provision" |
| + "github.com/carlosframework/platform/internal/registry" |
| ) |
| func TestClaimSpawnKernelChild(t *testing.T) { |
| @@ -20,6 +27,14 @@ func TestClaimSpawnKernelChild(t *testing.T) { |
| time.Sleep(time.Minute) |
| } |
| +// TestClaimContainmentCleanupRespawn kills a cgroup and spawns into the |
| +// same path again, on a real kernel. On the dev box (amadan #272: arm64, |
| +// kernel 6.12.105, systemd 257) a clone3(CLONE_INTO_CGROUP) into a cgroup |
| +// that took cgroup.kill is SIGKILLed at once, so each respawn below must |
| +// land in a fresh cgroup and stay alive. Three sequences: the strict |
| +// primitive (PrepareStrict, then a cleanup handle's kill), a worker's |
| +// KillHost then its respawn into carlos-exec@<host>, and the legacy |
| +// sidecar's KillSidecar then its respawn into role-sidecar. |
| func TestClaimContainmentCleanupRespawn(t *testing.T) { |
| if os.Getenv("CARLOS_CLAIM_SPAWN_KERNEL") != "1" { |
| t.Skip("requires a disposable delegated user service") |
| @@ -28,7 +43,6 @@ func TestClaimContainmentCleanupRespawn(t *testing.T) { |
| if manager == nil { |
| t.Fatal("missing delegated cgroup") |
| } |
| - const host = "claimspawn.example" |
| dir := t.TempDir() |
| build := func() *exec.Cmd { |
| cmd := exec.Command(os.Args[0], "-test.run=^TestClaimSpawnKernelChild$", "-test.count=1") |
| @@ -36,6 +50,13 @@ func TestClaimContainmentCleanupRespawn(t *testing.T) { |
| cmd.Dir = dir |
| return cmd |
| } |
| + t.Run("strict primitive", func(t *testing.T) { claimStrictRespawn(t, manager, build) }) |
| + t.Run("KillHost then respawn", func(t *testing.T) { killHostRespawn(t, manager, build) }) |
| + t.Run("KillSidecar then respawn", func(t *testing.T) { killSidecarRespawn(t, manager, build) }) |
| +} |
| + |
| +func claimStrictRespawn(t *testing.T, manager *cgroup.Manager, build func() *exec.Cmd) { |
| + const host = "claimspawn.example" |
| for attempt := range 2 { |
| // This tests the cgroup primitive with a synthetic test child. It |
| // grants no claim launch authority and is not a tenant-isolation test. |
| @@ -84,3 +105,113 @@ func TestClaimContainmentCleanupRespawn(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// spawnStaysUp fails the test if cmd exits within a moment of its start: |
| +// the #272 symptom is a child SIGKILLed as it lands. It returns the |
| +// channel cmd.Wait's result arrives on. |
| +func spawnStaysUp(t *testing.T, label string, cmd *exec.Cmd) <-chan error { |
| + t.Helper() |
| + done := make(chan error, 1) |
| + go func() { done <- cmd.Wait() }() |
| + select { |
| + case err := <-done: |
| + t.Fatalf("%s: the child died as it landed: %v", label, err) |
| + case <-time.After(500 * time.Millisecond): |
| + } |
| + return done |
| +} |
| + |
| +// reaped waits for a killed child's exit. |
| +func reaped(t *testing.T, label string, done <-chan error) { |
| + t.Helper() |
| + select { |
| + case <-done: |
| + case <-time.After(5 * time.Second): |
| + t.Fatalf("%s: the killed child was not reaped", label) |
| + } |
| +} |
| + |
| +func cgroupInode(t *testing.T, path string) uint64 { |
| + t.Helper() |
| + var st syscall.Stat_t |
| + if err := syscall.Lstat(path, &st); err != nil { |
| + t.Fatalf("stat %s: %v", path, err) |
| + } |
| + return st.Ino |
| +} |
| + |
| +// killHostRespawn is a worker's sequence: spawned through spawnInCgroup into |
| +// carlos-exec@<host>, proven terminated by the real ExecBackend.KillHost, |
| +// then respawned into the same path. Each respawn gets a fresh cgroup with |
| +// the tier written again, and stays up. |
| +func killHostRespawn(t *testing.T, manager *cgroup.Manager, build func() *exec.Cmd) { |
| + const host = "killhost.claimspawn.example" |
| + b := &ExecBackend{Cgroups: manager} |
| + rt := registry.Route{Host: host} |
| + dir := filepath.Join(manager.Root(), cgroup.ExecName(host)) |
| + var last uint64 |
| + for attempt := range 3 { |
| + cmd, settle, err := b.spawnInCgroup(rt, build) |
| + if err != nil { |
| + t.Fatalf("spawn %d: %v", attempt, err) |
| + } |
| + done := spawnStaysUp(t, fmt.Sprintf("spawn %d", attempt), cmd) |
| + ino := cgroupInode(t, dir) |
| + if attempt > 0 && ino == last { |
| + t.Fatalf("spawn %d reused the killed host cgroup", attempt) |
| + } |
| + last = ino |
| + if got, err := os.ReadFile(filepath.Join(dir, "memory.max")); err != nil || strings.TrimSpace(string(got)) != strconv.FormatInt(cgroup.TierDefault.MemoryMax, 10) { |
| + t.Fatalf("spawn %d: memory.max = %q, %v; want the tier", attempt, got, err) |
| + } |
| + ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second) |
| + err = b.KillHost(ctx, rt) |
| + cancel() |
| + if err != nil { |
| + t.Fatalf("KillHost %d: %v", attempt, err) |
| + } |
| + reaped(t, fmt.Sprintf("spawn %d", attempt), done) |
| + settle() |
| + } |
| + b.cleanupHost(host) |
| +} |
| + |
| +// killSidecarRespawn is the legacy sidecar's sequence under containment: a |
| +// flat row's sidecar spawned into carlos-exec@<host>/role-sidecar, proven |
| +// gone by the real ExecBackend.KillSidecar, then respawned. Each respawn gets |
| +// a fresh role-sidecar under the same parent, and stays up. |
| +func killSidecarRespawn(t *testing.T, manager *cgroup.Manager, build func() *exec.Cmd) { |
| + const host = "killsidecar.claimspawn.example" |
| + b := &ExecBackend{Cgroups: manager, GenContainment: true, DataDir: t.TempDir(), OptDir: t.TempDir()} |
| + rt := registry.Route{Host: host, Kind: "instance", DBPath: provision.DefaultDBPath(b.DataDir, host), |
| + Socket: filepath.Join(provision.SocketDir, host+".sock")} |
| + parent := filepath.Join(manager.Root(), cgroup.ExecName(host)) |
| + child := cgroup.RoleChild("sidecar") |
| + var last, parentIno uint64 |
| + for attempt := range 3 { |
| + cmd, _, err := b.spawnInCgroupAt(rt, child, false, build) |
| + if err != nil { |
| + t.Fatalf("spawn %d: %v", attempt, err) |
| + } |
| + done := spawnStaysUp(t, fmt.Sprintf("sidecar spawn %d", attempt), cmd) |
| + ino := cgroupInode(t, filepath.Join(parent, child)) |
| + if attempt > 0 && ino == last { |
| + t.Fatalf("sidecar spawn %d reused the killed role-sidecar", attempt) |
| + } |
| + if p := cgroupInode(t, parent); attempt > 0 && p != parentIno { |
| + t.Fatalf("sidecar spawn %d replaced the parent", attempt) |
| + } else { |
| + parentIno = p |
| + } |
| + last = ino |
| + ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second) |
| + err = b.KillSidecar(ctx, rt) |
| + cancel() |
| + if err != nil { |
| + t.Fatalf("KillSidecar %d: %v", attempt, err) |
| + } |
| + reaped(t, fmt.Sprintf("sidecar spawn %d", attempt), done) |
| + } |
| + manager.RemoveChild(host, child) |
| + manager.CleanupHost(host) |
| +} |
diff --git a/internal/activator/genbackend.go b/internal/activator/genbackend.go| index 7025d74bd..01c2fa852 100644 |
| --- a/internal/activator/genbackend.go |
| +++ b/internal/activator/genbackend.go |
| @@ -396,6 +396,10 @@ func (b *SplitBackend) StartSidecarGen(ctx context.Context, rt registry.Route, g |
| // to whatever SIGTERM left). Without containment the sidecar shares the |
| // host cgroup with the instance, and the terminal stop's evidence on that |
| // cgroup is the proof (errNoGenContainment, as KillGen answers). |
| +// |
| +// The role child is kept after the kill, and a clone3 into a cgroup that |
| +// took cgroup.kill is SIGKILLed at once (amadan #272), so the next sidecar |
| +// start (StartChildGated) removes it and makes it fresh first, or refuses. |
| func (b *ExecBackend) KillSidecar(ctx context.Context, rt registry.Route) error { |
| switch p := b.pathsFor(rt); p.layout { |
| case LayoutNested: |
| @@ -487,6 +491,11 @@ func (b *SplitBackend) GenPaths(rt registry.Route, n uint64) (Gen, bool) { |
| // through spawnInCgroup into carlos-exec@<host>, whatever the layout says, |
| // so a nested-looking worker is proven there too. An absent cgroup is |
| // empty. No cgroup delegation at all is an unproven kill. |
| +// |
| +// The host cgroup is kept after the kill, and a clone3 into a cgroup that |
| +// took cgroup.kill is SIGKILLed at once (amadan #272), so the worker's next |
| +// start (StartGated) removes it and makes it fresh first, or refuses. That |
| +// holds with the flag off too, where only Task 15's F6 path reaches here. |
| func (b *ExecBackend) KillHost(ctx context.Context, rt registry.Route) error { |
| if b.Cgroups == nil { |
| return cgroup.ErrNoDelegation |
diff --git a/internal/cgroup/cgroup.go b/internal/cgroup/cgroup.go| index de40a60e8..85393b496 100644 |
| --- a/internal/cgroup/cgroup.go |
| +++ b/internal/cgroup/cgroup.go |
| @@ -22,6 +22,7 @@ import ( |
| "path/filepath" |
| "strconv" |
| "strings" |
| + "sync" |
| "sync/atomic" |
| ) |
| @@ -147,6 +148,28 @@ type Manager struct { |
| // started normally then moved by PID. A filter is static for the life of |
| // the process, so retrying the syscall on every spawn buys nothing. |
| fallback atomic.Bool |
| + // locks holds each host's start lock (hostLock), made on first use. |
| + locksMu sync.Mutex |
| + locks map[string]*sync.Mutex |
| +} |
| + |
| +// hostLock is host's start lock. A start holds it while it checks for, and |
| +// removes, a cgroup of the host that took cgroup.kill (freshen, amadan |
| +// #272), so two starts of one host never both remove one. It is the |
| +// innermost lock: nothing is taken while it is held, and it is never held |
| +// across a before-start hook or a clone3. |
| +func (m *Manager) hostLock(host string) *sync.Mutex { |
| + m.locksMu.Lock() |
| + defer m.locksMu.Unlock() |
| + if m.locks == nil { |
| + m.locks = make(map[string]*sync.Mutex) |
| + } |
| + mu, ok := m.locks[host] |
| + if !ok { |
| + mu = &sync.Mutex{} |
| + m.locks[host] = mu |
| + } |
| + return mu |
| } |
| // Root is the delegated subtree's path — the sampler's third scan root. |
| @@ -263,6 +286,12 @@ func (m *Manager) sweepStale() { |
| } |
| } |
| +// ErrKilledCgroupKept is a start refused because the cgroup it would spawn |
| +// into took cgroup.kill and could not be removed and made fresh first |
| +// (amadan #272). Spawning into it anyway would have the kernel SIGKILL the |
| +// child at once, so nothing is started. |
| +var ErrKilledCgroupKept = errors.New("cgroup: a cgroup that took cgroup.kill could not be made fresh before a spawn") |
| + |
| // startCmd is Start's spawn seam — a package var so the clone3 refusal |
| // path is testable without a seccomp filter in the loop. |
| var startCmd = func(cmd *exec.Cmd) error { return cmd.Start() } |
| @@ -304,10 +333,18 @@ func (m *Manager) Start(host string, limit Limit, build func() *exec.Cmd) (*exec |
| // preparation as a failed start does (the cgroup dir this spawn would have |
| // used is removed if empty), and comes back as before's own error, never |
| // joined with an earlier start's. A nil before is Start exactly. |
| +// |
| +// A host cgroup that took cgroup.kill (KillHost keeps it) is removed and |
| +// made again before anything is spawned into it, and a start whose killed |
| +// cgroup will not go is refused with ErrKilledCgroupKept, never run |
| +// unaccounted (amadan #272, freshen). |
| func (m *Manager) StartGated(host string, limit Limit, build func() *exec.Cmd, before func() error) (*exec.Cmd, error) { |
| if m == nil { |
| return startPlain(build, before) |
| } |
| + if err := m.freshen(host, m.hostDir(host)); err != nil { |
| + return nil, err |
| + } |
| return m.startIn(host, m.hostDir(host), limit, build, before) |
| } |
| @@ -508,5 +545,5 @@ func (m *Manager) CleanupHost(host string) { |
| if m == nil { |
| return |
| } |
| - _ = os.Remove(m.hostDir(host)) |
| + _ = removeEmptyDir(m.hostDir(host)) |
| } |
diff --git a/internal/cgroup/cleanup_linux.go b/internal/cgroup/cleanup_linux.go| index f11b99e96..64bc29806 100644 |
| --- a/internal/cgroup/cleanup_linux.go |
| +++ b/internal/cgroup/cleanup_linux.go |
| @@ -313,6 +313,10 @@ func (c *Cleanup) KillAndEmpty(ctx context.Context) error { |
| if err := p.identityOK(); err != nil { |
| return err |
| } |
| + // Marked first: the next start into this cgroup makes it fresh, since |
| + // the kernel kills anything cloned into one that took cgroup.kill |
| + // (amadan #272, recreate_linux.go). |
| + markKilled(p.identity) |
| if err := strictWrite(p.kill, "1"); err != nil { |
| return err |
| } |
diff --git a/internal/cgroup/gen.go b/internal/cgroup/gen.go| index faf8c3873..69379ccf4 100644 |
| --- a/internal/cgroup/gen.go |
| +++ b/internal/cgroup/gen.go |
| @@ -208,6 +208,12 @@ func (m *Manager) StartChildGated(host, child string, parent, own Limit, build f |
| if m == nil { |
| return startPlain(build, before) |
| } |
| + // A parent or child that took cgroup.kill is removed and made again |
| + // first (amadan #272): KillSidecar keeps role-sidecar, and KillHost keeps |
| + // the parent. ensureParent and startIn then build them as on a cold start. |
| + if err := m.freshen(host, filepath.Join(m.hostDir(host), child)); err != nil { |
| + return nil, err |
| + } |
| if err := m.ensureParent(host, parent); err != nil { |
| return nil, err |
| } |
| @@ -249,5 +255,5 @@ func (m *Manager) RemoveChild(host, child string) { |
| if m == nil || !isChildName(child) { |
| return |
| } |
| - _ = os.Remove(filepath.Join(m.hostDir(host), child)) |
| + _ = removeEmptyDir(filepath.Join(m.hostDir(host), child)) |
| } |
diff --git a/internal/cgroup/recreate_linux.go b/internal/cgroup/recreate_linux.go| new file mode 100644 |
| index 000000000..f7c3c2480 |
| --- /dev/null |
| +++ b/internal/cgroup/recreate_linux.go |
| @@ -0,0 +1,225 @@ |
| +//go:build linux |
| + |
| +package cgroup |
| + |
| +import ( |
| + "errors" |
| + "fmt" |
| + "os" |
| + "path/filepath" |
| + "sync" |
| + |
| + "golang.org/x/sys/unix" |
| +) |
| + |
| +// A cgroup that took cgroup.kill is never spawned into again (amadan #272). |
| +// Found on the dev box (arm64, kernel 6.12.105, systemd 257): once |
| +// cgroup.kill has been written to a cgroup, every later |
| +// clone3(CLONE_INTO_CGROUP) into that same cgroup is SIGKILLed at once, |
| +// while a cgroup made fresh at the same path is not. Generation kills |
| +// (KillGen, CloseRemove) remove their child and never meet it. The kills |
| +// that keep their cgroup are a worker's host cgroup (KillHost) and the |
| +// legacy sidecar's role child (KillSidecar); a strict group's own kill |
| +// keeps its directory too. |
| +// |
| +// Every kill marks the cgroup it is about to kill, by identity (device and |
| +// inode), before it writes cgroup.kill. The next start into that path |
| +// (Start, StartChild, PrepareStrict) removes the killed cgroup, and any |
| +// empty cgroups left below it, and then creates it as a first start does, |
| +// so it gets the same limits and controller settings from the same code, |
| +// under the same parent. The recreation happens at the start, not right |
| +// after the kill, for three reasons. The start is the one place that knows |
| +// what the cgroup must carry (its tier, or the parent's), so a recreation |
| +// cannot drift from a creation. One check there covers every kill, a strict |
| +// one included, without each kill site having to remember it. And a strict |
| +// handle may still pin the killed directory when the kill returns (it is the |
| +// handle's identity), so removing it then would strand the handle; at the |
| +// start a pinned cgroup is refused instead, and removed once the handle lets |
| +// go. |
| +// |
| +// A removal that fails (EBUSY: something is in it again) refuses the start |
| +// with ErrKilledCgroupKept. The mark stays, so every later start refuses too |
| +// until the cgroup can be removed. Marks are by identity, so a cgroup made |
| +// fresh at the same path is never mistaken for the killed one, and a |
| +// cgroup someone else removed leaves only a harmless stale mark (cgroupfs |
| +// inode numbers are not reused). |
| +// |
| +// The marks live in this process only. An agent that restarts with Setup |
| +// sweeps the empty carlos-exec@ directories a previous run left, killed |
| +// ones included, so it starts with fresh cgroups. |
| + |
| +// killedCgroups is every cgroup this process wrote cgroup.kill to that may |
| +// still exist, by identity. Package-wide, not per Manager: a strict handle's |
| +// kill has no Manager, and an identity names one cgroup on the box. |
| +var killedCgroups = struct { |
| + sync.Mutex |
| + ids map[strictIdentity]bool |
| +}{ids: make(map[strictIdentity]bool)} |
| + |
| +// markKilled records that id is about to take cgroup.kill. It is called |
| +// before the write, so a kill that fails half way still counts. |
| +func markKilled(id strictIdentity) { |
| + killedCgroups.Lock() |
| + killedCgroups.ids[id] = true |
| + killedCgroups.Unlock() |
| +} |
| + |
| +func forgetKilled(id strictIdentity) { |
| + killedCgroups.Lock() |
| + delete(killedCgroups.ids, id) |
| + killedCgroups.Unlock() |
| +} |
| + |
| +func wasKilled(id strictIdentity) bool { |
| + killedCgroups.Lock() |
| + defer killedCgroups.Unlock() |
| + return killedCgroups.ids[id] |
| +} |
| + |
| +// anyKilled lets a start skip every stat while nothing was ever killed: on |
| +// a box that never kills a kept cgroup (the flag off, before Task 15's F6 |
| +// path) the start path is the one it always was. |
| +func anyKilled() bool { |
| + killedCgroups.Lock() |
| + defer killedCgroups.Unlock() |
| + return len(killedCgroups.ids) > 0 |
| +} |
| + |
| +func reservedStrict(id strictIdentity) bool { |
| + strictReservations.Lock() |
| + defer strictReservations.Unlock() |
| + return strictReservations.held[id] |
| +} |
| + |
| +// rmdirCgroup removes one cgroup directory. A seam so the temp-dir fixture, |
| +// whose interface files are ordinary files, can stand in for cgroupfs. |
| +var rmdirCgroup = func(path string) error { return unix.Rmdir(path) } |
| + |
| +// pathIdentity is the identity of the directory at path, not following a |
| +// symlink. ok is false when nothing is there. |
| +func pathIdentity(path string) (id strictIdentity, ok bool, err error) { |
| + var st unix.Stat_t |
| + if err := unix.Lstat(path, &st); err != nil { |
| + if errors.Is(err, unix.ENOENT) { |
| + return strictIdentity{}, false, nil |
| + } |
| + return strictIdentity{}, false, err |
| + } |
| + return strictIdentity{uint64(st.Dev), st.Ino}, true, nil |
| +} |
| + |
| +// freshen makes sure no start into dir (the host's cgroup, or one of its |
| +// children) lands in a cgroup that took a kill: the host's cgroup first, |
| +// since a kill there killed everything below it, then dir. It runs under |
| +// the host's start lock (hostLock), so two starts of one host never both |
| +// remove: the first removes the killed cgroup, and the second finds the |
| +// fresh one, whose identity took no kill, and keeps it. The lock is held |
| +// only for the check and the removal, not across the start's clone3: a |
| +// start's own directory is never removed by another start, because only a |
| +// killed identity is removed and every start checks under this lock first. |
| +// What it does not order is a kill landing while a start is between its |
| +// check and its clone3; the activator never does that (a kept cgroup is |
| +// killed only once its leader has exited, and nothing of that host or role |
| +// is started until the proof returns). |
| +func (m *Manager) freshen(host, dir string) error { |
| + if !anyKilled() { |
| + return nil |
| + } |
| + mu := m.hostLock(host) |
| + mu.Lock() |
| + defer mu.Unlock() |
| + hostDir := m.hostDir(host) |
| + if err := m.freshenOne(host, hostDir); err != nil { |
| + return err |
| + } |
| + if dir == hostDir { |
| + return nil |
| + } |
| + return m.freshenOne(host, dir) |
| +} |
| + |
| +func (m *Manager) freshenOne(host, dir string) error { |
| + id, ok, err := pathIdentity(dir) |
| + if err != nil { |
| + // Unknown is not fresh. |
| + return fmt.Errorf("%w: %s: %w", ErrKilledCgroupKept, dir, err) |
| + } |
| + if !ok || !wasKilled(id) { |
| + return nil |
| + } |
| + groups := 0 |
| + if err := removeKilledTree(dir, 0, &groups); err != nil { |
| + m.logf("cgroup: %s: %s took cgroup.kill and could not be removed, so nothing is started in it: %v", host, filepath.Base(dir), err) |
| + return fmt.Errorf("%w: %s: %w", ErrKilledCgroupKept, dir, err) |
| + } |
| + m.logf("cgroup: %s: %s took cgroup.kill; removed so the next spawn gets a fresh cgroup", host, filepath.Base(dir)) |
| + return nil |
| +} |
| + |
| +// removeKilledTree removes dir and every cgroup below it, deepest first: |
| +// rmdir refuses a cgroup with child cgroups, empty or not, and a tenant |
| +// 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. |
| +func removeKilledTree(dir string, depth int, groups *int) error { |
| + entries, err := os.ReadDir(dir) |
| + if errors.Is(err, os.ErrNotExist) { |
| + return nil |
| + } |
| + if err != nil { |
| + return err |
| + } |
| + for _, e := range entries { |
| + if !e.IsDir() { |
| + continue |
| + } |
| + if depth+1 > maxCleanupDepth { |
| + return fmt.Errorf("%w: cgroup tree deeper than %d", ErrStrictRefused, maxCleanupDepth) |
| + } |
| + if *groups++; *groups > maxCleanupGroups { |
| + return fmt.Errorf("%w: more than %d cgroups below one host", ErrStrictRefused, maxCleanupGroups) |
| + } |
| + if err := removeKilledTree(filepath.Join(dir, e.Name()), depth+1, groups); err != nil { |
| + 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 |
| + } |
| + forgetKilled(id) |
| + return nil |
| +} |
| + |
| +// removeEmptyDir is CleanupHost's and RemoveChild's os.Remove, which also |
| +// drops a kill mark the removed cgroup carried, so the set does not grow. |
| +func removeEmptyDir(path string) error { |
| + id, ok, _ := pathIdentity(path) |
| + err := os.Remove(path) |
| + if err == nil && ok { |
| + forgetKilled(id) |
| + } |
| + return err |
| +} |
| + |
| +// refuseKilledChild is pinChild's check on a child that already exists: a |
| +// strict child that took a kill is never pinned again for a spawn. Generation |
| +// numbers are never reused, and no production path re-pins a role child, so |
| +// a refusal costs nothing there; it keeps the rule whole. |
| +func refuseKilledChild(parent *Prepared, name string) error { |
| + var st unix.Stat_t |
| + if err := unix.Fstatat(int(parent.dir.Fd()), name, &st, unix.AT_SYMLINK_NOFOLLOW); err != nil { |
| + return ErrStrictRefused |
| + } |
| + if wasKilled(strictIdentity{uint64(st.Dev), st.Ino}) { |
| + return ErrStrictRefused |
| + } |
| + return nil |
| +} |
diff --git a/internal/cgroup/recreate_linux_test.go b/internal/cgroup/recreate_linux_test.go| new file mode 100644 |
| index 000000000..326de0449 |
| --- /dev/null |
| +++ b/internal/cgroup/recreate_linux_test.go |
| @@ -0,0 +1,374 @@ |
| +//go:build linux |
| + |
| +package cgroup |
| + |
| +import ( |
| + "context" |
| + "errors" |
| + "os" |
| + "os/exec" |
| + "path/filepath" |
| + "testing" |
| + "time" |
| + |
| + "golang.org/x/sys/unix" |
| +) |
| + |
| +// amadan #272: once cgroup.kill has been written to a cgroup, every later |
| +// clone3(CLONE_INTO_CGROUP) into that same cgroup is SIGKILLed at once. The |
| +// fixture has no kernel behind it, so these tests pin the bookkeeping: a |
| +// cgroup that took a kill is removed and made fresh (a new inode, its limit |
| +// files written again) before the next spawn into its path, and a removal |
| +// that fails refuses the spawn. |
| + |
| +// fakeCgroupRmdir stands in for rmdir on cgroupfs, where the interface files |
| +// do not count against it: the fixture's are ordinary files, so they go |
| +// first. fail, while non-nil, is returned instead. It records each removal. |
| +func fakeCgroupRmdir(t *testing.T, fail *error) *[]string { |
| + t.Helper() |
| + var removed []string |
| + orig := rmdirCgroup |
| + rmdirCgroup = func(path string) error { |
| + if fail != nil && *fail != nil { |
| + return *fail |
| + } |
| + entries, err := os.ReadDir(path) |
| + if err != nil { |
| + return err |
| + } |
| + for _, e := range entries { |
| + if !e.IsDir() { |
| + if err := os.Remove(filepath.Join(path, e.Name())); err != nil { |
| + return err |
| + } |
| + } |
| + } |
| + if err := unix.Rmdir(path); err != nil { |
| + return err |
| + } |
| + removed = append(removed, path) |
| + return nil |
| + } |
| + t.Cleanup(func() { rmdirCgroup = orig }) |
| + return &removed |
| +} |
| + |
| +// forgetKillsAfter clears the kill marks a test leaves, so a later test's |
| +// temp dir can never meet one. |
| +func forgetKillsAfter(t *testing.T) { |
| + t.Helper() |
| + t.Cleanup(func() { |
| + killedCgroups.Lock() |
| + clear(killedCgroups.ids) |
| + killedCgroups.Unlock() |
| + }) |
| +} |
| + |
| +func inodeOf(t *testing.T, path string) uint64 { |
| + t.Helper() |
| + var st unix.Stat_t |
| + if err := unix.Lstat(path, &st); err != nil { |
| + t.Fatalf("stat %s: %v", path, err) |
| + } |
| + return st.Ino |
| +} |
| + |
| +// killFixture gives dir the files a cleanup handle reads, empty, and writes |
| +// cgroup.kill through a cleanup handle on host (child "" is the host's own |
| +// cgroup), as KillHost and KillSidecar do. |
| +func killFixture(t *testing.T, m *Manager, host, child, dir string) { |
| + t.Helper() |
| + for name, body := range map[string]string{"cgroup.events": "populated 0\n", "cgroup.procs": "", "cgroup.kill": ""} { |
| + writeFixture(t, filepath.Join(dir, name), body) |
| + } |
| + c, err := m.openCleanupAt(host, child, func(*os.File) error { return nil }) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + ctx, cancel := context.WithTimeout(t.Context(), time.Second) |
| + defer cancel() |
| + if err := c.KillAndEmpty(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := c.Close(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got := readFixture(t, filepath.Join(dir, "cgroup.kill")); got != "1" { |
| + t.Fatalf("cgroup.kill = %q, want 1", got) |
| + } |
| +} |
| + |
| +// A worker's host cgroup (KillHost's) is killed and kept. The next start |
| +// into it removes it and creates it again, with its limit files written as |
| +// a first creation writes them, and a start that follows no kill removes |
| +// nothing. |
| +func TestAKilledHostCgroupIsRecreatedBeforeTheNextStart(t *testing.T) { |
| + forgetKillsAfter(t) |
| + quietOOM(t) |
| + m, err := SetupAt(newDelegatedRoot(t), t.Logf) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + removed := fakeCgroupRmdir(t, nil) |
| + calls := fakeStart(t, func(*exec.Cmd) error { return nil }) |
| + const host = "worker.example.com" |
| + dir := m.hostDir(host) |
| + for range 2 { |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if len(*removed) != 0 { |
| + t.Fatalf("starts that followed no kill removed %v", *removed) |
| + } |
| + before := inodeOf(t, dir) |
| + killFixture(t, m, host, "", dir) |
| + // A limit file left behind would say nothing about the recreation; |
| + // scribble on it so only a fresh write can restore it. |
| + writeFixture(t, filepath.Join(dir, "memory.max"), "max") |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(*calls) != 3 { |
| + t.Fatalf("started %d times, want 3", len(*calls)) |
| + } |
| + if len(*removed) != 1 || (*removed)[0] != dir { |
| + t.Fatalf("removed %v, want only %s", *removed, dir) |
| + } |
| + if after := inodeOf(t, dir); after == before { |
| + t.Fatalf("the respawn reused the killed cgroup (inode %d)", before) |
| + } |
| + for file, want := range map[string]string{ |
| + "memory.max": "536870912", |
| + "memory.high": "483183819", // HighOf: max/10*9 |
| + "memory.swap.max": "536870912", |
| + "memory.oom.group": "1", |
| + } { |
| + if got := readFixture(t, filepath.Join(dir, file)); got != want { |
| + t.Errorf("%s = %q, want %q", file, got, want) |
| + } |
| + } |
| + // The fresh cgroup took no kill: the next start keeps it. |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(*removed) != 1 { |
| + t.Fatalf("a start after the recreation removed again: %v", *removed) |
| + } |
| +} |
| + |
| +// A killed cgroup that will not go (EBUSY: something is in it again) is not |
| +// proven fresh, so the start refuses and execs nothing. Once it can be |
| +// removed, the next start proceeds into a fresh one. |
| +func TestAKilledCgroupThatCannotBeRemovedRefusesTheNextStart(t *testing.T) { |
| + forgetKillsAfter(t) |
| + quietOOM(t) |
| + m, err := SetupAt(newDelegatedRoot(t), t.Logf) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + fail := error(unix.EBUSY) |
| + removed := fakeCgroupRmdir(t, &fail) |
| + calls := fakeStart(t, func(*exec.Cmd) error { return nil }) |
| + const host = "worker.example.com" |
| + dir := m.hostDir(host) |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + before := inodeOf(t, dir) |
| + killFixture(t, m, host, "", dir) |
| + built := 0 |
| + cmd, err := m.Start(host, TierDefault, func() *exec.Cmd { built++; return buildTrue() }) |
| + if !errors.Is(err, ErrKilledCgroupKept) || !errors.Is(err, unix.EBUSY) || cmd != nil { |
| + t.Fatalf("start into a killed cgroup that would not go = %v, %v; want ErrKilledCgroupKept (EBUSY)", cmd, err) |
| + } |
| + if built != 0 || len(*calls) != 1 { |
| + t.Fatalf("the refused start built %d command(s) and started %d time(s) in all, want 0 and 1", built, len(*calls)) |
| + } |
| + if inodeOf(t, dir) != before { |
| + t.Fatal("the refused start replaced the cgroup") |
| + } |
| + // Still refused while it will not go: the mark outlives the failure. |
| + if _, err := m.Start(host, TierDefault, buildTrue); !errors.Is(err, ErrKilledCgroupKept) { |
| + t.Fatalf("second start = %v, want ErrKilledCgroupKept", err) |
| + } |
| + fail = nil |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(*removed) != 1 || inodeOf(t, dir) == before { |
| + t.Fatalf("removed %v; the start after the removal succeeded must be in a fresh cgroup", *removed) |
| + } |
| +} |
| + |
| +// The legacy sidecar's role child (KillSidecar's) is killed and kept. The |
| +// respawn gets a fresh role-sidecar under the very same parent, whose limit |
| +// and accounting are left as they were. |
| +func TestAKilledRoleSidecarIsRecreatedUnderTheSameParent(t *testing.T) { |
| + forgetKillsAfter(t) |
| + const host = "sc.example.com" |
| + m, parent := genManager(t, host) |
| + removed := fakeCgroupRmdir(t, nil) |
| + fakeStart(t, func(*exec.Cmd) error { return nil }) |
| + child := RoleChild("sidecar") |
| + dir := filepath.Join(parent, child) |
| + if _, err := m.StartChild(host, child, TierDefault, Limit{}, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + parentInode, childInode := inodeOf(t, parent), inodeOf(t, dir) |
| + killFixture(t, m, host, child, dir) |
| + writeFixture(t, filepath.Join(parent, "memory.max"), "1073741824") // a raise the respawn must not undo |
| + writeFixture(t, filepath.Join(parent, "cgroup.events"), "populated 1\n") |
| + if _, err := m.StartChild(host, child, TierDefault, Limit{}, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(*removed) != 1 || (*removed)[0] != dir { |
| + t.Fatalf("removed %v, want only %s", *removed, dir) |
| + } |
| + if inodeOf(t, dir) == childInode { |
| + t.Fatal("the sidecar respawn reused the killed role child") |
| + } |
| + if inodeOf(t, parent) != parentInode { |
| + t.Fatal("the sidecar respawn replaced the parent") |
| + } |
| + if got := readFixture(t, filepath.Join(parent, "memory.max")); got != "1073741824" { |
| + t.Fatalf("parent memory.max = %q, the respawn touched it", got) |
| + } |
| + if _, err := os.Stat(filepath.Join(dir, "memory.max")); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("a role child carries no limit of its own: %v", err) |
| + } |
| +} |
| + |
| +// KillHost on a generation parent kills everything under it. The next start |
| +// of any child removes the whole killed tree, children first, and builds |
| +// the parent again as a cold start does: controllers delegated, the tier |
| +// written with memory.oom.group=0. |
| +func TestAKilledParentIsRecreatedWithItsChildren(t *testing.T) { |
| + forgetKillsAfter(t) |
| + const host = "gp.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) |
| + } |
| + if _, err := m.StartChild(host, "g4", TierDefault, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + parentInode := inodeOf(t, parent) |
| + killFixture(t, m, host, "", parent) |
| + // The fixture's parent would lose its controller list with it; the |
| + // kernel lists them on every new cgroup, so the fake rmdir's recreation |
| + // needs it back. |
| + orig := rmdirCgroup |
| + rmdirCgroup = func(path string) error { |
| + if err := orig(path); err != nil { |
| + return err |
| + } |
| + if path == parent { |
| + if err := os.Mkdir(parent, 0o755); err != nil { |
| + return err |
| + } |
| + writeFixture(t, filepath.Join(parent, "cgroup.controllers"), "cpu io memory pids\n") |
| + } |
| + return nil |
| + } |
| + t.Cleanup(func() { rmdirCgroup = orig }) |
| + if _, err := m.StartChild(host, sidecar, TierDefault, Limit{}, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + want := map[string]bool{filepath.Join(parent, sidecar): true, filepath.Join(parent, "g4"): true} |
| + if len(*removed) != 3 || !want[(*removed)[0]] || !want[(*removed)[1]] || (*removed)[2] != parent { |
| + t.Fatalf("removed %v, want both children then %s", *removed, parent) |
| + } |
| + if inodeOf(t, parent) == parentInode { |
| + t.Fatal("the respawn reused the killed parent") |
| + } |
| + if _, err := os.Stat(filepath.Join(parent, "g4")); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("the killed generation child survived: %v", err) |
| + } |
| + for file, want := range map[string]string{ |
| + "cgroup.subtree_control": "+cpu +memory +io +pids", |
| + "memory.max": "536870912", |
| + "memory.oom.group": "0", |
| + } { |
| + if got := readFixture(t, filepath.Join(parent, file)); got != want { |
| + t.Errorf("parent %s = %q, want %q", file, got, want) |
| + } |
| + } |
| +} |
| + |
| +// A killed cgroup that a strict handle still pins (its reservation is |
| +// held) is not removed under it: removing it would strand the handle, whose |
| +// identity is that directory. The start refuses until the handle lets go. |
| +func TestAKilledCgroupAStrictHandleHoldsIsNotRemoved(t *testing.T) { |
| + forgetKillsAfter(t) |
| + quietOOM(t) |
| + m, err := SetupAt(newDelegatedRoot(t), t.Logf) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + removed := fakeCgroupRmdir(t, nil) |
| + fakeStart(t, func(*exec.Cmd) error { return nil }) |
| + const host = "pinned.example.com" |
| + dir := m.hostDir(host) |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + killFixture(t, m, host, "", dir) |
| + var st unix.Stat_t |
| + if err := unix.Lstat(dir, &st); err != nil { |
| + t.Fatal(err) |
| + } |
| + id := strictIdentity{uint64(st.Dev), st.Ino} |
| + strictReservations.Lock() |
| + strictReservations.held[id] = true |
| + strictReservations.Unlock() |
| + release := func() { |
| + strictReservations.Lock() |
| + delete(strictReservations.held, id) |
| + strictReservations.Unlock() |
| + } |
| + t.Cleanup(release) |
| + if _, err := m.Start(host, TierDefault, buildTrue); !errors.Is(err, ErrStrictState) || !errors.Is(err, ErrKilledCgroupKept) { |
| + t.Fatalf("start into a pinned killed cgroup = %v, want ErrKilledCgroupKept and ErrStrictState", err) |
| + } |
| + if len(*removed) != 0 { |
| + t.Fatalf("removed %v under a strict handle", *removed) |
| + } |
| + release() |
| + if _, err := m.Start(host, TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(*removed) != 1 { |
| + t.Fatalf("removed %v once the handle let go, want the killed cgroup", *removed) |
| + } |
| +} |
| + |
| +// A strict role child that took a kill is never pinned again for a spawn. |
| +func TestAKilledStrictRoleChildIsNotPinnedAgain(t *testing.T) { |
| + forgetKillsAfter(t) |
| + p := strictFixture(t) |
| + makeRoleFixture(t, p, "seed", "populated 0\n") |
| + child, err := p.PrepareRoleChild("seed") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + ctx, cancel := context.WithTimeout(t.Context(), time.Second) |
| + defer cancel() |
| + if err := child.KillAndEmpty(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := child.Close(); err != nil { |
| + t.Fatal(err) |
| + } |
| + again, err := p.PrepareRoleChild("seed") |
| + if err == nil { |
| + again.Close() |
| + t.Fatal("a killed role child was pinned again for a spawn") |
| + } |
| + if !errors.Is(err, ErrStrictRefused) { |
| + t.Fatalf("err = %v, want ErrStrictRefused", err) |
| + } |
| +} |
diff --git a/internal/cgroup/recreate_other.go b/internal/cgroup/recreate_other.go| new file mode 100644 |
| index 000000000..84e0b84c7 |
| --- /dev/null |
| +++ b/internal/cgroup/recreate_other.go |
| @@ -0,0 +1,11 @@ |
| +//go:build !linux |
| + |
| +package cgroup |
| + |
| +import "os" |
| + |
| +// No cgroup is ever killed off linux (Cleanup is unsupported there), so no |
| +// start has a killed cgroup to make fresh (recreate_linux.go). |
| +func (m *Manager) freshen(string, string) error { return nil } |
| + |
| +func removeEmptyDir(path string) error { return os.Remove(path) } |
diff --git a/internal/cgroup/strict_gen_linux.go b/internal/cgroup/strict_gen_linux.go| index 923532316..ce71653f6 100644 |
| --- a/internal/cgroup/strict_gen_linux.go |
| +++ b/internal/cgroup/strict_gen_linux.go |
| @@ -42,8 +42,15 @@ func pinChild(parent *Prepared, name string, beforeMkdir func() error, allowExis |
| return nil, err |
| } |
| } |
| - if err := unix.Mkdirat(int(parent.dir.Fd()), name, 0755); err != nil && (!allowExisting || !errors.Is(err, unix.EEXIST)) { |
| - return nil, ErrStrictRefused |
| + if err := unix.Mkdirat(int(parent.dir.Fd()), name, 0755); err != nil { |
| + if !allowExisting || !errors.Is(err, unix.EEXIST) { |
| + return nil, ErrStrictRefused |
| + } |
| + // An existing child that took cgroup.kill is never reused (amadan |
| + // #272). The caller holds the parent's mu. |
| + if err := refuseKilledChild(parent, name); err != nil { |
| + return nil, err |
| + } |
| } |
| child := &Prepared{strictPreparedState: &strictPreparedState{parent: parent, host: parent.host, limits: map[string]*os.File{}, expected: map[string]string{}, entryName: name, start: func(cmd *exec.Cmd) error { return cmd.Start() }}} |
diff --git a/internal/cgroup/strict_gen_linux_test.go b/internal/cgroup/strict_gen_linux_test.go| index dc6f873ae..556a94d1f 100644 |
| --- a/internal/cgroup/strict_gen_linux_test.go |
| +++ b/internal/cgroup/strict_gen_linux_test.go |
| @@ -205,9 +205,18 @@ func TestGenerationKillAndEmptyTouchesOnlyItsChild(t *testing.T) { |
| if _, err := g2.Populated(); !errors.Is(err, ErrStrictState) { |
| t.Fatalf("handle still open after CloseRemove: %v", err) |
| } |
| - again, err := p.PrepareGen(2, TierDefault) |
| - if err != nil { |
| - t.Fatalf("reservation not released by CloseRemove: %v", err) |
| + strictReservations.Lock() |
| + held := strictReservations.held[g2.identity] |
| + strictReservations.Unlock() |
| + if held { |
| + t.Fatal("reservation not released by CloseRemove") |
| + } |
| + // The directory survived its failed rmdir, but it took cgroup.kill, so |
| + // it is never pinned for a spawn again (amadan #272). |
| + if again, err := p.PrepareGen(2, TierDefault); !errors.Is(err, ErrStrictRefused) { |
| + if again != nil { |
| + again.Close() |
| + } |
| + t.Fatalf("re-pin of a killed generation child: %v, want ErrStrictRefused", err) |
| } |
| - again.Close() |
| } |
diff --git a/internal/cgroup/strict_linux.go b/internal/cgroup/strict_linux.go| index 581e22466..245b8e12e 100644 |
| --- a/internal/cgroup/strict_linux.go |
| +++ b/internal/cgroup/strict_linux.go |
| @@ -5,6 +5,7 @@ package cgroup |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| "io" |
| "os" |
| "os/exec" |
| @@ -231,6 +232,15 @@ func (m *Manager) prepareStrict(host string, limit Limit, genParent bool) (out * |
| } |
| } |
| }() |
| + // A group that took cgroup.kill is removed and made fresh first (amadan |
| + // #272). One a strict handle still holds stays, and the answer is the |
| + // reservation's own ErrStrictState. |
| + if e = m.freshen(host, m.hostDir(host)); e != nil { |
| + if errors.Is(e, ErrStrictState) { |
| + return nil, e |
| + } |
| + return nil, fmt.Errorf("%w: %w", ErrStrictRefused, e) |
| + } |
| if e = unix.Mkdirat(int(root.Fd()), ExecName(host), 0755); e != nil && !errors.Is(e, unix.EEXIST) { |
| return nil, ErrStrictRefused |
| } |
| @@ -394,6 +404,9 @@ func (p *Prepared) KillAndEmpty(ctx context.Context) error { |
| return e |
| } |
| p.attempted = true |
| + // Marked first, so no later start reuses this directory (amadan #272, |
| + // recreate_linux.go). |
| + markKilled(p.identity) |
| if e := strictWrite(p.kill, "1"); e != nil { |
| return ErrStrictRefused |
| } |