amadan
cgroup: a generation start reads the PID-move latch once and is never moved
Task 7 fix round 1: one latch read decides a required start, with a guard before the start-and-move tail; ENOENT/EACCES from a failed clone3 are classified by probing the child cgroup, ENOMEM/EAGAIN are the box's; the required path logs only its refusal; FakePlacementForTest panics outside tests; stricter no-PID-move check; recovery control cleanup kill.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8 files changed,
+320
−55
internal/activator/exec_gate_test.go+11 −2internal/activator/genparent_kernel_linux_test.go+1 −0internal/cgroup/cgroup.go+58 −14internal/cgroup/cgroup_linux.go+51 −15internal/cgroup/cgroup_linux_test.go+144 −13internal/cgroup/cgroup_other.go+1 −1internal/cgroup/gen.go+5 −4internal/cgroup/gen_test.go+49 −6
diff --git a/internal/activator/exec_gate_test.go b/internal/activator/exec_gate_test.go| index 33da67a70..cd25a8df4 100644 |
| --- a/internal/activator/exec_gate_test.go |
| +++ b/internal/activator/exec_gate_test.go |
| @@ -595,8 +595,17 @@ func TestTheExecGateIsAskedAtEachCgroupStart(t *testing.T) { |
| } |
| gens := b.gens() |
| b.prepGone(t, gens[0]) |
| - if got, err := os.ReadFile(filepath.Join(parent, cgroup.GenChild(gens[0]), "cgroup.procs")); err == nil && len(got) != 0 { |
| - t.Fatalf("generation %d's child cgroup was given pid %s by a PID move", gens[0], got) |
| + // No PID move. In this fixture the child's limit files are ordinary |
| + // files, so the refusal's rmdir leaves the child in place: it must |
| + // still be there with its memory.max written (else the check below |
| + // proves nothing), and with no cgroup.procs at all, which is the |
| + // file a start-and-move writes the PID into. |
| + child := filepath.Join(parent, cgroup.GenChild(gens[0])) |
| + if _, err := os.Stat(filepath.Join(child, "memory.max")); err != nil { |
| + t.Fatalf("generation %d's child cgroup has no limit written (%v): the start never reached its placement", gens[0], err) |
| + } |
| + if got, err := os.ReadFile(filepath.Join(child, "cgroup.procs")); !os.IsNotExist(err) { |
| + t.Fatalf("generation %d's child cgroup has a cgroup.procs (%q, %v): a PID move wrote it", gens[0], got, err) |
| } |
| }) |
| } |
diff --git a/internal/activator/genparent_kernel_linux_test.go b/internal/activator/genparent_kernel_linux_test.go| index df9b0f111..57e7437e5 100644 |
| --- a/internal/activator/genparent_kernel_linux_test.go |
| +++ b/internal/activator/genparent_kernel_linux_test.go |
| @@ -212,6 +212,7 @@ func genParentRecoveryControl(t *testing.T, b *ExecBackend, host, parent string, |
| if err != nil || g1 == nil { |
| t.Fatalf("g1 after the legacy process exited: %v, %v", g1, err) |
| } |
| + t.Cleanup(func() { _ = g1.Process.Kill() }) // a failed check below must not leave g1 running |
| done := spawnStaysUp(t, "recovered g1", g1) |
| expectGenParent(t, parent) |
| expectCappedGen(t, host, parent, cgroup.GenChild(1), g1) |
diff --git a/internal/cgroup/cgroup.go b/internal/cgroup/cgroup.go| index 50572d945..bd3ad6953 100644 |
| --- a/internal/cgroup/cgroup.go |
| +++ b/internal/cgroup/cgroup.go |
| @@ -27,6 +27,7 @@ import ( |
| "strings" |
| "sync" |
| "sync/atomic" |
| + "testing" |
| ) |
| // execPrefix is the ONE definition of the per-host exec cgroup naming — |
| @@ -386,12 +387,16 @@ func (m *Manager) StartGated(host string, limit Limit, build func() *exec.Cmd, b |
| // a temp directory is no cgroup, so the clone3 CLONE_INTO_CGROUP start |
| // answers EBADF there, and a generation start (StartChildGated) is never |
| // retried by start-and-move. Until t's cleanup, every start runs without |
| -// the cgroup fd, as if the kernel had placed it. t is a *testing.T; nothing |
| -// outside a test calls it. |
| +// the cgroup fd, as if the kernel had placed it. t is a *testing.T. It |
| +// panics outside a test binary. It swaps a package global, so a test that |
| +// calls it, and every other test in that binary, must not use t.Parallel. |
| func FakePlacementForTest(t interface { |
| Helper() |
| Cleanup(func()) |
| }) { |
| + if !testing.Testing() { |
| + panic("cgroup: FakePlacementForTest called outside a test") |
| + } |
| t.Helper() |
| orig := startCmd |
| startCmd = func(cmd *exec.Cmd) error { |
| @@ -437,9 +442,20 @@ func startPlain(build func() *exec.Cmd, before func() error) (*exec.Cmd, error) |
| // start-and-move. Without it (StartGated, the flat start) every one of |
| // those stays best-effort, exactly as before. |
| func (m *Manager) startIn(host, dir string, limit Limit, required bool, build func() *exec.Cmd, before func() error) (*exec.Cmd, error) { |
| - if required && (!intoCgroupSupported || m.fallback.Load()) { |
| - m.logf("cgroup: %s: clone3-into-cgroup is unavailable here, and a generation is never started then moved — generation start refused", host) |
| - return nil, fmt.Errorf("%w: %s: clone3-into-cgroup unavailable", ErrLimitNotApplied, host) |
| + // clone3 is whether this start places its process with clone3 |
| + // CLONE_INTO_CGROUP. A generation start reads the latch ONCE, here, and |
| + // that one value decides both the refusal below and the start further |
| + // down (Task 7 fix round 1): a concurrent flat start may latch the |
| + // fallback while this one makes its child and writes its limits, and a |
| + // second read would then send the generation to start-and-move. The flat |
| + // start keeps its one read where it always was, after its limits. |
| + clone3 := false |
| + if required { |
| + clone3 = intoCgroupSupported && !m.fallback.Load() |
| + if !clone3 { |
| + m.logf("cgroup: %s: clone3-into-cgroup is unavailable here, and a generation is never started then moved — generation start refused", host) |
| + return nil, fmt.Errorf("%w: %s: clone3-into-cgroup unavailable", ErrLimitNotApplied, host) |
| + } |
| } |
| if err := os.MkdirAll(dir, 0o755); err != nil { |
| if required { |
| @@ -449,12 +465,15 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| m.logf("cgroup: %s: create cgroup: %v — spawning unaccounted", host, err) |
| return startPlain(build, before) |
| } |
| - if err := m.applyLimit(host, dir, limit); err != nil && required { |
| + if err := m.applyLimit(host, dir, limit, required); err != nil && required { |
| _ = os.Remove(dir) |
| - m.logf("cgroup: %s: %s: generation start refused", host, filepath.Base(dir)) |
| + m.logf("cgroup: %s: %s: %v — generation start refused", host, filepath.Base(dir), err) |
| return nil, fmt.Errorf("%w: %s: %w", ErrLimitNotApplied, filepath.Base(dir), err) |
| } |
| - if intoCgroupSupported && !m.fallback.Load() { |
| + if !required { |
| + clone3 = intoCgroupSupported && !m.fallback.Load() |
| + } |
| + if clone3 { |
| f, err := os.Open(dir) |
| if err != nil { |
| if required { |
| @@ -486,9 +505,11 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| // the cgroup was the cause, and nothing latches. A failure that |
| // is the placement's (placementFailed) is the box's condition; |
| // any other is the start's own, as for a flat start whose retry |
| - // failed too. |
| + // failed too. placementFailed may probe dir, so it runs before |
| + // the dir goes. |
| + placement := placementFailed(firstErr, dir) |
| _ = os.Remove(dir) |
| - if placementFailed(firstErr) { |
| + if placement { |
| m.logf("cgroup: %s: clone3-into-cgroup failed (%v) — generation start refused, not retried by PID move", host, firstErr) |
| return nil, fmt.Errorf("%w: %s: clone3-into-cgroup: %w", ErrLimitNotApplied, host, firstErr) |
| } |
| @@ -514,6 +535,15 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| } |
| return retried, nil |
| } |
| + if required { |
| + // Unreachable while clone3 is read once above (a required start |
| + // without it was refused before its child was made). Kept so no |
| + // later edit can send a generation to start-and-move: a generation |
| + // is never started outside its cgroup and moved by PID. |
| + _ = os.Remove(dir) |
| + m.logf("cgroup: %s: a generation start reached the PID-move fallback — generation start refused", host) |
| + return nil, fmt.Errorf("%w: %s: a generation is never started then moved", ErrLimitNotApplied, host) |
| + } |
| cmd, _, err := m.startAndMoveIn(host, dir, build, before) |
| return cmd, err |
| } |
| @@ -523,8 +553,11 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| // control file that isn't ours) is one logged line and an uncapped child. |
| // It returns the first write that failed, for a caller that must not start |
| // without the whole limit (startIn's required); the flat start ignores it. |
| -// Written BEFORE the child is in the cgroup, so the cap holds from its |
| -// first page. memory.max first: if that one fails, high is pointless. |
| +// With required, the first failure returns at once and logs nothing here: |
| +// no child will run, so "runs uncapped" would be untrue, and startIn logs |
| +// the one refusal line (Task 7 fix round 1). Without it, every line is as |
| +// before. Written BEFORE the child is in the cgroup, so the cap holds from |
| +// its first page. memory.max first: if that one fails, high is pointless. |
| // |
| // The cap bounds the cgroup's TOTAL charge — anonymous memory plus the page |
| // cache its reads pull in — not RSS. A tenant whose cache alone exceeds |
| @@ -535,16 +568,21 @@ func (m *Manager) startIn(host, dir string, limit Limit, required bool, build fu |
| // memory.oom.group=1 makes an OOM kill inside this cgroup take the whole |
| // family: a hub whose children outlive it would otherwise leave orphans |
| // holding the socket while the activator respawns a fresh hub beside them. |
| -func (m *Manager) applyLimit(host, dir string, limit Limit) error { |
| +func (m *Manager) applyLimit(host, dir string, limit Limit, required bool) error { |
| if limit.MemoryMax <= 0 { |
| return nil |
| } |
| if err := os.WriteFile(filepath.Join(dir, "memory.max"), []byte(strconv.FormatInt(limit.MemoryMax, 10)), 0o644); err != nil { |
| - m.logf("cgroup: %s: memory.max: %v — child runs uncapped", host, err) |
| + if !required { |
| + m.logf("cgroup: %s: memory.max: %v — child runs uncapped", host, err) |
| + } |
| return fmt.Errorf("memory.max: %w", err) |
| } |
| var first error |
| if err := os.WriteFile(filepath.Join(dir, "memory.high"), []byte(strconv.FormatInt(HighOf(limit.MemoryMax), 10)), 0o644); err != nil { |
| + if required { |
| + return fmt.Errorf("memory.high: %w", err) |
| + } |
| m.logf("cgroup: %s: memory.high: %v — child is capped but not throttled first", host, err) |
| first = fmt.Errorf("memory.high: %w", err) |
| } |
| @@ -555,12 +593,18 @@ func (m *Manager) applyLimit(host, dir string, limit Limit) error { |
| // max: a tenant may spill up to its cap into swap (the slow-then-dead |
| // gradient), then the kill is real. |
| if err := os.WriteFile(filepath.Join(dir, "memory.swap.max"), []byte(strconv.FormatInt(limit.MemoryMax, 10)), 0o644); err != nil { |
| + if required { |
| + return fmt.Errorf("memory.swap.max: %w", err) |
| + } |
| m.logf("cgroup: %s: memory.swap.max: %v — the cap can be exceeded into swap", host, err) |
| if first == nil { |
| first = fmt.Errorf("memory.swap.max: %w", err) |
| } |
| } |
| if err := os.WriteFile(filepath.Join(dir, "memory.oom.group"), []byte("1"), 0o644); err != nil { |
| + if required { |
| + return fmt.Errorf("memory.oom.group: %w", err) |
| + } |
| m.logf("cgroup: %s: memory.oom.group: %v — an OOM kill here takes one process, not the family", host, err) |
| if first == nil { |
| first = fmt.Errorf("memory.oom.group: %w", err) |
diff --git a/internal/cgroup/cgroup_linux.go b/internal/cgroup/cgroup_linux.go| index 5448d97d9..864f6bba1 100644 |
| --- a/internal/cgroup/cgroup_linux.go |
| +++ b/internal/cgroup/cgroup_linux.go |
| @@ -4,7 +4,9 @@ package cgroup |
| import ( |
| "errors" |
| + "os" |
| "os/exec" |
| + "path/filepath" |
| "syscall" |
| ) |
| @@ -28,26 +30,60 @@ func setInto(cmd *exec.Cmd, fd int) { |
| // for the life of this process — EPERM (a seccomp filter predating clone3, |
| // or one that forbids it outright) and ENOSYS (a kernel predating clone3) |
| // mean the mechanism itself is gone; EINVAL means the kernel understood |
| -// clone3 but rejected CLONE_INTO_CGROUP. Anything else (e.g. ENODEV from the |
| -// cgroup dir disappearing between os.Open and start, raced by a concurrent |
| -// CleanupHost) is spawn-specific and says nothing about the mechanism — |
| -// latching on it would permanently downgrade the process over a fluke and |
| -// log a lie ("refused"). |
| +// clone3 but rejected CLONE_INTO_CGROUP. Anything else (e.g. ENOENT or |
| +// ENODEV from the cgroup dir disappearing between os.Open and start, raced |
| +// by a concurrent CleanupHost) is spawn-specific and says nothing about the |
| +// mechanism — latching on it would permanently downgrade the process over a |
| +// fluke and log a lie ("refused"). |
| func shouldLatch(err error) bool { |
| return errors.Is(err, syscall.EPERM) || errors.Is(err, syscall.ENOSYS) || errors.Is(err, syscall.EINVAL) |
| } |
| // placementFailed reports whether a failed clone3 CLONE_INTO_CGROUP start |
| -// failed on the placement rather than on the command: the mechanism refused |
| -// (shouldLatch), or the kernel would not place into the cgroup the fd names |
| -// (EBADF: not a cgroup; ENODEV: removed under the open fd; EBUSY: a domain |
| -// controller enabled in it; EOPNOTSUPP: a domain-invalid cgroup). An exec |
| -// failure (ENOENT, EACCES, ENOEXEC, ...) is the command's own. A generation |
| -// start is never retried by start-and-move, so this is the only evidence |
| -// of which it was (Task 7). |
| -func placementFailed(err error) bool { |
| - return shouldLatch(err) || errors.Is(err, syscall.EBADF) || errors.Is(err, syscall.ENODEV) || |
| - errors.Is(err, syscall.EBUSY) || errors.Is(err, syscall.EOPNOTSUPP) |
| +// into dir failed on the placement or the box rather than on the command. A |
| +// generation start is never retried by start-and-move, so this is the only |
| +// evidence of which it was (Task 7). A placement or box failure: |
| +// - the mechanism refused (shouldLatch: EPERM, ENOSYS, EINVAL) |
| +// - the kernel would not place into the cgroup the fd names: EBADF (not |
| +// a cgroup), ENODEV (a cgroup the kernel has already marked dead), |
| +// EBUSY (a domain controller enabled in it), EOPNOTSUPP (a |
| +// domain-invalid cgroup) |
| +// - the box is out of room to fork: ENOMEM, or EAGAIN (a process or |
| +// pids.max limit) |
| +// |
| +// ENOENT and EACCES are ambiguous. Exec answers them for a missing or |
| +// unexecutable binary. The placement answers ENOENT for a cgroup removed |
| +// under the open fd, and EACCES when cgroup.procs is not writable. For |
| +// those two, dir is probed (placementLive): a child that is still a live, |
| +// writable cgroup puts the failure on the command; otherwise it is the |
| +// placement's. Any other error (ENOEXEC, ...) is the command's own. |
| +func placementFailed(err error, dir string) bool { |
| + switch { |
| + case shouldLatch(err), errors.Is(err, syscall.EBADF), errors.Is(err, syscall.ENODEV), |
| + errors.Is(err, syscall.EBUSY), errors.Is(err, syscall.EOPNOTSUPP), |
| + errors.Is(err, syscall.ENOMEM), errors.Is(err, syscall.EAGAIN): |
| + return true |
| + case errors.Is(err, syscall.ENOENT), errors.Is(err, syscall.EACCES): |
| + return placementLive(dir) != nil |
| + } |
| + return false |
| +} |
| + |
| +// placementLive probes the cgroup a failed clone3 start was placing into: |
| +// dir must still be a live cgroup (its cgroup.events can be read) and its |
| +// cgroup.procs must open for writing. It writes nothing: the open without |
| +// O_TRUNC changes no file, and a cgroup.procs that is opened and closed |
| +// moves no process. A nil answer means the placement was sound, so the |
| +// start's failure was the command's. |
| +func placementLive(dir string) error { |
| + if _, err := os.ReadFile(filepath.Join(dir, "cgroup.events")); err != nil { |
| + return err |
| + } |
| + f, err := os.OpenFile(filepath.Join(dir, "cgroup.procs"), os.O_WRONLY, 0) |
| + if err != nil { |
| + return err |
| + } |
| + return f.Close() |
| } |
| // clearInto undoes setInto: the start runs without the cgroup fd. |
diff --git a/internal/cgroup/cgroup_linux_test.go b/internal/cgroup/cgroup_linux_test.go| index e0e769d38..f397d7991 100644 |
| --- a/internal/cgroup/cgroup_linux_test.go |
| +++ b/internal/cgroup/cgroup_linux_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "os" |
| "os/exec" |
| "path/filepath" |
| + "strings" |
| "syscall" |
| "testing" |
| ) |
| @@ -244,27 +245,42 @@ func TestStartGatedRefusedBeforeTheFirstStart(t *testing.T) { |
| } |
| // Task 7, G: a generation start whose clone3 placement fails is never |
| -// retried by start-and-move. A failure that is the placement's (the |
| -// mechanism gone: EPERM, ENOSYS, EINVAL; or a cgroup fd the kernel would |
| -// not place into: EBADF, ENODEV, EBUSY, EOPNOTSUPP) is ErrLimitNotApplied, |
| -// the box's condition. Any other failure (a missing binary) is the start's |
| -// own error. Neither latches (no retry proved the cgroup was the cause), |
| -// and nothing is moved by PID. |
| +// retried by start-and-move. A failure that is the placement's or the |
| +// box's is ErrLimitNotApplied: the mechanism gone (EPERM, ENOSYS, EINVAL), |
| +// a cgroup fd the kernel would not place into (EBADF, ENODEV, EBUSY, |
| +// EOPNOTSUPP), or no room to fork (ENOMEM, EAGAIN). ENOENT and EACCES are |
| +// the start's own error only while the child is still a live, writable |
| +// cgroup (fix round 1: placementLive); a removed or unwritable child makes |
| +// them the placement's. Nothing latches (no retry proved the cgroup was |
| +// the cause), and nothing is moved by PID. |
| func TestAGenerationStartIsNeverRetriedByPIDMove(t *testing.T) { |
| const host = "h.example.com" |
| for name, tc := range map[string]struct { |
| fail error |
| + live bool // the child is still a live, writable cgroup at the failure |
| want error |
| }{ |
| - "clone3 refused": {syscall.EPERM, ErrLimitNotApplied}, |
| - "clone3 not understood": {syscall.EINVAL, ErrLimitNotApplied}, |
| - "not a cgroup": {syscall.EBADF, ErrLimitNotApplied}, |
| - "cgroup removed": {syscall.ENODEV, ErrLimitNotApplied}, |
| - "a spawn failure": {syscall.ENOENT, syscall.ENOENT}, |
| + "clone3 refused": {syscall.EPERM, true, ErrLimitNotApplied}, |
| + "clone3 not understood": {syscall.EINVAL, true, ErrLimitNotApplied}, |
| + "not a cgroup": {syscall.EBADF, true, ErrLimitNotApplied}, |
| + "cgroup dead": {syscall.ENODEV, true, ErrLimitNotApplied}, |
| + "out of memory": {syscall.ENOMEM, true, ErrLimitNotApplied}, |
| + "out of processes": {syscall.EAGAIN, true, ErrLimitNotApplied}, |
| + "a missing binary": {syscall.ENOENT, true, syscall.ENOENT}, |
| + "a cgroup removed under the fd": {syscall.ENOENT, false, ErrLimitNotApplied}, |
| + "an unexecutable binary": {syscall.EACCES, true, syscall.EACCES}, |
| + "a cgroup that cannot be entered": {syscall.EACCES, false, ErrLimitNotApplied}, |
| } { |
| t.Run(name, func(t *testing.T) { |
| m, parent := genManager(t, host) |
| + g1 := filepath.Join(parent, "g1") |
| calls := fakeStart(t, func(cmd *exec.Cmd) error { |
| + if tc.live { |
| + // What cgroupfs shows in a live child: the kernel's own |
| + // files, cgroup.procs empty and writable. |
| + writeFixture(t, filepath.Join(g1, "cgroup.events"), "populated 0\nfrozen 0\n") |
| + writeFixture(t, filepath.Join(g1, "cgroup.procs"), "") |
| + } |
| if cmd.SysProcAttr != nil && cmd.SysProcAttr.UseCgroupFD { |
| return &os.PathError{Op: "fork/exec", Path: cmd.Path, Err: tc.fail} |
| } |
| @@ -283,8 +299,123 @@ func TestAGenerationStartIsNeverRetriedByPIDMove(t *testing.T) { |
| if m.fallback.Load() { |
| t.Error("a generation start latched the PID-move fallback") |
| } |
| - if _, err := os.Stat(filepath.Join(parent, "g1", "cgroup.procs")); !os.IsNotExist(err) { |
| - t.Errorf("a PID was moved into g1: %v", err) |
| + if got, err := os.ReadFile(filepath.Join(g1, "cgroup.procs")); err == nil && len(got) != 0 { |
| + t.Errorf("a PID was moved into g1: cgroup.procs %q", got) |
| + } else if err != nil && !tc.live && !os.IsNotExist(err) { |
| + t.Errorf("g1's cgroup.procs: %v", err) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// Fix round 1, finding 1: a generation start reads the PID-move latch once. |
| +// A flat start elsewhere in the agent may latch the fallback while this |
| +// generation's child is made and its limits written; the generation still |
| +// starts the one way it decided on, placed by clone3, never started in the |
| +// agent's cgroup and moved by PID. The latch is set from applyLimit's |
| +// summary log line, which comes after the old first read and before the |
| +// old second one. |
| +func TestAGenerationStartReadsTheLatchOnce(t *testing.T) { |
| + const host = "h.example.com" |
| + m, parent := genManager(t, host) |
| + latched := false |
| + m.logf = func(format string, args ...any) { |
| + t.Logf(format, args...) |
| + if !latched && strings.HasPrefix(format, "cgroup: %s: memory.max=") { |
| + latched = true |
| + m.fallback.Store(true) // a concurrent flat start's shouldLatch |
| + } |
| + } |
| + builds := 0 |
| + calls := fakeStart(t, func(*exec.Cmd) error { return nil }) |
| + cmd, err := m.StartChild(host, GenChild(1), TierDefault, TierDefault, func() *exec.Cmd { builds++; return buildTrue() }) |
| + if !latched { |
| + t.Fatal("the latch was never set during the start: applyLimit's summary line did not fire") |
| + } |
| + if err != nil || cmd == nil { |
| + t.Fatalf("StartChild = %v, %v: want the clone3 start it decided on", cmd, err) |
| + } |
| + if builds != 1 || len(*calls) != 1 { |
| + t.Fatalf("built %d, started %d times: want one of each", builds, len(*calls)) |
| + } |
| + if attr := (*calls)[0].SysProcAttr; attr == nil || !attr.UseCgroupFD { |
| + t.Fatal("the generation was started without its cgroup fd, to be moved by PID") |
| + } |
| + if _, err := os.Stat(filepath.Join(parent, "g1", "cgroup.procs")); !os.IsNotExist(err) { |
| + t.Errorf("a PID was moved into g1: %v", err) |
| + } |
| +} |
| + |
| +// placementLive is the ENOENT/EACCES probe: a child that is still a live |
| +// cgroup with a writable cgroup.procs puts a failed start on the command; |
| +// one removed, one that is no cgroup, or one whose cgroup.procs cannot be |
| +// written puts it on the placement. The probe writes nothing. |
| +func TestPlacementLiveProbesTheChild(t *testing.T) { |
| + live := func(t *testing.T) string { |
| + dir := t.TempDir() |
| + writeFixture(t, filepath.Join(dir, "cgroup.events"), "populated 0\nfrozen 0\n") |
| + writeFixture(t, filepath.Join(dir, "cgroup.procs"), "") |
| + return dir |
| + } |
| + t.Run("live and writable", func(t *testing.T) { |
| + dir := live(t) |
| + if err := placementLive(dir); err != nil { |
| + t.Fatalf("placementLive = %v, want nil", err) |
| + } |
| + if got := readFixture(t, filepath.Join(dir, "cgroup.procs")); got != "" { |
| + t.Errorf("the probe wrote cgroup.procs: %q", got) |
| + } |
| + for _, e := range []error{syscall.ENOENT, syscall.EACCES} { |
| + if placementFailed(&os.PathError{Op: "fork/exec", Path: "/bin/x", Err: e}, dir) { |
| + t.Errorf("%v into a live child read as the placement's", e) |
| + } |
| + } |
| + }) |
| + gone := map[string]func(t *testing.T) string{ |
| + "removed": func(t *testing.T) string { |
| + dir := live(t) |
| + if err := os.RemoveAll(dir); err != nil { |
| + t.Fatal(err) |
| + } |
| + return dir |
| + }, |
| + "no cgroup.events": func(t *testing.T) string { |
| + dir := live(t) |
| + if err := os.Remove(filepath.Join(dir, "cgroup.events")); err != nil { |
| + t.Fatal(err) |
| + } |
| + return dir |
| + }, |
| + "no cgroup.procs": func(t *testing.T) string { |
| + dir := live(t) |
| + if err := os.Remove(filepath.Join(dir, "cgroup.procs")); err != nil { |
| + t.Fatal(err) |
| + } |
| + return dir |
| + }, |
| + } |
| + if os.Geteuid() != 0 { // root opens a read-only file for writing |
| + gone["cgroup.procs not writable"] = func(t *testing.T) string { |
| + dir := live(t) |
| + if err := os.Chmod(filepath.Join(dir, "cgroup.procs"), 0o444); err != nil { |
| + t.Fatal(err) |
| + } |
| + return dir |
| + } |
| + } |
| + for name, mk := range gone { |
| + t.Run(name, func(t *testing.T) { |
| + dir := mk(t) |
| + if err := placementLive(dir); err == nil { |
| + t.Fatal("placementLive = nil, want the probe to fail") |
| + } |
| + for _, e := range []error{syscall.ENOENT, syscall.EACCES} { |
| + if !placementFailed(&os.PathError{Op: "fork/exec", Path: "/bin/x", Err: e}, dir) { |
| + t.Errorf("%v into a gone child read as the command's", e) |
| + } |
| + } |
| + if placementFailed(&os.PathError{Op: "fork/exec", Path: "/bin/x", Err: syscall.ENOEXEC}, dir) { |
| + t.Error("ENOEXEC read as the placement's") |
| } |
| }) |
| } |
diff --git a/internal/cgroup/cgroup_other.go b/internal/cgroup/cgroup_other.go| index 2317b0640..3a754cb04 100644 |
| --- a/internal/cgroup/cgroup_other.go |
| +++ b/internal/cgroup/cgroup_other.go |
| @@ -13,6 +13,6 @@ func setInto(cmd *exec.Cmd, fd int) {} |
| func shouldLatch(error) bool { return false } |
| -func placementFailed(error) bool { return false } |
| +func placementFailed(error, string) bool { return false } |
| func clearInto(*exec.Cmd) {} |
diff --git a/internal/cgroup/gen.go b/internal/cgroup/gen.go| index 1d1c81a76..91b9dd172 100644 |
| --- a/internal/cgroup/gen.go |
| +++ b/internal/cgroup/gen.go |
| @@ -139,10 +139,11 @@ var ErrLegacyParentPopulated = errors.New("cgroup: the host's parent cgroup hold |
| // memory), the parent's backstop could not be written, the generation's own |
| // child could not be made or one of its four limit writes failed, or the |
| // clone3 CLONE_INTO_CGROUP placement is unavailable, so the process could |
| -// only be started outside its cgroup and moved after. The flat start |
| -// (StartGated) stays best-effort and never returns it. Nothing is exec'd, |
| -// and it is no fault of the build: like ErrLegacyParentPopulated, the |
| -// condition is the box's (Task 7, zdd flag readiness round 2). |
| +// only be started outside its cgroup and moved after, or it failed on the |
| +// placement or the box rather than the command (placementFailed). The flat |
| +// start (StartGated) stays best-effort and never returns it. Nothing is |
| +// exec'd, and it is no fault of the build: like ErrLegacyParentPopulated, |
| +// the condition is the box's (Task 7, zdd flag readiness round 2). |
| var ErrLimitNotApplied = errors.New("cgroup: a generation's limits could not be applied") |
| // delegatesMemory reports whether dir hands the memory controller to its |
diff --git a/internal/cgroup/gen_test.go b/internal/cgroup/gen_test.go| index e8131e759..90669225e 100644 |
| --- a/internal/cgroup/gen_test.go |
| +++ b/internal/cgroup/gen_test.go |
| @@ -657,19 +657,62 @@ func TestAGenerationWhoseLimitsCannotBeWrittenIsRefused(t *testing.T) { |
| if err := os.Mkdir(dir, 0o755); err != nil { |
| t.Fatal(err) |
| } |
| - if err := m.applyLimit(host, dir, TierDefault); err != nil { |
| - t.Fatalf("applyLimit with every write landing = %v", err) |
| + for _, required := range []bool{false, true} { |
| + if err := m.applyLimit(host, dir, TierDefault, required); err != nil { |
| + t.Fatalf("applyLimit(required %v) with every write landing = %v", required, err) |
| + } |
| + if err := m.applyLimit(host, dir, Limit{}, required); err != nil { |
| + t.Fatalf("applyLimit(required %v) with no limit = %v", required, err) |
| + } |
| + } |
| +} |
| + |
| +// Fix round 1, finding 7: a generation whose memory.max cannot be written |
| +// is refused, so the journal must not say it "runs uncapped". The required |
| +// path logs one refusal line naming the file; the flat start's lines are |
| +// unchanged. |
| +func TestARefusedGenerationsLimitFailureLogsTheRefusalOnly(t *testing.T) { |
| + const host = "h.example.com" |
| + m, parent, lines := legacyParent(t, host, "") |
| + if err := os.MkdirAll(filepath.Join(parent, "g1", "memory.max"), 0o755); err != nil { // WriteFile: EISDIR |
| + t.Fatal(err) |
| + } |
| + refusedChild(t, m, host, GenChild(1), TierDefault, ErrLimitNotApplied) |
| + var refusals int |
| + for _, l := range *lines { |
| + if strings.Contains(l, "uncapped") || strings.Contains(l, ": memory.max=") { // the parent's own "parent memory.max=" line is true |
| + t.Errorf("a refused generation logged %q", l) |
| + } |
| + if strings.Contains(l, "g1: memory.max:") && strings.Contains(l, "generation start refused") { |
| + refusals++ |
| + } |
| + } |
| + if refusals != 1 { |
| + t.Errorf("log lines = %q, want one refusal naming g1's memory.max", *lines) |
| + } |
| + // The flat start keeps its line. |
| + *lines = nil |
| + flat := m.hostDir("flat.example.com") |
| + if err := os.MkdirAll(filepath.Join(flat, "memory.max"), 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + fakeStart(t, func(*exec.Cmd) error { return nil }) |
| + if _, err := m.Start("flat.example.com", TierDefault, buildTrue); err != nil { |
| + t.Fatal(err) |
| } |
| - if err := m.applyLimit(host, dir, Limit{}); err != nil { |
| - t.Fatalf("applyLimit with no limit = %v", err) |
| + if !slices.ContainsFunc(*lines, func(l string) bool { |
| + return strings.Contains(l, "memory.max:") && strings.Contains(l, "child runs uncapped") |
| + }) { |
| + t.Errorf("flat log lines = %q, want the unchanged \"child runs uncapped\" line", *lines) |
| } |
| } |
| // G: with the clone3 CLONE_INTO_CGROUP placement unavailable, a generation |
| // could only be started and then moved by PID: running, for that window, |
| // in the agent's cgroup with no limit, and for good if the move fails. |
| -// A latched fallback refuses before anything is created or built. The flat |
| -// start keeps the PID-move fallback. |
| +// A latched fallback refuses before the child is created or build() runs |
| +// (ensureParent has already run). The flat start keeps the PID-move |
| +// fallback. |
| func TestAGenerationIsNeverStartedThenMovedWhenTheFallbackIsLatched(t *testing.T) { |
| const host = "h.example.com" |
| m, parent := genManager(t, host) |