Skip to content

Commit 6ae6bee

Browse files
charlieparkcharliepark
authored
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
1 parent a0bf47a commit 6ae6bee

5 files changed

Lines changed: 147 additions & 59 deletions

File tree

‎app/components/form/fields/DiskSizeField.tsx‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@
55
*
66
* Copyright Oxide Computer Company
77
*/
8-
import type { FieldPath, FieldPathByValue, FieldValues } from 'react-hook-form'
8+
import type {
9+
FieldPath,
10+
FieldPathByValue,
11+
FieldValues,
12+
ValidateResult,
13+
} from 'react-hook-form'
914

1015
import { MAX_DISK_SIZE_GiB } from '@oxide/api'
1116

@@ -17,12 +22,19 @@ interface DiskSizeProps<
1722
TName extends FieldPath<TFieldValues>,
1823
> extends TextFieldProps<TFieldValues, TName> {
1924
minSize?: number
25+
validate?(diskSizeGiB: number): ValidateResult
2026
}
2127

2228
export function DiskSizeField<
2329
TFieldValues extends FieldValues,
2430
TName extends FieldPathByValue<TFieldValues, number>,
25-
>({ required = true, name, minSize = 1, ...props }: DiskSizeProps<TFieldValues, TName>) {
31+
>({
32+
required = true,
33+
name,
34+
minSize = 1,
35+
validate,
36+
...props
37+
}: DiskSizeProps<TFieldValues, TName>) {
2638
return (
2739
<NumberField
2840
units="GiB"
@@ -32,12 +44,18 @@ export function DiskSizeField<
3244
min={minSize}
3345
max={MAX_DISK_SIZE_GiB}
3446
validate={(diskSizeGiB) => {
47+
// Run a number of default validators
48+
if (Number.isNaN(diskSizeGiB)) {
49+
return 'Disk size is required'
50+
}
3551
if (diskSizeGiB < minSize) {
3652
return `Must be at least ${minSize} GiB`
3753
}
3854
if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
3955
return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
4056
}
57+
// Run any additional validators passed in from the callsite
58+
return validate?.(diskSizeGiB)
4159
}}
4260
{...props}
4361
/>

‎app/components/form/fields/NumberField.tsx‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ export const NumberFieldInner = <
7777
name={name}
7878
control={control}
7979
rules={{ required, validate }}
80-
render={({ field: { value, ...fieldRest }, fieldState: { error } }) => {
80+
render={({ field, fieldState: { error } }) => {
8181
return (
8282
<>
8383
<UINumberField
@@ -87,8 +87,7 @@ export const NumberFieldInner = <
8787
[`${id}-help-text`]: !!description,
8888
})}
8989
aria-describedby={description ? `${id}-label-tip` : undefined}
90-
defaultValue={value}
91-
{...fieldRest}
90+
{...field}
9291
/>
9392
<ErrorMessage error={error} label={label} />
9493
</>

‎app/forms/disk-create.tsx‎

Lines changed: 52 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* Copyright Oxide Computer Company
77
*/
88
import { format } from 'date-fns'
9+
import { useMemo } from 'react'
910
import { useController, type Control } from 'react-hook-form'
1011
import { useNavigate, type NavigateFunction } from 'react-router-dom'
1112

@@ -17,9 +18,10 @@ import {
1718
type Disk,
1819
type DiskCreate,
1920
type DiskSource,
21+
type Image,
2022
} from '@oxide/api'
2123
import { FieldLabel, FormDivider, Radio, RadioGroup } from '@oxide/ui'
22-
import { GiB } from '@oxide/util'
24+
import { bytesToGiB, GiB } from '@oxide/util'
2325

2426
import {
2527
DescriptionField,
@@ -79,6 +81,21 @@ export function CreateDiskSideModalForm({
7981
})
8082

8183
const form = useForm({ defaultValues })
84+
const { project } = useProjectSelector()
85+
const projectImages = useApiQuery('imageList', { query: { project } })
86+
const siloImages = useApiQuery('imageList', {})
87+
88+
// put project images first because if there are any, there probably aren't
89+
// very many and they're probably relevant
90+
const images = useMemo(
91+
() => [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])],
92+
[projectImages.data, siloImages.data]
93+
)
94+
const areImagesLoading = projectImages.isPending || siloImages.isPending
95+
96+
const selectedImageId = form.watch('diskSource.imageId')
97+
const selectedImageSize = images.find((image) => image.id === selectedImageId)?.size
98+
const imageSizeGiB = selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
8299

83100
return (
84101
<SideModalForm
@@ -96,13 +113,33 @@ export function CreateDiskSideModalForm({
96113
<NameField name="name" control={form.control} />
97114
<DescriptionField name="description" control={form.control} />
98115
<FormDivider />
99-
<DiskSourceField control={form.control} />
100-
<DiskSizeField name="size" control={form.control} />
116+
<DiskSourceField
117+
control={form.control}
118+
images={images}
119+
areImagesLoading={areImagesLoading}
120+
/>
121+
<DiskSizeField
122+
name="size"
123+
control={form.control}
124+
validate={(diskSizeGiB: number) => {
125+
if (imageSizeGiB && diskSizeGiB < imageSizeGiB) {
126+
return `Must be as large as selected image (min. ${imageSizeGiB} GiB)`
127+
}
128+
}}
129+
/>
101130
</SideModalForm>
102131
)
103132
}
104133

105-
const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
134+
const DiskSourceField = ({
135+
control,
136+
images,
137+
areImagesLoading,
138+
}: {
139+
control: Control<DiskCreate>
140+
images: Image[]
141+
areImagesLoading: boolean
142+
}) => {
106143
const {
107144
field: { value, onChange },
108145
} = useController({ control, name: 'diskSource' })
@@ -144,37 +181,24 @@ const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
144181
]}
145182
/>
146183
)}
147-
{value.type === 'image' && <ImageSelectField control={control} />}
184+
{value.type === 'image' && (
185+
<ListboxField
186+
control={control}
187+
name="diskSource.imageId"
188+
label="Source image"
189+
placeholder="Select an image"
190+
isLoading={areImagesLoading}
191+
items={images.map((i) => toListboxItem(i, true))}
192+
required
193+
/>
194+
)}
148195

149196
{value.type === 'snapshot' && <SnapshotSelectField control={control} />}
150197
</div>
151198
</>
152199
)
153200
}
154201

