Repository navigation
feat(sandboxd): reconcile live read-only volume attachments - #339
Merged
CMGS merged 57 commits intoOct 17, 2026
Merged
Conversation
Contributor
|
Thanks for the PR and the detailed write-up. Attaching a dataset to a running claim is a use case we want, and most of the pieces are already in place: claim-time volumes are hot-attached today ( What we want to keep
What we'd like changed
Next steps on our side |
Contributor
|
The cocoon side is merged: cocoonstack/cocoon#282 (6d76309).
For this PR, that means:
The change is on cocoon As before, once the PR is revised along the lines above, we will run the hardware checks on our testbed: real CH hotplug and unplug, a busy guest unmount, and a daemon restart with a change in flight. |
minatoaquaMK2
changed the title
CMGS
requested changes
Oct 16, 2026
CMGS
left a comment
CMGS
left a comment
Contributor
There was a problem hiding this comment.
thanks, this is close. the shape is right now: no Transition across device calls, no new lock, a declarative GET/PUT/PATCH /volumes with volumes_want and the union held, idempotency left to cocoon, and python parity. local go test -race on pool, engine and server passes, and go vet on e2e is clean.
must fix (inline, plus one here):
- the cocoon floor. cocoon v0.7.2 is out and carries cocoonstack/cocoon#282. every put re-attaches kept volumes, so a cocoon without #282 likely fails on the second call. set
RequiredCocooninengine/engine.gotov0.7.2. it only warns today, so also refuse put and patch with a clear error below the floor. - auth on the volume endpoints, removal after tenant removal, a hold leak on release, and the ci failure: see inline.
should fix: inline. the biggest one is the re-apply of every kept volume on each change.
style: inline. asl and the comment budget, plus the python layout.
once these are in, we run it on our testbed with cocoon v0.7.2: real ch hotplug and unplug, a busy guest unmount, and a sandboxd restart mid-change.
Contributor
Author
|
Local validation for PR #339 on 2026-10-16. The report and one-off result files are kept out of the PR file diff. The caller tutorial and reusable hardware program remain in the source.
The production code after the hardware revision is unchanged. The later source changes rename two test-local variables and group the test helpers with one shared held-run setup. The final documentation cleanup does not change runtime behavior. All 29 distinct software checks have passing latest results.
Go used the repository-pinned 1.27.1 toolchain. Root permission tests dropped The real node used the official Cocoon v0.7.2 release, Git
The fault observer forwards ordinary commands to official Cocoon. It blocks only after a successful real attach, or injects exit 73 before VM removal. It does not emulate version support, device idempotence, or successful removal. The original VMM PID The baseline hardware smoke returned HTTP 500 for busy cwd. The candidate returns 409. Its repeated-target observation window contains no disk call. The invalid PUT preserves the exact settled GET view and guest Reproduce the software checks from the sandboxd workflow and Python workflow. The hardware instructions describe the existing program and isolated fault hooks. The caller tutorial explains replacement and retry. Missing guest eject ACK remains unverified. The pause experiment did not establish ACK absence because official Cocoon resumed the guest before detach. Separate real crashes at every guest umount, host detach, journal, or fsync boundary and physical host power loss were not tested. The real daemon used Catalog names must keep the same immutable backing image while held. The journal cannot detect an operator repointing a name after restart. Journal v2 requires draining live claims and retirement work before downgrade. A VMM that dies while holding volumes leaves the sandbox release-only. These are local results. Remote CI is not claimed as passed. The earlier GitHub workflows at Complete software suite attempts and final helper rerunDiagnostic regression before and after its fixReal hardware smoke outputZero retained disk calls and final cleanup{
"start_ns": 1792161535537904360,
"end_ns": 1792161535607563767,
"disk_calls": []
}
{
"candidate": "cc444095b3ba000de9ac11d7ef0674e386153804",
"claims": 0,
"volume_retirements": 0,
"vms": [],
"runtime_sockets": [],
"owned_taps": [],
"vm_pids_gone": {
"1082008": true,
"1084550": true,
"1087371": true
},
"isolated_daemon_stopped": true,
"existing_services_stopped": [],
"retained_device_calls": 0,
"fault_markers_remaining": []
}Actual invalid PUT, unchanged GET, and guest data{
"sandbox": "sb_d578a0cb4762e49f",
"status": 400,
"failure": {
"volumes_want": [
{
"name": "dataset-v1",
"mount": "/datasets/invalid"
}
],
"volumes": [
{
"name": "dataset-v1",
"mount": "/datasets/invalid"
}
],
"states": [
{
"name": "dataset-v1",
"state": "attached"
}
],
"converging": false,
"error": "invalid volume request: duplicate volume \"dataset-v1\""
},
"before": {
"volumes_want": [
{
"name": "dataset-v1",
"mount": "/datasets/invalid"
}
],
"volumes": [
{
"name": "dataset-v1",
"mount": "/datasets/invalid"
}
],
"states": [
{
"name": "dataset-v1",
"state": "attached"
}
],
"converging": false
},
"after": {
"volumes_want": [
{
"name": "dataset-v1",
"mount": "/datasets/invalid"
}
],
"volumes": [
{
"name": "dataset-v1",
"mount": "/datasets/invalid"
}
],
"states": [
{
"name": "dataset-v1",
"state": "attached"
}
],
"converging": false
},
"marker_before": "v1\n",
"marker_after": "v1\n",
"vm_pid": 1087371,
"vm_pid_exists": true,
"invalid_put_disk_calls": [],
"PASS": true,
"release_status": 204
}Baseline busy-cwd result |
minatoaquaMK2
marked this pull request as ready for review
October 16, 2026 14:45
Contributor
|
thanks for round 3. every must-fix and should-fix from the last review is in, and it holds up on our testbed: official cocoon v0.7.2 + CH v54, real hotplug/unplug, busy cwd and bind mount → 409, kill -9 of sandboxd mid-change → restart → retry converges with the same VMM pid, tenant/sandbox token scoping, the dev-build 503, capture refused without a trim, and the full single-node e2e twice, no regressions. the rest is on us, and we'll do it on this branch so main never sees an intermediate state. maintainer edits are enabled, so we'll push a few commits on top of yours and squash-merge with you as the author. what we'll change and why:
we'll re-run the same hardware matrix after the changes and post the numbers here before merging. thanks again, this was a lot of careful work. |
…ence In-process, a teardown already keeps its holds in pendingRemovals until cocoon confirms the VM gone; after a restart, the hotplug image lock refuses a conflicting attach. The v2 envelope broke rolling sandboxd back, added a map clone and marshal to every claims commit and 2-3 full rewrites to a release with volumes, and the fence turned one stuck orphan into a node-wide 400 for every volume claim. quarantineClaim keeps main's egress handling.
An absent field reads back as equal to Volumes, so a claims commit and a list row cost the same as before live volumes, a legacy journal loads without a second decode, and an older daemon still parses the file. The claim path stores one slice for both sets instead of cloning it.
…rror ErrTenantStoreDown and ErrPoolStoreDown wrap the pgx error, whose text names the PostgreSQL host and user, and poolErrHTTP fell back to err.Error() for them on every route that maps pool errors.
…ll mountinfo mountinfo records the kernel-resolved path, so a mount under a symlinked parent read as a bind of its own device: a repeated PUT answered 409 and a live removal stayed refused until release. The check now resolves the requested path with realpath only when the device sits elsewhere. The mountinfo read had the 64 KiB sysfs cap, which a few dozen overlay mounts in the guest exceed; it gets its own 1 MiB cap. In the same functions: silkdOutput carries an exec's stdout, mountMode is the one ro/rw mapping, refuseOtherMounts names what volumeUnmounted did, and the parameter and field names match the rest of the package.
…and keeps Retry-After The handler hand-rolled writePoolErr's loop and dropped its Retry-After, so a tenant or pool-set store outage on the volume routes answered 503 without the hint every other route sends.
…s read-only probe
rollbackClaim dropped a published claim without cancelling a live volume run and freed heldVolumes(sb), which misses additions the run reserved before its target commit and removals committed while the claim was failing. The leaked reader kept every later writable claim of that volume busy until restart; it now takes the run's holds as Release and reap do.
The run touched the sandbox only after converging, so an idle sweep could pick a sandbox that was attaching its first volume before prepare reserved any hold, hibernate it, and fail the request with ErrPaused.
…anged target stageVolumeTarget holds Transition only for the bookkeeping; the claims write (an fsync under sync_claims) happens after it, so exec and port dials, which take Transition to wake, no longer wait on it. An identical PUT or PATCH reserves nothing, changes nothing and writes no journal. A volume step builds its applied set inside the one locked section that commits it, a run that lost its claim answers ErrUnknownSandbox rather than a cancellation, a failed target commit is not filed under the last precleaned volume, and the two retry hints are constants. The claims DTO carries volumes_want as a plain omitzero slice.
…heir env siblings
…en volume test tables Write-only fakeManager fields and the never-set snapSaveErr go. The server tests build every node through serveTestServer and newMeshTestServer, so all of them close their relays, and requests with a bearer token use doReq. The pool tests share waitPoolSettled and newTenantVolumeManager, the restart budget test reuses its assertion helper, and shadowed names, verb-less Errorf calls and helpers split around types are fixed.
ac70e41 touched the sandbox before validation, so a rejected or paused request kept refreshing LastSeen: a client retrying PUT on a hibernated claim held off archive_after forever. The stamp now follows the running check and still precedes every device call.
2726e7f only narrowed the type hints, so an untyped caller's set_volumes("data") or patch_volumes(remove="old") still sent one volume per character.
Contributor
|
pushed 24 commits on top of fea3531 (head 4f4ef60). @minatoaquaMK2 please take a look, especially at the behavior changes in the second list. what changed:
behavior worth a second pair of eyes:
hardware, official cocoon v0.7.2 + CH v54, at 9236121:
|
…alls A failed intent write keeps the target in memory for recommit, so a retry of the same PUT matched the unchanged-target shortcut, skipped the commit and attached while the journal still lacked the intent; a restart then lost the attachment's hold. The shortcut now requires a journal in step with memory, and otherwise commits first and fails before any device call.
The realpath lookup ran only when our device already sat at another path, so a new volume aimed at a symlinked alias of an occupied mount point saw no mount, stacked on top of the other device, and could not be removed later. Every live mount check now resolves the requested path first; it costs one guest exec on the live path only.
A journal write that failed after a device change filed "volume change failed" on that volume; the retry that converged it neither found it pending nor cleared it, so GET kept showing the stale error on an attached volume.
…s a guest without realpath -m 09924f6 ran `realpath -m` on every check and trimmed all whitespace: a mount path ending in a space read back as a bind of its own device, and a busybox or toybox-without-PATH guest failed every live add and removal. A path that mountinfo already lists is canonical and needs no lookup, only the trailing newline is trimmed, and a failed lookup checks the path as given, as before 09924f6, with a warning.
Mount targets are validated as absolute paths, so -- guards nothing, and a missing target already falls back to the literal path; plain realpath runs on every guest toolchain.
…s realpath lookups The trailing-space case now sits behind a symlink so TrimSpace fails it, and every case pins its realpath call count so dropping the listed-path shortcut fails too.
Guest images are Ubuntu with coreutils, so realpath fails only on a path with a missing component, and no mount can sit behind a path that does not exist.
…path After a mount refused because another device occupies the path, removing the pending volume hit the same conflict, so it could not be dropped until the guest unmounted the other device. Detach now unmounts only its own device at the path.
umount removes the top mount at a path, so with a guest mount stacked on the volume it removed the guest's mount and then refused. Detach now refuses before unmounting when another mount shares the volume's path.
…e path A guest bind of the volume's own subdirectory stacked on the volume shares its device, so the device-only check still unmounted the top mount, which was the guest's. Detach now unmounts only when the path holds exactly one mount and it is the volume's root.
Contributor
|
@minatoaquaMK2 thanks again. Since the last round we fixed most of the remaining issues on this branch (head 4c8d53b). All of them are covered by regression tests and verified on our testbed. The main ones:
Squash-merging now with you as the author. The claim issue that your v2 journal covered, where a VM's volume holds are lost across a sandboxd restart while a removal is pending, is tracked in #348. We'll fix it in the next PR. |
Contributor
Author
Thanks for the thoughtful reviews and all the hands-on work getting this over the finish line! I really appreciate the time you put into the implementation and hardware testing. Learned a lot from this collaboration. |
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.
A running sandbox can replace its read-only catalog datasets while keeping its VM, processes, and sessions. GET reads the current view, PUT converges a full desired set, and PATCH adds or removes names. The Go and Python SDK method signatures stay the same and retain the complete failure view.
Volume management uses the tenant API token and ownership check, matching
/env. New admissions alone require current tenant and catalog authorization. Held repair and removal remain available after ACL changes. Settled retained mounts make no device calls. Busy cwd, bind mounts, and foreign mounts return 409 with safe guest paths. The response distinguishes an active worker from an interrupted or failed change.Confirmed and desired sets remain the persistent authority. Release, reap, and startup quarantine transfer their holds into a bounded retirement record before removing a VM. Restart adopts those holds, and cleanup releases them only after confirmed VM removal and a committed retirement deletion. Device calls remain outside
Transition. Capture rejects held volumes before trim.This stays within #339. Live changes cover read-only mounted catalog entries. Existing writable and attach-only entries remain immutable. The runtime floor is Cocoon v0.7.2. Journal format v2 requires draining claims and retirement work before downgrade. Catalog names must keep the same backing image while held. A sandbox whose VMM dies with volume holds remains release-only.
Local validation passed on Linux.
The validation results in the PR discussion contain exact revisions, reproducible commands, raw logs, failed attempts, and cleanup evidence. Missing eject ACK was not established by the pause experiment. Separate hardware crashes at every unmount, detach, or fsync boundary and physical power loss remain untested. The process-restart smoke used
sync_claims: false.Update after maintainer revisions (2026-10-17)
The original description above is retained as history. This section describes the changes from
fea3531through37253a6and supersedes the earlier statements where they differ.claims.jsonreturns to the flat claim map. The v2 envelope,volume_retirements, andunknownVolumeVMsfence are removed.volumes_wantis stored only when it differs fromvolumes; an absent field means they are equal. In-process teardown still retains holds until VM removal is confirmed. After restart, conflicting attaches rely on the Cloud Hypervisor hotplug image lock. The persistent retirement guarantee described above no longer applies. The maintainer leaves the existing restart gap to separate work.Transition. A settled, unchanged target skips journal writes. Failed writes preserve the in-memory state and retry persistence in the background, including after a device change, rather than restoring fields that no longer match the guest.Retry-After. Claim removal also clears volume diagnostics.status >= 400.-liveorVOLUME_LIVE=1. The continuity program and Python fault hooks are removed. Fixtures are shared, ordering tests usesynctest, and documentation and code style follow the existing project conventions.The earlier validation results apply to their recorded revisions, including the retirement implementation that has now been removed. The maintainer reports hardware and e2e results at
9236121. Three later commits update activity handling, Python string validation, and test cleanup, so that report does not establish complete hardware validation of37253a6. The maintainer's rationale explains the compatibility and recovery tradeoffs.