From 892eac70b9d9f9b8173d0997f5a39370cb13b172 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 29 Oct 2026 15:45:22 +0800 Subject: [PATCH 1/5] pool: keep the live volume path to one rule per invariant Drop the live-writer replica policy (three errors, a 503 retry contract and a checkpoint-reference probe): a claim holding volumes is already never replicated, publication rechecks holds, and the scheduler retires the replica on its next pass. Release and live detach now share the engine's clean-unmount rule, so a writable superblock remounted read-only keeps the dirty marker on release too. Drop the temporary writer hold around the live marker unlink; it guarded a detach stalled between its final check and an unlink across a whole release and new writer admission. --- docs/deploy.md | 2 +- docs/sandboxd-api.md | 17 ++--- docs/sdk-python.md | 2 +- docs/sdk.md | 2 +- e2e/fakeengine_test.go | 9 +-- sandboxd/engine/volume.go | 15 ++--- sandboxd/engine/volume_live.go | 21 +++--- sandboxd/engine/volume_test.go | 8 +-- sandboxd/pool/claims.go | 6 -- sandboxd/pool/pool.go | 6 +- sandboxd/pool/pool_test.go | 13 ++-- sandboxd/pool/volume.go | 7 +- sandboxd/pool/volume_attach_test.go | 1 + sandboxd/pool/volume_mutation.go | 39 +---------- sandboxd/pool/volume_replica_test.go | 88 ------------------------- sandboxd/pool/volume_rw_test.go | 18 +++++ sandboxd/server/server.go | 3 - sandboxd/server/volume_mutation.go | 2 +- sandboxd/server/volume_mutation_test.go | 12 +--- 19 files changed, 76 insertions(+), 195 deletions(-) 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..04c1e81b4 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,9 +402,9 @@ 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 +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. 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 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..4b9f0634b 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, name, 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..766ce0154 100644 --- a/sandboxd/engine/volume_live.go +++ b/sandboxd/engine/volume_live.go @@ -62,26 +62,29 @@ func (e *Engine) MountLiveVolume(ctx context.Context, vsockSocket, name, mount s return e.mountDevice(ctx, vsockSocket, device, name, mount, rw) } -// DetachVolume unmounts and detaches a guest volume; true means a writable mount closed cleanly. +// DetachVolume runs UnmountVolume, then detaches the disk from the VM. func (e *Engine) DetachVolume(ctx context.Context, vmName, vsockSocket, name, mount string) (bool, error) { ctx, cancel := context.WithTimeout(ctx, VolumeMutationTimeout) defer cancel() - device, found, err := e.findVolumeDevice(ctx, vsockSocket, name) + clean, err := e.UnmountVolume(ctx, vsockSocket, name, mount) if err != nil { return false, err } - clean := false - if found { - if clean, err = e.unmountLiveVolume(ctx, vsockSocket, device, mount); err != nil { - return false, err - } - } if _, err = e.run(ctx, "vm", "disk", "detach", vmName, argName, name); err != nil { return false, err } return clean, nil } +// UnmountVolume unmounts a guest volume; true means its writable whole-device mount closed cleanly. +func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, name, mount string) (bool, error) { + device, found, err := e.findVolumeDevice(ctx, vsockSocket, name) + if err != nil || !found { + return false, err + } + return e.unmountLiveVolume(ctx, vsockSocket, device, mount) +} + func (e *Engine) unmountLiveVolume(ctx context.Context, vsockSocket, device, mount string) (bool, error) { if mount == "" { return false, e.refuseOtherMounts(ctx, vsockSocket, device) @@ -94,7 +97,7 @@ 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)} } 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..693347888 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, name, 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..c9c28da2d 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, name, 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, _, name, 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.Name == name && 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..4b7593d87 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.Name, 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_mutation.go b/sandboxd/pool/volume_mutation.go index 7be4a25bd..99c68b72c 100644 --- a/sandboxd/pool/volume_mutation.go +++ b/sandboxd/pool/volume_mutation.go @@ -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() @@ -299,11 +293,6 @@ func (m *Manager) holdLiveAdditionsLocked(ctx context.Context, sb *types.Sandbox 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 } @@ -312,19 +301,6 @@ func (m *Manager) holdLiveAdditionsLocked(ctx context.Context, sb *types.Sandbox 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 +313,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..a67e83652 100644 --- a/sandboxd/pool/volume_rw_test.go +++ b/sandboxd/pool/volume_rw_test.go @@ -196,6 +196,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 +451,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) } }) From e4991bc32b9cc3e60865bc71779a14c6b93f11c2 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 29 Oct 2026 15:50:42 +0800 Subject: [PATCH 2/5] pool: fold single-use live helpers and drop a redundant target copy Inline the live-addition hold helper back into stageVolumeTarget now that it carries no replica check, return SetVolumes' target as is since ValidateVolumes copies it, and name the shared unmount helper for both of its callers. --- sandboxd/engine/volume_live.go | 4 ++-- sandboxd/pool/volume_mutation.go | 33 ++++++++++++++------------------ 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/sandboxd/engine/volume_live.go b/sandboxd/engine/volume_live.go index 766ce0154..acda07fb9 100644 --- a/sandboxd/engine/volume_live.go +++ b/sandboxd/engine/volume_live.go @@ -82,10 +82,10 @@ func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, name, mount str if err != nil || !found { return false, err } - return e.unmountLiveVolume(ctx, vsockSocket, device, mount) + return e.unmountDevice(ctx, vsockSocket, device, mount) } -func (e *Engine) unmountLiveVolume(ctx context.Context, vsockSocket, device, mount string) (bool, error) { +func (e *Engine) unmountDevice(ctx context.Context, vsockSocket, device, mount string) (bool, error) { if mount == "" { return false, e.refuseOtherMounts(ctx, vsockSocket, device) } diff --git a/sandboxd/pool/volume_mutation.go b/sandboxd/pool/volume_mutation.go index 99c68b72c..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. @@ -259,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() @@ -286,21 +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 err := m.checkResolvedLocked(resolved); err != nil { - return err - } - m.adoptVolumes(additions) - run.holds = append(run.holds, additions...) - 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) From 696107ff2a8fec9f4a130c64bd5d9a5bb9ca23b4 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 29 Oct 2026 16:23:04 +0800 Subject: [PATCH 3/5] engine: treat ext4 emergency_ro as an unclean unmount On current guest kernels an ext4 error with errors=remount-ro leaves the superblock options at rw,emergency_ro instead of ro, so release and live detach cleared the dirty marker of a filesystem that had failed. Found on cocoon-test2 with trigger_fs_error. --- docs/sandboxd-api.md | 12 +++++++----- sandboxd/engine/volume_live.go | 2 +- sandboxd/engine/volume_live_test.go | 1 + 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/sandboxd-api.md b/docs/sandboxd-api.md index 04c1e81b4..88920c49e 100644 --- a/docs/sandboxd-api.md +++ b/docs/sandboxd-api.md @@ -404,11 +404,13 @@ timeouts return 504 and cancellation returns 408. A mounted writable attachment marks the image dirty before the guest can write. 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. 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. +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/sandboxd/engine/volume_live.go b/sandboxd/engine/volume_live.go index acda07fb9..c4a397ded 100644 --- a/sandboxd/engine/volume_live.go +++ b/sandboxd/engine/volume_live.go @@ -193,7 +193,7 @@ func parseVolumeMounts(raw string) ([]volumeMount, error) { 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..237cbc023 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"}, From 388de3e3dac034c86f41fa1609531ffa354eaf6c Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 29 Oct 2026 16:25:09 +0800 Subject: [PATCH 4/5] test(pool): drop tests of an unreachable input and of the removed marker hold The marshal test fed invalid UTF-8 that the HTTP decoder already rejects, and the marker-unlink test came with the temporary writer hold this branch removes; a failed unlink only keeps the marker. --- sandboxd/pool/volume_catalog_test.go | 13 -------- sandboxd/pool/volume_rw_test.go | 49 ---------------------------- 2 files changed, 62 deletions(-) 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_rw_test.go b/sandboxd/pool/volume_rw_test.go index a67e83652..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) From 4a07458b0885e8137df83faab794e9dcdb903b05 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 29 Oct 2026 16:29:58 +0800 Subject: [PATCH 5/5] engine: judge a release unmount from one mountinfo read Release reused live detach's device lookup and post-unmount rescan, six guest calls per writable mount, and cost cocoon-test2 +19 ms p50 per writable release. Release only needs the clean verdict, so it reads mountinfo once, takes the device from the mount entry, and checks options, superblock and other mounts of that device with the predicate live detach shares. --- e2e/fakeengine_test.go | 4 +-- sandboxd/engine/volume_live.go | 40 +++++++++++++++++++++++------ sandboxd/engine/volume_live_test.go | 30 ++++++++++++++++++++++ sandboxd/pool/pool.go | 2 +- sandboxd/pool/pool_test.go | 6 ++--- sandboxd/pool/volume.go | 2 +- 6 files changed, 69 insertions(+), 15 deletions(-) diff --git a/e2e/fakeengine_test.go b/e2e/fakeengine_test.go index 4b9f0634b..03192a543 100644 --- a/e2e/fakeengine_test.go +++ b/e2e/fakeengine_test.go @@ -201,7 +201,7 @@ 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) (bool, 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) @@ -216,7 +216,7 @@ func (f *fakeEngine) SyncGuest(_ context.Context, _ string) error { } func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount string) (bool, error) { - clean, err := f.UnmountVolume(ctx, sock, name, mount) + clean, err := f.UnmountVolume(ctx, sock, mount) if err != nil { return false, err } diff --git a/sandboxd/engine/volume_live.go b/sandboxd/engine/volume_live.go index c4a397ded..fe474b6f8 100644 --- a/sandboxd/engine/volume_live.go +++ b/sandboxd/engine/volume_live.go @@ -62,27 +62,47 @@ func (e *Engine) MountLiveVolume(ctx context.Context, vsockSocket, name, mount s return e.mountDevice(ctx, vsockSocket, device, name, mount, rw) } -// DetachVolume runs UnmountVolume, then detaches the disk from the VM. +// DetachVolume unmounts and detaches a guest volume; true means a writable mount closed cleanly. func (e *Engine) DetachVolume(ctx context.Context, vmName, vsockSocket, name, mount string) (bool, error) { ctx, cancel := context.WithTimeout(ctx, VolumeMutationTimeout) defer cancel() - clean, err := e.UnmountVolume(ctx, vsockSocket, name, mount) + device, found, err := e.findVolumeDevice(ctx, vsockSocket, name) if err != nil { return false, err } + clean := false + if found { + if clean, err = e.unmountDevice(ctx, vsockSocket, device, mount); err != nil { + return false, err + } + } if _, err = e.run(ctx, "vm", "disk", "detach", vmName, argName, name); err != nil { return false, err } return clean, nil } -// UnmountVolume unmounts a guest volume; true means its writable whole-device mount closed cleanly. -func (e *Engine) UnmountVolume(ctx context.Context, vsockSocket, name, mount string) (bool, error) { - device, found, err := e.findVolumeDevice(ctx, vsockSocket, name) - if err != nil || !found { +// 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 } - return e.unmountDevice(ctx, vsockSocket, device, mount) + 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) { @@ -103,7 +123,7 @@ func (e *Engine) unmountDevice(ctx context.Context, vsockSocket, device, mount s } 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 @@ -191,6 +211,10 @@ 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.ContainsFunc(parts, func(p string) bool { return p == "" || p == "ro" || p == "emergency_ro" }) diff --git a/sandboxd/engine/volume_live_test.go b/sandboxd/engine/volume_live_test.go index 237cbc023..49286230e 100644 --- a/sandboxd/engine/volume_live_test.go +++ b/sandboxd/engine/volume_live_test.go @@ -308,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/pool/pool.go b/sandboxd/pool/pool.go index 693347888..679b7f71b 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -152,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, name, mount string) (bool, 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 c9c28da2d..33b11eefb 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1676,7 +1676,7 @@ func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount stri clean := false if mount != "" { var err error - if clean, err = f.UnmountVolume(ctx, sock, name, mount); err != nil { + if clean, err = f.UnmountVolume(ctx, sock, mount); err != nil { return false, err } } @@ -1689,14 +1689,14 @@ func (f *fakeEngine) DetachVolume(ctx context.Context, _, sock, name, mount stri return clean, nil } -func (f *fakeEngine) UnmountVolume(_ context.Context, _, name, mount string) (bool, 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) if f.unmountVolumeErr != nil { return false, f.unmountVolumeErr } - return !f.detachUnclean && slices.ContainsFunc(f.volumeMounts, func(v types.Volume) bool { return v.Name == name && v.Mount == mount && v.RW() }), nil + 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 4b7593d87..b5df9ecc4 100644 --- a/sandboxd/pool/volume.go +++ b/sandboxd/pool/volume.go @@ -245,7 +245,7 @@ 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 } - clean, err := m.eng.UnmountVolume(ctx, sb.VsockSocket, volume.Name, volume.Mount) + 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