diff --git a/remote-worker/internal/vmpool/cgroup.go b/remote-worker/internal/vmpool/cgroup.go index ea8a1d87..714bbdb6 100644 --- a/remote-worker/internal/vmpool/cgroup.go +++ b/remote-worker/internal/vmpool/cgroup.go @@ -16,15 +16,16 @@ import ( // group. On start, sweep the slice for orphans from a previous incarnation." // // ARM-AGNOSTIC BY DESIGN (hardware-corrections D3): the cgroup-CREATION mechanism is -// per-arm — Firecracker's jailer makes one via --cgroup/--parent-cgroup/--cgroup-version -// (see firecrackerCgroupArgs in launcher_firecracker.go); Cloud Hypervisor has no jailer -// equivalent, so its launcher places the VMM in a `systemd-run --scope` instead (see -// launcher_chv.go). But SweepOrphans and vmCgroupPath below only ever walk directories -// and read/write the two files (cgroup.procs, memory.max) that both mechanisms produce -// under the SAME parent slice (spec §5.3). Neither function contains one line that knows -// which VMM made a given subdirectory — that is the whole point: one sweep, at worker -// start, covers whatever either arm left behind, including a mix of both across restarts -// where SH_VMM was changed. +// per-arm — on the Firecracker arm cgroupPool creates the cgroup and jailer is merely +// relocated into it by --parent-cgroup (see cgroup_pool.go, and firecrackerCgroupArgs in +// launcher_firecracker.go; jailer's own --cgroup created one per VM until #319); Cloud +// Hypervisor has no jailer equivalent, so its launcher places the VMM in a `systemd-run +// --scope` instead (see launcher_chv.go). But SweepOrphans and vmCgroupPath below only ever +// walk directories and read/write the two files (cgroup.procs, memory.max) that both +// mechanisms produce under the SAME parent slice (spec §5.3). Neither function contains one +// line that knows which VMM made a given subdirectory — that is the whole point: one sweep, +// at worker start, covers whatever either arm left behind, including a mix of both across +// restarts where SH_VMM was changed. // // D8 (comm-truncation trap): this sweep kills by reading pids out of cgroup.procs, never // by matching a process name. That is not just simpler — it is the only form of this that @@ -41,7 +42,8 @@ import ( // // /sys/fs/cgroup/microvm-vms.slice/ // ├── microvm-worker.service/ <-- cgroup.procs holds the SWEEPING PROCESS's own pid -// ├── vm-9/ <-- Firecracker: jailer --id/--parent-cgroup/--cgroup +// ├── pool-9/ <-- Firecracker: cgroupPool's, jailer relocated in +// ├── vm-9/ <-- Firecracker pre-#319: jailer --id + its own --cgroup // └── vm-vm-9.scope/ <-- Cloud Hypervisor: systemd-run --scope --slice= // // The original sweep's only filter was entry.IsDir(), so it read its own pid out of @@ -97,11 +99,15 @@ import ( // returning 0 swept while VMs leaked. const ( // vmIDPrefix prefixes every pool-generated VM id ("vm-" + a decimal sequence - // number). The Firecracker arm's cgroup directory IS that id: jailer, given + // number). That id is jailer's --id, so it names the VM's JAIL directory. + // + // It named the Firecracker arm's CGROUP directory too, until #319: jailer, given // --cgroup, "will create a new cgroup named for the microvm in the - // / subfolder" (firecracker docs/jailer.md), so - // --parent-cgroup replaces the exec-file-name default entirely and the VM cgroup is - // an immediate child of the slice. + // / subfolder" (firecracker docs/jailer.md), which made the + // VM cgroup an immediate child of the slice. That flag is gone, so a live VM now sits in + // a pooledCgroupPrefix directory (below) and a vm- cgroup can only be an orphan left + // by an incarnation that predates #319. The name stays in this list for exactly that + // reason — the sweep must still recognise one. vmIDPrefix = "vm-" // chvScopeUnitPrefix prefixes the Cloud Hypervisor arm's `systemd-run --scope diff --git a/remote-worker/internal/vmpool/launcher_chv.go b/remote-worker/internal/vmpool/launcher_chv.go index 1d1d7283..5a598da7 100644 --- a/remote-worker/internal/vmpool/launcher_chv.go +++ b/remote-worker/internal/vmpool/launcher_chv.go @@ -382,8 +382,13 @@ func chvCgroupSliceName(parentCgroup string) string { // Firecracker's jailer --parent-cgroup targets (spec §5.3), and // -p MemoryMax= — the value that must equal vmpool.PerVMBytes(cfg), never a // second constant (D1's argument, mirrored here). Split out of Restore's argv -// construction, like firecrackerCgroupArgs, so a test can assert the memory bound -// agrees with PerVMBytes(cfg) without spawning systemd-run. +// construction so a test can assert the memory bound agrees with PerVMBytes(cfg) +// without spawning systemd-run. +// +// The arms are NOT symmetric about that bound any more, so do not read one off the +// other: since #319 firecrackerCgroupArgs carries no bound at all — cgroupPool writes +// memory.max and jailer gets only --parent-cgroup — whereas this argv still carries +// its own, because systemd-run creates the scope's cgroup and nothing pools it. // // UNVERIFIED END TO END (see this file's package comment, D4 disposition): this task // has no host with systemd + KVM to run this argv for real. What IS verified is the diff --git a/remote-worker/internal/vmpool/launcher_firecracker.go b/remote-worker/internal/vmpool/launcher_firecracker.go index e429d103..63108039 100644 --- a/remote-worker/internal/vmpool/launcher_firecracker.go +++ b/remote-worker/internal/vmpool/launcher_firecracker.go @@ -144,14 +144,19 @@ func (o FirecrackerOptions) validate() error { return nil } -// firecrackerCgroupArgs returns the jailer flags that create and bound this VM's own -// cgroup: --cgroup-version 2 (D2: jailer's own default is version "1", which this -// cgroup2-only host does not have — always pass 2 explicitly), --parent-cgroup (so the -// VM's cgroup lands under Task 17's systemd slice, spec §5.3), and --cgroup -// memory.max= (D1: the flag that actually creates the per-VM cgroup at all; -// --parent-cgroup alone only relocates the process into the shared parent). Split out -// of Restore's argv construction so a test can assert the memory bound agrees with -// PerVMBytes(cfg) without spawning jailer. +// firecrackerCgroupArgs returns the jailer cgroup flags: --cgroup-version 2 (D2: jailer's +// own default is version "1", which this cgroup2-only host does not have — always pass 2 +// explicitly) and --parent-cgroup, which relocates the jailed process into the pooled +// cgroup cgroupPool already created under Task 17's systemd slice (spec §5.3). +// +// It creates nothing and bounds nothing itself, and deliberately returns no --cgroup — #319 +// removed that flag, and the body says why. D1's per-VM memory.max is not lost, only moved: +// cgroupPool writes it on that pooled cgroup, from the same PerVMBytes admission control +// charges. +// +// Split out of Restore's argv construction so a test can assert those flags — and the +// absence of --cgroup — without spawning jailer. The bound itself is asserted off the pool, +// in the same test (cgroup_test.go). func firecrackerCgroupArgs(parentRel string) []string { if parentRel == "" { return nil @@ -400,10 +405,8 @@ func (l *firecrackerLauncher) Restore(ctx context.Context, req RestoreRequest) ( "--gid", strconv.Itoa(l.opts.GID), "--chroot-base-dir", l.opts.ChrootBase, } - // firecrackerCgroupArgs appends --cgroup-version/--parent-cgroup/--cgroup: the last - // of the three is load-bearing, not decorative — without it jailer relocates this - // process into the shared parent cgroup but creates no cgroup of its own, so no - // per-VM memory.max is ever set (Task 17, hardware-corrections D1). + // firecrackerCgroupArgs appends --cgroup-version and --parent-cgroup, and deliberately + // not --cgroup: see its doc comment. args = append(args, firecrackerCgroupArgs(cgroupRel)...) // Coordinator finding #5: this launcher never issues PUT /network-interfaces — // standbys are headless by construction, not by omission. Nothing below adds one.