Repository navigation
pool: register runtime volume catalog entries - #382
Open
minatoaquaMK2 wants to merge 27 commits into
Open
minatoaquaMK2 wants to merge 27 commits into
minatoaquaMK2 wants to merge 27 commits into
Conversation
Load root-token registrations before claim recovery and refuse unregister while claims, operations or VM cleanup retain a reference. Keep detached references protected until their removal is committed.
minatoaquaMK2
marked this pull request as draft
October 26, 2026 13:05
A teardown that takes over an in-flight volume run's holds now parks them on the run, and finishVolumeRun releases them once. This drops the done poll, the requeues that re-ran vm rm every reap tick, and the ignored bool result of finishVolumeTeardown.
A stopped VM holds no volumes whether or not cocoon marks it stale, and a busy stale create now gets the same classification before it is queued and again when its retry inspects: running with a disk list takes per-volume holds, stopped or absent takes none. Only an Inspect error, a running VM without a disk list (cocoon v0.7.3 omits empty lists and swallows DiskList errors) or any other state keeps the node-wide fence. The table test covers each case by which claims and registrations are refused, with the disk list appearing only after the sweep so both retry paths classify.
The only owner caller checked its run under the same lock, so the check takes the owner ID instead of re-deriving it. It no longer adopts holds: each caller adopts the applied list it already holds and will store, under the same m.mu, so admission neither allocates nor takes a second list that must mirror the resolved one.
Every claimed volume is already counted in the admission holders, and the claim store keeps a per-name count for unacknowledged references, so the walk over every claim under m.mu adds nothing.
This reverts the Transition release around confirmVolumesClean that "pool: persist runtime volume registrations" introduced earlier in this PR. The clean check is one marker stat; dropping Transition around it let a lifecycle change interleave between reservation and staging for no gain.
The pool already rejects an invalid name with the same 400, so the handler check goes; the pool now validates before touching the pending catalog. The handler maps pool errors before context errors, so an uncertain catalog write still answers 503 without its own case. config.Load defaults static directio, and normalization builds the catalog entry directly instead of round-tripping through one-caller registration helpers. Marshaling the catalog record cannot fail: every string in it came through a UTF-8 validating JSON decode or a name regex.
Merge overlapping catalog, ACL, retirement and server tests into tables without per-case branching, share one failed-detach helper, and restore the base claim save helper and capture-test fixtures instead of rewriting their call sites. Drop cases for record file guards that no longer exist, rows that re-test the shared strict body decoder or static path validation, and a config test the pool ACL test covers. Every refusal and recovery path keeps a behavioural assertion.
The catalog map is replaced on every write, never edited, and a name a claim holds cannot be unregistered or rebound while held, so the snapshot taken with the hold state serves the whole pass instead of taking m.mu again for every mount.
Runtime registration rewrote resolveVolumes and the entry resolver but not the attach-only handling, so the two comments explaining why the flag is passed to validation and then cleared still apply.
The checkpoint count covers current and retained references, while the volume count covers retained references only, so the shared "volumes" names hid that difference. Name the count, its helper and the fast-path flag after the retained set they summarize, and seed a loaded claim's references with the same union commit uses instead of the change-aware helper with no previous list.
Registration and unregistration validate the name before they retry a pending catalog write, so an invalid name answers ErrBadVolume (400) instead of the pending write's 503. Nothing pinned the unregister refusal or that ordering. The uncertain-introduction test refused capture but never showed it working again once a snapshot acknowledges the recovery reference.
Keep the observable 503 instead of fence internals, plus the one assumption a reader needs: a stopped VM holds no volumes only because sandboxd controls VM starts and disk attachments in its namespace.
The tenants rule and the operator's backing duties were restated with small differences in the API reference, deploy guide, security model and live-volume example. Keep each in the API reference and link to it from the others. Drop the claim that temporary replacement files are not loaded: the daemon reads only the one record path, so the sentence described no behaviour of its own. Rewrap the overlong lines.
writeCatalog ignores the json.Marshal error, so a Go caller that bypasses the HTTP JSON decode could register a path that is not valid UTF-8 and persist a runtime volume catalog the daemon cannot load on next start. Validate the path in normalizeRegistration and reject it as ErrBadVolume (400 at the HTTP layer) so the catalog file always stays loadable. The name, directio, and tenant fields are already restricted to ASCII.
Contributor
Author
|
One thing I'd like your call on before this lands. With runtime registration, the root API token can register any absolute host path as a volume, writable and public if the request asks for it. That includes paths like My suggestion is an optional
That is a config field, a prefix check in |
Cocoon reports state=stopped, stale=true, pid=0 when it has lost a VM's PID file. That stop is inferred, not observed: the VMM can still be running and writing its disks. Since e942d31 every stopped VM took no holds, so a failed removal of such a VM lifted the node-wide fence and let a second writer attach, or the name be unregistered, while the old VMM might still hold it. Restore VMRecord.Stale and keep the fence for a stale stop on both the sweep and the busy stale-create retry, which share the classification. An ordinary stopped VM still takes no holds. The fence table gains a stale row for each path, and the inspect test again checks that the flag parses.
969b7ab dropped the claim walk from volumeReferenced on the premise that every claimed volume is counted in the admission holders or in the claim store's retained counts. AdoptArchived breaks that premise: it publishes the adopted claim in m.claimed without taking holders, and the store counts its volumes only once the claims.json commit starts. In that window unregister, or a re-registration under the same name, succeeded while a published claim still named the volume. Walk m.claimed under m.mu again, matching applied and wanted volumes by name without copying, ahead of the store check. The test adopts a peer's row that names a volume while an earlier claims.json write holds the adoption's own write back, and requires unregister to refuse.
The root API token can register any absolute host path as a volume, writable and open to every scope if the request asks for it, so the token reaches host files such as data_dir/claims.json over the network. Static config has the same reach but needs host access to change. An optional volume_roots list limits runtime registration to paths that, once cleaned, equal a root or sit under one on a separator boundary, so /srv/data2 does not match /srv/data and "/" admits everything. Anything else is the existing bad-volume error, a 400. The check is lexical, like static path validation; symlinks are not resolved. Config load rejects a relative or empty root and stores cleaned roots. Persisted registrations go through the same check at startup, so a narrowed list cannot leave an out-of-root registration claimable; like other invalid catalog metadata it prevents startup until the operator unregisters it or widens the roots. Static volumes are not checked, and an empty list keeps today's behaviour.
Operators need to know that the root token can register any absolute host path, and that volume_roots is the way to bound it. The config reference states the lexical rule, its symlink limit, the startup refusal and that static entries are exempt; security.md says to protect the token like host root or set the list, and links to the access-list rule instead of restating it.
Narrowing volume_roots left the daemon unable to start while any saved registration sat outside the new roots, so tightening the bound took the node down until the operator widened it again or edited runtime-volumes.json by hand. Startup now logs a warning naming the volume and leaves it out of the loaded catalog, so it cannot be claimed and the daemon serves the rest. The skip runs after the whole file validates, so malformed or conflicting metadata still refuses startup as before. A recovered claim that names the skipped volume keeps its holders and gets the same refusal as any name missing from the catalog. The entry stays on disk until the next registration change rewrites the file from the loaded catalog, which drops it.
Contributor
Author
|
I went ahead and added the
vet, the full race suite, and Linux and Darwin lint pass. asl shows the same five advisories as main. |
minatoaquaMK2
marked this pull request as ready for review
October 27, 2026 01:05
This branch has not been 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
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.
Follow-up to the #379 discussion. This PR covers runtime catalog registration.
Why
Controllers need to register volume backing paths without restarting sandboxd. Registrations must load before claim recovery, without waiting for the controller to reconnect.
What changed
/v1/volumes/{name}, requiring a configured API token. Repeating a normalized registration succeeds; changing an existing definition returns 409. Path validation and tenant ACLs follow static config. Empty tenant lists allow every tenant.runtime-volumes.jsonand load it before recovering claims. Attachment references remain in the claim journal.Scope
Live writable and per-volume attach-only support, and the controller sleep/wake flow, remain separate follow-ups. The API docs describe the controller's disk lifecycle, quiesce/unmount, reservations across sleep and access-gating responsibilities.
An unresolved catalog write fences admission only to its affected name; unrelated published volumes remain usable. Further catalog changes retry the accepted proposal first. A successful inspection of an ordinary stopped VM clears its inventory fence once cleanup is no longer busy. Unknown or inferred-stopped inventories remain fenced until retry or cleanup resolves them. Cocoon v0.7.3 omits disks for both empty inventories and failed queries, so an omitted list remains unknown. An accepted write can complete after a 503; callers retry the same request to confirm completion. Stale cleanup keeps main's assumption that no older Cocoon child attaches a disk after a post-failure inventory check.
Verification
Performance
Compared main
cf9280ewithf49765aon Linux amd64, Go 1.27.1,GOMAXPROCS=1and one pinned CPU. Each side ran five alternating two-second samples with 5,000 claims and 5,000 checkpoint references. The complete non-due replica scheduling scan checks all 5,000 states.Values are microseconds, median [min, max].
Both sides allocate 459,176 bytes and 20 allocations per scan. Medians are 5.3% and 0.3% higher here, with overlapping ranges. These five runs do not establish a stable difference or zero overhead.