Repository navigation
pool: allow live writable and attach-only volumes - #383
Draft
minatoaquaMK2 wants to merge 39 commits into
Draft
minatoaquaMK2 wants to merge 39 commits into
minatoaquaMK2 wants to merge 39 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.
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.
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.
An explicit empty list behaves like unset, and DELETE of a registration skipped at startup is a no-op until the next catalog write. Say both so operators are not surprised.
Controllers need explicit device-only attachment and live writable mounts. Preserve writer exclusion and failed-write references while extending the existing convergence API and SDKs.
Describe all four live volume modes and the coordinated daemon/SDK upgrade requirement so old default-mount behavior is not mistaken for attach-only support.
Keep the release and dirty-marker limits beside the API and make the replica tests match repository lint rules.
Accepted mapping inputs need conversion to plain dictionaries before JSON serialization. Preserve string shorthand without duplicating server validation.
The old read-only wording no longer describes the live API. Document writable and attach-only behavior and its remaining restrictions.
Turning off registry support first strands replica references. Document retirement checks and shared ownership risks before the operator disables failover.
Exercise the SDK and HTTP boundary for all four modes and both dirty-marker outcomes. Keep this proof with the live feature rather than the later controller workflow.
Disabling failover requires stopping both idle-time archives and archive-on-expiry claims. Name the configuration and renew operation so operators can stop new archive rows before draining the registry.
Keep Transition across the dirty-marker stat so lifecycle changes cannot interleave between reservation and staging. Release the manager mutex during the stat and keep device calls outside Transition. Both writer-admission callers already reject archived claims under the manager mutex. Remove those unreachable checks and keep the detach proof contract in a shorter comment. The pool and end-to-end suites cover the mixed GET-to-PUT contract, so remove the duplicate server test. Make marker-removal cases declare their setup and expected outcomes directly.
The handle copies volumes and volumes_want entries before returning a view or raising VolumeError, so malformed entries must still be rejected there. States are passed through without copying, so drop their unused entry scan while retaining the response envelope checks. Replace repeated mixed-mode GET-to-PUT scenarios with concrete SDK wire checks. Keep Go zero-field omission, Python explicit false and empty mount preservation, mountless response decoding, auth, partial errors, handle copies, and cancellation coverage. Put request expectations directly in the existing case tables and shorten the claim-wide attach-only comment without dropping caller responsibilities.
minatoaquaMK2
force-pushed
the
review/live-volume-modes-20261027
branch
from
October 28, 2026 09:39
dc7a4e3 to
e9c236d
Compare
Contributor
|
These changes look right in direction:
Two notes for when this is rebased on the slimmed #382:
|
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.
This is the live-volume follow-up to #382 in the #300 series, following the split agreed in #379. It depends on #382 and stays draft until that PR merges. The base is main, so GitHub currently also shows the catalog changes. Review only the live-volume changes.
Why
Controllers need to attach and detach writable disks on a running sandbox. Per-volume
attach_onlylets them mix mounted disks with devices whose mounts they manage themselves.What changed
attach_onlyin the API and Go and Python SDKs.Scope
Disk lifecycle and the combined controller sleep/wake example remain separate. Attach-only writers have no automatic dirty-marker protection. Changing a held volume's mode or mount requires removal first.
Upgrade the daemon and SDKs together. Older daemons can ignore the per-entry flag, and older Python clients reject it when callers reuse response entries in requests. The old claim-wide flag and saved empty-mount records remain supported.
Verification
At
e9c236d, race tests passed for engine, pool, server, types and the Go SDK. The two permission cases also passed as UID 65534; three PostgreSQL integration tests skipped without a database.The controller follow-up at
45bae1ccontains the same daemon and SDK implementation. Its 266 Python 3.9 tests passed, including 11 controller cases. A real Linux KVM run on that commit passed all four volume modes and detach → hibernate → daemon restart → wake → attach → mount, preserving disk identity and data with no writer errors. Busy unmount and a simulated lost detach response kept workload access closed. This is a single-node test, not proof of distributed reservations or all-node access gating.Earlier verification at
dc7a4e3:Performance
These measurements remain from
dc7a4e3; they were not rerun fore9c236d.Compared main
cf9280e, catalog09bb47band the earlier live headdc7a4e3on Linux amd64, Go 1.27.1,GOMAXPROCS=1, pinned to CPU 48. Five alternating two-second samples used 5,000 claims and 5,000 checkpoint references. Values are microseconds, median [min, max].dc7a4e3Each scan verifies all 5,000 states and uses 459,176 bytes and 20 allocations on every version. Commit uses about 43 KB and 19 allocations. Admission uses 584 bytes and six allocations on both measured versions. The live scan medians are 0.7% and 1.4% above the catalog parent, with overlapping ranges. These runs do not establish a stable difference. They do not measure fsync latency or end-to-end live attachment.