diff --git a/README.md b/README.md index 04468e8d..a69644d0 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ performance) — source in - `sandboxd/` — per-node control plane (Go): warm pools refilled from golden snapshot exports (online-retunable), claim/release/renew/hibernate/fork/ promote/checkpoint HTTP API, operator-catalog dataset volumes (read-only or - writable; read-only ones change on a running claim), signed preview URLs, + writable, mounted or attach-only, with live add and remove), signed preview URLs, the HTTP-upgrade byte relays to silkd and to any guest port, read-only file access to hibernated or archived claims from a warm VM of their pool (opt-in per pool `reader`), claim env with diff --git a/docs/deploy.md b/docs/deploy.md index f8020f0e..f9abff44 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -119,7 +119,8 @@ gets the intersection of the two. | `advertise_addr` | = `listen` | internal HTTP host:port for peer traffic and preview forwarding; also used by clients when `client_advertise` is unset. Must be routable when `listen` is a wildcard; a node with `mesh` set refuses to load while it names an unspecified host | | `client_advertise` | unset | client-facing HTTP(S) origin for this node, e.g. `https://node-a.sandbox.example.com`; published in owner, redirect, and peer responses. No path, query, fragment, or userinfo | | `bridges` / `networks` | unset | egress-lane attachment: a list of host bridge devices, or a list of CNI conflist names. Mutually exclusive; with neither set the node serves only the no-network lane. A Linux bridge holds at most 1024 ports (kernel `BR_MAX_PORTS`), so an N-entry list raises the node's egress ceiling to N×1024 — VMs spread over the list by a stable hash of the VM name, so size it with headroom (the spread is statistical, not exact). `bridges` keeps the raw TAP-on-bridge attachment (taps in the root netns, no per-VM network namespace or CNI plugin execution); `networks` runs the CNI chain per VM. [Guarded egress](egress.md) on the egress lane (an egress-lane pool policy or any tenant policy) needs `bridges` and rejects a CNI network at load; none-lane pool policies ride the proxy on either | -| `volumes` | unset | node-local catalog of operator-managed dataset images: `[ {"name":"imagenet","path":"/srv/datasets/imagenet.img","directio":"off","tenants":["acme"]}, {"name":"scratch-db","path":"/srv/datasets/scratch.img","writable":true} ]`. Names match `^[a-z][a-z0-9_-]{0,19}$` and cannot start with `cocoon-`; paths are absolute; `directio` is `on`, `off`, or `auto` and defaults to `off` for both read-only and writable entries. `tenants` is an optional access list: empty means every authenticated scope, and a listed name the tenant set does not hold grants nothing and logs a warning at boot (the [tenant API](sandboxd-api.md#tenants-v1tenants) may add it later); root always has access. `writable` (default `false`) lets a claim request `mode: "rw"` on that entry — see [Dataset volumes](#dataset-volumes). The catalog is intentionally not part of the cluster digest | +| `volumes` | unset | node-local catalog of operator-managed dataset images: `[ {"name":"imagenet","path":"/srv/datasets/imagenet.img","directio":"off","tenants":["acme"]}, {"name":"scratch-db","path":"/srv/datasets/scratch.img","writable":true} ]`. Names match `^[a-z][a-z0-9_-]{0,19}$` and cannot start with `cocoon-`; paths are absolute; `directio` is `on`, `off`, or `auto` and defaults to `off` for both read-only and writable entries. `tenants` is an optional [access list](sandboxd-api.md#volume-access-lists); a listed name the tenant set does not hold grants nothing and logs a warning at boot (the [tenant API](sandboxd-api.md#tenants-v1tenants) may add it later). `writable` (default `false`) lets a claim request `mode: "rw"` on that entry — see [Dataset volumes](#dataset-volumes). The catalog is intentionally not part of the cluster digest | +| `volume_roots` | unset | absolute directories that bound [runtime registration](sandboxd-api.md#put-v1volumesname): the registered path, once cleaned, must equal a root or sit under one, or the PUT returns 400. The check is lexical: `..` is resolved but symlinks are not followed, so a symlink under a root can point outside it. Static `volumes` entries are not checked. After narrowing, a registration already in `runtime-volumes.json` outside the roots is skipped with a warning at startup, so it cannot be claimed, and the next registration change drops it from the file. A DELETE of a skipped name that nothing references returns 204 without rewriting the file. Keep `data_dir` outside every root. Unset or an empty list accepts any absolute path | | `secrets` | unset | credentials the egress proxy injects by name: `[{"name": "gh", "header": "Authorization", "value_env": "GH_TOKEN"}]`. A pool or egress-class rule references the name (on a claim where both apply, only the pool rule's secret is injected); the value comes from the claim's `guest: false` env, then the node's environment, never this file. Without `value_env` the secret's name is the env to read, and only claims supply it. See [egress](egress.md#claim-env-and-secrets) | | `egress_internal_allow` | unset | CIDR prefixes re-admitted through the egress proxy's SSRF guard, node-wide (every pool and tenant); `prefix:port,port` scopes an entry to those ports, a bare prefix admits every port. Prefer the port form. Prefixes, not a permit-private switch — the guest bridges are themselves ULA/RFC1918. See [egress](egress.md) | | `egress_upstream` | unset | per-claim upstream proxies: `{"claim_env": "EGRESS_UPSTREAM", "allow": ["res.example.com", "10.1.0.0/16"]}`. A claim's `guest: false` entry under `claim_env` (`http://` or `socks5://`, optional `user:pass@`, or `direct`) routes its egress through that upstream, which must be on `allow` (host names, IPs, CIDR prefixes). See [upstream proxies](egress.md#upstream-proxies) | @@ -144,7 +145,7 @@ gets the intersection of the two. | `reader` (pool entry) | `false` | Serve read-only file calls on this pool's hibernated or archived claims from a warm VM of the same pool instead of waking them; needs `warm` of at least 1 and a cocoon whose `vm disk attach` takes `--from-snapshot` and `--from-dir`. See [Reading sleeping claims](#reading-sleeping-claims-without-waking-them). Config-owned like `capture_trim`: `PUT /v1/pools` rejects it | | `egress_classes` | unset | named tenant egress layers: `[{"name": "desk", "egress": {…}, "egress_upstream_env": "DESK_UPSTREAM"}]`. A tenant names one with `egress_class` through the [tenant API](sandboxd-api.md#tenants-v1tenants) and its claims take that policy intersected with the pool's. `egress` is required; `intercept` stays a pool-rule flag. Reloadable: a class's policy reaches live claims on their next request. A class removed while tenants still name it leaves them with no egress and is logged at load and at reload | | `egress_upstream_env` (pool or egress class entry) | unset | names a node environment variable holding the default upstream proxy URL for the pool's or class's claims (class before pool, a claim's own entry before both); needs `egress_upstream`, and the URL must be on its `allow`. Read and checked at startup. Config-owned like `egress`: `PUT /v1/pools` rejects it | -| `replicate_seconds` (pool entry) | 0 (off) | snapshot every running claim of this pool to the checkpoint store this often and point the claim's registry row at the snapshot, so another node restores the claim, memory and processes included, when this node is lost (see [running claims across node loss](cluster.md#running-claims-across-node-loss)). Each round pauses the guest for the length of a memory and disk capture. A restore resumes from the last committed replica and loses everything after it: up to the longer of the interval and one replication pass, plus the 5-second tick, plus one more pass, since a claim can run first in one pass and last in the next. A pass covers all the node's due claims of the replicating pools with bounded concurrency, so it grows with claim count and guest memory; the first round of a claim after a wake or adoption uploads the whole claim, so that pass is the long pass. The claim's row carries no retention deadline while it runs. Requires `archive_failover` and a `none`-lane pool, both checked at config load; a claim with volumes is skipped. Config-owned like `egress`: `PUT /v1/pools` rejects it. Restart only: a claim is replicated by its pool's setting from the moment it is claimed, so a reload that changes it is refused (a pool restarted with 0 drops its claims' rows and replicas on the first pass) | +| `replicate_seconds` (pool entry) | 0 (off) | snapshot every running claim of this pool to the checkpoint store this often and point the claim's registry row at the snapshot, so another node restores the claim, memory and processes included, when this node is lost (see [running claims across node loss](cluster.md#running-claims-across-node-loss)). Each round pauses the guest for the length of a memory and disk capture. A restore resumes from the last committed replica and loses everything after it: up to the longer of the interval and one replication pass, plus the 5-second tick, plus one more pass, since a claim can run first in one pass and last in the next. A pass covers all the node's due claims of the replicating pools with bounded concurrency, so it grows with claim count and guest memory; the first round of a claim after a wake or adoption uploads the whole claim, so that pass is the long pass. The claim's row carries no retention deadline while it runs. Requires `archive_failover` and a `none`-lane pool, both checked at config load; a claim with volumes is skipped. Config-owned like `egress`: `PUT /v1/pools` rejects it. Restart only: a claim is replicated by its pool's setting from the moment it is claimed, so a reload that changes it is refused (a pool restarted with 0 starts retiring its replicas while the registry remains enabled; see [Disabling archive failover](#disabling-archive-failover)) | | `storage` (pool entry) | unset (cocoon's default, 10G) | size of the copy-on-write disk the pool's golden is cold-booted with, in cocoon's own `--storage` spelling (`"40G"`, `"40GiB"`, `"40Gi"`: Docker/Kubernetes units, all binary) and passed to it verbatim; every clone, fork, checkpoint branch and promoted template of the pool inherits it. At least cocoon's default of 10G. Equal sizes spelled differently build the same golden; a changed size rebuilds the golden on the next build, and claims already out keep their disk. Config-owned like `egress`: `PUT /v1/pools` rejects it | | `max_claims` | 0 (unlimited) | node-wide cap on live claims; claim/fork/branch requests beyond it answer 429 with the pool state unharmed (on a cluster a non-volume claim tries a warm-peer redirect first; a volume claim answers the 429 with no redirect) | | `audit_log` | false | append every relayed frame that opens an RPC — its op + addressing fields, never payloads — to `/audit.jsonl`; continuation `data`, `data_end`, `stdin`, and `stdin_close` frames pass through unrecorded. The file is size-rotated with one `.1` backup. Records are `{t, id, op}` plus whichever addressing fields the op carries (`argv`, `path`, `dest`, `from`, `to`, `url`, `session`, `port`), plus `method` (`GET`, `CONNECT`, `SOCKS5`, …), `decision` (`allow`, `deny`, `rate_limited`), `secret` (the ref name, never its value), `upstream` (the [upstream proxy](egress.md#upstream-proxies)'s `host:port`, when one carried it), `tenant`, and on an allowed connection `src_addr` and `dst_addr` (the node-side and destination `ip:port` of the upstream TCP connection, the tunnel target through an upstream proxy) on `egress` records, which a relayed frame cannot set; preview accesses record as op `preview`, one per request; a tenant change records as op `tenants` with the `added`, `changed` and `removed` names, never a token. An opening frame whose first line exceeds 4 KiB records as op `oversized` with no addressing fields | @@ -247,9 +248,13 @@ the pool had `reader`, and its reads wake the claim as without the feature. ### Dataset volumes -Running claims can change mounted read-only catalog entries through the +Running claims can add or remove mounted and attach-only catalog entries in +read-only or writable mode through the [GET, PUT, and PATCH volume API](sandboxd-api.md#get-v1sandboxesidvolumes) -and both SDKs. The image must already exist on the owner node. Live changes +and both SDKs. Changing a held entry's descriptor requires successful removal +first. The [live volume contract](sandboxd-api.md#put-v1sandboxesidvolumes) +defines admission policy and dirty-marker handling. +The image must already exist on the owner node. Live changes require Cocoon v0.7.2 or newer for idempotent disk attach and detach. PUT and PATCH reject an older release, or a runtime whose version cannot be queried, with 503 before any intent, hold, or device change; a dev build is assumed @@ -323,10 +328,9 @@ Warm candidates retain their normal ranking, but a volume claim's candidate set, promoted-template intersection, and redirect follow the fleet-wide rule in [cluster](cluster.md#volumes-and-placement). -An empty catalog `tenants` list allows every authenticated scope; a nonempty -list limits the image to those tenants, while root always bypasses it. A -listed tenant the set no longer holds grants nothing and logs a warning at -boot. +Catalog `tenants` lists follow the +[access-list rule](sandboxd-api.md#volume-access-lists). A listed tenant the +set no longer holds grants nothing and logs a warning at boot. `GET /v1/volumes` reports the caller-visible fleet union and holder count, plus the answering node's current local availability and whether the entry is `writable`, without exposing host paths or node addresses. Applied names, @@ -338,7 +342,8 @@ below. With `directio=off`, readers share the host page cache; use `directio=on` when cache interference matters. A dataset mounted into an egress-lane sandbox can be uploaded wherever that tenant's egress policy permits, so treat the ACL and egress policy as one access -decision. Image replication, detach, and refcounting remain out of scope. +decision. Operators remain responsible for image distribution and cross-node +ownership. sandboxd tracks local holds and handles live detach through the volume API. #### Writable dataset volumes @@ -694,6 +699,36 @@ drains the warm pools; poll `GET /v1/info` until `claimed` reaches zero (or let the leases expire), then stop sandboxd. `DELETE /v1/drain` uncordons. The drain is not persisted — a restarted node serves again. +### Disabling archive failover + +Stop admitting claims that use `on_expire: archive`. Set the node and affected +pools' `archive_after_seconds` to `0`, and use +[`renew`](sandboxd-api.md#post-v1sandboxesidrenew) to switch existing active claims +to `on_expire: destroy` while keeping their leases valid. These changes stop +idle-time and expiry-triggered archives. Before setting `archive_failover` to `false`: + +1. Set every affected pool's `replicate_seconds` to `0` and restart its nodes + with failover and the registry still enabled. Keep the registry reachable + while replica retirement retries. +2. Check each node's `/claims.json` for affected claims. Wait until + `replica_ck` and `replica_pending` are absent after successful journal writes. + Use `sync_claims: true` when completion must survive power loss. +3. Wake or release existing registered archives while failover remains enabled. + Wait for `archive_pending` to clear in the journal. Verify that the affected + IDs have no rows in PostgreSQL's + `sandboxd_archives` table for your `meta_store.cell`. A consumed row is a + non-adoptable tombstone; wait for its cleanup retries to finish too. +4. Set `archive_failover` to `false` and restart the nodes. + +The public API omits replica journal fields. A fixed delay or an absence of +warnings does not prove retirement. Disabling failover first stops replica +retirement and leaves residual references that reject new live RW attachments. +A local archive can still wake and clear `archive_ck`, but that does not clear a +retained `replica_pending` or retire an old shared row. Release can remove local +state without reconciling that row. Re-enabling failover against stale rows can +fence or drop a local claim if another node has adopted it. Resolve registry +ownership before serving that claim again. + ### Shared tenant database With `meta_store` set, every node reads tenants from one PostgreSQL table diff --git a/docs/live-volumes.md b/docs/live-volumes.md index 790e0d21..da04c979 100644 --- a/docs/live-volumes.md +++ b/docs/live-volumes.md @@ -9,6 +9,13 @@ file. Keep each name bound to the same immutable image while held. Use Cocoon v0.7.2 or newer and set `SANDBOX_ADDR` and `SANDBOX_API_TOKEN` for the client. A sandbox token alone cannot manage volumes. +A configured node root token can +[register backing paths](sandboxd-api.md#put-v1volumesname) without editing +static config. Registration persists only node-local catalog metadata and does +not manage disk files. Runtime `tenants` lists follow the +[access-list rule](sandboxd-api.md#volume-access-lists). The API also supports +[writable and attach-only entries](sandboxd-api.md#put-v1sandboxesidvolumes). + Pause application reads before replacement. Close files, mmap mappings, cwd, and bind mount references on the old disk. Replacement unmounts the old disk before mounting the new one, so the path has a short unavailable interval. @@ -58,9 +65,10 @@ with client.new("rt:24.04", ttl_seconds=900) as sb: A successful run reads `v1`, then `v2`, and preserves every identity assertion. The repeated full target verifies that a settled mount can be retained. -This example starts without other volumes. If your sandbox has writable or -attach-only volumes, retain them in a full PUT or use PATCH to replace only -the dataset names. `None`, a nil Go slice, and a missing `volumes` field do not +This example starts without other volumes. Retain any volumes you intend to +keep in a full PUT, or use PATCH to replace only the dataset names. All mounted +and attach-only entries support live removal in either mode. +`None`, a nil Go slice, and a missing `volumes` field do not mean an empty target. Go uses the same owning handle and client API token. diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index 31a255cd..58f3e121 100644 --- a/docs/sandboxd-api.md +++ b/docs/sandboxd-api.md @@ -8,8 +8,8 @@ All bodies are JSON. Three token kinds: preview mint) and the tenant-scoped listings/deletes below; everything a tenant creates is stamped with its name. Operator surfaces (the per-id sandbox reads, `PUT /v1/sandboxes/{id}/instance-metadata`, `GET /v1/info`, - `PUT /v1/pools`, `POST/DELETE /v1/drain`, `GET /metrics`, - `GET /v1/checkpoints/{id}/blob`) + `PUT /v1/pools`, `PUT/DELETE /v1/volumes/{name}`, `POST/DELETE /v1/drain`, + `GET /metrics`, `GET /v1/checkpoints/{id}/blob`) answer a tenant token `403` — authenticated but not authorized; an unknown token stays `401`. On a node with a shared [`meta_store`](deploy.md#shared-tenant-database), a tenant token the node @@ -87,13 +87,12 @@ Auth: `Authorization: Bearer ` (when configured). defaults to `/volumes/`; a custom value must be absolute and clean, outside the guest OS tree, unique, and non-nesting within the request. `mode` is `"ro"` (default, omitted) or `"rw"`; `"rw"` requires the catalog - entry's `writable: true` (see [deploy](deploy.md#dataset-volumes)) -- `volumes_attach_only` (default `false`) attaches every requested volume - without mounting it, handing the whole mount contract to the workload. It - requires at least one volume, and rejects any entry carrying a `mount` — - the path is meaningless when the caller mounts the device. Everything below - describes the default, eager behaviour, which is completely unchanged; the - attach-only contract is in [its own section](#attach-only-volumes) + entry's `writable: true` (see [deploy](deploy.md#dataset-volumes)). + `attach_only: true` attaches that entry without mounting it and requires an + empty or omitted `mount`. Mounted and attach-only entries can share a list +- `volumes_attach_only` (default `false`) is the legacy claim-wide form of + `attach_only`. The server expands it onto every entry before validation. + It requires at least one volume and rejects a nonempty `mount`. Success: @@ -135,12 +134,14 @@ an existing populated guest directory for the claim's life. ### Attach-only volumes -`"volumes_attach_only": true` stops after the attach. The device appears in -the guest, nothing is mounted, and the echoed entries carry no `mount` key: +`"attach_only": true` stops after the attach for that entry. The device appears +in the guest without a mount. Responses preserve `attach_only: true` and omit +`mount`: ```json {"id": "sb_…", "token": "…", - "volumes": [{"name": "imagenet"}, {"name": "scratch-db", "mode": "rw"}]} + "volumes": [{"name": "imagenet", "attach_only": true}, + {"name": "scratch-db", "mode": "rw", "attach_only": true}]} ``` Find each device by its virtio serial, which is the catalog name — poll this, @@ -221,7 +222,7 @@ an unknown and a forbidden volume return the same error text. Errors: 400 unknown template axis, metadata over a bound (the message names it), an unknown `on_expire`, invalid/duplicate volumes, -`volumes_attach_only` with no volumes or with an entry carrying a `mount`, +`volumes_attach_only` with no volumes, or an attach-only entry with a nonempty `mount`, `mode: "rw"` against a non-writable entry, or a volume that is unknown or forbidden (the latter two are deliberately indistinguishable), or @@ -255,6 +256,86 @@ list; the field is emitted (as `true`) only for a writable entry this node declares and omitted otherwise (peer-only rows never carry it), so a read-only entry's response is byte-identical to v1. +## PUT /v1/volumes/{name} + +Requires the configured root API token. A tenant token or a node without an +`api_token` returns 403. Sandbox, unknown, and missing tokens return 401 when +`api_token` is configured. The URL supplies the name. Unknown body fields, +duplicate fields, and trailing JSON return 400. + +```json +{"path": "/srv/datasets/data.img", "directio": "off", + "writable": false, "tenants": ["acme"]} +``` + +- `name` matches `[a-z][a-z0-9_-]{0,19}` and cannot start with `cocoon-`. +- `path` uses static config's absolute-path validation. When the node sets + [`volume_roots`](deploy.md#configuration), the cleaned path must equal or sit + under one of them. Registration does not open the backing or check its type + or availability. A missing backing keeps its registration but cannot be + attached. +- `directio` is `on`, `off`, or `auto`, and defaults to `off`. +- `writable` defaults to `false` and controls writable claim admission. +- `tenants` is the entry's [access list](#volume-access-lists). + +Success is 204. An identical normalized registration is idempotent. A different +binding, changed fields, or a static name or cleaned path collision returns 409. +Invalid fields return 400. GET keeps its existing projection without host paths +or tenant lists. Runtime entries support the existing live volume API. + +Persistence uncertainty returns 503 with `Retry-After`. An accepted change may +complete through background retry or a later PUT or DELETE for any name. +A 503 can also precede acceptance. Retry the exact request after repairing +storage to establish completion. Cancellation does not undo an accepted write. +Until persistence succeeds, new admission refuses the affected name while +unrelated published names remain usable. Every catalog writer retries the +accepted change before accepting another change. + +The node stores metadata in `data_dir/runtime-volumes.json` independently of +`sync_claims` and loads it before claim recovery. New records use mode 0600. +Malformed or conflicting metadata prevents startup. Registration creates, +grows, and deletes no backing disks. The catalog is node-local with one +sandboxd writer. + +The operator or controller owns each backing: supply an image Cocoon supports, +keep the path and its symlink target stable while registered, coordinate +symlink and hardlink aliases, cross-node ownership, and availability, and keep +backings outside daemon cleanup directories such as `/readers`, +`/goldens`, and `/fork-*`. + +### Volume access lists + +Static and runtime entries share one `tenants` rule. An omitted or empty list +permits every authenticated scope. A nonempty list permits only claims owned +by the listed tenants, and root-owned claims can use any entry. Live additions +check the claim owner's list even when root sends the request; held names +finish repair or removal without a new grant, and a recovery-only reference +grants no fresh attachment. Names use the tenant API syntax: runtime +registration rejects an invalid name, while static config logs a warning and +skips it, so a nonempty static list with no valid names permits no tenant. A +valid name for a tenant that does not exist yet grants nothing until the +tenant is created. + +## DELETE /v1/volumes/{name} + +Auth and name validation match PUT. Deletes only runtime registration metadata. +Success, including an absent unreferenced name, is 204. Static names and names +referenced by a claim, an in-flight operation, cleanup, or an unacknowledged +recovery snapshot return 409. Persistence uncertainty returns 503 with +`Retry-After` and the same retry semantics as PUT. A failed DELETE can complete +later. An empty GET view does not establish that recovery references retired. + +Volume claims and registrations also return 503 with `Retry-After` while the +node removes a leftover VM whose attached volumes it cannot determine, +including one cocoon reports `stale` because it only inferred the stop from a +lost PID file. Any other stopped leftover VM holds none, which assumes sandboxd +controls VM starts and disk attachments in its namespace. Volume-free claims +are unaffected. + +Before downgrading to a daemon without runtime registration, drain runtime +references or supply equivalent static volume config and finish pending live +volume changes. + ## GET /v1/sandboxes/{id}/volumes Returns the persisted desired set (`volumes_want`), confirmed applied set @@ -277,7 +358,8 @@ when the volume is in both sets, `pending` when desired but not yet confirmed, or `detaching` when applied but no longer desired. An interrupted device call can leave the actual guest state different from the confirmed set. A failed PUT or PATCH adds a top-level `error` naming the refusal, and the affected -state may carry its own `error`. `converging` is true only while a request worker is running. +state may carry its own `error`. `converging` is true while a request worker +is running or a recovery-only reference awaits an acknowledged claim write. `pending` with `converging: false` requires an explicit retry; there is no background convergence. GET retains the last safe per-volume error for the current daemon lifetime. After daemon restart, unfinished states report an @@ -290,22 +372,32 @@ Replaces the full desired set and converges synchronously. Auth matches GET. The owner tenant's catalog access list applies to newly admitted names, including for root. Retaining, repairing, or removing held names does not reauthorize them. A new image must exist on the owning node. The request must include a non-null `volumes` -array. An explicit empty array removes all mutable entries. +array. An explicit empty array removes all volumes. ```json {"volumes": [{"name": "imagenet", "mount": "/datasets/training"}]} ``` -Only mounted read-only catalog entries can change live (`mode` omitted or -`ro`). A PUT must retain every existing writable or attach-only entry unchanged. -The claim-time name, mount, and eight-volume limits apply to the desired set. -An omitted mount defaults to `/volumes/`, except that a held attach-only -entry is retained by sending it as the claim returned it, without `mount`. To -change an existing name's -mount or mode, first remove that name and wait for successful convergence. -An identical target can be retried. Claims with `on_expire: archive` must change -that policy before adding a volume. - +Mounted read-only, mounted writable, attach-only read-only, and attach-only +writable entries can change live, including mixed lists. The claim-time name, +mount, and eight-volume limits apply. An omitted mount defaults to +`/volumes/` unless that entry has `attach_only: true`. Responses carry +that flag directly, so a GET target can be sent back with PUT. Omitting the flag +or sending `false` does not inherit an existing attachment's shape. + +To change a held name's mount, mode, or `attach_only`, first remove the name and +wait for successful convergence. An identical target can be retried. Claims +with `on_expire: archive` must change that policy before adding a volume. + +New live writable attachments require a non-replicating pool, including a +promoted template's source pool. A replicating pool returns 409 before changing +intent, holds, or devices. Initial claim-time writable volumes remain supported. +An existing or pending replica, or this claim's unacknowledged checkpoint +retirement, returns 503 with `Retry-After`. A replica or archive intent left +with `archive_failover` disabled returns 409 because it cannot retire through +the registry. A retained local checkpoint reference alone still returns 503. +Volume mutation does not evict remote replicas. Read-only additions, exact +retries of held writers, and removals remain available under these restrictions. The sandbox must be running. Paused, archived, and failed sandboxes return 409 without waking. A failed sandbox holding volumes is release-only after its VMM exits; PUT with an empty target cannot restore its processes. Missing, invalid, @@ -326,13 +418,29 @@ unchanged. An empty patch retries the current desired set. ``` Adding an identical entry or removing an absent valid name is harmless. -Duplicate names, a name in both arrays, or changes to writable or attach-only -entries return 400. Removal refuses busy or foreign mounts and uses neither +Duplicate names, a name in both arrays, or changes to a held entry's mount, +mode, or `attach_only` return 400. Removal refuses busy or foreign mounts and uses neither lazy nor forced unmount. A foreign mount, another bind mount, or a completed ordinary unmount with nonzero exit returns 409 with the guest mount path and a safe per-volume reason. Guest transport failures remain server errors; timeouts return 504 and cancellation returns 408. +A mounted writable attachment marks the image dirty before the guest can write. +Live removal clears the marker only when this call observes and normally +unmounts the exact writable whole-device filesystem, finds no residual device +mounts, and confirms host detach. Both mount and superblock options must be +writable. A missing device or pre-unmounted filesystem does not prove clean +unmount. The marker remains on uncertainty and blocks subsequent readers. +Attach-only writers have no automatic dirty-marker protection. The caller owns +mounting, flushing, unmounting, recovery, and fencing before reuse. + +The per-entry field requires coordinated daemon and SDK releases. Older daemons +can ignore `attach_only` in claims, and older Python SDKs reject the field when +returned descriptors are reused in requests. Responses without the field do +not safely roundtrip attach-only intent through new clients. The server still +accepts the legacy claim-wide request flag and normalizes old persisted empty +mounts when loading claims. + ### Partial failure and recovery PUT and PATCH return the GET response shape on success. A convergence error diff --git a/docs/sdk-python.md b/docs/sdk-python.md index d726d366..c03e19fc 100644 --- a/docs/sdk-python.md +++ b/docs/sdk-python.md @@ -161,13 +161,29 @@ peer advertising both the template and every volume; when that intersection is empty, one volume holder may self-verify access to a shared template store before provisioning. -## Changing read-only datasets +## Changing live volumes Use `sb.get_volumes()`, `sb.set_volumes([...])`, and `sb.patch_volumes(add=[...], remove=[...])` on the same running sandbox. These methods use the client's API token on the owning node; a sandbox token cannot -manage datasets. `set_volumes([])` clears mutable entries; `None` is rejected. -PUT must retain writable and attach-only entries. PATCH retains omitted names. +manage volumes. `set_volumes([])` detaches all entries; `None` is rejected. +PATCH retains omitted names. Parameters take lists of names or `Volume` mappings. +The SDK sends volume policy to sandboxd and preserves `attach_only` directly. +Use matching daemon and SDK releases for attach-only GET-to-PUT retries. + +```python +from cocoonsandbox import Volume + +volumes: list[Volume] = [ + {"name": "dataset", "mount": "/datasets"}, + {"name": "work", "mode": "rw", "attach_only": True}, +] +view = sb.set_volumes(volumes) +``` + +The caller mounts, flushes, and unmounts attach-only devices. See the +[live volume contract](sandboxd-api.md#put-v1sandboxesidvolumes) for +writer exclusion, replica refusals, and dirty-marker limits. Each method takes `timeout=` (default: the client timeout, 120 s); a large replacement can need longer, and a call that times out cancels the server run, so retry the same target. diff --git a/docs/sdk.md b/docs/sdk.md index 2d66f2aa..af049af4 100644 --- a/docs/sdk.md +++ b/docs/sdk.md @@ -234,16 +234,17 @@ checkpoint branches support neither volumes nor a claim reference in this versio `Checkpoint.New`: the labels follow the [claim bounds](sandboxd-api.md#post-v1claim), and a branch never inherits its source's labels or expiry action. -For datasets chosen after the sandbox starts, set the desired read-only mounts -on the same claim: +For datasets chosen after the sandbox starts, set the desired volumes on +the same claim: ```go view, err := sb.SetVolumes(ctx, []sandbox.Volume{ {Name: "imagenet", Mount: "/datasets/training"}, + {Name: "work", Mode: "rw", AttachOnly: true}, }) view, err = sb.GetVolumes(ctx) view, err = sb.PatchVolumes(ctx, sandbox.VolumePatch{Remove: []string{"imagenet"}}) -// An explicit empty slice clears mutable mounts. A nil slice is rejected. +// An explicit empty slice detaches all volumes. A nil slice is rejected. view, err = sb.SetVolumes(ctx, []sandbox.Volume{}) ``` @@ -256,6 +257,13 @@ view = sb.patch_volumes(remove=["imagenet"]) view = sb.set_volumes([]) # None is rejected. ``` +`Volume.AttachOnly` leaves mounting and unmounting to the caller. Each entry +can independently use `Mode: "rw"` and `AttachOnly: true`. The SDK preserves +these fields directly and leaves name, mode, and mount policy to sandboxd. +Use matching daemon and SDK releases for attach-only GET-to-PUT retries. +See the [live volume contract](sandboxd-api.md#put-v1sandboxesidvolumes) for +writer exclusion, replica refusals, and dirty-marker limits. + Each response contains `volumes_want`, `volumes`, `states`, and `converging`. Go exposes them as `SandboxVolumesResponse.VolumesWant`, `.Volumes`, `.States`, and `.Converging`. `VolumePhase` has `VolumeAttached`, `VolumePending`, and @@ -274,11 +282,14 @@ the handle retains its last known sets. Retry an interrupted operation with the same full PUT target, or set a new target to recover. `Client.Sandboxes` exposes `VolumesWant` and `Volumes` for inspection with an API token. -A PUT must retain existing writable and attach-only entries unchanged. PATCH -cannot remove or change them. Paused sandboxes are not woken, and images must -exist on the owner node. Admission holds cover the union of desired and applied +PUT and PATCH can add or remove mounted and attach-only entries in either mode. +Changing a held entry's mount, mode, or `attach_only` requires successful +removal first. See the [live volume contract](sandboxd-api.md#put-v1sandboxesidvolumes) +for admission policy and dirty-marker handling. Paused sandboxes are not woken, +and images must exist on the owner node. Admission holds cover the union of desired and applied sets. A settled retained entry receives no new attach or mount calls. `converging` -is true only during an active request. Pending or detaching entries with false +is true during an active request or while a recovery-only reference awaits an +acknowledged claim write. Pending or detaching entries with false require an explicit retry. Daemon restart replaces unfinished diagnostics with an interrupted message. Capture is refused while either set is nonempty. Release cancels convergence and hands held volumes to teardown. See the diff --git a/docs/security.md b/docs/security.md index 6c519f49..107bd7cc 100644 --- a/docs/security.md +++ b/docs/security.md @@ -94,15 +94,25 @@ rather than confirming existence, and operator surfaces answer tenants 403. The volume catalog is an operator-owned data boundary. A volume name and its access list must mean the same thing fleet-wide, although membership is -node-local. An empty entry `tenants` list permits every authenticated scope; a -nonempty list permits only those named tenants, while the root token always -has access. Config load rejects an access-list name that is not a configured -tenant. Claim lookup returns byte-identical errors for an unknown and a -forbidden name, and catalog discovery filters before replying, so a tenant -cannot enumerate restricted entries by probing. Gossip carries names only. +node-local. Entry `tenants` lists follow the [access-list +rule](sandboxd-api.md#volume-access-lists). Claim lookup returns byte-identical +errors for an unknown and a forbidden name, and catalog discovery filters before +replying, so a tenant cannot enumerate restricted entries by probing. Gossip +carries names only. Neither gossip, discovery, persisted claims, usage events, nor the sandbox index exposes host image paths or access lists. +Runtime registration and unregistration require a configured root `api_token`; +these endpoints return 403 when the token is unset. New catalog records use +mode 0600. Reads and replacements use the daemon's filesystem permissions. +Registration uses static configuration's path validation plus, when set, the +[`volume_roots`](deploy.md#configuration) bound. It does not verify backing type +or pin backing identity; the operator duties this leaves are listed under +[PUT /v1/volumes/{name}](sandboxd-api.md#put-v1volumesname). Without +`volume_roots` the root token can register any absolute host path, writable if +requested, for every scope its [access list](sandboxd-api.md#volume-access-lists) +permits. Protect the root token like host root, or set `volume_roots`. + Read-only is integrity protection for the shared image, not confidentiality. Mounting a dataset into an egress-lane sandbox gives that sandbox an export path to every destination its tenant egress policy permits; review the volume access @@ -116,8 +126,8 @@ immutable and publish a new name/path for new content. Changing a running claim's volumes needs the root token or the owning tenant's API token. A sandbox token cannot, so a guest holding its own sandbox token -cannot attach another dataset, and every addition passes the same access list -as a claim. +cannot attach another dataset, and every addition passes the same +[access list](sandboxd-api.md#volume-access-lists) as a claim. A writable entry (`writable: true`) adds a channel the read-only model does not have: whichever tenant is permitted to claim it `rw` changes what every diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index cf305baa..a97279b6 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -764,7 +764,7 @@ func TestAttachOnlyVolumeEndToEnd(t *testing.T) { if err != nil { t.Fatalf("attach-only claim: %v", err) } - if want := []sandbox.Volume{{Name: "scratch", Mode: "rw"}}; !slices.Equal(sb.Volumes, want) { + if want := []sandbox.Volume{{Name: "scratch", Mode: "rw", AttachOnly: true}}; !slices.Equal(sb.Volumes, want) { t.Errorf("claim volumes %+v, want %+v", sb.Volumes, want) } applied := []string{"attach:scratch:rw"} @@ -800,7 +800,7 @@ func TestAttachOnlyVolumeWireShape(t *testing.T) { { "attach-only omits the mount", `{"template":"rt:24.04","volumes_attach_only":true,"volumes":[{"name":"scratch","mode":"rw"}]}`, - map[string]any{"name": "scratch", "mode": "rw"}, + map[string]any{"name": "scratch", "mode": "rw", "attach_only": true}, }, { "eager claim is unchanged", diff --git a/e2e/fakeengine_test.go b/e2e/fakeengine_test.go index 9bbc9113..e57368b5 100644 --- a/e2e/fakeengine_test.go +++ b/e2e/fakeengine_test.go @@ -17,6 +17,11 @@ import ( "github.com/cocoonstack/sandbox/sdk/go/silkd/silkdtest" ) +type fakeVolumeMount struct { + name string + rw bool +} + type fakeEngine struct { real *engine.Engine dir string @@ -27,6 +32,7 @@ type fakeEngine struct { volumeOps []string volumeVMs map[string]bool guestEnvs map[string]string + mounts map[string]map[string]fakeVolumeMount seq int readerRoot string @@ -42,6 +48,7 @@ func newFakeEngine(dir string) *fakeEngine { socks: map[string]string{}, volumeVMs: map[string]bool{}, guestEnvs: map[string]string{}, + mounts: map[string]map[string]fakeVolumeMount{}, } } @@ -66,6 +73,7 @@ func (f *fakeEngine) Remove(_ context.Context, name string) error { _ = l.Close() } delete(f.listeners, name) + delete(f.mounts, f.socks[name]) delete(f.socks, name) if f.volumeVMs[name] { delete(f.volumeVMs, name) @@ -190,10 +198,14 @@ func (f *fakeEngine) DiskAttach(_ context.Context, vmName string, spec engine.Vo return nil } -func (f *fakeEngine) MountVolume(_ context.Context, _, name, mount string, rw bool) error { +func (f *fakeEngine) MountVolume(_ context.Context, sock, name, mount string, rw bool) error { f.mu.Lock() defer f.mu.Unlock() f.volumeOps = append(f.volumeOps, "mount:"+name+":"+mount+":"+volumeMode(rw)) + if f.mounts[sock] == nil { + f.mounts[sock] = map[string]fakeVolumeMount{} + } + f.mounts[sock][mount] = fakeVolumeMount{name: name, rw: rw} return nil } @@ -201,10 +213,10 @@ func (f *fakeEngine) MountLiveVolume(ctx context.Context, sock, name, mount stri return f.MountVolume(ctx, sock, name, mount, rw) } -func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { +func (f *fakeEngine) UnmountVolume(_ context.Context, sock, mount string) error { f.mu.Lock() defer f.mu.Unlock() - f.volumeOps = append(f.volumeOps, "umount:"+mount) + f.unmountVolumeLocked(sock, mount) return nil } @@ -215,14 +227,38 @@ func (f *fakeEngine) SyncGuest(_ context.Context, _ string) error { return nil } -func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount string) error { - if err := f.UnmountVolume(ctx, sock, mount); err != nil { - return err - } +func (f *fakeEngine) DetachVolume(_ context.Context, _, sock, name, mount string) (bool, error) { f.mu.Lock() defer f.mu.Unlock() + entry, mounted := f.mounts[sock][mount] + normal := mount != "" && mounted && entry.name == name && entry.rw + if mount != "" && mounted && entry.name == name { + f.unmountVolumeLocked(sock, mount) + } + for _, other := range f.mounts[sock] { + if other.name == name { + return false, &engine.VolumeMountConflict{Message: "volume still has a simulated guest mount"} + } + } f.volumeOps = append(f.volumeOps, "detach:"+name) - return nil + return normal, nil +} + +func (f *fakeEngine) unmountNamedVolume(name, mount string) error { + f.mu.Lock() + defer f.mu.Unlock() + for sock, mounts := range f.mounts { + if mounts[mount].name == name { + f.unmountVolumeLocked(sock, mount) + return nil + } + } + return fmt.Errorf("volume %q is not mounted at %q", name, mount) +} + +func (f *fakeEngine) unmountVolumeLocked(sock, mount string) { + f.volumeOps = append(f.volumeOps, "umount:"+mount) + delete(f.mounts[sock], mount) } func (f *fakeEngine) volumeOpsLog() []string { diff --git a/e2e/volume_catalog_test.go b/e2e/volume_catalog_test.go new file mode 100644 index 00000000..3858f3e5 --- /dev/null +++ b/e2e/volume_catalog_test.go @@ -0,0 +1,74 @@ +package e2e + +import ( + "encoding/json/v2" + "net/http" + "slices" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/tenants" + sandbox "github.com/cocoonstack/sandbox/sdk/go" +) + +func TestRuntimeVolumeRegistrationThroughHTTP(t *testing.T) { + st := startTenantStack(t, "node-token", []tenants.TenantSpec{ + {Name: "acme", Token: "acme-token"}, {Name: "beta", Token: "beta-token"}, + }, []config.VolumeSpec{{Name: "public", Path: writeVolumeImage(t, "public.img", "shared")}}) + for name, body := range map[string]map[string]any{ + "home": {"path": writeVolumeImage(t, "home.img", "shared"), "writable": true}, + "private": {"path": writeVolumeImage(t, "private.img", "private"), "tenants": []string{"acme"}}, + } { + raw, err := json.Marshal(body) + if err != nil { + t.Fatal(err) + } + if status, resp := rawJSON(t, st, http.MethodPut, "/v1/volumes/"+name, string(raw)); status != http.StatusNoContent { + t.Fatalf("register %s: status=%d body=%s", name, status, resp) + } + } + home := sandbox.Volume{Name: "home", Mount: "/mnt/home"} + for _, tc := range []struct { + token string + want []string + private bool + }{{"acme-token", []string{"home", "private", "public"}, true}, {"beta-token", []string{"home", "public"}, false}} { + client, err := sandbox.Connect(st.addr, sandbox.WithAPIToken(tc.token)) + if err != nil { + t.Fatal(err) + } + volumes, err := client.Volumes(t.Context()) + names := make([]string, 0, len(volumes)) + for _, volume := range volumes { + names = append(names, volume.Name) + } + if err != nil || !slices.Equal(names, tc.want) { + t.Fatalf("catalog for %s: names=%v want=%v err=%v", tc.token, names, tc.want, err) + } + claim, err := client.New(t.Context(), "rt:24.04", sandbox.WithVolumes(home)) + if err != nil || !slices.Equal(claim.Volumes, []sandbox.Volume{home}) { + t.Fatalf("claim with public runtime volume: %+v, %v", claim, err) + } + defer claim.Close() + if _, err = claim.SetVolumes(t.Context(), []sandbox.Volume{}); err != nil { + t.Fatal(err) + } + view, err := claim.PatchVolumes(t.Context(), sandbox.VolumePatch{Add: []sandbox.Volume{home, {Name: "private"}}}) + if (err == nil) != tc.private || tc.private && !slices.Contains(view.Volumes, home) { + t.Fatalf("live attach for %s: view=%+v err=%v", tc.token, view, err) + } + } + caller := *st + caller.token = "beta-token" + var responses []string + for _, name := range []string{"private", "unknown"} { + status, body := rawJSON(t, &caller, http.MethodPost, "/v1/claim", `{"template":"rt:24.04","volumes":[{"name":"`+name+`"}]}`) + if status != http.StatusBadRequest { + t.Fatalf("claim privacy for %s: status=%d body=%s", name, status, body) + } + responses = append(responses, string(body)) + } + if responses[0] != responses[1] { + t.Fatalf("forbidden and unknown responses differ: %s versus %s", responses[0], responses[1]) + } +} diff --git a/e2e/volume_mutation_test.go b/e2e/volume_mutation_test.go index 5fcedef8..38dd119c 100644 --- a/e2e/volume_mutation_test.go +++ b/e2e/volume_mutation_test.go @@ -3,6 +3,7 @@ package e2e import ( "errors" "net/http" + "os" "slices" "strings" "testing" @@ -11,6 +12,61 @@ import ( sandbox "github.com/cocoonstack/sandbox/sdk/go" ) +func TestLiveWritableRemovalClearsMarkerThroughSDK(t *testing.T) { + for _, tt := range []struct { + name string + beforeDetach func(*stack) error + markerErr error + readerBlocked bool + }{ + {name: "daemon unmount", beforeDetach: func(*stack) error { return nil }, markerErr: os.ErrNotExist}, + {name: "external unmount", beforeDetach: func(s *stack) error { + return s.eng.unmountNamedVolume("data", "/volumes/data") + }, readerBlocked: true}, + } { + t.Run(tt.name, func(t *testing.T) { + image := writeVolumeImage(t, "data.img", "original-data") + stack := startTenantStack(t, "node-token", nil, []config.VolumeSpec{{Name: "data", Path: image, Writable: true}}) + writer, err := stack.client.New(t.Context(), "rt:24.04") + if err != nil { + t.Fatal(err) + } + defer writer.Close() + reader, err := stack.client.New(t.Context(), "rt:24.04") + if err != nil { + t.Fatal(err) + } + defer reader.Close() + if _, err = writer.SetVolumes(t.Context(), []sandbox.Volume{{Name: "data", Mode: "rw"}}); err != nil { + t.Fatal(err) + } + if _, err = os.Stat(image + ".dirty"); err != nil { + t.Fatalf("writer has no dirty marker: %v", err) + } + if _, err = reader.SetVolumes(t.Context(), []sandbox.Volume{{Name: "data"}}); err == nil { + t.Fatal("reader admitted while writer was attached") + } + if err = tt.beforeDetach(stack); err != nil { + t.Fatal(err) + } + if _, err = writer.PatchVolumes(t.Context(), sandbox.VolumePatch{Remove: []string{"data"}}); err != nil { + t.Fatal(err) + } + _, markerErr := os.Stat(image + ".dirty") + if !errors.Is(markerErr, tt.markerErr) { + t.Fatalf("dirty marker: got %v, want %v", markerErr, tt.markerErr) + } + _, err = reader.SetVolumes(t.Context(), []sandbox.Volume{{Name: "data"}}) + if (err != nil) != tt.readerBlocked { + t.Fatalf("reader admission after detach: blocked=%t err=%v", tt.readerBlocked, err) + } + if data, readErr := os.ReadFile(image); readErr != nil || string(data) != "original-data" { + t.Fatalf("detach changed backing contents: %q, %v", data, readErr) + } + }) + } +} + func TestLiveVolumesEndToEnd(t *testing.T) { image := writeVolumeImage(t, "dataset.img", "dataset-bytes") stack := startTenantStack(t, "node-token", nil, []config.VolumeSpec{{Name: "dataset", Path: image}}) @@ -52,7 +108,7 @@ func TestRejectedVolumeTargetExplainsRefusalWithoutChangingState(t *testing.T) { t.Fatal(checkErr) } _, err = sb.SetVolumes(t.Context(), []sandbox.Volume{{Name: "dataset"}, {Name: "dataset"}}) - if refusal, ok := errors.AsType[*sandbox.VolumeError](err); !ok || refusal.Status != http.StatusBadRequest || !strings.Contains(refusal.Message, "duplicate volume") { + if refusal, ok := errors.AsType[*sandbox.VolumeError](err); !ok || refusal.Status != http.StatusBadRequest || !strings.Contains(refusal.Message, "duplicates name") { t.Errorf("invalid target explanation: %v", err) } view, err := sb.GetVolumes(t.Context()) @@ -77,3 +133,57 @@ func TestEmptyVolumePatchChangesNothing(t *testing.T) { t.Fatalf("empty patch: %+v err=%v", got, err) } } + +func TestMixedLiveVolumeModesThroughSDK(t *testing.T) { + volumes := []sandbox.Volume{ + {Name: "mounted-ro"}, + {Name: "mounted-rw", Mode: "rw"}, + {Name: "device-ro", AttachOnly: true}, + {Name: "device-rw", Mode: "rw", AttachOnly: true}, + } + var catalog []config.VolumeSpec + for _, volume := range volumes { + catalog = append(catalog, config.VolumeSpec{ + Name: volume.Name, Path: writeVolumeImage(t, volume.Name+".img", "seed"), Writable: volume.Mode == "rw", + }) + } + stack := startTenantStack(t, "node-token", nil, catalog) + sb, err := stack.client.New(t.Context(), "rt:24.04") + if err != nil { + t.Fatal(err) + } + defer sb.Close() + view, err := sb.PatchVolumes(t.Context(), sandbox.VolumePatch{Add: volumes}) + if err != nil { + t.Fatal(err) + } + want := slices.Clone(volumes) + want[0].Mount, want[1].Mount = "/volumes/mounted-ro", "/volumes/mounted-rw" + if !slices.Equal(view.Volumes, want) || !slices.Equal(sb.Volumes, want) { + t.Fatalf("mixed SDK attachment = %+v, handle=%+v, want %+v", view, sb.Volumes, want) + } + status, body := rawJSON(t, stack, http.MethodGet, "/v1/sandboxes/"+sb.ID+"/volumes", "") + if status != http.StatusOK || strings.Count(string(body), `"attach_only":true`) != 4 { + t.Fatalf("mixed HTTP view lost explicit attach_only in desired or applied entries: status=%d body=%s", status, body) + } + view, err = sb.GetVolumes(t.Context()) + if err != nil { + t.Fatal(err) + } + view, err = sb.SetVolumes(t.Context(), view.VolumesWant) + if err != nil || !slices.Equal(view.Volumes, want) || view.Converging { + t.Fatalf("mixed GET-to-PUT = %+v, err=%v", view, err) + } + view, err = sb.SetVolumes(t.Context(), []sandbox.Volume{}) + if err != nil || len(view.Volumes) != 0 || len(view.VolumesWant) != 0 || view.Converging { + t.Fatalf("mixed removal = %+v, err=%v", view, err) + } + initial, err := stack.client.New(t.Context(), "rt:24.04", sandbox.WithVolumes(volumes...)) + if err != nil { + t.Fatalf("mixed initial claim: %v", err) + } + defer initial.Close() + if !slices.Equal(initial.Volumes, want) { + t.Fatalf("mixed initial claim = %+v, want %+v", initial.Volumes, want) + } +} diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index fba3efba..03a8a694 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -3,6 +3,7 @@ package config import ( "cmp" + "context" "encoding/base64" "encoding/json/jsontext" "encoding/json/v2" @@ -18,6 +19,7 @@ import ( "strings" "github.com/docker/go-units" + "github.com/projecteru2/core/log" "github.com/cocoonstack/sandbox/sandboxd/egress" "github.com/cocoonstack/sandbox/sandboxd/store" @@ -275,8 +277,10 @@ type Config struct { DiskReserve *DiskReserve `json:"disk_reserve,omitempty"` Volumes []VolumeSpec `json:"volumes,omitempty"` - Mesh *MeshConfig `json:"mesh,omitempty"` - Pools []PoolSpec `json:"pools"` + // VolumeRoots bounds runtime registration paths only; static volumes stay unchecked. + VolumeRoots []string `json:"volume_roots,omitempty"` + Mesh *MeshConfig `json:"mesh,omitempty"` + Pools []PoolSpec `json:"pools"` } // HasEgress reports whether the node can attach egress-lane VMs. @@ -398,28 +402,23 @@ func (c *Config) validateReplication() error { } func (c *Config) validateVolumes() error { - names := make(map[string]struct{}, len(c.Volumes)) - paths := make(map[string]string, len(c.Volumes)) - for _, volume := range c.Volumes { - if !types.ValidVolumeName(volume.Name) { - return fmt.Errorf("volume name %q must match %s and not start with cocoon-", volume.Name, types.VolumeNameRe) - } - if _, ok := names[volume.Name]; ok { - return fmt.Errorf("duplicate volume name %q", volume.Name) - } - names[volume.Name] = struct{}{} - if !filepath.IsAbs(volume.Path) { - return fmt.Errorf("volume %q path must be absolute", volume.Name) - } - if other, ok := paths[filepath.Clean(volume.Path)]; ok { - return fmt.Errorf("volume %q shares its path with volume %q: admission is keyed by name", volume.Name, other) - } - paths[filepath.Clean(volume.Path)] = volume.Name - if !types.ValidDirectIO(volume.DirectIO) { - return fmt.Errorf("volume %q directio must be on, off, or auto, got %q", volume.Name, volume.DirectIO) + for i, root := range c.VolumeRoots { + if !filepath.IsAbs(root) { + return fmt.Errorf("volume_roots %q must be an absolute path", root) } + c.VolumeRoots[i] = filepath.Clean(root) } - return nil + validation := slices.Clone(c.Volumes) + for i, volume := range c.Volumes { + validation[i].Tenants = slices.DeleteFunc(slices.Clone(volume.Tenants), func(tenant string) bool { + if types.NameRe.MatchString(tenant) { + return false + } + log.WithFunc("config.validateVolumes").Warnf(context.Background(), "volume %q: skipping invalid tenant %q", volume.Name, tenant) + return true + }) + } + return ValidateVolumeCatalog(validation) } func (c *Config) validatePreview() error { @@ -637,6 +636,37 @@ func (c *Config) validateMetaStore() error { return nil } +// ValidateVolumeCatalog checks the shared static and runtime catalog rules without I/O. +func ValidateVolumeCatalog(volumes []VolumeSpec) error { + names := make(map[string]struct{}, len(volumes)) + paths := make(map[string]string, len(volumes)) + for _, volume := range volumes { + if !types.ValidVolumeName(volume.Name) { + return fmt.Errorf("volume name %q must match %s and not start with cocoon-", volume.Name, types.VolumeNameRe) + } + if _, ok := names[volume.Name]; ok { + return fmt.Errorf("duplicate volume name %q", volume.Name) + } + names[volume.Name] = struct{}{} + if !filepath.IsAbs(volume.Path) { + return fmt.Errorf("volume %q path must be absolute", volume.Name) + } + if other, ok := paths[filepath.Clean(volume.Path)]; ok { + return fmt.Errorf("volume %q shares its path with volume %q: admission is keyed by name", volume.Name, other) + } + paths[filepath.Clean(volume.Path)] = volume.Name + if !types.ValidDirectIO(volume.DirectIO) { + return fmt.Errorf("volume %q directio must be on, off, or auto, got %q", volume.Name, volume.DirectIO) + } + for _, tenant := range volume.Tenants { + if !types.NameRe.MatchString(tenant) { + return fmt.Errorf("volume %q tenant %q must match %s", volume.Name, tenant, types.NameRe) + } + } + } + return nil +} + // Load reads a JSON config file, applies defaults, and validates. func Load(path string) (*Config, error) { raw, err := os.ReadFile(path) //nolint:gosec // path is the operator-supplied -config flag diff --git a/sandboxd/config/config_test.go b/sandboxd/config/config_test.go index 06c4fe3c..b1e51586 100644 --- a/sandboxd/config/config_test.go +++ b/sandboxd/config/config_test.go @@ -167,6 +167,8 @@ func TestLoadRejectsInvalid(t *testing.T) { {"duplicate volume name", `{"pools":[],"volumes":[{"name":"data","path":"/srv/datasets/a.img"},{"name":"data","path":"/srv/datasets/b.img"}]}`, "duplicate volume"}, {"relative volume path", `{"pools":[],"volumes":[{"name":"data","path":"datasets/a.img"}]}`, "path must be absolute"}, {"bad volume directio", `{"pools":[],"volumes":[{"name":"data","path":"/srv/datasets/a.img","directio":"yes"}]}`, "directio must be"}, + {"relative volume root", `{"pools":[],"volume_roots":["srv/datasets"]}`, "volume_roots"}, + {"empty volume root", `{"pools":[],"volume_roots":[""]}`, "volume_roots"}, {"reader without warm", `{"pools":[{"template":"rt:24.04","warm":0,"reader":true}]}`, "reader needs warm"}, } { t.Run(tt.name, func(t *testing.T) { @@ -292,7 +294,7 @@ func TestLoadReadsReader(t *testing.T) { } func TestLoadAcceptsVolumes(t *testing.T) { - path := writeConfig(t, `{"pools":[],"volumes":[ + path := writeConfig(t, `{"pools":[],"volume_roots":["/srv/models/"],"volumes":[ {"name":"imagenet","path":"/srv/datasets/imagenet.img"}, {"name":"weights-llama","path":"/srv/datasets/llama.img","directio":"on"}]}`) cfg, err := Load(path) @@ -303,6 +305,9 @@ func TestLoadAcceptsVolumes(t *testing.T) { cfg.Volumes[1].DirectIO != types.DirectIOOn { t.Errorf("volumes = %+v", cfg.Volumes) } + if !slices.Equal(cfg.VolumeRoots, []string{"/srv/models"}) { + t.Errorf("volume_roots = %q, want the cleaned root with static volumes outside it accepted", cfg.VolumeRoots) + } } func TestLoadAcceptsVolumeTenantAccessList(t *testing.T) { @@ -463,6 +468,19 @@ func TestLoadAcceptsAShardedNetworkList(t *testing.T) { } } +func TestValidateVolumeCatalogTenantNames(t *testing.T) { + for _, tenant := range []string{"", "has space", "_leading", strings.Repeat("a", 64)} { + spec := VolumeSpec{Name: "data", Path: "/images/data", DirectIO: types.DirectIOOff, Tenants: []string{tenant}} + if err := ValidateVolumeCatalog([]VolumeSpec{spec}); err == nil { + t.Errorf("invalid tenant %q accepted", tenant) + } + } + spec := VolumeSpec{Name: "data", Path: "/images/data", DirectIO: types.DirectIOOff, Tenants: []string{"future-tenant", "team/project:v1"}} + if err := ValidateVolumeCatalog([]VolumeSpec{spec}); err != nil { + t.Fatalf("syntactically valid future tenant rejected: %v", err) + } +} + func writeConfig(t *testing.T, body string) string { t.Helper() path := filepath.Join(t.TempDir(), "config.json") diff --git a/sandboxd/engine/cocoond_test.go b/sandboxd/engine/cocoond_test.go index 053dc205..0db76951 100644 --- a/sandboxd/engine/cocoond_test.go +++ b/sandboxd/engine/cocoond_test.go @@ -45,6 +45,8 @@ func TestInspectReadsOneVMAndReportsAMissingOne(t *testing.T) { fakeCocoon(t, "#!/bin/sh\n[ \"$1 $2\" = \"vm inspect\" ] || exit 2\n"+ "if [ \"$3\" = sbx-1 ]; then echo '{\"config\":{\"name\":\"sbx-1\"},\"state\":\"running\","+ "\"attached_devices\":{\"disks\":[{\"id\":\"disk-a\",\"name\":\"imagenet\",\"path\":\"/v/a.img\",\"readonly\":true},{\"id\":\"disk-b\",\"name\":\"scratch\",\"path\":\"/v/b.img\"}]}}'; exit 0; fi\n"+ + "if [ \"$3\" = sbx-stale ]; then echo '{\"state\":\"stopped\",\"stale\":true,\"pid\":0}'; exit 0; fi\n"+ + "if [ \"$3\" = sbx-stopped ]; then echo '{\"state\":\"stopped\"}'; exit 0; fi\n"+ "echo \"Error: inspect: vm $3: vm not found\" >&2; exit 1\n") e := New("cocoon", nil, nil, false, false, "") rec, ok, err := e.Inspect(t.Context(), "sbx-1") @@ -52,11 +54,37 @@ func TestInspectReadsOneVMAndReportsAMissingOne(t *testing.T) { if err != nil || !ok || rec.State != "running" || rec.Config.Name != "sbx-1" || !slices.Equal(rec.AttachedDevices.Disks, wantDisks) { t.Errorf("Inspect sbx-1 = %+v, %v, %v", rec, ok, err) } + for _, name := range []string{"sbx-stale", "sbx-stopped"} { + rec, present, err := e.Inspect(t.Context(), name) + if err != nil || !present || rec.State != "stopped" || rec.Stale != (name == "sbx-stale") { + t.Errorf("Inspect %s lost stopped classification: %+v, %v, %v", name, rec, present, err) + } + } if _, ok, err := e.Inspect(t.Context(), "sbx-gone"); err != nil || ok { t.Errorf("Inspect of a missing VM = %v, %v; want not found without an error", ok, err) } } +func TestInspectPreservesMissingDiskInventory(t *testing.T) { + for _, tc := range []struct { + name, inventory string + unknown bool + }{ + {"omitted devices", "", true}, + {"omitted disks", `,"attached_devices":{}`, true}, + {"null disks", `,"attached_devices":{"disks":null}`, true}, + {"empty disks", `,"attached_devices":{"disks":[]}`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + fakeCocoon(t, "#!/bin/sh\necho '{\"state\":\"running\""+tc.inventory+"}'\n") + rec, present, err := New("cocoon", nil, nil, false, false, "").Inspect(t.Context(), "sbx-1") + if err != nil || !present || (rec.AttachedDevices.Disks == nil) != tc.unknown { + t.Fatalf("inventory classification: %+v, %v, %v", rec, present, err) + } + }) + } +} + func serveCocoond(t *testing.T, socket, stream string) { t.Helper() ln, err := net.Listen("unix", socket) diff --git a/sandboxd/engine/volume_live.go b/sandboxd/engine/volume_live.go index e1033a84..f6af7f3f 100644 --- a/sandboxd/engine/volume_live.go +++ b/sandboxd/engine/volume_live.go @@ -31,7 +31,7 @@ func (e *LiveVolumeUnsupported) Error() string { } type volumeMount struct { - devNum, root, path, options string + devNum, root, path, options, superOptions string } // CheckLiveVolumeSupport rejects runtimes without idempotent live disk attach and detach. @@ -62,39 +62,51 @@ func (e *Engine) MountLiveVolume(ctx context.Context, vsockSocket, name, mount s return e.mountDevice(ctx, vsockSocket, device, name, mount, rw) } -// DetachVolume unmounts a guest volume before confirming host removal. -func (e *Engine) DetachVolume(ctx context.Context, vmName, vsockSocket, name, mount string) error { +// DetachVolume returns cleanRW only after normally unmounting the exact whole-device +// mount with writable mount and superblock options, no residual mounts, and successful host detach. +func (e *Engine) DetachVolume(ctx context.Context, vmName, vsockSocket, name, mount string) (cleanRW bool, err error) { ctx, cancel := context.WithTimeout(ctx, VolumeMutationTimeout) defer cancel() device, found, err := e.findVolumeDevice(ctx, vsockSocket, name) if err != nil { - return err + return false, err } + normal := false if found { - if err = e.unmountLiveVolume(ctx, vsockSocket, device, mount); err != nil { - return err + if normal, err = e.unmountLiveVolume(ctx, vsockSocket, device, mount); err != nil { + return false, err } } - _, err = e.run(ctx, "vm", "disk", "detach", vmName, argName, name) - return err + if _, err = e.run(ctx, "vm", "disk", "detach", vmName, argName, name); err != nil { + return false, err + } + return normal, nil } -func (e *Engine) unmountLiveVolume(ctx context.Context, vsockSocket, device, mount string) error { +func (e *Engine) unmountLiveVolume(ctx context.Context, vsockSocket, device, mount string) (bool, error) { + if mount == "" { + return false, e.refuseOtherMounts(ctx, vsockSocket, device) + } devNum, mounts, err := e.volumeMounts(ctx, vsockSocket, device) if err != nil { - return err + return false, err } target := e.resolveMount(ctx, vsockSocket, mount, mounts) atTarget := slices.DeleteFunc(mounts, func(m volumeMount) bool { return m.path != target }) + normal := false if len(atTarget) == 1 && atTarget[0].devNum == devNum && atTarget[0].root == "/" { if err = e.UnmountVolume(ctx, vsockSocket, mount); err != nil { if _, ok := errors.AsType[*guestExitError](err); ok { - return &VolumeMountConflict{Message: fmt.Sprintf("cannot unmount %q; close guest references and retry", mount)} + return false, &VolumeMountConflict{Message: fmt.Sprintf("cannot unmount %q; close guest references and retry", mount)} } - return err + return false, err } + normal = writableMountOptions(atTarget[0].options) && writableMountOptions(atTarget[0].superOptions) } - return e.refuseOtherMounts(ctx, vsockSocket, device) + if err = e.refuseOtherMounts(ctx, vsockSocket, device); err != nil { + return false, err + } + return normal, nil } func (e *Engine) volumeMounts(ctx context.Context, vsockSocket, device string) (string, []volumeMount, error) { @@ -106,15 +118,8 @@ func (e *Engine) volumeMounts(ctx context.Context, vsockSocket, device string) ( if err != nil { return "", nil, err } - var mounts []volumeMount - for line := range strings.SplitSeq(strings.TrimSpace(string(raw)), "\n") { - fields := strings.Fields(line) - if len(fields) < 10 || !slices.Contains(fields[6:], "-") { - return "", nil, errors.New("invalid guest mountinfo") - } - mounts = append(mounts, volumeMount{fields[2], mountUnescaper.Replace(fields[3]), mountUnescaper.Replace(fields[4]), fields[5]}) - } - return strings.TrimSpace(string(devNum)), mounts, nil + mounts, err := parseVolumeMounts(string(raw)) + return strings.TrimSpace(string(devNum)), mounts, err } func (e *Engine) volumeMounted(ctx context.Context, vsockSocket, device, mount string, rw bool) (bool, error) { @@ -164,6 +169,25 @@ func (e *Engine) refuseOtherMounts(ctx context.Context, vsockSocket, device stri return nil } +func parseVolumeMounts(raw string) ([]volumeMount, error) { + var mounts []volumeMount + for line := range strings.SplitSeq(strings.TrimSpace(raw), "\n") { + mount, filesystem, ok := strings.Cut(line, " - ") + fields := strings.Fields(mount) + fsFields := strings.Split(filesystem, " ") + if !ok || len(fields) < 6 || len(fsFields) != 3 || fsFields[0] == "" || fsFields[2] == "" { + return nil, errors.New("invalid guest mountinfo") + } + mounts = append(mounts, volumeMount{fields[2], mountUnescaper.Replace(fields[3]), mountUnescaper.Replace(fields[4]), fields[5], fsFields[2]}) + } + return mounts, nil +} + +func writableMountOptions(options string) bool { + parts := strings.Split(options, ",") + return slices.Contains(parts, "rw") && !slices.Contains(parts, "ro") && !slices.Contains(parts, "") +} + func bindConflict(path string) *VolumeMountConflict { return &VolumeMountConflict{Message: fmt.Sprintf("volume device has another mount at %q; remove guest mount references before retrying", path)} } diff --git a/sandboxd/engine/volume_live_test.go b/sandboxd/engine/volume_live_test.go index a731f2e1..f7c0c3b4 100644 --- a/sandboxd/engine/volume_live_test.go +++ b/sandboxd/engine/volume_live_test.go @@ -54,7 +54,7 @@ func TestLiveVolumeConvergesMountAndDetach(t *testing.T) { t.Fatal(err) } } - if err := e.DetachVolume(t.Context(), "sbx-1", sock, spec.Name, tt.mount); err != nil { + if _, err := e.DetachVolume(t.Context(), "sbx-1", sock, spec.Name, tt.mount); err != nil { t.Fatal(err) } fake.mu.Lock() @@ -74,6 +74,48 @@ func TestLiveVolumeConvergesMountAndDetach(t *testing.T) { } } +func TestLiveVolumeAcceptsEmptyMountSource(t *testing.T) { + for _, tt := range []struct { + name, mount string + rw bool + }{ + {name: "mounted read-only", mount: "/data"}, + {name: "mounted writable", mount: "/data", rw: true}, + {name: "attach-only"}, + } { + t.Run(tt.name, func(t *testing.T) { + sock := sockPath(t) + fake := serveFakeSilkd(t, sock) + configureVolumeDevices(fake) + spec := liveDiskCLI(t, "") + const unrelated = "501 84 0:62 / /empty-source rw,relatime - tmpfs rw,size=1024k,inode64\n" + fake.mu.Lock() + fake.serial["/sys/block/vdc/dev"] = []byte("8:32\n") + fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo + unrelated) + fake.execHook = func(argv []string) { + switch argv[0] { + case "mount": + mode := mountMode(tt.rw) + fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo + unrelated + "2 1 8:32 / /data " + mode + " - ext4 /dev/vdc " + mode + "\n") + case "umount": + fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo + unrelated) + } + } + fake.mu.Unlock() + e := New("cocoon", nil, nil, false, false, "") + if tt.mount != "" { + if err := e.MountLiveVolume(t.Context(), sock, spec.Name, tt.mount, tt.rw); err != nil { + t.Fatalf("mount with unrelated empty source: %v", err) + } + } + result, err := e.DetachVolume(t.Context(), "sbx-1", sock, spec.Name, tt.mount) + if err != nil || result != tt.rw { + t.Fatalf("detach with unrelated empty source: result=%+v err=%v", result, err) + } + }) + } +} + func TestLiveMountRefusesAnOccupiedPathBehindASymlink(t *testing.T) { sock := sockPath(t) fake := serveFakeSilkd(t, sock) @@ -191,7 +233,7 @@ func TestLiveDetachKeepsBusyAndUnobservableDevices(t *testing.T) { fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo + "2 1 8:32 / /data ro - ext4 /dev/vdc ro\n3 2 8:32 /sub /data ro - ext4 /dev/vdc ro\n") } fake.mu.Unlock() - err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, "/data") + _, err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, "/data") if err == nil { t.Fatal("detach must fail closed") } @@ -222,7 +264,7 @@ func TestLiveDetachMissingGuestStillChecksHost(t *testing.T) { sock := sockPath(t) serveFakeSilkd(t, sock) spec := liveDiskCLI(t, hostErr) - err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, "/data") + _, err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, "/data") wantErr := hostErr != "" if (err != nil) != wantErr { t.Fatalf("got %v, want error=%v", err, wantErr) @@ -240,7 +282,7 @@ func TestLiveDetachIgnoresAnotherDeviceAtThePath(t *testing.T) { fake.serial["/sys/block/vdc/dev"] = []byte("8:32\n") fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo + "2 1 8:48 / /data ro - ext4 /dev/vdd ro\n") fake.mu.Unlock() - if err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, "/data"); err != nil { + if _, err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, "/data"); err != nil { t.Fatalf("detach of an unmounted device blocked by another device's mount: %v", err) } fake.mu.Lock() @@ -250,6 +292,66 @@ func TestLiveDetachIgnoresAnotherDeviceAtThePath(t *testing.T) { } } +func TestLiveDetachCleanEvidenceRequiresWritableUnmountAndHostSuccess(t *testing.T) { + for _, tc := range []struct { + name, options, superOptions, mount, hostErr string + missing, unmounted, busy, residual, wantClean, wantErr bool + }{ + {name: "normal", options: "rw", superOptions: "rw", mount: "/data", wantClean: true}, + {name: "unordered options", options: "relatime,rw", superOptions: "errors=remount-ro,rw", mount: "/data", wantClean: true}, + {name: "readonly", options: "ro", superOptions: "ro", mount: "/data"}, + {name: "mount readonly", options: "ro", superOptions: "rw", mount: "/data"}, + {name: "contradictory mount", options: "rw,ro", superOptions: "rw", mount: "/data"}, + {name: "superblock readonly", options: "rw", superOptions: "ro", mount: "/data"}, + {name: "contradictory superblock", options: "rw", superOptions: "rw,ro", mount: "/data"}, + {name: "empty option", options: "rw,,relatime", superOptions: "rw", mount: "/data"}, + {name: "missing superblock options", options: "rw", mount: "/data", wantErr: true}, + {name: "extra superblock fields", options: "rw", superOptions: "rw extra", mount: "/data", wantErr: true}, + {name: "pre-unmounted", unmounted: true, mount: "/data"}, + {name: "missing device", missing: true, unmounted: true, mount: "/data"}, + {name: "host failure", options: "rw", superOptions: "rw", mount: "/data", hostErr: "host removal failed", wantErr: true}, + {name: "busy", options: "rw", superOptions: "rw", mount: "/data", busy: true, wantErr: true}, + {name: "residual mount", options: "rw", superOptions: "rw", mount: "/data", residual: true, wantErr: true}, + {name: "attach-only mounted", options: "rw", superOptions: "rw", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + sock := sockPath(t) + fake := serveFakeSilkd(t, sock) + if !tc.missing { + configureVolumeDevices(fake) + } + spec := liveDiskCLI(t, tc.hostErr) + fake.mu.Lock() + fake.serial["/sys/block/vdc/dev"] = []byte("8:32\n") + raw := rootMountInfo + if !tc.unmounted { + raw += "2 1 8:32 / /data " + tc.options + " shared:4 - ext4 /dev/vdc " + tc.superOptions + "\n" + } + fake.serial["/proc/self/mountinfo"] = []byte(raw) + if tc.busy { + fake.execCode = 32 + } + fake.execHook = func(argv []string) { + if argv[0] == "umount" && !tc.residual && !tc.busy { + fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo) + } + } + fake.mu.Unlock() + clean, err := New("cocoon", nil, nil, false, false, "").DetachVolume(t.Context(), "sbx-1", sock, spec.Name, tc.mount) + if (err != nil) != tc.wantErr || clean != tc.wantClean { + t.Fatalf("clean=%t err=%v", clean, err) + } + if tc.mount == "" { + fake.mu.Lock() + defer fake.mu.Unlock() + if len(guestChanges(fake.execCalls)) != 0 { + t.Fatal("unmounted a caller-managed filesystem") + } + } + }) + } +} + func liveDiskCLI(t *testing.T, detachErr string) VolumeSpec { t.Helper() dir := t.TempDir() diff --git a/sandboxd/pool/checkpoint_references_test.go b/sandboxd/pool/checkpoint_references_test.go index 3125c68a..c3ec0ae3 100644 --- a/sandboxd/pool/checkpoint_references_test.go +++ b/sandboxd/pool/checkpoint_references_test.go @@ -126,33 +126,8 @@ func TestClaimCheckpointReferencesOutliveUnacknowledgedWrites(t *testing.T) { t.Fatalf("want a failed directory sync after the rename: %v", err) } case captureWrite: - if err := syscall.Mkfifo(s.path+".tmp", 0o600); err != nil { - t.Fatal(err) - } - done := make(chan error, 1) - go func() { done <- s.commit(snap) }() - waitFor(t, func() bool { - s.mu.Lock() - defer s.mu.Unlock() - return len(s.retained["owner"]) != 0 - }) - finish = func() { - r, err := os.Open(s.path + ".tmp") - if err != nil { - t.Fatal(err) - } - raw, err := io.ReadAll(r) - _ = r.Close() - if err = errors.Join(err, <-done); err != nil { - t.Fatal(err) - } - if err = os.Remove(s.path); err != nil { - t.Fatal(err) - } - if err = os.WriteFile(s.path, raw, 0o600); err != nil { - t.Fatal(err) - } - } + release := captureClaimWrite(t, s, snap, "owner") + finish = func() { release() } } if tc.mutate != nil { tc.mutate(s) @@ -208,3 +183,35 @@ func failClaimWrites(t *testing.T, s *claimStore) func() { t.Cleanup(restore) return restore } + +func captureClaimWrite(t *testing.T, s *claimStore, snap claimSnapshot, owner string) func() []byte { + t.Helper() + if err := syscall.Mkfifo(s.path+".tmp", 0o600); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- s.commit(snap) }() + waitFor(t, func() bool { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.retained[owner]) != 0 || len(s.retainedVolumes[owner]) != 0 + }) + return func() []byte { + r, err := os.Open(s.path + ".tmp") + if err != nil { + t.Fatal(err) + } + raw, err := io.ReadAll(r) + _ = r.Close() + if err = errors.Join(err, <-done); err != nil { + t.Fatal(err) + } + if err = os.Remove(s.path); err != nil { + t.Fatal(err) + } + if err = os.WriteFile(s.path, raw, 0o600); err != nil { + t.Fatal(err) + } + return raw + } +} diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index f9427792..18ace6c0 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -371,7 +371,7 @@ func (m *Manager) overQuota(extra int, tenant string) error { // admitVolumes takes one claim's holds; a refusal on either check leaves nothing held. func (m *Manager) admitVolumes(tenant string, volumes []resolvedVolume) ([]types.Volume, error) { applied := appliedVolumes(volumes) - if err := m.admitClaim(tenant, applied); err != nil { + if err := m.admitClaim(tenant, volumes, applied); err != nil { return nil, err } if err := m.confirmVolumesClean(volumes); err != nil { @@ -382,13 +382,17 @@ func (m *Manager) admitVolumes(tenant string, volumes []resolvedVolume) ([]types } // admitClaim refuses a busy volume or an over-quota tenant before any VM is built. -func (m *Manager) admitClaim(tenant string, volumes []types.Volume) error { +func (m *Manager) admitClaim(tenant string, volumes []resolvedVolume, applied []types.Volume) error { m.mu.Lock() defer m.mu.Unlock() if err := m.quotaErr(1, tenant); err != nil { return err } - return m.reserveVolumes(volumes) + if err := m.checkResolved(volumes, ""); err != nil { + return err + } + m.adoptVolumes(applied) + return nil } // quotaErr answers ErrQuota when the node takes no more claims; callers hold m.mu. diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index 2f07ecaf..5d6967d5 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -81,16 +81,20 @@ type claimStore struct { writeMu sync.Mutex buf []byte - mu sync.Mutex - rows map[string]*claimRow - seq uint64 - written uint64 - refs map[string][4]string - retained map[string][]string - changed map[string]struct{} - checkpoints map[string]int - - recommitting atomic.Bool + mu sync.Mutex + rows map[string]*claimRow + seq uint64 + written uint64 + refs map[string][4]string + retained map[string][]string + changed map[string]struct{} + checkpoints map[string]int + currentVolumes map[string][]types.Volume + retainedVolumes map[string][]types.Volume + retainedVolumeCounts map[string]volumeHolders + + recommitting atomic.Bool + anyRetainedVolume atomic.Bool } func newClaimStore(dataDir string, sync bool) *claimStore { @@ -98,6 +102,7 @@ func newClaimStore(dataDir string, sync bool) *claimStore { path: filepath.Join(dataDir, "claims.json"), sync: sync, rows: map[string]*claimRow{}, refs: map[string][4]string{}, retained: map[string][]string{}, changed: map[string]struct{}{}, checkpoints: map[string]int{}, + currentVolumes: map[string][]types.Volume{}, retainedVolumes: map[string][]types.Volume{}, retainedVolumeCounts: map[string]volumeHolders{}, } } @@ -116,9 +121,18 @@ func (s *claimStore) load() (map[string]*types.Sandbox, error) { s.mu.Lock() defer s.mu.Unlock() for _, sb := range claims { + for _, volumes := range [][]types.Volume{sb.Volumes, sb.VolumesWant} { + for i := range volumes { + volumes[i].AttachOnly = volumes[i].AttachOnly || volumes[i].Mount == "" + } + } if sb.VolumesWant == nil { sb.VolumesWant = sb.Volumes } + if refs := appendRecoveryRefs(slices.Clone(sb.Volumes), sb.VolumesWant); len(refs) != 0 { + s.changeRetainedVolumes(sb.ID, refs) + s.changed[sb.ID] = struct{}{} + } if refs := checkpointRefs(sb); refs != [4]string{} { s.changeRetainedRefs(sb.ID, appendRefs(nil, refs)) s.changed[sb.ID] = struct{}{} @@ -151,24 +165,34 @@ func (s *claimStore) del(ids ...string) claimSnapshot { func (s *claimStore) put(id string, sb *types.Sandbox) { var refs [4]string + var volumes []types.Volume if sb == nil { delete(s.rows, id) } else { + dto := dtoOf(sb) refs = checkpointRefs(sb) - s.rows[id] = &claimRow{dto: dtoOf(sb)} + volumes = volumeRefs(s.currentVolumes[id], dto.Volumes, dto.VolumesWant) + s.rows[id] = &claimRow{dto: dto} } before := s.refs[id] - if before == refs { - return + if before != refs { + s.changeCheckpointCounts(before[:], -1) + s.changeCheckpointCounts(refs[:], 1) + if refs == [4]string{} { + delete(s.refs, id) + } else { + s.refs[id] = refs + } + s.changed[id] = struct{}{} } - s.changeCheckpointCounts(before[:], -1) - s.changeCheckpointCounts(refs[:], 1) - if refs == [4]string{} { - delete(s.refs, id) - } else { - s.refs[id] = refs + if !slices.Equal(s.currentVolumes[id], volumes) { + if len(volumes) == 0 { + delete(s.currentVolumes, id) + } else { + s.currentVolumes[id] = volumes + } + s.changed[id] = struct{}{} } - s.changed[id] = struct{}{} } // mark sequences a persist of the projection as it already stands. @@ -211,9 +235,17 @@ func (s *claimStore) commit(snap claimSnapshot) error { } rows, seq := slices.AppendSeq(make([]*claimRow, 0, len(s.rows)), maps.Values(s.rows)), s.seq captured := make(map[string][4]string, len(s.changed)) + var capturedVolumes map[string][]types.Volume for id := range s.changed { captured[id] = s.refs[id] s.changeRetainedRefs(id, appendRefs(s.retained[id], s.refs[id])) + if volumes := s.currentVolumes[id]; len(volumes) != 0 { + if capturedVolumes == nil { + capturedVolumes = make(map[string][]types.Volume) + } + capturedVolumes[id] = volumes + } + s.changeRetainedVolumes(id, appendRecoveryRefs(s.retainedVolumes[id], s.currentVolumes[id])) } s.mu.Unlock() s.buf = append(s.buf[:0], '{') @@ -237,7 +269,8 @@ func (s *claimStore) commit(snap claimSnapshot) error { s.mu.Lock() for id, refs := range captured { s.changeRetainedRefs(id, appendRefs(nil, refs)) - if s.refs[id] == refs { + s.changeRetainedVolumes(id, capturedVolumes[id]) + if s.refs[id] == refs && slices.Equal(s.currentVolumes[id], capturedVolumes[id]) { delete(s.changed, id) } } @@ -291,11 +324,6 @@ func (s *claimStore) replace(raw []byte) error { return nil } -// save is the combined form for the startup Reconcile pass, before contention exists. -func (s *claimStore) save(claims map[string]*types.Sandbox) error { - return s.commit(s.reset(claims)) -} - func (s *claimStore) synced() bool { s.mu.Lock() defer s.mu.Unlock() @@ -340,6 +368,94 @@ func (s *claimStore) checkpointReferences() map[string]struct{} { return refs } +func (s *claimStore) changeRetainedVolumeCounts(refs []types.Volume, delta int) { + for _, v := range refs { + count := s.retainedVolumeCounts[v.Name] + if v.RW() { + count.writers += delta + } else { + count.readers += delta + } + if count == (volumeHolders{}) { + delete(s.retainedVolumeCounts, v.Name) + } else { + s.retainedVolumeCounts[v.Name] = count + } + } +} + +func (s *claimStore) changeRetainedVolumes(id string, after []types.Volume) { + s.changeRetainedVolumeCounts(s.retainedVolumes[id], -1) + s.changeRetainedVolumeCounts(after, 1) + if len(after) == 0 { + delete(s.retainedVolumes, id) + } else { + s.retainedVolumes[id] = after + } + s.anyRetainedVolume.Store(len(s.retainedVolumeCounts) != 0) +} + +func (s *claimStore) hasCheckpointReferences(owner string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.refs[owner] != [4]string{} || len(s.retained[owner]) != 0 +} + +func (s *claimStore) referencesVolume(name string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.retainedVolumeCounts[name] != (volumeHolders{}) +} + +func (s *claimStore) volumeConflicts(volume types.Volume, ownerID string) bool { + s.mu.Lock() + defer s.mu.Unlock() + count := s.retainedVolumeCounts[volume.Name] + for _, ref := range s.retainedVolumes[ownerID] { + if ref == volume { + if ref.RW() { + count.writers-- + } else { + count.readers-- + } + } + } + return count.writers > 0 || volume.RW() && count.readers > 0 +} + +func (s *claimStore) recoveryVolumesFor(owner string) []types.Volume { + s.mu.Lock() + defer s.mu.Unlock() + return slices.Clone(s.retainedVolumes[owner]) +} + +func (s *claimStore) detachingOwners() map[string]bool { + if !s.anyRetainedVolume.Load() { + return nil + } + s.mu.Lock() + defer s.mu.Unlock() + var owners map[string]bool + for id := range s.changed { + if len(s.currentVolumes[id]) == 0 && len(s.retainedVolumes[id]) != 0 { + if owners == nil { + owners = map[string]bool{} + } + owners[id] = true + } + } + return owners +} + +func (s *claimStore) hasRecoveryVolumes(owner string) bool { + if !s.anyRetainedVolume.Load() { + return false + } + s.mu.Lock() + defer s.mu.Unlock() + return len(s.retainedVolumes[owner]) != 0 +} + func encodeRow(dto *claimDTO) ([]byte, error) { raw, err := json.Marshal(map[string]*claimDTO{dto.ID: dto}) if err != nil { @@ -381,3 +497,31 @@ func appendRefs(list []string, refs [4]string) []string { } return list } + +func appendRecoveryRefs(dst, src []types.Volume) []types.Volume { + for _, v := range src { + if !slices.Contains(dst, v) { + dst = append(dst, v) + } + } + return dst +} + +func volumeRefs(previous, applied, wanted []types.Volume) []types.Volume { + if len(applied) == 0 { + applied, wanted = wanted, nil + } + for _, refs := range [2][]types.Volume{applied, wanted} { + for _, v := range refs { + if !slices.Contains(previous, v) { + return appendRecoveryRefs(applied, wanted) + } + } + } + for _, v := range previous { + if !slices.Contains(applied, v) && !slices.Contains(wanted, v) { + return appendRecoveryRefs(applied, wanted) + } + } + return previous +} diff --git a/sandboxd/pool/claims_test.go b/sandboxd/pool/claims_test.go index f7e19311..8ddae7aa 100644 --- a/sandboxd/pool/claims_test.go +++ b/sandboxd/pool/claims_test.go @@ -88,10 +88,14 @@ func TestStoreRoundTrip(t *testing.T) { if err := s.save(claims); err != nil { t.Fatalf("save (sync=%v): %v", fsync, err) } - got, err := newClaimStore(filepath.Dir(s.path), false).load() + loadedStore := newClaimStore(filepath.Dir(s.path), false) + got, err := loadedStore.load() if err != nil { t.Fatalf("load (sync=%v): %v", fsync, err) } + if !loadedStore.hasRecoveryVolumes("sb_a") || loadedStore.hasRecoveryVolumes("sb_b") { + t.Fatal("loaded recovery volumes lost owner visibility") + } if !reflect.DeepEqual(got, claims) { t.Errorf("sync=%v: got %+v, want %+v", fsync, got, claims) } @@ -103,9 +107,10 @@ func TestStoreRoundTrip(t *testing.T) { func TestClaimStoreSnapshotDetachesVolumes(t *testing.T) { s := newClaimStore(t.TempDir(), false) - sb := &types.Sandbox{ID: "sb_a", Volumes: []types.Volume{{Name: "dataset-a", Mount: "/datasets/a"}}} + sb := &types.Sandbox{ID: "sb_a", Volumes: []types.Volume{{Name: "dataset-a", Mount: "/datasets/a"}}, VolumesWant: []types.Volume{{Name: "dataset-b", Mount: "/datasets/b"}}} snap := s.set(sb) sb.Volumes[0].Mount = "/mutated" + sb.VolumesWant[0].Mount = "/mutated-wanted" if err := s.commit(snap); err != nil { t.Fatalf("commit: %v", err) } @@ -120,6 +125,9 @@ func TestClaimStoreSnapshotDetachesVolumes(t *testing.T) { if !slices.Equal(loaded.Volumes, []types.Volume{{Name: "dataset-a", Mount: "/datasets/a"}}) { t.Errorf("volumes %v, want detached snapshot", loaded.Volumes) } + if !slices.Equal(loaded.VolumesWant, []types.Volume{{Name: "dataset-b", Mount: "/datasets/b"}}) { + t.Errorf("wanted volumes %v, want detached snapshot", loaded.VolumesWant) + } } func TestClaimRowOmitsUnsetFlags(t *testing.T) { @@ -231,3 +239,7 @@ func TestClaimStoreConcurrentCommits(t *testing.T) { t.Fatalf("after concurrent commits: %+v, %v", got, err) } } + +func (s *claimStore) save(claims map[string]*types.Sandbox) error { + return s.commit(s.reset(claims)) +} diff --git a/sandboxd/pool/failover.go b/sandboxd/pool/failover.go index b02b20dd..7246e222 100644 --- a/sandboxd/pool/failover.go +++ b/sandboxd/pool/failover.go @@ -130,6 +130,10 @@ func (m *Manager) putRow(ctx context.Context, sb *types.Sandbox, ck string, repl return 0, nil } m.mu.Lock() + if replica && m.hasVolumeHolds(sb) { + m.mu.Unlock() + return 0, ErrVolumeCapture + } dto, deadline, held := dtoOf(sb), sb.Deadline, sb.ArchiveEpoch switch { case replica: @@ -299,7 +303,9 @@ func (m *Manager) reputArchive(ctx context.Context, sb *types.Sandbox, held int6 epoch, err = m.putReplica(ctx, sb, sb.ReplicaCk) } if err != nil { - log.WithFunc("pool.reputArchive").Warnf(ctx, "re-put archive row %s: %v", sb.ID, err) + if !errors.Is(err, ErrVolumeCapture) { + log.WithFunc("pool.reputArchive").Warnf(ctx, "re-put archive row %s: %v", sb.ID, err) + } return } _, _ = m.commitIfLive(ctx, sb, func() { sb.ArchiveEpoch = epoch }) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 9e0124cb..c156a329 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -72,6 +72,7 @@ const ( hibernatePrefix = "sbx-hib-" forkPrefix = "sbx-fork-" vmStateRunning = "running" + vmStateStopped = "stopped" vmStateCreating = "creating" goldenStampSuffix = ".stamp" @@ -102,6 +103,11 @@ var ( ErrNoEgressFork = errors.New("egress-lane sandboxes cannot fork, checkpoint, or promote: a resumed guest egresses before its fresh tap can be locked") ErrVolumeCapture = errors.New("sandboxes with volumes cannot hibernate, fork, checkpoint, or promote") ErrVolumeBusy = errors.New("volume is held by another claim") + + ErrReplicaPresent = errors.New("live writable attachment requires replica and recovery checkpoint retirement") + ErrReplicatingPool = errors.New("live writable volumes require a non-replicating pool") + ErrReplicaStranded = errors.New("live writable attachment is blocked by retained replica or archive state with archive_failover disabled") + // Replaying a journal takes a writable mount, so readers stay out. ErrVolumeNeedsRecovery = errors.New("volume needs recovery by a writable claim") ErrArchived = errors.New("sandbox is archived; an exec or file call wakes it first") @@ -116,6 +122,9 @@ var ( errNotSleeping = errors.New("sandbox is no longer hibernated or archived") errWokeMeanwhile = errors.New("woke between sweep and hibernate") errNoEgressTap = errors.New("egress-lane claim has no lockable tap") + + ErrVolumeRegistrationConflict = errors.New("volume registration conflicts with the catalog") + ErrVolumeCatalogUnavailable = errors.New("volume catalog requires a successful persistence retry") ) // Engine is the slice of the cocoon driver the manager consumes. @@ -145,7 +154,7 @@ type Engine interface { TrimCow(ctx context.Context, vsockSocket string) error CheckLiveVolumeSupport(ctx context.Context) error DiskAttach(ctx context.Context, vmName string, spec engine.VolumeSpec) error - DetachVolume(ctx context.Context, vmName, sock, name, mount string) error + DetachVolume(ctx context.Context, vmName, sock, name, mount string) (cleanRW bool, err error) MountVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error MountLiveVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error UnmountVolume(ctx context.Context, vsockSocket, mount string) error @@ -321,16 +330,14 @@ type PeerDeleteFunc func(ctx context.Context, id string) // Manager owns the node's pools, claims, and their persistence. type Manager struct { - eng Engine - dataDir string - egress bool - maxFork int - store *claimStore - volumes map[string]catalogVolume - // volumeAdmission counts the live holders of each volume name; guarded by m.mu. - volumeAdmission map[string]volumeHolders - volumeRuns map[*types.Sandbox]*volumeRun - volumeErrors map[*types.Sandbox]map[string]string + eng Engine + dataDir string + egress bool + maxFork int + store *claimStore + catalog volumeCatalog + volumeRuns map[*types.Sandbox]*volumeRun + volumeErrors map[*types.Sandbox]map[string]string poolStore *poolStore // poolShared is the cell's pool set with a meta store; poolVersion, under m.mu, is its newest version applied. @@ -448,8 +455,7 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg cocoondSocket: cfg.CocoondSocket, maxFork: cmp.Or(cfg.MaxForkCount, defaultMaxFork), store: newClaimStore(cfg.DataDir, cfg.SyncClaims), - volumes: make(map[string]catalogVolume, len(cfg.Volumes)), - volumeAdmission: map[string]volumeHolders{}, + catalog: volumeCatalog{entries: make(map[string]*catalogVolume, len(cfg.Volumes)), roots: cfg.VolumeRoots, holders: map[string]volumeHolders{}}, volumeRuns: map[*types.Sandbox]*volumeRun{}, volumeErrors: map[*types.Sandbox]map[string]string{}, poolStore: newPoolStore(cfg.DataDir), @@ -478,12 +484,15 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg healSem: make(chan struct{}, maxConcurrentHeals), } for _, volume := range cfg.Volumes { - m.volumes[volume.Name] = catalogVolume{ + m.catalog.entries[volume.Name] = &catalogVolume{ disk: engine.VolumeSpec{Name: volume.Name, Path: volume.Path, DirectIO: volume.DirectIO}, tenants: slices.Clone(volume.Tenants), writable: volume.Writable, } } + if err := m.loadRuntimeVolumes(ctx); err != nil { + return nil, err + } if err := os.MkdirAll(m.goldensDir(), 0o750); err != nil { return nil, fmt.Errorf("create goldens dir: %w", err) } @@ -600,6 +609,7 @@ func (m *Manager) Run(ctx context.Context) { case <-m.refillKick: m.refillOnce(ctx) case <-reap.C: + go m.retryVolumeCatalog(ctx) m.retryRemovals(ctx) m.reapOnce(ctx) m.idleOnce(ctx) diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 56e4148f..fa51d1e5 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1253,8 +1253,12 @@ type fakeEngine struct { mountVolumeErr error unmountVolumeErr error detachVolumeErr error + detachNormalUnmount *bool + liveVolumeSupportErr error liveVolumeHook func(string) error stopped map[string]bool + stale map[string]bool + inspectHook func() error disks map[string][]types.VMDisk creating map[string]bool staleOutcome engine.StaleCreateOutcome @@ -1295,7 +1299,7 @@ type fakeEngine struct { func newFakeEngine() *fakeEngine { return &fakeEngine{ - vms: map[string]string{}, stopped: map[string]bool{}, creating: map[string]bool{}, pids: map[string]int{}, + vms: map[string]string{}, stopped: map[string]bool{}, stale: map[string]bool{}, creating: map[string]bool{}, pids: map[string]int{}, attachDirty: map[string]bool{}, removeSeenOps: map[string][]string{}, removeSeenDirty: map[string][]string{}, metadataDocs: map[string]string{}, guestEnvs: map[string]string{}, sockRoot: "/vsock", } @@ -1526,7 +1530,13 @@ func (f *fakeEngine) List(ctx context.Context) ([]types.VMRecord, error) { func (f *fakeEngine) Inspect(ctx context.Context, name string) (types.VMRecord, bool, error) { f.mu.Lock() f.inspects++ + hook := f.inspectHook f.mu.Unlock() + if hook != nil { + if err := hook(); err != nil { + return types.VMRecord{}, false, err + } + } vms, _ := f.list(ctx) i := slices.IndexFunc(vms, func(vm types.VMRecord) bool { return vm.Config.Name == name }) if i < 0 { @@ -1616,7 +1626,7 @@ func (f *fakeEngine) TrimCow(_ context.Context, sock string) error { return f.trimErr } -func (f *fakeEngine) CheckLiveVolumeSupport(context.Context) error { return nil } +func (f *fakeEngine) CheckLiveVolumeSupport(context.Context) error { return f.liveVolumeSupportErr } func (f *fakeEngine) DiskAttach(ctx context.Context, _ string, spec engine.VolumeSpec) error { if f.liveVolumeHook != nil { @@ -1662,24 +1672,31 @@ func (f *fakeEngine) MountLiveVolume(ctx context.Context, sock, name, mount stri return f.MountVolume(ctx, sock, name, mount, rw) } -func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount string) error { +func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount string) (cleanRW bool, err error) { if f.liveVolumeHook != nil { if err := f.liveVolumeHook("detach"); err != nil { - return err + return false, err } } if err := ctx.Err(); err != nil { - return err + return false, err } if mount != "" { if err := f.UnmountVolume(ctx, sock, mount); err != nil { - return err + return false, err } } f.mu.Lock() defer f.mu.Unlock() f.volumeOps = append(f.volumeOps, "detach:"+name) - return f.detachVolumeErr + if f.detachVolumeErr != nil { + return false, f.detachVolumeErr + } + normal := slices.ContainsFunc(f.volumeMounts, func(v types.Volume) bool { return v.Name == name && v.Mount == mount && v.RW() }) + if f.detachNormalUnmount != nil { + normal = *f.detachNormalUnmount + } + return normal, nil } func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { @@ -1762,9 +1779,9 @@ func (f *fakeEngine) list(_ context.Context) ([]types.VMRecord, error) { case f.creating[name]: state = vmStateCreating case f.stopped[name]: - state = "stopped" + state = vmStateStopped } - rec := types.VMRecord{State: state, PID: f.pids[name], VsockSocket: sock, Config: types.VMConfig{Name: name}} + rec := types.VMRecord{Stale: f.stale[name], State: state, PID: f.pids[name], VsockSocket: sock, Config: types.VMConfig{Name: name}} if f.tap != "" { rec.NetworkConfigs = []types.VMNetConfig{{TAP: f.tap}} } diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index a116b580..3ccbfa06 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -70,13 +70,14 @@ func (m *Manager) Reconcile(ctx context.Context) error { owned[sb.VMName] = true referenced[sb.HibernateSnap] = true } - saveErr := m.store.save(m.claimed) + js := m.store.reset(m.claimed) pooled := make(map[string]bool, len(m.pools)) for _, p := range m.pools { m.adoptGolden(p, imageIDs[p.key.Template]) pooled[p.hash] = true } m.mu.Unlock() + saveErr := m.store.commit(js) for _, sb := range adopted { m.recordHibernate(ctx, sb) } @@ -156,6 +157,10 @@ func (m *Manager) sweepStaleVMs(ctx context.Context, live map[string]types.VMRec // removeStaleVM reclaims one unowned VM, reporting whether it is gone. func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRecord) bool { logger := log.WithFunc("pool.removeStaleVM") + m.mu.Lock() + m.catalog.unknown++ + m.mu.Unlock() + td := volumeTeardown{unknown: true} if rec.State == vmStateCreating { // a canceled ctx must not skip the busy check guarding the forced remove switch outcome, err := m.eng.ReconcileStaleCreate(context.WithoutCancel(ctx), name); { @@ -163,41 +168,53 @@ func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRe // verb missing (cocoon < v0.5.8) or failed: keep the old sweep. logger.Warnf(ctx, "reconcile stale create %s: %v; removing", name, err) case outcome == engine.StaleCreateCollected, outcome == engine.StaleCreateNotFound: + m.finishVolumeTeardown(ctx, td) return true case outcome == engine.StaleCreateBusy: logger.Infof(ctx, "stale create %s has an in-flight owner; queued for retry", name) - m.queueStaleCreate(name, rec.TapDevice()) + m.queueStaleCreate(name, rec.TapDevice(), m.resolveStaleVolumes(ctx, name, td)) return false } // not-creating: the record moved on under the lock; remove normally. } if m.removeVM(ctx, name) { + m.finishVolumeTeardown(ctx, td) return true } - m.queueRemoval(name, "", rec.TapDevice(), volumeTeardown{holds: m.holdStaleVolumes(ctx, name, rec)}, time.Time{}) + td = m.resolveStaleVolumes(ctx, name, td) + m.queueRemoval(name, "", rec.TapDevice(), td, time.Time{}) return false } -func (m *Manager) holdStaleVolumes(ctx context.Context, name string, rec types.VMRecord) []types.Volume { - if rec.State != vmStateRunning { - return nil +func (m *Manager) resolveStaleVolumes(ctx context.Context, name string, td volumeTeardown) volumeTeardown { + if !td.unknown { + return td } - inspected, _, err := m.eng.Inspect(ctx, name) + inspected, present, err := m.eng.Inspect(ctx, name) if err != nil { - log.WithFunc("pool.holdStaleVolumes").Warnf(ctx, "inspect stale vm %s, its volumes stay unheld: %v", name, err) - return nil + log.WithFunc("pool.resolveStaleVolumes").Warnf(ctx, "inspect stale vm %s, keeping volume fence: %v", name, err) + return td } - holds := make([]types.Volume, len(inspected.AttachedDevices.Disks)) - for i, disk := range inspected.AttachedDevices.Disks { - holds[i].Name = disk.Name - if !disk.ReadOnly { - holds[i].Mode = types.VolumeModeRW + var holds []types.Volume + switch { + case present && inspected.State == vmStateRunning && inspected.AttachedDevices.Disks != nil: + holds = make([]types.Volume, len(inspected.AttachedDevices.Disks)) + for i, disk := range inspected.AttachedDevices.Disks { + holds[i].Name = disk.Name + if !disk.ReadOnly { + holds[i].Mode = types.VolumeModeRW + } } + // a stale stop is inferred from a lost PID file; the VMM may still be writing its disks + case present && (inspected.State != vmStateStopped || inspected.Stale): + return td } m.mu.Lock() m.adoptVolumes(holds) + m.catalog.unknown-- m.mu.Unlock() - return holds + td.unknown, td.holds = false, holds + return td } // resyncEgress re-locks adopted egress claims, quarantining any it cannot lock. diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go index 75efec26..9d104dac 100644 --- a/sandboxd/pool/remove.go +++ b/sandboxd/pool/remove.go @@ -58,9 +58,9 @@ func (m *Manager) queueRemoval(name, sandboxID, tap string, td volumeTeardown, n m.mu.Unlock() } -func (m *Manager) queueStaleCreate(name, tap string) { +func (m *Manager) queueStaleCreate(name, tap string, td volumeTeardown) { m.mu.Lock() - m.pendingRemovals[name] = pendingRemoval{tap: tap, staleCreate: true} + m.pendingRemovals[name] = pendingRemoval{tap: tap, staleCreate: true, volumes: td} m.mu.Unlock() } @@ -77,7 +77,7 @@ func (m *Manager) retryRemovals(ctx context.Context) *sync.WaitGroup { } m.mu.Unlock() names := slices.Collect(maps.Keys(batch)) - return m.runBounded(ctx, len(names), func(ctx context.Context, i int) { + return m.runBounded(context.WithoutCancel(ctx), len(names), func(ctx context.Context, i int) { m.retryRemoval(ctx, names[i], batch[names[i]]) }) } @@ -87,10 +87,10 @@ func (m *Manager) retryRemoval(ctx context.Context, name string, pending pending switch outcome, err := m.eng.ReconcileStaleCreate(ctx, name); { case err != nil: log.WithFunc("pool.retryRemoval").Warnf(ctx, "reconcile stale create %s: %v; retrying", name, err) - m.queueStaleCreate(name, pending.tap) + m.queueStaleCreate(name, pending.tap, pending.volumes) return case outcome == engine.StaleCreateBusy: - m.queueStaleCreate(name, pending.tap) + m.queueStaleCreate(name, pending.tap, m.resolveStaleVolumes(ctx, name, pending.volumes)) return case outcome == engine.StaleCreateCollected, outcome == engine.StaleCreateNotFound: m.finishRemoval(ctx, pending) @@ -98,6 +98,7 @@ func (m *Manager) retryRemoval(ctx context.Context, name string, pending pending } } if !m.removeVM(ctx, name) { + pending.volumes = m.resolveStaleVolumes(ctx, name, pending.volumes) m.queueRemoval(name, pending.sandboxID, pending.tap, pending.volumes, time.Time{}) return } diff --git a/sandboxd/pool/replica.go b/sandboxd/pool/replica.go index 6524154d..8d6ff141 100644 --- a/sandboxd/pool/replica.go +++ b/sandboxd/pool/replica.go @@ -27,6 +27,7 @@ func (m *Manager) replicateOnce(ctx context.Context) { var withVolumes []string next := make(map[string]replicaState, len(m.replicas)) m.mu.Lock() + detaching := m.store.detachingOwners() for id, sb := range m.claimed { every, st := v.poolReplicate[sb.PolicyKey()], m.replicas[id] switch { @@ -35,7 +36,7 @@ func (m *Manager) replicateOnce(ctx context.Context) { retire = append(retire, sb) } case !m.replicable(sb): - case m.hasVolumeHolds(sb): + case m.hasLiveVolumeHolds(sb) || detaching[id]: if sb.HasReplica() { retire = append(retire, sb) } @@ -112,16 +113,21 @@ func (m *Manager) replicate(ctx context.Context, sb *types.Sandbox) error { sb.Transition.Lock() defer sb.Transition.Unlock() m.mu.Lock() - ok := m.claimed[sb.ID] == sb && sb.ArchiveCk == "" && sb.ArchivePending == "" var intent claimSnapshot - if ok { + var refusal error + switch { + case m.claimed[sb.ID] != sb || sb.ArchiveCk != "" || sb.ArchivePending != "": + refusal = errWokeMeanwhile + case m.hasVolumeHolds(sb): + refusal = ErrVolumeCapture + default: sb.ReplicaPending = ckID intent = m.store.set(sb) } m.mu.Unlock() - if !ok { + if refusal != nil { m.deleteOrphanArchiveCk(ctx, ckID) - return errWokeMeanwhile + return refusal } if err := m.store.commit(intent); err != nil { _ = m.clearReplicaIntent(ctx, sb, ckID) diff --git a/sandboxd/pool/replica_test.go b/sandboxd/pool/replica_test.go index 0a157b61..f7bd658f 100644 --- a/sandboxd/pool/replica_test.go +++ b/sandboxd/pool/replica_test.go @@ -1002,6 +1002,47 @@ func TestReplicaAdoptionWakesTheClaimFromTheLastReplica(t *testing.T) { } } +func TestReplicaScanWaitsForDetachAcknowledgment(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + if err := m.RegisterVolume(t.Context(), "data", types.VolumeRegistration{Path: writeVolumeImage(t, "replicated-data", "bytes")}); err != nil { + t.Fatal(err) + } + sb, attached, ordinary := mustClaim(t, m, testKey), mustClaim(t, m, testKey), mustClaim(t, m, testKey) + for _, owner := range []*types.Sandbox{sb, attached} { + if _, err := m.SetVolumes(t.Context(), owner.ID, "", []types.Volume{{Name: "data"}}); err != nil { + t.Fatal(err) + } + } + eng := m.eng.(*fakeEngine) + unblock := detachWithoutCommit(t, m, eng, sb.ID, "") + m.replicateOnce(t.Context()) + if m.replicating.Load() || !m.replicas[sb.ID].skipLogged || !m.replicas[attached.ID].skipLogged || m.replicas[ordinary.ID].tried.IsZero() || m.replicas[ordinary.ID].skipLogged { + t.Fatalf("scan failed to distinguish detached, attached, and ordinary claims: %+v", m.replicas) + } + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrVolumeCapture) { + t.Fatalf("capture ignored retained attachment: %v", err) + } + if sb.HasReplica() || len(eng.snapSaves) != 0 { + t.Fatal("pending detach published a replica") + } + unblock() + if err := m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + m.replicateOnce(t.Context()) + if st := m.replicas[sb.ID]; st.skipLogged || st.tried.IsZero() { + t.Fatalf("acknowledged detach stayed skipped: %+v", st) + } + m.replicas[sb.ID] = replicaState{tried: time.Now().Add(-time.Hour)} + m.replicateOnce(t.Context()) + waitFor(t, func() bool { return !m.replicating.Load() }) + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil || !ok || rec.Consumed || rec.Checkpoint == "" || !sb.HasReplica() { + t.Fatalf("acknowledged detach did not permit replication: %+v, %t, %v", rec, ok, err) + } +} + func newReplicaCluster(t *testing.T) *failoverCluster { t.Helper() c := newFailoverCluster(t) diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index 5a1e8eed..1eea410a 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -31,31 +31,36 @@ type volumeHolders struct { } type resolvedVolume struct { + entry *catalogVolume disk engine.VolumeSpec applied types.Volume } // volumeTeardown captures marker paths at quiesce time; a later config can re-point the entry. type volumeTeardown struct { - holds []types.Volume - clears []string + run *volumeRun + unknown bool + holds []types.Volume + clears []string } type catalogVolume struct { + runtime bool disk engine.VolumeSpec tenants []string writable bool } -func (v catalogVolume) allowed(tenant string) bool { +func (v *catalogVolume) allowedFor(tenant string) bool { return tenant == "" || len(v.tenants) == 0 || slices.Contains(v.tenants, tenant) } // Volumes reports the caller-visible fleet catalog; an empty tenant means root. func (m *Manager) Volumes(tenant string, holders map[string]int) []types.VolumeInfo { - infos := make([]types.VolumeInfo, 0, len(m.volumes)) - for name, volume := range m.volumes { - if !volume.allowed(tenant) { + catalog := m.catalogSnapshot() + infos := make([]types.VolumeInfo, 0, len(catalog)) + for name, volume := range catalog { + if !volume.allowedFor(tenant) { continue } info := types.VolumeInfo{ @@ -74,7 +79,7 @@ func (m *Manager) Volumes(tenant string, holders map[string]int) []types.VolumeI // root can see peer-only names; a tenant needs local catalog metadata to apply the access list if tenant == "" { for name, nodes := range holders { - if _, ok := m.volumes[name]; !ok { + if _, ok := catalog[name]; !ok { infos = append(infos, types.VolumeInfo{ Name: name, DefaultMount: types.DefaultVolumeMount(name), @@ -89,8 +94,9 @@ func (m *Manager) Volumes(tenant string, holders map[string]int) []types.VolumeI // VolumeNames returns the sorted names this node can currently serve. func (m *Manager) VolumeNames() []string { - names := make([]string, 0, len(m.volumes)) - for name, volume := range m.volumes { + catalog := m.catalogSnapshot() + names := make([]string, 0, len(catalog)) + for name, volume := range catalog { if _, err := os.Stat(volume.disk.Path); err == nil { names = append(names, name) } @@ -108,8 +114,9 @@ func (m *Manager) VolumePlacement(key types.PoolKey, tenant string, names []stri return true, nil } local := true + catalog := m.catalogSnapshot() for _, name := range names { - volume, ok := m.volumes[name] + volume, ok := catalog[name] if !ok { if tenant == "" { local = false @@ -117,7 +124,7 @@ func (m *Manager) VolumePlacement(key types.PoolKey, tenant string, names []stri } return false, ErrVolumeUnavailable } - if !volume.allowed(tenant) { + if !volume.allowedFor(tenant) { return false, ErrVolumeUnavailable } if _, err := os.Stat(volume.disk.Path); err != nil { @@ -131,18 +138,27 @@ func (m *Manager) resolveVolumes(ctx context.Context, tenant string, requested [ if len(requested) == 0 { return nil, nil } - // an attach-only claim has no mounts to default; re-defaulting here would mount them - applied, err := types.ValidateVolumes(requested, types.VolumesAttachOnly(requested)) + applied, err := types.ValidateVolumes(requested) if err != nil { return nil, fmt.Errorf("%w: %w", ErrBadVolume, err) } - resolved := make([]resolvedVolume, 0, len(applied)) + m.mu.Lock() + catalog := m.catalog.entries for _, volume := range applied { - entry, ok := m.volumes[volume.Name] - if !ok || !entry.allowed(tenant) { + entry := catalog[volume.Name] + if entry == nil || !entry.allowedFor(tenant) { + m.mu.Unlock() return nil, ErrVolumeUnavailable } - resolvedEntry, resolveErr := m.resolveCatalogVolume(ctx, volume) + if err := m.catalog.admissionError(volume.Name); err != nil { + m.mu.Unlock() + return nil, err + } + } + m.mu.Unlock() + resolved := make([]resolvedVolume, 0, len(applied)) + for _, volume := range applied { + resolvedEntry, resolveErr := m.resolveCatalogEntry(ctx, volume, catalog[volume.Name]) if resolveErr != nil { return nil, resolveErr } @@ -151,9 +167,8 @@ func (m *Manager) resolveVolumes(ctx context.Context, tenant string, requested [ return resolved, nil } -func (m *Manager) resolveCatalogVolume(ctx context.Context, volume types.Volume) (resolvedVolume, error) { - entry, ok := m.volumes[volume.Name] - if !ok { +func (m *Manager) resolveCatalogEntry(ctx context.Context, volume types.Volume, entry *catalogVolume) (resolvedVolume, error) { + if entry == nil { return resolvedVolume{}, ErrVolumeUnavailable } if volume.RW() && !entry.writable { @@ -161,13 +176,12 @@ func (m *Manager) resolveCatalogVolume(ctx context.Context, volume types.Volume) } if _, statErr := os.Stat(entry.disk.Path); statErr != nil { // the operator's path stays server-side; the refusal matches the unknown one - log.WithFunc("pool.resolveCatalogVolume").Warnf(ctx, "stat volume %q image %s: %v", volume.Name, entry.disk.Path, statErr) + log.WithFunc("pool.resolveCatalogEntry").Warnf(ctx, "stat volume %q image %s: %v", volume.Name, entry.disk.Path, statErr) return resolvedVolume{}, ErrVolumeUnavailable } disk := entry.disk disk.RW = volume.RW() - volume.AttachOnly = false // past validation the empty mount carries it - return resolvedVolume{disk: disk, applied: volume}, nil + return resolvedVolume{entry: entry, disk: disk, applied: volume}, nil } // confirmVolumesClean refuses a read-only claim of a dirty image; run after holds are taken. @@ -199,7 +213,7 @@ func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes [ // applyVolume keeps one volume's steps strictly ordered; siblings overlap freely. func (m *Manager) applyVolume(ctx context.Context, sb *types.Sandbox, volume resolvedVolume, live bool) error { // attach-only mounts nothing, so there is no umount to verify and no marker. - attachOnly := volume.applied.Mount == "" + attachOnly := volume.applied.AttachOnly // write-ahead: the marker must be durable before any guest write can be. if volume.disk.RW && !attachOnly { if err := markVolumeDirty(volume.disk.Path); err != nil { @@ -227,7 +241,10 @@ func (m *Manager) applyVolume(ctx context.Context, sb *types.Sandbox, volume res // quiesceVolumes unmounts a claim's writable mounts in reverse; a failed unmount keeps the marker. func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeTeardown { - td := volumeTeardown{holds: heldVolumes(sb)} + m.mu.Lock() + td := volumeTeardown{run: m.volumeRuns[sb], holds: heldVolumes(sb)} + catalog := m.catalog.entries + m.mu.Unlock() mounts := writableMounts(sb.Volumes) if mounts == 0 { return td @@ -246,7 +263,7 @@ func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeT stuck = true continue } - if entry, ok := m.volumes[volume.Name]; ok { + if entry, ok := catalog[volume.Name]; ok { td.clears = append(td.clears, entry.disk.Path) } } @@ -261,59 +278,57 @@ func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeT return td } -// finishVolumeTeardown runs only once the VM is gone, when its image lock is finally released. +// finishVolumeTeardown runs once the VM is gone; an in-flight volume run releases the holds instead. func (m *Manager) finishVolumeTeardown(ctx context.Context, td volumeTeardown) { - if len(td.holds) == 0 { - return - } logger := log.WithFunc("pool.finishVolumeTeardown") for _, path := range td.clears { if err := clearVolumeDirty(path); err != nil { logger.Errorf(ctx, err, "clear dirty marker %s", path) } } - m.unreserveVolumes(td.holds) -} - -// reserveVolumes admits one claim's volumes atomically; callers hold m.mu. -func (m *Manager) reserveVolumes(volumes []types.Volume) error { - for _, volume := range volumes { - holders := m.volumeAdmission[volume.Name] - if holders.writers > 0 || (volume.RW() && holders.readers > 0) { - return fmt.Errorf("%w: volume %q", ErrVolumeBusy, volume.Name) + m.mu.Lock() + defer m.mu.Unlock() + if td.unknown { + m.catalog.unknown-- + } + if td.run != nil { + select { + case <-td.run.done: + default: + td.run.teardown = append(td.run.teardown, td.holds...) + return } } - m.adoptVolumes(volumes) - return nil + m.releaseVolumes(td.holds) } // adoptVolumes counts volumes an adopted claim already holds; callers hold m.mu. func (m *Manager) adoptVolumes(volumes []types.Volume) { for _, volume := range volumes { - holders := m.volumeAdmission[volume.Name] + holders := m.catalog.holders[volume.Name] if volume.RW() { holders.writers++ } else { holders.readers++ } - m.volumeAdmission[volume.Name] = holders + m.catalog.holders[volume.Name] = holders } } // releaseVolumes drops one claim's admission holds; callers hold m.mu. func (m *Manager) releaseVolumes(volumes []types.Volume) { for _, volume := range volumes { - holders := m.volumeAdmission[volume.Name] + holders := m.catalog.holders[volume.Name] if volume.RW() { holders.writers-- } else { holders.readers-- } if holders.writers < 1 && holders.readers < 1 { - delete(m.volumeAdmission, volume.Name) + delete(m.catalog.holders, volume.Name) continue } - m.volumeAdmission[volume.Name] = holders + m.catalog.holders[volume.Name] = holders } } @@ -326,6 +341,33 @@ func (m *Manager) unreserveVolumes(volumes []types.Volume) { m.releaseVolumes(volumes) } +func (m *Manager) catalogSnapshot() map[string]*catalogVolume { + m.mu.Lock() + defer m.mu.Unlock() + return m.catalog.entries +} + +// checkResolved refuses the request if any volume is fenced, rebound or busy; callers hold m.mu. +func (m *Manager) checkResolved(volumes []resolvedVolume, ownerID string) error { + for _, resolved := range volumes { + volume := resolved.applied + if err := m.catalog.admissionError(volume.Name); err != nil { + return err + } + if m.catalog.entries[volume.Name] != resolved.entry { + return ErrVolumeUnavailable + } + if m.store.volumeConflicts(volume, ownerID) { + return fmt.Errorf("%w: volume %q", ErrVolumeBusy, volume.Name) + } + holders := m.catalog.holders[volume.Name] + if holders.writers > 0 || (volume.RW() && holders.readers > 0) { + return fmt.Errorf("%w: volume %q", ErrVolumeBusy, volume.Name) + } + } + return nil +} + // quiesceBudget bounds one quiesce: a slice per writable mount plus one for the sync fallback. func quiesceBudget(mounts int) time.Duration { return min(time.Duration(mounts+1)*engine.VolumeCallTimeout, volumeQuiesceMax) @@ -342,7 +384,7 @@ func writableMounts(volumes []types.Volume) int { } func needsUnmount(volume types.Volume) bool { - return volume.RW() && volume.Mount != "" + return volume.RW() && !volume.AttachOnly } func appliedVolumes(volumes []resolvedVolume) []types.Volume { diff --git a/sandboxd/pool/volume_acl_test.go b/sandboxd/pool/volume_acl_test.go new file mode 100644 index 00000000..f4136667 --- /dev/null +++ b/sandboxd/pool/volume_acl_test.go @@ -0,0 +1,55 @@ +package pool + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestRuntimeCatalogEmptyACLIsPublic(t *testing.T) { + eng := newFakeEngine() + m := newVolumeManager(t, eng, nil) + spec := types.VolumeRegistration{Path: writeVolumeImage(t, "home.img", "private"), Writable: true, Tenants: []string{}} + if err := m.RegisterVolume(t.Context(), "home", spec); err != nil { + t.Fatal(err) + } + for _, current := range []*Manager{m, newVolumeManagerAt(t, eng, m.dataDir, nil)} { + if got := current.Volumes("acme", nil); len(got) != 1 || got[0].Name != "home" { + t.Fatalf("public runtime volume hidden from a tenant: %+v", got) + } + if local, err := current.VolumePlacement(testKey, "acme", []string{"home"}); err != nil || !local { + t.Fatalf("tenant placement: %v %v", local, err) + } + sb, err := current.ClaimProvision(t.Context(), testKey, ClaimOptions{Tenant: "acme", Volumes: []types.Volume{{Name: "home", Mount: "/home"}}}) + if err != nil { + t.Fatalf("tenant attachment: %v", err) + } + if err = current.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + } +} + +func TestInvalidStaticTenantListNeverBecomesPublic(t *testing.T) { + path := writeVolumeImage(t, "data", "private contents") + file := filepath.Join(t.TempDir(), "config.json") + raw := fmt.Sprintf(`{"pools":[],"volumes":[{"name":"data","path":%q,"tenants":["has space"]}]}`, path) + if err := os.WriteFile(file, []byte(raw), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := config.Load(file) + if err != nil { + t.Fatalf("legacy config stopped booting: %v", err) + } + m := newVolumeManager(t, newFakeEngine(), cfg.Volumes) + if got := m.Volumes("acme", nil); len(got) != 0 { + t.Fatalf("invalid restricted ACL became public: %+v", got) + } + if err = m.RegisterVolume(t.Context(), "other", types.VolumeRegistration{Path: path + ".other"}); err != nil { + t.Fatalf("invalid legacy ACL blocked unrelated registration: %v", err) + } +} diff --git a/sandboxd/pool/volume_attach_test.go b/sandboxd/pool/volume_attach_test.go index 7fc14e75..519f4aff 100644 --- a/sandboxd/pool/volume_attach_test.go +++ b/sandboxd/pool/volume_attach_test.go @@ -21,7 +21,7 @@ func TestAttachOnlyClaimAttachesWithoutMounting(t *testing.T) { if err != nil { t.Fatalf("ClaimProvision: %v", err) } - want := []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}} + want := []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW, AttachOnly: true}} if !slices.Equal(sb.Volumes, want) { t.Errorf("applied volumes=%v, want %v", sb.Volumes, want) } @@ -78,11 +78,7 @@ func TestAttachOnlyClaimAttachesEveryVolumeConcurrently(t *testing.T) { if err != nil { t.Fatalf("ClaimProvision: %v", err) } - want := []types.Volume{ - {Name: "scratch", Mode: types.VolumeModeRW}, - {Name: "dataset"}, - {Name: "cache", Mode: types.VolumeModeRW}, - } + want := slices.Clone(requested) if !slices.Equal(sb.Volumes, want) { t.Errorf("volumes=%v, want the request order %v", sb.Volumes, want) } @@ -380,7 +376,7 @@ func TestQuiesceVolumesSkipsAttachOnlyEntries(t *testing.T) { sb := &types.Sandbox{ ID: "sb_mixed", VMName: "sbx-mixed", Key: testKey, VsockSocket: "/vsock/mixed", Volumes: []types.Volume{ - {Name: "scratch", Mode: types.VolumeModeRW}, + {Name: "scratch", Mode: types.VolumeModeRW, AttachOnly: true}, {Name: "mounted", Mount: "/volumes/mounted", Mode: types.VolumeModeRW}, }, } @@ -406,11 +402,11 @@ func TestWritableMountsCountsOnlyQuiescableEntries(t *testing.T) { }{ {"none", nil, 0}, {"read-only mount", []types.Volume{{Name: "a", Mount: "/a"}}, 0}, - {"attach-only writer", []types.Volume{{Name: "a", Mode: types.VolumeModeRW}}, 0}, + {"attach-only writer", []types.Volume{{Name: "a", Mode: types.VolumeModeRW, AttachOnly: true}}, 0}, {"writable mount", []types.Volume{{Name: "a", Mount: "/a", Mode: types.VolumeModeRW}}, 1}, {"mixed", []types.Volume{ {Name: "a", Mount: "/a", Mode: types.VolumeModeRW}, - {Name: "b", Mode: types.VolumeModeRW}, + {Name: "b", Mode: types.VolumeModeRW, AttachOnly: true}, {Name: "c", Mount: "/c"}, }, 1}, } { diff --git a/sandboxd/pool/volume_bench_test.go b/sandboxd/pool/volume_bench_test.go new file mode 100644 index 00000000..a7c75e0b --- /dev/null +++ b/sandboxd/pool/volume_bench_test.go @@ -0,0 +1,83 @@ +package pool + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func BenchmarkVolumeAdmissionWithCheckpoints(b *testing.B) { + for _, checkpoints := range []bool{false, true} { + b.Run(fmt.Sprintf("claims=5000/checkpoints=%t", checkpoints), func(b *testing.B) { + m := volumeBenchManager(b, checkpoints) + requested := []types.Volume{{Name: "bench-data", Mount: "/mnt/data"}} + b.ReportAllocs() + for b.Loop() { + resolved, err := m.resolveVolumes(b.Context(), "", requested) + if err != nil { + b.Fatal(err) + } + if len(resolved) != 1 || resolved[0].applied != requested[0] { + b.Fatalf("resolved volumes: %+v", resolved) + } + applied := appliedVolumes(resolved) + m.mu.Lock() + err = m.checkResolved(resolved, "") + if err == nil { + m.adoptVolumes(applied) + m.releaseVolumes(applied) + } + m.mu.Unlock() + if err != nil { + b.Fatal(err) + } + } + if len(m.catalog.holders) != 0 { + b.Fatalf("unreleased admission holds: %v", m.catalog.holders) + } + }) + } +} + +func BenchmarkVolumeHoldsWithCheckpoints(b *testing.B) { + for _, checkpoints := range []bool{false, true} { + b.Run(fmt.Sprintf("claims=5000/checkpoints=%t", checkpoints), func(b *testing.B) { + m := volumeBenchManager(b, checkpoints) + b.ReportAllocs() + for b.Loop() { + m.mu.Lock() + held := 0 + for _, sb := range m.claimed { + if m.hasVolumeHolds(sb) { + held++ + } + } + m.mu.Unlock() + if held != 0 { + b.Fatalf("unrelated claims hold %d volumes", held) + } + } + }) + } +} + +func volumeBenchManager(b *testing.B, checkpoints bool) *Manager { + b.Helper() + m := newTestManager(b, newFakeEngine(), config.PoolSpec{PoolKey: testKey}) + m.claimed = checkpointBenchClaims(checkpoints) + if err := m.store.save(m.claimed); err != nil { + b.Fatal(err) + } + path := filepath.Join(b.TempDir(), "data.raw") + if err := os.WriteFile(path, []byte("benchmark backing"), 0o600); err != nil { + b.Fatal(err) + } + if err := m.RegisterVolume(b.Context(), "bench-data", types.VolumeRegistration{Path: path}); err != nil { + b.Fatal(err) + } + return m +} diff --git a/sandboxd/pool/volume_catalog.go b/sandboxd/pool/volume_catalog.go new file mode 100644 index 00000000..c5622f4d --- /dev/null +++ b/sandboxd/pool/volume_catalog.go @@ -0,0 +1,257 @@ +package pool + +import ( + "cmp" + "context" + "encoding/json/v2" + "errors" + "fmt" + "maps" + "os" + "path/filepath" + "slices" + "strings" + "sync" + "unicode/utf8" + + "github.com/projecteru2/core/log" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/engine" + "github.com/cocoonstack/sandbox/sandboxd/types" + "github.com/cocoonstack/sandbox/sandboxd/utils" +) + +const runtimeVolumesFile = "runtime-volumes.json" + +type runtimeCatalog struct { + Version int `json:"version"` + Volumes map[string]types.VolumeRegistration `json:"volumes"` +} + +type catalogWrite struct { + name string + entries map[string]*catalogVolume + raw []byte +} + +type volumeCatalog struct { + entries map[string]*catalogVolume + roots []string + pending *catalogWrite + holders map[string]volumeHolders + unknown int + writeMu sync.Mutex +} + +func (c *volumeCatalog) admissionError(name string) error { + if c.unknown > 0 || c.pending != nil && c.pending.name == name { + return ErrVolumeCatalogUnavailable + } + return nil +} + +func (c *volumeCatalog) withinRoots(path string) bool { + return len(c.roots) == 0 || slices.ContainsFunc(c.roots, func(root string) bool { + return path == root || strings.HasPrefix(path, strings.TrimSuffix(root, string(filepath.Separator))+string(filepath.Separator)) + }) +} + +// RegisterVolume durably binds a root-managed name to a backing path. +func (m *Manager) RegisterVolume(ctx context.Context, name string, spec types.VolumeRegistration) error { + volume, err := normalizeRegistration(name, spec) + if err != nil { + return err + } + if !m.catalog.withinRoots(volume.disk.Path) { + return fmt.Errorf("%w: volume %q path is outside volume_roots", ErrBadVolume, name) + } + m.catalog.writeMu.Lock() + defer m.catalog.writeMu.Unlock() + if err = m.commitPendingCatalog(ctx); err != nil { + return err + } + m.mu.Lock() + entries := m.catalog.entries + err = m.catalog.admissionError(name) + m.mu.Unlock() + if err != nil { + return err + } + if entry := entries[name]; entry != nil { + if entry.runtime && entry.disk == volume.disk && entry.writable == volume.writable && slices.Equal(entry.tenants, volume.tenants) { + return nil + } + return fmt.Errorf("%w: volume %q already has different semantics or is static", ErrVolumeRegistrationConflict, name) + } + if err := catalogPathConflict(entries, volume.disk.Path); err != nil { + return fmt.Errorf("%w: %w", ErrVolumeRegistrationConflict, err) + } + entries = maps.Clone(entries) + entries[name] = volume + return m.writeCatalog(ctx, name, entries) +} + +// UnregisterVolume retires an unreferenced runtime binding without touching its backing. +func (m *Manager) UnregisterVolume(ctx context.Context, name string) error { + if !types.ValidVolumeName(name) { + return fmt.Errorf("%w: invalid volume name", ErrBadVolume) + } + m.catalog.writeMu.Lock() + defer m.catalog.writeMu.Unlock() + if err := m.commitPendingCatalog(ctx); err != nil { + return err + } + m.mu.Lock() + entries := m.catalog.entries + entry := entries[name] + err := m.catalog.admissionError(name) + if err == nil && ((entry != nil && !entry.runtime) || (entry == nil && m.volumeReferenced(name))) { + err = fmt.Errorf("%w: volume %q is static or referenced", ErrVolumeRegistrationConflict, name) + } + m.mu.Unlock() + if err != nil || entry == nil { + return err + } + entries = maps.Clone(entries) + delete(entries, name) + return m.writeCatalog(ctx, name, entries) +} + +func (m *Manager) loadRuntimeVolumes(ctx context.Context) error { + raw, err := os.ReadFile(filepath.Join(m.dataDir, runtimeVolumesFile)) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("read runtime volume catalog: %w", err) + } + var record runtimeCatalog + if err = json.Unmarshal(raw, &record, json.RejectUnknownMembers(true)); err != nil { + return fmt.Errorf("parse runtime volume catalog: %w", err) + } + if record.Version != 1 || record.Volumes == nil { + return errors.New("invalid runtime volume catalog version or volumes") + } + entries := maps.Clone(m.catalog.entries) + for name, spec := range record.Volumes { + volume, normalizeErr := normalizeRegistration(name, spec) + if normalizeErr != nil { + return fmt.Errorf("runtime volume catalog: %w", normalizeErr) + } + if m.catalog.entries[name] != nil { + return fmt.Errorf("runtime volume catalog duplicates static name %q", name) + } + if err := catalogPathConflict(entries, volume.disk.Path); err != nil { + return fmt.Errorf("runtime volume catalog: %w", err) + } + entries[name] = volume + } + // skip only once every entry validated, so other bad metadata still refuses startup + for name := range record.Volumes { + if path := entries[name].disk.Path; !m.catalog.withinRoots(path) { + log.WithFunc("pool.loadRuntimeVolumes").Warnf(ctx, "runtime volume %q: skipping image %s outside volume_roots", name, path) + delete(entries, name) + } + } + m.catalog.entries = entries + return nil +} + +func (m *Manager) volumeReferenced(name string) bool { + if m.catalog.holders[name] != (volumeHolders{}) { + return true + } + // an adopted claim is published with no holders before its claims.json write counts it + for _, sb := range m.claimed { + if volumeIndex(sb.Volumes, name) >= 0 || volumeIndex(sb.VolumesWant, name) >= 0 { + return true + } + } + return m.store.referencesVolume(name) +} + +func (m *Manager) writeCatalog(ctx context.Context, name string, entries map[string]*catalogVolume) error { + record := runtimeCatalog{Version: 1, Volumes: map[string]types.VolumeRegistration{}} + for key, entry := range entries { + if entry.runtime { + record.Volumes[key] = types.VolumeRegistration{Path: entry.disk.Path, DirectIO: entry.disk.DirectIO, Writable: entry.writable, Tenants: entry.tenants} + } + } + raw, _ := json.Marshal(record, json.Deterministic(true)) + proposal := &catalogWrite{name: name, entries: entries, raw: raw} + m.mu.Lock() + err := ctx.Err() + if err == nil && m.volumeReferenced(name) { + err = fmt.Errorf("%w: volume %q is referenced", ErrVolumeRegistrationConflict, name) + } + if err == nil { + m.catalog.pending = proposal + } + m.mu.Unlock() + if err != nil { + return err + } + return m.persistCatalog(proposal) +} + +func (m *Manager) commitPendingCatalog(ctx context.Context) error { + m.mu.Lock() + proposal := m.catalog.pending + m.mu.Unlock() + if proposal == nil { + return nil + } + if err := ctx.Err(); err != nil { + return fmt.Errorf("%w: %w", ErrVolumeCatalogUnavailable, err) + } + return m.persistCatalog(proposal) +} + +func (m *Manager) persistCatalog(proposal *catalogWrite) error { + if err := utils.WriteFileSync(filepath.Join(m.dataDir, runtimeVolumesFile), proposal.raw, 0o600); err != nil { + return fmt.Errorf("%w: %w", ErrVolumeCatalogUnavailable, err) + } + m.mu.Lock() + m.catalog.entries = proposal.entries + m.catalog.pending = nil + m.mu.Unlock() + return nil +} + +func (m *Manager) retryVolumeCatalog(ctx context.Context) { + if !m.catalog.writeMu.TryLock() { + return + } + defer m.catalog.writeMu.Unlock() + if err := m.commitPendingCatalog(ctx); err != nil { + log.WithFunc("pool.retryVolumeCatalog").Error(ctx, err, "persist runtime volume catalog") + } +} + +func catalogPathConflict(entries map[string]*catalogVolume, path string) error { + for name, entry := range entries { + if filepath.Clean(entry.disk.Path) == path { + return fmt.Errorf("backing path already registered as %q", name) + } + } + return nil +} + +func normalizeRegistration(name string, spec types.VolumeRegistration) (*catalogVolume, error) { + spec.DirectIO = cmp.Or(spec.DirectIO, types.DirectIOOff) + if !utf8.ValidString(spec.Path) { + return nil, fmt.Errorf("%w: volume %q path must be valid UTF-8", ErrBadVolume, name) + } + if err := config.ValidateVolumeCatalog([]config.VolumeSpec{{ + Name: name, Path: spec.Path, DirectIO: spec.DirectIO, Writable: spec.Writable, Tenants: spec.Tenants, + }}); err != nil { + return nil, fmt.Errorf("%w: %w", ErrBadVolume, err) + } + return &catalogVolume{ + runtime: true, + disk: engine.VolumeSpec{Name: name, Path: filepath.Clean(spec.Path), DirectIO: spec.DirectIO}, + tenants: slices.Compact(slices.Sorted(slices.Values(spec.Tenants))), + writable: spec.Writable, + }, nil +} diff --git a/sandboxd/pool/volume_catalog_test.go b/sandboxd/pool/volume_catalog_test.go new file mode 100644 index 00000000..34d7697e --- /dev/null +++ b/sandboxd/pool/volume_catalog_test.go @@ -0,0 +1,536 @@ +package pool + +import ( + "context" + "encoding/json/v2" + "errors" + "fmt" + "maps" + "os" + "path/filepath" + "slices" + "sync" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/types" + "github.com/cocoonstack/sandbox/sandboxd/utils" +) + +func TestRuntimeCatalogPersistsNormalizedSemantics(t *testing.T) { + eng := newFakeEngine() + m := newVolumeManager(t, eng, nil) + path := writeVolumeImage(t, "runtime", "backing bytes stay intact") + spec := types.VolumeRegistration{Path: filepath.Dir(path) + "/./runtime", Writable: true, Tenants: []string{"beta", "acme", "acme"}} + before := m.catalogSnapshot() + if err := m.RegisterVolume(t.Context(), "runtime", spec); err != nil { + t.Fatal(err) + } + if before["runtime"] != nil || spec.Tenants[0] != "beta" { + t.Fatal("registration mutated a captured catalog or the caller's spec") + } + normalized := types.VolumeRegistration{Path: path, DirectIO: types.DirectIOOff, Writable: true, Tenants: []string{"acme", "beta"}} + if err := m.RegisterVolume(t.Context(), "runtime", normalized); err != nil { + t.Fatalf("normalized retry: %v", err) + } + if st, err := os.Stat(filepath.Join(m.dataDir, runtimeVolumesFile)); err != nil || st.Mode().Perm() != 0o600 { + t.Fatalf("metadata mode: %v %v", st, err) + } + sb, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "runtime"}}}) + if err != nil { + t.Fatal(err) + } + m2 := newVolumeManagerAt(t, eng, m.dataDir, nil) + if got := m2.Volumes("acme", nil); len(got) != 1 || got[0].Name != "runtime" || !got[0].Available || !got[0].Writable { + t.Fatalf("catalog absent before reconcile: %+v", got) + } + if got := m2.Volumes("forbidden", nil); len(got) != 0 { + t.Fatalf("ACL changed at restart: %+v", got) + } + if err = m2.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + if _, err = m2.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "runtime", Mode: types.VolumeModeRW}}}); !errors.Is(err, ErrVolumeBusy) { + t.Fatalf("recovered holder missing: %v", err) + } + changed := normalized + changed.DirectIO = "on" + if err = m2.RegisterVolume(t.Context(), "runtime", changed); !errors.Is(err, ErrVolumeRegistrationConflict) { + t.Fatalf("changed semantics: %v", err) + } + if err = m2.UnregisterVolume(t.Context(), "runtime"); !errors.Is(err, ErrVolumeRegistrationConflict) { + t.Fatalf("unregister of a held volume: %v", err) + } + if err = m2.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + for range 2 { + if err = m2.UnregisterVolume(t.Context(), "runtime"); err != nil { + t.Fatal(err) + } + } + if got, err := os.ReadFile(path); err != nil || string(got) != "backing bytes stay intact" { + t.Fatalf("backing changed: %q %v", got, err) + } + if got := newVolumeManagerAt(t, eng, m.dataDir, nil).VolumeNames(); len(got) != 0 { + t.Fatalf("unregister not durable: %v", got) + } +} + +func TestRuntimeCatalogRegistersUncheckedBacking(t *testing.T) { + link := filepath.Join(t.TempDir(), "backing-link") + if err := os.Symlink(writeVolumeImage(t, "backing", "bytes"), link); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name, path string + wantErr error + }{{"symlink", link, nil}, {"missing", filepath.Join(t.TempDir(), "missing"), ErrVolumeUnavailable}} { + t.Run(tc.name, func(t *testing.T) { + m := newVolumeManager(t, newFakeEngine(), nil) + registration := types.VolumeRegistration{Path: tc.path, Writable: true} + if err := m.RegisterVolume(t.Context(), "runtime", registration); err != nil { + t.Fatalf("register: %v", err) + } + m2 := newVolumeManagerAt(t, newFakeEngine(), m.dataDir, nil) + if err := m2.RegisterVolume(t.Context(), "runtime", registration); err != nil { + t.Fatalf("idempotent retry after restart: %v", err) + } + if infos := m2.Volumes("", nil); len(infos) != 1 || infos[0].Available != (tc.wantErr == nil) { + t.Fatalf("availability: %+v", infos) + } + if _, err := m2.resolveVolumes(t.Context(), "", []types.Volume{{Name: "runtime"}}); !errors.Is(err, tc.wantErr) { + t.Fatalf("resolve = %v, want %v", err, tc.wantErr) + } + if err := m2.UnregisterVolume(t.Context(), "runtime"); err != nil { + t.Fatal(err) + } + }) + } +} + +func TestRuntimeCatalogRejectsCollisionsAndInvalidRegistrations(t *testing.T) { + static := liveCatalog(t) + m := newVolumeManager(t, newFakeEngine(), static) + other := writeVolumeImage(t, "other-runtime", "bytes") + sb := mustClaim(t, m, testKey) + sb.Volumes = []types.Volume{{Name: "unexplained", Mount: "/data"}} + if err := m.store.commit(m.store.set(sb)); err != nil { + t.Fatal(err) + } + alias := filepath.Dir(static[0].Path) + "/./" + filepath.Base(static[0].Path) + for _, tc := range []struct { + name string + spec types.VolumeRegistration + want error + }{ + {"data", types.VolumeRegistration{Path: other}, ErrVolumeRegistrationConflict}, + {"runtime", types.VolumeRegistration{Path: static[0].Path}, ErrVolumeRegistrationConflict}, + {"runtime", types.VolumeRegistration{Path: alias}, ErrVolumeRegistrationConflict}, + {"unexplained", types.VolumeRegistration{Path: other}, ErrVolumeRegistrationConflict}, + {"cocoon-reserved", types.VolumeRegistration{Path: other}, ErrBadVolume}, + {"bad/name", types.VolumeRegistration{Path: other}, ErrBadVolume}, + {"runtime", types.VolumeRegistration{Path: "relative"}, ErrBadVolume}, + {"runtime", types.VolumeRegistration{Path: other, DirectIO: "invalid"}, ErrBadVolume}, + {"runtime", types.VolumeRegistration{Path: other, Tenants: []string{"has space"}}, ErrBadVolume}, + {"runtime", types.VolumeRegistration{Path: "/srv/\xff"}, ErrBadVolume}, + } { + if err := m.RegisterVolume(t.Context(), tc.name, tc.spec); !errors.Is(err, tc.want) { + t.Errorf("register %s %+v = %v, want %v", tc.name, tc.spec, err, tc.want) + } + } + if _, err := os.Stat(filepath.Join(m.dataDir, runtimeVolumesFile)); !errors.Is(err, os.ErrNotExist) || m.catalog.entries["runtime"] != nil { + t.Fatalf("rejected registration persisted: stat=%v entry=%+v", err, m.catalog.entries["runtime"]) + } + for _, tc := range []struct { + name string + want error + }{{"data", ErrVolumeRegistrationConflict}, {"unexplained", ErrVolumeRegistrationConflict}, {"bad/name", ErrBadVolume}} { + if err := m.UnregisterVolume(t.Context(), tc.name); !errors.Is(err, tc.want) { + t.Errorf("unregister %s = %v, want %v", tc.name, err, tc.want) + } + } +} + +func TestRuntimeCatalogVolumeRootsBoundRegistration(t *testing.T) { + newRootedManager := func(t *testing.T, eng *fakeEngine, dataDir string, roots ...string) (*Manager, error) { + t.Helper() + cfg := &config.Config{DataDir: dataDir, VolumeRoots: roots} + return NewManager(t.Context(), cfg, eng, testSecrets(t), testBackends(t, cfg)) + } + for _, tc := range []struct { + name, path string + roots []string + want error + }{ + {"inside a root", "/srv/data/a.img", []string{"/mnt/other", "/srv/data"}, nil}, + {"equal to a root", "/srv/data/", []string{"/srv/data"}, nil}, + {"sibling sharing a prefix", "/srv/data2/a.img", []string{"/srv/data"}, ErrBadVolume}, + {"dot-dot out of a root", "/srv/data/../etc/a.img", []string{"/srv/data"}, ErrBadVolume}, + {"outside every root", "/etc/a.img", []string{"/srv/data"}, ErrBadVolume}, + {"filesystem root", "/etc/a.img", []string{"/"}, nil}, + {"no roots", "/etc/a.img", nil, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + m, err := newRootedManager(t, newFakeEngine(), t.TempDir(), tc.roots...) + if err != nil { + t.Fatal(err) + } + if err := m.RegisterVolume(t.Context(), "runtime", types.VolumeRegistration{Path: tc.path}); !errors.Is(err, tc.want) { + t.Fatalf("register %s under %v = %v, want %v", tc.path, tc.roots, err, tc.want) + } + }) + } + eng := newFakeEngine() + m := newVolumeManager(t, eng, nil) + images := map[string]string{"outside": writeVolumeImage(t, "outside", "bytes"), "inside": writeVolumeImage(t, "inside", "bytes")} + for name, path := range images { + if err := m.RegisterVolume(t.Context(), name, types.VolumeRegistration{Path: path}); err != nil { + t.Fatal(err) + } + } + outside, inside := filepath.Dir(images["outside"]), filepath.Dir(images["inside"]) + sb, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "outside"}}}) + if err != nil { + t.Fatal(err) + } + wide, err := newRootedManager(t, eng, m.dataDir, outside, inside) + if err != nil || wide.catalogSnapshot()["outside"] == nil { + t.Fatalf("restart with the registration inside volume_roots: %v", err) + } + m2, err := newRootedManager(t, eng, m.dataDir, inside) + if err != nil { + t.Fatalf("restart refused a persisted registration outside volume_roots: %v", err) + } + if err = m2.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + if got := m2.catalogSnapshot(); got["outside"] != nil || got["inside"] == nil { + t.Fatalf("narrowed catalog = %v, want only the in-root registration", slices.Sorted(maps.Keys(got))) + } + if got := volumeHoldersOf(m2, "outside"); got.readers != 1 { + t.Fatalf("recovered claim holders of the skipped registration = %+v", got) + } + if _, err = m2.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "outside"}}}); !errors.Is(err, ErrVolumeUnavailable) { + t.Fatalf("claim of the skipped registration = %v, want ErrVolumeUnavailable", err) + } + if err = claimAndRelease(t, m2, types.Volume{Name: "inside"}); err != nil { + t.Fatalf("in-root registration: %v", err) + } + if err = m2.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatalf("release of a recovered claim holding the skipped registration: %v", err) + } + if err = m2.RegisterVolume(t.Context(), "later", types.VolumeRegistration{Path: filepath.Join(inside, "later")}); err != nil { + t.Fatal(err) + } + if wide, err = newRootedManager(t, eng, m.dataDir, outside, inside); err != nil || wide.catalogSnapshot()["outside"] != nil { + t.Fatalf("a later catalog write kept the skipped registration: %v", err) + } +} + +func TestRuntimeCatalogPendingWriteConverges(t *testing.T) { + eng := newFakeEngine() + m := newVolumeManager(t, eng, liveCatalog(t)) + spec := types.VolumeRegistration{Path: writeVolumeImage(t, "runtime", "bytes")} + canceled, cancel := context.WithCancel(t.Context()) + cancel() + if err := m.RegisterVolume(canceled, "runtime", spec); !errors.Is(err, context.Canceled) || errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("cancellation before acceptance: %v", err) + } + m.retryVolumeCatalog(t.Context()) + if _, err := os.Stat(filepath.Join(m.dataDir, runtimeVolumesFile)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("canceled proposal left catalog: %v", err) + } + restore := blockCatalogRename(t, m) + if err := m.RegisterVolume(t.Context(), "runtime", spec); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("write failure: %v", err) + } + if err := m.UnregisterVolume(t.Context(), "runtime"); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("failed retry cleared pending write: %v", err) + } + for _, err := range []error{m.RegisterVolume(t.Context(), "bad/name", spec), m.UnregisterVolume(t.Context(), "bad/name")} { + if !errors.Is(err, ErrBadVolume) || errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("invalid name behind a pending write: %v", err) + } + } + m.retryVolumeCatalog(t.Context()) + for _, volumes := range [][]types.Volume{nil, {{Name: "data"}}} { + if err := claimAndRelease(t, m, volumes...); err != nil { + t.Fatalf("pending write blocked unrelated claim %v: %v", volumes, err) + } + } + restore() + installPendingCatalogRecord(t, m) + if got := newVolumeManagerAt(t, eng, m.dataDir, nil).VolumeNames(); !slices.Equal(got, []string{"runtime"}) { + t.Fatalf("possible replacement not recovered: %v", got) + } + if _, err := m.resolveVolumes(t.Context(), "", []types.Volume{{Name: "runtime"}}); !errors.Is(err, ErrVolumeUnavailable) { + t.Fatalf("unpublished admission: %v", err) + } + if err := m.UnregisterVolume(canceled, "runtime"); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("canceled retry cleared pending write: %v", err) + } + m.retryVolumeCatalog(t.Context()) + if !slices.Contains(m.VolumeNames(), "runtime") { + t.Fatal("retry lost accepted registration") + } + restore = blockCatalogRename(t, m) + if err := m.UnregisterVolume(t.Context(), "runtime"); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("failed unregister: %v", err) + } + restore() + installPendingCatalogRecord(t, m) + if got := newVolumeManagerAt(t, eng, m.dataDir, nil).VolumeNames(); len(got) != 0 { + t.Fatalf("possible unregister replacement not recovered: %v", got) + } + if err := m.RegisterVolume(t.Context(), "replacement", spec); err != nil { + t.Fatalf("pending unregister did not converge before reuse: %v", err) + } + if got := newVolumeManagerAt(t, eng, m.dataDir, nil).VolumeNames(); !slices.Equal(got, []string{"replacement"}) { + t.Fatalf("durable reuse: %v", got) + } +} + +func TestRuntimeCatalogPendingWriteFencesOnlyAffectedName(t *testing.T) { + unregister := func(m *Manager, _ types.VolumeRegistration) error { return m.UnregisterVolume(t.Context(), "affected") } + register := func(m *Manager, spec types.VolumeRegistration) error { + return m.RegisterVolume(t.Context(), "affected", spec) + } + for _, tc := range []struct { + name string + before []string + write func(*Manager, types.VolumeRegistration) error + resolved []types.Volume + wantResolved, want error + }{ + {"register", []string{"healthy"}, register, []types.Volume{{Name: "healthy"}}, nil, ErrVolumeUnavailable}, + {"unregister", []string{"healthy", "affected"}, unregister, []types.Volume{{Name: "healthy"}, {Name: "affected"}}, ErrVolumeCatalogUnavailable, ErrVolumeCatalogUnavailable}, + } { + t.Run(tc.name, func(t *testing.T) { + eng := newFakeEngine() + m, sb := newTenantVolumeManager(t, eng, liveCatalog(t)) + specs := map[string]types.VolumeRegistration{ + "healthy": {Path: writeVolumeImage(t, "healthy", "bytes")}, + "affected": {Path: writeVolumeImage(t, "affected", "bytes"), Tenants: []string{"other"}}, + } + for _, name := range tc.before { + if err := m.RegisterVolume(t.Context(), name, specs[name]); err != nil { + t.Fatal(err) + } + } + resolved, err := m.resolveVolumes(t.Context(), "", tc.resolved) + if err != nil { + t.Fatal(err) + } + blockCatalogRename(t, m) + if err = tc.write(m, specs["affected"]); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("obstructed catalog write: %v", err) + } + holds, err := m.admitVolumes("", resolved) + m.unreserveVolumes(holds) + if !errors.Is(err, tc.wantResolved) { + t.Fatalf("admission resolved before the write = %v, want %v", err, tc.wantResolved) + } + for _, volumes := range [][]types.Volume{{{Name: "affected"}}, {{Name: "healthy"}, {Name: "affected"}}} { + if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: volumes}); !errors.Is(err, tc.want) { + t.Errorf("root claim %v = %v, want %v", volumes, err, tc.want) + } + } + _, deniedErr := m.SetVolumes(t.Context(), sb.ID, "acme", []types.Volume{{Name: "data"}, {Name: "affected"}}) + _, missingErr := m.SetVolumes(t.Context(), sb.ID, "acme", []types.Volume{{Name: "data"}, {Name: "missing"}}) + if !errors.Is(deniedErr, ErrVolumeUnavailable) || !errors.Is(missingErr, ErrVolumeUnavailable) || deniedErr.Error() != missingErr.Error() { + t.Errorf("private name distinguishable: denied=%v missing=%v", deniedErr, missingErr) + } + if len(eng.volumeSpecs) != 0 { + t.Fatal("refused request reached device attachment") + } + for _, name := range []string{"data", "healthy", "affected", "missing"} { + if got := volumeHoldersOf(m, name); got != (volumeHolders{}) { + t.Fatalf("refused request reserved %s: %+v", name, got) + } + } + for _, name := range []string{"data", "healthy"} { + if err := claimAndRelease(t, m, types.Volume{Name: name}); err != nil { + t.Errorf("unrelated %s claim: %v", name, err) + } + } + if _, err := m.SetVolumes(t.Context(), sb.ID, "acme", []types.Volume{{Name: "data"}, {Name: "healthy"}}); err != nil { + t.Errorf("unrelated live addition: %v", err) + } + }) + } +} + +func TestResolvedCatalogBindingRequiresSameEntry(t *testing.T) { + m := newVolumeManager(t, newFakeEngine(), nil) + spec := types.VolumeRegistration{Path: writeVolumeImage(t, "runtime", "bytes")} + if err := m.RegisterVolume(t.Context(), "runtime", spec); err != nil { + t.Fatal(err) + } + resolved, err := m.resolveVolumes(t.Context(), "", []types.Volume{{Name: "runtime"}}) + if err != nil { + t.Fatal(err) + } + if err = m.RegisterVolume(t.Context(), "other", types.VolumeRegistration{Path: writeVolumeImage(t, "other", "bytes")}); err != nil { + t.Fatal(err) + } + holds, err := m.admitVolumes("", resolved) + if err != nil { + t.Fatalf("unrelated registration invalidated resolution: %v", err) + } + m.unreserveVolumes(holds) + if err = m.UnregisterVolume(t.Context(), "runtime"); err != nil { + t.Fatal(err) + } + if err = m.RegisterVolume(t.Context(), "runtime", spec); err != nil { + t.Fatal(err) + } + if _, err = m.admitVolumes("", resolved); !errors.Is(err, ErrVolumeUnavailable) { + t.Fatalf("same-file reuse admitted stale resolution: %v", err) + } + if got := volumeHoldersOf(m, "runtime"); got != (volumeHolders{}) { + t.Fatalf("stale binding leaked reservation: %+v", got) + } +} + +func TestRuntimeCatalogRejectsInvalidMetadata(t *testing.T) { + static := liveCatalog(t) + for _, tc := range []struct{ name, raw string }{ + {"malformed", "{"}, + {"version", `{"version":2,"volumes":{}}`}, + {"null", `{"version":1,"volumes":null}`}, + {"name", `{"version":1,"volumes":{"bad/name":{"path":"/data"}}}`}, + {"relative", `{"version":1,"volumes":{"runtime":{"path":"relative"}}}`}, + {"directio", `{"version":1,"volumes":{"runtime":{"path":"/data","directio":"bad"}}}`}, + {"duplicate-name", `{"version":1,"volumes":{"runtime":{"path":"/one"},"runtime":{"path":"/two"}}}`}, + {"duplicate-path", `{"version":1,"volumes":{"runtime":{"path":"/one"},"second":{"path":"/one"}}}`}, + {"static-name", `{"version":1,"volumes":{"data":{"path":"/other"}}}`}, + {"static-path", fmt.Sprintf(`{"version":1,"volumes":{"runtime":{"path":%q}}}`, static[0].Path)}, + {"unknown-field", `{"version":1,"volumes":{},"unexpected":true}`}, + } { + t.Run(tc.name, func(t *testing.T) { + eng := newFakeEngine() + m := newVolumeManager(t, eng, static) + if err := os.WriteFile(filepath.Join(m.dataDir, runtimeVolumesFile), []byte(tc.raw), 0o600); err != nil { + t.Fatal(err) + } + cfg := &config.Config{DataDir: m.dataDir, Volumes: static} + if _, err := NewManager(t.Context(), cfg, eng, testSecrets(t), testBackends(t, cfg)); err == nil { + t.Fatalf("accepted %s metadata", tc.name) + } + }) + } +} + +func TestRuntimeCatalogAcceptanceRechecksReferences(t *testing.T) { + m := newVolumeManager(t, newFakeEngine(), nil) + if err := m.RegisterVolume(t.Context(), "runtime", types.VolumeRegistration{Path: writeVolumeImage(t, "runtime", "bytes")}); err != nil { + t.Fatal(err) + } + entries := maps.Clone(m.catalogSnapshot()) + delete(entries, "runtime") + resolved, err := m.resolveVolumes(t.Context(), "", []types.Volume{{Name: "runtime"}}) + if err != nil { + t.Fatal(err) + } + holds, err := m.admitVolumes("", resolved) + if err != nil { + t.Fatal(err) + } + m.catalog.writeMu.Lock() + defer m.catalog.writeMu.Unlock() + if err := m.writeCatalog(t.Context(), "runtime", entries); !errors.Is(err, ErrVolumeRegistrationConflict) { + t.Fatalf("accepted removal prepared before reservation: %v", err) + } + m.unreserveVolumes(holds) + if err := m.writeCatalog(t.Context(), "runtime", entries); err != nil { + t.Fatalf("reservation retirement did not unblock removal: %v", err) + } +} + +func TestAdoptedClaimReferencesVolumeBeforeItsClaimWrite(t *testing.T) { + c := newFailoverCluster(t) + a, b := c.node(t, "a", t.TempDir(), keepAnHour), c.node(t, "b", t.TempDir(), keepAnHour) + if err := b.RegisterVolume(t.Context(), "data", types.VolumeRegistration{Path: writeVolumeImage(t, "adopted", "bytes")}); err != nil { + t.Fatal(err) + } + sb := mustClaim(t, a, testKey) + mustArchive(t, a, sb) + rec, _, err := c.reg.Get(t.Context(), sb.ID) + claim := &types.Sandbox{} + if err == nil { + err = json.Unmarshal(rec.Claim, claim) + } + // the row is a peer's record, so the adopter cannot assume it names no volumes + claim.Volumes = []types.Volume{{Name: "data", Mount: "/data"}} + if err == nil { + rec.Claim, err = json.Marshal(claim) + } + if err == nil { + _, err = c.reg.Put(t.Context(), rec) + } + if err != nil { + t.Fatal(err) + } + c.expire("a") + // an earlier claims.json write in flight keeps the adoption's write from starting + b.store.writeMu.Lock() + unlock := sync.OnceFunc(b.store.writeMu.Unlock) + t.Cleanup(unlock) + adopted := make(chan error, 1) + go func() { + ok, adoptErr := b.AdoptArchived(t.Context(), sb.ID, sb.Token) + if adoptErr == nil && !ok { + adoptErr = errors.New("not adopted") + } + adopted <- adoptErr + }() + waitFor(t, func() bool { _, held := b.byID(sb.ID); return held }) + err = b.UnregisterVolume(t.Context(), "data") + unlock() + if adoptErr := <-adopted; adoptErr != nil { + t.Fatal(adoptErr) + } + if !errors.Is(err, ErrVolumeRegistrationConflict) { + t.Fatalf("unregister while a published adoption references the volume = %v", err) + } +} + +func blockCatalogRename(t *testing.T, m *Manager) func() { + t.Helper() + record := filepath.Join(m.dataDir, runtimeVolumesFile) + raw, err := os.ReadFile(record) + if err != nil && !errors.Is(err, os.ErrNotExist) { + t.Fatal(err) + } + existed := err == nil + if err := errors.Join(os.RemoveAll(record), os.Mkdir(record, 0o700)); err != nil { + t.Fatal(err) + } + restore := sync.OnceFunc(func() { + if err := os.Remove(record); err != nil { + t.Fatal(err) + } + if existed { + if err := os.WriteFile(record, raw, 0o600); err != nil { + t.Fatal(err) + } + } + }) + t.Cleanup(restore) + return restore +} + +// installPendingCatalogRecord models the canonical bytes after a successful rename and an uncertain directory sync. +func installPendingCatalogRecord(t *testing.T, m *Manager) { + t.Helper() + m.mu.Lock() + proposal := m.catalog.pending + m.mu.Unlock() + if proposal == nil { + t.Fatal("possible replacement fixture requires an accepted pending write") + } + if err := utils.WriteFileSync(filepath.Join(m.dataDir, runtimeVolumesFile), proposal.raw, 0o600); err != nil { + t.Fatal(err) + } +} diff --git a/sandboxd/pool/volume_cleanup_test.go b/sandboxd/pool/volume_cleanup_test.go new file mode 100644 index 00000000..6cd4aacb --- /dev/null +++ b/sandboxd/pool/volume_cleanup_test.go @@ -0,0 +1,290 @@ +package pool + +import ( + "context" + "errors" + "slices" + "sync" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/cocoonstack/sandbox/sandboxd/engine" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestRemovedVMKeepsVolumeUntilCanceledWorkerReturns(t *testing.T) { + base := newFakeEngine() + m := newVolumeManager(t, base, nil) + if err := m.RegisterVolume(t.Context(), "data", types.VolumeRegistration{Path: writeVolumeImage(t, "runtime", "bytes"), Writable: true}); err != nil { + t.Fatal(err) + } + sb := mustClaim(t, m, testKey) + eng := newGatedVolumeEngine(base) + m.eng = eng + changed := make(chan error, 1) + go func() { _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data"}}); changed <- err }() + <-eng.entered + resumed := false + defer func() { + if !resumed { + close(eng.resume) + <-changed + } + }() + released := make(chan error, 1) + go func() { released <- m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}) }() + select { + case err := <-released: + if err != nil { + t.Fatal(err) + } + case <-time.After(time.Second): + t.Fatal("release waited for paused worker") + } + m.retryRemovals(t.Context()).Wait() + if got := volumeHoldersOf(m, "data"); got != (volumeHolders{readers: 1}) { + t.Errorf("removed VM freed active worker count: %+v", got) + } + if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}}); !errors.Is(err, ErrVolumeBusy) { + t.Errorf("writer overtook paused canceled worker: %v", err) + } + if err := m.UnregisterVolume(t.Context(), "data"); !errors.Is(err, ErrVolumeRegistrationConflict) { + t.Errorf("unregister overtook paused worker: %v", err) + } + close(eng.resume) + resumed = true + if err := <-changed; err == nil { + t.Fatal("removed claim accepted late attachment") + } + if got := volumeHoldersOf(m, "data"); got != (volumeHolders{}) { + t.Fatalf("completed worker leaked count: %+v", got) + } + if n := len(slices.DeleteFunc(base.removedNames(), func(n string) bool { return n != sb.VMName })); n != 1 { + t.Fatalf("vm rm ran %d times, want once", n) + } + if err := m.UnregisterVolume(t.Context(), "data"); err != nil { + t.Fatal(err) + } +} + +func TestDispatchedCleanupRetainsCatalogReference(t *testing.T) { + base := newFakeEngine() + m := newVolumeManager(t, base, nil) + if err := m.RegisterVolume(t.Context(), "data", types.VolumeRegistration{Path: writeVolumeImage(t, "runtime", "bytes")}); err != nil { + t.Fatal(err) + } + sb, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data"}}}) + if err != nil { + t.Fatal(err) + } + base.removeErrFor = sb.VMName + if err = m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err == nil { + t.Fatal("expected survivor") + } + wg := dispatchBlocked(t.Context(), t, m) + err = m.UnregisterVolume(t.Context(), "data") + unblock(m, wg) + if !errors.Is(err, ErrVolumeRegistrationConflict) { + t.Fatalf("dispatched cleanup lost guard: %v", err) + } + base.removeErrFor = "" + m.retryRemovals(t.Context()).Wait() + if err := m.UnregisterVolume(t.Context(), "data"); err != nil { + t.Fatal(err) + } +} + +func TestStaleVMFenceFollowsInspection(t *testing.T) { + writer, reader := []types.VMDisk{{Name: "data"}}, []types.VMDisk{{Name: "data", ReadOnly: true}} + busy, fenced, inspectErr := ErrVolumeBusy, ErrVolumeCatalogUnavailable, errors.New("inspection unavailable") + staleStop := vmStateStopped + " stale" + for _, tc := range []struct { + name, swept, listed, state string + disks []types.VMDisk + inspectErr error + want [3]error // data, data rw, other + register error + }{ + {"running writer", vmStateRunning, "sbx-stale", vmStateRunning, writer, nil, [3]error{busy, busy, nil}, nil}, + {"running reader", vmStateRunning, "sbx-stale", vmStateRunning, reader, nil, [3]error{nil, busy, nil}, nil}, + {"running empty", vmStateRunning, "sbx-stale", vmStateRunning, []types.VMDisk{}, nil, [3]error{}, nil}, + {"running omitted disks", vmStateRunning, "sbx-stale", vmStateRunning, nil, nil, [3]error{fenced, fenced, fenced}, fenced}, + {"stopped", vmStateRunning, "sbx-stale", vmStateStopped, writer, nil, [3]error{}, nil}, + {"stopped stale (pid file lost)", vmStateRunning, "sbx-stale", staleStop, writer, nil, [3]error{fenced, fenced, fenced}, fenced}, + {"inspected creating", vmStateRunning, "sbx-stale", vmStateCreating, writer, nil, [3]error{fenced, fenced, fenced}, fenced}, + {"inspect error", vmStateRunning, "sbx-stale", vmStateRunning, writer, inspectErr, [3]error{fenced, fenced, fenced}, fenced}, + {"busy creating", vmStateCreating, "sbx-stale", vmStateCreating, writer, nil, [3]error{fenced, fenced, fenced}, fenced}, + {"busy running", vmStateCreating, "sbx-stale", vmStateRunning, reader, nil, [3]error{nil, busy, nil}, nil}, + {"busy omitted disks", vmStateCreating, "sbx-stale", vmStateRunning, nil, nil, [3]error{fenced, fenced, fenced}, fenced}, + {"busy stopped", vmStateCreating, "sbx-stale", vmStateStopped, writer, nil, [3]error{}, nil}, + {"busy stopped stale", vmStateCreating, "sbx-stale", staleStop, writer, nil, [3]error{fenced, fenced, fenced}, fenced}, + {"busy absent", vmStateCreating, "sbx-other", vmStateRunning, writer, nil, [3]error{}, nil}, + {"busy inspect error", vmStateCreating, "sbx-stale", vmStateRunning, writer, inspectErr, [3]error{fenced, fenced, fenced}, fenced}, + } { + t.Run(tc.name, func(t *testing.T) { + eng := newFakeEngine() + m := newVolumeManager(t, eng, liveCatalog(t)) + name := "sbx-stale" + eng.vms[tc.listed], eng.removeErrFor, eng.staleOutcome = "/vsock/stale", name, engine.StaleCreateBusy + eng.stopped[tc.listed], eng.creating[tc.listed] = tc.state == vmStateStopped || tc.state == staleStop, tc.state == vmStateCreating + eng.stale[tc.listed] = tc.state == staleStop + eng.inspectHook = func() error { return tc.inspectErr } + if m.removeStaleVM(t.Context(), name, types.VMRecord{State: tc.swept, Config: types.VMConfig{Name: name}}) { + t.Fatal("stale VM reported gone") + } + // the disk list appears after the sweep, so running rows are classified by the retry + eng.disks = map[string][]types.VMDisk{name: tc.disks} + m.retryRemovals(t.Context()).Wait() + for i, volume := range []types.Volume{{Name: "data"}, {Name: "data", Mode: types.VolumeModeRW}, {Name: "other"}} { + if err := claimAndRelease(t, m, volume); !errors.Is(err, tc.want[i]) { + t.Errorf("claim %+v = %v, want %v", volume, err, tc.want[i]) + } + } + if err := m.RegisterVolume(t.Context(), "fresh", types.VolumeRegistration{Path: writeVolumeImage(t, "fresh", "bytes")}); !errors.Is(err, tc.register) { + t.Errorf("unrelated registration = %v, want %v", err, tc.register) + } + if err := claimAndRelease(t, m); err != nil { + t.Errorf("volume-free claim: %v", err) + } + eng.removeErrFor, eng.staleOutcome = "", engine.StaleCreateCollected + m.retryRemovals(t.Context()).Wait() + for _, volume := range []types.Volume{{Name: "data", Mode: types.VolumeModeRW}, {Name: "other", Mode: types.VolumeModeRW}} { + if err := claimAndRelease(t, m, volume); err != nil { + t.Errorf("claim %+v after confirmed removal: %v", volume, err) + } + } + if m.catalog.unknown != 0 { + t.Fatalf("cleanup retained or double-released its fence: unknown=%d", m.catalog.unknown) + } + }) + } +} + +func TestStaleInventoryIsTakenAfterFailedRemoval(t *testing.T) { + sweep := func(ctx context.Context, m *Manager, name string) { + m.removeStaleVM(ctx, name, types.VMRecord{Config: types.VMConfig{Name: name}}) + } + retry := func(ctx context.Context, m *Manager, _ string) { m.retryRemovals(ctx).Wait() } + for _, tc := range []struct { + name string + before, after func(context.Context, *Manager, string) + }{{"sweep", func(context.Context, *Manager, string) {}, sweep}, {"retry", sweep, retry}} { + t.Run(tc.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + eng := newFakeEngine() + m := newVolumeManager(t, eng, liveCatalog(t)) + name := "sbx-attach-during-remove" + eng.vms[name], eng.removeErrFor = "/vsock/stale", name + tc.before(t.Context(), m, name) + eng.disks = map[string][]types.VMDisk{name: {{Name: "data", ReadOnly: true}}} + eng.removeStall = make(chan struct{}) + done := make(chan struct{}) + go func() { defer close(done); tc.after(t.Context(), m, name) }() + synctest.Wait() + if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data"}}}); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Errorf("in-flight removal admitted a reader: %v", err) + } + eng.mu.Lock() + eng.disks[name] = append(eng.disks[name], types.VMDisk{Name: "other"}) + eng.mu.Unlock() + close(eng.removeStall) + <-done + for _, c := range []struct { + volume types.Volume + want error + }{ + {types.Volume{Name: "other", Mode: types.VolumeModeRW}, ErrVolumeBusy}, + {types.Volume{Name: "data", Mode: types.VolumeModeRW}, ErrVolumeBusy}, + {types.Volume{Name: "data"}, nil}, + } { + if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{c.volume}}); !errors.Is(err, c.want) { + t.Errorf("claim %+v = %v, want %v", c.volume, err, c.want) + } + } + }) + }) + } +} + +func TestUnknownFencePrecedesInspection(t *testing.T) { + base := newFakeEngine() + m := newVolumeManager(t, base, liveCatalog(t)) + name := "sbx-stale-before-inspect" + base.vms[name], base.removeErrFor = "/vsock/stale", name + entered, resume := make(chan struct{}), make(chan struct{}) + var gated atomic.Bool + base.inspectHook = func() error { + if gated.CompareAndSwap(false, true) { + close(entered) + <-resume + } + return errors.New("inspection unavailable") + } + done := make(chan bool, 1) + go func() { done <- m.removeStaleVM(t.Context(), name, base.record(name)) }() + <-entered + _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}}) + close(resume) + if gone := <-done; gone || !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("admission while inspection was blocked: %v (gone=%t)", err, gone) + } +} + +func TestCanceledRemovalDispatchKeepsUnknownFenceOwned(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + base := newFakeEngine() + m := newVolumeManager(t, base, liveCatalog(t)) + name := "sbx-canceled-dispatch" + base.vms[name], base.removeErrFor = "/vsock/stale", name + base.inspectHook = func() error { return errors.New("inspection unavailable") } + if m.removeStaleVM(t.Context(), name, base.record(name)) { + t.Fatal("expected survivor") + } + ctx, cancel := context.WithCancel(t.Context()) + wg := dispatchBlocked(ctx, t, m) + cancel() + synctest.Wait() + if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data"}}}); !errors.Is(err, ErrVolumeCatalogUnavailable) { + t.Fatalf("dispatched unknown fence lost: %v", err) + } + base.removeErrFor = "" + unblock(m, wg) + m.retryRemovals(t.Context()).Wait() + if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}}); err != nil { + t.Fatalf("canceled dispatch stranded unknown fence: %v", err) + } + }) +} + +func claimAndRelease(t *testing.T, m *Manager, volumes ...types.Volume) error { + t.Helper() + sb, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: volumes}) + if err != nil { + return err + } + return m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}) +} + +// dispatchBlocked moves the removal queue into workers parked on a full maintenance semaphore. +func dispatchBlocked(ctx context.Context, t *testing.T, m *Manager) *sync.WaitGroup { + t.Helper() + for range cap(m.maintSem) { + m.maintSem <- struct{}{} + } + wg := m.retryRemovals(ctx) + m.mu.Lock() + defer m.mu.Unlock() + if len(m.pendingRemovals) != 0 { + t.Fatal("cleanup was not dispatched") + } + return wg +} + +func unblock(m *Manager, wg *sync.WaitGroup) { + for range cap(m.maintSem) { + <-m.maintSem + } + wg.Wait() +} diff --git a/sandboxd/pool/volume_convergence_test.go b/sandboxd/pool/volume_convergence_test.go index 7c002ecc..8ca48522 100644 --- a/sandboxd/pool/volume_convergence_test.go +++ b/sandboxd/pool/volume_convergence_test.go @@ -22,7 +22,7 @@ func TestSettledVolumesDoNotCallDevicesOrResolveCatalog(t *testing.T) { if _, err := m.SetVolumes(t.Context(), sb.ID, sb.Tenant, []types.Volume{{Name: "data"}}); err != nil { t.Fatal(err) } - delete(m.volumes, "data") + delete(m.catalog.entries, "data") before := len(eng.volumeSpecs) if _, err := m.SetVolumes(t.Context(), sb.ID, sb.Tenant, []types.Volume{{Name: "data"}, {Name: "other"}}); err != nil { t.Fatal(err) @@ -57,9 +57,9 @@ func TestRemovedTenantRetainsAndRepairsHeldVolumes(t *testing.T) { t.Fatal("expected detaching state") } eng.detachVolumeErr = nil - entry := m.volumes["data"] + entry := m.catalog.entries["data"] entry.tenants = []string{"other-tenant"} - m.volumes["data"] = entry + m.catalog.entries["data"] = entry if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data"}}); err != nil { t.Fatalf("held repair checked new authorization: %v", err) } @@ -288,7 +288,7 @@ func TestUnsupportedRuntimeDoesNotChangeVolumeIntent(t *testing.T) { eng := newFakeEngine() m := newVolumeManager(t, eng, liveCatalog(t)) sb := mustClaim(t, m, testKey) - m.eng = &unsupportedVolumeEngine{eng} + eng.liveVolumeSupportErr = &engine.LiveVolumeUnsupported{} before, err := os.ReadFile(m.store.path) if err != nil { t.Fatal(err) @@ -330,12 +330,9 @@ func TestVolumeHoldsRejectCaptureBeforeTrim(t *testing.T) { t.Fatal(err) } m.mu.Lock() - err = m.reserveVolumes(held) + m.adoptVolumes(held) run.holds = held m.mu.Unlock() - if err != nil { - t.Fatal(err) - } defer m.finishVolumeRun(sb, run) } if _, err := m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "blocked", ""); !errors.Is(err, ErrVolumeCapture) { @@ -424,12 +421,6 @@ func TestUncertainTargetCommitReleasesOnlyConfirmedPrecleanedHolds(t *testing.T) } } -type unsupportedVolumeEngine struct{ *fakeEngine } - -func (e *unsupportedVolumeEngine) CheckLiveVolumeSupport(context.Context) error { - return &engine.LiveVolumeUnsupported{} -} - func unmountConflict() *engine.VolumeMountConflict { return &engine.VolumeMountConflict{Message: `cannot unmount "/volumes/data"; close guest references and retry`} } diff --git a/sandboxd/pool/volume_mutation.go b/sandboxd/pool/volume_mutation.go index 33dc50e2..222fb6cc 100644 --- a/sandboxd/pool/volume_mutation.go +++ b/sandboxd/pool/volume_mutation.go @@ -19,9 +19,10 @@ const ( type volumeEdit func([]types.Volume) ([]types.Volume, error) type volumeRun struct { - cancel context.CancelFunc - done chan struct{} - holds []types.Volume + cancel context.CancelFunc + done chan struct{} + holds []types.Volume + teardown []types.Volume } // GetVolumes reads a claim's desired and applied volumes; a tenant reaches only its own claims. @@ -40,13 +41,13 @@ func (m *Manager) SetVolumes(ctx context.Context, id, tenant string, want []type return m.changeVolumes(ctx, id, tenant, func([]types.Volume) ([]types.Volume, error) { return slices.Clone(want), nil }) } -// PatchVolumes adds and removes a running claim's read-only mounted volumes by name. +// PatchVolumes adds and removes a running claim's volumes by name. func (m *Manager) PatchVolumes(ctx context.Context, id, tenant string, patch types.VolumePatch) (types.SandboxVolumesResponse, error) { return m.changeVolumes(ctx, id, tenant, func(current []types.Volume) ([]types.Volume, error) { if err := validateDetachNames(patch.Remove); err != nil { return nil, err } - additions, err := normalizeLiveVolumes(patch.Add, current) + additions, err := normalizeLiveVolumes(patch.Add) if err != nil { return nil, err } @@ -86,20 +87,21 @@ func (m *Manager) changeVolumes(ctx context.Context, id, tenant string, change v }() m.mu.Lock() current, held := slices.Clone(sb.VolumesWant), heldVolumes(sb) + recovery := m.store.recoveryVolumesFor(sb.ID) m.mu.Unlock() target, err := change(current) if err != nil { return view, err } - target, err = normalizeLiveVolumes(target, held) + target, err = normalizeLiveVolumes(target) if err != nil { return view, err } - if err = validateLiveTarget(held, target); err != nil { + if err = validateLiveTarget(slices.Concat(held, recovery), target); err != nil { return view, err } additions := volumeDifference(target, held) - if len(additions) != 0 { + if len(additions) > 0 { if err = m.tenantRemoved(ctx, sb.Tenant); err != nil { return view, err } @@ -108,7 +110,13 @@ func (m *Manager) changeVolumes(ctx context.Context, id, tenant string, change v if err != nil { return view, err } - if err = m.checkVolumeRun(ctx, sb, run); err != nil { + m.mu.Lock() + err = m.checkVolumeRunLocked(ctx, sb, run) + if err == nil && slices.ContainsFunc(additions, types.Volume.RW) { + err = m.liveWriterErrorLocked(sb) + } + m.mu.Unlock() + if err != nil { return view, err } sb.Touch() @@ -139,14 +147,14 @@ func (m *Manager) changeVolumes(ctx context.Context, id, tenant string, change v m.clearVolumeError(sb, run, v.Name) } m.mu.Lock() - pending := volumeDifference(sb.VolumesWant, sb.Volumes) + pending, catalog := volumeDifference(sb.VolumesWant, sb.Volumes), m.catalog.entries m.mu.Unlock() for _, v := range pending { failedName = v.Name if err = m.checkVolumeRun(ctx, sb, run); err != nil { return view, err } - repair, resolveErr := m.resolveCatalogVolume(ctx, v) + repair, resolveErr := m.resolveCatalogEntry(ctx, v, catalog[v.Name]) if resolveErr != nil { return view, resolveErr } @@ -200,6 +208,7 @@ func (m *Manager) finishVolumeRun(sb *types.Sandbox, run *volumeRun) types.Sandb if m.claimed[sb.ID] == sb { m.releaseVolumes(volumeDifference(run.holds, heldVolumes(sb))) } + m.releaseVolumes(run.teardown) delete(m.volumeRuns, sb) view := m.volumeView(sb) close(run.done) @@ -258,21 +267,29 @@ func (m *Manager) prepareVolumeTarget(ctx context.Context, sb *types.Sandbox, ru func (m *Manager) stageVolumeTarget(ctx context.Context, sb *types.Sandbox, run *volumeRun, target, additions []types.Volume, resolved []resolvedVolume) (claimSnapshot, bool, error) { sb.Transition.Lock() defer sb.Transition.Unlock() - m.mu.Lock() - if err := m.checkVolumeRunLocked(ctx, sb, run); err != nil { - m.mu.Unlock() - return claimSnapshot{}, false, err - } - if sb.OnExpire == types.ExpireArchive && len(target) > 0 { - m.mu.Unlock() - return claimSnapshot{}, false, ErrVolumeCapture - } - if err := m.reserveVolumes(additions); err != nil { - m.mu.Unlock() + if err := func() error { + m.mu.Lock() + defer m.mu.Unlock() + if err := m.checkVolumeRunLocked(ctx, sb, run); err != nil { + return err + } + if sb.OnExpire == types.ExpireArchive && len(target) > 0 { + return ErrVolumeCapture + } + if slices.ContainsFunc(additions, types.Volume.RW) { + if err := m.liveWriterErrorLocked(sb); err != nil { + return err + } + } + if err := m.checkResolved(resolved, sb.ID); err != nil { + return err + } + m.adoptVolumes(additions) + run.holds = append(run.holds, additions...) + return nil + }(); err != nil { return claimSnapshot{}, false, err } - run.holds = append(run.holds, additions...) - m.mu.Unlock() cleanErr := m.confirmVolumesClean(resolved) m.mu.Lock() defer m.mu.Unlock() @@ -295,14 +312,40 @@ func (m *Manager) stageVolumeTarget(ctx context.Context, sb *types.Sandbox, run return m.store.set(sb), true, nil } +func (m *Manager) liveWriterErrorLocked(sb *types.Sandbox) error { + if m.view.Load().poolReplicate[sb.PolicyKey()] > 0 { + return ErrReplicatingPool + } + if m.archives == nil && (sb.HasReplica() || sb.ArchivePending != "") { + return ErrReplicaStranded + } + if sb.HasReplica() || sb.ArchivePending != "" || m.store.hasCheckpointReferences(sb.ID) { + return ErrReplicaPresent + } + return nil +} + func (m *Manager) detachVolumeRun(ctx context.Context, sb *types.Sandbox, run *volumeRun, v types.Volume) error { - if err := m.checkVolumeRun(ctx, sb, run); err != nil { + m.mu.Lock() + err := m.checkVolumeRunLocked(ctx, sb, run) + entry := m.catalog.entries[v.Name] + m.mu.Unlock() + if err != nil { return err } - if err := m.eng.DetachVolume(ctx, sb.VMName, sb.VsockSocket, v.Name, v.Mount); err != nil { + cleanRW, err := m.eng.DetachVolume(ctx, sb.VMName, sb.VsockSocket, v.Name, v.Mount) + if err != nil { return fmt.Errorf("detach volume %q: %w", v.Name, err) } - return m.checkVolumeRun(ctx, sb, run) + if err = m.checkVolumeRun(ctx, sb, run); err != nil { + return err + } + if needsUnmount(v) && cleanRW && entry != nil { + if err = clearVolumeDirty(entry.disk.Path); err != nil { + return fmt.Errorf("clear dirty marker for volume %q: %w", v.Name, err) + } + } + return nil } func (m *Manager) commitVolumeApplied(ctx context.Context, sb *types.Sandbox, run *volumeRun, v types.Volume, attached bool) error { @@ -343,19 +386,26 @@ func (m *Manager) ownsRunLocked(sb *types.Sandbox, run *volumeRun) bool { return m.claimed[sb.ID] == sb && m.volumeRuns[sb] == run } -func (m *Manager) hasVolumeHolds(sb *types.Sandbox) bool { +func (m *Manager) hasLiveVolumeHolds(sb *types.Sandbox) bool { run := m.volumeRuns[sb] return hasVolumes(sb) || run != nil && len(run.holds) > 0 } +func (m *Manager) hasVolumeHolds(sb *types.Sandbox) bool { + return m.hasLiveVolumeHolds(sb) || m.store.hasRecoveryVolumes(sb.ID) +} + func (m *Manager) volumeView(sb *types.Sandbox) types.SandboxVolumesResponse { + held := heldVolumes(sb) + recovering := volumeDifference(m.store.recoveryVolumesFor(sb.ID), held) view := types.SandboxVolumesResponse{ Volumes: slices.Clone(sb.Volumes), VolumesWant: slices.Clone(sb.VolumesWant), States: []types.VolumeState{}, - Converging: m.volumeRuns[sb] != nil, + Converging: m.volumeRuns[sb] != nil || len(recovering) > 0, } - for _, v := range heldVolumes(sb) { + held = append(held, recovering...) + for _, v := range held { state := types.VolumeAttached if volumeIndex(sb.VolumesWant, v.Name) < 0 { state = types.VolumeDetaching @@ -412,30 +462,9 @@ func (m *Manager) clearVolumeError(sb *types.Sandbox, run *volumeRun, name strin } } -func normalizeLiveVolumes(volumes, held []types.Volume) ([]types.Volume, error) { - if len(volumes) > types.MaxClaimVolumes { - return nil, fmt.Errorf("%w: too many volumes", ErrBadVolume) - } - out := make([]types.Volume, 0, len(volumes)) - for _, v := range volumes { - attachOnly := v.AttachOnly - if i := volumeIndex(held, v.Name); i >= 0 && held[i].Mount == "" && v.Mount == "" { - attachOnly = true - } - normalized, err := types.ValidateVolumes([]types.Volume{v}, attachOnly) - if err != nil { - return nil, fmt.Errorf("%w: %w", ErrBadVolume, err) - } - if volumeIndex(out, v.Name) >= 0 { - return nil, fmt.Errorf("%w: duplicate volume %q", ErrBadVolume, v.Name) - } - if attachOnly { - normalized[0].AttachOnly = false - } - out = append(out, normalized[0]) - } - mounted := slices.DeleteFunc(slices.Clone(out), func(v types.Volume) bool { return v.Mount == "" }) - if _, err := types.ValidateVolumes(mounted, false); err != nil { +func normalizeLiveVolumes(volumes []types.Volume) ([]types.Volume, error) { + out, err := types.ValidateVolumes(volumes) + if err != nil { return nil, fmt.Errorf("%w: %w", ErrBadVolume, err) } return out, nil @@ -447,18 +476,10 @@ func validateLiveTarget(held, target []types.Volume) error { if i >= 0 && target[i] != v { return fmt.Errorf("%w: remove volume %q before changing its mount or mode", ErrBadVolume, v.Name) } - if immutableVolume(v) && i < 0 { - return fmt.Errorf("%w: writable and attach-only volumes cannot change live", ErrBadVolume) - } - } - if slices.ContainsFunc(volumeDifference(target, held), immutableVolume) { - return fmt.Errorf("%w: live additions require read-only mounts", ErrBadVolume) } return nil } -func immutableVolume(v types.Volume) bool { return v.RW() || v.AttachOnly || v.Mount == "" } - func volumeIndex(volumes []types.Volume, name string) int { return slices.IndexFunc(volumes, func(v types.Volume) bool { return v.Name == name }) } diff --git a/sandboxd/pool/volume_mutation_test.go b/sandboxd/pool/volume_mutation_test.go index 4f94b749..22a2b9e0 100644 --- a/sandboxd/pool/volume_mutation_test.go +++ b/sandboxd/pool/volume_mutation_test.go @@ -18,6 +18,75 @@ import ( "github.com/cocoonstack/sandbox/sandboxd/types" ) +func TestLiveVolumeFourModesAndMixedRoundtrip(t *testing.T) { + eng := newFakeEngine() + catalog := []config.VolumeSpec{} + request := []types.Volume{{Name: "mounted"}, {Name: "writer", Mode: types.VolumeModeRW}, {Name: "raw", AttachOnly: true}, {Name: "raw-writer", Mode: types.VolumeModeRW, AttachOnly: true}} + for _, v := range request { + catalog = append(catalog, config.VolumeSpec{Name: v.Name, Path: writeVolumeImage(t, v.Name, "seed"), Writable: true}) + } + m := newVolumeManager(t, eng, catalog) + sb := mustClaim(t, m, testKey) + view, err := m.PatchVolumes(t.Context(), sb.ID, "", types.VolumePatch{Add: request}) + if err != nil { + t.Fatal(err) + } + if len(view.Volumes) != 4 || len(eng.volumeMounts) != 2 { + t.Fatalf("mixed modes: view=%+v mounts=%+v", view, eng.volumeMounts) + } + for _, v := range view.Volumes { + if v.AttachOnly != (v.Mount == "") { + t.Fatalf("lost canonical shape: %+v", v) + } + } + before := slices.Clone(eng.volumeOpsLog()) + if _, err = m.SetVolumes(t.Context(), sb.ID, "", view.VolumesWant); err != nil { + t.Fatal(err) + } + if !slices.Equal(before, eng.volumeOpsLog()) { + t.Fatal("GET-to-PUT roundtrip changed devices") + } + for _, change := range []types.Volume{{Name: "raw", Mount: "/new"}, {Name: "raw-writer", AttachOnly: true}, {Name: "writer", Mount: "/other", Mode: types.VolumeModeRW}} { + if _, err = m.PatchVolumes(t.Context(), sb.ID, "", types.VolumePatch{Add: []types.Volume{change}}); !errors.Is(err, ErrBadVolume) { + t.Fatalf("held shape change accepted: %+v, %v", change, err) + } + } + legacy := slices.Clone(view.VolumesWant) + for i := range legacy { + legacy[i].AttachOnly = false + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", legacy); !errors.Is(err, ErrBadVolume) { + t.Fatalf("explicit false inherited attach-only: %v", err) + } + assertLiveJournal(t, m, sb) + raw, err := os.ReadFile(m.store.path) + if err != nil { + t.Fatal(err) + } + if err = os.WriteFile(m.store.path, []byte(strings.ReplaceAll(string(raw), `,"attach_only":true`, "")), 0o600); err != nil { + t.Fatal(err) + } + restarted := newVolumeManagerAt(t, eng, m.dataDir, catalog) + if err = restarted.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + recovered, err := restarted.GetVolumes(sb.ID, "") + if err != nil || !slices.Equal(recovered.Volumes, view.Volumes) { + t.Fatalf("mixed restart: %+v, %v", recovered, err) + } + if _, err = restarted.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "raw-writer", AttachOnly: true}}}); !errors.Is(err, ErrVolumeBusy) { + t.Fatalf("restart lost writer exclusion: %v", err) + } + if _, err = restarted.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err != nil { + t.Fatal(err) + } + for _, spec := range catalog { + if volumeDirty(spec.Path) || volumeHoldersOf(restarted, spec.Name) != (volumeHolders{}) { + t.Fatalf("detach leaked marker or hold: %s", spec.Name) + } + } +} + func TestLiveVolumesCommitAndRetry(t *testing.T) { eng := newFakeEngine() m := newVolumeManager(t, eng, liveCatalog(t)) @@ -68,8 +137,8 @@ func TestLiveAttachPreservesWritableAndAttachOnlyVolumes(t *testing.T) { if _, err := m.SetVolumes(t.Context(), sb.ID, sb.Tenant, []types.Volume{{Name: "other", Mount: original.Mount}}); !errors.Is(err, ErrBadVolume) { t.Fatalf("accepted mode change: %v", err) } - if _, err := m.PatchVolumes(t.Context(), sb.ID, sb.Tenant, types.VolumePatch{Remove: []string{"data", "other"}}); !errors.Is(err, ErrBadVolume) || len(sb.Volumes) != 2 { - t.Fatalf("writable detach changed the batch: %v", err) + if _, err := m.PatchVolumes(t.Context(), sb.ID, sb.Tenant, types.VolumePatch{Remove: []string{"data", "other"}}); err != nil || len(sb.Volumes) != 0 { + t.Fatalf("writable detach failed: %v", err) } }) } @@ -80,8 +149,8 @@ func TestLiveVolumeValidationPrecedesDeviceCalls(t *testing.T) { m := newVolumeManager(t, eng, liveCatalog(t)) sb := mustClaim(t, m, testKey) for _, request := range [][]types.Volume{ - {{Name: "data", Mode: "rw"}}, - {{Name: "data", AttachOnly: true}}, + {{Name: "data", Mode: "unknown"}}, + {{Name: "data", AttachOnly: true, Mount: "/data"}}, {{Name: "data", Mount: "/etc"}}, {{Name: "data"}, {Name: "data"}}, {{Name: "data", Mount: "/a"}, {Name: "other", Mount: "/a/b"}}, @@ -226,7 +295,7 @@ func TestLiveVolumeDoesNotBlockLifecycle(t *testing.T) { eng.liveVolumeHook = func(string) error { close(entered); <-resume; return nil } attached := make(chan error, 1) go func() { - _, err := m.SetVolumes(t.Context(), sb.ID, sb.Tenant, []types.Volume{{Name: "data"}}) + _, err := m.SetVolumes(t.Context(), sb.ID, sb.Tenant, []types.Volume{{Name: "data", Mode: types.VolumeModeRW, AttachOnly: true}}) attached <- err }() <-entered @@ -307,7 +376,7 @@ func TestLiveVolumeCanceledRequestRetainsIntent(t *testing.T) { sb := mustClaim(t, m, testKey) ctx, cancel := context.WithCancel(t.Context()) eng.liveVolumeHook = func(string) error { cancel(); return nil } - _, err := m.SetVolumes(ctx, sb.ID, sb.Tenant, []types.Volume{{Name: "data"}}) + _, err := m.SetVolumes(ctx, sb.ID, sb.Tenant, []types.Volume{{Name: "data", Mode: types.VolumeModeRW, AttachOnly: true}}) if !errors.Is(err, context.Canceled) || slices.Equal(sb.VolumesWant, sb.Volumes) { t.Fatalf("canceled attach: %v, pending=%v", err, sb.VolumesWant) } @@ -384,6 +453,7 @@ func TestDesiredVolumesJournalMigration(t *testing.T) { want []types.Volume }{ {"legacy", "", []types.Volume{{Name: "data", Mount: "/data"}}}, + {"legacy attach-only desired", `,"volumes_want":[{"name":"raw","mode":"rw"}]`, []types.Volume{{Name: "raw", Mode: types.VolumeModeRW, AttachOnly: true}}}, {"explicit empty", `,"volumes_want":[]`, nil}, {"desired", `,"volumes_want":[{"name":"other","mount":"/other"}]`, []types.Volume{{Name: "other", Mount: "/other"}}}, } { @@ -489,7 +559,10 @@ func TestCanceledVolumeWriterOwnsHardwareUntilReturn(t *testing.T) { m.eng = eng ctx, cancel := context.WithCancel(t.Context()) first := make(chan error, 1) - go func() { _, err := m.SetVolumes(ctx, sb.ID, sb.Tenant, []types.Volume{{Name: "data"}}); first <- err }() + go func() { + _, err := m.SetVolumes(ctx, sb.ID, sb.Tenant, []types.Volume{{Name: "data", Mode: types.VolumeModeRW, AttachOnly: true}}) + first <- err + }() <-eng.entered cancel() waitingCtx, stopWaiting := context.WithTimeout(t.Context(), 25*time.Millisecond) @@ -853,9 +926,7 @@ func beginAdmittedVolumeRun(t *testing.T, m *Manager, sb *types.Sandbox) *volume m.mu.Lock() defer m.mu.Unlock() transient := []types.Volume{{Name: "data", Mount: "/volumes/data"}} - if err = m.reserveVolumes(transient); err != nil { - t.Fatal(err) - } + m.adoptVolumes(transient) run.holds = append(run.holds, transient...) return run } diff --git a/sandboxd/pool/volume_replica_test.go b/sandboxd/pool/volume_replica_test.go new file mode 100644 index 00000000..0261f925 --- /dev/null +++ b/sandboxd/pool/volume_replica_test.go @@ -0,0 +1,223 @@ +package pool + +import ( + "errors" + "slices" + "sync" + "testing" + "time" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestReplicaRecoveryDoesNotRepublishReadOnlyHold(t *testing.T) { + m, c := newReplicaVolumeManager(t) + sb := mustClaim(t, m, testKey) + replicateNow(t, m, sb) + if view, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data"}}); err != nil || len(view.Volumes) != 1 { + t.Fatalf("read-only live attachment: %+v, %v", view, err) + } + if _, err := c.reg.Consume(t.Context(), sb.ID, sb.ArchiveEpoch); err != nil { + t.Fatal(err) + } + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil || !ok || !rec.Consumed { + t.Fatalf("setup: %+v, %t, %v", rec, ok, err) + } + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if rec, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok || !rec.Consumed { + t.Fatalf("recovery republished read-only hold: %+v, %t, %v", rec, ok, err) + } +} + +func TestReplicaPublicationRefusesVolumeAdmittedDuringExport(t *testing.T) { + m, c := newReplicaVolumeManager(t) + disableReplication(m) + sb := mustClaim(t, m, testKey) + stalled := &stallingStore{Store: m.ckpts, published: make(chan string), release: make(chan struct{})} + m.ckpts = stalled + result := make(chan error, 1) + done := make(chan struct{}) + go func() { defer close(done); result <- m.replicate(t.Context(), sb) }() + ck := <-stalled.published + release := sync.OnceFunc(func() { close(stalled.release) }) + t.Cleanup(func() { release(); <-done }) + writer := []types.Volume{{Name: "data", Mode: types.VolumeModeRW, AttachOnly: true}} + view, err := m.SetVolumes(t.Context(), sb.ID, "", writer) + if err != nil || len(view.Volumes) != 1 { + t.Fatalf("writer during checkpoint publication: %+v, %v", view, err) + } + release() + if err = <-result; !errors.Is(err, ErrVolumeCapture) { + t.Fatalf("stale replica registered after writer admission: %v", err) + } + if _, ok, getErr := c.reg.Get(t.Context(), sb.ID); getErr != nil || ok || sb.HasReplica() || ckExists(t, m, ck) { + t.Fatalf("stale replica remains recoverable: row=%t, claim=%t, checkpoint=%t, err=%v", ok, sb.HasReplica(), ckExists(t, m, ck), getErr) + } + if got := volumeHoldersOf(m, "data"); got.writers != 1 { + t.Fatalf("refusing replica lost writer hold: %+v", got) + } + if err = m.replicate(t.Context(), sb); !errors.Is(err, ErrVolumeCapture) { + t.Fatalf("snapshot admission while writer is held: %v", err) + } +} + +func TestReplicaCaptureRefusesPendingLiveWriter(t *testing.T) { + m, c := newReplicaVolumeManager(t) + disableReplication(m) + sb := mustClaim(t, m, testKey) + eng := m.eng.(*fakeEngine) + entered, resume := make(chan struct{}), make(chan struct{}) + release := sync.OnceFunc(func() { close(resume) }) + eng.liveVolumeHook = func(verb string) error { + if verb == "attach" { + close(entered) + <-resume + } + return nil + } + result := make(chan error, 1) + done := make(chan struct{}) + go func() { + defer close(done) + _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}) + result <- err + }() + t.Cleanup(func() { release(); <-done }) + <-entered + view, err := m.GetVolumes(sb.ID, "") + if err != nil || len(view.Volumes) != 0 || len(view.VolumesWant) != 1 || !view.Converging { + t.Fatalf("writer did not reach pending attachment: %+v, %v", view, err) + } + if err = m.replicate(t.Context(), sb); !errors.Is(err, ErrVolumeCapture) { + t.Fatalf("captured pending writer: %v", err) + } + if _, ok, getErr := c.reg.Get(t.Context(), sb.ID); getErr != nil || ok || len(eng.snapSaves) != 0 { + t.Fatalf("pending writer produced snapshot or row: snapshots=%v, row=%t, err=%v", eng.snapSaves, ok, getErr) + } + release() + if err = <-result; err != nil { + t.Fatalf("pending writer did not complete: %v", err) + } +} + +func TestLiveWriterPolicyAndCheckpointRefusals(t *testing.T) { + for _, tc := range []struct { + name string + prepare func(*Manager, *types.Sandbox) + want error + }{ + {"replicating pool", func(*Manager, *types.Sandbox) {}, ErrReplicatingPool}, + {"source pool replicates", func(_ *Manager, sb *types.Sandbox) { + sb.PolicySource = testKey + sb.Key.Template = "promoted:template" + }, ErrReplicatingPool}, + {"source pool does not replicate", func(_ *Manager, sb *types.Sandbox) { + sb.PolicySource = testKey + sb.PolicySource.Template = "other:template" + }, nil}, + {"published replica", func(m *Manager, sb *types.Sandbox) { disableReplication(m); replicateNow(t, m, sb) }, ErrReplicaPresent}, + {"pending replica", func(m *Manager, sb *types.Sandbox) { + disableReplication(m) + sb.ReplicaPending = "pending" + m.store.set(sb) + }, ErrReplicaPresent}, + {"registry disabled", func(m *Manager, sb *types.Sandbox) { disableReplication(m); replicateNow(t, m, sb); m.archives = nil }, ErrReplicaStranded}, + {"archive pending without registry", func(m *Manager, sb *types.Sandbox) { + disableReplication(m) + sb.ArchivePending = "pending" + m.archives = nil + }, ErrReplicaStranded}, + {"retained only without registry", func(m *Manager, sb *types.Sandbox) { + disableReplication(m) + sb.ReplicaCk = "retained" + if err := m.store.commit(m.store.set(sb)); err != nil { + t.Fatal(err) + } + sb.ReplicaCk = "" + m.store.set(sb) + m.archives = nil + }, ErrReplicaPresent}, + {"unrelated checkpoint", func(m *Manager, _ *types.Sandbox) { + disableReplication(m) + other := mustClaim(t, m, testKey) + other.ReplicaCk = "unrelated" + m.store.set(other) + }, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + m, _ := newReplicaVolumeManager(t) + sb := mustClaim(t, m, testKey) + tc.prepare(m, sb) + before, ops := m.store.mark(), m.eng.(*fakeEngine).volumeOpsLog() + view, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW, AttachOnly: true}}) + if !errors.Is(err, tc.want) { + t.Fatalf("writer refusal = %v, want %v", err, tc.want) + } + if tc.want != nil && (len(view.VolumesWant) != 0 || len(view.Volumes) != 0 || view.Converging || m.store.mark() != before || volumeHoldersOf(m, "data") != (volumeHolders{}) || !slices.Equal(ops, m.eng.(*fakeEngine).volumeOpsLog())) { + t.Fatalf("refusal changed claim or device: %+v", view) + } + }) + } +} + +func TestInitialWriterAndRetiredReplicaAllowLiveWork(t *testing.T) { + m, _ := newReplicaVolumeManager(t) + sb, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}}) + if err != nil { + t.Fatal(err) + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", sb.Volumes); err != nil { + t.Fatalf("held writer retry: %v", err) + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", nil); err != nil { + t.Fatalf("writer cleanup: %v", err) + } + replicateNow(t, m, sb) + disableReplication(m) + m.store.recommitting.Store(true) + restore := failClaimWrites(t, m.store) + m.retireReplica(t.Context(), sb) + writer := []types.Volume{{Name: "data", Mode: types.VolumeModeRW}} + if sb.HasReplica() || !m.store.hasCheckpointReferences(sb.ID) { + t.Fatal("failed clear lost checkpoint uncertainty") + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", writer); !errors.Is(err, ErrReplicaPresent) { + t.Fatalf("unacknowledged retirement: %v", err) + } + restore() + if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", writer); err != nil { + t.Fatalf("acknowledged retirement: %v", err) + } +} + +func newReplicaVolumeManager(t *testing.T) (*Manager, *failoverCluster) { + t.Helper() + c := newReplicaCluster(t) + spec := archivePool(keepAnHour) + spec.ReplicateSeconds = c.replicate + cfg := &config.Config{ + APIToken: "root", DataDir: t.TempDir(), CheckpointDir: c.ckDir, + Pools: []config.PoolSpec{spec}, Mesh: &config.MeshConfig{NodeID: "a"}, + Volumes: []config.VolumeSpec{{Name: "data", Path: writeVolumeImage(t, "data", "original"), Writable: true}}, + } + backends := testBackends(t, cfg) + backends.Archives = c.reg + m, err := NewManager(t.Context(), cfg, newFakeEngine(), testSecrets(t), backends) + if err != nil { + t.Fatal(err) + } + c.renew(t, "a") + m.liftFence() + return m, c +} + +func disableReplication(m *Manager) { + view := *m.view.Load() + view.poolReplicate = nil + m.view.Store(&view) +} diff --git a/sandboxd/pool/volume_retirement_test.go b/sandboxd/pool/volume_retirement_test.go new file mode 100644 index 00000000..c6a224bc --- /dev/null +++ b/sandboxd/pool/volume_retirement_test.go @@ -0,0 +1,345 @@ +package pool + +import ( + "encoding/json/v2" + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/types" + "github.com/cocoonstack/sandbox/sandboxd/utils" +) + +func TestUnacknowledgedDetachKeepsRecoveryReference(t *testing.T) { + for _, volume := range []types.Volume{{Name: "home", Mount: "/home"}, {Name: "home", Mount: "/home", Mode: types.VolumeModeRW}, {Name: "home", AttachOnly: true}, {Name: "home", Mode: types.VolumeModeRW, AttachOnly: true}} { + t.Run(fmt.Sprintf("rw=%t/raw=%t", volume.RW(), volume.AttachOnly), func(t *testing.T) { + expectedHolds := volumeHolders{readers: 1} + if volume.RW() { + expectedHolds = volumeHolders{writers: 1} + } + + eng := newFakeEngine() + m, sb := newTenantVolumeManager(t, eng, liveCatalog(t)) + spec := types.VolumeRegistration{Path: writeVolumeImage(t, "home.img", "private"), Writable: true, Tenants: []string{"acme"}} + if err := m.RegisterVolume(t.Context(), "home", spec); err != nil { + t.Fatal(err) + } + want := []types.Volume{volume} + for _, caller := range []string{"", "acme"} { + if _, err := m.SetVolumes(t.Context(), sb.ID, caller, want); err != nil { + t.Fatal(err) + } + } + unblock := detachWithoutCommit(t, m, eng, sb.ID, "acme") + claims, err := newClaimStore(m.dataDir, false).load() + if err != nil || claims[sb.ID] == nil || !slices.Equal(claims[sb.ID].Volumes, want) { + t.Fatalf("old durable attachment missing: %+v, %v", claims, err) + } + view, err := m.GetVolumes(sb.ID, "") + if err != nil || len(view.States) != 1 || view.States[0].State != types.VolumeDetaching || !view.Converging { + t.Fatalf("unretired view: %+v, %v", view, err) + } + writer := ClaimOptions{Volumes: []types.Volume{{Name: "home", Mode: types.VolumeModeRW}}} + for _, tc := range []struct { + name string + err error + want error + }{ + {"writer", errOf(m.ClaimProvision(t.Context(), testKey, writer)), ErrVolumeBusy}, + {"checkpoint", errOf(m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "unretired", "")), ErrVolumeCapture}, + {"hibernate", m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}), ErrVolumeCapture}, + {"unregister", m.UnregisterVolume(t.Context(), "home"), ErrVolumeRegistrationConflict}, + {"rebind", m.RegisterVolume(t.Context(), "home", types.VolumeRegistration{Path: writeVolumeImage(t, "different", "bytes")}), ErrVolumeRegistrationConflict}, + {"alias", m.RegisterVolume(t.Context(), "same-file", spec), ErrVolumeRegistrationConflict}, + {"reshape", errOf(m.SetVolumes(t.Context(), sb.ID, "acme", []types.Volume{{Name: "home", Mount: "/different"}})), ErrBadVolume}, + } { + if !errors.Is(tc.err, tc.want) { + t.Errorf("%s with unretired reference = %v, want %v", tc.name, tc.err, tc.want) + } + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "acme", nil); err == nil { + t.Fatal("empty retry acknowledged an unretired file") + } + unblock() + setVolumeTenants(m, "home", "other") + before := len(eng.volumeSpecs) + _, putErr := m.SetVolumes(t.Context(), sb.ID, "acme", want) + _, patchErr := m.PatchVolumes(t.Context(), sb.ID, "acme", types.VolumePatch{Add: want}) + if !errors.Is(putErr, ErrVolumeUnavailable) || !errors.Is(patchErr, ErrVolumeUnavailable) || len(eng.volumeSpecs) != before { + t.Fatalf("recovery-only reference authorized a fresh attachment: put=%v patch=%v", putErr, patchErr) + } + setVolumeTenants(m, "home", "acme") + for range 2 { + if _, err = m.SetVolumes(t.Context(), sb.ID, "acme", want); err != nil { + t.Fatal(err) + } + if got := volumeHoldersOf(m, "home"); got != expectedHolds { + t.Fatalf("own retry counts: %+v", got) + } + } + detachWithoutCommit(t, m, eng, sb.ID, "acme")() + if err = m.DeleteTenant(t.Context(), "acme"); err != nil { + t.Fatal(err) + } + _, putErr = m.SetVolumes(t.Context(), sb.ID, "", want) + _, patchErr = m.PatchVolumes(t.Context(), sb.ID, "", types.VolumePatch{Add: []types.Volume{{Name: "data"}}}) + if !errors.Is(putErr, ErrTenantRemoved) || !errors.Is(patchErr, ErrTenantRemoved) || volumeHoldersOf(m, "home") != (volumeHolders{}) { + t.Fatalf("recovery-only retry bypassed tenant removal: put=%v patch=%v", putErr, patchErr) + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", nil); err != nil { + t.Fatalf("retirement after tenant removal: %v", err) + } + if err = claimAndRelease(t, m, writer.Volumes...); err != nil { + t.Fatalf("acknowledged detach kept the reader exclusion: %v", err) + } + if err = errors.Join(m.UnregisterVolume(t.Context(), "home"), m.RegisterVolume(t.Context(), "same-file", spec)); err != nil { + t.Fatalf("retired name or path stayed reserved: %v", err) + } + }) + } +} + +func TestPossibleClaimIntroductionRemainsProtected(t *testing.T) { + eng := newFakeEngine() + catalog := liveCatalog(t) + m := newVolumeManager(t, eng, catalog) + sb := mustClaim(t, m, testKey) + claims, err := newClaimStore(m.dataDir, false).load() + if err != nil { + t.Fatal(err) + } + restoreWrites := failClaimWrites(t, m.store) + if _, err = m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data"}}); err == nil { + t.Fatal("expected failed intent write") + } + claims[sb.ID].VolumesWant = []types.Volume{{Name: "data", Mount: types.DefaultVolumeMount("data"), Mode: types.VolumeModeRO}} + raw, err := json.Marshal(claims) + if err != nil { + t.Fatal(err) + } + // model an intent whose rename succeeded before an uncertain directory sync + if err = utils.WriteFileSync(m.store.path, raw, 0o600); err != nil { + t.Fatal(err) + } + if _, err = m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err == nil { + t.Fatal("expected empty snapshot error") + } + if len(sb.Volumes) != 0 || len(sb.VolumesWant) != 0 || volumeHoldersOf(m, "data") != (volumeHolders{}) { + t.Fatalf("memory not empty: %+v", sb) + } + writer := ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}} + if _, err = m.ClaimProvision(t.Context(), testKey, writer); !errors.Is(err, ErrVolumeBusy) { + t.Fatalf("writer admitted after uncertain introduction: %v", err) + } + if _, err = m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "uncertain-introduction", ""); !errors.Is(err, ErrVolumeCapture) { + t.Fatalf("capture admitted: %v", err) + } + restoreWrites() + m2 := newVolumeManagerAt(t, eng, m.dataDir, catalog) + if err = m2.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + if _, err = m2.ClaimProvision(t.Context(), testKey, writer); !errors.Is(err, ErrVolumeBusy) { + t.Fatalf("restart admitted writer: %v", err) + } + if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + if _, err = m.ClaimProvision(t.Context(), testKey, writer); err != nil { + t.Fatalf("successful whole snapshot did not retire attempted row: %v", err) + } + if _, err = m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "retired-introduction", ""); err != nil { + t.Fatalf("capture after acknowledgment: %v", err) + } +} + +func TestClaimRecoveryReferencesRetireOnlyOnAcknowledgedWrite(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", Volumes: []types.Volume{{Name: "data", Mount: "/data"}}} + if err := s.commit(s.set(sb)); err != nil { + t.Fatal(err) + } + restoreWrites := failClaimWrites(t, s) + for i := range 200 { + sb.Token = fmt.Sprint(i) + sb.Volumes[0].Mount = []string{"/data", "/other"}[i%2] + if err := s.commit(s.set(sb)); err == nil { + t.Fatal("expected failed snapshot") + } + } + refs := s.recoveryVolumesFor(sb.ID) + refs[0].Name = "caller-copy" + if got := s.recoveryVolumesFor(sb.ID); len(got) != 2 || got[0].Name != "data" { + t.Fatalf("recovery references duplicated or shared: %+v", got) + } + restoreWrites() + s = newClaimStore(filepath.Dir(s.path), false) + if _, err := s.load(); err != nil { + t.Fatal(err) + } + s.reset(map[string]*types.Sandbox{}) + if got := s.recoveryVolumesFor(sb.ID); len(got) != 1 || got[0].Name != "data" { + t.Fatalf("reset erased the file reference: %+v", got) + } + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if s.hasRecoveryVolumes(sb.ID) { + t.Fatal("empty snapshot did not retire file references") + } +} + +func TestClaimSnapshotAcknowledgesCapturedRowsBeforeNewerReplacement(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + applied := []types.Volume{{Name: "data", Mount: "/data"}} + wanted := []types.Volume{{Name: "wanted", Mount: "/wanted"}} + sb := &types.Sandbox{ID: "owner", Volumes: slices.Clone(applied), VolumesWant: slices.Clone(wanted)} + old := s.set(sb) + release := captureClaimWrite(t, s, old, sb.ID) + newer := s.del(sb.ID) + sb.Volumes[0].Name = "mutated-applied" + sb.VolumesWant[0].Name = "mutated-wanted" + raw := release() + if !s.pending(newer) { + t.Fatal("captured snapshot acknowledged newer deletion") + } + if refs := s.recoveryVolumesFor(sb.ID); !slices.Equal(refs, append(slices.Clone(applied), wanted...)) { + t.Fatalf("captured row not acknowledged: %+v", refs) + } + var persisted map[string]*types.Sandbox + err := json.Unmarshal(raw, &persisted) + if err != nil || persisted[sb.ID] == nil || !slices.Equal(persisted[sb.ID].Volumes, applied) || !slices.Equal(persisted[sb.ID].VolumesWant, wanted) { + t.Fatalf("captured write: %+v, %v", persisted, err) + } + if err = s.commit(newer); err != nil { + t.Fatal(err) + } + if s.hasRecoveryVolumes(sb.ID) { + t.Fatal("empty replacement retained old reference") + } + failClaimWrites(t, s) + if err = s.commit(old); err != nil { + t.Fatal(err) + } + persisted, err = newClaimStore(filepath.Dir(s.path), false).load() + if err != nil || len(persisted) != 0 { + t.Fatalf("old token resurrected snapshot: %+v, %v", persisted, err) + } +} + +func TestClaimRecoveryVolumeConflicts(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + reader := types.Volume{Name: "shared", Mount: "/data"} + writer, moved := types.Volume{Name: "data", Mount: "/data", Mode: types.VolumeModeRW}, types.Volume{Name: "data", Mount: "/elsewhere", Mode: types.VolumeModeRW} + first, second := &types.Sandbox{ID: "first", Volumes: []types.Volume{reader}}, &types.Sandbox{ID: "second", Volumes: []types.Volume{reader}} + owner := &types.Sandbox{ID: "owner", Volumes: []types.Volume{writer}} + if err := s.commit(s.set(first, second, owner)); err != nil { + t.Fatal(err) + } + restore := failClaimWrites(t, s) + owner.VolumesWant = []types.Volume{{Name: "other", Mount: "/other"}} + if err := s.commit(s.set(owner)); err == nil { + t.Fatal("expected write failure") + } + sharedWriter, ownerReader := reader, writer + sharedWriter.Mode, ownerReader.Mode = types.VolumeModeRW, types.VolumeModeRO + for _, tc := range []struct { + volume types.Volume + owner string + conflict bool + }{ + {reader, "", false}, + {sharedWriter, "", true}, + {sharedWriter, first.ID, true}, + {writer, owner.ID, false}, + {writer, "different", true}, + {ownerReader, owner.ID, true}, + {moved, owner.ID, true}, + } { + if got := s.volumeConflicts(tc.volume, tc.owner); got != tc.conflict { + t.Errorf("volume %+v owner %q conflict=%t, want %t", tc.volume, tc.owner, got, tc.conflict) + } + } + s.set(&types.Sandbox{ID: owner.ID, Volumes: []types.Volume{writer}}) + restore() + if err := s.commit(s.del(first.ID)); err != nil { + t.Fatal(err) + } + if !s.referencesVolume(reader.Name) || s.hasRecoveryVolumes(first.ID) || !s.volumeConflicts(sharedWriter, first.ID) { + t.Fatal("retiring one reader released the other") + } + if s.referencesVolume("other") || s.volumeConflicts(writer, owner.ID) { + t.Fatal("successful return retained attempted attachment") + } + if err := s.commit(s.del(second.ID)); err != nil { + t.Fatal(err) + } + if s.referencesVolume(reader.Name) || s.volumeConflicts(sharedWriter, "") { + t.Fatal("last reader left recovery reservation") + } +} + +func TestClaimCaptureAcknowledgesBothReferenceDomains(t *testing.T) { + for _, tc := range []struct { + name string + drop func(*types.Sandbox) + checkpoint, volumes bool + }{ + {"volumes", func(sb *types.Sandbox) { sb.Volumes = nil }, true, false}, + {"checkpoints", func(sb *types.Sandbox) { sb.ReplicaCk = "" }, false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", ReplicaCk: "checkpoint", Volumes: []types.Volume{{Name: "data"}}} + release := captureClaimWrite(t, s, s.set(sb), sb.ID) + tc.drop(sb) + newer := s.set(sb) + release() + if !s.pending(newer) || !s.referencesCheckpoint("checkpoint") || !s.hasRecoveryVolumes(sb.ID) { + t.Fatal("older capture retired newer state or its recovery references") + } + if err := s.commit(newer); err != nil { + t.Fatal(err) + } + if s.referencesCheckpoint("checkpoint") != tc.checkpoint || s.hasRecoveryVolumes(sb.ID) != tc.volumes { + t.Fatalf("references after newer commit: checkpoint=%t volumes=%t", s.referencesCheckpoint("checkpoint"), s.hasRecoveryVolumes(sb.ID)) + } + }) + } +} + +// detachWithoutCommit detaches every volume on the device while the claim write fails until unblock. +func detachWithoutCommit(t *testing.T, m *Manager, eng *fakeEngine, id, tenant string) (unblock func()) { + t.Helper() + m.store.recommitting.Store(true) + tmp := m.store.path + ".tmp" + eng.liveVolumeHook = func(verb string) error { + if verb == "detach" { + return os.Mkdir(tmp, 0o700) + } + return nil + } + view, err := m.SetVolumes(t.Context(), id, tenant, nil) + eng.liveVolumeHook = nil + if err == nil || len(view.Volumes) != 0 || len(view.VolumesWant) != 0 { + t.Fatalf("detach did not fail after device removal: %+v, %v", view, err) + } + return func() { + if err := os.Remove(tmp); err != nil { + t.Fatal(err) + } + } +} + +func errOf(_ any, err error) error { return err } + +func setVolumeTenants(m *Manager, name string, tenants ...string) { + m.mu.Lock() + defer m.mu.Unlock() + entry := *m.catalog.entries[name] + entry.tenants = tenants + m.catalog.entries[name] = &entry +} diff --git a/sandboxd/pool/volume_rw_test.go b/sandboxd/pool/volume_rw_test.go index a81ff1bb..0369858d 100644 --- a/sandboxd/pool/volume_rw_test.go +++ b/sandboxd/pool/volume_rw_test.go @@ -3,6 +3,7 @@ package pool import ( "encoding/json/v2" "errors" + "fmt" "os" "path/filepath" "slices" @@ -16,6 +17,84 @@ import ( "github.com/cocoonstack/sandbox/sandboxd/types" ) +func TestLiveWritableDetachRequiresCleanEvidence(t *testing.T) { + for _, proof := range []bool{false, true} { + t.Run(fmt.Sprint(proof), func(t *testing.T) { + base := newFakeEngine() + catalog := liveCatalog(t) + m := newVolumeManager(t, base, catalog) + base.detachNormalUnmount = &proof + sb := mustClaim(t, m, testKey) + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}); err != nil { + t.Fatal(err) + } + path := catalog[0].Path + if !volumeDirty(path) || !base.attachDirty["data"] { + t.Fatal("writer was attached before dirty marker") + } + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err != nil { + t.Fatal(err) + } + if volumeDirty(path) == proof { + t.Fatalf("marker disagrees with clean evidence: proof=%t dirty=%t", proof, volumeDirty(path)) + } + _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data"}}}) + if proof && err != nil || !proof && !errors.Is(err, ErrVolumeNeedsRecovery) { + t.Fatalf("reader admission after detach: %v", err) + } + }) + } +} + +func TestLiveWritableFailedHostDetachKeepsMarkerAndHold(t *testing.T) { + eng := newFakeEngine() + catalog := liveCatalog(t) + m := newVolumeManager(t, eng, catalog) + sb := mustClaim(t, m, testKey) + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}); err != nil { + t.Fatal(err) + } + eng.detachVolumeErr = errors.New("host removal uncertain") + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err == nil { + t.Fatal("failed host detach reported success") + } + if !volumeDirty(catalog[0].Path) || volumeHoldersOf(m, "data").writers != 1 { + t.Fatal("failed host detach lost dirty marker or writer") + } + if _, err := m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "uncertain", ""); !errors.Is(err, ErrVolumeCapture) { + t.Fatalf("capture after uncertain detach: %v", err) + } + eng.detachVolumeErr = nil + eng.detachNormalUnmount = new(bool) + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err != nil { + t.Fatal(err) + } + if !volumeDirty(catalog[0].Path) { + t.Fatal("absent-device retry invented clean evidence") + } +} + +func TestLiveWritableUncertainAdditionClearsOnlyAfterDetach(t *testing.T) { + eng := newFakeEngine() + catalog := liveCatalog(t) + m := newVolumeManager(t, eng, catalog) + sb := mustClaim(t, m, testKey) + eng.mountVolumeErr = errors.New("mount response lost") + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}); err == nil { + t.Fatal("expected unknown mount result") + } + if len(sb.Volumes) != 0 || len(sb.VolumesWant) != 1 || !volumeDirty(catalog[0].Path) { + t.Fatal("pending writable attachment lost intent or marker") + } + eng.mountVolumeErr = nil + if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err != nil { + t.Fatal(err) + } + if volumeDirty(catalog[0].Path) || volumeHoldersOf(m, "data") != (volumeHolders{}) { + t.Fatal("uncertain addition cleanup retained marker or hold") + } +} + func TestWritableClaimMarksDirtyBeforeAttachAndClearsOnRelease(t *testing.T) { path := writeVolumeImage(t, "scratch.img", "data") eng := newFakeEngine() @@ -152,10 +231,7 @@ func TestConfirmVolumesCleanCatchesMarkerAfterAdmission(t *testing.T) { if markErr := markVolumeDirty(path); markErr != nil { t.Fatalf("mark dirty: %v", markErr) } - m.mu.Lock() - reserveErr := m.reserveVolumes(appliedVolumes(resolved)) - m.mu.Unlock() - if reserveErr != nil { + if reserveErr := m.admitClaim("", resolved, appliedVolumes(resolved)); reserveErr != nil { t.Fatalf("reserve once the writer released: %v", reserveErr) } @@ -222,14 +298,18 @@ func TestVolumeAdmissionMatrix(t *testing.T) { func TestReserveVolumesExcludesReaderUnderWriter(t *testing.T) { path := writeVolumeImage(t, "scratch.img", "data") m := newVolumeManager(t, newFakeEngine(), []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + resolved, err := m.resolveVolumes(t.Context(), "", []types.Volume{{Name: "scratch", Mount: "/volumes/scratch"}}) + if err != nil { + t.Fatal(err) + } m.mu.Lock() - defer m.mu.Unlock() m.adoptVolumes([]types.Volume{{Name: "scratch", Mount: "/volumes/scratch", Mode: types.VolumeModeRW}}) + m.mu.Unlock() - if err := m.reserveVolumes([]types.Volume{{Name: "scratch", Mount: "/volumes/scratch"}}); !errors.Is(err, ErrVolumeBusy) { + if err := m.admitClaim("", resolved, appliedVolumes(resolved)); !errors.Is(err, ErrVolumeBusy) { t.Errorf("read-only reservation under a writer: %v, want ErrVolumeBusy", err) } - if holders := m.volumeAdmission["scratch"]; holders != (volumeHolders{writers: 1}) { + if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{writers: 1}) { t.Errorf("registry after the refusal=%+v, want the writer alone", holders) } } @@ -580,5 +660,5 @@ func newVolumeManagerAt(t *testing.T, eng *fakeEngine, dataDir string, volumes [ func volumeHoldersOf(m *Manager, name string) volumeHolders { m.mu.Lock() defer m.mu.Unlock() - return m.volumeAdmission[name] + return m.catalog.holders[name] } diff --git a/sandboxd/server/api.go b/sandboxd/server/api.go index 6e26e0c9..9a26c5df 100644 --- a/sandboxd/server/api.go +++ b/sandboxd/server/api.go @@ -1,6 +1,8 @@ package server import ( + "errors" + "slices" "time" "github.com/cocoonstack/sandbox/sandboxd/types" @@ -42,6 +44,19 @@ func (r ClaimRequest) Key() types.PoolKey { return types.PoolKey{Template: r.Template, Net: r.Net, Size: r.Size}.Defaulted() } +func (r ClaimRequest) normalizedVolumes() ([]types.Volume, error) { + volumes := slices.Clone(r.Volumes) + if r.VolumesAttachOnly { + if len(volumes) == 0 { + return nil, errors.New("volumes_attach_only requires at least one volume") + } + for i := range volumes { + volumes[i].AttachOnly = true + } + } + return types.ValidateVolumes(volumes) +} + // ClaimResponse is the wire reply of POST /v1/claim; success and Redirect are exclusive. type ClaimResponse struct { ID string `json:"id,omitempty"` diff --git a/sandboxd/server/server.go b/sandboxd/server/server.go index 78b44fd6..270c2ad0 100644 --- a/sandboxd/server/server.go +++ b/sandboxd/server/server.go @@ -47,6 +47,8 @@ var poolErrHTTP = []struct { }{ {pool.ErrBadKey, http.StatusBadRequest, ""}, {pool.ErrBadVolume, http.StatusBadRequest, ""}, + {pool.ErrVolumeRegistrationConflict, http.StatusConflict, pool.ErrVolumeRegistrationConflict.Error()}, + {pool.ErrVolumeCatalogUnavailable, http.StatusServiceUnavailable, "volume catalog unavailable; retry"}, {pool.ErrBadName, http.StatusBadRequest, ""}, {pool.ErrBadCount, http.StatusBadRequest, ""}, {pool.ErrBadEnv, http.StatusBadRequest, ""}, @@ -63,6 +65,9 @@ var poolErrHTTP = []struct { {pool.ErrNoEgressFork, http.StatusConflict, ""}, {pool.ErrVolumeCapture, http.StatusConflict, ""}, {pool.ErrVolumeBusy, http.StatusConflict, ""}, + {pool.ErrReplicaPresent, http.StatusServiceUnavailable, ""}, + {pool.ErrReplicatingPool, http.StatusConflict, ""}, + {pool.ErrReplicaStranded, http.StatusConflict, ""}, {pool.ErrVolumeNeedsRecovery, http.StatusConflict, ""}, {pool.ErrArchived, http.StatusConflict, ""}, {pool.ErrPaused, http.StatusConflict, ""}, @@ -97,6 +102,8 @@ type Manager interface { TenantClaims() map[string]int VolumePlacement(key types.PoolKey, tenant string, names []string) (bool, error) Volumes(tenant string, holders map[string]int) []types.VolumeInfo + RegisterVolume(ctx context.Context, name string, spec types.VolumeRegistration) error + UnregisterVolume(ctx context.Context, name string) error GetVolumes(id, tenant string) (types.SandboxVolumesResponse, error) SetVolumes(ctx context.Context, id, tenant string, volumes []types.Volume) (types.SandboxVolumesResponse, error) PatchVolumes(ctx context.Context, id, tenant string, patch types.VolumePatch) (types.SandboxVolumesResponse, error) @@ -269,6 +276,8 @@ func (s *Server) Handler() http.Handler { mux := http.NewServeMux() mux.HandleFunc("POST /v1/claim", s.requireToken(s.handleClaim)) mux.HandleFunc("GET /v1/volumes", s.requireToken(s.handleVolumes)) + mux.HandleFunc("PUT /v1/volumes/{name}", s.requireRoot(s.handleVolumeRegistration)) + mux.HandleFunc("DELETE /v1/volumes/{name}", s.requireRoot(s.handleVolumeRegistration)) mux.HandleFunc("POST /v1/sandboxes/{id}/release", s.handleSandboxVerb("release", s.mgr.Release)) mux.HandleFunc("POST /v1/sandboxes/{id}/hibernate", s.handleSandboxVerb("hibernate", s.mgr.Hibernate)) mux.HandleFunc("POST /v1/sandboxes/{id}/wake", s.handleSandboxVerb("wake", s.mgr.Wake)) @@ -350,7 +359,7 @@ func (s *Server) handleClaim(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleVolumeClaim(w http.ResponseWriter, r *http.Request, req ClaimRequest, key types.PoolKey, hash, tenant string) { - volumes, err := types.ValidateVolumes(req.Volumes, req.VolumesAttachOnly) + volumes, err := req.normalizedVolumes() if err != nil { writeErr(w, http.StatusBadRequest, fmt.Sprintf("%v: %v", pool.ErrBadVolume, err)) return diff --git a/sandboxd/server/server_test.go b/sandboxd/server/server_test.go index 6debba66..f24769d1 100644 --- a/sandboxd/server/server_test.go +++ b/sandboxd/server/server_test.go @@ -2482,6 +2482,8 @@ type checkpointClaimFunc func(string) (*types.Sandbox, error) type idActionFunc func(string) error type fakeManager struct { + registerVolume func(context.Context, string, types.VolumeRegistration) error + unregisterVolume func(context.Context, string) error getVolumes func(string, string) (types.SandboxVolumesResponse, error) setVolumes func(string, string, []types.Volume) (types.SandboxVolumesResponse, error) patchVolumes func(string, string, types.VolumePatch) (types.SandboxVolumesResponse, error) @@ -2557,6 +2559,14 @@ type fakeManager struct { reader *fakeReader } +func (f *fakeManager) RegisterVolume(ctx context.Context, name string, spec types.VolumeRegistration) error { + return f.registerVolume(ctx, name, spec) +} + +func (f *fakeManager) UnregisterVolume(ctx context.Context, name string) error { + return f.unregisterVolume(ctx, name) +} + func (f *fakeManager) GetVolumes(id, tenant string) (types.SandboxVolumesResponse, error) { return f.getVolumes(id, tenant) } diff --git a/sandboxd/server/volume_mutation.go b/sandboxd/server/volume_mutation.go index 1e4f86f9..86dd6398 100644 --- a/sandboxd/server/volume_mutation.go +++ b/sandboxd/server/volume_mutation.go @@ -60,7 +60,7 @@ func (s *Server) handleSandboxVolumes(w http.ResponseWriter, r *http.Request) { code, view.Error = mapped, msg } } - if code >= http.StatusInternalServerError { + if code >= http.StatusInternalServerError && !errors.Is(err, pool.ErrReplicaPresent) { log.WithFunc("server.handleSandboxVolumes").Errorf(r.Context(), err, "converge volumes for %s", id) } } diff --git a/sandboxd/server/volume_mutation_test.go b/sandboxd/server/volume_mutation_test.go index 48ff46b6..4cd91e17 100644 --- a/sandboxd/server/volume_mutation_test.go +++ b/sandboxd/server/volume_mutation_test.go @@ -153,7 +153,7 @@ func TestLiveVolumeRoutesHideOtherTenants(t *testing.T) { func TestLiveVolumeRoutesReportRefusalsOverRecordedDiagnostics(t *testing.T) { const cause = "cannot unmount /data; close guest references and retry" - for _, refusal := range []error{pool.ErrVolumeBusy, pool.ErrVolumeCapture} { + for _, refusal := range []error{pool.ErrVolumeBusy, pool.ErrVolumeCapture, pool.ErrReplicaPresent, pool.ErrReplicatingPool, pool.ErrReplicaStranded} { t.Run(refusal.Error(), func(t *testing.T) { mgr := &fakeManager{setVolumes: func(string, string, []types.Volume) (types.SandboxVolumesResponse, error) { return types.SandboxVolumesResponse{VolumesWant: []types.Volume{{Name: "old"}}, States: []types.VolumeState{{Name: "old", State: "pending", Error: cause}}, Error: "volume change failed; retry the desired target"}, fmt.Errorf("%w: volume %q", refusal, "data") @@ -165,9 +165,45 @@ func TestLiveVolumeRoutesReportRefusalsOverRecordedDiagnostics(t *testing.T) { if err := json.UnmarshalRead(resp.Body, &got); err != nil { t.Fatal(err) } - if resp.StatusCode != http.StatusConflict || !strings.HasPrefix(got.Error, refusal.Error()) || len(got.States) != 1 || got.States[0].Error != cause { + wantStatus := http.StatusConflict + if errors.Is(refusal, pool.ErrReplicaPresent) { + wantStatus = http.StatusServiceUnavailable + if resp.Header.Get("Retry-After") == "" { + t.Fatal("temporary replica retirement refusal has no Retry-After") + } + } else if resp.Header.Get("Retry-After") != "" { + t.Fatal("permanent conflict advertises a retry delay") + } + if resp.StatusCode != wantStatus || !strings.HasPrefix(got.Error, refusal.Error()) || len(got.States) != 1 || got.States[0].Error != cause { t.Fatalf("status=%d view=%+v", resp.StatusCode, got) } }) } } + +func TestClaimVolumeCompatibilityExpansion(t *testing.T) { + for _, tt := range []struct { + name string + req ClaimRequest + want []types.Volume + err bool + }{ + {"default mount", ClaimRequest{Volumes: []types.Volume{{Name: "data"}}}, []types.Volume{{Name: "data", Mount: "/volumes/data"}}, false}, + {"legacy all devices", ClaimRequest{VolumesAttachOnly: true, Volumes: []types.Volume{{Name: "data"}, {Name: "work", Mode: types.VolumeModeRW}}}, []types.Volume{{Name: "data", AttachOnly: true}, {Name: "work", Mode: types.VolumeModeRW, AttachOnly: true}}, false}, + {"mixed", ClaimRequest{Volumes: []types.Volume{{Name: "data"}, {Name: "work", AttachOnly: true}}}, []types.Volume{{Name: "data", Mount: "/volumes/data"}, {Name: "work", AttachOnly: true}}, false}, + {"mixed reversed", ClaimRequest{Volumes: []types.Volume{{Name: "work", AttachOnly: true}, {Name: "data"}}}, []types.Volume{{Name: "work", AttachOnly: true}, {Name: "data", Mount: "/volumes/data"}}, false}, + {"empty legacy", ClaimRequest{VolumesAttachOnly: true}, nil, true}, + {"legacy mount conflict", ClaimRequest{VolumesAttachOnly: true, Volumes: []types.Volume{{Name: "data", Mount: "/data"}}}, nil, true}, + } { + t.Run(tt.name, func(t *testing.T) { + before := slices.Clone(tt.req.Volumes) + got, err := tt.req.normalizedVolumes() + if (err != nil) != tt.err || !slices.Equal(got, tt.want) { + t.Fatalf("normalized volumes=%+v err=%v, want %+v error=%t", got, err, tt.want, tt.err) + } + if !slices.Equal(tt.req.Volumes, before) { + t.Fatal("legacy expansion mutated caller input") + } + }) + } +} diff --git a/sandboxd/server/volume_registration.go b/sandboxd/server/volume_registration.go new file mode 100644 index 00000000..29fae3ab --- /dev/null +++ b/sandboxd/server/volume_registration.go @@ -0,0 +1,38 @@ +package server + +import ( + "context" + "errors" + "net/http" + + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func (s *Server) handleVolumeRegistration(w http.ResponseWriter, r *http.Request) { + if s.apiToken == "" { + writeErr(w, http.StatusForbidden, "volume registration requires a configured api token") + return + } + name := r.PathValue("name") + var err error + op := "unregister volume" + if r.Method == http.MethodPut { + req, ok := decodeBodyStrict[types.VolumeRegistration](w, r) + if !ok { + return + } + op = "register volume" + err = s.mgr.RegisterVolume(r.Context(), name, req) + } else { + err = s.mgr.UnregisterVolume(r.Context(), name) + } + switch { + case writePoolErr(w, err): + case errors.Is(err, context.DeadlineExceeded): + writeErr(w, http.StatusGatewayTimeout, op+" timed out") + case errors.Is(err, context.Canceled): + writeErr(w, http.StatusRequestTimeout, op+" canceled") + default: + writeResult(w, r, op, name, op+" failed", err, noContent(w)) + } +} diff --git a/sandboxd/server/volume_registration_test.go b/sandboxd/server/volume_registration_test.go new file mode 100644 index 00000000..037d225b --- /dev/null +++ b/sandboxd/server/volume_registration_test.go @@ -0,0 +1,148 @@ +package server + +import ( + "bytes" + "context" + "encoding/json/v2" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "reflect" + "strconv" + "strings" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/pool" + "github.com/cocoonstack/sandbox/sandboxd/tenants" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestVolumeRegistrationRoutesRequireRootBeforeReadingBody(t *testing.T) { + for _, method := range []string{http.MethodPut, http.MethodDelete} { + for _, tt := range []struct { + name, apiToken, token string + code int + wantCalled bool + }{ + {"root", "root", "root", http.StatusNoContent, true}, + {"tenant", "root", "acme-token", http.StatusForbidden, false}, + {"sandbox", "root", "sandbox-token", http.StatusUnauthorized, false}, + {"unknown", "root", "unknown", http.StatusUnauthorized, false}, + {"missing", "root", "", http.StatusUnauthorized, false}, + {"no api token", "", "", http.StatusForbidden, false}, + } { + t.Run(method+"/"+tt.name, func(t *testing.T) { + called := false + mgr := &fakeManager{ + registerVolume: func(context.Context, string, types.VolumeRegistration) error { called = true; return nil }, + unregisterVolume: func(context.Context, string) error { called = true; return nil }, + tenants: []tenants.TenantSpec{{Name: "acme", Token: "acme-token"}}, + } + body := bytes.NewBufferString(`{"path":"/private/data.img","writable":true}`) + unread := body.Len() + req := httptest.NewRequest(method, "/v1/volumes/data", body) + req.Header.Set("Authorization", "Bearer "+tt.token) + w := httptest.NewRecorder() + New(tt.apiToken, "node:7777", mgr, &fakeDialer{}, nil, nil, nil, nil).Handler().ServeHTTP(w, req) + if w.Code != tt.code || called != tt.wantCalled || !tt.wantCalled && body.Len() != unread || w.Header().Get("Retry-After") != "" { + t.Fatalf("status=%d called=%v unread=%d/%d body=%s headers=%v, want %d", w.Code, called, body.Len(), unread, w.Body, w.Header(), tt.code) + } + }) + } + } +} + +func TestVolumeRegistrationRoutesForwardBindingAndContext(t *testing.T) { + type requestKey struct{} + const body = `{"path":"/private/images/../data.img","directio":"auto","writable":true,"tenants":["beta","acme"]}` + want := types.VolumeRegistration{Path: "/private/images/../data.img", DirectIO: "auto", Writable: true, Tenants: []string{"beta", "acme"}} + var registered types.VolumeRegistration + var registeredName, removedName string + checkContext := func(ctx context.Context) { + if ctx.Value(requestKey{}) != "request-value" { + t.Error("manager did not receive the request context") + } + } + mgr := &fakeManager{ + registerVolume: func(ctx context.Context, name string, spec types.VolumeRegistration) error { + checkContext(ctx) + registeredName, registered = name, spec + return nil + }, + unregisterVolume: func(ctx context.Context, name string) error { + checkContext(ctx) + removedName = name + return nil + }, + } + handler := New("root", "", mgr, nil, nil, nil, nil, nil).Handler() + for _, method := range []string{http.MethodPut, http.MethodDelete} { + req := httptest.NewRequest(method, "/v1/volumes/data-v1", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer root") + req = req.WithContext(context.WithValue(req.Context(), requestKey{}, "request-value")) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if w.Code != http.StatusNoContent || w.Body.Len() != 0 { + t.Fatalf("%s status=%d body=%s, want empty 204", method, w.Code, w.Body) + } + } + if registeredName != "data-v1" || removedName != "data-v1" || !reflect.DeepEqual(registered, want) { + t.Fatalf("registered %q %+v, removed %q, want data-v1 %+v", registeredName, registered, removedName, want) + } +} + +func TestVolumeRegistrationRejectsBodyName(t *testing.T) { + called := false + mgr := &fakeManager{registerVolume: func(context.Context, string, types.VolumeRegistration) error { + called = true + return nil + }} + ts := newTestServer(t, "root", mgr, nil) + resp := doReq(t, http.MethodPut, ts.URL+"/v1/volumes/data", "root", `{"name":"other","path":"/private/data.img"}`) + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusBadRequest || called { + t.Fatalf("status=%d called=%v body=%s, want 400 without registration", resp.StatusCode, called, body) + } +} + +func TestVolumeRegistrationRoutesMapSafeErrors(t *testing.T) { + for _, method := range []string{http.MethodPut, http.MethodDelete} { + for _, tt := range []struct { + name string + err error + code int + }{ + {"invalid registration", fmt.Errorf("%w: invalid directio", pool.ErrBadVolume), http.StatusBadRequest}, + {"binding conflict", fmt.Errorf("%w: /private/data.img", pool.ErrVolumeRegistrationConflict), http.StatusConflict}, + {"catalog unavailable", fmt.Errorf("%w: sync /private/runtime-volumes.json", pool.ErrVolumeCatalogUnavailable), http.StatusServiceUnavailable}, + {"uncertain cancellation", fmt.Errorf("%w: %w", pool.ErrVolumeCatalogUnavailable, context.Canceled), http.StatusServiceUnavailable}, + {"uncertain timeout", fmt.Errorf("%w: %w", pool.ErrVolumeCatalogUnavailable, context.DeadlineExceeded), http.StatusServiceUnavailable}, + {"timeout", fmt.Errorf("register: %w", context.DeadlineExceeded), http.StatusGatewayTimeout}, + {"canceled", fmt.Errorf("register: %w", context.Canceled), http.StatusRequestTimeout}, + {"unexpected failure", errors.New("open /private/data.img: permission denied"), http.StatusInternalServerError}, + } { + t.Run(method+"/"+tt.name, func(t *testing.T) { + mgr := &fakeManager{ + registerVolume: func(context.Context, string, types.VolumeRegistration) error { return tt.err }, + unregisterVolume: func(context.Context, string) error { return tt.err }, + } + ts := newTestServer(t, "root", mgr, nil) + resp := doReq(t, method, ts.URL+"/v1/volumes/data", "root", `{"path":"/private/data.img"}`) + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + var got map[string]string + if err := json.Unmarshal(body, &got); err != nil || len(got) != 1 || got["error"] == "" { + t.Fatalf("error body = %s, decode error = %v", body, err) + } + retry := tt.code == http.StatusServiceUnavailable + seconds, _ := strconv.Atoi(resp.Header.Get("Retry-After")) + if resp.StatusCode != tt.code || strings.Contains(string(body), "/private") || (seconds >= 1) != retry || strings.Contains(got["error"], "retry") != retry { + t.Fatalf("status=%d headers=%v body=%s, want safe %d with retry=%t", resp.StatusCode, resp.Header, body, tt.code, retry) + } + }) + } + } +} diff --git a/sandboxd/types/api.go b/sandboxd/types/api.go index ac0cd6b4..0d81b540 100644 --- a/sandboxd/types/api.go +++ b/sandboxd/types/api.go @@ -22,6 +22,14 @@ type VolumeInfo struct { Writable bool `json:"writable,omitzero"` } +// VolumeRegistration binds a backing path to a runtime catalog name. +type VolumeRegistration struct { + Path string `json:"path"` + DirectIO string `json:"directio,omitempty"` + Writable bool `json:"writable,omitzero"` + Tenants []string `json:"tenants,omitempty"` +} + // VolumePatch is the wire body of PATCH /v1/sandboxes/{id}/volumes. type VolumePatch struct { Add []Volume `json:"add,omitzero"` diff --git a/sandboxd/types/types.go b/sandboxd/types/types.go index 4cc8d307..be49f2ef 100644 --- a/sandboxd/types/types.go +++ b/sandboxd/types/types.go @@ -731,6 +731,8 @@ type VMDevices struct { // VMRecord is the subset of cocoon's VM records the control plane reads. type VMRecord struct { + // Stale marks a stop cocoon inferred from a lost PID file, not one it observed. + Stale bool `json:"stale,omitzero"` State string `json:"state"` PID int `json:"pid"` VsockSocket string `json:"vsock_socket"` @@ -752,8 +754,8 @@ type Volume struct { Name string `json:"name"` Mount string `json:"mount,omitempty"` Mode string `json:"mode,omitempty"` - // AttachOnly carries the claim's flag into validation; past it the empty Mount is the signal. - AttachOnly bool `json:"-"` + // AttachOnly leaves mounting and unmounting to the caller. + AttachOnly bool `json:"attach_only,omitzero"` } // RW reports whether the entry asks for write access. @@ -803,19 +805,11 @@ func VolumeRWNames(volumes []Volume) []string { return names } -// VolumesAttachOnly reports whether the caller mounts the devices itself. -func VolumesAttachOnly(volumes []Volume) bool { - return slices.ContainsFunc(volumes, func(v Volume) bool { return v.AttachOnly }) -} - // ValidateVolumes returns detached entries with defaults filled and modes normalized. -func ValidateVolumes(volumes []Volume, attachOnly bool) ([]Volume, error) { +func ValidateVolumes(volumes []Volume) ([]Volume, error) { if len(volumes) > MaxClaimVolumes { return nil, fmt.Errorf("volumes must contain at most %d entries, got %d", MaxClaimVolumes, len(volumes)) } - if attachOnly && len(volumes) == 0 { - return nil, errors.New("volumes_attach_only requires at least one volume") - } applied := make([]Volume, len(volumes)) names := make(map[string]struct{}, len(volumes)) for i, volume := range volumes { @@ -834,7 +828,7 @@ func ValidateVolumes(volumes []Volume, attachOnly bool) ([]Volume, error) { default: return nil, fmt.Errorf("volumes[%d] mode %q must be %s or %s", i, mode, VolumeModeRO, VolumeModeRW) } - if attachOnly { + if volume.AttachOnly { if volume.Mount != "" { return nil, fmt.Errorf("volumes[%d] mount %q is meaningless when the caller mounts the device itself", i, volume.Mount) } @@ -847,6 +841,9 @@ func ValidateVolumes(volumes []Volume, attachOnly bool) ([]Volume, error) { } for j := range i { other := applied[j].Mount + if other == "" { + continue + } switch { case mount == other: return nil, fmt.Errorf("volumes[%d] mount %q duplicates volumes[%d]", i, mount, j) diff --git a/sandboxd/types/volume_test.go b/sandboxd/types/volume_test.go index 9549af49..c7f428f1 100644 --- a/sandboxd/types/volume_test.go +++ b/sandboxd/types/volume_test.go @@ -31,7 +31,7 @@ func TestValidVolumeName(t *testing.T) { func TestValidateVolumes(t *testing.T) { volumes := []Volume{{Name: "dataset"}, {Name: "weights-1", Mount: "/models"}} wantInput := slices.Clone(volumes) - got, err := ValidateVolumes(volumes, false) + got, err := ValidateVolumes(volumes) if err != nil { t.Fatalf("ValidateVolumes: %v", err) } @@ -54,7 +54,7 @@ func TestValidateVolumes(t *testing.T) { {Name: "f"}, {Name: "g"}, {Name: "h"}, - }, false); err != nil || len(got) != MaxClaimVolumes { + }); err != nil || len(got) != MaxClaimVolumes { t.Errorf("ValidateVolumes at limit: got=%v err=%v", got, err) } @@ -75,7 +75,7 @@ func TestValidateVolumes(t *testing.T) { {"uppercase-mode", []Volume{{Name: "dataset", Mode: "RW"}}}, } { t.Run(tt.name, func(t *testing.T) { - if _, err := ValidateVolumes(tt.volumes, false); err == nil { + if _, err := ValidateVolumes(tt.volumes); err == nil { t.Fatal("ValidateVolumes succeeded") } }) @@ -87,7 +87,7 @@ func TestValidateVolumesNormalizesMode(t *testing.T) { {Name: "shared"}, {Name: "explicit", Mode: VolumeModeRO}, {Name: "writable", Mode: VolumeModeRW}, - }, false) + }) if err != nil { t.Fatalf("ValidateVolumes: %v", err) } @@ -112,10 +112,10 @@ func TestValidateVolumesNormalizesMode(t *testing.T) { func TestValidateVolumesAttachOnly(t *testing.T) { got, err := ValidateVolumes([]Volume{ - {Name: "dataset"}, - {Name: "scratch", Mode: VolumeModeRW}, - {Name: "explicit", Mode: VolumeModeRO}, - }, true) + {Name: "dataset", AttachOnly: true}, + {Name: "scratch", Mode: VolumeModeRW, AttachOnly: true}, + {Name: "explicit", Mode: VolumeModeRO, AttachOnly: true}, + }) if err != nil { t.Fatalf("ValidateVolumes: %v", err) } @@ -127,18 +127,11 @@ func TestValidateVolumesAttachOnly(t *testing.T) { if !slices.Equal(got, want) { t.Errorf("volumes %v, want %v", got, want) } - if !VolumesAttachOnly(got) { - t.Error("validated attach-only set does not report itself") - } - if VolumesAttachOnly(nil) || VolumesAttachOnly([]Volume{{Name: "dataset", Mount: "/volumes/dataset"}}) { - t.Error("mounted set reports itself attach-only") - } for _, tt := range []struct { name string volumes []Volume }{ - {"no volumes", nil}, {"default mount spelled out", []Volume{{Name: "dataset", Mount: "/volumes/dataset"}}}, {"custom mount", []Volume{{Name: "dataset", Mount: "/datasets"}}}, {"one entry of many", []Volume{{Name: "dataset"}, {Name: "scratch", Mount: "/scratch"}}}, @@ -147,7 +140,10 @@ func TestValidateVolumesAttachOnly(t *testing.T) { {"unknown mode", []Volume{{Name: "dataset", Mode: "readwrite"}}}, } { t.Run(tt.name, func(t *testing.T) { - if _, err := ValidateVolumes(tt.volumes, true); err == nil { + for i := range tt.volumes { + tt.volumes[i].AttachOnly = true + } + if _, err := ValidateVolumes(tt.volumes); err == nil { t.Fatal("ValidateVolumes succeeded") } }) @@ -157,19 +153,38 @@ func TestValidateVolumesAttachOnly(t *testing.T) { func TestValidateVolumesRejectsGuestOSMounts(t *testing.T) { for _, root := range append([]string{"/"}, guestOSMountRoots...) { t.Run(root, func(t *testing.T) { - if _, err := ValidateVolumes([]Volume{{Name: "dataset", Mount: root}}, false); err == nil { + if _, err := ValidateVolumes([]Volume{{Name: "dataset", Mount: root}}); err == nil { t.Fatalf("accepted OS mount %q", root) } if root != "/" { - if _, err := ValidateVolumes([]Volume{{Name: "dataset", Mount: root + "/child"}}, false); err == nil { + if _, err := ValidateVolumes([]Volume{{Name: "dataset", Mount: root + "/child"}}); err == nil { t.Fatalf("accepted mount under OS root %q", root) } } }) } for _, mount := range []string{"/data", "/home/dataset", "/opt-data", "/usrdata"} { - if _, err := ValidateVolumes([]Volume{{Name: "dataset", Mount: mount}}, false); err != nil { + if _, err := ValidateVolumes([]Volume{{Name: "dataset", Mount: mount}}); err != nil { t.Errorf("rejected allowed mount %q: %v", mount, err) } } } + +func TestValidateMixedVolumesPreservesPerEntryIntent(t *testing.T) { + request := []Volume{{Name: "mounted"}, {Name: "writer", Mode: VolumeModeRW}, {Name: "raw", AttachOnly: true}, {Name: "raw-writer", Mode: VolumeModeRW, AttachOnly: true}} + got, err := ValidateVolumes(request) + if err != nil { + t.Fatal(err) + } + want := []Volume{{Name: "mounted", Mount: "/volumes/mounted"}, {Name: "writer", Mount: "/volumes/writer", Mode: VolumeModeRW}, {Name: "raw", AttachOnly: true}, {Name: "raw-writer", Mode: VolumeModeRW, AttachOnly: true}} + if !slices.Equal(got, want) { + t.Fatalf("mixed intent changed: got %+v, want %+v", got, want) + } + again, err := ValidateVolumes(got) + if err != nil || !slices.Equal(again, want) { + t.Fatalf("second validation changed mixed intent: %+v, %v", again, err) + } + if _, err = ValidateVolumes([]Volume{{Name: "raw", AttachOnly: true, Mount: "/data"}}); err == nil { + t.Fatal("accepted attach-only with a mount") + } +} diff --git a/sdk/go/client.go b/sdk/go/client.go index ccb932fd..587f2422 100644 --- a/sdk/go/client.go +++ b/sdk/go/client.go @@ -110,18 +110,6 @@ func (r claimRequest) rejectPinnedAxes() error { return nil } -func (r claimRequest) validateVolumes() error { - for _, v := range r.Volumes { - if v.Mode != "" && v.Mode != volumeModeRW { - return fmt.Errorf("volume %q: mode must be \"\", %q, or %q, got %q", v.Name, volumeModeRO, volumeModeRW, v.Mode) - } - if r.VolumesAttachOnly && v.Mount != "" { - return fmt.Errorf("volume %q: mount %q is meaningless with WithVolumesAttachOnly, which leaves mounting to the caller", v.Name, v.Mount) - } - } - return nil -} - type claimResponse struct { ID string `json:"id"` Token string `json:"token"` @@ -172,9 +160,6 @@ func (c *Client) New(ctx context.Context, template string, opts ...Option) (*San for _, opt := range opts { opt(&claim) } - if err := claim.validateVolumes(); err != nil { - return nil, err - } addr, cr, err := claimFollow(c.addr, "claim", func(noRedirect, requirePromoted bool) ([]byte, error) { claim.NoRedirect, claim.RequirePromoted = noRedirect, requirePromoted return encodeBody("claim", claim) diff --git a/sdk/go/options.go b/sdk/go/options.go index 20b75401..3b0b815f 100644 --- a/sdk/go/options.go +++ b/sdk/go/options.go @@ -17,9 +17,6 @@ const ( Large Size = "large" XLarge Size = "xlarge" XXLarge Size = "2xlarge" - - volumeModeRO = "ro" - volumeModeRW = "rw" ) // NetShape selects the sandbox network lane. @@ -35,6 +32,8 @@ type Volume struct { Mount string `json:"mount,omitempty"` // Mode is "rw" for a writable mount; empty (or "ro") means read-only. Mode string `json:"mode,omitempty"` + // AttachOnly leaves mounting and unmounting to the caller. + AttachOnly bool `json:"attach_only,omitzero"` } // VolumeInfo describes one visible fleet catalog entry. @@ -80,18 +79,11 @@ func WithSize(s Size) Option { // read-only, set Volume.Mode to "rw" for a writable mount. func WithVolumes(volumes ...Volume) Option { volumes = slices.Clone(volumes) - for i, v := range volumes { - if v.Mode == volumeModeRO { - volumes[i].Mode = "" - } - } return func(r *claimRequest) { r.Volumes = volumes } } -// WithVolumesAttachOnly attaches the claim's volumes without mounting them; -// the workload finds each device by its serial and owns the mount — and its -// flush — from there: release without a clean self-umount discards unsynced -// guest pages, since the sandbox performs no sync of its own. +// WithVolumesAttachOnly is the legacy claim-wide form of Volume.AttachOnly. +// Callers mount, flush, and unmount devices; release does not sync them. func WithVolumesAttachOnly() Option { return func(r *claimRequest) { r.VolumesAttachOnly = true } } diff --git a/sdk/go/template.go b/sdk/go/template.go index 34ee31f7..c7f45c7b 100644 --- a/sdk/go/template.go +++ b/sdk/go/template.go @@ -29,9 +29,6 @@ func (t *Template) New(ctx context.Context, opts ...Option) (*Sandbox, error) { if err := claim.rejectPinnedAxes(); err != nil { return nil, err } - if err := claim.validateVolumes(); err != nil { - return nil, err - } claim.Net, claim.Size = t.net, t.size noVolumes := len(claim.Volumes) == 0 addr, cr, err := claimFollow(t.addr, "claim", func(noRedirect, _ bool) ([]byte, error) { diff --git a/sdk/go/volume_mutation_test.go b/sdk/go/volume_mutation_test.go index 7f65427c..fab7c423 100644 --- a/sdk/go/volume_mutation_test.go +++ b/sdk/go/volume_mutation_test.go @@ -13,31 +13,28 @@ import ( ) func TestLiveVolumesUseOwnerAndAPIToken(t *testing.T) { - for _, method := range []string{http.MethodGet, http.MethodPut, http.MethodPatch} { - t.Run(method, func(t *testing.T) { + for _, request := range []struct{ method, body string }{ + {http.MethodGet, ""}, + {http.MethodPut, `{"volumes":[{"name":"data"}]}`}, + {http.MethodPatch, `{"add":[{"name":"data"}],"remove":["old"]}`}, + } { + t.Run(request.method, func(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != method || r.URL.Path != "/v1/sandboxes/sb_1/volumes" || r.Header.Get("Authorization") != "Bearer operator-token" { + if r.Method != request.method || r.URL.Path != "/v1/sandboxes/sb_1/volumes" || r.Header.Get("Authorization") != "Bearer operator-token" { t.Errorf("request: %s %s %s", r.Method, r.URL.Path, r.Header.Get("Authorization")) } body, _ := io.ReadAll(r.Body) - want := "" - switch method { - case http.MethodPut: - want = `{"volumes":[{"name":"data"}]}` - case http.MethodPatch: - want = `{"add":[{"name":"data"}],"remove":["old"]}` - } - if string(body) != want { - t.Errorf("body=%s want=%s", body, want) + if string(body) != request.body { + t.Errorf("body=%s want=%s", body, request.body) } - _, _ = io.WriteString(w, `{"volumes_want":[{"name":"data","mount":"/data"}],"volumes":[{"name":"data","mount":"/data"}],"converging":true,"states":[{"name":"data","state":"attached"}]}`) + _, _ = io.WriteString(w, `{"volumes_want":[{"name":"data","attach_only":true}],"volumes":[{"name":"data","attach_only":true}],"converging":true,"states":[{"name":"data","state":"attached"}]}`) })) t.Cleanup(ts.Close) c := testClient(t, ts, WithAPIToken("operator-token")) sb := &Sandbox{ID: "sb_1", token: "sandbox-token", owner: c.addr, c: c} c.addr = "unused.invalid:7777" - got, err := callVolumes(t, sb, method) - if err != nil || !slices.Equal(got.Volumes, []Volume{{Name: "data", Mount: "/data"}}) || !slices.Equal(got.VolumesWant, got.Volumes) || got.States[0].State != VolumeAttached || !got.Converging { + got, err := callVolumes(t, sb, request.method) + if err != nil || !slices.Equal(got.Volumes, []Volume{{Name: "data", AttachOnly: true}}) || !slices.Equal(got.VolumesWant, got.Volumes) || got.States[0].State != VolumeAttached || !got.Converging { t.Fatalf("got=%+v err=%v", got, err) } got.Volumes[0].Name, got.VolumesWant[0].Name = "caller copy", "caller copy" @@ -51,7 +48,7 @@ func TestLiveVolumesUseOwnerAndAPIToken(t *testing.T) { func TestLiveVolumeFailureRetainsPartialView(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusInternalServerError) - _, _ = io.WriteString(w, `{"volumes_want":[{"name":"new","mount":"/new"}],"volumes":[{"name":"old","mount":"/old"}],"converging":false,"states":[{"name":"new","state":"pending","error":"retry"},{"name":"old","state":"detaching"}],"error":"convergence failed"}`) + _, _ = io.WriteString(w, `{"volumes_want":[{"name":"new","mode":"rw","attach_only":true}],"volumes":[{"name":"old","attach_only":true}],"converging":false,"states":[{"name":"new","state":"pending","error":"retry"},{"name":"old","state":"detaching"}],"error":"convergence failed"}`) })) t.Cleanup(ts.Close) c := testClient(t, ts) @@ -63,7 +60,10 @@ func TestLiveVolumeFailureRetainsPartialView(t *testing.T) { if !isVolume || !isAPI || api.Status != http.StatusInternalServerError || failure.Response == nil { t.Fatalf("%s error lost response or APIError: %v", method, err) } - if !reflect.DeepEqual(got, *failure.Response) || got.States[0].State != "pending" || got.States[1].State != "detaching" || sb.VolumesWant[0].Name != "new" || sb.Volumes[0].Name != "old" { + if !reflect.DeepEqual(got, *failure.Response) || got.States[0].State != "pending" || got.States[1].State != "detaching" { + t.Fatalf("partial view=%+v", got) + } + if !slices.Equal(sb.VolumesWant, []Volume{{Name: "new", Mode: "rw", AttachOnly: true}}) || !slices.Equal(sb.Volumes, []Volume{{Name: "old", AttachOnly: true}}) { t.Fatalf("partial view=%+v handle=%+v", got, sb) } failure.Response.Volumes[0].Name = "error caller" @@ -177,6 +177,30 @@ func TestLiveVolumeIncompleteViewPreservesHandle(t *testing.T) { } } +func TestLiveVolumeMutationEncodesDeviceFields(t *testing.T) { + const want = `{"volumes":[{"name":"device","mode":"rw","attach_only":true},{"name":"mounted","mount":"/work"}]}` + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Error(err) + } + if r.Method != http.MethodPut || string(body) != want { + t.Errorf("request=%s %s want=PUT %s", r.Method, body, want) + } + _, _ = io.WriteString(w, `{"volumes_want":[],"volumes":[],"states":[],"converging":false}`) + })) + t.Cleanup(ts.Close) + c := testClient(t, ts) + sb := &Sandbox{ID: "sb_1", owner: c.addr, c: c} + _, err := sb.SetVolumes(t.Context(), []Volume{ + {Name: "device", Mode: "rw", AttachOnly: true}, + {Name: "mounted", Mount: "/work", AttachOnly: false}, + }) + if err != nil { + t.Fatal(err) + } +} + func callVolumes(t *testing.T, sb *Sandbox, method string) (SandboxVolumesResponse, error) { t.Helper() switch method { diff --git a/sdk/go/volumes.go b/sdk/go/volumes.go index 69e1dea4..4d7e3910 100644 --- a/sdk/go/volumes.go +++ b/sdk/go/volumes.go @@ -55,7 +55,7 @@ func (s *Sandbox) GetVolumes(ctx context.Context) (SandboxVolumesResponse, error return s.requestVolumes(ctx, http.MethodGet, nil, "get volumes") } -// SetVolumes converges the full desired set; an explicit empty slice clears mutable mounts, and nil is rejected. +// SetVolumes converges the full desired set; an explicit empty slice detaches all volumes, and nil is rejected. func (s *Sandbox) SetVolumes(ctx context.Context, volumes []Volume) (SandboxVolumesResponse, error) { if volumes == nil { return SandboxVolumesResponse{}, errors.New("set volumes: use an explicit empty slice to clear volumes") @@ -65,7 +65,7 @@ func (s *Sandbox) SetVolumes(ctx context.Context, volumes []Volume) (SandboxVolu }{Volumes: volumes}, "set volumes") } -// PatchVolumes adds or removes read-only mounted entries by name, retaining immutable entries. +// PatchVolumes adds or removes volumes by name. func (s *Sandbox) PatchVolumes(ctx context.Context, patch VolumePatch) (SandboxVolumesResponse, error) { return s.requestVolumes(ctx, http.MethodPatch, patch, "patch volumes") } diff --git a/sdk/go/volumes_test.go b/sdk/go/volumes_test.go index d8a2c14e..57dda6ce 100644 --- a/sdk/go/volumes_test.go +++ b/sdk/go/volumes_test.go @@ -196,7 +196,7 @@ func TestWithVolumesEncodesMode(t *testing.T) { mode string }{ {"empty stays omitted", ""}, - {"ro normalizes to omitted", "ro"}, + {"ro stays explicit", "ro"}, {"rw rides the wire", "rw"}, } for _, tt := range tests { @@ -215,9 +215,9 @@ func TestWithVolumesEncodesMode(t *testing.T) { t.Fatalf("New: %v", err) } got, present := raw.Volumes[0]["mode"] - if tt.mode == volumeModeRW { - if !present || got != volumeModeRW { - t.Errorf("wire mode = %v (present=%v), want %q", got, present, volumeModeRW) + if tt.mode != "" { + if !present || got != tt.mode { + t.Errorf("wire mode = %v (present=%v), want %q", got, present, tt.mode) } } else if present { t.Errorf("wire mode = %v, want omitted", got) @@ -231,7 +231,7 @@ func TestWithVolumesAttachOnlySendsFlagAndDecodesMountlessEcho(t *testing.T) { Volumes []map[string]any `json:"volumes"` VolumesAttachOnly bool `json:"volumes_attach_only"` } - echo := []Volume{{Name: "imagenet"}, {Name: "scratch", Mode: "rw"}} + echo := []Volume{{Name: "imagenet", AttachOnly: true}, {Name: "scratch", Mode: "rw", AttachOnly: true}} ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if err := json.UnmarshalRead(r.Body, &raw); err != nil { t.Errorf("decode body: %v", err) @@ -259,20 +259,7 @@ func TestWithVolumesAttachOnlySendsFlagAndDecodesMountlessEcho(t *testing.T) { } } -func TestWithVolumesAttachOnlyRejectsMountLocally(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { - t.Error("server should not be contacted for a locally invalid claim") - })) - t.Cleanup(ts.Close) - - _, err := testClient(t, ts).New(t.Context(), "rt:24.04", - WithVolumes(Volume{Name: "imagenet", Mount: "/datasets"}), WithVolumesAttachOnly()) - if err == nil || !strings.Contains(err.Error(), "meaningless with WithVolumesAttachOnly") { - t.Errorf("err = %v, want local mount rejection", err) - } -} - -func TestRejectsInvalidVolumeModeLocally(t *testing.T) { +func TestVolumePolicyErrorsComeFromServer(t *testing.T) { tests := []struct { name string claim func(ctx context.Context, c *Client) (*Sandbox, error) @@ -287,14 +274,15 @@ func TestRejectsInvalidVolumeModeLocally(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { - t.Error("server should not be contacted for a locally invalid mode") + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"mode must be ro or rw"}`)) })) t.Cleanup(ts.Close) _, err := tt.claim(t.Context(), testClient(t, ts)) if err == nil || !strings.Contains(err.Error(), "mode must be") { - t.Errorf("err = %v, want local mode rejection", err) + t.Errorf("err = %v, want server mode rejection", err) } }) } diff --git a/sdk/python/cocoonsandbox/client.py b/sdk/python/cocoonsandbox/client.py index aa3c8fd1..cdf8587e 100644 --- a/sdk/python/cocoonsandbox/client.py +++ b/sdk/python/cocoonsandbox/client.py @@ -9,7 +9,7 @@ import threading import time import urllib.parse -from collections.abc import Callable, Iterable, Mapping, Sequence +from collections.abc import Callable, Iterable, Sequence from typing import Any, TypeVar, cast from .checkpoint import Checkpoint @@ -18,6 +18,7 @@ from .errors import APIError, SandboxTimeout, require_field from .sandbox import Sandbox from .template import _claim_body, _template_query +from .volumes import Volume T = TypeVar("T") @@ -60,7 +61,7 @@ def new( size: str = "", ttl_seconds: int = 0, claim_ref: str = "", - volumes: list[str | Mapping[str, str]] | None = None, + volumes: list[str | Volume] | None = None, mount: bool = True, *, deadline: float | None = None, diff --git a/sdk/python/cocoonsandbox/sandbox.py b/sdk/python/cocoonsandbox/sandbox.py index 1a83fa86..b3368d44 100644 --- a/sdk/python/cocoonsandbox/sandbox.py +++ b/sdk/python/cocoonsandbox/sandbox.py @@ -7,7 +7,7 @@ import threading import time from collections import deque -from collections.abc import Callable, Iterator, Mapping +from collections.abc import Callable, Iterator from typing import TYPE_CHECKING, Any, cast from .checkpoint import Checkpoint @@ -45,7 +45,7 @@ def __init__( deadline: str = "", from_checkpoint: str = "", template_digest: str = "", - volumes: list[dict[str, str]] | None = None, + volumes: list[Volume] | None = None, ) -> None: self._client = client self.id = id @@ -54,8 +54,8 @@ def __init__( self.deadline = deadline self.from_checkpoint = from_checkpoint self.template_digest = template_digest - self.volumes = [dict(volume) for volume in volumes or []] - self.volumes_want = [dict(volume) for volume in self.volumes] + self.volumes = [volume.copy() for volume in volumes or []] + self.volumes_want = [volume.copy() for volume in self.volumes] self._pool = ConnPool(client.keep_alive) self._proto = 0 @@ -73,29 +73,27 @@ def get_volumes(self, *, timeout: float | None = None) -> SandboxVolumesResponse """Reads the desired and applied volumes from the owning node and records them on this handle.""" return self._volumes_request("GET", None, "get volumes", timeout) - def set_volumes( - self, volumes: list[str | Mapping[str, str] | Volume], *, timeout: float | None = None - ) -> SandboxVolumesResponse: - """Replaces the desired set and converges; an explicit empty list clears mutable mounts.""" - if volumes is None or isinstance(volumes, str): + def set_volumes(self, volumes: list[str | Volume], *, timeout: float | None = None) -> SandboxVolumesResponse: + """Replaces the desired set and converges; an explicit empty list detaches all volumes.""" + if not isinstance(volumes, list): raise TypeError("set volumes requires an explicit list; use [] to clear volumes") return self._volumes_request( - "PUT", {"volumes": [_volume_body(volume, True) for volume in volumes]}, "set volumes", timeout + "PUT", {"volumes": [_volume_body(volume) for volume in volumes]}, "set volumes", timeout ) def patch_volumes( self, *, - add: list[str | Mapping[str, str] | Volume] | None = None, + add: list[str | Volume] | None = None, remove: list[str] | None = None, timeout: float | None = None, ) -> SandboxVolumesResponse: - """Adds or removes mounted read-only entries by name; omitted arguments leave the set unchanged.""" - if isinstance(add, str) or isinstance(remove, str): + """Adds or removes volumes by name; omitted arguments leave the set unchanged.""" + if (add is not None and not isinstance(add, list)) or (remove is not None and not isinstance(remove, list)): raise TypeError("patch volumes takes lists of volumes and names") body: dict[str, Any] = {} if add is not None: - body["add"] = [_volume_body(volume, True) for volume in add] + body["add"] = [_volume_body(volume) for volume in add] if remove is not None: body["remove"] = list(remove) return self._volumes_request("PATCH", body, "patch volumes", timeout) @@ -445,8 +443,8 @@ def _volumes_request( return view def _record_volumes(self, view: SandboxVolumesResponse) -> None: - self.volumes_want = [cast(dict[str, str], volume.copy()) for volume in view["volumes_want"]] - self.volumes = [cast(dict[str, str], volume.copy()) for volume in view["volumes"]] + self.volumes_want = [volume.copy() for volume in view["volumes_want"]] + self.volumes = [volume.copy() for volume in view["volumes"]] def _dial(self, deadline: float | None = None) -> Conn: return self._client._dial(self.owner, self.id, self.token, deadline) diff --git a/sdk/python/cocoonsandbox/template.py b/sdk/python/cocoonsandbox/template.py index 5e468aa0..94db0da6 100644 --- a/sdk/python/cocoonsandbox/template.py +++ b/sdk/python/cocoonsandbox/template.py @@ -3,10 +3,9 @@ from __future__ import annotations import urllib.parse -from collections.abc import Mapping from typing import TYPE_CHECKING, Any -from .volumes import _volume_body +from .volumes import Volume, _volume_body if TYPE_CHECKING: from .client import Client @@ -27,7 +26,7 @@ def __init__(self, client: Client, addr: str, name: str, net: str, size: str, co def new( self, ttl_seconds: int = 0, - volumes: list[str | Mapping[str, str]] | None = None, + volumes: list[str | Volume] | None = None, mount: bool = True, *, deadline: float | None = None, @@ -55,7 +54,7 @@ def _claim_body( net: str, size: str, ttl_seconds: int, - volumes: list[str | Mapping[str, str]] | None = None, + volumes: list[str | Volume] | None = None, mount: bool = True, claim_ref: str = "", ) -> dict[str, Any]: @@ -66,8 +65,10 @@ def _claim_body( claim["size"] = size if ttl_seconds: claim["ttl_seconds"] = ttl_seconds + if volumes is not None and not isinstance(volumes, list): + raise TypeError("volumes must be a list") if volumes: - claim["volumes"] = [_volume_body(volume, mount) for volume in volumes] + claim["volumes"] = [_volume_body(volume) for volume in volumes] if not mount: claim["volumes_attach_only"] = True if claim_ref: diff --git a/sdk/python/cocoonsandbox/volumes.py b/sdk/python/cocoonsandbox/volumes.py index ce156554..209aaa50 100644 --- a/sdk/python/cocoonsandbox/volumes.py +++ b/sdk/python/cocoonsandbox/volumes.py @@ -16,10 +16,11 @@ class _VolumeName(TypedDict): class Volume(_VolumeName, total=False): - """A catalog volume by name, with its guest mount point and an `rw` mode for writable claims.""" + """A catalog volume with a guest mount or caller-managed device attachment.""" mount: str mode: str + attach_only: bool class _VolumeState(TypedDict): @@ -51,29 +52,15 @@ def _volume_response(reply: dict[str, Any] | None) -> SandboxVolumesResponse | N reply is None or not all(isinstance(reply.get(key), list) for key in ("volumes_want", "volumes", "states")) or not isinstance(reply.get("converging"), bool) + or any(not isinstance(entry, dict) for key in ("volumes_want", "volumes") for entry in reply[key]) ): return None return cast(SandboxVolumesResponse, reply) -def _volume_body(volume: str | Mapping[str, str] | Volume, mount: bool) -> dict[str, str]: +def _volume_body(volume: str | Volume) -> Volume: if isinstance(volume, str): return {"name": volume} if not isinstance(volume, Mapping): raise TypeError("volume must be a name string or mapping") - unknown = sorted(set(volume) - {"name", "mount", "mode"}) - if unknown: - raise TypeError( - f"volume mapping accepts only name, mount, and mode, got unexpected key(s): {', '.join(unknown)}" - ) - if not mount and "mount" in volume: - raise TypeError("volume mount is meaningless with mount=False, which leaves mounting to the caller") - body = dict(cast(Mapping[str, str], volume)) - mode = body.get("mode") - if mode in (None, "", "ro"): - body.pop("mode", None) - elif mode != "rw": - name = volume.get("name") - prefix = f"volume {name!r}: " if name is not None else "" - raise TypeError(f"{prefix}mode must be 'rw' or 'ro', got {mode!r}") - return body + return cast(Volume, dict(volume)) diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index 90b799dc..de1e43dc 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -75,8 +75,6 @@ def test_claim_sends_volumes(node): ("volumes", "match"), [ ([("imagenet", "/datasets/imagenet")], "name string or mapping"), - ([{"name": "imagenet", "mode": "rwx"}], "volume 'imagenet': mode must be 'rw' or 'ro', got 'rwx'"), - ([{"name": "imagenet", "bogus": "x"}], r"unexpected key\(s\): bogus"), ], ) def test_claim_rejects_invalid_volumes(node, volumes, match): @@ -100,11 +98,12 @@ def test_claim_sends_volume_mode_rw(node): assert sb.volumes == [{"name": "scratch", "mount": "/data", "mode": "rw"}] -def test_claim_omits_volume_mode_ro(node): +def test_claim_preserves_explicit_volume_mode(node): seen = recording_claim({"id": "sb_1", "token": "tok"}) Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mode": "ro"}]) Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mode": ""}]) - assert seen[0]["volumes"] == seen[1]["volumes"] == [{"name": "imagenet"}], seen + assert seen[0]["volumes"] == [{"name": "imagenet", "mode": "ro"}] + assert seen[1]["volumes"] == [{"name": "imagenet", "mode": ""}] def test_claim_attaches_volumes_without_mounting(node): @@ -130,13 +129,12 @@ def test_template_claim_attaches_volumes_without_mounting(node): assert sb.volumes == [{"name": "imagenet"}] -def test_claim_rejects_mount_without_mounting(node): - with pytest.raises(TypeError, match="meaningless with mount=False"): - Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mount": "/datasets"}], mount=False) - with pytest.raises(TypeError, match="meaningless with mount=False"): - Template(Client(node), node, "task:v1", "none", "small").new( - volumes=[{"name": "imagenet", "mount": "/datasets"}], mount=False - ) +def test_claim_leaves_volume_policy_to_server(node): + seen = recording_claim({"id": "sb_1", "token": "tok"}) + entry = {"name": "imagenet", "mount": "/datasets", "attach_only": True, "mode": "future-mode"} + Client(node).new("rt:24.04", volumes=[entry], mount=False) + Template(Client(node), node, "task:v1", "none", "small").new(volumes=[entry], mount=False) + assert [request["volumes"] for request in seen] == [[entry], [entry]] def test_claim_keeps_mounting_by_default(node): diff --git a/sdk/python/tests/test_volumes.py b/sdk/python/tests/test_volumes.py index 3226b82d..516d1245 100644 --- a/sdk/python/tests/test_volumes.py +++ b/sdk/python/tests/test_volumes.py @@ -1,5 +1,7 @@ """Live volume HTTP contract and partial convergence recovery.""" +from collections import UserDict + import pytest from conftest import FakeNode, sandbox_at @@ -14,8 +16,31 @@ } -@pytest.mark.parametrize("method", ["GET", "PUT", "PATCH"]) -def test_volume_requests_use_owner_and_api_token(spawn_node, method): +def test_volume_mapping_is_json_encoded(spawn_node): + seen = [] + + def reply(body, path): + seen.append(body) + return 200, VIEW + + sb = sandbox_at(spawn_node({("PUT", PATH): reply})) + sb.set_volumes([UserDict({"name": "data", "mount": "/data"})]) + assert seen == [{"volumes": [{"name": "data", "mount": "/data"}]}] + + +@pytest.mark.parametrize( + "method,body,call", + [ + ("GET", {}, lambda sb: sb.get_volumes()), + ("PUT", {"volumes": [{"name": "data"}]}, lambda sb: sb.set_volumes(["data"])), + ( + "PATCH", + {"add": [{"name": "data"}], "remove": ["old"]}, + lambda sb: sb.patch_volumes(add=["data"], remove=["old"]), + ), + ], +) +def test_volume_requests_use_owner_and_api_token(spawn_node, method, body, call): seen = [] def reply(body, path): @@ -25,15 +50,8 @@ def reply(body, path): addr = spawn_node({(method, PATH): reply}) sb = sandbox_at(addr, api_token="operator-token") sb._client.addr = "unused.invalid:7777" - if method == "GET": - got = sb.get_volumes() - assert seen == [{}] - elif method == "PUT": - got = sb.set_volumes(["data"]) - assert seen == [{"volumes": [{"name": "data"}]}] - else: - got = sb.patch_volumes(add=["data"], remove=["old"]) - assert seen == [{"add": [{"name": "data"}], "remove": ["old"]}] + got = call(sb) + assert seen == [body] assert FakeNode.last_headers["Authorization"] == "Bearer operator-token" assert got == VIEW assert sb.volumes == VIEW["volumes"] @@ -43,11 +61,18 @@ def reply(body, path): assert sb.volumes[0]["name"] == sb.volumes_want[0]["name"] == "data" -@pytest.mark.parametrize("method", ["GET", "PUT", "PATCH"]) -def test_volume_error_preserves_typed_partial_view(spawn_node, method): +@pytest.mark.parametrize( + "method,call", + [ + ("GET", lambda sb: sb.get_volumes()), + ("PUT", lambda sb: sb.set_volumes(["new"])), + ("PATCH", lambda sb: sb.patch_volumes(remove=["old"])), + ], +) +def test_volume_error_preserves_typed_partial_view(spawn_node, method, call): partial = { - "volumes_want": [{"name": "new", "mount": "/new"}], - "volumes": [{"name": "old", "mount": "/old"}], + "volumes_want": [{"name": "new", "mode": "rw", "attach_only": True}], + "volumes": [{"name": "old", "attach_only": True}], "converging": False, "states": [ {"name": "new", "state": "pending", "error": "retry"}, @@ -57,12 +82,7 @@ def test_volume_error_preserves_typed_partial_view(spawn_node, method): } sb = sandbox_at(spawn_node({(method, PATH): lambda body, path: (500, partial)})) with pytest.raises(VolumeError) as caught: - if method == "GET": - sb.get_volumes() - elif method == "PUT": - sb.set_volumes(["new"]) - else: - sb.patch_volumes(remove=["old"]) + call(sb) assert isinstance(caught.value, APIError) assert caught.value.status == 500 assert caught.value.response == partial @@ -164,3 +184,38 @@ def test_incomplete_volume_view_preserves_handle(spawn_node, status): def test_volume_methods_reject_a_bare_string(black_hole, call): with pytest.raises(TypeError): call(sandbox_at(black_hole)) + + +def test_volume_mutations_encode_explicit_device_fields(spawn_node): + entries = [ + {"name": "device", "mount": "", "mode": "rw", "attach_only": True}, + {"name": "mounted", "attach_only": False}, + ] + seen = [] + + def reply(body, path): + seen.append(body) + return 200, VIEW + + sb = sandbox_at(spawn_node({("PUT", PATH): reply, ("PATCH", PATH): reply})) + sb.set_volumes(entries) + sb.patch_volumes(add=entries) + assert seen == [{"volumes": entries}, {"add": entries}] + + +@pytest.mark.parametrize("status", [200, 409]) +@pytest.mark.parametrize("field", ["volumes", "volumes_want"]) +def test_malformed_volume_entry_preserves_error_and_handle(spawn_node, status, field): + payload = {**VIEW, "error": "storage unavailable", field: [None]} + sb = sandbox_at(spawn_node({("GET", PATH): lambda body, path: (status, payload)})) + sb.volumes = [{"name": "old"}] + sb.volumes_want = [{"name": "wanted"}] + with pytest.raises(APIError) as caught: + sb.get_volumes() + assert sb.volumes == [{"name": "old"}] + assert sb.volumes_want == [{"name": "wanted"}] + if status == 409: + assert isinstance(caught.value, VolumeError) + assert caught.value.status == 409 + assert caught.value.message == "storage unavailable" + assert caught.value.reply == payload