Repository navigation
Phase 0 fork: carry PR #321, re-adopt on restart (#301), cpu/pids quotas (#159) - #1
Draft
keeganmccallum wants to merge 18 commits into
Draft
keeganmccallum wants to merge 18 commits into
keeganmccallum wants to merge 18 commits into
Conversation
A snapshot's rootfs is one ext4, and Firecracker reopens that path verbatim for
every child, so children of one tag share a filesystem. With the drive opened
read-write — how every tag here was baked — two concurrent children are two
guest kernels writing one filesystem with no coordinator: package files pick up
other files' bytes, `/var/lib/dpkg` directory entries return EBADMSG, and the
damage reads as a random build failure rather than a sandbox error.
Firecracker cannot re-point a drive at load time (there is no `drive_overrides`
in any release — checked in the v1.16.2 and v1.17.0 binaries), but it *can*
re-point one on a restored VM via `PATCH /drives/{drive_id}`, and that call
genuinely moves the device's storage: a write made in the guest afterwards lands
in the new backing and not in the old file. So each child now gets its own
reflink clone of the tag's rootfs and is re-pointed at it.
The ordering matters and is the reason this is a three-call sequence rather than
one. Loading with `resume_vm: false` leaves the guest un-executed, so nothing it
does at boot — journal replay, `/var/log`, agent startup — can reach the shared
base before the drive is re-pointed; measured, the base stays byte-identical
through boot while the backing changes. Resuming after the re-point then hands
the guest a disk that was always its own.
**Fail closed.** A child whose rootfs could not be re-pointed is not resumed: it
would run against the shared base, which is the defect being fixed. The failure
propagates and the child is dropped (killing its Firecracker) rather than run
unsafely.
The backing lives in the child's work dir, so removing the dir reclaims it —
a crash that skips that leaves the same class of straggler as the sockets
already do. Snapshots with no recorded rootfs (daemon-side branches inherit the
source's) keep the previous single-call behaviour.
Requires Firecracker >= 1.15 for PATCH-on-restore, and a vmstate version match,
so it is deployed together with an upgrade and a re-bake.
Signed-off-by: jrimmer <jason@rimmer.net>
…uire The vendored fork reads as mandatory today, which it is not: it carries the opt-in `MemBackendConfig::shared` field and `forkd-vmm` sends `shared` in exactly one place — the memfd path live-fork uses. Everything else, including the whole bake/restore/branch/exec surface, runs on stock Firecracker. Correcting that also gives somewhere to record the two version facts that now matter: - the tested baseline is v1.17.0, and per-child rootfs backings need **v1.15+**, because `PATCH /drives` on a restored VM is accepted by older builds but does not move the device's storage — a hard minimum, not a warning, since the failure mode is silent sharing rather than an error; - a vmstate is version-pinned, so upgrading invalidates every snapshot and means re-baking every tag. Adds the matching upgrade note and the feature entry to CHANGELOG. Signed-off-by: jrimmer <jason@rimmer.net>
Verifying the per-child backings on a live host showed the leak immediately: after killing two children, both 1.6 GiB copies were still on disk and the work dir was still there. Work dirs are not reclaimed on their own — the host had seven leftover ones holding only sockets and consoles, which was harmless until a backing moved in. The child owns its backing now, and `Drop` removes it alongside the socket and cgroup it already cleans up, so a kill reclaims the copy without depending on the caller's work-dir hygiene. Set from the backing pass rather than at construction, because the child has to exist before anything can own it. A controller crash can still strand a backing; that is the same class as the stale staging dirs and `.prev-*` backups, which are swept by their next use rather than by Drop. Signed-off-by: jrimmer <jason@rimmer.net>
Restore clones each child's writable backing from the snapshot's recorded rootfs, so a snapshot with no recorded path has nothing to clone and its children fall back to sharing one file. The CLI has always written the field; the daemon never did, which left daemon-created tags — `cp`/clone and preview branches — as the one place children still shared a rootfs. The path is the one the parent actually booted from, canonicalized, matching what the CLI records. Daemon-created branches already inherit `rootfs` from their head snapshot, so those are covered by this single site. Signed-off-by: jrimmer <jason@rimmer.net>
A child killed while the controller is up has its backing removed by Vm::drop. The other order — controller crash, then startup reap — had nothing collecting them: the watchdog's stale-dir sweep keys on child-1.sock being absent, and it is present because the orphaned child held it until the reap killed it. The reap now reads each orphan's work dir from its own --api-sock while /proc still has it, and removes the child-* backings in that dir once the process is confirmed dead. Only the backings are removed, never the dir (the watchdog owns that), and only for a directory whose process this reap just reaped, so a live VM cannot be caught. Signed-off-by: jrimmer <jason@rimmer.net>
…shapes The predicate selects what a startup reap deletes, and the work dir holds `child-<n>.sock` and `child-<n>.console` under the same prefix, while the stem is the tag's rootfs filename and so not always `rootfs.ext4`. Each assertion names the shape it is defending. Signed-off-by: jrimmer <jason@rimmer.net>
The per-kill reclaim could not cover a row retired *without* a kill. The crash simulation showed the ordinary restart shape: systemd kills the unit's cgroup, so the children die with the controller, the reap finds dead processes, and it prunes rather than kills — which is correct, but nothing then reaches `Vm::drop`, so nothing removes the backing. The watchdog leaves that directory alone because the socket file is still present. The sweep runs before the reap, when those children are already gone and their directories are unowned. Its gate is process ownership rather than file presence: a directory is skipped while any live Firecracker's `--api-sock` points inside it, and if any live Firecracker cannot be read the sweep removes nothing, because ownership can no longer be established. Leaking a backing costs disk; deleting a live VM's backing destroys that VM's disk mid-job. Signed-off-by: jrimmer <jason@rimmer.net>
… happy path The predicate already had the cases its callers rely on. These are the ones a reader would guess the other way, and two of them guard against a plausible future edit: - `child-1.sock.bak` and `child-1.SOCK` match, because the comparison is against everything after the first dot rather than a suffix. Nothing creates such a file; the assertions exist so the approximation is visible rather than assumed exact. - a multi-dot or non-ext4 stem (`child-1.tar.gz`, `child-1.rootfs.squashfs`) matches, because the stem is the tag's rootfs filename. Tightening this to `.ext4` would look like a hardening and would silently stop collecting backings for any tag whose rootfs is not an ext4. - the index is never parsed, so leading zeros and absurd values pass. - ASCII digits only: generalising to `is_numeric()` would accept unicode digits and start deleting names we never created. Every assertion was confirmed by running it, not by reading the predicate. Signed-off-by: jrimmer <jason@rimmer.net>
…their own rootfs
Three defects in per-child backings, found in review and on a KVM host.
Branches did not own their rootfs. A snapshot of a restored child recorded the
child's backing, a file in a work dir that dies with the child, so the new tag
stopped loading once its source was gone ("Failed to restore from snapshot ...
No such file"), and until then every child restored from it wrote the source's
live disk. BRANCH (full, diff and live) and workspace suspend now point the
paused source's drive at a clone the new tag owns for the length of the
snapshot, record it as the tag's rootfs, and point the drive home before the
source resumes. A source whose drive cannot be pointed home is killed rather
than resumed onto the tag's disk.
A PATCH was trusted on its 204. Every re-point now reads GET /vm/config back
and fails unless Firecracker reports the new path, so a child is never resumed
on the shared base on the strength of an accepted request.
Read-only rootfs was copied per child for nothing. Whether the drive is
writable is now read from the restored vmstate itself, so read-only tags,
including ones baked before this, share the file without a copy. The copies
move into the per-child restore threads, so they run in parallel.
The restore work-dir root and its prefix come from one helper, shared by the
spawn path and the startup sweep.
…ease measured The floor rested on a claim that PATCH /drives on a restored VM is accepted but does not move storage before v1.15. Measured on stock v1.5.1, v1.7.0, v1.10.1, v1.12.1, v1.14.1 and v1.17.0: every one moves the guest's writes to the new file and reports the new path in /vm/config. So no upgrade, and no re-bake, is needed for per-child backings, and the upgrade note goes with it.
The .prev rootfs sweep leaves a backup alone while the pid in its name is a running process, since it belongs to a concurrent bake. The tests named their "dead" backups prev-1000 and prev-2000, which are often live pids on a CI runner, so the sweep skipped them and four tests failed depending on what the runner happened to be running. Pointing the constant at pid 1 reproduces the exact failures. Use pids above PID_MAX_LIMIT (2^22), which the kernel never assigns.
PR deeplethe#321 tree introduced libc/setns/metadata-ext/memfd usages without their #[cfg] companions; upstream CI only builds Linux so this broke macOS dev builds. No behavior change on Linux. - run_in_netns + exec/eval/ping_in_netns: cfg(linux) + fail-loud stubs - RootfsDetour::go_home kill: libc::kill on Linux, kill(1) elsewhere - DiffSnapshot physical_size_bytes: MetadataExt on Linux, len() else - MemfdRegion::backend_path: non-Linux stub returning empty PathBuf - preallocate_memory_file: posix_fallocate on Linux, set_len elsewhere - tests: proc_exists via kill -0, pid_alive via kill -0, gate the SEEK_DATA-dependent assemble test and the /dev/null ENOTDIR test
ChildQuota { cpu_max, pids_max } on ForkOpts, threaded from
CreateSandboxRequest / CreateWorkspaceRequest through place_child,
which writes cpu.max and pids.max alongside memory.max. Controllers
already enable the cpu/io/pids controllers in subtree_control, so
the files exist in every child leaf; None fields are not written,
preserving the previous behavior exactly.
Red-first: api::quota_tests pin request parsing (quota present and
absent shapes) before the cgroup write itself, which needs a real
/sys/fs/cgroup (verified on the Phase 0 node instead).
…ning them (deeplethe#301) A controller restart used to SIGKILL every identity-verified live child (kill_orphans, deeplethe#298), pruning the whole pool in one stroke. Now recover_orphans offers each verified row to an adoption closure FIRST: Vm::adopt reconstructs a managing handle around the running Firecracker (socket probed via GET /vm/config), run_daemon re-inserts it into live_vms, marks its netns index active (adopt_active_index) and restores the shared-tap lease before the HTTP listener starts. Rows that fail adoption fall back to the deeplethe#298 kill path unchanged, and identity gates (pidfd, starttime+boot_id, comm) still run BEFORE any adoption, so the fail-closed contract holds: no live row is ever left registered-but-unmanaged. Vm::proc becomes Option<Child>: adopted VMs are not our children, so Drop/Kill signal them by raw pid (kill_raw). vm: adopt + unit tests: missing socket, dead socket file, cmdline/sock parse, backing discovery, allocator active-index blocking.
Remove the unused prune_sandbox_row helper (recovery path prunes via its own lock+flush), reattach the recover_or_discard_prev_rootfs doc comment, apply rustfmt across the quota/adoption changes.
The non-Linux import trim dropped Context from the Linux build too; gated both variants.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
…unblocked) (#2) * fix(controller): default n>1 REST spawns to per-child netns (fan-out unblocked) POST /v1/sandboxes with n>1 and no explicit per_child_netns previously 503'd ('shared-TAP spawns with n>1 are not supported') — the shared forkd-tap0 cannot back a concurrent multi-restore batch, blocking all daemon-side fan-out (s2-03 receipt: 'REST n>1 fails - shared tap busy'). per_child_netns is now Option<bool>: absent => controller picks (n=1 shared tap, n>1 per-child netns); explicit false with n>1 keeps the 503. CLI flag defaults, n=1 behavior, and the workspace path are unchanged. Red-first: create_sandbox_n_gt_1_defaults_to_per_child_netns failed on the old behavior with the exact production 503; green after the fix. Explicit-false test pins the 503 stays. * fix(vmm): drop dead linux-gated MetadataExt import breaking CI clippy -D warnings The inner 'use std::os::unix::fs::MetadataExt' at the diff-snapshot site makes the top-level import dead; Linux CI (cargo clippy --all-targets --all-features -D warnings) fails on it while macOS builds skip the cfg(target_os = "linux") import entirely, so it slipped through. Blocking release CI for v0.5.3-urun.2. * docs: changelog + API entry for the n>1 per-child-netns default * bench: node-side fan-out bench + controller swap scripts used for the s3-01 proof * fix(vmm,controller): bound clock-sync agent wait per spawn (2s on REST create) sync_guest_clocks blocked POST /v1/sandboxes on a 10s/child guest-agent poll. Agent-less snapshots (E2B-shaped envd images, no forkd-agent on :8888) burned the full budget every spawn: measured 10s on n=1 and 20-26s on n=2 on forkd-p0-1 while the restore itself took ~60ms. ForkOpts gains clock_sync_timeout_secs (0 = historical 10s default). The daemon's sandbox-create and workspace-spawn paths use 2s: a restored forkd-agent answers in well under a second, so real sync still lands; agent-less snapshots cap their penalty at 2s instead of 10s. Clock sync stays best-effort non-fatal (unchanged semantics). CLI paths keep the 10s default; sync_guest_clocks=false paths are untouched. * test(vmm): pin clock-sync default + mark agent timeout const test-only 717846a threaded the agent-wait budget through ForkOpts but left the old const as a non-test definition — dead in lib builds (clippy noise, same class as d7914dc) — and shipped no guard pinning the default. Adds fork_opts_clock_sync_default_is_historical_10s (default stays 10s so CLI/restore keep their budget; 0 falls back, never a zero-second poll) and cfg(test)-gates the legacy const.
…nk probe, REST smoke spawn (#3) * feat(cli): doctor readiness gate — --json, FC version pin, netns floor, reflink workdir probe, REST smoke spawn forkd doctor today is a human diagnostic (18 checks, glyph report). A serving pool needs a GATE: a cpu-sbx node is ready only when every blocking check is green AND the node can actually fork. - --json: machine-readable {ready, checks[]} output; exit code is the gate (0 = no FAIL). WARN never blocks, same as the human path. - --expected-fc-version: exact pin compare (snapshot-family rule: restore only on the exact FC version x CPU family x guest kernel; '>=1.10' is too loose for a homogeneous pool). - --min-netns N: fan-out >1 needs one forkd-child-* netns per concurrent child; fewer = FAIL. - reflink workdir check (new, always on): per-child rootfs backing (PR deeplethe#321) is FICLONE on XFS (~6 ms/fork) but a full stream copy on ext4 (~265 s/fork for a 24G base). Probes ioctl(FICLONE) in TMPDIR. - --smoke-tag TAG [--smoke-port P]: REST-spawn one sandbox through the live daemon (per_child_netns), optionally TCP-probe envd (:49983), always delete. Proves restore+fork e2e. tempfile moves dev->runtime deps (doctor uses it on the probe path). * fix(cli): doctor smoke spawn probes envd via the child netns The guest of a per-child-netns sandbox lives at 10.42.0.2 INSIDE its netns; from the root netns the probe got 'No route to host'. Probe now setns()es via 'ip netns exec' in a helper child (never in-process) and retries for 10s to cover envd/socat bind lag — same contract as lane-02's init-loop.sh. Non-Linux gets a plain-connect stub. Also derive Default for DoctorOpts (clippy derivable_impls). * feat(cli): doctor warns on ARP gc_thresh defaults that starve big netns pools Each per-child netns carries its own ARP neighbor table. The Linux defaults (gc_thresh1/2/3 = 128/512/1024) overflow once a pool does n~100 REST fan-out spawns: every subsequent spawn then stalls in guest-net bring-up with 'neighbour: arp_cache: neighbor table overflow!' in dmesg (observed live on the Phase 0 C3 node, forkd-p0-1: POST /v1/sandboxes >55s stall while GETs stayed instant). WARN, not FAIL: a fresh node passes until it fans out hard, and the fix is one sysctl — but a serving pool must raise it at bootstrap. * fix(cli): doctor tests — Check::warn/fail take a hint argument * fix(cli): doctor review fixes — serde_json spawn body, timeout on netns probe, assert serialized readiness CodeRabbit PR#3: - smoke spawn body built with serde_json::json! (a --smoke-tag with quotes/backslashes previously produced malformed JSON and could inject extra fields like n/memory_limit_mib). - the in-netns TCP probe is argv-wrapped in `timeout 3` (the kernel's SYN retry window is ~2 min; without the cap a dropped-SYN guest blocked the readiness gate for minutes past the 10 s deadline) and the address is passed as a positional parameter ($1), never interpolated into the shell text. - json gate test asserts the serialized {"ready":…} value (via the extracted json_output helper) instead of re-testing the predicate. * fix(cli): smoke spawn FAILS when cleanup DELETE fails (CodeRabbit PR#3) A leaked probe sandbox on a 'passing' node defeats the gate: it holds a netns and a firecracker slot. The cleanup closure now checks the DELETE result and downgrades the Check to fail on transport error or non-2xx. * fix(cli): netns probe used the colon /dev/tcp form — bash requires /dev/tcp/HOST/PORT 'exec 3<>/dev/tcp/10.42.0.2:49983' is not valid bash /dev/tcp syntax; it fails with 'No such file or directory' even when the guest answers. Live diagnosis on forkd-p0-1: colon form always fails, slash form (/dev/tcp/10.42.0.2/49983) connects in the same child. The address is a parse-validated SocketAddr, so building the probe text from it carries no injection surface.
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Our forkd fork for agent sandboxes (Stream 2 Phase 0), based on upstream
deeplethe/forkdPR deeplethe#321 tree (which itself sits on upstreamdevand carries the merged deeplethe#322 FICLONE fix):recover_orphans(adopt)offers verified rows to adoption first:Vm::adoptreconstructs a managing handle (socket probed viaGET /vm/config),run_daemonre-inserts intolive_vms, re-marks netns indices active, restores the shared-tap lease — before the listener binds. Fail-closed Controller restart loses netns allocator and shared-tap ownership deeplethe/forkd#298 contract preserved: identity gates run before adoption; unadoptable rows still die.cgroup::ensure_parentdoesn't enable cpu/io/pids controllers deeplethe/forkd#159 done —ChildQuota { cpu_max, pids_max }onForkOpts, written ascpu.max/pids.maxbesidememory.maxinplace_child. RESTCreateSandboxRequest/CreateWorkspaceRequesttake optionalcpu_max/pids_max.Evidence
Vm::adoptrefusal of missing/dead socket (2), cmdline/sock recovery parse, backing discovery, allocator adopt-active-index blocking.v0.5.3-urun.1→ release.yml buildsforkd-v0.5.3-urun.1-x86_64-linux.tar.gz+ SHA256SUMS (upstream shape) for the Phase 0 C3 node.Status
Draft per spike-then-lock-in: merges only after the C3-node hops (install, doctor, fork-100, deeplethe#317 repro→fix on XFS, restart-recovery live check) are seen working. Upstream PRs deliberately deferred.