Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/components/form/fields/ImageSelectField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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))
}
}}
/>
Expand Down
7 changes: 3 additions & 4 deletions app/forms/disk-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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))
}
}}
/>
Expand Down Expand Up @@ -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))
}
}}
/>
Expand Down
6 changes: 3 additions & 3 deletions app/forms/instance-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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 }],
}

Expand Down Expand Up @@ -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))
}
}}
>
Expand Down
9 changes: 6 additions & 3 deletions app/util/math.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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)
})
11 changes: 8 additions & 3 deletions app/util/math.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
17 changes: 15 additions & 2 deletions mock-api/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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',
},
Expand All @@ -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(),
Expand Down
12 changes: 10 additions & 2 deletions test/e2e/disks.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
expectNoToast,
expectRowVisible,
expectToast,
expectVisible,
test,
} from './utils'

Expand Down Expand Up @@ -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
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/pagination.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions test/e2e/snapshots.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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
Expand All @@ -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 }) => {
Expand Down
Loading