Fix auto-updating of boot disk size when selected image is larger than current disk size; add tests (#1829)

* Automatically increase the boot disk size when building it off a particular image

* Add validation to both Instance Create and Disk Create flows

Author: charliepark

app/components/form/fields/DiskSizeField.tsx

@@ -5,7 +5,12 @@
  *
  * Copyright Oxide Computer Company
  */
-import type { FieldPath, FieldPathByValue, FieldValues } from 'react-hook-form'
+import type {
+  FieldPath,
+  FieldPathByValue,
+  FieldValues,
+  ValidateResult,
+} from 'react-hook-form'
 
 import { MAX_DISK_SIZE_GiB } from '@oxide/api'
 
@@ -17,12 +22,19 @@ interface DiskSizeProps<
   TName extends FieldPath<TFieldValues>,
 > extends TextFieldProps<TFieldValues, TName> {
   minSize?: number
+  validate?(diskSizeGiB: number): ValidateResult
 }
 
 export function DiskSizeField<
   TFieldValues extends FieldValues,
   TName extends FieldPathByValue<TFieldValues, number>,
->({ required = true, name, minSize = 1, ...props }: DiskSizeProps<TFieldValues, TName>) {
+>({
+  required = true,
+  name,
+  minSize = 1,
+  validate,
+  ...props
+}: DiskSizeProps<TFieldValues, TName>) {
   return (
     <NumberField
       units="GiB"
@@ -32,12 +44,18 @@ export function DiskSizeField<
       min={minSize}
       max={MAX_DISK_SIZE_GiB}
       validate={(diskSizeGiB) => {
+        // Run a number of default validators
+        if (Number.isNaN(diskSizeGiB)) {
+          return 'Disk size is required'
+        }
         if (diskSizeGiB < minSize) {
           return `Must be at least ${minSize} GiB`
         }
         if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
           return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
         }
+        // Run any additional validators passed in from the callsite
+        return validate?.(diskSizeGiB)
       }}
       {...props}
     />

app/components/form/fields/NumberField.tsx

@@ -77,7 +77,7 @@ export const NumberFieldInner = <
       name={name}
       control={control}
       rules={{ required, validate }}
-      render={({ field: { value, ...fieldRest }, fieldState: { error } }) => {
+      render={({ field, fieldState: { error } }) => {
         return (
           <>
             <UINumberField
@@ -87,8 +87,7 @@ export const NumberFieldInner = <
                 [`${id}-help-text`]: !!description,
               })}
               aria-describedby={description ? `${id}-label-tip` : undefined}
-              defaultValue={value}
-              {...fieldRest}
+              {...field}
             />
             <ErrorMessage error={error} label={label} />
           </>

app/forms/disk-create.tsx

@@ -6,6 +6,7 @@
  * Copyright Oxide Computer Company
  */
 import { format } from 'date-fns'
+import { useMemo } from 'react'
 import { useController, type Control } from 'react-hook-form'
 import { useNavigate, type NavigateFunction } from 'react-router-dom'
 
@@ -17,9 +18,10 @@ import {
   type Disk,
   type DiskCreate,
   type DiskSource,
+  type Image,
 } from '@oxide/api'
 import { FieldLabel, FormDivider, Radio, RadioGroup } from '@oxide/ui'
-import { GiB } from '@oxide/util'
+import { bytesToGiB, GiB } from '@oxide/util'
 
 import {
   DescriptionField,
@@ -79,6 +81,21 @@ export function CreateDiskSideModalForm({
   })
 
   const form = useForm({ defaultValues })
+  const { project } = useProjectSelector()
+  const projectImages = useApiQuery('imageList', { query: { project } })
+  const siloImages = useApiQuery('imageList', {})
+
+  // put project images first because if there are any, there probably aren't
+  // very many and they're probably relevant
+  const images = useMemo(
+    () => [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])],
+    [projectImages.data, siloImages.data]
+  )
+  const areImagesLoading = projectImages.isPending || siloImages.isPending
+
+  const selectedImageId = form.watch('diskSource.imageId')
+  const selectedImageSize = images.find((image) => image.id === selectedImageId)?.size
+  const imageSizeGiB = selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
 
   return (
     <SideModalForm
@@ -96,13 +113,33 @@ export function CreateDiskSideModalForm({
       <NameField name="name" control={form.control} />
       <DescriptionField name="description" control={form.control} />
       <FormDivider />
-      <DiskSourceField control={form.control} />
-      <DiskSizeField name="size" control={form.control} />
+      <DiskSourceField
+        control={form.control}
+        images={images}
+        areImagesLoading={areImagesLoading}
+      />
+      <DiskSizeField
+        name="size"
+        control={form.control}
+        validate={(diskSizeGiB: number) => {
+          if (imageSizeGiB && diskSizeGiB < imageSizeGiB) {
+            return `Must be as large as selected image (min. ${imageSizeGiB} GiB)`
+          }
+        }}
+      />
     </SideModalForm>
   )
 }
 
-const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
+const DiskSourceField = ({
+  control,
+  images,
+  areImagesLoading,
+}: {
+  control: Control<DiskCreate>
+  images: Image[]
+  areImagesLoading: boolean
+}) => {
   const {
     field: { value, onChange },
   } = useController({ control, name: 'diskSource' })
@@ -144,37 +181,24 @@ const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
             ]}
           />
         )}
-        {value.type === 'image' && <ImageSelectField control={control} />}
+        {value.type === 'image' && (
+          <ListboxField
+            control={control}
+            name="diskSource.imageId"
+            label="Source image"
+            placeholder="Select an image"
+            isLoading={areImagesLoading}
+            items={images.map((i) => toListboxItem(i, true))}
+            required
+          />
+        )}
 
         {value.type === 'snapshot' && <SnapshotSelectField control={control} />}
       </div>
     </>
   )
 }
 
