Repository navigation
Conversation
Defect found while checking what a consumer needs: branching a restored child is no longer durable
Measured on a v1.17 host with this branch deployed: Two things make it worse than a stale path. The clone's This matters beyond a corner case: The fix belongs here: on branch, materialise the child's rootfs into the new tag — a reflink clone of the backing, recorded as the new tag's I would not merge this PR without it. I have not written it yet, and I am marking it here rather than leaving it for whoever reads the diff. |
The clone path was dead: wrong FICLONE ioctl numberWhile checking what a consumer needed I measured the per-child cost on a ZFS pool and got a full copy — 12291 MiB of pool growth for one 12 GiB child, when both the source and the destination were on the same pool and that pool demonstrably clones (
ENOTTY is in the list this function treats as "this filesystem does not support reflink", so it falls through to a full streamed copy — on every filesystem, silently, including the ones where cloning works. That is why the number being wrong was invisible: the failure path is the same as the legitimate one. Two consequences beyond this PR: a restored child cost a full copy of the rootfs (measured above), and the bake path's "reflink preferred" baseline clone was a full copy too. Corrected in this PR; |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks. Pausing on a baked-in drive and re-pointing it with PATCH /drives before resume is the right mechanism, and the evidence table is exactly what this needed. Not merging yet. Beyond the branch-durability blocker you already flagged, here is what I found.
Already landed separately: the FICLONE fix you described (0x40209409 → 0x40049409) was never pushed to this branch; the diff doesn't touch chain.rs. It is in dev now as #322, credited to you and verified on an XFS reflink=1 loopback (old number → ENOTTY, new → shared extents). Please rebase onto dev.
Blocking
- Branch durability (your comment): a branch taken from a restored child has to materialise the child's backing into the new tag and record it as that tag's
rootfs. - No runtime Firecracker version gate. The CHANGELOG says
PATCH /drivesis accepted but doesn't move storage on FC < 1.15, so on an older binary children silently share the base again. That is the exact corruption this PR fixes, now with no signal. A documented minimum isn't enough; this needs to fail closed. Options: checkfirecracker --versionwhen the controller/CLI starts, orGET /vm/configafter the PATCH and refuse to resume unless the drive'spath_on_hostis the backing. - Read-only rootfs snapshots pay a full copy per child for nothing. The CLI read-only (squashfs) bake records
snap.rootfs = cfg.rootfs, and restore clones wheneverself.rootfs.is_some(). Every child of a read-only tag now copies the squashfs and PATCHes a drive that can never be written.Snapshotdoesn't persist the drive's read-only flag (your ask #2 on #317), so the fix probably starts there: record it at bake and skip backings for read-only drives. - Cost on filesystems without reflink. On ext4 (the default on most hosts, including our test box) each child is a full streamed copy of the rootfs. The copies run sequentially in the loop before any load, so
fork -n Ncosts N × rootfs in both disk and spawn latency, with no warning. At minimum: say once per restore that the fallback happened, gate on free space (theavailable_byteshelper from #316), and run the copies in the per-child threads. Whether ext4 hosts should get an explicit opt-in/refusal instead of silent full copies is a product call worth making in this PR.
Non-blocking
sweep_stranded_backingsscansstd::env::temp_dir()forforkd-daemon-*. It matches where the controller creates work dirs only while both use the sameTMPDIR, so please derive both from one helper.- The upgrade note (re-bake every tag when moving to FC ≥ 1.15) is a release-level decision. We'll schedule it with the release rather than let it land silently in
dev.
I can't exercise the KVM path on my side (the test host has no KVM), so your measured evidence plus a rerun after these changes will be what we rely on.
0x40209409 encodes a 32-byte payload; FICLONE is _IOW(0x94, 9, int) = 0x40049409. The kernel answered ENOTTY, which the fallback reads as no reflink, so every reflink_copy streamed a full copy on every filesystem. Reported by @jrimmer in deeplethe#321. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the thorough review, and for landing the FICLONE fix as #322. A correction first: my earlier comment said the FICLONE constant was "corrected in this PR". It wasn't — that commit never reached this branch. #322 is the fix; I'll rebase onto Plan for this PR:
I'd like to split 4 (cost without reflink) into a follow-up PR, since it carries the product call you raised (silent full copy vs. opt-in vs. refusal on ext4). My proposal there: warn once per restore on fallback, gate on The non-blocking items (one helper for the work-dir root; the upgrade note belonging to the release) I'll take here. I'll rerun the evidence table on KVM after the changes and post the results. |
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.
60d0b8c to
3f98a20
Compare
|
Rebased onto A correction that removes blocker 2's premiseThe "Firecracker ≥ 1.15, else
So there is no version floor and no re-bake: the upgrade note is gone from CHANGELOG and What changed
Evidence:
|
| check | result |
|---|---|
| two concurrent children of one writable tag each write their own backing | TICK…24 → …34 in each backing |
| the tag's rootfs through that | byte-identical |
| branch a child with the detour; source after resume | back on its own backing (…36 → …46) |
| the branch's rootfs after the source resumes | unchanged, holds the snapshot instant (…35) |
| kill the source | its backing is reclaimed |
| restore the branch after its source is gone | loads; the child continues from …35 on its own backing; branch rootfs unwritten |
| control: same branch without the detour, kill source, restore | PUT /snapshot/load returned 400 … Failed to restore devices … No such file — the defect reproduced |
| read-only tag, two children | both restored, no backing made, nothing to detour |
Not covered yet
- This ran on aarch64 KVM through
forkd-vmmdirectly, without netns, so multi-child batches ran as two single-child batches of the same tag. The controller's BRANCH/suspend handlers are covered by build, clippy and unit tests, not yet by a KVM run through the daemon. I'll rerun the full table through the controller on our x86 host (v1.17.0) and post it here before this leaves draft. - The two
forkd-vmmtests that fail on this machine (apply_diff_copies_only_data_regions,assemble_chain_memory_produces_correct_bytes) fail identically ondev: they assume 4 KiB pages and this host has 16 KiB.
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.
|
CI note: bd01919 fixes a flaky test from #314, not part of this change. The |
Fixes the defect in #317 — concurrent children of one tag sharing a writable rootfs — and carries the version policy that comes with it.
The mechanism, and why it is this one
Neither released Firecracker has
drive_overrides: I checked the v1.16.2 and v1.17.0 binaries, not the source, and neither contains the string. So the load-time override from #5774 is not available in a released build.What a released Firecracker does have is
PATCH /drives/{drive_id}on a restored VM, and it moves the device's storage rather than just its config. Measured: after re-pointing a child's rootfs, a write made inside the guest landed in the new backing and not in the original file.So each child gets its own reflink clone of the tag's rootfs and is re-pointed at it before it runs:
The ordering is load-bearing, and step 2 is why. Loading paused 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.Fail closed. A child whose rootfs cannot be re-pointed is not resumed. It would run against the shared base, which is the defect, so the failure surfaces and the child is dropped (killing its Firecracker) rather than run unsafely.
Two API details worth having:
PATCH /drivesrequiresdrive_idin the body (without it a 400 reads as a body error, not a capability error), andPUT /drivesto add a drive post-restore fails withPCI is not enabledsince forkd bootspci=off— irrelevant here, relevant if anyone wants per-child extra devices later.Evidence
child-1: AAA=1 BBB=0,child-2: AAA=0 BBB=1)DELETESIGKILLthe controller, let systemd restart it: 2 backings → 0POST /v1/snapshotsnow writesrootfsintosnapshot.json; daemon branches inherit itThe crash case is interesting and shaped the sweep. On a restart, systemd kills the unit's cgroup, so the children die with the controller; the reap finds dead processes and correctly prunes rather than kills, so nothing reaches
Vm::dropand nothing used to remove the backing — while the watchdog skips the directory because its socket file is still present. A sweep now runs before the reap, when those directories are unowned.Its gate is process ownership, not file presence: a directory is skipped while any live Firecracker's
--api-sockpoints inside it, and if any live Firecracker cannot be read, the sweep removes nothing. Leaking a backing costs disk; deleting a running VM's backing destroys its disk mid-job, so every ambiguity resolves toward keeping it.The per-kill reclaim is kept alongside the sweep rather than replaced: it covers a process verified ours and confirmed dead at kill time, which the pre-reap sweep cannot see.
Consequences to plan for
PATCH /driveswithout moving storage. That was secondhand and wrong: measured on stock v1.5.1, v1.7.0, v1.10.1, v1.12.1, v1.14.1 and v1.17.0, a restored VM's writes move to the new file and/vm/configreports it. Existing snapshots keep loading; nothing needs re-baking. Every re-point is still verified by reading/vm/configback, and a child is not resumed unless it reports its own backing.--live. It carriesMemBackendConfig::shared, andforkd-vmmsendssharedin exactly one place — the memfd path live-fork uses. Everything else runs on stock Firecracker.std::env::temp_dir()-derived (now from one helper shared with the sweep). On a host where/tmpis tmpfs that charges a child's rootfs copy to RAM;TMPDIRrelocates it./dev/vdb:VolumeSpec.read_onlydefaults tofalseand the path is frozen, so children of a tag carrying one all open the same file. Out of scope here; worth its own issue.Not covered by the evidence
Both crash runs exercised the cgroup-kill flavour, because the children were spawned without a memory limit and so died with the controller. The other flavour — Firecracker outliving the controller, e.g. children in their own per-VM cgroup — exercises the kill path, which is verified through a plain
DELETEbut not yet through a crash.Snapshots with no recorded rootfs keep the previous behaviour. Daemon-baked snapshots, branches and workspace suspends all record one now.