From c9977df43ced20971825b906407011d71a07e895 Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 12 Aug 2026 20:41:58 +0800 Subject: [PATCH 01/10] sandboxd: writable catalog volumes with dirty markers and unmount quiesce --- sandboxd/config/config.go | 4 +- sandboxd/engine/volume.go | 41 +++- sandboxd/engine/volume_test.go | 101 ++++++--- sandboxd/pool/claim.go | 41 +++- sandboxd/pool/journal.go | 11 +- sandboxd/pool/pool.go | 18 +- sandboxd/pool/pool_test.go | 46 +++- sandboxd/pool/reconcile.go | 2 + sandboxd/pool/volume.go | 200 ++++++++++++++++- sandboxd/pool/volume_rw_test.go | 369 ++++++++++++++++++++++++++++++++ sandboxd/server/server.go | 2 + sandboxd/server/server_test.go | 2 + sandboxd/types/api.go | 3 + sandboxd/types/types.go | 37 +++- sandboxd/types/volume_test.go | 30 +++ 15 files changed, 836 insertions(+), 71 deletions(-) create mode 100644 sandboxd/pool/volume_rw_test.go diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index 61addf8..fdfb3f6 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -112,11 +112,13 @@ type TenantSpec struct { Egress *egress.Policy `json:"egress,omitempty"` } -// VolumeSpec declares one operator-managed read-only dataset disk. +// VolumeSpec declares one operator-managed dataset disk. Writable admits rw +// claims of this image; a writable entry is held by exactly one node. type VolumeSpec struct { Name string `json:"name"` Path string `json:"path"` DirectIO string `json:"directio,omitempty"` + Writable bool `json:"writable,omitempty"` Tenants []string `json:"tenants,omitempty"` } diff --git a/sandboxd/engine/volume.go b/sandboxd/engine/volume.go index 6d520b0..0347692 100644 --- a/sandboxd/engine/volume.go +++ b/sandboxd/engine/volume.go @@ -13,9 +13,10 @@ import ( ) const ( - volumePollInterval = 10 * time.Millisecond - volumeProbeTimeout = 2 * time.Second - volumeSetupTimeout = 10 * time.Second + volumePollInterval = 10 * time.Millisecond + volumeProbeTimeout = 2 * time.Second + volumeSetupTimeout = 10 * time.Second + volumeUmountTimeout = 2 * time.Second ) // VolumeSpec describes one operator-owned disk image attached to a sandbox. @@ -23,6 +24,7 @@ type VolumeSpec struct { Name string Path string DirectIO string + RW bool } func (s VolumeSpec) directIO() (string, error) { @@ -33,7 +35,7 @@ func (s VolumeSpec) directIO() (string, error) { return mode, nil } -// DiskAttach hot-attaches an operator-owned disk read-only through cocoon. +// DiskAttach hot-attaches an operator-owned disk through cocoon. func (e *Engine) DiskAttach(ctx context.Context, vmName string, spec VolumeSpec) error { args, err := e.diskAttachArgs(vmName, spec) if err != nil { @@ -43,8 +45,8 @@ func (e *Engine) DiskAttach(ctx context.Context, vmName string, spec VolumeSpec) return err } -// MountVolume discovers and mounts a hot-attached disk read-only at mount. -func (e *Engine) MountVolume(ctx context.Context, vsockSocket, name, mount string) error { +// MountVolume discovers and mounts a hot-attached disk at mount, read-only unless rw. +func (e *Engine) MountVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error { ctx, cancel := context.WithTimeout(ctx, volumeSetupTimeout) defer cancel() device, err := e.waitForVolumeDevice(ctx, vsockSocket, name) @@ -54,12 +56,27 @@ func (e *Engine) MountVolume(ctx context.Context, vsockSocket, name, mount strin if err := e.silkdExec(ctx, vsockSocket, "mkdir", "-p", "--", mount); err != nil { return fmt.Errorf("create volume mount point %s: %w", mount, err) } - if err := e.silkdExec(ctx, vsockSocket, "mount", "-o", "ro", "--", device, mount); err != nil { + mode := types.VolumeModeRO + if rw { + mode = types.VolumeModeRW + } + if err := e.silkdExec(ctx, vsockSocket, "mount", "-o", mode, "--", device, mount); err != nil { return fmt.Errorf("mount volume %s: %w", name, err) } return nil } +// UnmountVolume flushes and detaches a guest mount, so a writable image's +// dirty state reaches the backing file before the VM is removed. +func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, mount string) error { + ctx, cancel := context.WithTimeout(ctx, volumeUmountTimeout) + defer cancel() + if err := e.silkdExec(ctx, vsockSocket, "umount", "--", mount); err != nil { + return fmt.Errorf("unmount volume %s: %w", mount, err) + } + return nil +} + func (e *Engine) waitForVolumeDevice(ctx context.Context, vsockSocket, name string) (string, error) { ctx, cancel := context.WithTimeout(ctx, volumeProbeTimeout) defer cancel() @@ -107,11 +124,13 @@ func (e *Engine) diskAttachArgs(vmName string, spec VolumeSpec) ([]string, error if err != nil { return nil, err } - return []string{ + args := []string{ "vm", "disk", "attach", vmName, "--path", spec.Path, argName, spec.Name, - "--readonly", - "--directio", directIO, - }, nil + } + if !spec.RW { + args = append(args, "--readonly") + } + return append(args, "--directio", directIO), nil } diff --git a/sandboxd/engine/volume_test.go b/sandboxd/engine/volume_test.go index 2ad84f9..4c07aec 100644 --- a/sandboxd/engine/volume_test.go +++ b/sandboxd/engine/volume_test.go @@ -11,20 +11,22 @@ import ( "github.com/cocoonstack/sandbox/protocol/wire" ) -func TestDiskAttachArgsReadOnlyAndDirectIO(t *testing.T) { +func TestDiskAttachArgsModeAndDirectIO(t *testing.T) { for _, tt := range []struct { name string directIO string + rw bool wantIO string }{ - {"default buffered", "", "off"}, - {"direct", "on", "on"}, - {"auto", "auto", "auto"}, + {"default buffered", "", false, "off"}, + {"direct", "on", false, "on"}, + {"auto", "auto", false, "auto"}, + {"writable", "", true, "off"}, } { t.Run(tt.name, func(t *testing.T) { e := New("cocoon", nil, nil, false, "") args, err := e.diskAttachArgs("sbx-1", VolumeSpec{ - Name: "imagenet", Path: "/srv/datasets/imagenet.img", DirectIO: tt.directIO, + Name: "imagenet", Path: "/srv/datasets/imagenet.img", DirectIO: tt.directIO, RW: tt.rw, }) if err != nil { t.Fatalf("diskAttachArgs: %v", err) @@ -33,9 +35,11 @@ func TestDiskAttachArgsReadOnlyAndDirectIO(t *testing.T) { "vm", "disk", "attach", "sbx-1", "--path", "/srv/datasets/imagenet.img", "--name", "imagenet", - "--readonly", - "--directio", tt.wantIO, } + if !tt.rw { + want = append(want, "--readonly") + } + want = append(want, "--directio", tt.wantIO) if !slices.Equal(args, want) { t.Errorf("args = %v, want %v", args, want) } @@ -50,35 +54,70 @@ func TestDiskAttachArgsRejectBadOptions(t *testing.T) { } } -func TestMountVolumeUsesSysfsAndReadOnlyMount(t *testing.T) { +func TestMountVolumeUsesSysfsAndRequestedMode(t *testing.T) { + for _, tt := range []struct { + name string + rw bool + wantMode string + }{ + {"read-only", false, "ro"}, + {"writable", true, "rw"}, + } { + t.Run(tt.name, func(t *testing.T) { + path := sockPath(t) + fake := serveFakeSilkd(t, path) + configureVolumeDevices(fake) + if err := New("cocoon", nil, nil, false, "").MountVolume( + t.Context(), path, "imagenet", "/datasets/training", tt.rw, + ); err != nil { + t.Fatalf("MountVolume: %v", err) + } + + fake.mu.Lock() + defer fake.mu.Unlock() + wantExec := [][]string{ + {"mkdir", "-p", "--", "/datasets/training"}, + {"mount", "-o", tt.wantMode, "--", "/dev/vdc", "/datasets/training"}, + } + if !slices.EqualFunc(fake.execCalls, wantExec, slices.Equal) { + t.Errorf("exec calls = %v, want %v", fake.execCalls, wantExec) + } + wantReads := []string{ + "/sys/block/vda/serial", + "/sys/block/vdb/serial", + "/sys/block/vdc/serial", + } + if fake.listCalls != 1 || !slices.Equal(fake.readCalls, wantReads) { + t.Errorf("sysfs calls = list:%d read:%v, want list:1 read:%v", fake.listCalls, fake.readCalls, wantReads) + } + if fake.execEnv["PATH"] == "" { + t.Error("guest exec PATH is empty") + } + }) + } +} + +func TestUnmountVolumeExecsBoundedUmount(t *testing.T) { + if volumeUmountTimeout != 2*time.Second { + t.Fatalf("volume umount timeout = %s, want 2s", volumeUmountTimeout) + } path := sockPath(t) fake := serveFakeSilkd(t, path) - configureVolumeDevices(fake) - if err := New("cocoon", nil, nil, false, "").MountVolume( - t.Context(), path, "imagenet", "/datasets/training", - ); err != nil { - t.Fatalf("MountVolume: %v", err) + e := New("cocoon", nil, nil, false, "") + if err := e.UnmountVolume(t.Context(), path, "/datasets/training"); err != nil { + t.Fatalf("UnmountVolume: %v", err) } fake.mu.Lock() - defer fake.mu.Unlock() - wantExec := [][]string{ - {"mkdir", "-p", "--", "/datasets/training"}, - {"mount", "-o", "ro", "--", "/dev/vdc", "/datasets/training"}, - } + wantExec := [][]string{{"umount", "--", "/datasets/training"}} if !slices.EqualFunc(fake.execCalls, wantExec, slices.Equal) { t.Errorf("exec calls = %v, want %v", fake.execCalls, wantExec) } - wantReads := []string{ - "/sys/block/vda/serial", - "/sys/block/vdb/serial", - "/sys/block/vdc/serial", - } - if fake.listCalls != 1 || !slices.Equal(fake.readCalls, wantReads) { - t.Errorf("sysfs calls = list:%d read:%v, want list:1 read:%v", fake.listCalls, fake.readCalls, wantReads) - } - if fake.execEnv["PATH"] == "" { - t.Error("guest exec PATH is empty") + fake.execCode, fake.execFailAt = 32, 2 + fake.mu.Unlock() + err := e.UnmountVolume(t.Context(), path, "/datasets/training") + if err == nil || !strings.Contains(err.Error(), "unmount volume /datasets/training") { + t.Errorf("got %v, want umount failure", err) } } @@ -90,7 +129,7 @@ func TestMountVolumeWaitsForDelayedSysfsSerial(t *testing.T) { fake.readMisses["/sys/block/vdc/serial"] = 1 fake.mu.Unlock() if err := New("cocoon", nil, nil, false, "").MountVolume( - t.Context(), path, "imagenet", "/datasets/training", + t.Context(), path, "imagenet", "/datasets/training", false, ); err != nil { t.Fatalf("MountVolume: %v", err) } @@ -125,7 +164,7 @@ func TestMountVolumeStopsAtFailedStage(t *testing.T) { tt.prepare(fake) fake.mu.Unlock() err := New("cocoon", nil, nil, false, "").MountVolume( - t.Context(), path, "imagenet", "/datasets/training", + t.Context(), path, "imagenet", "/datasets/training", false, ) if err == nil || !strings.Contains(err.Error(), tt.wantErr) { t.Errorf("got %v, want %q failure", err, tt.wantErr) @@ -149,7 +188,7 @@ func TestMountVolumeDeviceProbeIsBoundedAndCancelable(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) cancel() err := New("cocoon", nil, nil, false, "").MountVolume( - ctx, path, "missing", "/datasets/training", + ctx, path, "missing", "/datasets/training", false, ) if !errors.Is(err, context.Canceled) { t.Errorf("error = %v, want context cancellation", err) diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index d4b9b14..7c3dfb6 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -37,7 +37,16 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur if quotaErr := m.overQuota(1, tenant); quotaErr != nil { return nil, quotaErr } + applied := appliedVolumes(volumeSpecs) + // Holds belong to this path until finalize; past it the sandbox carries them. + var reserved []types.Volume + defer func() { m.unreserveVolumes(reserved) }() m.mu.Lock() + if reserveErr := m.reserveVolumes(applied); reserveErr != nil { + m.mu.Unlock() + return nil, reserveErr + } + reserved = applied var sb *types.Sandbox if p := m.pools[key]; p != nil { p.noteArrival(start) @@ -57,6 +66,7 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur } sb.Tenant = tenant sb.ClaimRef = claimRef + reserved = nil out, err := m.finalize(ctx, sb, ttl) if err == nil { m.counters.claimsWarm.Add(1) @@ -179,6 +189,7 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM m.untrack(m.pendingCks, ck) } + m.teardownVolumes(ctx, sb, true) var err error if vmName != "" && !m.removeOrRetry(ctx, vmName, id, "") { err = fmt.Errorf("vm %s survived removal", vmName) @@ -198,6 +209,18 @@ func (m *Manager) overQuota(extra int, tenant string) error { return m.quotaErr(extra, tenant) } +// admitClaim is the provision path's one admission section: the advisory quota +// precheck plus the authoritative volume reservation, so a busy volume is +// refused before any VM is built. +func (m *Manager) admitClaim(tenant string, volumes []types.Volume) error { + m.mu.Lock() + defer m.mu.Unlock() + if err := m.quotaErr(1, tenant); err != nil { + return err + } + return m.reserveVolumes(volumes) +} + // quotaErr answers ErrQuota over the node cap, the tenant cap, or a // draining node; callers hold m.mu. func (m *Manager) quotaErr(extra int, tenant string) error { @@ -251,6 +274,9 @@ func (m *Manager) finalizeBatch(ctx context.Context, sbs []*types.Sandbox, ttl t if quotaErr := m.quotaErr(len(sbs), sbs[0].Tenant); quotaErr != nil { m.mu.Unlock() for _, sb := range sbs { + // No quiesce: the claim was never handed out, so nothing wrote to + // the guest and the marker converges on the next writable claim. + m.teardownVolumes(ctx, sb, false) m.destroy(ctx, sb.VMName) } return quotaErr @@ -277,7 +303,8 @@ func (m *Manager) finalizeBatch(ctx context.Context, sbs []*types.Sandbox, ttl t m.recordUsage(ctx, usageEvent{ Event: "claim", //nolint:goconst // event name; other occurrences are test assertions ID: sb.ID, VMName: sb.VMName, - KeyHash: sb.Key.Hash(), Tenant: sb.Tenant, Volumes: types.VolumeNames(sb.Volumes), + KeyHash: sb.Key.Hash(), Tenant: sb.Tenant, + Volumes: types.VolumeNames(sb.Volumes), VolumesRW: types.VolumeRWNames(sb.Volumes), }) } return nil @@ -296,6 +323,7 @@ func (m *Manager) rollbackClaim(ctx context.Context, sbs []*types.Sandbox) { m.mu.Unlock() m.recommit(ctx, rb) for _, sb := range sbs { + m.teardownVolumes(ctx, sb, true) m.disarmEgress(sb.ID, m.removeOrRetry(ctx, sb.VMName, sb.ID, "")) } } @@ -408,6 +436,7 @@ func (m *Manager) reapOnce(ctx context.Context) { case reapArchive: logSweepResult(ctx, logger, m.archive(ctx, v.sb), "archived expired sandbox "+v.id, "archive expired sandbox "+v.id) default: + m.teardownVolumes(ctx, v.sb, true) m.disarmEgress(v.id, m.removeOrRetry(ctx, v.vmName, v.id, "")) m.dropSnap(ctx, v.snap) m.counters.reaps.Add(1) @@ -470,9 +499,14 @@ func (m *Manager) claimProvision(ctx context.Context, key types.PoolKey, ttl tim if err != nil { return nil, err } - if quotaErr := m.overQuota(1, tenant); quotaErr != nil { - return nil, quotaErr + applied := appliedVolumes(volumeSpecs) + // Holds belong to this path until finalize; past it the sandbox carries them. + var reserved []types.Volume + defer func() { m.unreserveVolumes(reserved) }() + if admitErr := m.admitClaim(tenant, applied); admitErr != nil { + return nil, admitErr } + reserved = applied golden, err := m.resolveGolden(ctx, key) if err != nil { return nil, fmt.Errorf("resolve template: %w", err) @@ -493,6 +527,7 @@ func (m *Manager) claimProvision(ctx context.Context, key types.PoolKey, ttl tim sb.TemplateDigest = golden.templateDigest sb.Tenant = tenant sb.ClaimRef = claimRef + reserved = nil out, err := m.finalize(ctx, sb, ttl) if err == nil { if golden.dir != "" { diff --git a/sandboxd/pool/journal.go b/sandboxd/pool/journal.go index df64022..96ff9fd 100644 --- a/sandboxd/pool/journal.go +++ b/sandboxd/pool/journal.go @@ -21,11 +21,12 @@ type usageEvent struct { Event string `json:"ev"` // claim|hibernate|wake|fork|promote|checkpoint|release|reap ID string `json:"id"` VMName string `json:"vm,omitempty"` - KeyHash string `json:"key,omitempty"` // claim only - Tenant string `json:"tenant,omitempty"` // claim only - Volumes []string `json:"volumes,omitempty"` // claim only - Children []string `json:"children,omitempty"` // fork only - Reference string `json:"ref,omitempty"` // promote: template; checkpoint: ckpt id + KeyHash string `json:"key,omitempty"` // claim only + Tenant string `json:"tenant,omitempty"` // claim only + Volumes []string `json:"volumes,omitempty"` // claim only + VolumesRW []string `json:"volumes_rw,omitempty"` // claim only: the write-enabled subset + Children []string `json:"children,omitempty"` // fork only + Reference string `json:"ref,omitempty"` // promote: template; checkpoint: ckpt id } // journal is an append-only JSONL writer with size rotation. Writes happen diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 07525ae..a5e47d8 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -93,7 +93,10 @@ var ( ErrNoEgressHibernate = errors.New("egress-lane sandboxes do not hibernate") ErrNoEgressFork = errors.New("egress-lane sandboxes cannot fork, checkpoint, or promote: a resumed guest egresses before its fresh tap can be locked") ErrVolumeCapture = errors.New("sandboxes with volumes cannot hibernate, fork, checkpoint, or promote") - ErrQuota = errors.New("node claim quota reached") + ErrVolumeBusy = errors.New("volume is held by another claim") + // Replaying a journal takes a writable mount, so readers stay out. + ErrVolumeNeedsRecovery = errors.New("volume needs recovery by a writable claim") + ErrQuota = errors.New("node claim quota reached") errWokeMeanwhile = errors.New("woke between sweep and hibernate") errNoEgressTap = errors.New("egress-lane claim has no lockable tap") @@ -117,7 +120,8 @@ type Engine interface { DialGuestPort(ctx context.Context, vsockSocket string, port uint16) (net.Conn, error) InstallCACert(ctx context.Context, vsockSocket string, certPEM []byte) error DiskAttach(ctx context.Context, vmName string, spec engine.VolumeSpec) error - MountVolume(ctx context.Context, vsockSocket, name, mount string) error + MountVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error + UnmountVolume(ctx context.Context, vsockSocket, mount string) error } // SandboxSummary is the ops view of one live claim — no tokens. @@ -245,6 +249,10 @@ type Manager struct { maxFork int store *claimStore volumes map[string]catalogVolume + // volumeAdmission counts the live holders of each volume name, mirroring + // the hypervisor's own per-image lock so a conflicting claim is refused + // before it pays any attach cost; guarded by m.mu. + volumeAdmission map[string]volumeHolders poolStore *poolStore configSeedHash string // config pools' hash, to warn when a file edit is overridden @@ -382,6 +390,7 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg maxFork: maxFork, store: newClaimStore(cfg.DataDir), volumes: make(map[string]catalogVolume, len(cfg.Volumes)), + volumeAdmission: map[string]volumeHolders{}, poolStore: newPoolStore(cfg.DataDir), pools: make(map[types.PoolKey]*pool, len(cfg.Pools)), claimed: map[string]*types.Sandbox{}, @@ -405,8 +414,9 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg } for _, volume := range cfg.Volumes { m.volumes[volume.Name] = catalogVolume{ - disk: engine.VolumeSpec{Name: volume.Name, Path: volume.Path, DirectIO: volume.DirectIO}, - tenants: slices.Clone(volume.Tenants), + disk: engine.VolumeSpec{Name: volume.Name, Path: volume.Path, DirectIO: volume.DirectIO}, + tenants: slices.Clone(volume.Tenants), + writable: volume.Writable, } } if err := os.MkdirAll(m.goldensDir(), 0o750); err != nil { diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 9247688..b574473 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -765,6 +765,11 @@ type fakeEngine struct { volumeSpecs []engine.VolumeSpec volumeMounts []types.Volume volumeOps []string + // attachDirty records whether an image's dirty marker was already down + // when it was attached; removeSeenOps snapshots the volume-op log at each + // VM removal, so teardown ordering is checkable across the two logs. + attachDirty map[string]bool + removeSeenOps map[string][]string hibernates, restores, snapRemoves []string snapSaves, exports, snapshots []string @@ -775,6 +780,7 @@ type fakeEngine struct { diskAttachErr error diskAttachCancel context.CancelFunc mountVolumeErr error + unmountVolumeErr error stopped map[string]bool creating map[string]bool // VMs List reports in the creating state staleOutcome engine.StaleCreateOutcome @@ -799,7 +805,10 @@ type fakeEngine struct { } func newFakeEngine() *fakeEngine { - return &fakeEngine{vms: map[string]string{}, stopped: map[string]bool{}, creating: map[string]bool{}, pids: map[string]int{}} + return &fakeEngine{ + vms: map[string]string{}, stopped: map[string]bool{}, creating: map[string]bool{}, pids: map[string]int{}, + attachDirty: map[string]bool{}, removeSeenOps: map[string][]string{}, + } } func (f *fakeEngine) Clone(_ context.Context, fromDir, name string, _ types.PoolKey) (types.VMRecord, error) { @@ -838,6 +847,7 @@ func (f *fakeEngine) Remove(ctx context.Context, name string) error { if name == f.removeErrFor { return errors.New("remove failed") } + f.removeSeenOps[name] = slices.Clone(f.volumeOps) f.removes = append(f.removes, name) delete(f.vms, name) return nil @@ -1000,20 +1010,32 @@ func (f *fakeEngine) DiskAttach(_ context.Context, _ string, spec engine.VolumeS defer f.mu.Unlock() f.volumeSpecs = append(f.volumeSpecs, spec) f.volumeOps = append(f.volumeOps, "attach:"+spec.Name) + f.attachDirty[spec.Name] = volumeDirty(spec.Path) if f.diskAttachCancel != nil { f.diskAttachCancel() } return f.diskAttachErr } -func (f *fakeEngine) MountVolume(_ context.Context, _, name, mount string) error { +func (f *fakeEngine) MountVolume(_ context.Context, _, name, mount string, rw bool) error { f.mu.Lock() defer f.mu.Unlock() - f.volumeMounts = append(f.volumeMounts, types.Volume{Name: name, Mount: mount}) + mode := "" + if rw { + mode = types.VolumeModeRW + } + f.volumeMounts = append(f.volumeMounts, types.Volume{Name: name, Mount: mount, Mode: mode}) f.volumeOps = append(f.volumeOps, "mount:"+name+":"+mount) return f.mountVolumeErr } +func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.volumeOps = append(f.volumeOps, "umount:"+mount) + return f.unmountVolumeErr +} + func (f *fakeEngine) clone(from, name string) (types.VMRecord, error) { f.mu.Lock() f.clones = append(f.clones, name) @@ -1064,6 +1086,24 @@ func (f *fakeEngine) removed(name string) bool { return slices.Contains(f.removes, name) } +func (f *fakeEngine) dirtyAtAttach(name string) bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.attachDirty[name] +} + +func (f *fakeEngine) opsAtRemoval(vmName string) []string { + f.mu.Lock() + defer f.mu.Unlock() + return f.removeSeenOps[vmName] +} + +func (f *fakeEngine) volumeOpsLog() []string { + f.mu.Lock() + defer f.mu.Unlock() + return slices.Clone(f.volumeOps) +} + func (f *fakeEngine) listCalls() int { f.mu.Lock() defer f.mu.Unlock() diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index 00d06d5..0cd520b 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -71,6 +71,7 @@ func (m *Manager) Reconcile(ctx context.Context) error { } m.claimed[id] = sb m.tenantDelta(sb.Tenant, 1) + m.adoptVolumes(sb.Volumes) owned[sb.VMName] = true referenced[sb.HibernateSnap] = true } @@ -218,6 +219,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco // A failed remove stays out of service and queued until teardown succeeds. func (m *Manager) quarantineClaim(ctx context.Context, sb *types.Sandbox) bool { + m.teardownVolumes(ctx, sb, true) gone := m.removeOrRetry(ctx, sb.VMName, sb.ID, "") m.mu.Lock() delete(m.claimed, sb.ID) diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index 30e22da..44294cc 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -2,24 +2,45 @@ package pool import ( "context" + "errors" "fmt" "os" + "path/filepath" "slices" "strings" + "time" + + "github.com/projecteru2/core/log" "github.com/cocoonstack/sandbox/sandboxd/engine" "github.com/cocoonstack/sandbox/sandboxd/types" ) +const ( + // Sidecar, not data_dir: the marker travels with the image and survives a + // data_dir wipe. + volumeDirtySuffix = ".dirty" + // Bounds the whole quiesce: teardown must not hang on a wedged guest. + volumeQuiesceTimeout = 5 * time.Second +) + type catalogVolume struct { - disk engine.VolumeSpec - tenants []string + disk engine.VolumeSpec + tenants []string + writable bool } func (v catalogVolume) allowed(tenant string) bool { return tenant == "" || len(v.tenants) == 0 || slices.Contains(v.tenants, tenant) } +// volumeHolders is one name's live admission state: a writer excludes every +// other claim, readers only exclude a writer. +type volumeHolders struct { + writers int + readers int +} + type resolvedVolume struct { disk engine.VolumeSpec applied types.Volume @@ -33,7 +54,12 @@ func (m *Manager) Volumes(tenant string, holders map[string]int) []types.VolumeI if !volume.allowed(tenant) { continue } - info := types.VolumeInfo{Name: name, DefaultMount: types.DefaultVolumeMount(name), Nodes: holders[name]} + info := types.VolumeInfo{ + Name: name, + DefaultMount: types.DefaultVolumeMount(name), + Nodes: holders[name], + Writable: volume.writable, + } if st, err := os.Stat(volume.disk.Path); err == nil { info.SizeBytes = st.Size() info.Available = true @@ -116,10 +142,19 @@ func (m *Manager) resolveVolumes(key types.PoolKey, tenant string, requested []t if !ok || !entry.allowed(tenant) { return nil, ErrVolumeUnavailable } + if volume.RW() && !entry.writable { + return nil, fmt.Errorf("%w: volume %q is not writable", ErrBadVolume, volume.Name) + } if _, statErr := os.Stat(entry.disk.Path); statErr != nil { return nil, fmt.Errorf("volume %q path %q: %w", volume.Name, entry.disk.Path, statErr) } - resolved = append(resolved, resolvedVolume{disk: entry.disk, applied: volume}) + // A live writer's own marker is expected; admission answers that conflict. + if !volume.RW() && volumeDirty(entry.disk.Path) && !m.volumeHeld(volume.Name) { + return nil, fmt.Errorf("%w: volume %q", ErrVolumeNeedsRecovery, volume.Name) + } + disk := entry.disk + disk.RW = volume.RW() + resolved = append(resolved, resolvedVolume{disk: disk, applied: volume}) } return resolved, nil } @@ -128,16 +163,165 @@ func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes [ if len(volumes) == 0 { return nil } - applied := make([]types.Volume, len(volumes)) - for i, volume := range volumes { + for _, volume := range volumes { + // Write-ahead: the marker must be durable before any guest write can be. + if volume.disk.RW { + if err := markVolumeDirty(volume.disk.Path); err != nil { + return fmt.Errorf("mark volume %q dirty: %w", volume.applied.Name, err) + } + } if err := m.eng.DiskAttach(ctx, sb.VMName, volume.disk); err != nil { return fmt.Errorf("attach volume %q: %w", volume.applied.Name, err) } - if err := m.eng.MountVolume(ctx, sb.VsockSocket, volume.applied.Name, volume.applied.Mount); err != nil { + if err := m.eng.MountVolume(ctx, sb.VsockSocket, volume.applied.Name, volume.applied.Mount, volume.disk.RW); err != nil { return fmt.Errorf("setup volume %q: %w", volume.applied.Name, err) } + } + sb.Volumes = appliedVolumes(volumes) + return nil +} + +// teardownVolumes quiesces the guest (paths that still own a live VM, before +// it is removed) and releases the admission holds. Exactly once per claim: a +// leaked hold keeps the name unclaimable until the daemon restarts. +func (m *Manager) teardownVolumes(ctx context.Context, sb *types.Sandbox, quiesce bool) { + if len(sb.Volumes) == 0 { + return + } + if quiesce { + m.quiesceVolumes(ctx, sb) + } + m.unreserveVolumes(sb.Volumes) +} + +// quiesceVolumes unmounts the writable mounts in reverse order, clearing the +// marker of each image that unmounted cleanly. A failure never blocks +// teardown: the surviving marker routes the image to a recovering writer. +func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) { + if !slices.ContainsFunc(sb.Volumes, types.Volume.RW) { + return + } + logger := log.WithFunc("pool.quiesceVolumes") + // Cancellation-immune like removal: a caller hanging up must not skip the flush. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), volumeQuiesceTimeout) + defer cancel() + for _, volume := range slices.Backward(sb.Volumes) { + if !volume.RW() { + continue + } + if err := m.eng.UnmountVolume(ctx, sb.VsockSocket, volume.Mount); err != nil { + logger.Errorf(ctx, err, "unmount volume %s of %s", volume.Name, sb.ID) + continue + } + entry, ok := m.volumes[volume.Name] + if !ok { + continue + } + if err := clearVolumeDirty(entry.disk.Path); err != nil { + logger.Errorf(ctx, err, "clear dirty marker of volume %s", volume.Name) + } + } +} + +// reserveVolumes admits one claim's volumes; every name is checked before any +// is taken, so a refusal leaves the registry untouched. 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.adoptVolumes(volumes) + return nil +} + +// adoptVolumes counts volumes an adopted claim already holds, with no +// admission check. Callers hold m.mu. +func (m *Manager) adoptVolumes(volumes []types.Volume) { + for _, volume := range volumes { + holders := m.volumeAdmission[volume.Name] + if volume.RW() { + holders.writers++ + } else { + holders.readers++ + } + m.volumeAdmission[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] + if volume.RW() { + holders.writers-- + } else { + holders.readers-- + } + if holders.writers < 1 && holders.readers < 1 { + delete(m.volumeAdmission, volume.Name) + continue + } + m.volumeAdmission[volume.Name] = holders + } +} + +func (m *Manager) volumeHeld(name string) bool { + m.mu.Lock() + defer m.mu.Unlock() + return m.volumeAdmission[name].writers > 0 +} + +func (m *Manager) unreserveVolumes(volumes []types.Volume) { + if len(volumes) == 0 { + return + } + m.mu.Lock() + defer m.mu.Unlock() + m.releaseVolumes(volumes) +} + +func appliedVolumes(volumes []resolvedVolume) []types.Volume { + applied := make([]types.Volume, len(volumes)) + for i, volume := range volumes { applied[i] = volume.applied } - sb.Volumes = applied + return applied +} + +func markVolumeDirty(path string) error { + f, err := os.OpenFile(volumeDirtyPath(path), os.O_CREATE|os.O_WRONLY, 0o644) //nolint:gosec // sidecar of an operator-configured image path + if err != nil { + return err + } + if err := f.Close(); err != nil { + return err + } + return syncDir(filepath.Dir(path)) +} + +func clearVolumeDirty(path string) error { + if err := os.Remove(volumeDirtyPath(path)); err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + _ = syncDir(filepath.Dir(path)) // a lost removal re-converges on the next clean release return nil } + +func volumeDirty(path string) bool { + _, err := os.Stat(volumeDirtyPath(path)) + return err == nil +} + +func volumeDirtyPath(path string) string { + return path + volumeDirtySuffix +} + +func syncDir(dir string) error { + d, err := os.Open(dir) //nolint:gosec // parent of an operator-configured image path + if err != nil { + return err + } + return errors.Join(d.Sync(), d.Close()) +} diff --git a/sandboxd/pool/volume_rw_test.go b/sandboxd/pool/volume_rw_test.go new file mode 100644 index 0000000..7da5c44 --- /dev/null +++ b/sandboxd/pool/volume_rw_test.go @@ -0,0 +1,369 @@ +package pool + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestWritableClaimMarksDirtyBeforeAttachAndClearsOnRelease(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + + sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + if err != nil { + t.Fatalf("ClaimProvision: %v", err) + } + want := []types.Volume{{Name: "scratch", Mount: "/volumes/scratch", Mode: types.VolumeModeRW}} + if !slices.Equal(sb.Volumes, want) { + t.Errorf("applied volumes=%v, want %v", sb.Volumes, want) + } + if !eng.dirtyAtAttach("scratch") { + t.Error("attach ran before the dirty marker was durable") + } + if specs := eng.volumeSpecs; len(specs) != 1 || !specs[0].RW { + t.Errorf("attached specs=%+v, want one writable disk", specs) + } + if !slices.Equal(eng.volumeMounts, want) { + t.Errorf("mounts=%v, want %v", eng.volumeMounts, want) + } + if !volumeDirty(path) { + t.Error("live writable claim left no dirty marker") + } + + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatalf("Release: %v", err) + } + if volumeDirty(path) { + t.Error("clean release left the image dirty") + } + if seen := eng.opsAtRemoval(sb.VMName); !slices.Contains(seen, "umount:/volumes/scratch") { + t.Errorf("ops at removal=%v, want the unmount already done", seen) + } + if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{}) { + t.Errorf("registry after release=%+v, want empty", holders) + } +} + +func TestClaimRejectsWriteOnReadOnlyEntry(t *testing.T) { + readOnly := writeVolumeImage(t, "data.img", "data") + writable := writeVolumeImage(t, "scratch.img", "scratch") + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{ + {Name: "data", Path: readOnly}, + {Name: "scratch", Path: writable, Writable: true}, + }) + + _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}) + if !errors.Is(err, ErrBadVolume) || !strings.Contains(err.Error(), "not writable") { + t.Errorf("error=%v, want a not-writable ErrBadVolume", err) + } + if ops := eng.volumeOpsLog(); len(ops) != 0 { + t.Errorf("rejected claim ran %v", ops) + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}); err != nil { + t.Errorf("read-only claim of a writable entry: %v", err) + } + if volumeDirty(writable) { + t.Error("read-only claim marked the image dirty") + } +} + +func TestDirtyVolumeBlocksReadersUntilWriterRecovers(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + if err := markVolumeDirty(path); err != nil { + t.Fatalf("mark dirty: %v", err) + } + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + readOnly := []types.Volume{{Name: "scratch"}} + + _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", readOnly) + if !errors.Is(err, ErrVolumeNeedsRecovery) { + t.Errorf("read-only claim of a dirty image: %v, want ErrVolumeNeedsRecovery", err) + } + if ops := eng.volumeOpsLog(); len(ops) != 0 { + t.Errorf("refused claim ran %v", ops) + } + sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + if err != nil { + t.Fatalf("writable recovery claim: %v", err) + } + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatalf("Release: %v", err) + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", readOnly); err != nil { + t.Errorf("read-only claim after recovery: %v", err) + } +} + +func TestVolumeAdmissionMatrix(t *testing.T) { + for _, tt := range []struct { + name string + first, second string + wantBusy bool + }{ + {"writer excludes writer", types.VolumeModeRW, types.VolumeModeRW, true}, + {"writer excludes reader", types.VolumeModeRW, "", true}, + {"reader excludes writer", "", types.VolumeModeRW, true}, + {"readers share", "", "", false}, + } { + t.Run(tt.name, func(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + second := []types.Volume{{Name: "scratch", Mode: tt.second}} + + first, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch", Mode: tt.first}}) + if err != nil { + t.Fatalf("first claim: %v", err) + } + before := eng.volumeOpsLog() + _, err = m.ClaimProvision(t.Context(), testKey, 0, "", "", second) + if !tt.wantBusy { + if err != nil { + t.Fatalf("concurrent read-only claim: %v", err) + } + return + } + if !errors.Is(err, ErrVolumeBusy) { + t.Fatalf("second claim: %v, want ErrVolumeBusy", err) + } + if ops := eng.volumeOpsLog(); !slices.Equal(ops, before) { + t.Errorf("refused claim ran %v, want nothing past %v", ops, before) + } + if err := m.Release(t.Context(), first.ID, Cred{Token: first.Token}); err != nil { + t.Fatalf("release first: %v", err) + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", second); err != nil { + t.Errorf("claim after release: %v", err) + } + }) + } +} + +func TestVolumeAdmissionRefusalHoldsNothing(t *testing.T) { + held := writeVolumeImage(t, "held.img", "held") + free := writeVolumeImage(t, "free.img", "free") + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{ + {Name: "held", Path: held, Writable: true}, + {Name: "free", Path: free, Writable: true}, + }) + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "held", Mode: types.VolumeModeRW}}); err != nil { + t.Fatalf("first claim: %v", err) + } + + _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{ + {Name: "free", Mode: types.VolumeModeRW}, + {Name: "held", Mode: types.VolumeModeRW}, + }) + if !errors.Is(err, ErrVolumeBusy) { + t.Fatalf("mixed claim: %v, want ErrVolumeBusy", err) + } + if holders := volumeHoldersOf(m, "free"); holders != (volumeHolders{}) { + t.Errorf("registry for the free name=%+v, want empty", holders) + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "free", Mode: types.VolumeModeRW}}); err != nil { + t.Errorf("claim of the free name after a refusal: %v", err) + } +} + +func TestVolumeAdmissionReleasedAfterSetupFailure(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + eng.mountVolumeErr = errors.New("mount failed") + m := newVolumePoolManager(t, eng, t.TempDir(), []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + writable := []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}} + + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", writable); err == nil { + t.Fatal("ClaimProvision succeeded") + } + if ops := eng.volumeOpsLog(); slices.ContainsFunc(ops, func(op string) bool { return strings.HasPrefix(op, "umount:") }) { + t.Errorf("setup failure quiesced a claim that was never handed out: %v", ops) + } + if !volumeDirty(path) { + t.Error("setup failure cleared the dirty marker") + } + if _, err := m.ClaimWarm(t.Context(), testKey, 0, "", "", writable); !errors.Is(err, ErrNoWarm) { + t.Fatalf("warm claim after a failed setup: %v, want ErrNoWarm", err) + } + eng.mountVolumeErr = nil + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", writable); err != nil { + t.Errorf("claim after a failed setup: %v", err) + } +} + +func TestRollbackQuiescesWritableVolumesBeforeRemoval(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + eng.vms["sbx-rw"] = "/vsock/rw" + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + if err := markVolumeDirty(path); err != nil { + t.Fatalf("mark dirty: %v", err) + } + sb := &types.Sandbox{ + ID: "sb_rw", VMName: "sbx-rw", Key: testKey, VsockSocket: "/vsock/rw", + Volumes: []types.Volume{{Name: "scratch", Mount: "/volumes/scratch", Mode: types.VolumeModeRW}}, + } + m.mu.Lock() + m.claimed[sb.ID] = sb + m.adoptVolumes(sb.Volumes) + m.mu.Unlock() + + m.rollbackClaim(t.Context(), []*types.Sandbox{sb}) + waitFor(t, m.store.synced) + + if seen := eng.opsAtRemoval(sb.VMName); !slices.Contains(seen, "umount:/volumes/scratch") { + t.Errorf("ops at removal=%v, want the unmount already done", seen) + } + if volumeDirty(path) { + t.Error("rollback left the image dirty after a clean unmount") + } + if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{}) { + t.Errorf("registry after rollback=%+v, want empty", holders) + } +} + +func TestReapQuiescesWritableVolumesBeforeRemoval(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + sb, err := m.ClaimProvision(t.Context(), testKey, time.Hour, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + if err != nil { + t.Fatalf("ClaimProvision: %v", err) + } + m.mu.Lock() + sb.Deadline = time.Now().Add(-time.Second) + m.mu.Unlock() + + m.reapOnce(t.Context()) + waitFor(t, func() bool { return volumeHoldersOf(m, "scratch") == volumeHolders{} }) + + if seen := eng.opsAtRemoval(sb.VMName); !slices.Contains(seen, "umount:/volumes/scratch") { + t.Errorf("ops at removal=%v, want the unmount already done", seen) + } + if volumeDirty(path) { + t.Error("reap left the image dirty after a clean unmount") + } +} + +func TestQuiesceFailureKeepsDirtyMarker(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + eng.unmountVolumeErr = errors.New("umount: target is busy") + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + if err != nil { + t.Fatalf("ClaimProvision: %v", err) + } + + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatalf("Release: %v", err) + } + if !volumeDirty(path) { + t.Error("failed unmount cleared the dirty marker") + } + if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{}) { + t.Errorf("registry after a failed unmount=%+v, want empty", holders) + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}); !errors.Is(err, ErrVolumeNeedsRecovery) { + t.Errorf("read-only claim after a failed unmount: %v, want ErrVolumeNeedsRecovery", err) + } +} + +func TestReconcileRebuildsVolumeAdmission(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + dataDir := t.TempDir() + catalog := []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}} + eng := newFakeEngine() + m := newVolumeManagerAt(t, eng, dataDir, catalog) + sb, err := m.ClaimProvision(t.Context(), testKey, time.Hour, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + if err != nil { + t.Fatalf("ClaimProvision: %v", err) + } + + m2 := newVolumeManagerAt(t, eng, dataDir, catalog) + if err := m2.Reconcile(t.Context()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if holders := volumeHoldersOf(m2, "scratch"); holders != (volumeHolders{writers: 1}) { + t.Errorf("adopted registry=%+v, want one writer", holders) + } + if _, err := m2.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}); !errors.Is(err, ErrVolumeBusy) { + t.Errorf("claim against an adopted writer: %v, want ErrVolumeBusy", err) + } + if err := m2.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatalf("release adopted claim: %v", err) + } + if _, err := m2.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}); err != nil { + t.Errorf("claim after releasing the adopted writer: %v", err) + } +} + +func TestWritableDiscoveryAndUsageEvent(t *testing.T) { + readOnly := writeVolumeImage(t, "data.img", "data") + writable := writeVolumeImage(t, "scratch.img", "scratch") + m := newVolumeManager(t, newFakeEngine(), []config.VolumeSpec{ + {Name: "data", Path: readOnly}, + {Name: "scratch", Path: writable, Writable: true}, + }) + + want := []types.VolumeInfo{ + {Name: "data", DefaultMount: "/volumes/data", SizeBytes: int64(len("data")), Available: true, Nodes: 1}, + {Name: "scratch", DefaultMount: "/volumes/scratch", SizeBytes: int64(len("scratch")), Available: true, Nodes: 1, Writable: true}, + } + if got := m.Volumes("", nil); !slices.Equal(got, want) { + t.Errorf("catalog=%+v, want %+v", got, want) + } + + sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{ + {Name: "data"}, + {Name: "scratch", Mode: types.VolumeModeRW}, + }) + if err != nil { + t.Fatalf("ClaimProvision: %v", err) + } + raw, err := os.ReadFile(filepath.Join(m.dataDir, "usage.jsonl")) + if err != nil { + t.Fatalf("read usage journal: %v", err) + } + for line := range strings.SplitSeq(strings.TrimSpace(string(raw)), "\n") { + var event usageEvent + if err := json.Unmarshal([]byte(line), &event); err != nil { + t.Fatalf("decode usage event: %v", err) + } + if event.Event != "claim" || event.ID != sb.ID { + continue + } + if !slices.Equal(event.Volumes, []string{"data", "scratch"}) || !slices.Equal(event.VolumesRW, []string{"scratch"}) { + t.Errorf("claim event volumes=%v rw=%v, want [data scratch] and [scratch]", event.Volumes, event.VolumesRW) + } + return + } + t.Fatal("claim usage event not found") +} + +func newVolumeManagerAt(t *testing.T, eng *fakeEngine, dataDir string, volumes []config.VolumeSpec) *Manager { + t.Helper() + m, err := NewManager(t.Context(), &config.Config{DataDir: dataDir, Volumes: volumes}, eng, testSecrets(t)) + if err != nil { + t.Fatalf("setup manager: %v", err) + } + return m +} + +func volumeHoldersOf(m *Manager, name string) volumeHolders { + m.mu.Lock() + defer m.mu.Unlock() + return m.volumeAdmission[name] +} diff --git a/sandboxd/server/server.go b/sandboxd/server/server.go index 673ae5e..d313dd8 100644 --- a/sandboxd/server/server.go +++ b/sandboxd/server/server.go @@ -48,6 +48,8 @@ var poolErrHTTP = []struct { {pool.ErrNoEgressHibernate, http.StatusConflict, ""}, {pool.ErrNoEgressFork, http.StatusConflict, ""}, {pool.ErrVolumeCapture, http.StatusConflict, ""}, + {pool.ErrVolumeBusy, http.StatusConflict, ""}, + {pool.ErrVolumeNeedsRecovery, http.StatusConflict, ""}, {pool.ErrQuota, http.StatusTooManyRequests, ""}, {pool.ErrHealBusy, http.StatusServiceUnavailable, ""}, {pool.ErrPooledTemplate, http.StatusConflict, ""}, diff --git a/sandboxd/server/server_test.go b/sandboxd/server/server_test.go index 920fc28..b7f07a4 100644 --- a/sandboxd/server/server_test.go +++ b/sandboxd/server/server_test.go @@ -76,6 +76,8 @@ func TestClaimErrorMapping(t *testing.T) { {"bad key", `{"template":"rt:24.04","net":"lan"}`, fmt.Errorf("%w: unknown net", pool.ErrBadKey), http.StatusBadRequest}, {"bad volume", `{"template":"rt:24.04","volumes":[{"name":"data"}]}`, fmt.Errorf("%w: unknown volume", pool.ErrBadVolume), http.StatusBadRequest}, {"no egress", `{"template":"rt:24.04","net":"egress"}`, pool.ErrNoEgress, http.StatusConflict}, + {"volume busy", `{"template":"rt:24.04","volumes":[{"name":"data","mode":"rw"}]}`, fmt.Errorf("%w: volume %q", pool.ErrVolumeBusy, "data"), http.StatusConflict}, + {"volume needs recovery", `{"template":"rt:24.04","volumes":[{"name":"data"}]}`, fmt.Errorf("%w: volume %q", pool.ErrVolumeNeedsRecovery, "data"), http.StatusConflict}, {"engine failure", `{"template":"rt:24.04"}`, errors.New("cocoon vm run: boom"), http.StatusInternalServerError}, } for _, tt := range tests { diff --git a/sandboxd/types/api.go b/sandboxd/types/api.go index bd3bf11..1ea2f7b 100644 --- a/sandboxd/types/api.go +++ b/sandboxd/types/api.go @@ -70,6 +70,9 @@ type VolumeInfo struct { SizeBytes int64 `json:"size_bytes"` Available bool `json:"available"` Nodes int `json:"nodes"` + // Writable reports whether the operator allows rw claims of this name; + // unset for a name known only from a peer's advertisement. + Writable bool `json:"writable,omitempty"` } // VolumeListResponse is the wire reply of GET /v1/volumes. diff --git a/sandboxd/types/types.go b/sandboxd/types/types.go index c7183ea..5bb3684 100644 --- a/sandboxd/types/types.go +++ b/sandboxd/types/types.go @@ -33,6 +33,9 @@ const ( MaxClaimVolumes = 8 + VolumeModeRO = "ro" + VolumeModeRW = "rw" + DirectIOOn = "on" DirectIOOff = "off" DirectIOAuto = "auto" @@ -179,7 +182,7 @@ type Sandbox struct { // the operator index so a listed sandbox maps back to its claim name. // Empty for warm-pool, fork, and checkpoint-branch claims. ClaimRef string `json:"claim_ref,omitempty"` - // Volumes records the read-only volumes successfully applied to this claim. + // Volumes records the volumes successfully applied to this claim. Volumes []Volume `json:"volumes,omitempty"` VsockSocket string `json:"vsock_socket,omitempty"` @@ -272,13 +275,18 @@ type VMConfig struct { Name string `json:"name"` } -// Volume is one requested or applied read-only dataset mount. Mount is empty -// only before request validation; persisted and response entries are effective. +// Volume is one requested or applied dataset mount. Mount is empty only +// before request validation; persisted and response entries are effective. +// Mode is normalized to "" (read-only) or VolumeModeRW. type Volume struct { Name string `json:"name"` Mount string `json:"mount,omitempty"` + Mode string `json:"mode,omitempty"` } +// RW reports whether the entry asks for write access. +func (v Volume) RW() bool { return v.Mode == VolumeModeRW } + // ValidVolumeName reports whether name is a legal cocoon data-disk serial. func ValidVolumeName(name string) bool { return VolumeNameRe.MatchString(name) && !strings.HasPrefix(name, "cocoon-") @@ -306,8 +314,19 @@ func VolumeNames(volumes []Volume) []string { return names } +// VolumeRWNames projects the names of the write-enabled entries; nil for none. +func VolumeRWNames(volumes []Volume) []string { + var names []string + for _, volume := range volumes { + if volume.RW() { + names = append(names, volume.Name) + } + } + return names +} + // ValidateVolumes validates a request and returns detached entries with every -// default mount filled. The input is not modified. +// default mount filled and every mode normalized. The input is not modified. func ValidateVolumes(volumes []Volume) ([]Volume, error) { if len(volumes) > MaxClaimVolumes { return nil, fmt.Errorf("volumes must contain at most %d entries, got %d", MaxClaimVolumes, len(volumes)) @@ -322,6 +341,14 @@ func ValidateVolumes(volumes []Volume) ([]Volume, error) { return nil, fmt.Errorf("volumes[%d] duplicates name %q", i, volume.Name) } names[volume.Name] = struct{}{} + mode := volume.Mode + switch mode { + case VolumeModeRO: + mode = "" + case "", VolumeModeRW: + default: + return nil, fmt.Errorf("volumes[%d] mode %q must be %s or %s", i, mode, VolumeModeRO, VolumeModeRW) + } mount := volume.Mount if mount == "" { mount = DefaultVolumeMount(volume.Name) @@ -338,7 +365,7 @@ func ValidateVolumes(volumes []Volume) ([]Volume, error) { return nil, fmt.Errorf("volumes[%d] mount %q nests with volumes[%d] mount %q", i, mount, j, other) } } - applied[i] = Volume{Name: volume.Name, Mount: mount} + applied[i] = Volume{Name: volume.Name, Mount: mount, Mode: mode} } return applied, nil } diff --git a/sandboxd/types/volume_test.go b/sandboxd/types/volume_test.go index 581be64..6af7878 100644 --- a/sandboxd/types/volume_test.go +++ b/sandboxd/types/volume_test.go @@ -71,6 +71,8 @@ func TestValidateVolumes(t *testing.T) { {"duplicate-mount", []Volume{{Name: "a", Mount: "/data"}, {Name: "b", Mount: "/data"}}}, {"nested-mount", []Volume{{Name: "a", Mount: "/data"}, {Name: "b", Mount: "/data/child"}}}, {"parent-after-child", []Volume{{Name: "a", Mount: "/data/child"}, {Name: "b", Mount: "/data"}}}, + {"unknown-mode", []Volume{{Name: "dataset", Mode: "readwrite"}}}, + {"uppercase-mode", []Volume{{Name: "dataset", Mode: "RW"}}}, } { t.Run(tt.name, func(t *testing.T) { if _, err := ValidateVolumes(tt.volumes); err == nil { @@ -80,6 +82,34 @@ func TestValidateVolumes(t *testing.T) { } } +func TestValidateVolumesNormalizesMode(t *testing.T) { + got, err := ValidateVolumes([]Volume{ + {Name: "shared"}, + {Name: "explicit", Mode: VolumeModeRO}, + {Name: "writable", Mode: VolumeModeRW}, + }) + if err != nil { + t.Fatalf("ValidateVolumes: %v", err) + } + want := []Volume{ + {Name: "shared", Mount: "/volumes/shared"}, + {Name: "explicit", Mount: "/volumes/explicit"}, + {Name: "writable", Mount: "/volumes/writable", Mode: VolumeModeRW}, + } + if !slices.Equal(got, want) { + t.Errorf("volumes %v, want %v", got, want) + } + if got[0].RW() || got[1].RW() || !got[2].RW() { + t.Errorf("RW predicate disagrees with modes %v", got) + } + if names := VolumeRWNames(got); !slices.Equal(names, []string{"writable"}) { + t.Errorf("rw names %v, want [writable]", names) + } + if names := VolumeRWNames(got[:2]); names != nil { + t.Errorf("rw names %v, want nil", names) + } +} + func TestValidateVolumesRejectsGuestOSMounts(t *testing.T) { for _, root := range append([]string{"/"}, guestOSMountRoots...) { t.Run(root, func(t *testing.T) { From 89798ab4966ed05da96e385d94b9f70fbe41187a Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 12 Aug 2026 20:41:58 +0800 Subject: [PATCH 02/10] sdk: per-volume mode on claims and writable in discovery --- sdk/go/client.go | 14 +++++ sdk/go/options.go | 16 ++++- sdk/go/sandbox.go | 2 +- sdk/go/template.go | 3 + sdk/go/volumes_test.go | 97 ++++++++++++++++++++++++++++++ sdk/python/cocoonsandbox/client.py | 12 +++- sdk/python/tests/test_client.py | 55 ++++++++++++++++- 7 files changed, 190 insertions(+), 9 deletions(-) diff --git a/sdk/go/client.go b/sdk/go/client.go index 104c091..0ec48d5 100644 --- a/sdk/go/client.go +++ b/sdk/go/client.go @@ -52,6 +52,9 @@ func (c *Client) New(ctx context.Context, template string, opts ...Option) (*San for _, opt := range opts { opt(&claim) } + if err := claim.validateVolumes(); err != nil { + return nil, err + } addr, cr, err := claimFollow(c.addr, "claim", func(noRedirect, requirePromoted bool) ([]byte, error) { claim.NoRedirect, claim.RequirePromoted = noRedirect, requirePromoted return encodeBody("claim", claim) @@ -441,6 +444,17 @@ func (r claimRequest) rejectPinnedAxes() error { return nil } +// validateVolumes rejects a volume mode outside the wire's vocabulary before +// it reaches the network; WithVolumes already normalizes "ro" to "". +func (r claimRequest) validateVolumes() error { + for _, v := range r.Volumes { + if v.Mode != "" && v.Mode != volumeModeRW { + return fmt.Errorf("volume %q: mode must be \"\", %q, or %q, got %q", v.Name, volumeModeRO, volumeModeRW, v.Mode) + } + } + return nil +} + type claimResponse struct { ID string `json:"id"` Token string `json:"token"` diff --git a/sdk/go/options.go b/sdk/go/options.go index faeb49f..bead197 100644 --- a/sdk/go/options.go +++ b/sdk/go/options.go @@ -15,6 +15,9 @@ const ( Medium Size = "medium" Large Size = "large" XLarge Size = "xlarge" + + volumeModeRO = "ro" + volumeModeRW = "rw" ) // NetShape selects the sandbox network lane. @@ -24,10 +27,12 @@ type NetShape string // node's warm pools. type Size string -// Volume requests one catalog entry at an optional guest mount path. +// Volume requests one catalog entry at an optional guest mount path and mode. type Volume struct { Name string `json:"name"` Mount string `json:"mount,omitempty"` + // Mode is "rw" for a writable mount; empty (or "ro") means read-only. + Mode string `json:"mode,omitempty"` } // VolumeInfo describes one visible fleet catalog entry. @@ -37,6 +42,7 @@ type VolumeInfo struct { SizeBytes int64 `json:"size_bytes"` Available bool `json:"available"` Nodes int `json:"nodes"` + Writable bool `json:"writable,omitempty"` } // Option configures a New claim. @@ -52,9 +58,15 @@ func WithSize(s Size) Option { return func(r *claimRequest) { r.Size = string(s) } } -// WithVolumes requests read-only catalog volumes for a claim. +// WithVolumes requests catalog volumes for a claim; each entry defaults to +// read-only, set Volume.Mode to "rw" for a writable mount. func WithVolumes(volumes ...Volume) Option { volumes = slices.Clone(volumes) + for i, v := range volumes { + if v.Mode == volumeModeRO { + volumes[i].Mode = "" + } + } return func(r *claimRequest) { r.Volumes = volumes } } diff --git a/sdk/go/sandbox.go b/sdk/go/sandbox.go index fcd23c6..260b238 100644 --- a/sdk/go/sandbox.go +++ b/sdk/go/sandbox.go @@ -55,7 +55,7 @@ func (e *ExitError) Error() string { type Sandbox struct { ID string Deadline time.Time - // Volumes lists the read-only volumes finalized on this claim. + // Volumes lists the volumes finalized on this claim, each echoing its mode. Volumes []Volume // TemplateDigest is the content identity of the promoted-template export // this sandbox was cloned from; empty for any other source. diff --git a/sdk/go/template.go b/sdk/go/template.go index 0d5f106..3e8a58d 100644 --- a/sdk/go/template.go +++ b/sdk/go/template.go @@ -29,6 +29,9 @@ func (t *Template) New(ctx context.Context, opts ...Option) (*Sandbox, error) { if err := claim.rejectPinnedAxes(); err != nil { return nil, err } + if err := claim.validateVolumes(); err != nil { + return nil, err + } claim.Net, claim.Size = t.net, t.size if len(claim.Volumes) == 0 { claim.NoRedirect = true diff --git a/sdk/go/volumes_test.go b/sdk/go/volumes_test.go index 7d3f8df..3fba304 100644 --- a/sdk/go/volumes_test.go +++ b/sdk/go/volumes_test.go @@ -170,3 +170,100 @@ func TestCheckpointNewRejectsVolumesLocally(t *testing.T) { t.Errorf("sandbox %+v, want nil", sb) } } + +func TestWithVolumesEncodesMode(t *testing.T) { + tests := []struct { + name string + mode string + }{ + {"empty stays omitted", ""}, + {"ro normalizes to omitted", "ro"}, + {"rw rides the wire", "rw"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var raw struct { + Volumes []map[string]any `json:"volumes"` + } + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := json.NewDecoder(r.Body).Decode(&raw); err != nil { + t.Errorf("decode body: %v", err) + } + _ = json.NewEncoder(w).Encode(claimResponse{ID: "sb_1", Token: "tok"}) + })) + t.Cleanup(ts.Close) + if _, err := testClient(t, ts).New(t.Context(), "rt:24.04", WithVolumes(Volume{Name: "a", Mode: tt.mode})); err != nil { + t.Fatalf("New: %v", err) + } + got, present := raw.Volumes[0]["mode"] + if tt.mode == volumeModeRW { + if !present || got != volumeModeRW { + t.Errorf("wire mode = %v (present=%v), want %q", got, present, volumeModeRW) + } + } else if present { + t.Errorf("wire mode = %v, want omitted", got) + } + }) + } +} + +func TestNewRejectsInvalidVolumeModeLocally(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("server should not be contacted for a locally invalid mode") + })) + t.Cleanup(ts.Close) + + _, err := testClient(t, ts).New(t.Context(), "rt:24.04", WithVolumes(Volume{Name: "a", Mode: "readwrite"})) + if err == nil || !strings.Contains(err.Error(), "mode must be") { + t.Errorf("err = %v, want local mode rejection", err) + } +} + +func TestTemplateNewRejectsInvalidVolumeModeLocally(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("server should not be contacted for a locally invalid mode") + })) + t.Cleanup(ts.Close) + + c := testClient(t, ts) + tpl := &Template{Name: "task:v1", c: c, addr: c.addr, net: "none", size: "small"} + _, err := tpl.New(t.Context(), WithVolumes(Volume{Name: "a", Mode: "bogus"})) + if err == nil || !strings.Contains(err.Error(), "mode must be") { + t.Errorf("err = %v, want local mode rejection", err) + } +} + +func TestSandboxVolumesEchoMode(t *testing.T) { + want := []Volume{{Name: "imagenet"}, {Name: "scratch", Mode: "rw"}} + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(claimResponse{ID: "sb_1", Token: "tok", Volumes: want}) + })) + t.Cleanup(ts.Close) + + sb, err := testClient(t, ts).New(t.Context(), "rt:24.04") + if err != nil { + t.Fatalf("New: %v", err) + } + if !slices.Equal(sb.Volumes, want) { + t.Errorf("volumes = %+v, want %+v", sb.Volumes, want) + } +} + +func TestClientVolumesDecodesWritable(t *testing.T) { + want := []VolumeInfo{ + {Name: "imagenet", DefaultMount: "/volumes/imagenet", SizeBytes: 42, Available: true, Nodes: 3}, + {Name: "scratch", DefaultMount: "/volumes/scratch", SizeBytes: 7, Available: true, Nodes: 1, Writable: true}, + } + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(volumeListResponse{Volumes: want}) + })) + t.Cleanup(ts.Close) + + got, err := testClient(t, ts).Volumes(t.Context()) + if err != nil { + t.Fatalf("Volumes: %v", err) + } + if !slices.Equal(got, want) { + t.Errorf("volumes = %+v, want %+v", got, want) + } +} diff --git a/sdk/python/cocoonsandbox/client.py b/sdk/python/cocoonsandbox/client.py index 683b97d..b4dc483 100644 --- a/sdk/python/cocoonsandbox/client.py +++ b/sdk/python/cocoonsandbox/client.py @@ -181,9 +181,15 @@ def _volume_body(volume: str | Mapping[str, str]) -> dict: return {"name": volume} if not isinstance(volume, Mapping): raise TypeError("volume must be a name string or mapping") - if set(volume) - {"name", "mount"}: - raise TypeError("volume mapping accepts only name and mount") - return dict(volume) + if set(volume) - {"name", "mount", "mode"}: + raise TypeError("volume mapping accepts only name, mount, and mode") + body = dict(volume) + mode = body.get("mode") + if mode in (None, "", "ro"): + body.pop("mode", None) + elif mode != "rw": + raise TypeError("volume mode must be 'rw' or 'ro'") + return body def _template_query(template: str, net: str, size: str) -> dict: diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index 71de5bc..a9e4824 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -89,9 +89,45 @@ def test_claim_rejects_legacy_volume_tuple(node): Client(node).new("rt:24.04", volumes=[("imagenet", "/datasets/imagenet")]) -def test_claim_rejects_volume_mode(node): - with pytest.raises(TypeError, match="only name and mount"): - Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mode": "rw"}]) +def test_claim_sends_volume_mode_rw(node): + seen = [] + + def claim(body, path): + seen.append(body) + return 200, {"id": "sb_1", "token": "tok", "volumes": [ + {"name": "scratch", "mount": "/data", "mode": "rw"}, + ]} + + FakeNode.routes[("POST", "/v1/claim")] = claim + sb = Client(node).new("rt:24.04", volumes=[{"name": "scratch", "mount": "/data", "mode": "rw"}]) + assert seen == [{"template": "rt:24.04", "volumes": [ + {"name": "scratch", "mount": "/data", "mode": "rw"}, + ]}] + assert sb.volumes == [{"name": "scratch", "mount": "/data", "mode": "rw"}] + + +def test_claim_omits_volume_mode_ro(node): + seen = [] + + def claim(body, path): + seen.append(body) + return 200, {"id": "sb_1", "token": "tok"} + + FakeNode.routes[("POST", "/v1/claim")] = claim + Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mode": "ro"}]) + Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mode": ""}]) + # "ro" and "" both normalize to an omitted key, byte-identical to a v1 request. + assert seen[0]["volumes"] == seen[1]["volumes"] == [{"name": "imagenet"}] + + +def test_claim_rejects_invalid_volume_mode(node): + with pytest.raises(TypeError, match="mode must be"): + Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "mode": "rwx"}]) + + +def test_claim_rejects_unknown_volume_key(node): + with pytest.raises(TypeError, match="only name, mount, and mode"): + Client(node).new("rt:24.04", volumes=[{"name": "imagenet", "bogus": "x"}]) def test_template_claim_sends_volumes(node): @@ -146,6 +182,19 @@ def test_volume_catalog(node): assert Client(node).volumes() == want +def test_volume_catalog_surfaces_writable(node): + want = [{ + "name": "scratch", + "default_mount": "/data", + "size_bytes": 42, + "available": True, + "nodes": 1, + "writable": True, + }] + FakeNode.routes[("GET", "/v1/volumes")] = lambda body, path: (200, {"volumes": want}) + assert Client(node).volumes() == want + + def test_promote_returns_content_digest(node): FakeNode.routes[("POST", "/v1/claim")] = lambda body, path: ( 200, {"id": "sb_1", "token": "tok", "owner_addr": node}) From 4fbd252d117860544f8aa01ef029de5d58365111 Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 12 Aug 2026 20:41:58 +0800 Subject: [PATCH 03/10] docs: writable dataset volume contract --- docs/cluster.md | 22 +++++++++--- docs/deploy.md | 81 +++++++++++++++++++++++++++++++++----------- docs/sandboxd-api.md | 47 +++++++++++++++++-------- docs/sdk-python.md | 19 ++++++----- docs/sdk.md | 20 ++++++----- docs/security.md | 19 +++++++++-- 6 files changed, 149 insertions(+), 59 deletions(-) diff --git a/docs/cluster.md b/docs/cluster.md index 2557d25..4ca5b12 100644 --- a/docs/cluster.md +++ b/docs/cluster.md @@ -58,7 +58,7 @@ Node death is honest: a dead node's sandboxes die with it (memory state is node-local by design). SWIM detects the death and peers stop redirecting to it. -### Read-only volumes and placement +### Volumes and placement A volume name has one fleet-wide meaning and access list, while catalog membership is node-local and deliberately excluded from the cluster config @@ -67,6 +67,16 @@ and access lists never leave the node. After config load the set appears on the next gossip tick; later image distribution or removal is detected the same way. The node epoch bumps only when the advertised name set changes. +A writable name (`writable: true`) is expected to have exactly one holder +fleet-wide — the operator contract in +[deploy](deploy.md#writable-dataset-volumes), not a mechanism this layer +enforces. Because a node only ever advertises catalog names it actually +holds, every claim for that name — `ro` or `rw` — already resolves to the +single node that has it through the ordinary redirect logic below; there is +no new gossip field or admission message for writable routing. Configuring +the same writable name on two nodes is an operator error the fleet has no way +to detect. + A volume claim may consume an ordinary warm VM because attach happens after the pop and before finalization. Warm candidates retain their normal ranking, but a candidate must advertise every requested volume. If the entry node cannot serve @@ -81,7 +91,8 @@ fails without a second hop even while template gossip is one tick stale. `GET /v1/volumes` and the SDK discovery calls return the gossiped union filtered through the answering node's fleet-uniform access lists. `nodes` counts members advertising each name, while `available` and `size_bytes` describe only the -answering node's image. No node address or dataset-to-host mapping is returned; +answering node's image, and `writable` is the entry's catalog configuration, +uniform fleet-wide. No node address or dataset-to-host mapping is returned; claim placement resolves the holder. ## Querying members @@ -262,6 +273,7 @@ the mesh. - `cluster_key` set if the gossip network is not otherwise trusted - pool changes via `Client.SetPoolsCluster` (or per-node `SetPools`); the applied set persists to `pools.json` and survives restart -- keep each volume name's dataset identity and access list identical across the - fleet, distribute its immutable image to every node meant to advertise it, - and verify the fleet view and holder count with `GET /v1/volumes` +- keep each volume name's dataset identity and access list identical across + the fleet; distribute a read-only image to every node meant to advertise + it, but a writable (`writable: true`) image belongs on exactly one node — + verify the fleet view and holder count with `GET /v1/volumes` diff --git a/docs/deploy.md b/docs/deploy.md index 6ed34bd..11da5c0 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -88,7 +88,7 @@ sandboxd reads one JSON file (`-config`, default | `no_direct_io` | false | use buffered writable disks for Cloud Hypervisor cold boots and clones; recommended for dense ephemeral pools to avoid direct-I/O CoW journal contention | | `advertise_addr` | = `listen` | the host:port clients reach this node at; returned as a claim's owner address and gossiped to peers. Must be routable when `listen` is a wildcard | | `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) needs `bridges` and rejects a CNI network at load | -| `volumes` | unset | node-local catalog of operator-managed read-only dataset images: `[ {"name":"imagenet","path":"/srv/datasets/imagenet.img","directio":"off","tenants":["acme"]} ]`. 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`. `tenants` is an optional access list: empty means every authenticated scope, while every listed name must exist in the node's `tenants` config. Root always has access. 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: empty means every authenticated scope, while every listed name must exist in the node's `tenants` config; 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 | | `egress_ca` | unset | [HTTPS-interception](egress.md#https-interception) PKI: `root_cert` (the cluster root baked into intercepted guests; may bundle old+new roots during rotation) plus this node's `intermediate_cert`/`intermediate_key` from `sandboxd ca issue-intermediate`. Required when any pool rule sets `intercept` | | `api_token` | unset | the operator (root) credential: when set, guards the node-level endpoints (Bearer) with full access, including release-by-id cleanup. Per-sandbox tokens guard ordinary sandbox-scoped calls | | `tenants` | unset | multi-tenant tokens next to `api_token`: `[{"name": "acme", "token": "…", "max_claims": 50}]`. A tenant token reaches the resource-creating verbs (claim, fork, promote, checkpoint, preview), catalog discovery, and its own sandbox/checkpoint listings; everything it creates is stamped with the tenant name. Root-only surfaces (per-id sandbox reads, `GET /v1/info`, `PUT /v1/pools`, `POST/DELETE /v1/drain`, `/metrics`) answer it 403. `max_claims` (0 = unlimited) caps that tenant's live claims next to the node-wide cap. Requires `api_token` set. Names and tokens must be unique, tokens distinct from `api_token`. On a cluster all nodes must carry the same tenants set (the SDK replays whichever token authorized a redirect), and per-node caps mean a tenant's effective cluster limit is `max_claims` × nodes. Empty = exactly the single-token behavior | @@ -120,13 +120,13 @@ fragment the warm pools): | `large` | 4 | 4G | | `xlarge` | 4 | 8G | -### Read-only dataset volumes +### Dataset volumes -Each catalog path must name an immutable disk image containing a mountable -whole-device filesystem. A missing path produces a startup warning and fails -only claims that request it, allowing images to be distributed after sandboxd -starts. Do not replace, truncate, or delete an image while it is attached; -publish a new catalog name or path instead. +Each catalog path must name a disk image containing a mountable whole-device +filesystem. A read-only entry's image must stay immutable: do not replace, +truncate, or delete it while attached; publish a new catalog name or path +instead. A missing path produces a startup warning and fails only claims that +request it, allowing images to be distributed after sandboxd starts. For example, build a whole-device ext4 image directly from a prepared tree, then make the published file host-read-only: @@ -149,10 +149,12 @@ mounts may shadow an existing populated guest directory for that claim's life. A volume claim may consume an ordinary warm Cloud Hypervisor VM. sandboxd attaches after the warm pop or provision, polls `/sys/block/*/serial` for the -attach name for up to 2 seconds, then mounts the device read-only before -finalizing the claim. Both the Cloud Hypervisor attachment and the guest -filesystem mount are read-only. Setup failure destroys the VM; a popped warm VM -is refilled normally. Firecracker volume claims are rejected. +attach name for up to 2 seconds, then mounts the device before finalizing the +claim: `mode: "ro"` (the default) attaches and mounts read-only; `mode: "rw"` +requires the catalog entry's `writable: true` and attaches and mounts +read-write. Setup failure destroys the VM without quiescing — the claim was +never handed out, so no workload write happened — and a popped warm VM is +refilled normally. Firecracker volume claims are rejected. Warm candidates retain their normal ranking, but a candidate for a volume claim must hold every requested image. If the entry node cannot serve them all, @@ -165,16 +167,57 @@ An empty catalog `tenants` list allows every authenticated scope; a nonempty list limits the image to those tenants, while root always bypasses it. Removing a tenant therefore requires removing every catalog reference in the same edit. `GET /v1/volumes` reports the caller-visible fleet union and holder count, plus -the answering node's current local availability, without exposing host paths or -node addresses. Applied names and effective mounts are persisted with the -claim. Such a claim cannot hibernate, fork, checkpoint, or promote; the idle -hibernate sweep leaves it running. Release removes the VM but never deletes the -operator-owned backing image. With `directio=off`, readers share the host page -cache; use `directio=on` when cache interference matters. +the answering node's current local availability and whether the entry is +`writable`, without exposing host paths or node addresses. Applied names, +effective mounts, and (for `rw` entries) mode are persisted with the claim. +Such a claim cannot hibernate, fork, checkpoint, or promote; the idle +hibernate sweep leaves it running. Release removes the VM but never deletes +the operator-owned backing image — an `rw` release quiesces (unmounts) first, +below. With `directio=off`, readers share the host page cache; use +`directio=on` when cache interference matters. A dataset mounted into an egress-lane sandbox can be uploaded wherever that tenant's egress policy permits, so treat the ACL and egress policy as one access -decision. Image replication, write-enabled dataset disks, detach, and -refcounting remain out of scope. +decision. Image replication, detach, and refcounting remain out of scope. + +#### Writable dataset volumes + +A catalog entry with `writable: true` may be claimed with `mode: "rw"` +(omitted, or `"ro"`, always mounts read-only, even against a writable entry). +`directio` still defaults to `off` for a writable entry — the durability +nuance is that the host page cache sits between the guest's flush and the +device, so `directio=on` is the knob when that gap matters, not just cache +interference. + +**Single holder, by operator contract.** A writable name must be attached +from exactly one node — the same weight as "replacing an attached image is +operator error." Nodes advertise only the catalog paths they actually hold, +so every claim for that name, `ro` or `rw`, already funnels to that one node; +running the same writable name from two nodes at once is dataset divergence +that nothing in the protocol detects for you. + +**The `.dirty` marker.** Before the first `rw` attach of an image, +sandboxd durably creates a `.dirty` sidecar beside it; a clean `rw` +release removes it. It is a write-ahead record, not a lock: it survives a +killed sandboxd or a dead VMM, so anything short of a clean unmount leaves it +in place. It is operator-visible — `ls` next to the image shows whether it's +mid-write — and travels with the image on shared storage. A leftover marker +is not corruption (a journaling filesystem already makes a hard VM kill +crash-consistent); it means the image needs one `rw` claim, which replays the +journal as a side effect of mounting, followed by a clean release, before it +can serve `ro` again. A `ro` claim against a dirty image is refused rather +than auto-healed: replaying the journal takes the write lock, which conflicts +with concurrent readers. + +**Concurrency, for one image name:** + +- `rw` ∥ `rw` — refused (409). +- `rw` ∥ `ro`, either order — refused (409): a writer under live readers hands + every reader torn metadata, since ext4/xfs are not cluster filesystems. +- `ro` ∥ `ro` — fine, same as v1. +- Sequential `rw` → release → `ro` is the supported publish/update workflow, + gated by the dirty marker: a clean writer release clears it and readers + proceed; a crashed writer leaves it, and `ro` claims are refused until one + `rw` claim replays and releases cleanly. ### A fuller config diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index 45b822c..ca92966 100644 --- a/docs/sandboxd-api.md +++ b/docs/sandboxd-api.md @@ -26,7 +26,8 @@ Auth: `Authorization: Bearer ` (when configured). ```json {"template": "base:24.04", "net": "none", "size": "small", "ttl_seconds": 300, - "volumes": [{"name": "imagenet"}, {"name": "weights", "mount": "/models"}], + "volumes": [{"name": "imagenet"}, {"name": "weights", "mount": "/models"}, + {"name": "scratch-db", "mode": "rw"}], "claim_ref": "namespace/workload", "no_redirect": false, "require_promoted": false} ``` @@ -42,8 +43,10 @@ Auth: `Authorization: Bearer ` (when configured). a promoted template name if its gossip view is stale - `volumes` is an ordered list of at most eight unique catalog names. `mount` defaults to `/volumes/`; a custom value must be absolute and clean, - outside the guest OS tree, unique, and non-nesting within the request. Volumes - are read-only and require Cloud Hypervisor + outside the guest OS tree, unique, and non-nesting within the request. + `mode` is `"ro"` (default, omitted) or `"rw"`; `"rw"` requires the catalog + entry's `writable: true` (see [deploy](deploy.md#dataset-volumes)). Volumes + require Cloud Hypervisor Success: @@ -51,7 +54,8 @@ Success: {"id": "sb_…", "token": "…", "deadline": "2026-07-06T00:05:00Z", "owner_addr": "10.0.0.5:7777", "template_digest": "sha256:…", "volumes": [{"name": "imagenet", "mount": "/volumes/imagenet"}, - {"name": "weights", "mount": "/models"}]} + {"name": "weights", "mount": "/models"}, + {"name": "scratch-db", "mount": "/volumes/scratch-db", "mode": "rw"}]} ``` A claim cloned from a promoted template carries `template_digest`, the exact @@ -63,11 +67,13 @@ A claim branched from a checkpoint (fork children included) additionally carries `"from_checkpoint": "ck_…"` — the lineage edge for reconstructing the checkpoint tree. -`volumes` reports the names and effective mounts applied and persisted at -finalization. sandboxd attaches each disk read-only, polls +`volumes` reports the names, effective mounts, and (`rw` only) mode applied +and persisted at finalization — `mode` is omitted from the echo for `ro` +entries, matching the request shape. sandboxd attaches each disk, polls `/sys/block/*/serial` for its attach name for up to 2 seconds, and mounts the -filesystem read-only before returning. A custom mount may shadow an existing -populated guest directory for the claim's life. +filesystem — read-only, unless the entry requested and was granted `rw` — +before returning. A custom mount may shadow an existing populated guest +directory for the claim's life. Redirects (mutually exclusive with the fields above) name peers to retry at — sent on a warm miss with warm peers, when the node lacks a golden for @@ -96,11 +102,18 @@ name (attributed in the usage journal and counted against the tenant's `max_claims`). A catalog access list may restrict an entry to named tenants; an unknown and a forbidden volume return the same error text. -Errors: 400 unknown template axis, invalid/duplicate volumes, or a volume that -is unknown or forbidden (the latter two are deliberately indistinguishable), -Firecracker with volumes, or bad body; 401 bad api token; 409 egress requested -on a node without an egress attachment; 429 node at `max_claims`, the calling -tenant at its own `max_claims`, or the node draining; 500 provisioning failed. +Errors: 400 unknown template axis, invalid/duplicate volumes, `mode: "rw"` +against a non-writable entry, or a volume that is unknown or forbidden (the +latter two are deliberately indistinguishable), Firecracker with volumes, or +bad body; 401 bad api token; 409 egress requested on a node without an egress +attachment, a writable name already claimed in a conflicting mode (volume +busy — a live writer excludes every other claim for that name, live readers +exclude a writer), or a `ro` claim against a writable image left dirty by an +unclean `rw` release (needs recovery — one `rw` claim must replay and cleanly +release before `ro` claims resume; see +[deploy](deploy.md#writable-dataset-volumes)); 429 node at `max_claims`, the +calling tenant at its own `max_claims`, or the node draining; 500 +provisioning failed. ## GET /v1/volumes @@ -109,7 +122,8 @@ caller may use, without host paths or holder addresses: ```json {"volumes": [{"name": "imagenet", "default_mount": "/volumes/imagenet", - "size_bytes": 214748364800, "available": true, "nodes": 3}]} + "size_bytes": 214748364800, "available": true, "nodes": 3, + "writable": false}]} ``` Root sees every entry; a tenant sees unrestricted entries plus those whose @@ -117,6 +131,8 @@ access list names it. The response is the gossiped union: `nodes` counts members advertising the name. `size_bytes` and `available` are a best-effort stat of the answering node's image, so a peer-only entry remains discoverable with `available: false`. Membership is eventually consistent by one gossip tick. +`writable` is the entry's catalog configuration, fleet-uniform like the access +list, not per-node state. ## POST /v1/sandboxes/{id}/release @@ -409,7 +425,8 @@ Always on: every lifecycle transition appends one JSONL event to "id": "sb_…", "vm": "sbx-…"}` plus `key` and `tenant` (the pool key and owning tenant, claim events), `children` (fork) and `ref` (the promoted template / checkpoint id, or the egress host). A volume claim also carries -`volumes`, the applied catalog names (mounts and host paths are not billing +`volumes`, the applied catalog names, each flagged when claimed `rw` so +billing can discriminate write access (mounts and host paths are not billing dimensions). The file rotates at 64 MiB keeping one `.1` backup, so a tailing collector never loses a window silently. Folding rules: billable compute seconds per sandbox = diff --git a/docs/sdk-python.md b/docs/sdk-python.md index b4048f0..91f198d 100644 --- a/docs/sdk-python.md +++ b/docs/sdk-python.md @@ -87,22 +87,23 @@ sb = client.lookup(id, token) # asks the entry node, then each mesh peer ```python sb = client.new("ghcr.io/cocoonstack/sandbox/rt:24.04", net="egress", size="medium", ttl_seconds=600, - volumes=["imagenet", {"name": "weights", "mount": "/models"}]) + volumes=["imagenet", {"name": "weights", "mount": "/models"}, + {"name": "scratch-db", "mode": "rw"}]) ``` | parameter | values | default | meaning | |---|---|---|---| | `net` | `"none"`, `"egress"` | `"none"` | Cloud Hypervisor network shape: `none` disables the NIC and uses vsock-only I/O; `egress` attaches a bridge/CNI NIC | | `size` | `"small"`, `"medium"`, `"large"`, `"xlarge"` | `"small"` | resource tier: 1cpu/512M, 2cpu/1G, 4cpu/4G, 4cpu/8G | -| `volumes` | bare names or `{name, mount?}` mappings | `None` | attach and mount up to eight unique read-only dataset disks; an omitted mount defaults to `/volumes/`; accepted by `Client.new` and `Template.new` | +| `volumes` | bare names or `{name, mount?, mode?}` mappings | `None` | attach and mount up to eight unique catalog dataset disks; an omitted mount defaults to `/volumes/`; `mode` is `"ro"` (default) or `"rw"` — `"rw"` requires the catalog entry's `writable: true`; accepted by `Client.new` and `Template.new` | | `ttl_seconds` | int | server default 5m | sandbox TTL, server-capped at 24h. The node reaps the sandbox after the TTL even if the client vanishes | `new` returns when the sandbox's silkd answers: a warm hit is milliseconds, a cold key can take the full boot. A volume claim may consume an ordinary warm VM and returns only after every requested disk is mounted; `sb.volumes` contains -dictionaries with the finalized name and effective mount. Custom mounts must be -absolute and clean, stay outside the guest OS tree, and cannot duplicate or -nest. The handle +dictionaries with the finalized name, effective mount, and (for `rw` entries) +mode. Custom mounts must be absolute and clean, stay outside the guest OS +tree, and cannot duplicate or nest. The handle exposes `sb.id`, `sb.token`, `sb.owner`, `sb.deadline`, and `sb.from_checkpoint` (the lineage edge when branched). `sb.template_digest` is the exact content identity when the claim @@ -110,17 +111,19 @@ cloned a promoted template; it is empty for other sources. `Sandbox` is a context manager; `sb.close()` releases it (releasing one already gone is not an error — double-release and reap races stay silent). Volume sandboxes cannot hibernate, fork, checkpoint, or promote. Checkpoint -branches do not accept volumes in this version. +branches do not accept volumes of either mode in this version. The caller-visible constraints are deliberate: volume claims may consume a -warm VM, remain non-capturable, mount read-only, and require Cloud Hypervisor. +warm VM, remain non-capturable, mount read-only by default, and require Cloud +Hypervisor. `client.volumes()` returns the fleet entries this token may use: ```python for volume in client.volumes(): print(volume["name"], volume["default_mount"], - volume["size_bytes"], volume["available"], volume["nodes"]) + volume["size_bytes"], volume["available"], volume["nodes"], + volume["writable"]) ``` Discovery returns the gossiped union and holder count; availability and size diff --git a/docs/sdk.md b/docs/sdk.md index 7d81cb4..c0b86af 100644 --- a/docs/sdk.md +++ b/docs/sdk.md @@ -133,7 +133,8 @@ sb, err := client.New(ctx, "base:24.04", sandbox.WithSize(sandbox.Medium), sandbox.WithVolumes( sandbox.Volume{Name: "imagenet"}, - sandbox.Volume{Name: "weights", Mount: "/models"}), + sandbox.Volume{Name: "weights", Mount: "/models"}, + sandbox.Volume{Name: "scratch-db", Mode: "rw"}), sandbox.WithTimeout(10*time.Minute)) defer sb.Close() ``` @@ -142,15 +143,15 @@ defer sb.Close() |---|---|---|---| | `WithNetwork(n)` | `NetNone`, `NetEgress` | `NetNone` | Cloud Hypervisor network shape: `NetNone` disables the NIC and uses vsock-only I/O; `NetEgress` attaches a bridge/CNI NIC | | `WithSize(s)` | `Small`, `Medium`, `Large`, `XLarge` | `Small` | resource tier: 1cpu/512M, 2cpu/1G, 4cpu/4G, 4cpu/8G | -| `WithVolumes(volumes...)` | `Volume{Name, Mount?}` entries | none | attach and mount up to eight unique read-only dataset disks; `Mount` defaults to `/volumes/`; supported by `Client.New` and `Template.New` | +| `WithVolumes(volumes...)` | `Volume{Name, Mount?, Mode?}` entries | none | attach and mount up to eight unique catalog dataset disks; `Mount` defaults to `/volumes/`; `Mode` is `"ro"` (default) or `"rw"` — `"rw"` requires the catalog entry's `writable: true`; supported by `Client.New` and `Template.New` | | `WithTimeout(d)` | duration | server default 5m | sandbox TTL, rounded up to seconds, server-capped at 24h. The node reaps the sandbox after the TTL even if the client vanishes | `New` returns when the sandbox's silkd answers: a warm hit is milliseconds, a cold key can take the full boot. A volume claim may consume an ordinary warm -VM and returns only after every requested disk is mounted; the finalized name -and effective mount are available in `Sandbox.Volumes`. Custom mounts must be -absolute and clean, stay outside the guest OS tree, and cannot duplicate or -nest. +VM and returns only after every requested disk is mounted; the finalized +name, effective mount, and (for `rw` entries) mode are available in +`Sandbox.Volumes`. Custom mounts must be absolute and clean, stay outside the +guest OS tree, and cannot duplicate or nest. `Sandbox.ID`, `Sandbox.Deadline`, and `Sandbox.FromCheckpoint` (the lineage edge when branched) are exported. `Sandbox.TemplateDigest` is the exact content identity when the claim cloned a @@ -160,17 +161,18 @@ later `Lookup`; `Close()` releases the sandbox (releasing one already gone is not an error, and `Close` is bounded internally so it stays defer-friendly). Volume sandboxes cannot hibernate, fork, checkpoint, or promote. Passing `WithVolumes` to `Checkpoint.New` returns a local error because checkpoint -branches do not support volumes in this version. +branches do not support volumes of either mode in this version. The caller-visible constraints are deliberate: volume claims may consume a -warm VM, remain non-capturable, mount read-only, and require Cloud Hypervisor. +warm VM, remain non-capturable, mount read-only by default, and require Cloud +Hypervisor. Discover the fleet entries this token may use before planning a claim: ```go catalog, err := client.Volumes(ctx) // []sandbox.VolumeInfo for _, volume := range catalog { - fmt.Println(volume.Name, volume.DefaultMount, volume.SizeBytes, volume.Available, volume.Nodes) + fmt.Println(volume.Name, volume.DefaultMount, volume.SizeBytes, volume.Available, volume.Nodes, volume.Writable) } ``` diff --git a/docs/security.md b/docs/security.md index b1e6bbe..124c6a3 100644 --- a/docs/security.md +++ b/docs/security.md @@ -85,7 +85,7 @@ or partitioned at that moment keeps its own replica branchable until Tenants are isolated at the API layer — listings filter, deletes answer 404 rather than confirming existence, and operator surfaces answer tenants 403. -## Read-only dataset volumes +## Dataset volumes 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 @@ -103,10 +103,23 @@ 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 list and egress policy together. With `directio=off`, concurrent readers share the host page cache, which improves reuse but lets one tenant's large scan evict -another's cached pages. `directio=on` is the per-volume mitigation; v1 has no -per-tenant cache quota or accounting. Operators must keep an attached image +another's cached pages — the same holds for a writer: a large `rw` write can +evict cached pages backing another volume's readers exactly like a large scan +would. `directio=on` is the per-volume mitigation; v1 has no per-tenant cache +quota or accounting. Operators must keep a read-only entry's attached image immutable and publish a new name/path for new content. +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 +other permitted tenant reads next, and any of them can be the writer in +turn — a multi-tenant access list on a writable entry is a bidirectional +channel between those tenants, not just a shared read. Recommend a writable +entry's `tenants` name exactly one tenant — an empty list permits every +authenticated scope, which for a writable entry means every tenant can write +to every other tenant's next read. A dataset that genuinely needs multiple +writers needs an out-of-band process for who writes when, which the catalog +ACL does not provide. + ## Known limitations Facts to plan around, stated so the boundary is honest: From 05b11fe85e87ce2d532fcc20517b2b60bc15ddbc Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 00:31:12 +0800 Subject: [PATCH 04/10] e2e: writable volume coverage, wire-shape pins, volumesmoke rw leg --- e2e/cmd/volumesmoke/main.go | 75 ++++++++++++++- e2e/e2e_test.go | 183 ++++++++++++++++++++++++++++++++++-- e2e/fakeengine_test.go | 44 ++++++++- 3 files changed, 291 insertions(+), 11 deletions(-) diff --git a/e2e/cmd/volumesmoke/main.go b/e2e/cmd/volumesmoke/main.go index fe450f8..a088280 100644 --- a/e2e/cmd/volumesmoke/main.go +++ b/e2e/cmd/volumesmoke/main.go @@ -1,4 +1,5 @@ -// volumesmoke validates shared read-only dataset mounts on a live Cloud Hypervisor node. +// volumesmoke validates shared read-only dataset mounts, and optionally one +// writable dataset mount, on a live Cloud Hypervisor node. package main import ( @@ -22,16 +23,17 @@ func main() { token := flag.String("token", "", "node api token") template := flag.String("template", "rt:24.04", "template ref") volume := flag.String("volume", "", "catalog volume name") + rwVolume := flag.String("rw-volume", "", "writable catalog volume name (adds the writable leg)") probe := flag.String("probe", "volume-e2e.txt", "non-empty file inside the volume") flag.Parse() - if err := run(*addr, *token, *template, *volume, *probe); err != nil { + if err := run(*addr, *token, *template, *volume, *rwVolume, *probe); err != nil { fmt.Fprintln(os.Stderr, "volumesmoke:", err) os.Exit(1) } } -func run(addr, token, template, volume, probe string) error { +func run(addr, token, template, volume, rwVolume, probe string) error { if volume == "" { return errors.New("volume is required") } @@ -116,5 +118,72 @@ func run(addr, token, template, volume, probe string) error { } released = true fmt.Printf("VOLUME PASS concurrent_read_bytes=%d read_only=true released=true\n", len(outputs[0])) + if rwVolume == "" { + return nil + } + return runWritable(ctx, client, template, rwVolume) +} + +// runWritable proves the publish workflow: a writer's bytes survive its own +// release, it excludes concurrent claims, and the next reader sees them. +func runWritable(ctx context.Context, client *sandbox.Client, template, volume string) error { + const ( + mount = "/datasets/e2e-rw" + file = "volume-rw-probe.txt" + ) + stamp := fmt.Sprintf("rw-%d", time.Now().UnixNano()) + start := time.Now() + writer, err := client.New(ctx, template, sandbox.WithNetwork(sandbox.NetNone), + sandbox.WithVolumes(sandbox.Volume{Name: volume, Mount: mount, Mode: "rw"})) + if err != nil { + return fmt.Errorf("writable claim: %w", err) + } + claimRW := time.Since(start) + live := writer + defer func() { + if live != nil { + _ = live.Close() + } + }() + if want := []sandbox.Volume{{Name: volume, Mount: mount, Mode: "rw"}}; !slices.Equal(writer.Volumes, want) { + return fmt.Errorf("writable claim volumes %+v, want %+v", writer.Volumes, want) + } + if _, err = writer.Exec(ctx, "sh", "-c", fmt.Sprintf("printf %%s %s > %s", stamp, path.Join(mount, file))); err != nil { + return fmt.Errorf("write through the writable mount: %w", err) + } + // A live writer owns the image: the second claim is refused before attach. + busy, busyErr := client.New(ctx, template, sandbox.WithNetwork(sandbox.NetNone), + sandbox.WithVolumes(sandbox.Volume{Name: volume, Mount: mount, Mode: "rw"})) + if busyErr == nil { + _ = busy.Close() + return errors.New("second writable claim succeeded while the first held the image") + } + if !strings.Contains(busyErr.Error(), "409") { + return fmt.Errorf("second writable claim failed without 409: %w", busyErr) + } + if err = writer.Close(); err != nil { + return fmt.Errorf("release writable claim: %w", err) + } + live = nil + + reader, err := client.New(ctx, template, sandbox.WithNetwork(sandbox.NetNone), + sandbox.WithVolumes(sandbox.Volume{Name: volume, Mount: mount})) + if err != nil { + return fmt.Errorf("read-only claim after the writer released: %w", err) + } + live = reader + out, err := reader.Exec(ctx, "cat", path.Join(mount, file)) + if err != nil { + return fmt.Errorf("read back the written file: %w", err) + } + if strings.TrimSpace(out) != stamp { + return fmt.Errorf("read back %q, want %q", strings.TrimSpace(out), stamp) + } + if err = reader.Close(); err != nil { + return fmt.Errorf("release read-only claim: %w", err) + } + live = nil + fmt.Printf("VOLUME RW PASS volume=%s claim=%.1fms durable=true busy_refused=true\n", + volume, float64(claimRW.Microseconds())/1000) return nil } diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 5db5821..84916a1 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -5,6 +5,11 @@ package e2e import ( + "encoding/json" + "fmt" + "io" + "maps" + "net/http" "net/http/httptest" "os" "path/filepath" @@ -284,12 +289,13 @@ func TestWrongAPITokenRejected(t *testing.T) { } func TestVolumesEndToEnd(t *testing.T) { - image := filepath.Join(t.TempDir(), "dataset.img") - if err := os.WriteFile(image, []byte("dataset-bytes"), 0o600); err != nil { - t.Fatalf("write image: %v", err) - } + image := writeVolumeImage(t, "dataset.img", "dataset-bytes") + scratch := writeVolumeImage(t, "scratch.img", "scratch-bytes") stack := startTenantStack(t, "node-token", nil, - []config.VolumeSpec{{Name: "dataset", Path: image, DirectIO: "off"}}, + []config.VolumeSpec{ + {Name: "dataset", Path: image, DirectIO: "off"}, + {Name: "scratch", Path: scratch, Writable: true}, + }, config.PoolSpec{PoolKey: testKey, Warm: 1}) waitFor(t, func() bool { infos, _ := stack.mgr.Info() @@ -318,16 +324,128 @@ func TestVolumesEndToEnd(t *testing.T) { want := []sandbox.VolumeInfo{{ Name: "dataset", DefaultMount: "/volumes/dataset", SizeBytes: int64(len("dataset-bytes")), Available: true, Nodes: 1, + }, { + Name: "scratch", DefaultMount: "/volumes/scratch", + SizeBytes: int64(len("scratch-bytes")), Available: true, Nodes: 1, Writable: true, }} if !slices.Equal(infos, want) { t.Errorf("catalog %+v, want %+v", infos, want) } + + var listed struct { + Volumes []map[string]any `json:"volumes"` + } + _, body := rawJSON(t, stack, http.MethodGet, "/v1/volumes", "") + if err := json.Unmarshal(body, &listed); err != nil { + t.Fatalf("decode catalog %s: %v", body, err) + } + if len(listed.Volumes) != len(want) { + t.Fatalf("catalog bytes %s, want %d entries", body, len(want)) + } + for _, entry := range listed.Volumes { + var writable any + if entry["name"] == "scratch" { + writable = true + } + if entry["writable"] != writable { + t.Errorf("volume %v writable=%v, want %v", entry["name"], entry["writable"], writable) + } + } +} + +// TestWritableVolumeEndToEnd drives one writable claim through the whole +// stack: the SDK's mode reaches the engine as a writable attach, a live writer +// refuses every other claim on the name, and release unmounts before removal. +func TestWritableVolumeEndToEnd(t *testing.T) { + scratch := writeVolumeImage(t, "scratch.img", "scratch-bytes") + stack := startTenantStack(t, "node-token", nil, + []config.VolumeSpec{{Name: "scratch", Path: scratch, Writable: true}}) + + sb, err := stack.client.New(t.Context(), "rt:24.04", + sandbox.WithVolumes(sandbox.Volume{Name: "scratch", Mount: "/datasets/rw", Mode: "rw"})) + if err != nil { + t.Fatalf("writable claim: %v", err) + } + if want := []sandbox.Volume{{Name: "scratch", Mount: "/datasets/rw", Mode: "rw"}}; !slices.Equal(sb.Volumes, want) { + t.Errorf("claim volumes %+v, want %+v", sb.Volumes, want) + } + applied := []string{"attach:scratch:rw", "mount:scratch:/datasets/rw:rw"} + if got := stack.eng.volumeOpsLog(); !slices.Equal(got, applied) { + t.Errorf("engine ops %v, want %v", got, applied) + } + for _, requested := range []string{`{"name":"scratch"}`, `{"name":"scratch","mode":"rw"}`} { + if status, _ := rawClaim(t, stack, requested); status != http.StatusConflict { + t.Errorf("claim %s under a live writer: %d, want 409", requested, status) + } + } + + if err := sb.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + want := slices.Concat(applied, []string{"umount:/datasets/rw", "remove"}) + if got := stack.eng.volumeOpsLog(); !slices.Equal(got, want) { + t.Errorf("engine ops after release %v, want %v", got, want) + } +} + +// TestVolumeModeWireShape pins the claim reply's volume bytes independently of +// the SDK mirror. The read-only leg runs second on purpose: it is admitted +// only because the writer's release cleared the dirty marker. +func TestVolumeModeWireShape(t *testing.T) { + scratch := writeVolumeImage(t, "scratch.img", "scratch-bytes") + stack := startTenantStack(t, "node-token", nil, + []config.VolumeSpec{{Name: "scratch", Path: scratch, Writable: true}}) + for _, tt := range []struct { + name string + requested string + want map[string]any + }{ + { + "writable echoes its mode", + `{"name":"scratch","mount":"/datasets/x","mode":"rw"}`, + map[string]any{"name": "scratch", "mount": "/datasets/x", "mode": "rw"}, + }, + { + "read-only omits mode", + `{"name":"scratch","mount":"/datasets/x"}`, + map[string]any{"name": "scratch", "mount": "/datasets/x"}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + status, claimed := rawClaim(t, stack, tt.requested) + if status != http.StatusOK { + t.Fatalf("claim: %d, want 200", status) + } + if len(claimed.Volumes) != 1 || !maps.Equal(claimed.Volumes[0], tt.want) { + t.Errorf("reply volumes %v, want [%v]", claimed.Volumes, tt.want) + } + if err := stack.client.Attach(stack.addr, claimed.ID, claimed.Token).Close(); err != nil { + t.Fatalf("release: %v", err) + } + }) + } +} + +// TestDirtyVolumeRefusesReader: the marker a crashed writer leaves behind +// (pre-created here) turns read-only claims into 409s over the wire. +func TestDirtyVolumeRefusesReader(t *testing.T) { + scratch := writeVolumeImage(t, "scratch.img", "scratch-bytes") + if err := os.WriteFile(scratch+".dirty", nil, 0o600); err != nil { + t.Fatalf("write dirty marker: %v", err) + } + stack := startTenantStack(t, "node-token", nil, + []config.VolumeSpec{{Name: "scratch", Path: scratch, Writable: true}}) + if status, _ := rawClaim(t, stack, `{"name":"scratch"}`); status != http.StatusConflict { + t.Errorf("read-only claim on a dirty image: %d, want 409", status) + } } type stack struct { client *sandbox.Client mgr *pool.Manager + eng *fakeEngine addr string + token string } func startStack(t *testing.T, apiToken string, pools ...config.PoolSpec) *stack { @@ -362,7 +480,7 @@ func startTenantStack(t *testing.T, apiToken string, tenants []config.TenantSpec if err != nil { t.Fatalf("connect: %v", err) } - return &stack{client: client, mgr: mgr, addr: addr} + return &stack{client: client, mgr: mgr, eng: eng, addr: addr, token: apiToken} } func waitFor(t *testing.T, cond func() bool) { @@ -376,3 +494,56 @@ func waitFor(t *testing.T, cond func() bool) { } t.Fatal("condition not met within 10s") } + +func writeVolumeImage(t *testing.T, name, content string) string { + t.Helper() + image := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(image, []byte(content), 0o600); err != nil { + t.Fatalf("write volume image: %v", err) + } + return image +} + +// rawClaimResponse decodes the volume entries generically, so the assertion +// is the server's own JSON rather than the SDK's mirror of it. +type rawClaimResponse struct { + ID string `json:"id"` + Token string `json:"token"` + Volumes []map[string]any `json:"volumes"` +} + +func rawClaim(t *testing.T, st *stack, volume string) (int, rawClaimResponse) { + t.Helper() + status, body := rawJSON(t, st, http.MethodPost, "/v1/claim", + fmt.Sprintf(`{"template":"rt:24.04","volumes":[%s]}`, volume)) + var claimed rawClaimResponse + if status == http.StatusOK { + if err := json.Unmarshal(body, &claimed); err != nil { + t.Fatalf("decode claim %s: %v", body, err) + } + } + return status, claimed +} + +func rawJSON(t *testing.T, st *stack, method, route, body string) (int, []byte) { + t.Helper() + var payload io.Reader + if body != "" { + payload = strings.NewReader(body) + } + req, err := http.NewRequestWithContext(t.Context(), method, "http://"+st.addr+route, payload) + if err != nil { + t.Fatalf("%s %s: %v", method, route, err) + } + req.Header.Set("Authorization", "Bearer "+st.token) + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", method, route, err) + } + defer resp.Body.Close() + out, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read %s %s: %v", method, route, err) + } + return resp.StatusCode, out +} diff --git a/e2e/fakeengine_test.go b/e2e/fakeengine_test.go index da81291..575704c 100644 --- a/e2e/fakeengine_test.go +++ b/e2e/fakeengine_test.go @@ -26,6 +26,8 @@ type fakeEngine struct { mu sync.Mutex listeners map[string]io.Closer socks map[string]string + volumeOps []string + volumeVMs map[string]bool seq int } @@ -35,6 +37,7 @@ func newFakeEngine(dir string) *fakeEngine { dir: dir, listeners: map[string]io.Closer{}, socks: map[string]string{}, + volumeVMs: map[string]bool{}, } } @@ -58,6 +61,10 @@ func (f *fakeEngine) Remove(_ context.Context, name string) error { } delete(f.listeners, name) delete(f.socks, name) + if f.volumeVMs[name] { + delete(f.volumeVMs, name) + f.volumeOps = append(f.volumeOps, "remove") + } return nil } @@ -109,9 +116,35 @@ func (f *fakeEngine) DialGuestPort(context.Context, string, uint16) (net.Conn, e func (f *fakeEngine) InstallCACert(context.Context, string, []byte) error { return nil } -func (f *fakeEngine) DiskAttach(context.Context, string, engine.VolumeSpec) error { return nil } +func (f *fakeEngine) DiskAttach(_ context.Context, vmName string, spec engine.VolumeSpec) error { + f.mu.Lock() + defer f.mu.Unlock() + f.volumeVMs[vmName] = true + f.volumeOps = append(f.volumeOps, "attach:"+spec.Name+":"+volumeMode(spec.RW)) + return nil +} -func (f *fakeEngine) MountVolume(context.Context, string, string, string) error { return nil } +func (f *fakeEngine) MountVolume(_ context.Context, _, name, mount string, rw bool) error { + f.mu.Lock() + defer f.mu.Unlock() + f.volumeOps = append(f.volumeOps, "mount:"+name+":"+mount+":"+volumeMode(rw)) + return nil +} + +func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.volumeOps = append(f.volumeOps, "umount:"+mount) + return nil +} + +// Removals join the trace only for VMs that carried a volume, so warm-pool +// churn cannot perturb the order. +func (f *fakeEngine) volumeOpsLog() []string { + f.mu.Lock() + defer f.mu.Unlock() + return slices.Clone(f.volumeOps) +} func (f *fakeEngine) create(name string) (string, error) { f.mu.Lock() @@ -134,3 +167,10 @@ func (f *fakeEngine) createRecord(name string) (types.VMRecord, error) { } return types.VMRecord{VsockSocket: sock, Config: types.VMConfig{Name: name}}, nil } + +func volumeMode(rw bool) string { + if rw { + return types.VolumeModeRW + } + return types.VolumeModeRO +} From e1bd049fc8770f7b421b328f9491f67999c05f19 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 00:31:12 +0800 Subject: [PATCH 05/10] docs, sdk/python: fix volume drift (writable omitempty, volumes_rw, entry-vs-image) --- README.md | 12 +++++++----- docs/cluster.md | 34 ++++++++++++++++++++------------- docs/sandboxd-api.md | 12 ++++++------ docs/sdk-python.md | 5 ++++- sdk/python/tests/test_client.py | 32 ++++++++++++++++++++++--------- 5 files changed, 61 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 26088a7..2ab717b 100644 --- a/README.md +++ b/README.md @@ -33,9 +33,10 @@ performance) — source in baked into the base image - `sandboxd/` — per-node control plane (Go): warm pools refilled from golden snapshot exports (online-retunable), claim/release/hibernate/fork/promote/ - checkpoint HTTP API, operator-catalog read-only dataset volumes, signed - preview URLs, the HTTP-upgrade byte relay to silkd, usage + audit journals, - /metrics, reap + restart reconcile, memberlist mesh with redirect placement + checkpoint HTTP API, operator-catalog dataset volumes (read-only or + writable), signed preview URLs, the HTTP-upgrade byte relay to silkd, + usage + audit journals, /metrics, reap + restart reconcile, memberlist + mesh with redirect placement - `sdk/go/` — Go SDK (stdlib-only): `Connect/New/Lookup`, `Exec/Run`, files, `Push/Pull`, sessions, `Find/Replace`, `Watch`, git verbs, `OpenPty`, `Fork/Hibernate/Promote/Checkpoint`, `DialPort/ProxyPort/PreviewURL`, @@ -103,8 +104,9 @@ TEMPLATE=rt:24.04 scripts/sandboxd-e2e.sh # `ip link add br0 type bridge` with no uplink is enough (NIC, not network). # SANDBOXD_BIN/DEMO_BIN/SMOKE_BIN point at prebuilt binaries for nodes # without a Go toolchain. -# VOLUME_IMAGE=/absolute/dataset.img enables the read-only sharing proof; the -# image contains volume-e2e.txt. Prebuilt runs also set VOLUME_SMOKE_BIN. +# VOLUME_IMAGE=/absolute/dataset.img enables the volumes proof (ro sharing +# + rw claim/release); the image contains volume-e2e.txt. Prebuilt runs +# also set VOLUME_SMOKE_BIN. ``` ## CI diff --git a/docs/cluster.md b/docs/cluster.md index 4ca5b12..d18c3a9 100644 --- a/docs/cluster.md +++ b/docs/cluster.md @@ -67,15 +67,21 @@ and access lists never leave the node. After config load the set appears on the next gossip tick; later image distribution or removal is detected the same way. The node epoch bumps only when the advertised name set changes. -A writable name (`writable: true`) is expected to have exactly one holder -fleet-wide — the operator contract in -[deploy](deploy.md#writable-dataset-volumes), not a mechanism this layer -enforces. Because a node only ever advertises catalog names it actually -holds, every claim for that name — `ro` or `rw` — already resolves to the -single node that has it through the ordinary redirect logic below; there is -no new gossip field or admission message for writable routing. Configuring -the same writable name on two nodes is an operator error the fleet has no way -to detect. +A writable name (`writable: true`) still needs its catalog entry — name, +access list, and the `writable` flag — declared identically on every node +meant to serve it, the same rule the read-only case already needs: gossip +carries only currently-available names, never ACL or `writable` metadata, so +a tenant claim landing on a node with no local entry for that name is a hard +error (unknown/forbidden), not a gossip redirect. Root tokens carry no ACL to +enforce and are exempt — they can still redirect off gossip alone. What a +writable entry adds is a constraint on the backing *image file*, not the +entry: exactly one of those nodes should actually have the file present at +the configured path (the operator contract in +[deploy](deploy.md#writable-dataset-volumes)); the others log a missing-path +warning and never advertise the name, so an ordinary claim still funnels to +that one holder. Two nodes both holding the writable file is the operator +error the fleet cannot detect — two nodes both declaring the entry is normal +and expected. A volume claim may consume an ordinary warm VM because attach happens after the pop and before finalization. Warm candidates retain their normal ranking, but a @@ -273,7 +279,9 @@ the mesh. - `cluster_key` set if the gossip network is not otherwise trusted - pool changes via `Client.SetPoolsCluster` (or per-node `SetPools`); the applied set persists to `pools.json` and survives restart -- keep each volume name's dataset identity and access list identical across - the fleet; distribute a read-only image to every node meant to advertise - it, but a writable (`writable: true`) image belongs on exactly one node — - verify the fleet view and holder count with `GET /v1/volumes` +- declare each volume's catalog entry — name, access list, and `writable` + — identically on every node meant to serve it (a tenant claim gets a hard + error, not a redirect, on a node missing the entry); put the actual image + file on every one of those nodes for a read-only name, but keep a writable + name's file on exactly one of them — verify the fleet view and holder + count with `GET /v1/volumes` diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index ca92966..c1bb360 100644 --- a/docs/sandboxd-api.md +++ b/docs/sandboxd-api.md @@ -122,8 +122,7 @@ caller may use, without host paths or holder addresses: ```json {"volumes": [{"name": "imagenet", "default_mount": "/volumes/imagenet", - "size_bytes": 214748364800, "available": true, "nodes": 3, - "writable": false}]} + "size_bytes": 214748364800, "available": true, "nodes": 3}]} ``` Root sees every entry; a tenant sees unrestricted entries plus those whose @@ -132,7 +131,8 @@ advertising the name. `size_bytes` and `available` are a best-effort stat of the answering node's image, so a peer-only entry remains discoverable with `available: false`. Membership is eventually consistent by one gossip tick. `writable` is the entry's catalog configuration, fleet-uniform like the access -list, not per-node state. +list; the field is emitted (as `true`) only for a writable entry and omitted +otherwise, so a read-only entry's response is byte-identical to v1. ## POST /v1/sandboxes/{id}/release @@ -425,9 +425,9 @@ Always on: every lifecycle transition appends one JSONL event to "id": "sb_…", "vm": "sbx-…"}` plus `key` and `tenant` (the pool key and owning tenant, claim events), `children` (fork) and `ref` (the promoted template / checkpoint id, or the egress host). A volume claim also carries -`volumes`, the applied catalog names, each flagged when claimed `rw` so -billing can discriminate write access (mounts and host paths are not billing -dimensions). The file rotates at +`volumes`, the applied catalog names, and — omitted when empty — `volumes_rw`, +the subset of those names claimed `rw`, so billing can discriminate write +access (mounts and host paths are not billing dimensions). The file rotates at 64 MiB keeping one `.1` backup, so a tailing collector never loses a window silently. Folding rules: billable compute seconds per sandbox = Σ(claim→release/reap) − Σ(hibernate→wake); hibernated storage seconds = diff --git a/docs/sdk-python.md b/docs/sdk-python.md index 91f198d..b44b70c 100644 --- a/docs/sdk-python.md +++ b/docs/sdk-python.md @@ -123,9 +123,12 @@ Hypervisor. for volume in client.volumes(): print(volume["name"], volume["default_mount"], volume["size_bytes"], volume["available"], volume["nodes"], - volume["writable"]) + volume.get("writable", False)) ``` +`writable` is present (and `true`) only for a writable entry; a read-only +entry omits the key, so read it with `.get`, not `[...]`. + Discovery returns the gossiped union and holder count; availability and size describe the connected node. Warm candidates retain normal ranking, filtered to nodes advertising every requested name. A promoted-template claim prefers a diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index a9e4824..d5dc24f 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -183,16 +183,30 @@ def test_volume_catalog(node): def test_volume_catalog_surfaces_writable(node): - want = [{ - "name": "scratch", - "default_mount": "/data", - "size_bytes": 42, - "available": True, - "nodes": 1, - "writable": True, - }] + # The server omits "writable" for read-only entries (json omitempty); the + # client passes both shapes through as-is, key present or absent. + want = [ + { + "name": "scratch", + "default_mount": "/data", + "size_bytes": 42, + "available": True, + "nodes": 1, + "writable": True, + }, + { + "name": "imagenet", + "default_mount": "/volumes/imagenet", + "size_bytes": 42, + "available": True, + "nodes": 3, + }, + ] FakeNode.routes[("GET", "/v1/volumes")] = lambda body, path: (200, {"volumes": want}) - assert Client(node).volumes() == want + got = Client(node).volumes() + assert got == want + assert got[0]["writable"] is True + assert "writable" not in got[1] def test_promote_returns_content_digest(node): From ed6dca5ecf03107760aad1d8e94b9360a0089e79 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 00:49:37 +0800 Subject: [PATCH 06/10] pool, engine: hold volume admission and dirty markers until removal confirms A live VM keeps the hypervisor's image lock until it is gone, so releasing the admission hold or clearing the dirty marker before removal confirms let a failed removal re-dirty a clean image and turned contracted 409s into attach-time failures. Quiesce now records per-mount outcomes, the payload rides pendingRemoval across retries, and readers re-check markers after reservation. Umount failures fall back to one guest-wide sync; the quiesce budget scales with the writable mount count. --- e2e/fakeengine_test.go | 7 ++ sandboxd/config/config.go | 5 +- sandboxd/engine/volume.go | 24 ++++- sandboxd/engine/volume_test.go | 24 ++++- sandboxd/pool/claim.go | 23 ++-- sandboxd/pool/pool.go | 2 + sandboxd/pool/pool_test.go | 43 ++++++-- sandboxd/pool/reconcile.go | 4 +- sandboxd/pool/remove.go | 28 +++-- sandboxd/pool/volume.go | 104 +++++++++++------- sandboxd/pool/volume_rw_test.go | 180 +++++++++++++++++++++++++++++--- 11 files changed, 360 insertions(+), 84 deletions(-) diff --git a/e2e/fakeengine_test.go b/e2e/fakeengine_test.go index 575704c..7e61242 100644 --- a/e2e/fakeengine_test.go +++ b/e2e/fakeengine_test.go @@ -138,6 +138,13 @@ func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { return nil } +func (f *fakeEngine) SyncGuest(_ context.Context, _ string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.volumeOps = append(f.volumeOps, "sync") + return nil +} + // Removals join the trace only for VMs that carried a volume, so warm-pool // churn cannot perturb the order. func (f *fakeEngine) volumeOpsLog() []string { diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index fdfb3f6..16c4367 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -112,8 +112,9 @@ type TenantSpec struct { Egress *egress.Policy `json:"egress,omitempty"` } -// VolumeSpec declares one operator-managed dataset disk. Writable admits rw -// claims of this image; a writable entry is held by exactly one node. +// VolumeSpec declares one operator-managed dataset disk. The entry — name, +// access list, mode — is declared fleet-wide; Writable admits rw claims, and +// the image at Path is held by exactly one node. type VolumeSpec struct { Name string `json:"name"` Path string `json:"path"` diff --git a/sandboxd/engine/volume.go b/sandboxd/engine/volume.go index 0347692..c85b72f 100644 --- a/sandboxd/engine/volume.go +++ b/sandboxd/engine/volume.go @@ -13,10 +13,13 @@ import ( ) const ( - volumePollInterval = 10 * time.Millisecond - volumeProbeTimeout = 2 * time.Second - volumeSetupTimeout = 10 * time.Second - volumeUmountTimeout = 2 * time.Second + // VolumeCallTimeout bounds one guest teardown exec; the pool scales its + // whole quiesce budget from it. + VolumeCallTimeout = 2 * time.Second + + volumePollInterval = 10 * time.Millisecond + volumeProbeTimeout = 2 * time.Second + volumeSetupTimeout = 10 * time.Second ) // VolumeSpec describes one operator-owned disk image attached to a sandbox. @@ -69,7 +72,7 @@ func (e *Engine) MountVolume(ctx context.Context, vsockSocket, name, mount strin // UnmountVolume flushes and detaches a guest mount, so a writable image's // dirty state reaches the backing file before the VM is removed. func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, mount string) error { - ctx, cancel := context.WithTimeout(ctx, volumeUmountTimeout) + ctx, cancel := context.WithTimeout(ctx, VolumeCallTimeout) defer cancel() if err := e.silkdExec(ctx, vsockSocket, "umount", "--", mount); err != nil { return fmt.Errorf("unmount volume %s: %w", mount, err) @@ -77,6 +80,17 @@ func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, mount string) e return nil } +// SyncGuest flushes the guest page cache — the only flush left for a mount that +// refused to unmount, since a lazy unmount would keep writing behind us. +func (e *Engine) SyncGuest(ctx context.Context, vsockSocket string) error { + ctx, cancel := context.WithTimeout(ctx, VolumeCallTimeout) + defer cancel() + if err := e.silkdExec(ctx, vsockSocket, "sync"); err != nil { + return fmt.Errorf("sync guest: %w", err) + } + return nil +} + func (e *Engine) waitForVolumeDevice(ctx context.Context, vsockSocket, name string) (string, error) { ctx, cancel := context.WithTimeout(ctx, volumeProbeTimeout) defer cancel() diff --git a/sandboxd/engine/volume_test.go b/sandboxd/engine/volume_test.go index 4c07aec..72ab3c2 100644 --- a/sandboxd/engine/volume_test.go +++ b/sandboxd/engine/volume_test.go @@ -98,8 +98,8 @@ func TestMountVolumeUsesSysfsAndRequestedMode(t *testing.T) { } func TestUnmountVolumeExecsBoundedUmount(t *testing.T) { - if volumeUmountTimeout != 2*time.Second { - t.Fatalf("volume umount timeout = %s, want 2s", volumeUmountTimeout) + if VolumeCallTimeout != 2*time.Second { + t.Fatalf("volume call timeout = %s, want 2s", VolumeCallTimeout) } path := sockPath(t) fake := serveFakeSilkd(t, path) @@ -121,6 +121,26 @@ func TestUnmountVolumeExecsBoundedUmount(t *testing.T) { } } +func TestSyncGuestExecsPlainSync(t *testing.T) { + path := sockPath(t) + fake := serveFakeSilkd(t, path) + e := New("cocoon", nil, nil, false, "") + if err := e.SyncGuest(t.Context(), path); err != nil { + t.Fatalf("SyncGuest: %v", err) + } + + fake.mu.Lock() + wantExec := [][]string{{"sync"}} + if !slices.EqualFunc(fake.execCalls, wantExec, slices.Equal) { + t.Errorf("exec calls = %v, want %v", fake.execCalls, wantExec) + } + fake.execCode, fake.execFailAt = 1, 2 + fake.mu.Unlock() + if err := e.SyncGuest(t.Context(), path); err == nil || !strings.Contains(err.Error(), "sync guest") { + t.Errorf("got %v, want sync failure", err) + } +} + func TestMountVolumeWaitsForDelayedSysfsSerial(t *testing.T) { path := sockPath(t) fake := serveFakeSilkd(t, path) diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 7c3dfb6..35022fa 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -60,6 +60,10 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur return nil, ErrNoWarm } m.kickRefill() + if cleanErr := m.confirmVolumesClean(volumeSpecs); cleanErr != nil { + m.destroy(ctx, sb.VMName) + return nil, cleanErr + } if volumeErr := m.applyVolumes(ctx, sb, volumeSpecs); volumeErr != nil { m.destroy(ctx, sb.VMName) return nil, volumeErr @@ -189,9 +193,11 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM m.untrack(m.pendingCks, ck) } - m.teardownVolumes(ctx, sb, true) + td := m.quiesceVolumes(ctx, sb) var err error - if vmName != "" && !m.removeOrRetry(ctx, vmName, id, "") { + if vmName == "" { + m.finishVolumeTeardown(ctx, td) // archived: no VM to confirm gone + } else if !m.removeClaimVM(ctx, vmName, id, td) { err = fmt.Errorf("vm %s survived removal", vmName) } m.disarmEgress(id, err == nil) @@ -276,7 +282,7 @@ func (m *Manager) finalizeBatch(ctx context.Context, sbs []*types.Sandbox, ttl t for _, sb := range sbs { // No quiesce: the claim was never handed out, so nothing wrote to // the guest and the marker converges on the next writable claim. - m.teardownVolumes(ctx, sb, false) + m.unreserveVolumes(sb.Volumes) m.destroy(ctx, sb.VMName) } return quotaErr @@ -323,8 +329,8 @@ func (m *Manager) rollbackClaim(ctx context.Context, sbs []*types.Sandbox) { m.mu.Unlock() m.recommit(ctx, rb) for _, sb := range sbs { - m.teardownVolumes(ctx, sb, true) - m.disarmEgress(sb.ID, m.removeOrRetry(ctx, sb.VMName, sb.ID, "")) + td := m.quiesceVolumes(ctx, sb) + m.disarmEgress(sb.ID, m.removeClaimVM(ctx, sb.VMName, sb.ID, td)) } } @@ -436,8 +442,8 @@ func (m *Manager) reapOnce(ctx context.Context) { case reapArchive: logSweepResult(ctx, logger, m.archive(ctx, v.sb), "archived expired sandbox "+v.id, "archive expired sandbox "+v.id) default: - m.teardownVolumes(ctx, v.sb, true) - m.disarmEgress(v.id, m.removeOrRetry(ctx, v.vmName, v.id, "")) + td := m.quiesceVolumes(ctx, v.sb) + m.disarmEgress(v.id, m.removeClaimVM(ctx, v.vmName, v.id, td)) m.dropSnap(ctx, v.snap) m.counters.reaps.Add(1) m.recordUsage(ctx, usageEvent{Event: "reap", ID: v.id, VMName: v.vmName}) @@ -507,6 +513,9 @@ func (m *Manager) claimProvision(ctx context.Context, key types.PoolKey, ttl tim return nil, admitErr } reserved = applied + if cleanErr := m.confirmVolumesClean(volumeSpecs); cleanErr != nil { + return nil, cleanErr + } golden, err := m.resolveGolden(ctx, key) if err != nil { return nil, fmt.Errorf("resolve template: %w", err) diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index a5e47d8..f78b0ba 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -122,6 +122,7 @@ type Engine interface { DiskAttach(ctx context.Context, vmName string, spec engine.VolumeSpec) error MountVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error UnmountVolume(ctx context.Context, vsockSocket, mount string) error + SyncGuest(ctx context.Context, vsockSocket string) error } // SandboxSummary is the ops view of one live claim — no tokens. @@ -165,6 +166,7 @@ type pendingRemoval struct { sandboxID string tap string staleCreate bool + volumes volumeTeardown } type pool struct { diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index b574473..dfd57e5 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -765,11 +765,17 @@ type fakeEngine struct { volumeSpecs []engine.VolumeSpec volumeMounts []types.Volume volumeOps []string - // attachDirty records whether an image's dirty marker was already down - // when it was attached; removeSeenOps snapshots the volume-op log at each - // VM removal, so teardown ordering is checkable across the two logs. - attachDirty map[string]bool - removeSeenOps map[string][]string + // attachDirty records whether an image's dirty marker was already down when + // it was attached; removeSeenOps and removeSeenDirty snapshot the volume-op + // log and the markers still on disk at each VM removal, so teardown ordering + // is checkable on both sides of the removal. + attachDirty map[string]bool + removeSeenOps map[string][]string + removeSeenDirty map[string][]string + // dirtyPaths are the images removeSeenDirty samples; tests set it to the + // catalog paths the claim under test uses. + dirtyPaths []string + syncs []string // vsock sockets SyncGuest was called on hibernates, restores, snapRemoves []string snapSaves, exports, snapshots []string @@ -807,7 +813,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{}, - attachDirty: map[string]bool{}, removeSeenOps: map[string][]string{}, + attachDirty: map[string]bool{}, removeSeenOps: map[string][]string{}, removeSeenDirty: map[string][]string{}, } } @@ -848,6 +854,11 @@ func (f *fakeEngine) Remove(ctx context.Context, name string) error { return errors.New("remove failed") } f.removeSeenOps[name] = slices.Clone(f.volumeOps) + for _, path := range f.dirtyPaths { + if volumeDirty(path) { + f.removeSeenDirty[name] = append(f.removeSeenDirty[name], path) + } + } f.removes = append(f.removes, name) delete(f.vms, name) return nil @@ -1036,6 +1047,14 @@ func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { return f.unmountVolumeErr } +func (f *fakeEngine) SyncGuest(_ context.Context, vsockSocket string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.syncs = append(f.syncs, vsockSocket) + f.volumeOps = append(f.volumeOps, "sync") + return nil +} + func (f *fakeEngine) clone(from, name string) (types.VMRecord, error) { f.mu.Lock() f.clones = append(f.clones, name) @@ -1098,6 +1117,18 @@ func (f *fakeEngine) opsAtRemoval(vmName string) []string { return f.removeSeenOps[vmName] } +func (f *fakeEngine) dirtyAtRemoval(vmName string) []string { + f.mu.Lock() + defer f.mu.Unlock() + return f.removeSeenDirty[vmName] +} + +func (f *fakeEngine) syncCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.syncs) +} + func (f *fakeEngine) volumeOpsLog() []string { f.mu.Lock() defer f.mu.Unlock() diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index 0cd520b..f4dcce7 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -219,8 +219,8 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco // A failed remove stays out of service and queued until teardown succeeds. func (m *Manager) quarantineClaim(ctx context.Context, sb *types.Sandbox) bool { - m.teardownVolumes(ctx, sb, true) - gone := m.removeOrRetry(ctx, sb.VMName, sb.ID, "") + td := m.quiesceVolumes(ctx, sb) + gone := m.removeClaimVM(ctx, sb.VMName, sb.ID, td) m.mu.Lock() delete(m.claimed, sb.ID) m.tenantDelta(sb.Tenant, -1) diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go index 1fae125..e324edb 100644 --- a/sandboxd/pool/remove.go +++ b/sandboxd/pool/remove.go @@ -49,13 +49,26 @@ func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID, tap string if m.removeVM(ctx, name) { return true } - m.queueRemoval(name, sandboxID, tap) + m.queueRemoval(name, sandboxID, tap, volumeTeardown{}) return false } -func (m *Manager) queueRemoval(name, sandboxID, tap string) { +// removeClaimVM removes a quiesced claim's VM and finishes its volume teardown, +// which only a confirmed-gone VM may do: a survivor still holds the images, so +// the payload rides the retry queue instead. A restart loses it — the markers +// then stay until an rw claim clears them, and the holds die with the process. +func (m *Manager) removeClaimVM(ctx context.Context, name, sandboxID string, td volumeTeardown) bool { + if m.removeVM(ctx, name) { + m.finishVolumeTeardown(ctx, td) + return true + } + m.queueRemoval(name, sandboxID, "", td) + return false +} + +func (m *Manager) queueRemoval(name, sandboxID, tap string, td volumeTeardown) { m.mu.Lock() - m.pendingRemovals[name] = pendingRemoval{sandboxID: sandboxID, tap: tap} + m.pendingRemovals[name] = pendingRemoval{sandboxID: sandboxID, tap: tap, volumes: td} m.mu.Unlock() } @@ -94,18 +107,19 @@ func (m *Manager) retryRemoval(ctx context.Context, name string, pending pending m.queueStaleCreate(name, pending.tap) return case outcome == engine.StaleCreateCollected, outcome == engine.StaleCreateNotFound: - m.finishRemoval(pending) + m.finishRemoval(ctx, pending) return } } if !m.removeVM(ctx, name) { - m.queueRemoval(name, pending.sandboxID, pending.tap) + m.queueRemoval(name, pending.sandboxID, pending.tap, pending.volumes) return } - m.finishRemoval(pending) + m.finishRemoval(ctx, pending) } -func (m *Manager) finishRemoval(pending pendingRemoval) { +func (m *Manager) finishRemoval(ctx context.Context, pending pendingRemoval) { + m.finishVolumeTeardown(ctx, pending.volumes) if pending.sandboxID != "" { m.disarmEgress(pending.sandboxID, true) } else if pending.tap != "" { diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index 44294cc..836295c 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -20,8 +20,8 @@ const ( // Sidecar, not data_dir: the marker travels with the image and survives a // data_dir wipe. volumeDirtySuffix = ".dirty" - // Bounds the whole quiesce: teardown must not hang on a wedged guest. - volumeQuiesceTimeout = 5 * time.Second + // Caps the per-mount budget below, so teardown can never hang for long. + volumeQuiesceMax = 10 * time.Second ) type catalogVolume struct { @@ -46,6 +46,15 @@ type resolvedVolume struct { applied types.Volume } +// volumeTeardown is what a quiesced claim leaves for after its VM is confirmed +// gone: the admission holds to drop, and the marker paths — resolved through +// the catalog while the claim still existed — of the mounts that came down +// cleanly. +type volumeTeardown struct { + holds []types.Volume + clears []string +} + // Volumes reports the caller-visible fleet catalog, projected through this // node's ACL metadata and local path state. Empty tenant means root. func (m *Manager) Volumes(tenant string, holders map[string]int) []types.VolumeInfo { @@ -148,8 +157,7 @@ func (m *Manager) resolveVolumes(key types.PoolKey, tenant string, requested []t if _, statErr := os.Stat(entry.disk.Path); statErr != nil { return nil, fmt.Errorf("volume %q path %q: %w", volume.Name, entry.disk.Path, statErr) } - // A live writer's own marker is expected; admission answers that conflict. - if !volume.RW() && volumeDirty(entry.disk.Path) && !m.volumeHeld(volume.Name) { + if !volume.RW() && volumeDirty(entry.disk.Path) { return nil, fmt.Errorf("%w: volume %q", ErrVolumeNeedsRecovery, volume.Name) } disk := entry.disk @@ -159,6 +167,19 @@ func (m *Manager) resolveVolumes(key types.PoolKey, tenant string, requested []t return resolved, nil } +// confirmVolumesClean re-stats the read-only entries' markers once the holds +// are taken, closing the gap the resolve-time check leaves: a writable claim +// that failed between the two marks its image and releases, and mounting that +// image read-only would fail deep in guest setup instead of here. +func (m *Manager) confirmVolumesClean(volumes []resolvedVolume) error { + for _, volume := range volumes { + if !volume.applied.RW() && volumeDirty(volume.disk.Path) { + return fmt.Errorf("%w: volume %q", ErrVolumeNeedsRecovery, volume.applied.Name) + } + } + return nil +} + func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes []resolvedVolume) error { if len(volumes) == 0 { return nil @@ -181,46 +202,56 @@ func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes [ return nil } -// teardownVolumes quiesces the guest (paths that still own a live VM, before -// it is removed) and releases the admission holds. Exactly once per claim: a -// leaked hold keeps the name unclaimable until the daemon restarts. -func (m *Manager) teardownVolumes(ctx context.Context, sb *types.Sandbox, quiesce bool) { - if len(sb.Volumes) == 0 { - return - } - if quiesce { - m.quiesceVolumes(ctx, sb) - } - m.unreserveVolumes(sb.Volumes) -} - -// quiesceVolumes unmounts the writable mounts in reverse order, clearing the -// marker of each image that unmounted cleanly. A failure never blocks -// teardown: the surviving marker routes the image to a recovering writer. -func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) { - if !slices.ContainsFunc(sb.Volumes, types.Volume.RW) { - return +// quiesceVolumes unmounts a claim's writable mounts in reverse order and +// returns the teardown its VM removal must finish. A failed unmount never +// blocks teardown: the image keeps its marker and waits for a recovering +// writer. Runs while the guest is still live, before the VM is removed. +func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeTeardown { + td := volumeTeardown{holds: slices.Clone(sb.Volumes)} + mounts := len(types.VolumeRWNames(sb.Volumes)) + if mounts == 0 { + return td } logger := log.WithFunc("pool.quiesceVolumes") // Cancellation-immune like removal: a caller hanging up must not skip the flush. - ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), volumeQuiesceTimeout) + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), quiesceBudget(mounts)) defer cancel() + stuck := false for _, volume := range slices.Backward(sb.Volumes) { if !volume.RW() { continue } if err := m.eng.UnmountVolume(ctx, sb.VsockSocket, volume.Mount); err != nil { logger.Errorf(ctx, err, "unmount volume %s of %s", volume.Name, sb.ID) + stuck = true continue } - entry, ok := m.volumes[volume.Name] - if !ok { - continue + if entry, ok := m.volumes[volume.Name]; ok { + td.clears = append(td.clears, entry.disk.Path) } - if err := clearVolumeDirty(entry.disk.Path); err != nil { - logger.Errorf(ctx, err, "clear dirty marker of volume %s", volume.Name) + } + if stuck { + if err := m.eng.SyncGuest(ctx, sb.VsockSocket); err != nil { + logger.Errorf(ctx, err, "sync guest of %s", sb.ID) } } + return td +} + +// finishVolumeTeardown completes teardown once the VM is confirmed gone: only +// then is the hypervisor's image lock released, so an earlier marker clear or +// hold release would hand the name to a claim the still-live writer blocks. +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; every name is checked before any @@ -267,12 +298,6 @@ func (m *Manager) releaseVolumes(volumes []types.Volume) { } } -func (m *Manager) volumeHeld(name string) bool { - m.mu.Lock() - defer m.mu.Unlock() - return m.volumeAdmission[name].writers > 0 -} - func (m *Manager) unreserveVolumes(volumes []types.Volume) { if len(volumes) == 0 { return @@ -282,6 +307,12 @@ func (m *Manager) unreserveVolumes(volumes []types.Volume) { m.releaseVolumes(volumes) } +// quiesceBudget bounds one quiesce: a slice per writable mount plus one for the +// sync fallback, so a wedged multi-volume guest still gets every umount tried. +func quiesceBudget(mounts int) time.Duration { + return min(time.Duration(mounts+1)*engine.VolumeCallTimeout, volumeQuiesceMax) +} + func appliedVolumes(volumes []resolvedVolume) []types.Volume { applied := make([]types.Volume, len(volumes)) for i, volume := range volumes { @@ -309,9 +340,10 @@ func clearVolumeDirty(path string) error { return nil } +// volumeDirty fails closed: an unreadable marker must not read as a clean image. func volumeDirty(path string) bool { _, err := os.Stat(volumeDirtyPath(path)) - return err == nil + return !errors.Is(err, os.ErrNotExist) } func volumeDirtyPath(path string) string { diff --git a/sandboxd/pool/volume_rw_test.go b/sandboxd/pool/volume_rw_test.go index 7da5c44..210a254 100644 --- a/sandboxd/pool/volume_rw_test.go +++ b/sandboxd/pool/volume_rw_test.go @@ -11,12 +11,14 @@ import ( "time" "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/engine" "github.com/cocoonstack/sandbox/sandboxd/types" ) func TestWritableClaimMarksDirtyBeforeAttachAndClearsOnRelease(t *testing.T) { path := writeVolumeImage(t, "scratch.img", "data") eng := newFakeEngine() + eng.dirtyPaths = []string{path} m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) @@ -49,6 +51,12 @@ func TestWritableClaimMarksDirtyBeforeAttachAndClearsOnRelease(t *testing.T) { if seen := eng.opsAtRemoval(sb.VMName); !slices.Contains(seen, "umount:/volumes/scratch") { t.Errorf("ops at removal=%v, want the unmount already done", seen) } + if seen := eng.dirtyAtRemoval(sb.VMName); !slices.Contains(seen, path) { + t.Errorf("markers at removal=%v, want the image still marked until the VM is gone", seen) + } + if got := eng.syncCount(); got != 0 { + t.Errorf("guest syncs=%d, want none when every unmount succeeded", got) + } if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{}) { t.Errorf("registry after release=%+v, want empty", holders) } @@ -106,16 +114,54 @@ func TestDirtyVolumeBlocksReadersUntilWriterRecovers(t *testing.T) { } } +func TestConfirmVolumesCleanCatchesMarkerAfterAdmission(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + m := newVolumeManager(t, newFakeEngine(), []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + readOnly := []types.Volume{{Name: "scratch"}} + + // The reader resolves a clean image; a writable claim then fails between + // that resolve and admission, leaving its marker but no hold behind. + resolved, err := m.resolveVolumes(testKey, "", readOnly) + if err != nil { + t.Fatalf("resolveVolumes: %v", err) + } + 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 { + t.Fatalf("reserve once the writer released: %v", reserveErr) + } + + if cleanErr := m.confirmVolumesClean(resolved); !errors.Is(cleanErr, ErrVolumeNeedsRecovery) { + t.Errorf("reader over a marker that landed after resolve: %v, want ErrVolumeNeedsRecovery", cleanErr) + } + m.unreserveVolumes(appliedVolumes(resolved)) + + writable, err := m.resolveVolumes(testKey, "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + if err != nil { + t.Fatalf("resolveVolumes writable: %v", err) + } + if cleanErr := m.confirmVolumesClean(writable); cleanErr != nil { + t.Errorf("writable recovery claim over a dirty image: %v, want it admitted", cleanErr) + } +} + func TestVolumeAdmissionMatrix(t *testing.T) { for _, tt := range []struct { name string first, second string - wantBusy bool + wantErr error }{ - {"writer excludes writer", types.VolumeModeRW, types.VolumeModeRW, true}, - {"writer excludes reader", types.VolumeModeRW, "", true}, - {"reader excludes writer", "", types.VolumeModeRW, true}, - {"readers share", "", "", false}, + {"writer excludes writer", types.VolumeModeRW, types.VolumeModeRW, ErrVolumeBusy}, + // A live writer keeps the image marked, so a reader is turned away by + // the marker before admission ever sees it. Admission's own rule for + // this direction is pinned in TestReserveVolumesExcludesReaderUnderWriter. + {"writer excludes reader", types.VolumeModeRW, "", ErrVolumeNeedsRecovery}, + {"reader excludes writer", "", types.VolumeModeRW, ErrVolumeBusy}, + {"readers share", "", "", nil}, } { t.Run(tt.name, func(t *testing.T) { path := writeVolumeImage(t, "scratch.img", "data") @@ -129,14 +175,14 @@ func TestVolumeAdmissionMatrix(t *testing.T) { } before := eng.volumeOpsLog() _, err = m.ClaimProvision(t.Context(), testKey, 0, "", "", second) - if !tt.wantBusy { + if tt.wantErr == nil { if err != nil { t.Fatalf("concurrent read-only claim: %v", err) } return } - if !errors.Is(err, ErrVolumeBusy) { - t.Fatalf("second claim: %v, want ErrVolumeBusy", err) + if !errors.Is(err, tt.wantErr) { + t.Fatalf("second claim: %v, want %v", err, tt.wantErr) } if ops := eng.volumeOpsLog(); !slices.Equal(ops, before) { t.Errorf("refused claim ran %v, want nothing past %v", ops, before) @@ -151,6 +197,21 @@ 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}}) + m.mu.Lock() + defer m.mu.Unlock() + m.adoptVolumes([]types.Volume{{Name: "scratch", Mount: "/volumes/scratch", Mode: types.VolumeModeRW}}) + + if err := m.reserveVolumes([]types.Volume{{Name: "scratch", Mount: "/volumes/scratch"}}); !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}) { + t.Errorf("registry after the refusal=%+v, want the writer alone", holders) + } +} + func TestVolumeAdmissionRefusalHoldsNothing(t *testing.T) { held := writeVolumeImage(t, "held.img", "held") free := writeVolumeImage(t, "free.img", "free") @@ -257,12 +318,19 @@ func TestReapQuiescesWritableVolumesBeforeRemoval(t *testing.T) { } } -func TestQuiesceFailureKeepsDirtyMarker(t *testing.T) { - path := writeVolumeImage(t, "scratch.img", "data") +func TestQuiesceFailureSyncsOnceAndKeepsMarkers(t *testing.T) { + first := writeVolumeImage(t, "first.img", "first") + second := writeVolumeImage(t, "second.img", "second") eng := newFakeEngine() eng.unmountVolumeErr = errors.New("umount: target is busy") - m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) - sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}) + m := newVolumeManager(t, eng, []config.VolumeSpec{ + {Name: "first", Path: first, Writable: true}, + {Name: "second", Path: second, Writable: true}, + }) + sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{ + {Name: "first", Mode: types.VolumeModeRW}, + {Name: "second", Mode: types.VolumeModeRW}, + }) if err != nil { t.Fatalf("ClaimProvision: %v", err) } @@ -270,14 +338,91 @@ func TestQuiesceFailureKeepsDirtyMarker(t *testing.T) { if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { t.Fatalf("Release: %v", err) } + if got := eng.syncCount(); got != 1 { + t.Errorf("guest syncs=%d, want exactly one fallback for the whole quiesce", got) + } + if !volumeDirty(first) || !volumeDirty(second) { + t.Error("failed unmounts cleared a dirty marker") + } + for _, name := range []string{"first", "second"} { + if holders := volumeHoldersOf(m, name); holders != (volumeHolders{}) { + t.Errorf("registry for %s after a failed unmount=%+v, want empty", name, holders) + } + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "first"}}); !errors.Is(err, ErrVolumeNeedsRecovery) { + t.Errorf("read-only claim after a failed unmount: %v, want ErrVolumeNeedsRecovery", err) + } +} + +func TestPendingRemovalHoldsVolumesUntilDrained(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + eng := newFakeEngine() + m := newVolumeManager(t, eng, []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + writable := []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}} + sb, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", writable) + if err != nil { + t.Fatalf("ClaimProvision: %v", err) + } + eng.removeErrFor = sb.VMName // survives removal: the VM still holds the image + + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err == nil { + t.Fatal("Release reported success for a VM that survived removal") + } if !volumeDirty(path) { - t.Error("failed unmount cleared the dirty marker") + t.Error("marker cleared while the VM still holds the image") + } + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", writable); !errors.Is(err, ErrVolumeBusy) { + t.Errorf("claim against a surviving VM: %v, want ErrVolumeBusy", err) + } + + eng.removeErrFor = "" + m.retryRemovals(t.Context()).Wait() + + if volumeDirty(path) { + t.Error("drained removal left the image dirty") } if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{}) { - t.Errorf("registry after a failed unmount=%+v, want empty", holders) + t.Errorf("registry after the drain=%+v, want empty", holders) } - if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}); !errors.Is(err, ErrVolumeNeedsRecovery) { - t.Errorf("read-only claim after a failed unmount: %v, want ErrVolumeNeedsRecovery", err) + if _, err := m.ClaimProvision(t.Context(), testKey, 0, "", "", writable); err != nil { + t.Errorf("claim after the drain: %v", err) + } +} + +func TestQuiesceBudgetScalesWithMounts(t *testing.T) { + for _, tt := range []struct { + mounts int + want time.Duration + }{ + {1, 2 * engine.VolumeCallTimeout}, + {2, 3 * engine.VolumeCallTimeout}, + {4, volumeQuiesceMax}, + {8, volumeQuiesceMax}, + } { + if got := quiesceBudget(tt.mounts); got != tt.want { + t.Errorf("quiesceBudget(%d)=%s, want %s", tt.mounts, got, tt.want) + } + } +} + +func TestVolumeDirtyFailsClosed(t *testing.T) { + image := filepath.Join(t.TempDir(), "scratch.img") + if err := os.WriteFile(image, []byte("data"), 0o600); err != nil { + t.Fatalf("write image: %v", err) + } + if volumeDirty(image) { + t.Error("image without a marker reads dirty") + } + if err := markVolumeDirty(image); err != nil { + t.Fatalf("mark dirty: %v", err) + } + if !volumeDirty(image) { + t.Error("marked image reads clean") + } + // The marker of an image behind a non-directory parent cannot be read at + // all; an unreadable marker must never open the image to readers. + if !volumeDirty(filepath.Join(image, "nested.img")) { + t.Error("unreadable marker reads clean") } } @@ -299,7 +444,8 @@ func TestReconcileRebuildsVolumeAdmission(t *testing.T) { if holders := volumeHoldersOf(m2, "scratch"); holders != (volumeHolders{writers: 1}) { t.Errorf("adopted registry=%+v, want one writer", holders) } - if _, err := m2.ClaimProvision(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}); !errors.Is(err, ErrVolumeBusy) { + writable := []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}} + if _, err := m2.ClaimProvision(t.Context(), testKey, 0, "", "", writable); !errors.Is(err, ErrVolumeBusy) { t.Errorf("claim against an adopted writer: %v, want ErrVolumeBusy", err) } if err := m2.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { From f52484a7e3e938e9857533d03e96d4ff8ab97e74 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 00:51:15 +0800 Subject: [PATCH 07/10] scripts, docs: wire the writable volume leg into the testbed e2e --- docs/deploy.md | 4 ++++ scripts/sandboxd-e2e.sh | 37 ++++++++++++++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index 11da5c0..d6889ad 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -417,6 +417,10 @@ enforcement, warm-pool consumption, and an unchanged source checksum. On a node using prebuilt binaries, also set `VOLUME_SMOKE_BIN` beside `SANDBOXD_BIN`, `DEMO_BIN`, and `SMOKE_BIN`. +Add `VOLUME_RW_IMAGE=/srv/datasets/scratch.img` (a second, writable image) to +also run the writable leg: a durable write across release, second-writer +exclusion, and a clean read-only claim afterward. + ## Preview URLs `preview_listen` starts a second HTTP server that serves a sandbox's guest diff --git a/scripts/sandboxd-e2e.sh b/scripts/sandboxd-e2e.sh index 465b43e..b7a4a0f 100755 --- a/scripts/sandboxd-e2e.sh +++ b/scripts/sandboxd-e2e.sh @@ -13,6 +13,9 @@ VOLUME_IMAGE=${VOLUME_IMAGE:-} VOLUME_NAME=${VOLUME_NAME:-e2e-data} VOLUME_PROBE=${VOLUME_PROBE:-volume-e2e.txt} VOLUME_DIRECTIO=${VOLUME_DIRECTIO:-off} +# VOLUME_RW_IMAGE adds the writable leg; its own image is mutated by design, so it must not be VOLUME_IMAGE. +VOLUME_RW_IMAGE=${VOLUME_RW_IMAGE:-} +VOLUME_RW_NAME=${VOLUME_RW_NAME:-e2e-scratch} VOLUME_CHECKSUM="" if [[ -n $VOLUME_IMAGE ]]; then @@ -30,6 +33,20 @@ if [[ -n $VOLUME_IMAGE ]]; then VOLUME_CHECKSUM=$(sha256sum -- "$VOLUME_IMAGE" | awk '{print $1}') fi +VOLUME_RW_CHECKSUM="" +if [[ -n $VOLUME_RW_IMAGE ]]; then + [[ -n $VOLUME_IMAGE ]] || { echo "VOLUME_RW_IMAGE needs VOLUME_IMAGE: the writable leg extends the volume proof"; exit 1; } + [[ $VOLUME_RW_IMAGE == /* ]] || { echo "VOLUME_RW_IMAGE must be absolute"; exit 1; } + [[ -f $VOLUME_RW_IMAGE && -w $VOLUME_RW_IMAGE ]] || { echo "VOLUME_RW_IMAGE must be a writable file"; exit 1; } + [[ $VOLUME_RW_IMAGE != "$VOLUME_IMAGE" ]] || { echo "VOLUME_RW_IMAGE must differ from VOLUME_IMAGE: the writable leg mutates its image"; exit 1; } + [[ $VOLUME_RW_NAME =~ ^[a-z][a-z0-9_-]{0,19}$ && $VOLUME_RW_NAME != cocoon-* ]] || { + echo "VOLUME_RW_NAME must match ^[a-z][a-z0-9_-]{0,19}$ and not start with cocoon-" + exit 1 + } + [[ $VOLUME_RW_NAME != "$VOLUME_NAME" ]] || { echo "VOLUME_RW_NAME must differ from VOLUME_NAME"; exit 1; } + VOLUME_RW_CHECKSUM=$(sha256sum -- "$VOLUME_RW_IMAGE" | awk '{print $1}') +fi + DATA=$(mktemp -d /tmp/sandboxd-e2e.XXXXXX) DAEMON_PID="" @@ -111,10 +128,14 @@ VOLUME_LINE="" if [[ -n $VOLUME_IMAGE ]]; then VOLUME_CATALOG=$(jq -cn --arg name "$VOLUME_NAME" --arg path "$VOLUME_IMAGE" --arg directio "$VOLUME_DIRECTIO" \ '[{name: $name, path: $path, directio: $directio}]') + if [[ -n $VOLUME_RW_IMAGE ]]; then + VOLUME_CATALOG=$(jq -cn --argjson catalog "$VOLUME_CATALOG" --arg name "$VOLUME_RW_NAME" --arg path "$VOLUME_RW_IMAGE" \ + '$catalog + [{name: $name, path: $path, writable: true}]') + fi VOLUME_LINE="\"volumes\": $VOLUME_CATALOG," fi -echo "== start (pool: $TEMPLATE none/small warm=$WARM${BRIDGE:+, egress via $BRIDGE}${S3_ENDPOINT:+, s3 store at $S3_ENDPOINT}${VOLUME_IMAGE:+, volume $VOLUME_NAME})" +echo "== start (pool: $TEMPLATE none/small warm=$WARM${BRIDGE:+, egress via $BRIDGE}${S3_ENDPOINT:+, s3 store at $S3_ENDPOINT}${VOLUME_IMAGE:+, volume $VOLUME_NAME}${VOLUME_RW_IMAGE:+, writable volume $VOLUME_RW_NAME})" cat >"$DATA/config.json" <= .target)' >/dev/null 2>&1; then break @@ -169,7 +190,7 @@ if [[ -n $VOLUME_IMAGE ]]; then done warm_before=$(warm_claims) "$DATA/volumesmoke" -addr "$ADDR" -token "$TOKEN" -template "$TEMPLATE" \ - -volume "$VOLUME_NAME" -probe "$VOLUME_PROBE" + -volume "$VOLUME_NAME" -probe "$VOLUME_PROBE" ${VOLUME_RW_IMAGE:+-rw-volume "$VOLUME_RW_NAME"} warm_after=$(warm_claims) [[ $warm_after -ge $((warm_before + 2)) ]] || { echo "volume claims did not consume the two ready warm VMs: before=$warm_before after=$warm_after" @@ -182,6 +203,16 @@ if [[ -n $VOLUME_IMAGE ]]; then exit 1 } echo "volume backing checksum unchanged: $after_checksum" + # The writable image is mutated on purpose: an unchanged digest means the + # writer's bytes never reached the host file. + if [[ -n $VOLUME_RW_IMAGE ]]; then + rw_checksum=$(sha256sum -- "$VOLUME_RW_IMAGE" | awk '{print $1}') + [[ $rw_checksum != "$VOLUME_RW_CHECKSUM" ]] || { + echo "writable backing image unchanged after the writable leg: $rw_checksum" + exit 1 + } + echo "writable backing checksum advanced: $VOLUME_RW_CHECKSUM -> $rw_checksum" + fi fi echo "== reap: leaked 5s-ttl claim is destroyed by the owner" From b2098b2d3be2bf89b5d25ff015175aa89d98ecfd Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 01:05:33 +0800 Subject: [PATCH 08/10] review: fold duplicate paths and drop redundant copies from the volume round --- sandboxd/pool/claim.go | 21 +++++++---------- sandboxd/pool/reconcile.go | 4 ++-- sandboxd/pool/remove.go | 23 ++++++------------ sandboxd/pool/remove_test.go | 4 ++-- sandboxd/pool/volume.go | 8 ++++--- sandboxd/pool/volume_test.go | 6 +---- sdk/go/volumes_test.go | 45 +++++++++++++++++++----------------- 7 files changed, 49 insertions(+), 62 deletions(-) diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 35022fa..c241e98 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -34,19 +34,15 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur if err != nil { return nil, err } - if quotaErr := m.overQuota(1, tenant); quotaErr != nil { - return nil, quotaErr - } applied := appliedVolumes(volumeSpecs) // Holds belong to this path until finalize; past it the sandbox carries them. var reserved []types.Volume defer func() { m.unreserveVolumes(reserved) }() - m.mu.Lock() - if reserveErr := m.reserveVolumes(applied); reserveErr != nil { - m.mu.Unlock() - return nil, reserveErr + if admitErr := m.admitClaim(tenant, applied); admitErr != nil { + return nil, admitErr } reserved = applied + m.mu.Lock() var sb *types.Sandbox if p := m.pools[key]; p != nil { p.noteArrival(start) @@ -64,7 +60,7 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur m.destroy(ctx, sb.VMName) return nil, cleanErr } - if volumeErr := m.applyVolumes(ctx, sb, volumeSpecs); volumeErr != nil { + if volumeErr := m.applyVolumes(ctx, sb, volumeSpecs, applied); volumeErr != nil { m.destroy(ctx, sb.VMName) return nil, volumeErr } @@ -197,7 +193,7 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand var err error if vmName == "" { m.finishVolumeTeardown(ctx, td) // archived: no VM to confirm gone - } else if !m.removeClaimVM(ctx, vmName, id, td) { + } else if !m.removeOrRetry(ctx, vmName, id, "", td) { err = fmt.Errorf("vm %s survived removal", vmName) } m.disarmEgress(id, err == nil) @@ -330,7 +326,7 @@ func (m *Manager) rollbackClaim(ctx context.Context, sbs []*types.Sandbox) { m.recommit(ctx, rb) for _, sb := range sbs { td := m.quiesceVolumes(ctx, sb) - m.disarmEgress(sb.ID, m.removeClaimVM(ctx, sb.VMName, sb.ID, td)) + m.disarmEgress(sb.ID, m.removeOrRetry(ctx, sb.VMName, sb.ID, "", td)) } } @@ -443,7 +439,7 @@ func (m *Manager) reapOnce(ctx context.Context) { logSweepResult(ctx, logger, m.archive(ctx, v.sb), "archived expired sandbox "+v.id, "archive expired sandbox "+v.id) default: td := m.quiesceVolumes(ctx, v.sb) - m.disarmEgress(v.id, m.removeClaimVM(ctx, v.vmName, v.id, td)) + m.disarmEgress(v.id, m.removeOrRetry(ctx, v.vmName, v.id, "", td)) m.dropSnap(ctx, v.snap) m.counters.reaps.Add(1) m.recordUsage(ctx, usageEvent{Event: "reap", ID: v.id, VMName: v.vmName}) @@ -506,7 +502,6 @@ func (m *Manager) claimProvision(ctx context.Context, key types.PoolKey, ttl tim return nil, err } applied := appliedVolumes(volumeSpecs) - // Holds belong to this path until finalize; past it the sandbox carries them. var reserved []types.Volume defer func() { m.unreserveVolumes(reserved) }() if admitErr := m.admitClaim(tenant, applied); admitErr != nil { @@ -529,7 +524,7 @@ func (m *Manager) claimProvision(ctx context.Context, key types.PoolKey, ttl tim if err != nil { return nil, err } - if volumeErr := m.applyVolumes(ctx, sb, volumeSpecs); volumeErr != nil { + if volumeErr := m.applyVolumes(ctx, sb, volumeSpecs, applied); volumeErr != nil { m.destroy(ctx, sb.VMName) return nil, volumeErr } diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index f4dcce7..1ac31e3 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -169,7 +169,7 @@ func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRe } // not-creating: the record moved on under the lock; remove normally. } - return m.removeOrRetry(ctx, name, "", rec.TapDevice()) + return m.removeOrRetry(ctx, name, "", rec.TapDevice(), volumeTeardown{}) } // resyncEgress re-locks adopted egress claims after a restart, quarantines any @@ -220,7 +220,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco // A failed remove stays out of service and queued until teardown succeeds. func (m *Manager) quarantineClaim(ctx context.Context, sb *types.Sandbox) bool { td := m.quiesceVolumes(ctx, sb) - gone := m.removeClaimVM(ctx, sb.VMName, sb.ID, td) + gone := m.removeOrRetry(ctx, sb.VMName, sb.ID, "", td) m.mu.Lock() delete(m.claimed, sb.ID) m.tenantDelta(sb.Tenant, -1) diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go index e324edb..b90b2ff 100644 --- a/sandboxd/pool/remove.go +++ b/sandboxd/pool/remove.go @@ -44,25 +44,16 @@ func (m *Manager) confirmGone(ctx context.Context, name string) bool { // removeOrRetry reports whether the VM is confirmed gone; a survivor is queued // for the reap tick to retry, carrying what its cleanup needs (the sandbox ID -// when a claim owns the tap via egressTaps, the tap itself when none does). -func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID, tap string) bool { - if m.removeVM(ctx, name) { - return true - } - m.queueRemoval(name, sandboxID, tap, volumeTeardown{}) - return false -} - -// removeClaimVM removes a quiesced claim's VM and finishes its volume teardown, -// which only a confirmed-gone VM may do: a survivor still holds the images, so -// the payload rides the retry queue instead. A restart loses it — the markers -// then stay until an rw claim clears them, and the holds die with the process. -func (m *Manager) removeClaimVM(ctx context.Context, name, sandboxID string, td volumeTeardown) bool { +// when a claim owns the tap via egressTaps, the tap itself when none does, and +// the volume teardown only a confirmed-gone VM may finish — a survivor still +// holds the images). A restart loses a queued teardown: its markers stay until +// an rw claim clears them, and its holds die with the process. +func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID, tap string, td volumeTeardown) bool { if m.removeVM(ctx, name) { m.finishVolumeTeardown(ctx, td) return true } - m.queueRemoval(name, sandboxID, "", td) + m.queueRemoval(name, sandboxID, tap, td) return false } @@ -128,5 +119,5 @@ func (m *Manager) finishRemoval(ctx context.Context, pending pendingRemoval) { } func (m *Manager) destroy(ctx context.Context, name string) { - m.removeOrRetry(ctx, name, "", "") + m.removeOrRetry(ctx, name, "", "", volumeTeardown{}) } diff --git a/sandboxd/pool/remove_test.go b/sandboxd/pool/remove_test.go index 7331896..67f7e3a 100644 --- a/sandboxd/pool/remove_test.go +++ b/sandboxd/pool/remove_test.go @@ -17,7 +17,7 @@ func TestRemoveVMClassifiesOutcome(t *testing.T) { } delete(eng.vms, "survivor") - if !m.removeOrRetry(t.Context(), "survivor", "", "") { + if !m.removeOrRetry(t.Context(), "survivor", "", "", volumeTeardown{}) { t.Fatal("absent VM reported present") } m.mu.Lock() @@ -77,7 +77,7 @@ func TestRemovalRetryFinishesEgressCleanup(t *testing.T) { eng.removeErrFor = "survivor" m.egressTaps["sb_survivor"] = "tap-survivor" - removed := m.removeOrRetry(t.Context(), "survivor", "sb_survivor", "") + removed := m.removeOrRetry(t.Context(), "survivor", "sb_survivor", "", volumeTeardown{}) m.disarmEgress("sb_survivor", removed) if removed { t.Fatal("surviving VM reported gone") diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index 836295c..d4f0b46 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -180,7 +180,9 @@ func (m *Manager) confirmVolumesClean(volumes []resolvedVolume) error { return nil } -func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes []resolvedVolume) error { +// applyVolumes attaches and mounts the resolved set; applied is that same set +// in request shape, recorded on the sandbox only once every mount is up. +func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes []resolvedVolume, applied []types.Volume) error { if len(volumes) == 0 { return nil } @@ -198,7 +200,7 @@ func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes [ return fmt.Errorf("setup volume %q: %w", volume.applied.Name, err) } } - sb.Volumes = appliedVolumes(volumes) + sb.Volumes = applied return nil } @@ -207,7 +209,7 @@ func (m *Manager) applyVolumes(ctx context.Context, sb *types.Sandbox, volumes [ // blocks teardown: the image keeps its marker and waits for a recovering // writer. Runs while the guest is still live, before the VM is removed. func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeTeardown { - td := volumeTeardown{holds: slices.Clone(sb.Volumes)} + td := volumeTeardown{holds: sb.Volumes} mounts := len(types.VolumeRWNames(sb.Volumes)) if mounts == 0 { return td diff --git a/sandboxd/pool/volume_test.go b/sandboxd/pool/volume_test.go index 1b9d761..abb82dc 100644 --- a/sandboxd/pool/volume_test.go +++ b/sandboxd/pool/volume_test.go @@ -402,11 +402,7 @@ func TestClaimProvisionRejectsMissingVolumePathBeforeProvision(t *testing.T) { func newVolumeManager(t *testing.T, eng *fakeEngine, volumes []config.VolumeSpec) *Manager { t.Helper() - m, err := NewManager(t.Context(), &config.Config{DataDir: t.TempDir(), Volumes: volumes}, eng, testSecrets(t)) - if err != nil { - t.Fatalf("setup manager: %v", err) - } - return m + return newVolumeManagerAt(t, eng, t.TempDir(), volumes) } func newVolumePoolManager(t *testing.T, eng *fakeEngine, dataDir string, volumes []config.VolumeSpec) *Manager { diff --git a/sdk/go/volumes_test.go b/sdk/go/volumes_test.go index 3fba304..4133254 100644 --- a/sdk/go/volumes_test.go +++ b/sdk/go/volumes_test.go @@ -1,6 +1,7 @@ package sandbox import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -207,29 +208,31 @@ func TestWithVolumesEncodesMode(t *testing.T) { } } -func TestNewRejectsInvalidVolumeModeLocally(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - t.Error("server should not be contacted for a locally invalid mode") - })) - t.Cleanup(ts.Close) - - _, err := testClient(t, ts).New(t.Context(), "rt:24.04", WithVolumes(Volume{Name: "a", Mode: "readwrite"})) - if err == nil || !strings.Contains(err.Error(), "mode must be") { - t.Errorf("err = %v, want local mode rejection", err) +func TestRejectsInvalidVolumeModeLocally(t *testing.T) { + tests := []struct { + name string + claim func(c *Client, ctx context.Context) (*Sandbox, error) + }{ + {"Client.New", func(c *Client, ctx context.Context) (*Sandbox, error) { + return c.New(ctx, "rt:24.04", WithVolumes(Volume{Name: "a", Mode: "readwrite"})) + }}, + {"Template.New", func(c *Client, ctx context.Context) (*Sandbox, error) { + tpl := &Template{Name: "task:v1", c: c, addr: c.addr, net: "none", size: "small"} + return tpl.New(ctx, WithVolumes(Volume{Name: "a", Mode: "bogus"})) + }}, } -} - -func TestTemplateNewRejectsInvalidVolumeModeLocally(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - t.Error("server should not be contacted for a locally invalid mode") - })) - t.Cleanup(ts.Close) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("server should not be contacted for a locally invalid mode") + })) + t.Cleanup(ts.Close) - c := testClient(t, ts) - tpl := &Template{Name: "task:v1", c: c, addr: c.addr, net: "none", size: "small"} - _, err := tpl.New(t.Context(), WithVolumes(Volume{Name: "a", Mode: "bogus"})) - if err == nil || !strings.Contains(err.Error(), "mode must be") { - t.Errorf("err = %v, want local mode rejection", err) + _, err := tt.claim(testClient(t, ts), t.Context()) + if err == nil || !strings.Contains(err.Error(), "mode must be") { + t.Errorf("err = %v, want local mode rejection", err) + } + }) } } From b061f35037e21929da34a0ddcb502fd21d28e098 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 01:42:38 +0800 Subject: [PATCH 09/10] pool: let admission answer a live writer before the marker check A healthy writer's own write-ahead marker made a concurrent read-only claim report needs-recovery instead of busy. The post-reservation check is the single dirty authority now, and it runs before the warm pop so a refused claim never consumes a VM. --- sandboxd/pool/claim.go | 7 +++---- sandboxd/pool/volume.go | 11 ++++------- sandboxd/pool/volume_rw_test.go | 32 ++++++++++++++++++++++++++++---- 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index c241e98..62d1a39 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -42,6 +42,9 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur return nil, admitErr } reserved = applied + if cleanErr := m.confirmVolumesClean(volumeSpecs); cleanErr != nil { + return nil, cleanErr + } m.mu.Lock() var sb *types.Sandbox if p := m.pools[key]; p != nil { @@ -56,10 +59,6 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur return nil, ErrNoWarm } m.kickRefill() - if cleanErr := m.confirmVolumesClean(volumeSpecs); cleanErr != nil { - m.destroy(ctx, sb.VMName) - return nil, cleanErr - } if volumeErr := m.applyVolumes(ctx, sb, volumeSpecs, applied); volumeErr != nil { m.destroy(ctx, sb.VMName) return nil, volumeErr diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index d4f0b46..6d313e2 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -157,9 +157,6 @@ func (m *Manager) resolveVolumes(key types.PoolKey, tenant string, requested []t if _, statErr := os.Stat(entry.disk.Path); statErr != nil { return nil, fmt.Errorf("volume %q path %q: %w", volume.Name, entry.disk.Path, statErr) } - if !volume.RW() && volumeDirty(entry.disk.Path) { - return nil, fmt.Errorf("%w: volume %q", ErrVolumeNeedsRecovery, volume.Name) - } disk := entry.disk disk.RW = volume.RW() resolved = append(resolved, resolvedVolume{disk: disk, applied: volume}) @@ -167,10 +164,10 @@ func (m *Manager) resolveVolumes(key types.PoolKey, tenant string, requested []t return resolved, nil } -// confirmVolumesClean re-stats the read-only entries' markers once the holds -// are taken, closing the gap the resolve-time check leaves: a writable claim -// that failed between the two marks its image and releases, and mounting that -// image read-only would fail deep in guest setup instead of here. +// confirmVolumesClean refuses a read-only claim of an image a writer left +// dirty. It is the only marker check, and runs once the holds are taken: no +// writer can be admitted alongside, so the marker is stable and it can only +// mean a crashed writer — a live one is answered by admission as busy. func (m *Manager) confirmVolumesClean(volumes []resolvedVolume) error { for _, volume := range volumes { if !volume.applied.RW() && volumeDirty(volume.disk.Path) { diff --git a/sandboxd/pool/volume_rw_test.go b/sandboxd/pool/volume_rw_test.go index 210a254..ca65ef3 100644 --- a/sandboxd/pool/volume_rw_test.go +++ b/sandboxd/pool/volume_rw_test.go @@ -114,6 +114,31 @@ func TestDirtyVolumeBlocksReadersUntilWriterRecovers(t *testing.T) { } } +func TestDirtyVolumeRefusesWarmClaimBeforeTakingAVM(t *testing.T) { + path := writeVolumeImage(t, "scratch.img", "data") + if err := markVolumeDirty(path); err != nil { + t.Fatalf("mark dirty: %v", err) + } + eng := newFakeEngine() + m := newVolumePoolManager(t, eng, t.TempDir(), []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) + warm := &types.Sandbox{VMName: "sbx-warm", Key: testKey, VsockSocket: "/vsock/warm"} + m.pools[testKey].warm = append(m.pools[testKey].warm, warm) + + _, err := m.ClaimWarm(t.Context(), testKey, 0, "", "", []types.Volume{{Name: "scratch"}}) + if !errors.Is(err, ErrVolumeNeedsRecovery) { + t.Fatalf("read-only warm claim of a dirty image: %v, want ErrVolumeNeedsRecovery", err) + } + m.mu.Lock() + warmLeft := len(m.pools[testKey].warm) + m.mu.Unlock() + if warmLeft != 1 || eng.removed(warm.VMName) { + t.Errorf("warm pool has %d VMs and removes=%v, want the warm VM untouched", warmLeft, eng.removedNames()) + } + if holders := volumeHoldersOf(m, "scratch"); holders != (volumeHolders{}) { + t.Errorf("registry after the refusal=%+v, want empty", holders) + } +} + func TestConfirmVolumesCleanCatchesMarkerAfterAdmission(t *testing.T) { path := writeVolumeImage(t, "scratch.img", "data") m := newVolumeManager(t, newFakeEngine(), []config.VolumeSpec{{Name: "scratch", Path: path, Writable: true}}) @@ -156,10 +181,9 @@ func TestVolumeAdmissionMatrix(t *testing.T) { wantErr error }{ {"writer excludes writer", types.VolumeModeRW, types.VolumeModeRW, ErrVolumeBusy}, - // A live writer keeps the image marked, so a reader is turned away by - // the marker before admission ever sees it. Admission's own rule for - // this direction is pinned in TestReserveVolumesExcludesReaderUnderWriter. - {"writer excludes reader", types.VolumeModeRW, "", ErrVolumeNeedsRecovery}, + // Admission answers first: a live writer is a busy conflict, never the + // recovery verdict its own write-ahead marker would otherwise suggest. + {"writer excludes reader", types.VolumeModeRW, "", ErrVolumeBusy}, {"reader excludes writer", "", types.VolumeModeRW, ErrVolumeBusy}, {"readers share", "", "", nil}, } { From 78427ece48491cb396f839bedf3ecd1dfb96c225 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 13 Aug 2026 02:16:13 +0800 Subject: [PATCH 10/10] review: fold volume admission into one owner; drop stale read-only wording --- docs/deploy.md | 2 +- sandboxd/pool/claim.go | 41 +++++++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index d6889ad..15099a0 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -38,7 +38,7 @@ The scalar egress-attachment keys are retired: rename `"bridge": "br0"` to starting the new binary — config loading rejects the old spellings loudly rather than silently dropping the egress lane. -Read-only dataset volumes require a lockstep rollout. Upgrade every sandboxd +Dataset volumes require a lockstep rollout. Upgrade every sandboxd node and cocoon to the required version before enabling the catalog or shipping an SDK that requests volumes. Mixed-version serving is unsupported. Once a volume claim has finalized, do not roll a node back to an older sandboxd until diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 62d1a39..bc3d861 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -34,17 +34,13 @@ func (m *Manager) ClaimWarm(ctx context.Context, key types.PoolKey, ttl time.Dur if err != nil { return nil, err } - applied := appliedVolumes(volumeSpecs) + applied, err := m.admitVolumes(tenant, volumeSpecs) + if err != nil { + return nil, err + } // Holds belong to this path until finalize; past it the sandbox carries them. - var reserved []types.Volume + reserved := applied defer func() { m.unreserveVolumes(reserved) }() - if admitErr := m.admitClaim(tenant, applied); admitErr != nil { - return nil, admitErr - } - reserved = applied - if cleanErr := m.confirmVolumesClean(volumeSpecs); cleanErr != nil { - return nil, cleanErr - } m.mu.Lock() var sb *types.Sandbox if p := m.pools[key]; p != nil { @@ -210,6 +206,19 @@ func (m *Manager) overQuota(extra int, tenant string) error { return m.quotaErr(extra, tenant) } +// admitVolumes takes one claim's holds, then the marker check; a refusal on either 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 { + return nil, err + } + if err := m.confirmVolumesClean(volumes); err != nil { + m.unreserveVolumes(applied) + return nil, err + } + return applied, nil +} + // admitClaim is the provision path's one admission section: the advisory quota // precheck plus the authoritative volume reservation, so a busy volume is // refused before any VM is built. @@ -500,16 +509,12 @@ func (m *Manager) claimProvision(ctx context.Context, key types.PoolKey, ttl tim if err != nil { return nil, err } - applied := appliedVolumes(volumeSpecs) - var reserved []types.Volume - defer func() { m.unreserveVolumes(reserved) }() - if admitErr := m.admitClaim(tenant, applied); admitErr != nil { - return nil, admitErr - } - reserved = applied - if cleanErr := m.confirmVolumesClean(volumeSpecs); cleanErr != nil { - return nil, cleanErr + applied, err := m.admitVolumes(tenant, volumeSpecs) + if err != nil { + return nil, err } + reserved := applied + defer func() { m.unreserveVolumes(reserved) }() golden, err := m.resolveGolden(ctx, key) if err != nil { return nil, fmt.Errorf("resolve template: %w", err)