-const ImageSelectField = ({ control }: { control: Control<DiskCreate> }) => {
-  const { project } = useProjectSelector()
-
-  const projectImages = useApiQuery('imageList', { query: { project } })
-  const siloImages = useApiQuery('imageList', {})
-
-  // put project images first because if there are any, there probably aren't
-  // very many and they're probably relevant
-  const images = [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])]
-
-  return (
-    <ListboxField
-      control={control}
-      name="diskSource.imageId"
-      label="Source image"
-      placeholder="Select an image"
-      isLoading={projectImages.isPending || siloImages.isPending}
-      items={images.map((i) => toListboxItem(i, true))}
-      required
-    />
-  )
-}
-
 const DiskNameFromId = ({ disk }: { disk: string }) => {
   const { data, isPending, isError } = useApiQuery(
     'diskView',

app/forms/instance-create.tsx

@@ -14,7 +14,6 @@ import {
   genName,
   INSTANCE_MAX_CPU,
   INSTANCE_MAX_RAM_GiB,
-  MAX_DISK_SIZE_GiB,
   useApiMutation,
   useApiQueryClient,
   usePrefetchedApiQuery,
@@ -137,9 +136,11 @@ export function CreateInstanceForm() {
 
   const imageInput = useWatch({ control: control, name: 'image' })
   const image = allImages.find((i) => i.id === imageInput)
+  const imageSize = image?.size ? Math.ceil(image.size / GiB) : undefined
 
   return (
     <FullPageForm
+      submitDisabled={allImages.length ? undefined : 'Image required'}
       id="create-instance-form"
       form={form}
       title="Create instance"
@@ -151,9 +152,10 @@ export function CreateInstanceForm() {
           values.presetId === 'custom'
             ? { memory: values.memory, ncpus: values.ncpus }
             : { memory: preset.memory, ncpus: preset.ncpus }
-
-        // we should also never have an image ID that's not in the list
         const image = allImages.find((i) => values.image === i.id)
+        // There should always be an image present, because …
+        // - The form is disabled unless there are images available.
+        // - The form defaults to including at least one image.
         invariant(image, 'Expected image to be defined')
 
         const bootDiskName = values.bootDiskName || genName(values.name, image.name)
@@ -287,37 +289,51 @@ export function CreateInstanceForm() {
       <FormDivider />
 
       <Form.Heading id="boot-disk">Boot disk</Form.Heading>
-      <Tabs.Root id="boot-disk-tabs" className="full-width" defaultValue="project">
+      <Tabs.Root
+        id="boot-disk-tabs"
+        className="full-width"
+        // default to the project images tab if there are only project images
+        defaultValue={
+          projectImages.length > 0 && siloImages.length === 0 ? 'project' : 'silo'
+        }
+      >
         <Tabs.List aria-describedby="boot-disk">
-          <Tabs.Trigger value="project">Project images</Tabs.Trigger>
           <Tabs.Trigger value="silo">Silo images</Tabs.Trigger>
+          <Tabs.Trigger value="project">Project images</Tabs.Trigger>
         </Tabs.List>
-        <Tabs.Content value="project" className="space-y-4">
-          {projectImages.length === 0 ? (
+        {allImages.length === 0 && (
+          <Message
+            className="mb-8 ml-10 max-w-lg"
+            variant="notice"
+            content="Images are required to create a boot disk."
+          />
+        )}
+        <Tabs.Content value="silo" className="space-y-4">
+          {siloImages.length === 0 ? (
             <div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
               <EmptyMessage
                 icon={<Images16Icon />}
-                title="No project images found"
-                body="An image needs to be uploaded to be seen here"
-                buttonText="Upload image"
-                onClick={() => navigate(pb.projectImageNew(projectSelector))}
+                title="No silo images found"
+                body="Project images need to be promoted to be seen here"
               />
             </div>
           ) : (
-            <ImageSelectField images={projectImages} control={control} />
+            <ImageSelectField images={siloImages} control={control} />
           )}
         </Tabs.Content>
-        <Tabs.Content value="silo" className="space-y-4">
-          {siloImages.length === 0 ? (
+        <Tabs.Content value="project" className="space-y-4">
+          {projectImages.length === 0 ? (
             <div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
               <EmptyMessage
                 icon={<Images16Icon />}
-                title="No silo images found"
-                body="Project images need to be promoted to be seen here"
+                title="No project images found"
+                body="An image needs to be uploaded to be seen here"
+                buttonText="Upload image"
+                onClick={() => navigate(pb.projectImageNew(projectSelector))}
               />
             </div>
           ) : (
-            <ImageSelectField images={siloImages} control={control} />
+            <ImageSelectField images={projectImages} control={control} />
           )}
         </Tabs.Content>
       </Tabs.Root>
@@ -328,15 +344,9 @@ export function CreateInstanceForm() {
         label="Disk size"
         name="bootDiskSize"
         control={control}
-        // Imitate API logic: only require that the disk is big enough to fit the image
-        validate={(diskSizeGiB) => {
-          if (!image) return true
-          if (diskSizeGiB < image.size / GiB) {
-            const minSize = Math.ceil(image.size / GiB)
-            return `Must be as large as selected image (min. ${minSize} GiB)`
-          }
-          if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
-            return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
+        validate={(diskSizeGiB: number) => {
+          if (imageSize && diskSizeGiB < imageSize) {
+            return `Must be as large as selected image (min. ${imageSize} GiB)`
           }
         }}
       />

app/test/e2e/instance-create.e2e.ts

@@ -128,6 +128,43 @@ test('can create an instance with custom hardware', async ({ page }) => {
   ])
 })
 
+test('automatically updates disk size when larger image selected', async ({ page }) => {
+  await page.goto('/projects/mock-project/instances-new')
+
+  const instanceName = 'my-new-instance'
+  await page.fill('input[name=name]', instanceName)
+
+  // set the disk size larger than it needs to be, to verify it doesn't get reduced
+  const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' })
+  await diskSizeInput.fill('5')
+
+  // pick a disk image that's smaller than 5GiB (the first project image works [4GiB])
+  await page.getByRole('tab', { name: 'Project images' }).click()
+  await page.getByRole('button', { name: 'Image' }).click()
+  await page.getByRole('option', { name: images[0].name }).click()
+
+  // test that it still says 5, as that's larger than the given image
+  await expect(diskSizeInput).toHaveValue('5')
+
+  // pick a disk image that's larger than 5GiB (the third project image works [6GiB])
+  await page.getByRole('button', { name: 'Image' }).click()
+  await page.getByRole('option', { name: images[2].name }).click()
+
+  // test that it has been automatically increased to next-largest incremement of 10
+  await expect(diskSizeInput).toHaveValue('10')
+
+  // pick another image, just to verify that the diskSizeInput stays as it was
+  await page.getByRole('button', { name: 'Image' }).click()
+  await page.getByRole('option', { name: images[1].name }).click()
+  await expect(diskSizeInput).toHaveValue('10')
+
+  const submitButton = page.getByRole('button', { name: 'Create instance' })
+  await submitButton.click()
+
+  await expect(page).toHaveURL(`/projects/mock-project/instances/${instanceName}/storage`)
+  await expectVisible(page, [`h1:has-text("${instanceName}")`, 'text=10 GiB'])
+})
+
 test('with disk name already taken', async ({ page }) => {
   await page.goto('/projects/mock-project/instances-new')
   await page.fill('input[name=name]', 'my-instance')