diff --git a/docs/deploy.md b/docs/deploy.md index f8020f0e..a704a01b 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) | @@ -323,10 +324,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, diff --git a/docs/live-volumes.md b/docs/live-volumes.md index 790e0d21..43fa04cb 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). Live changes remain +limited to mounted read-only entries. + 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. diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index 31a255cd..518cb71f 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 @@ -255,6 +255,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 +357,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 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/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/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/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..0f73ad60 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{}, } } @@ -119,6 +124,10 @@ func (s *claimStore) load() (map[string]*types.Sandbox, error) { 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 +160,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 +230,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 +264,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 +319,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 +363,88 @@ 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) 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 +486,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/pool.go b/sandboxd/pool/pool.go index 9e0124cb..f385c897 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" @@ -116,6 +117,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. @@ -321,16 +325,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 +450,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 +479,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 +604,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..1318da41 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1255,6 +1255,8 @@ type fakeEngine struct { detachVolumeErr 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 +1297,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 +1528,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 { @@ -1762,9 +1770,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..93b666c9 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) } 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..bfbf9a85 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 { @@ -136,13 +143,23 @@ func (m *Manager) resolveVolumes(ctx context.Context, tenant string, 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 +168,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 +177,13 @@ 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. @@ -227,7 +243,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 +265,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 +280,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 +343,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) 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_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..2c71a21a 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) } @@ -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) { diff --git a/sandboxd/pool/volume_mutation.go b/sandboxd/pool/volume_mutation.go index 33dc50e2..8d36045a 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. @@ -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, slices.Concat(held, recovery)) 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 } @@ -139,14 +141,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 +202,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) @@ -267,10 +270,11 @@ func (m *Manager) stageVolumeTarget(ctx context.Context, sb *types.Sandbox, run m.mu.Unlock() return claimSnapshot{}, false, ErrVolumeCapture } - if err := m.reserveVolumes(additions); err != nil { + if err := m.checkResolved(resolved, sb.ID); err != nil { m.mu.Unlock() return claimSnapshot{}, false, err } + m.adoptVolumes(additions) run.holds = append(run.holds, additions...) m.mu.Unlock() cleanErr := m.confirmVolumesClean(resolved) @@ -343,19 +347,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 diff --git a/sandboxd/pool/volume_mutation_test.go b/sandboxd/pool/volume_mutation_test.go index 4f94b749..87ef4c23 100644 --- a/sandboxd/pool/volume_mutation_test.go +++ b/sandboxd/pool/volume_mutation_test.go @@ -853,9 +853,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_retirement_test.go b/sandboxd/pool/volume_retirement_test.go new file mode 100644 index 00000000..e72d1ed0 --- /dev/null +++ b/sandboxd/pool/volume_retirement_test.go @@ -0,0 +1,336 @@ +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) { + 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{{Name: "home", Mount: "/home"}} + 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 != (volumeHolders{readers: 1}) { + 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..fda04c98 100644 --- a/sandboxd/pool/volume_rw_test.go +++ b/sandboxd/pool/volume_rw_test.go @@ -152,10 +152,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 +219,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 +581,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/server.go b/sandboxd/server/server.go index 78b44fd6..70279f63 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, ""}, @@ -97,6 +99,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 +273,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)) 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_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..a92c2afd 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"`