From cff5dd6c94b984617c7342fc447f8cee7b43c4cb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 11 Feb 2026 23:16:45 -0600 Subject: [PATCH] can't snapshot read-only disks --- app/api/util.spec.ts | 42 ++++++++++++++++++++------ app/api/util.ts | 10 +++--- app/forms/snapshot-create.tsx | 2 +- app/pages/project/instances/common.tsx | 7 +++-- mock-api/msw/handlers.ts | 3 +- test/e2e/disks.e2e.ts | 15 ++++++++- 6 files changed, 60 insertions(+), 19 deletions(-) diff --git a/app/api/util.spec.ts b/app/api/util.spec.ts index b21fe731b9..266f990237 100644 --- a/app/api/util.spec.ts +++ b/app/api/util.spec.ts @@ -151,22 +151,46 @@ test('diskCan', () => { expect(diskCan.delete({ state: { state: 'attached', instance: 'xyz' } })).toBe(false) expect(diskCan.delete({ state: { state: 'detached' } })).toBe(true) - // snapshot requires distributed disk type - expect(diskCan.snapshot({ state: { state: 'detached' }, diskType: 'distributed' })).toBe( - true - ) + // snapshot requires distributed, non-read-only disk type + expect( + diskCan.snapshot({ + state: { state: 'detached' }, + diskType: 'distributed', + readOnly: false, + }) + ).toBe(true) expect( diskCan.snapshot({ state: { state: 'attached', instance: 'x' }, diskType: 'distributed', + readOnly: false, }) ).toBe(true) - expect(diskCan.snapshot({ state: { state: 'creating' }, diskType: 'distributed' })).toBe( - false - ) - expect(diskCan.snapshot({ state: { state: 'detached' }, diskType: 'local' })).toBe(false) expect( - diskCan.snapshot({ state: { state: 'attached', instance: 'x' }, diskType: 'local' }) + diskCan.snapshot({ + state: { state: 'creating' }, + diskType: 'distributed', + readOnly: false, + }) + ).toBe(false) + expect( + diskCan.snapshot({ state: { state: 'detached' }, diskType: 'local', readOnly: false }) + ).toBe(false) + expect( + diskCan.snapshot({ + state: { state: 'attached', instance: 'x' }, + diskType: 'local', + readOnly: false, + }) + ).toBe(false) + + // read-only disks cannot be snapshotted + expect( + diskCan.snapshot({ + state: { state: 'detached' }, + diskType: 'distributed', + readOnly: true, + }) ).toBe(false) // @ts-expect-error typechecker rejects actions that don't exist diff --git a/app/api/util.ts b/app/api/util.ts index ebd6396304..6bc420c1c0 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -209,16 +209,18 @@ const diskStateActions = { // snapshot has a type check in addition to state check // https://github.com/oxidecomputer/omicron/blob/078f636/nexus/src/app/snapshot.rs#L100-L109 -// NOTE: .states only captures the state requirement; local disks cannot be -// snapshotted regardless of state +// NOTE: .states only captures the state requirement; local and read-only disks +// cannot be snapshotted regardless of state const snapshotStates: DiskState['state'][] = ['attached', 'detached'] // accept both camelCase (Disk) and snake_case (Json) for use in MSW handlers type SnapshotDisk = - | Pick - | { state: DiskState; disk_type: DiskType } + | Pick + | { state: DiskState; disk_type: DiskType; read_only: boolean } const canSnapshot = (d: SnapshotDisk) => { const diskType = 'diskType' in d ? d.diskType : d.disk_type + const readOnly = 'readOnly' in d ? d.readOnly : d.read_only return ( + !readOnly && snapshotStates.includes(d.state.state) && match(diskType) .with('distributed', () => true) diff --git a/app/forms/snapshot-create.tsx b/app/forms/snapshot-create.tsx index 525f7d86d5..8079c59f30 100644 --- a/app/forms/snapshot-create.tsx +++ b/app/forms/snapshot-create.tsx @@ -84,7 +84,7 @@ export default function SnapshotCreate() { intersperse(states.map(white), <>, , <> or ) /** Returns a disabled reason if the disk cannot be snapshotted, false otherwise */ -export function snapshotDisabledReason(disk: Pick): ReactNode { +export function snapshotDisabledReason(disk: Disk): ReactNode { if (diskCan.snapshot(disk)) return false + if (disk.readOnly) return "Read-only disks don't support snapshots" return match(disk.diskType) .with('distributed', () => ( - <>Only disks in state {fancifyStates(diskCan.snapshot.states)} can be snapshotted + <>Only disks in state {fancifyStates(diskCan.snapshot.states)} support snapshots )) - .with('local', () => 'Only distributed disks support snapshots') + .with('local', () => "Local disks don't support snapshots") .exhaustive() } diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 07d71db7bf..6708e0c220 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -1349,9 +1349,10 @@ export const handlers = makeHandlers({ const disk = lookup.disk({ ...query, disk: body.disk }) if (!diskCan.snapshot(disk)) { + if (disk.read_only) throw "Read-only disks don't support snapshots" throw match(disk.disk_type) .with('distributed', () => 'Cannot snapshot disk in state ' + disk.state.state) - .with('local', () => 'Only distributed disks support snapshots') + .with('local', () => "Local disks don't support snapshots") .exhaustive() } diff --git a/test/e2e/disks.e2e.ts b/test/e2e/disks.e2e.ts index cb5ac57277..913c4ee669 100644 --- a/test/e2e/disks.e2e.ts +++ b/test/e2e/disks.e2e.ts @@ -100,9 +100,22 @@ test('Local disk snapshot disabled', async ({ page }) => { await expect(snapshotItem).toBeDisabled() // hover to see tooltip with disabled reason + await snapshotItem.hover() + await expect(page.getByRole('tooltip')).toHaveText("Local disks don't support snapshots") +}) + +test('Read-only disk snapshot disabled', async ({ page }) => { + await page.goto('/projects/mock-project/disks') + + const row = page.getByRole('row', { name: 'read-only-disk', exact: false }) + await row.getByRole('button', { name: 'Row actions' }).click() + + const snapshotItem = page.getByRole('menuitem', { name: 'Snapshot' }) + await expect(snapshotItem).toBeDisabled() + await snapshotItem.hover() await expect(page.getByRole('tooltip')).toHaveText( - 'Only distributed disks support snapshots' + "Read-only disks don't support snapshots" ) })