Fix size validation on disk create from 1023 GiB snapshot (#2641)

* fix size validation on disk create from 1023 GiB snapshot

* test it, rework helpers to avoid mistakes

* nearest10 -> diskSizeNearest10

Author: david-crespo

app/components/form/fields/ImageSelectField.tsx

@@ -12,7 +12,7 @@ import type { Image } from '@oxide/api'
 import type { InstanceCreateInput } from '~/forms/instance-create'
 import type { ComboboxItem } from '~/ui/lib/Combobox'
 import { Slash } from '~/ui/lib/Slash'
-import { nearest10 } from '~/util/math'
+import { diskSizeNearest10 } from '~/util/math'
 import { bytesToGiB, GiB } from '~/util/units'
 
 import { ComboboxField } from './ComboboxField'
@@ -50,7 +50,7 @@ export function BootDiskImageSelectField({
         if (!image) return
         const imageSizeGiB = image.size / GiB
         if (diskSizeField.value < imageSizeGiB) {
-          diskSizeField.onChange(nearest10(imageSizeGiB))
+          diskSizeField.onChange(diskSizeNearest10(imageSizeGiB))
         }
       }}
     />

app/forms/disk-create.tsx

@@ -37,6 +37,7 @@ import { Radio } from '~/ui/lib/Radio'
 import { RadioGroup } from '~/ui/lib/RadioGroup'
 import { Slash } from '~/ui/lib/Slash'
 import { toLocaleDateString } from '~/util/date'
+import { diskSizeNearest10 } from '~/util/math'
 import { bytesToGiB, GiB } from '~/util/units'
 
 const blankDiskSource: DiskSource = {
@@ -227,8 +228,7 @@ const DiskSourceField = ({
               const image = images.find((i) => i.id === id)! // if it's selected, it must be present
               const imageSizeGiB = image.size / GiB
               if (diskSizeField.value < imageSizeGiB) {
-                const nearest10 = Math.ceil(imageSizeGiB / 10) * 10
-                diskSizeField.onChange(nearest10)
+                diskSizeField.onChange(diskSizeNearest10(imageSizeGiB))
               }
             }}
           />
@@ -288,8 +288,7 @@ const SnapshotSelectField = ({ control }: { control: Control<DiskCreate> }) => {
         const snapshot = snapshots.find((i) => i.id === id)! // if it's selected, it must be present
         const snapshotSizeGiB = snapshot.size / GiB
         if (diskSizeField.value < snapshotSizeGiB) {
-          const nearest10 = Math.ceil(snapshotSizeGiB / 10) * 10
-          diskSizeField.onChange(nearest10)
+          diskSizeField.onChange(diskSizeNearest10(snapshotSizeGiB))
         }
       }}
     />

app/forms/instance-create.tsx

@@ -78,7 +78,7 @@ import { TipIcon } from '~/ui/lib/TipIcon'
 import { ALL_ISH } from '~/util/consts'
 import { readBlobAsBase64 } from '~/util/file'
 import { docLinks, links } from '~/util/links'
-import { nearest10 } from '~/util/math'
+import { diskSizeNearest10 } from '~/util/math'
 import { pb } from '~/util/path-builder'
 import { GiB } from '~/util/units'
 
@@ -225,7 +225,7 @@ export function CreateInstanceForm() {
     ...baseDefaultValues,
     bootDiskSourceType: defaultSource,
     sshPublicKeys: allKeys,
-    bootDiskSize: nearest10(defaultImage?.size / GiB),
+    bootDiskSize: diskSizeNearest10(defaultImage?.size / GiB),
     externalIps: [{ type: 'ephemeral', pool: defaultPool }],
   }
 
@@ -474,7 +474,7 @@ export function CreateInstanceForm() {
           onValueChange={(val) => {
             setValue('bootDiskSourceType', val as BootDiskSourceType)
             if (imageSizeGiB && imageSizeGiB > bootDiskSize) {
-              setValue('bootDiskSize', nearest10(imageSizeGiB))
+              setValue('bootDiskSize', diskSizeNearest10(imageSizeGiB))
             }
           }}
         >

app/util/math.spec.ts

@@ -7,7 +7,7 @@
  */
 import { describe, expect, it } from 'vitest'
 
-import { displayBigNum, nearest10, percentage, round, splitDecimal } from './math'
+import { diskSizeNearest10, displayBigNum, percentage, round, splitDecimal } from './math'
 import { GiB } from './units'
 
 function roundTest() {
@@ -207,6 +207,9 @@ it.each([
   [109, 110],
   [110, 110],
   [111, 120],
-])('nearest10 %d → %d', (input, output) => {
-  expect(nearest10(input)).toEqual(output)
+  // clamped at max disk size
+  [1021, 1023],
+  [1023, 1023],
+])('diskSizeNearest10 %d → %d', (input, output) => {
+  expect(diskSizeNearest10(input)).toEqual(output)
 })

app/util/math.ts

@@ -8,6 +8,8 @@
 
 import * as R from 'remeda'
 
+import { MAX_DISK_SIZE_GiB } from '~/api'
+
 /**
  * Get the two parts of a number (before decimal and after-and-including
  * decimal) as strings. Round to 2 decimal points if necessary.
@@ -94,8 +96,11 @@ export function displayBigNum(
 }
 
 /**
- * Gets the closest multiple of 10 larger than the passed-in number
+ * Calculate disk size based on image or snapshot size. We round up to the
+ * nearest 10, but also cap it at the max disk size so that, for example, a 1023
+ * GiB image doesn't produce a 1030 GiB disk, which is not valid.
  */
-export function nearest10(num: number): number {
-  return Math.ceil(num / 10) * 10
+export function diskSizeNearest10(imageSizeGiB: number) {
+  const nearest10 = Math.ceil(imageSizeGiB / 10) * 10
+  return Math.min(nearest10, MAX_DISK_SIZE_GiB)
 }

mock-api/snapshot.ts

@@ -10,6 +10,8 @@ import { v4 as uuid } from 'uuid'
 
 import type { Snapshot } from '@oxide/api'
 
+import { GiB } from '~/util/units'
+
 import { disks } from './disk'
 import type { Json } from './json-type'
 import { project } from './project'
@@ -81,7 +83,18 @@ export const snapshots: Json<Snapshot>[] = [
     project_id: project.id,
     time_created: new Date().toISOString(),
     time_modified: new Date().toISOString(),
-    size: 1024 * 1024 * 1024 * 20,
+    size: 20 * GiB,
+    disk_id: disks[3].id,
+    state: 'ready',
+  },
+  {
+    id: '3b252b04-d896-49d3-bae3-0ac102eed9b4',
+    name: 'snapshot-max-size',
+    description: '',
+    project_id: project.id,
+    time_created: new Date().toISOString(),
+    time_modified: new Date().toISOString(),
+    size: 1023 * GiB, // the biggest size allowed
     disk_id: disks[3].id,
     state: 'ready',
   },
@@ -91,7 +104,7 @@ export const snapshots: Json<Snapshot>[] = [
 function generateSnapshot(index: number): Json<Snapshot> {
   return {
     id: uuid(),
-    name: `disk-1-snapshot-${index + 7}`,
+    name: `disk-1-snapshot-${index + 8}`,
     description: '',
     project_id: project.id,
     time_created: new Date().toISOString(),

test/e2e/disks.e2e.ts

@@ -11,7 +11,6 @@ import {
   expectNoToast,
   expectRowVisible,
   expectToast,
-  expectVisible,
   test,
 } from './utils'
 
@@ -60,14 +59,16 @@ test('Disk snapshot error', async ({ page }) => {
 test.describe('Disk create', () => {
   test.beforeEach(async ({ page }) => {
     await page.goto('/projects/mock-project/disks-new')
+    await expect(page.getByRole('dialog', { name: 'Create disk' })).toBeVisible()
     await page.getByRole('textbox', { name: 'Name' }).fill('a-new-disk')
   })
 
   test.afterEach(async ({ page }) => {
     await page.getByRole('button', { name: 'Create disk' }).click()
 
+    await expect(page.getByRole('dialog', { name: 'Create disk' })).toBeHidden()
     await expectToast(page, 'Disk a-new-disk created')
-    await expectVisible(page, ['role=cell[name="a-new-disk"]'])
+    await expect(page.getByRole('cell', { name: 'a-new-disk' })).toBeVisible()
   })
 
   // expects are in the afterEach
@@ -83,6 +84,13 @@ test.describe('Disk create', () => {
     await page.getByRole('option', { name: 'delete-500' }).click()
   })
 
+  // max-size snapshot required a fix
+  test('from max-size snapshot', async ({ page }) => {
+    await page.getByRole('radio', { name: 'Snapshot' }).click()
+    await page.getByRole('button', { name: 'Source snapshot' }).click()
+    await page.getByRole('option', { name: 'snapshot-max' }).click()
+  })
+
   test('from image', async ({ page }) => {
     await page.getByRole('radio', { name: 'Image' }).click()
     await page.getByRole('button', { name: 'Source image' }).click()

test/e2e/pagination.e2e.ts

@@ -52,7 +52,7 @@ test('pagination', async ({ page }) => {
   await nextButton.click()
   await expectCell(page, 'disk-1-snapshot-76')
   await expectCell(page, 'disk-1-snapshot-86')
-  await expect(rows).toHaveCount(11)
+  await expect(rows).toHaveCount(12)
   await expect(nextButton).toBeDisabled() // no more pages
 
   await scrollTo(page, 250)

test/e2e/snapshots.e2e.ts

@@ -5,7 +5,7 @@
  *
  * Copyright Oxide Computer Company
  */
-import { expect, expectNotVisible, expectRowVisible, expectVisible, test } from './utils'
+import { expect, expectRowVisible, expectVisible, test } from './utils'
 
 test('Click through snapshots', async ({ page }) => {
   await page.goto('/projects/mock-project')
@@ -28,7 +28,7 @@ test('Click through snapshots', async ({ page }) => {
 test('Confirm delete snapshot', async ({ page }) => {
   await page.goto('/projects/mock-project/snapshots')
 
-  const row = page.getByRole('row', { name: 'disk-1-snapshot-7' })
+  const row = page.getByRole('row', { name: 'disk-1-snapshot-10' })
 
   // scroll a little so the dropdown menu isn't behind the pagination bar
   await page.getByRole('table').click() // focus the content pane
@@ -53,7 +53,8 @@ test('Confirm delete snapshot', async ({ page }) => {
   await page.getByRole('button', { name: 'Confirm' }).click()
 
   // modal closes, row is gone
-  await expectNotVisible(page, [modal, row])
+  await expect(modal).toBeHidden()
+  await expect(row).toBeHidden()
 })
 
 test('Error on delete snapshot', async ({ page }) => {