155-
const ImageSelectField = ({ control }: { control: Control<DiskCreate> }) => {
156-
const { project } = useProjectSelector()
157-
158-
const projectImages = useApiQuery('imageList', { query: { project } })
159-
const siloImages = useApiQuery('imageList', {})
160-
161-
// put project images first because if there are any, there probably aren't
162-
// very many and they're probably relevant
163-
const images = [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])]
164-
165-
return (
166-
<ListboxField
167-
control={control}
168-
name="diskSource.imageId"
169-
label="Source image"
170-
placeholder="Select an image"
171-
isLoading={projectImages.isPending || siloImages.isPending}
172-
items={images.map((i) => toListboxItem(i, true))}
173-
required
174-
/>
175-
)
176-
}
177-
178202
const DiskNameFromId = ({ disk }: { disk: string }) => {
179203
const { data, isPending, isError } = useApiQuery(
180204
'diskView',

‎app/forms/instance-create.tsx‎

Lines changed: 36 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import {
1414
genName,
1515
INSTANCE_MAX_CPU,
1616
INSTANCE_MAX_RAM_GiB,
17-
MAX_DISK_SIZE_GiB,
1817
useApiMutation,
1918
useApiQueryClient,
2019
usePrefetchedApiQuery,
@@ -137,9 +136,11 @@ export function CreateInstanceForm() {
137136

138137
const imageInput = useWatch({ control: control, name: 'image' })
139138
const image = allImages.find((i) => i.id === imageInput)
139+
const imageSize = image?.size ? Math.ceil(image.size / GiB) : undefined
140140

141141
return (
142142
<FullPageForm
143+
submitDisabled={allImages.length ? undefined : 'Image required'}
143144
id="create-instance-form"
144145
form={form}
145146
title="Create instance"
@@ -151,9 +152,10 @@ export function CreateInstanceForm() {
151152
values.presetId === 'custom'
152153
? { memory: values.memory, ncpus: values.ncpus }
153154
: { memory: preset.memory, ncpus: preset.ncpus }
154-
155-
// we should also never have an image ID that's not in the list
156155
const image = allImages.find((i) => values.image === i.id)
156+
// There should always be an image present, because …
157+
// - The form is disabled unless there are images available.
158+
// - The form defaults to including at least one image.
157159
invariant(image, 'Expected image to be defined')
158160

159161
const bootDiskName = values.bootDiskName || genName(values.name, image.name)
@@ -287,37 +289,51 @@ export function CreateInstanceForm() {
287289
<FormDivider />
288290

289291
<Form.Heading id="boot-disk">Boot disk</Form.Heading>
290-
<Tabs.Root id="boot-disk-tabs" className="full-width" defaultValue="project">
292+
<Tabs.Root
293+
id="boot-disk-tabs"
294+
className="full-width"
295+
// default to the project images tab if there are only project images
296+
defaultValue={
297+
projectImages.length > 0 && siloImages.length === 0 ? 'project' : 'silo'
298+
}
299+
>
291300
<Tabs.List aria-describedby="boot-disk">
292-
<Tabs.Trigger value="project">Project images</Tabs.Trigger>
293301
<Tabs.Trigger value="silo">Silo images</Tabs.Trigger>
302+
<Tabs.Trigger value="project">Project images</Tabs.Trigger>
294303
</Tabs.List>
295-
<Tabs.Content value="project" className="space-y-4">
296-
{projectImages.length === 0 ? (
304+
{allImages.length === 0 && (
305+
<Message
306+
className="mb-8 ml-10 max-w-lg"
307+
variant="notice"
308+
content="Images are required to create a boot disk."
309+
/>
310+
)}
311+
<Tabs.Content value="silo" className="space-y-4">
312+
{siloImages.length === 0 ? (
297313
<div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
298314
<EmptyMessage
299315
icon={<Images16Icon />}
300-
title="No project images found"
301-
body="An image needs to be uploaded to be seen here"
302-
buttonText="Upload image"
303-
onClick={() => navigate(pb.projectImageNew(projectSelector))}
316+
title="No silo images found"
317+
body="Project images need to be promoted to be seen here"
304318
/>
305319
</div>
306320
) : (
307-
<ImageSelectField images={projectImages} control={control} />
321+
<ImageSelectField images={siloImages} control={control} />
308322
)}
309323
</Tabs.Content>
310-
<Tabs.Content value="silo" className="space-y-4">
311-
{siloImages.length === 0 ? (
324+
<Tabs.Content value="project" className="space-y-4">
325+
{projectImages.length === 0 ? (
312326
<div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
313327
<EmptyMessage
314328
icon={<Images16Icon />}
315-
title="No silo images found"
316-
body="Project images need to be promoted to be seen here"
329+
title="No project images found"
330+
body="An image needs to be uploaded to be seen here"
331+
buttonText="Upload image"
332+
onClick={() => navigate(pb.projectImageNew(projectSelector))}
317333
/>
318334
</div>
319335
) : (
320-
<ImageSelectField images={siloImages} control={control} />
336+
<ImageSelectField images={projectImages} control={control} />
321337
)}
322338
</Tabs.Content>
323339
</Tabs.Root>
@@ -328,15 +344,9 @@ export function CreateInstanceForm() {
328344
label="Disk size"
329345
name="bootDiskSize"
330346
control={control}
331-
// Imitate API logic: only require that the disk is big enough to fit the image
332-
validate={(diskSizeGiB) => {
333-
if (!image) return true
334-
if (diskSizeGiB < image.size / GiB) {
335-
const minSize = Math.ceil(image.size / GiB)
336-
return `Must be as large as selected image (min. ${minSize} GiB)`
337-
}
338-
if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
339-
return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
347+
validate={(diskSizeGiB: number) => {
348+
if (imageSize && diskSizeGiB < imageSize) {
349+
return `Must be as large as selected image (min. ${imageSize} GiB)`
340350
}
341351
}}
342352
/>

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,43 @@ test('can create an instance with custom hardware', async ({ page }) => {
128128
])
129129
})
130130

131+
test('automatically updates disk size when larger image selected', async ({ page }) => {
132+
await page.goto('/projects/mock-project/instances-new')
133+
134+
const instanceName = 'my-new-instance'
135+
await page.fill('input[name=name]', instanceName)
136+
137+
// set the disk size larger than it needs to be, to verify it doesn't get reduced
138+
const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' })
139+
await diskSizeInput.fill('5')
140+
141+
// pick a disk image that's smaller than 5GiB (the first project image works [4GiB])
142+
await page.getByRole('tab', { name: 'Project images' }).click()
143+
await page.getByRole('button', { name: 'Image' }).click()
144+
await page.getByRole('option', { name: images[0].name }).click()
145+
146+
// test that it still says 5, as that's larger than the given image
147+
await expect(diskSizeInput).toHaveValue('5')
148+
149+
// pick a disk image that's larger than 5GiB (the third project image works [6GiB])
150+
await page.getByRole('button', { name: 'Image' }).click()
151+
await page.getByRole('option', { name: images[2].name }).click()
152+
153+
// test that it has been automatically increased to next-largest incremement of 10
154+
await expect(diskSizeInput).toHaveValue('10')
155+
156+
// pick another image, just to verify that the diskSizeInput stays as it was
157+
await page.getByRole('button', { name: 'Image' }).click()
158+
await page.getByRole('option', { name: images[1].name }).click()
159+
await expect(diskSizeInput).toHaveValue('10')
160+
161+
const submitButton = page.getByRole('button', { name: 'Create instance' })
162+
await submitButton.click()
163+
164+
await expect(page).toHaveURL(`/projects/mock-project/instances/${instanceName}/storage`)
165+
await expectVisible(page, [`h1:has-text("${instanceName}")`, 'text=10 GiB'])
166+
})
167+
131168
test('with disk name already taken', async ({ page }) => {
132169
await page.goto('/projects/mock-project/instances-new')
133170
await page.fill('input[name=name]', 'my-instance')

0 commit comments

Comments
 (0)