diff --git a/docs/deploy.md b/docs/deploy.md index 0a8b39776..01114f986 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -734,7 +734,7 @@ idle-time and expiry-triggered archives. Before setting `archive_failover` to `f The public API omits replica journal fields. A fixed delay or an absence of warnings does not prove retirement. Disabling failover first stops replica -retirement and leaves residual references that reject new live RW attachments. +retirement and leaves residual references. A local archive can still wake and clear `archive_ck`, but that does not clear a retained `replica_pending` or retire an old shared row. Release can remove local state without reconciling that row. Re-enabling failover against stale rows can diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index 783da5c97..88920c49e 100644 --- a/docs/sandboxd-api.md +++ b/docs/sandboxd-api.md @@ -372,15 +372,8 @@ To change a held name's mount, mode, or `attach_only`, first remove the name and wait for successful convergence. An identical target can be retried. Claims with `on_expire: archive` must change that policy before adding a volume. -New live writable attachments require a non-replicating pool, including a -promoted template's source pool. A replicating pool returns 409 before changing -intent, holds, or devices. Initial claim-time writable volumes remain supported. -An existing or pending replica, a pending archive, or this claim's unacknowledged checkpoint -retirement, returns 503 with `Retry-After`. A replica or archive intent left -with `archive_failover` disabled returns 409 because it cannot retire through -the registry. A retained local checkpoint reference alone still returns 503. -Volume mutation does not evict remote replicas. Read-only additions, exact -retries of held writers, and removals remain available under these restrictions. +A claim that holds volumes is not replicated. Its replica retires on the next +replication pass, and a replica capture that races a live addition is refused. The sandbox must be running. Paused, archived, and failed sandboxes return 409 without waking. A failed sandbox holding volumes is release-only after its VMM exits; PUT with an empty target cannot restore its processes. Missing, invalid, @@ -409,13 +402,15 @@ a safe per-volume reason. Guest transport failures remain server errors; timeouts return 504 and cancellation returns 408. A mounted writable attachment marks the image dirty before the guest can write. -Live removal clears the marker only when this call observes and normally -unmounts the exact writable whole-device filesystem, finds no residual device -mounts, and confirms host detach. Both mount and superblock options must be -writable. A missing device or pre-unmounted filesystem does not prove clean -unmount. The marker remains on uncertainty and blocks subsequent readers. -Attach-only writers have no automatic dirty-marker protection. The caller owns -mounting, flushing, unmounting, recovery, and fencing before reuse. +Live removal and release clear the marker only when the unmount observes and +normally unmounts the exact writable whole-device filesystem and finds no +residual device mounts. Live removal also needs a confirmed host detach. Both +mount and superblock options must be writable; ext4's `emergency_ro` after a +filesystem error counts as read-only. A missing device or pre-unmounted +filesystem does not prove clean unmount. The marker remains on uncertainty and +blocks subsequent readers. Attach-only writers have no automatic dirty-marker +protection. The caller owns mounting, flushing, unmounting, recovery, and +fencing before reuse. The per-entry field requires coordinated daemon and SDK releases. Older daemons ignore `attach_only` in claims and reject it in live PUT and PATCH with 400. diff --git a/docs/sdk-python.md b/docs/sdk-python.md index ac483ebd7..375dd0d64 100644 --- a/docs/sdk-python.md +++ b/docs/sdk-python.md @@ -185,7 +185,7 @@ view = sb.set_volumes(volumes) The caller mounts, flushes, and unmounts attach-only devices. See the [live volume contract](sandboxd-api.md#put-v1sandboxesidvolumes) for -writer exclusion, replica refusals, and dirty-marker limits. +writer exclusion and dirty-marker limits. Each method takes `timeout=` (default: the client timeout, 120 s); a large replacement can need longer, and a call that times out cancels the server run, so retry the same target. diff --git a/docs/sdk.md b/docs/sdk.md index 278ea9d63..382d2dccf 100644 --- a/docs/sdk.md +++ b/docs/sdk.md @@ -262,7 +262,7 @@ can independently use `Mode: "rw"` and `AttachOnly: true`. The SDK preserves these fields directly and leaves name, mode, and mount policy to sandboxd. Use matching daemon and SDK releases for attach-only GET-to-PUT retries. See the [live volume contract](sandboxd-api.md#put-v1sandboxesidvolumes) for -writer exclusion, replica refusals, and dirty-marker limits. +writer exclusion and dirty-marker limits. Each response contains `volumes_want`, `volumes`, `states`, and `converging`. Go exposes them as `SandboxVolumesResponse.VolumesWant`, `.Volumes`, `.States`, diff --git a/e2e/fakeengine_test.go b/e2e/fakeengine_test.go index 79eccfe12..03192a543 100644 --- a/e2e/fakeengine_test.go +++ b/e2e/fakeengine_test.go @@ -201,11 +201,11 @@ func (f *fakeEngine) MountLiveVolume(ctx context.Context, sock, name, mount stri return f.MountVolume(ctx, sock, name, mount, rw) } -func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { +func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) (bool, error) { f.mu.Lock() defer f.mu.Unlock() f.volumeOps = append(f.volumeOps, "umount:"+mount) - return nil + return mount != "", nil } func (f *fakeEngine) SyncGuest(_ context.Context, _ string) error { @@ -216,13 +216,14 @@ func (f *fakeEngine) SyncGuest(_ context.Context, _ string) error { } func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount string) (bool, error) { - if err := f.UnmountVolume(ctx, sock, mount); err != nil { + clean, err := f.UnmountVolume(ctx, sock, mount) + if err != nil { return false, err } f.mu.Lock() defer f.mu.Unlock() f.volumeOps = append(f.volumeOps, "detach:"+name) - return mount != "", nil + return clean, nil } func (f *fakeEngine) volumeOpsLog() []string { diff --git a/sandboxd/engine/volume.go b/sandboxd/engine/volume.go index 637cc7dc9..3032b2578 100644 --- a/sandboxd/engine/volume.go +++ b/sandboxd/engine/volume.go @@ -71,22 +71,21 @@ func (e *Engine) MountVolume(ctx context.Context, vsockSocket, name, mount strin return e.mountDevice(ctx, vsockSocket, device, name, mount, rw) } -// UnmountVolume flushes and detaches a guest mount before the VM is removed. -func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, mount string) error { +// SyncGuest flushes the guest page cache, the only flush left for a failed unmount. +func (e *Engine) SyncGuest(ctx context.Context, vsockSocket string) error { 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) + if err := e.silkdExec(ctx, vsockSocket, "sync"); err != nil { + return fmt.Errorf("sync guest: %w", err) } return nil } -// SyncGuest flushes the guest page cache, the only flush left for a failed unmount. -func (e *Engine) SyncGuest(ctx context.Context, vsockSocket string) error { +func (e *Engine) umountVolume(ctx context.Context, vsockSocket, mount 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) + if err := e.silkdExec(ctx, vsockSocket, "umount", "--", mount); err != nil { + return fmt.Errorf("unmount volume %s: %w", mount, err) } return nil } diff --git a/sandboxd/engine/volume_live.go b/sandboxd/engine/volume_live.go index 02b625186..fe474b6f8 100644 --- a/sandboxd/engine/volume_live.go +++ b/sandboxd/engine/volume_live.go @@ -72,7 +72,7 @@ func (e *Engine) DetachVolume(ctx context.Context, vmName, vsockSocket, name, mo } clean := false if found { - if clean, err = e.unmountLiveVolume(ctx, vsockSocket, device, mount); err != nil { + if clean, err = e.unmountDevice(ctx, vsockSocket, device, mount); err != nil { return false, err } } @@ -82,7 +82,30 @@ func (e *Engine) DetachVolume(ctx context.Context, vmName, vsockSocket, name, mo return clean, nil } -func (e *Engine) unmountLiveVolume(ctx context.Context, vsockSocket, device, mount string) (bool, error) { +// UnmountVolume unmounts a guest mount before VM removal; true means its writable whole-filesystem mount closed cleanly. +func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, mount string) (bool, error) { + raw, err := e.silkdReadFile(ctx, vsockSocket, "/proc/self/mountinfo", mountInfoMax) + if err != nil { + return false, err + } + mounts, err := parseVolumeMounts(string(raw)) + if err != nil { + return false, err + } + target := e.resolveMount(ctx, vsockSocket, mount, mounts) + i := slices.IndexFunc(mounts, func(m volumeMount) bool { return m.path == target }) + if i < 0 { + return false, nil + } + entry := mounts[i] + if err = e.umountVolume(ctx, vsockSocket, mount); err != nil { + return false, err + } + shared := slices.ContainsFunc(mounts, func(m volumeMount) bool { return m.devNum == entry.devNum && m != entry }) + return !shared && cleanMount(entry), nil +} + +func (e *Engine) unmountDevice(ctx context.Context, vsockSocket, device, mount string) (bool, error) { if mount == "" { return false, e.refuseOtherMounts(ctx, vsockSocket, device) } @@ -94,13 +117,13 @@ func (e *Engine) unmountLiveVolume(ctx context.Context, vsockSocket, device, mou atTarget := slices.DeleteFunc(mounts, func(m volumeMount) bool { return m.path != target }) clean := false if len(atTarget) == 1 && atTarget[0].devNum == devNum && atTarget[0].root == "/" { - if err = e.UnmountVolume(ctx, vsockSocket, mount); err != nil { + if err = e.umountVolume(ctx, vsockSocket, mount); err != nil { if _, ok := errors.AsType[*guestExitError](err); ok { return false, &VolumeMountConflict{Message: fmt.Sprintf("cannot unmount %q; close guest references and retry", mount)} } return false, err } - clean = writableMountOptions(atTarget[0].options) && writableMountOptions(atTarget[0].superOptions) + clean = cleanMount(atTarget[0]) } if err = e.refuseOtherMounts(ctx, vsockSocket, device); err != nil { return false, err @@ -188,9 +211,13 @@ func parseVolumeMounts(raw string) ([]volumeMount, error) { return mounts, nil } +func cleanMount(m volumeMount) bool { + return m.root == "/" && writableMountOptions(m.options) && writableMountOptions(m.superOptions) +} + func writableMountOptions(options string) bool { parts := strings.Split(options, ",") - return slices.Contains(parts, "rw") && !slices.Contains(parts, "ro") && !slices.Contains(parts, "") + return slices.Contains(parts, "rw") && !slices.ContainsFunc(parts, func(p string) bool { return p == "" || p == "ro" || p == "emergency_ro" }) } func bindConflict(path string) *VolumeMountConflict { diff --git a/sandboxd/engine/volume_live_test.go b/sandboxd/engine/volume_live_test.go index 4569f0c7b..49286230e 100644 --- a/sandboxd/engine/volume_live_test.go +++ b/sandboxd/engine/volume_live_test.go @@ -260,6 +260,7 @@ func TestLiveDetachCleanEvidenceRequiresWritableUnmountAndHostSuccess(t *testing {name: "readonly", options: "ro", superOptions: "ro", mount: "/data"}, {name: "mount readonly", options: "ro", superOptions: "rw", mount: "/data"}, {name: "superblock readonly", options: "rw", superOptions: "ro", mount: "/data"}, + {name: "ext4 emergency read-only", options: "rw,relatime", superOptions: "rw,emergency_ro", mount: "/data"}, {name: "extra superblock fields", options: "rw", superOptions: "rw extra", mount: "/data", wantErr: true}, {name: "pre-unmounted", unmounted: true, mount: "/data"}, {name: "missing device", missing: true, unmounted: true, mount: "/data"}, @@ -307,6 +308,36 @@ func TestLiveDetachCleanEvidenceRequiresWritableUnmountAndHostSuccess(t *testing } } +func TestUnmountVolumeReportsCleanWritableUnmount(t *testing.T) { + const entry = "2 1 8:32 / /data rw,relatime shared:4 - ext4 /dev/vdc " + for _, tc := range []struct { + name, mountinfo string + busy, wantClean bool + wantErr bool + }{ + {name: "clean", mountinfo: entry + "rw\n", wantClean: true}, + {name: "ext4 emergency read-only", mountinfo: entry + "rw,emergency_ro\n"}, + {name: "bind mount elsewhere", mountinfo: entry + "rw\n3 1 8:32 / /other rw - ext4 /dev/vdc rw\n"}, + {name: "not mounted"}, + {name: "busy", mountinfo: entry + "rw\n", busy: true, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + sock := sockPath(t) + fake := serveFakeSilkd(t, sock) + fake.mu.Lock() + fake.serial["/proc/self/mountinfo"] = []byte(rootMountInfo + tc.mountinfo) + if tc.busy { + fake.execCode = 32 + } + fake.mu.Unlock() + clean, err := New("cocoon", nil, nil, false, false, "").UnmountVolume(t.Context(), sock, "/data") + if (err != nil) != tc.wantErr || clean != tc.wantClean { + t.Fatalf("clean=%t err=%v", clean, err) + } + }) + } +} + func liveDiskCLI(t *testing.T, detachErr string) VolumeSpec { t.Helper() dir := t.TempDir() diff --git a/sandboxd/engine/volume_test.go b/sandboxd/engine/volume_test.go index 62ad7ce79..17bde498c 100644 --- a/sandboxd/engine/volume_test.go +++ b/sandboxd/engine/volume_test.go @@ -136,15 +136,15 @@ func TestMountVolumeUsesSysfsAndRequestedMode(t *testing.T) { } } -func TestUnmountVolumeExecsBoundedUmount(t *testing.T) { +func TestUmountVolumeExecsBoundedUmount(t *testing.T) { if VolumeCallTimeout != 2*time.Second { t.Fatalf("volume call timeout = %s, want 2s", VolumeCallTimeout) } path := sockPath(t) fake := serveFakeSilkd(t, path) e := New("cocoon", nil, nil, false, false, "") - if err := e.UnmountVolume(t.Context(), path, "/datasets/training"); err != nil { - t.Fatalf("UnmountVolume: %v", err) + if err := e.umountVolume(t.Context(), path, "/datasets/training"); err != nil { + t.Fatalf("umountVolume: %v", err) } fake.mu.Lock() @@ -154,7 +154,7 @@ func TestUnmountVolumeExecsBoundedUmount(t *testing.T) { } fake.execCode, fake.execFailAt = 32, 2 fake.mu.Unlock() - err := e.UnmountVolume(t.Context(), path, "/datasets/training") + err := e.umountVolume(t.Context(), path, "/datasets/training") if err == nil || !strings.Contains(err.Error(), "unmount volume /datasets/training") { t.Errorf("got %v, want umount failure", err) } diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index aaeb92417..cfe2138e4 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -345,12 +345,6 @@ func (s *claimStore) checkpointReferences() map[string]struct{} { return refs } -func (s *claimStore) hasCheckpointReferences(owner string) bool { - s.mu.Lock() - defer s.mu.Unlock() - return s.refs[owner] != [4]string{} || len(s.retained[owner]) != 0 -} - func encodeRow(dto *claimDTO) ([]byte, error) { raw, err := json.Marshal(map[string]*claimDTO{dto.ID: dto}) if err != nil { diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 184a13744..679b7f71b 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -106,10 +106,6 @@ var ( ErrVolumeRegistrationConflict = errors.New("volume registration conflicts with the catalog") ErrVolumeCatalogUnavailable = errors.New("volume catalog requires a successful persistence retry") - ErrReplicaPresent = errors.New("live writable attachment requires replica and recovery checkpoint retirement") - ErrReplicatingPool = errors.New("live writable volumes require a non-replicating pool") - ErrReplicaStranded = errors.New("live writable attachment is blocked by retained replica or archive state with archive_failover disabled") - // Replaying a journal takes a writable mount, so readers stay out. ErrVolumeNeedsRecovery = errors.New("volume needs recovery by a writable claim") ErrArchived = errors.New("sandbox is archived; an exec or file call wakes it first") @@ -156,7 +152,7 @@ type Engine interface { DetachVolume(ctx context.Context, vmName, sock, name, mount string) (cleanRW bool, err error) MountVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error MountLiveVolume(ctx context.Context, vsockSocket, name, mount string, rw bool) error - UnmountVolume(ctx context.Context, vsockSocket, mount string) error + UnmountVolume(ctx context.Context, vsockSocket, mount string) (bool, error) SyncGuest(ctx context.Context, vsockSocket string) error DiskAttachSnapshot(ctx context.Context, vmName, snapName string) (string, error) DiskAttachDir(ctx context.Context, vmName, dir string) (string, error) diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 513c05dee..33b11eefb 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1673,8 +1673,10 @@ func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount stri if err := ctx.Err(); err != nil { return false, err } + clean := false if mount != "" { - if err := f.UnmountVolume(ctx, sock, mount); err != nil { + var err error + if clean, err = f.UnmountVolume(ctx, sock, mount); err != nil { return false, err } } @@ -1684,14 +1686,17 @@ func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount stri if f.detachVolumeErr != nil { return false, f.detachVolumeErr } - return !f.detachUnclean && slices.ContainsFunc(f.volumeMounts, func(v types.Volume) bool { return v.Name == name && v.Mount == mount && v.RW() }), nil + return clean, nil } -func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) error { +func (f *fakeEngine) UnmountVolume(_ context.Context, _, mount string) (bool, error) { f.mu.Lock() defer f.mu.Unlock() f.volumeOps = append(f.volumeOps, "umount:"+mount) - return f.unmountVolumeErr + if f.unmountVolumeErr != nil { + return false, f.unmountVolumeErr + } + return !f.detachUnclean && slices.ContainsFunc(f.volumeMounts, func(v types.Volume) bool { return v.Mount == mount && v.RW() }), nil } func (f *fakeEngine) SyncGuest(_ context.Context, vsockSocket string) error { diff --git a/sandboxd/pool/volume.go b/sandboxd/pool/volume.go index fc3ec2baf..b5df9ecc4 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -228,7 +228,7 @@ func (m *Manager) applyVolume(ctx context.Context, sb *types.Sandbox, volume res return nil } -// quiesceVolumes unmounts a claim's writable mounts in reverse; a failed unmount keeps the marker. +// quiesceVolumes unmounts a claim's writable mounts in reverse; an unclean unmount keeps the marker. func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeTeardown { td := volumeTeardown{holds: heldVolumes(sb)} mounts := writableMounts(sb.Volumes) @@ -245,12 +245,13 @@ func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeT if !needsUnmount(volume) { continue // attach-only: no mount to bring down, VM removal closes the device } - if err := m.eng.UnmountVolume(ctx, sb.VsockSocket, volume.Mount); err != nil { + clean, err := m.eng.UnmountVolume(ctx, sb.VsockSocket, volume.Mount) + if err != nil { logger.Errorf(ctx, err, "unmount volume %s of %s", volume.Name, sb.ID) stuck = true continue } - if entry, ok := catalog[volume.Name]; ok { + if entry, ok := catalog[volume.Name]; ok && clean { td.clears = append(td.clears, entry.disk.Path) } } diff --git a/sandboxd/pool/volume_attach_test.go b/sandboxd/pool/volume_attach_test.go index 427b7e867..bb8ebe0df 100644 --- a/sandboxd/pool/volume_attach_test.go +++ b/sandboxd/pool/volume_attach_test.go @@ -379,6 +379,7 @@ func TestQuiesceVolumesSkipsAttachOnlyEntries(t *testing.T) { {Name: "mounted", Mount: "/volumes/mounted", Mode: types.VolumeModeRW}, }, } + eng.volumeMounts = sb.Volumes[1:] td := m.quiesceVolumes(t.Context(), sb) diff --git a/sandboxd/pool/volume_catalog_test.go b/sandboxd/pool/volume_catalog_test.go index 0ee286259..9a9b79d00 100644 --- a/sandboxd/pool/volume_catalog_test.go +++ b/sandboxd/pool/volume_catalog_test.go @@ -2,7 +2,6 @@ package pool import ( "context" - "encoding/json/jsontext" "encoding/json/v2" "errors" "fmt" @@ -342,18 +341,6 @@ func TestRuntimeCatalogRejectsCollisionsAndInvalidRegistrations(t *testing.T) { } } -func TestRuntimeCatalogMarshalFailureDoesNotPublish(t *testing.T) { - m := newVolumeManager(t, newFakeEngine(), nil) - err := m.RegisterVolume(t.Context(), "runtime", types.VolumeRegistration{Path: "/srv/\xff"}) - _, wrapped := errors.AsType[*jsontext.SyntacticError](err) - if errors.Is(err, ErrVolumeCatalogUnavailable) || !wrapped { - t.Fatalf("marshal error was not reported as permanent: %v", err) - } - if _, err := os.Stat(filepath.Join(m.dataDir, runtimeVolumesFile)); !errors.Is(err, os.ErrNotExist) || m.catalog.entries["runtime"] != nil { - t.Fatalf("failed registration persisted: stat=%v entry=%+v", err, m.catalog.entries["runtime"]) - } -} - func TestRuntimeCatalogVolumeRootsBoundRegistration(t *testing.T) { newRootedManager := func(t *testing.T, eng *fakeEngine, dataDir string, roots ...string) (*Manager, error) { t.Helper() diff --git a/sandboxd/pool/volume_mutation.go b/sandboxd/pool/volume_mutation.go index 7be4a25bd..6de24c604 100644 --- a/sandboxd/pool/volume_mutation.go +++ b/sandboxd/pool/volume_mutation.go @@ -37,7 +37,7 @@ func (m *Manager) GetVolumes(id, tenant string) (types.SandboxVolumesResponse, e // SetVolumes converges a running claim onto the full desired volume set. func (m *Manager) SetVolumes(ctx context.Context, id, tenant string, want []types.Volume) (types.SandboxVolumesResponse, error) { - return m.changeVolumes(ctx, id, tenant, func([]types.Volume) ([]types.Volume, error) { return slices.Clone(want), nil }) + return m.changeVolumes(ctx, id, tenant, func([]types.Volume) ([]types.Volume, error) { return want, nil }) } // PatchVolumes adds and removes a running claim's volumes by name. @@ -108,13 +108,7 @@ func (m *Manager) changeVolumes(ctx context.Context, id, tenant string, change v if err != nil { return view, err } - m.mu.Lock() - err = m.checkVolumeRunLocked(ctx, sb, run) - if err == nil && slices.ContainsFunc(additions, types.Volume.RW) { - err = m.liveWriterErrorLocked(sb) - } - m.mu.Unlock() - if err != nil { + if err = m.checkVolumeRun(ctx, sb, run); err != nil { return view, err } sb.Touch() @@ -265,11 +259,21 @@ func (m *Manager) stageVolumeTarget(ctx context.Context, sb *types.Sandbox, run sb.Transition.Lock() defer sb.Transition.Unlock() m.mu.Lock() - err := m.holdLiveAdditionsLocked(ctx, sb, run, target, additions, resolved) - m.mu.Unlock() - if err != nil { + if err := m.checkVolumeRunLocked(ctx, sb, run); err != nil { + m.mu.Unlock() + return claimSnapshot{}, false, err + } + if sb.OnExpire == types.ExpireArchive && len(target) > 0 { + m.mu.Unlock() + return claimSnapshot{}, false, ErrVolumeCapture + } + if err := m.checkResolvedLocked(resolved); err != nil { + m.mu.Unlock() return claimSnapshot{}, false, err } + m.adoptVolumes(additions) + run.holds = append(run.holds, additions...) + m.mu.Unlock() cleanErr := m.confirmVolumesClean(resolved) m.mu.Lock() defer m.mu.Unlock() @@ -292,39 +296,6 @@ func (m *Manager) stageVolumeTarget(ctx context.Context, sb *types.Sandbox, run return m.store.set(sb), true, nil } -func (m *Manager) holdLiveAdditionsLocked(ctx context.Context, sb *types.Sandbox, run *volumeRun, target, additions []types.Volume, resolved []resolvedVolume) error { - if err := m.checkVolumeRunLocked(ctx, sb, run); err != nil { - return err - } - if sb.OnExpire == types.ExpireArchive && len(target) > 0 { - return ErrVolumeCapture - } - if slices.ContainsFunc(additions, types.Volume.RW) { - if err := m.liveWriterErrorLocked(sb); err != nil { - return err - } - } - if err := m.checkResolvedLocked(resolved); err != nil { - return err - } - m.adoptVolumes(additions) - run.holds = append(run.holds, additions...) - return nil -} - -func (m *Manager) liveWriterErrorLocked(sb *types.Sandbox) error { - if m.view.Load().poolReplicate[sb.PolicyKey()] > 0 { - return ErrReplicatingPool - } - if m.archives == nil && (sb.HasReplica() || sb.ArchivePending != "") { - return ErrReplicaStranded - } - if sb.HasReplica() || sb.ArchivePending != "" || m.store.hasCheckpointReferences(sb.ID) { - return ErrReplicaPresent - } - return nil -} - func (m *Manager) detachVolumeRun(ctx context.Context, sb *types.Sandbox, run *volumeRun, v types.Volume) error { m.mu.Lock() err := m.checkVolumeRunLocked(ctx, sb, run) @@ -337,19 +308,10 @@ func (m *Manager) detachVolumeRun(ctx context.Context, sb *types.Sandbox, run *v if err != nil { return fmt.Errorf("detach volume %q: %w", v.Name, err) } - clearMarker := needsUnmount(v) && cleanRW && entry != nil - hold := [1]types.Volume{v} - m.mu.Lock() - err = m.checkVolumeRunLocked(ctx, sb, run) - if err == nil && clearMarker { - m.adoptVolumes(hold[:]) - } - m.mu.Unlock() - if err != nil { + if err = m.checkVolumeRun(ctx, sb, run); err != nil { return err } - if clearMarker { - defer m.unreserveVolumes(hold[:]) + if needsUnmount(v) && cleanRW && entry != nil { if err = clearVolumeDirty(entry.disk.Path); err != nil { return fmt.Errorf("clear dirty marker for volume %q: %w", v.Name, err) } diff --git a/sandboxd/pool/volume_replica_test.go b/sandboxd/pool/volume_replica_test.go index 7124f2357..29746fe5c 100644 --- a/sandboxd/pool/volume_replica_test.go +++ b/sandboxd/pool/volume_replica_test.go @@ -2,7 +2,6 @@ package pool import ( "errors" - "slices" "sync" "testing" "time" @@ -102,93 +101,6 @@ func TestReplicaCaptureRefusesPendingLiveWriter(t *testing.T) { } } -func TestLiveWriterAdmissionAgainstReplicaState(t *testing.T) { - for _, tc := range []struct { - name string - prepare func(*testing.T, *Manager, *types.Sandbox) - want error - }{ - {"replicating pool", func(*testing.T, *Manager, *types.Sandbox) {}, ErrReplicatingPool}, - {"source pool replicates", func(_ *testing.T, _ *Manager, sb *types.Sandbox) { - sb.PolicySource = testKey - sb.Key.Template = "promoted:template" - }, ErrReplicatingPool}, - {"source pool does not replicate", func(_ *testing.T, _ *Manager, sb *types.Sandbox) { - sb.PolicySource = testKey - sb.PolicySource.Template = "other:template" - }, nil}, - {"published replica", func(t *testing.T, m *Manager, sb *types.Sandbox) { disableReplication(m); replicateNow(t, m, sb) }, ErrReplicaPresent}, - {"pending replica", func(_ *testing.T, m *Manager, sb *types.Sandbox) { - disableReplication(m) - sb.ReplicaPending = "pending" - m.store.set(sb) - }, ErrReplicaPresent}, - {"registry disabled", func(t *testing.T, m *Manager, sb *types.Sandbox) { - disableReplication(m) - replicateNow(t, m, sb) - m.archives = nil - }, ErrReplicaStranded}, - {"archive pending without registry", func(_ *testing.T, m *Manager, sb *types.Sandbox) { - disableReplication(m) - sb.ArchivePending = "pending" - m.archives = nil - }, ErrReplicaStranded}, - {"unrelated checkpoint", func(t *testing.T, m *Manager, _ *types.Sandbox) { - disableReplication(m) - other := mustClaim(t, m, testKey) - other.ReplicaCk = "unrelated" - m.store.set(other) - }, nil}, - } { - t.Run(tc.name, func(t *testing.T) { - m, _ := newReplicaVolumeManager(t) - sb := mustClaim(t, m, testKey) - tc.prepare(t, m, sb) - before, ops := m.store.mark(), m.eng.(*fakeEngine).volumeOpsLog() - view, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW, AttachOnly: true}}) - if !errors.Is(err, tc.want) { - t.Fatalf("writer refusal = %v, want %v", err, tc.want) - } - if tc.want != nil && (len(view.VolumesWant) != 0 || len(view.Volumes) != 0 || view.Converging || m.store.mark() != before || volumeHoldersOf(m, "data") != (volumeHolders{}) || !slices.Equal(ops, m.eng.(*fakeEngine).volumeOpsLog())) { - t.Fatalf("refusal changed claim or device: %+v", view) - } - }) - } -} - -func TestInitialWriterAndRetiredReplicaAllowLiveWork(t *testing.T) { - m, _ := newReplicaVolumeManager(t) - sb, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}}) - if err != nil { - t.Fatal(err) - } - if _, err = m.SetVolumes(t.Context(), sb.ID, "", sb.Volumes); err != nil { - t.Fatalf("held writer retry: %v", err) - } - if _, err = m.SetVolumes(t.Context(), sb.ID, "", nil); err != nil { - t.Fatalf("writer cleanup: %v", err) - } - replicateNow(t, m, sb) - disableReplication(m) - m.store.recommitting.Store(true) - restore := failClaimWrites(t, m.store) - m.retireReplica(t.Context(), sb) - writer := []types.Volume{{Name: "data", Mode: types.VolumeModeRW}} - if sb.HasReplica() || !m.store.hasCheckpointReferences(sb.ID) { - t.Fatal("failed clear lost checkpoint uncertainty") - } - if _, err = m.SetVolumes(t.Context(), sb.ID, "", writer); !errors.Is(err, ErrReplicaPresent) { - t.Fatalf("unacknowledged retirement: %v", err) - } - restore() - if err = m.store.commit(m.store.mark()); err != nil { - t.Fatal(err) - } - if _, err = m.SetVolumes(t.Context(), sb.ID, "", writer); err != nil { - t.Fatalf("acknowledged retirement: %v", err) - } -} - func newReplicaVolumeManager(t *testing.T) (*Manager, *failoverCluster) { t.Helper() c := newReplicaCluster(t) diff --git a/sandboxd/pool/volume_rw_test.go b/sandboxd/pool/volume_rw_test.go index f950f40d3..eaa5ee0ef 100644 --- a/sandboxd/pool/volume_rw_test.go +++ b/sandboxd/pool/volume_rw_test.go @@ -79,55 +79,6 @@ func TestLiveWritableFailedHostDetachKeepsMarkerAndHold(t *testing.T) { } } -func TestLiveWritableMarkerClearFailureRetainsOnlyClaimHold(t *testing.T) { - eng := newFakeEngine() - catalog := liveCatalog(t) - m := newVolumeManager(t, eng, catalog) - sb := mustClaim(t, m, testKey) - if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}); err != nil { - t.Fatal(err) - } - marker := volumeDirtyPath(catalog[0].Path) - if err := os.Remove(marker); err != nil { - t.Fatal(err) - } - if err := os.Mkdir(marker, 0o700); err != nil { - t.Fatal(err) - } - child := filepath.Join(marker, "block-unlink") - if err := os.WriteFile(child, nil, 0o600); err != nil { - t.Fatal(err) - } - if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err == nil || !strings.Contains(err.Error(), "clear dirty marker") { - t.Fatalf("marker unlink failure: %v", err) - } - if !volumeDirty(catalog[0].Path) || volumeHoldersOf(m, "data") != (volumeHolders{writers: 1}) { - t.Fatal("failed marker clear lost the marker or leaked an operation hold") - } - eng.detachUnclean = true - if _, err := m.SetVolumes(t.Context(), sb.ID, "", []types.Volume{}); err != nil { - t.Fatal(err) - } - if !volumeDirty(catalog[0].Path) || volumeHoldersOf(m, "data") != (volumeHolders{}) { - t.Fatal("absent-device retry cleared the marker or retained an admission hold") - } - if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data"}}}); !errors.Is(err, ErrVolumeNeedsRecovery) { - t.Fatalf("reader admission after failed marker clear: %v", err) - } - if err := os.Remove(child); err != nil { - t.Fatal(err) - } - if err := os.Remove(marker); err != nil { - t.Fatal(err) - } - if err := markVolumeDirty(catalog[0].Path); err != nil { - t.Fatal(err) - } - if _, err := m.ClaimProvision(t.Context(), testKey, ClaimOptions{Volumes: []types.Volume{{Name: "data", Mode: types.VolumeModeRW}}}); err != nil { - t.Fatalf("recovery writer admission: %v", err) - } -} - func TestLiveWritableUncertainAdditionClearsOnlyAfterDetach(t *testing.T) { eng := newFakeEngine() catalog := liveCatalog(t) @@ -196,6 +147,23 @@ func TestWritableClaimMarksDirtyBeforeAttachAndClearsOnRelease(t *testing.T) { } } +func TestReleaseKeepsMarkerAfterUncleanUnmount(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, ClaimOptions{Volumes: []types.Volume{{Name: "scratch", Mode: types.VolumeModeRW}}}) + if err != nil { + t.Fatal(err) + } + eng.detachUnclean = true + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + if !volumeDirty(path) { + t.Error("release cleared the marker after an unclean unmount") + } +} + func TestClaimRejectsWriteOnReadOnlyEntry(t *testing.T) { readOnly := writeVolumeImage(t, "data.img", "data") writable := writeVolumeImage(t, "scratch.img", "scratch") @@ -434,6 +402,7 @@ func TestRollbackQuiescesWritableVolumesBeforeRemoval(t *testing.T) { ID: "sb_rw", VMName: "sbx-rw", Key: testKey, VsockSocket: "/vsock/rw", Volumes: []types.Volume{{Name: "scratch", Mount: "/volumes/scratch", Mode: types.VolumeModeRW}}, } + eng.volumeMounts = sb.Volumes m.mu.Lock() m.claimed[sb.ID] = sb m.adoptVolumes(sb.Volumes) diff --git a/sandboxd/server/server.go b/sandboxd/server/server.go index a2ba7799f..c26137baa 100644 --- a/sandboxd/server/server.go +++ b/sandboxd/server/server.go @@ -65,9 +65,6 @@ var poolErrHTTP = []struct { {pool.ErrVolumeCapture, http.StatusConflict, ""}, {pool.ErrVolumeBusy, http.StatusConflict, ""}, {pool.ErrVolumeRegistrationConflict, http.StatusConflict, pool.ErrVolumeRegistrationConflict.Error()}, - {pool.ErrReplicaPresent, http.StatusServiceUnavailable, ""}, - {pool.ErrReplicatingPool, http.StatusConflict, ""}, - {pool.ErrReplicaStranded, http.StatusConflict, ""}, {pool.ErrVolumeNeedsRecovery, http.StatusConflict, ""}, {pool.ErrArchived, http.StatusConflict, ""}, {pool.ErrPaused, http.StatusConflict, ""}, diff --git a/sandboxd/server/volume_mutation.go b/sandboxd/server/volume_mutation.go index 86dd63983..1e4f86f9a 100644 --- a/sandboxd/server/volume_mutation.go +++ b/sandboxd/server/volume_mutation.go @@ -60,7 +60,7 @@ func (s *Server) handleSandboxVolumes(w http.ResponseWriter, r *http.Request) { code, view.Error = mapped, msg } } - if code >= http.StatusInternalServerError && !errors.Is(err, pool.ErrReplicaPresent) { + if code >= http.StatusInternalServerError { log.WithFunc("server.handleSandboxVolumes").Errorf(r.Context(), err, "converge volumes for %s", id) } } diff --git a/sandboxd/server/volume_mutation_test.go b/sandboxd/server/volume_mutation_test.go index 4cd91e174..528914909 100644 --- a/sandboxd/server/volume_mutation_test.go +++ b/sandboxd/server/volume_mutation_test.go @@ -153,7 +153,7 @@ func TestLiveVolumeRoutesHideOtherTenants(t *testing.T) { func TestLiveVolumeRoutesReportRefusalsOverRecordedDiagnostics(t *testing.T) { const cause = "cannot unmount /data; close guest references and retry" - for _, refusal := range []error{pool.ErrVolumeBusy, pool.ErrVolumeCapture, pool.ErrReplicaPresent, pool.ErrReplicatingPool, pool.ErrReplicaStranded} { + for _, refusal := range []error{pool.ErrVolumeBusy, pool.ErrVolumeCapture} { t.Run(refusal.Error(), func(t *testing.T) { mgr := &fakeManager{setVolumes: func(string, string, []types.Volume) (types.SandboxVolumesResponse, error) { return types.SandboxVolumesResponse{VolumesWant: []types.Volume{{Name: "old"}}, States: []types.VolumeState{{Name: "old", State: "pending", Error: cause}}, Error: "volume change failed; retry the desired target"}, fmt.Errorf("%w: volume %q", refusal, "data") @@ -165,16 +165,10 @@ func TestLiveVolumeRoutesReportRefusalsOverRecordedDiagnostics(t *testing.T) { if err := json.UnmarshalRead(resp.Body, &got); err != nil { t.Fatal(err) } - wantStatus := http.StatusConflict - if errors.Is(refusal, pool.ErrReplicaPresent) { - wantStatus = http.StatusServiceUnavailable - if resp.Header.Get("Retry-After") == "" { - t.Fatal("temporary replica retirement refusal has no Retry-After") - } - } else if resp.Header.Get("Retry-After") != "" { + if resp.Header.Get("Retry-After") != "" { t.Fatal("permanent conflict advertises a retry delay") } - if resp.StatusCode != wantStatus || !strings.HasPrefix(got.Error, refusal.Error()) || len(got.States) != 1 || got.States[0].Error != cause { + if resp.StatusCode != http.StatusConflict || !strings.HasPrefix(got.Error, refusal.Error()) || len(got.States) != 1 || got.States[0].Error != cause { t.Fatalf("status=%d view=%+v", resp.StatusCode, got) } })