Can't snapshot read-only disks (#3069)

david-crespo · web-flow · commit 5304c4b7aa8b · 2026-02-12T05:38:11.000Z
can't snapshot read-only disks
diff --git 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
@@ -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<Disk>) for use in MSW handlers
 type SnapshotDisk =
-  | Pick<Disk, 'state' | 'diskType'>
-  | { state: DiskState; disk_type: DiskType }
+  | Pick<Disk, 'state' | 'diskType' | 'readOnly'>
+  | { 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
@@ -84,7 +84,7 @@ export default function SnapshotCreate() {
       <ComboboxField
         label="Disk"
         name="disk"
-        description="Only disks that can be snapshotted are listed"
+        description="Only disks that support snapshots are listed"
         placeholder="Select a disk"
         items={diskItemsForCombobox}
         required
diff --git a/app/pages/project/instances/common.tsx b/app/pages/project/instances/common.tsx
@@ -23,13 +23,14 @@ export const fancifyStates = (states: string[]) =>
   intersperse(states.map(white), <>, </>, <> or </>)
 
 /** Returns a disabled reason if the disk cannot be snapshotted, false otherwise */
-export function snapshotDisabledReason(disk: Pick<Disk, 'state' | 'diskType'>): 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
@@ -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
@@ -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"
   )
 })
 
