From 350e769055500c0083c7e81996f3f200371ce7ee Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 22 Sep 2026 15:33:22 -0400 Subject: [PATCH 1/3] fix(vmpool): correct the stale --cgroup comment at the jailer arg site (#319 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment above the firecrackerCgroupArgs call still described the flag set as "--cgroup-version/--parent-cgroup/--cgroup" and called --cgroup "the load-bearing one of three". That was accurate until #258/#319, which removed --cgroup deliberately: creating a cgroup per VM cost 195.52 ms of a 253 ms Destroy at 64 slots plus an unbounded dying-cgroup population. So the caller's comment contradicted the callee's, which opens with "NO --cgroup (#258)". Whichever a reader saw first, the other looked like a bug — and the plausible reaction to the stale one is to "restore" the flag and reintroduce a 3.25x regression. Corrected, and says why D1's per-VM memory bound still holds without it: cgroupPool writes memory.max = PerVMBytes on the pooled cgroup the jailer relocates into, rather than jailer writing it on a fresh one. Comment only; no behaviour change. `go build ./...` clean, and the full vmpool suite passes (44 cgroup-matching tests, verified non-vacuous). Refs #258 #319 Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- remote-worker/internal/vmpool/launcher_firecracker.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/remote-worker/internal/vmpool/launcher_firecracker.go b/remote-worker/internal/vmpool/launcher_firecracker.go index e429d103..2d9281c0 100644 --- a/remote-worker/internal/vmpool/launcher_firecracker.go +++ b/remote-worker/internal/vmpool/launcher_firecracker.go @@ -400,10 +400,12 @@ 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 own doc comment. An earlier version of this comment claimed + // --cgroup was the load-bearing one of three, which was true until #258/#319 removed it + // — creating a cgroup per VM cost 195.52 ms of a 253 ms Destroy at 64 slots. D1's bound + // still holds because cgroupPool writes memory.max = PerVMBytes on the pooled cgroup this + // relocates into, rather than jailer writing it on a fresh one. 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. From f8dad58486df14f27f0f96ae59aaad5bdd581740 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 22 Sep 2026 16:52:46 -0400 Subject: [PATCH 2/3] fix(vmpool): sweep the remaining stale --cgroup claims, not just the call site Review on #330 found the first version fixed one copy of the stale claim and then pointed at another. It deferred to firecrackerCgroupArgs' doc comment, which was byte-identical to base and still asserted the function returns "--cgroup memory.max= (D1: the flag that actually creates the per-VM cgroup at all)" -- the exact claim the PR exists to remove. The contradiction moved one hop and gained an explicit pointer aimed at the stale side. So the correction now lands on the function, where godoc renders it and where a reader following any pointer arrives: - firecrackerCgroupArgs' doc comment: says what it returns (two flags), that it creates and bounds nothing, that #319 removed --cgroup, and how D1's bound survives via cgroupPool. Three clauses were stale, not one: "create and bound this VM's own cgroup", the --cgroup description, and "so a test can assert the memory bound" -- that test asserts the bound off the pool and asserts flags-only about this function. - The call site shrinks to a bare pointer. Most of what it carried belongs on the function, and describing what a previous version of the comment claimed left the next reader two dead states to carry instead of one; git blame already holds that. - launcher_chv.go: dropped the "like firecrackerCgroupArgs" analogy, which inherited the pre-#319 property. CHV's -p MemoryMax= still carries a bound; the function it compared itself to no longer does. Replaced with the asymmetry stated explicitly, so neither arm is read off the other. - cgroup.go: three more of the same class, found by sweeping the package rather than the diff. The header's arm-agnostic paragraph pointed at firecrackerCgroupArgs while asserting jailer makes the cgroup; the slice tree diagram showed only the pre-#319 vm- form; and vmIDPrefix claimed "The Firecracker arm's cgroup directory IS that id", contradicted by pooledCgroupPrefix 28 lines below in the same const block, which says a vm- cgroup at startup is an orphan. Attribution corrected throughout: #258 is the measurement issue that produced the 195.52 ms figure, #319 is the PR that dropped the flag. Comment-only: every changed line in the diff is a comment, checked mechanically. gofmt clean, go build and go vet clean, full vmpool suite passes with -count=1 (the cgroup-filtered run matched 32 tests, counted rather than trusting a bare ok), all 9 remote-worker packages pass, make lint passes. Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- remote-worker/internal/vmpool/cgroup.go | 27 ++++++++++++------- remote-worker/internal/vmpool/launcher_chv.go | 9 +++++-- .../internal/vmpool/launcher_firecracker.go | 27 ++++++++++--------- 3 files changed, 38 insertions(+), 25 deletions(-) diff --git a/remote-worker/internal/vmpool/cgroup.go b/remote-worker/internal/vmpool/cgroup.go index ea8a1d87..d6912f71 100644 --- a/remote-worker/internal/vmpool/cgroup.go +++ b/remote-worker/internal/vmpool/cgroup.go @@ -16,11 +16,13 @@ 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 +// 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 @@ -41,7 +43,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 +100,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 2d9281c0..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 @@ -401,11 +406,7 @@ func (l *firecrackerLauncher) Restore(ctx context.Context, req RestoreRequest) ( "--chroot-base-dir", l.opts.ChrootBase, } // firecrackerCgroupArgs appends --cgroup-version and --parent-cgroup, and deliberately - // NOT --cgroup: see its own doc comment. An earlier version of this comment claimed - // --cgroup was the load-bearing one of three, which was true until #258/#319 removed it - // — creating a cgroup per VM cost 195.52 ms of a 253 ms Destroy at 64 slots. D1's bound - // still holds because cgroupPool writes memory.max = PerVMBytes on the pooled cgroup this - // relocates into, rather than jailer writing it on a fresh one. + // 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. From dc6fe82cefcce85746ac64e9e2bde22c1b37178f Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 22 Sep 2026 18:32:27 -0400 Subject: [PATCH 3/3] style(vmpool): reflow the cgroup.go arm-agnostic header, which my rewrap left mid-sentence Review nit on #330. Fixing an over-long line in f8dad58 left the opposite defect one line down: a 31-character fragment, "// that both mechanisms produce", wedged between a 91- and a 90-character line and breaking mid-sentence. gofmt does not reflow comment prose, so it passed every check twice. The paragraph is now reflowed end to end at 85-92 characters, matching the block. The prose is unchanged -- asserted mechanically, by rejoining the reflowed lines and comparing to the original string, so only line breaks moved. Two judgement calls worth recording, since both went against the obvious choice: The reflow splits `systemd-run --scope` across a line break. Keeping it whole forces a 75-character line at every wrap width, because the quoted command is an unbreakable 21-character token -- I measured widths 86 through 93 and none of them avoided it. The split is already this file's own idiom (chvScopeUnitPrefix, :114, breaks the same command at the same place), so it costs nothing a reader is not used to here. The reviewer's "87-92" describes this header block, not the file: cgroup.go has many mid-paragraph comment lines at 73-81. So the band was worth matching for this block's sake, not as a file-wide rule. On measurement: byte and character widths differ here, because the paragraph contains an em-dash (3 bytes) and a section sign (2). In characters the block is 85-92; in bytes 85-94. The file's pre-existing maximum is 97 either way, so nothing in this commit is wider than what cgroup.go already carried. Comment-only, checked mechanically. gofmt clean, go build and go vet clean, all 9 remote-worker packages pass with -count=1, make lint passes. Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- remote-worker/internal/vmpool/cgroup.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/remote-worker/internal/vmpool/cgroup.go b/remote-worker/internal/vmpool/cgroup.go index d6912f71..714bbdb6 100644 --- a/remote-worker/internal/vmpool/cgroup.go +++ b/remote-worker/internal/vmpool/cgroup.go @@ -19,14 +19,13 @@ import ( // 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. +// 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