Skip to content

Commit

Permalink
Fix auto-updating of boot disk size when selected image is larger tha…
Browse files Browse the repository at this point in the history 
…n 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
  • Loading branch information
@charliepark
charliepark authored Dec 7, 2023
1 parent a0bf47a commit 6ae6bee
Show file tree
Hide file tree
Showing 5 changed files with 147 additions and 59 deletions.
22 changes: 20 additions & 2 deletions app/components/form/fields/DiskSizeField.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -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"
Expand All @@ -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}
/>
Expand Down
5 changes: 2 additions & 3 deletions app/components/form/fields/NumberField.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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} />
</>
Expand Down
80 changes: 52 additions & 28 deletions app/forms/disk-create.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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' })
Expand Down Expand Up @@ -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',
Expand Down
62 changes: 36 additions & 26 deletions app/forms/instance-create.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
genName,
INSTANCE_MAX_CPU,
INSTANCE_MAX_RAM_GiB,
MAX_DISK_SIZE_GiB,
useApiMutation,
useApiQueryClient,
usePrefetchedApiQuery,
Expand Down Expand Up @@ -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"
Expand All @@ -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)
Expand Down Expand Up @@ -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>
Expand All @@ -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)`
}
}}
/>
Expand Down
37 changes: 37 additions & 0 deletions app/test/e2e/instance-create.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down

0 comments on commit 6ae6bee

Please sign in to comment.