From 65a84ac8e09559dccac92127595bb72be12e2b09 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sun, 22 Sep 2024 00:39:02 -0500 Subject: [PATCH 01/25] bump API for boot disk changes --- OMICRON_VERSION | 2 +- app/api/__generated__/Api.ts | 50 ++++++++++++++++++++++++--- app/api/__generated__/OMICRON_VERSION | 2 +- app/api/__generated__/msw-handlers.ts | 16 +++++++++ app/api/__generated__/validate.ts | 24 ++++++++++++- 5 files changed, 86 insertions(+), 8 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 451cd1c94b..a7f47b9fef 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -d2e3f344e82fd7a1f45a39a9d1f867f9238d171d +414a0b5191dfa9e28d5ada696b72647fe98a3d99 diff --git a/app/api/__generated__/Api.ts b/app/api/__generated__/Api.ts index 6a327138ef..2d3fac3237 100644 --- a/app/api/__generated__/Api.ts +++ b/app/api/__generated__/Api.ts @@ -1755,6 +1755,8 @@ export type InstanceState = * View of an Instance */ export type Instance = { + /** the ID of the disk used to boot this Instance, if a specific one is assigned. */ + bootDiskId?: string /** human-readable free-form text about a resource */ description: string /** RFC1035-compliant hostname for the Instance. */ @@ -1829,6 +1831,8 @@ If more than one interface is provided, then the first will be designated the pr * Create-time parameters for an `Instance` */ export type InstanceCreate = { + /** Choice of which disk this instance should boot from. */ + bootDisk?: NameOrId description: string /** The disks to be created or attached for this instance. */ disks?: InstanceDiskAttachment[] @@ -1937,6 +1941,11 @@ export type InstanceSerialConsoleData = { lastByteOffset: number } +/** + * Parameters of an `Instance` that can be reconfigured after creation. + */ +export type InstanceUpdate = { bootDisk?: NameOrId } + /** * A collection of IP ranges. If a pool is linked to a silo, IP addresses from the pool can be allocated within that silo */ @@ -2579,18 +2588,18 @@ export type RouteConfig = { } /** - * A `RouteDestination` is used to match traffic with a routing rule based on the destination of that traffic. + * A `RouteDestination` is used to match traffic with a routing rule, on the destination of that traffic. * * When traffic is to be sent to a destination that is within a given `RouteDestination`, the corresponding `RouterRoute` applies, and traffic will be forward to the `RouteTarget` for that rule. */ export type RouteDestination = - /** Route applies to traffic destined for the specified IP address */ + /** Route applies to traffic destined for a specific IP address */ | { type: 'ip'; value: string } - /** Route applies to traffic destined for the specified IP subnet */ + /** Route applies to traffic destined for a specific IP subnet */ | { type: 'ip_net'; value: IpNet } - /** Route applies to traffic destined for the specified VPC */ + /** Route applies to traffic destined for the given VPC. */ | { type: 'vpc'; value: Name } - /** Route applies to traffic destined for the specified VPC subnet */ + /** Route applies to traffic */ | { type: 'subnet'; value: Name } /** @@ -4196,6 +4205,14 @@ export interface InstanceViewQueryParams { project?: NameOrId } +export interface InstanceUpdatePathParams { + instance: NameOrId +} + +export interface InstanceUpdateQueryParams { + project?: NameOrId +} + export interface InstanceDeletePathParams { instance: NameOrId } @@ -5688,6 +5705,29 @@ export class Api extends HttpClient { ...params, }) }, + /** + * Update instance + */ + instanceUpdate: ( + { + path, + query = {}, + body, + }: { + path: InstanceUpdatePathParams + query?: InstanceUpdateQueryParams + body: InstanceUpdate + }, + params: FetchParams = {} + ) => { + return this.request({ + path: `/v1/instances/${path.instance}`, + method: 'PUT', + body, + query, + ...params, + }) + }, /** * Delete instance */ diff --git a/app/api/__generated__/OMICRON_VERSION b/app/api/__generated__/OMICRON_VERSION index ffdb941808..fa425c863d 100644 --- a/app/api/__generated__/OMICRON_VERSION +++ b/app/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -d2e3f344e82fd7a1f45a39a9d1f867f9238d171d +414a0b5191dfa9e28d5ada696b72647fe98a3d99 diff --git a/app/api/__generated__/msw-handlers.ts b/app/api/__generated__/msw-handlers.ts index 27fb72c2f2..49df51a418 100644 --- a/app/api/__generated__/msw-handlers.ts +++ b/app/api/__generated__/msw-handlers.ts @@ -305,6 +305,14 @@ export interface MSWHandlers { req: Request cookies: Record }) => Promisable> + /** `PUT /v1/instances/:instance` */ + instanceUpdate: (params: { + path: Api.InstanceUpdatePathParams + query: Api.InstanceUpdateQueryParams + body: Json + req: Request + cookies: Record + }) => Promisable> /** `DELETE /v1/instances/:instance` */ instanceDelete: (params: { path: Api.InstanceDeletePathParams @@ -1593,6 +1601,14 @@ export function makeHandlers(handlers: MSWHandlers): HttpHandler[] { '/v1/instances/:instance', handler(handlers['instanceView'], schema.InstanceViewParams, null) ), + http.put( + '/v1/instances/:instance', + handler( + handlers['instanceUpdate'], + schema.InstanceUpdateParams, + schema.InstanceUpdate + ) + ), http.delete( '/v1/instances/:instance', handler(handlers['instanceDelete'], schema.InstanceDeleteParams, null) diff --git a/app/api/__generated__/validate.ts b/app/api/__generated__/validate.ts index 5b1902e28c..9bac9f6d9c 100644 --- a/app/api/__generated__/validate.ts +++ b/app/api/__generated__/validate.ts @@ -1680,6 +1680,7 @@ export const InstanceState = z.preprocess( export const Instance = z.preprocess( processResponseBody, z.object({ + bootDiskId: z.string().uuid().optional(), description: z.string(), hostname: z.string(), id: z.string().uuid(), @@ -1743,6 +1744,7 @@ export const InstanceNetworkInterfaceAttachment = z.preprocess( export const InstanceCreate = z.preprocess( processResponseBody, z.object({ + bootDisk: NameOrId.default(null).optional(), description: z.string(), disks: InstanceDiskAttachment.array().default([]).optional(), externalIps: ExternalIpCreate.array().default([]).optional(), @@ -1833,6 +1835,14 @@ export const InstanceSerialConsoleData = z.preprocess( z.object({ data: z.number().min(0).max(255).array(), lastByteOffset: z.number().min(0) }) ) +/** + * Parameters of an `Instance` that can be reconfigured after creation. + */ +export const InstanceUpdate = z.preprocess( + processResponseBody, + z.object({ bootDisk: NameOrId.optional() }) +) + /** * A collection of IP ranges. If a pool is linked to a silo, IP addresses from the pool can be allocated within that silo */ @@ -2475,7 +2485,7 @@ export const RouteConfig = z.preprocess( ) /** - * A `RouteDestination` is used to match traffic with a routing rule based on the destination of that traffic. + * A `RouteDestination` is used to match traffic with a routing rule, on the destination of that traffic. * * When traffic is to be sent to a destination that is within a given `RouteDestination`, the corresponding `RouterRoute` applies, and traffic will be forward to the `RouteTarget` for that rule. */ @@ -4148,6 +4158,18 @@ export const InstanceViewParams = z.preprocess( }) ) +export const InstanceUpdateParams = z.preprocess( + processResponseBody, + z.object({ + path: z.object({ + instance: NameOrId, + }), + query: z.object({ + project: NameOrId.optional(), + }), + }) +) + export const InstanceDeleteParams = z.preprocess( processResponseBody, z.object({ From c90040610791f2f054ff7f705316a3c058e26de7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 23 Sep 2024 11:10:29 -0500 Subject: [PATCH 02/25] implement instance update endpoint --- mock-api/msw/handlers.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index d1d346615e..2a5f93e008 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -542,6 +542,25 @@ export const handlers = makeHandlers({ return json(newInstance, { status: 201 }) }, instanceView: ({ path, query }) => lookup.instance({ ...path, ...query }), + instanceUpdate({ path, query, body }) { + const instance = lookup.instance({ ...path, ...query }) + + if (body.boot_disk) { + // Only include project if it's a name, otherwise lookup will error. + // This will 404 if the disk doesn't exist, which I think is right. + const disk = lookup.disk({ + disk: body.boot_disk, + project: isUuid(body.boot_disk) ? undefined : query.project, + }) + + instance.boot_disk_id = disk.id + } else { + // we're clearing the boot disk! + instance.boot_disk_id = undefined + } + + return instance + }, instanceDelete({ path, query }) { const instance = lookup.instance({ ...path, ...query }) db.instances = db.instances.filter((i) => i.id !== instance.id) From 63aca059ea0f2a34c63b4c96147eff681b59d885 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 23 Sep 2024 11:23:11 -0500 Subject: [PATCH 03/25] show boot disk with badge in table --- .../instances/instance/tabs/StorageTab.tsx | 23 ++++++++++++++++--- mock-api/instance.ts | 1 + 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index 5e05d34df0..422eaeddcc 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -30,6 +30,7 @@ import { addToast } from '~/stores/toast' import { useColsWithActions, type MenuAction } from '~/table/columns/action-col' import { Columns } from '~/table/columns/common' import { Table } from '~/table/Table' +import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { TableEmptyBox } from '~/ui/lib/Table' @@ -67,11 +68,22 @@ StorageTab.loader = async ({ params }: LoaderFunctionArgs) => { // row actions menus type InstanceDisk = Disk & { instanceState: InstanceState + isBootDisk: boolean } const colHelper = createColumnHelper() const staticCols = [ - colHelper.accessor('name', { header: 'Disk' }), + colHelper.accessor('name', { + header: 'Disk', + cell: (info) => { + return ( + <> + {info.getValue()} + {info.row.original.isBootDisk && Boot} + + ) + }, + }), colHelper.accessor('size', Columns.size), colHelper.accessor((row) => row.state.state, { header: 'state', @@ -177,8 +189,13 @@ export function StorageTab() { const { data: disks } = usePrefetchedApiQuery('instanceDiskList', instancePathQuery) const rows = useMemo( - () => disks.items.map((disk) => ({ ...disk, instanceState: instance.runState })), - [disks.items, instance.runState] + () => + disks.items.map((disk) => ({ + ...disk, + instanceState: instance.runState, + isBootDisk: instance.bootDiskId === disk.id, + })), + [disks.items, instance.runState, instance.bootDiskId] ) const table = useReactTable({ diff --git a/mock-api/instance.ts b/mock-api/instance.ts index c519297c02..63ced52f5f 100644 --- a/mock-api/instance.ts +++ b/mock-api/instance.ts @@ -21,6 +21,7 @@ export const instance: Json = { hostname: 'oxide.com', project_id: project.id, run_state: 'running', + boot_disk_id: '7f2309a5-13e3-47e0-8a4c-2a3b3bc992fd', // disk-1 time_created: new Date().toISOString(), time_modified: new Date().toISOString(), time_run_state_updated: new Date().toISOString(), From 2916de682a50c1666fa36927bf72ebc04474e1a2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 23 Sep 2024 11:36:39 -0500 Subject: [PATCH 04/25] add row actions --- .../instances/instance/tabs/StorageTab.tsx | 39 ++++++++++++++++++- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index 422eaeddcc..43331d25e3 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -22,10 +22,12 @@ import { } from '@oxide/api' import { Storage24Icon } from '@oxide/design-system/icons/react' +import { HL } from '~/components/HL' import { DiskStateBadge } from '~/components/StateBadge' import { AttachDiskSideModalForm } from '~/forms/disk-attach' import { CreateDiskSideModalForm } from '~/forms/disk-create' import { getInstanceSelector, useInstanceSelector } from '~/hooks/use-params' +import { confirmAction } from '~/stores/confirm-action' import { addToast } from '~/stores/toast' import { useColsWithActions, type MenuAction } from '~/table/columns/action-col' import { Columns } from '~/table/columns/common' @@ -132,6 +134,12 @@ export function StorageTab() { const { data: instance } = usePrefetchedApiQuery('instanceView', instancePathQuery) + const { mutateAsync: instanceUpdate } = useApiMutation('instanceUpdate', { + onSuccess() { + apiQueryClient.invalidateQueries('instanceView') + }, + }) + const makeActions = useCallback( (disk: InstanceDisk): MenuAction[] => [ { @@ -163,11 +171,38 @@ export function StorageTab() { ), onActivate() { - detachDisk({ body: { disk: disk.name }, ...instancePathQuery }) + detachDisk({ body: { disk: disk.name }, path: { instance: instance.id } }) + }, + }, + { + label: disk.isBootDisk ? 'Unset as boot disk' : 'Set as boot disk', + onActivate: () => { + const verb = disk.isBootDisk ? 'unset' : 'set' + return confirmAction({ + doAction: () => + instanceUpdate({ + path: { instance: instance.id }, + body: { bootDisk: disk.isBootDisk ? undefined : disk.id }, + }), + errorTitle: `Could not ${verb} boot disk`, + modalTitle: `Confirm ${verb} boot disk`, + // TODO: link to docs in both cases + modalContent: disk.isBootDisk ? ( +

+ Are you sure you want to unset {disk.name} as the boot disk? This + will TODO COPY RE: WHAT HAPPENS HERE +

+ ) : ( +

+ Are you sure you want to set {disk.name} as the boot disk? +

+ ), + actionType: 'primary', + }) }, }, ], - [detachDisk, instancePathQuery, createSnapshot, project] + [detachDisk, createSnapshot, project, instanceUpdate, instance.id] ) const attachDisk = useApiMutation('instanceDiskAttach', { From 9b31e264db3a906ff5f653135b6336162a81b2fb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 23 Sep 2024 12:28:29 -0500 Subject: [PATCH 05/25] instance create works good --- app/forms/instance-create.tsx | 1 + mock-api/msw/handlers.ts | 43 +++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index e0a33bc08c..ebfc62a12c 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -318,6 +318,7 @@ export function CreateInstanceForm() { memory: instance.memory * GiB, ncpus: instance.ncpus, disks: [bootDisk, ...values.disks], + bootDisk: bootDisk.name, externalIps: values.externalIps, start: values.start, networkInterfaces: values.networkInterfaces, diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 2a5f93e008..e9e1e5dac5 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -440,6 +440,41 @@ export const handlers = makeHandlers({ }) } + // Validate boot disk against list of disks to create/attach. This must + // happen before disks are created/attached because it can throw. + if (body.boot_disk) { + // if it's an ID, things are annoying + if (isUuid(body.boot_disk)) { + // it must already exist + const disk = lookup.disk({ disk: body.boot_disk }) + // it must be in this project + if (disk.project_id !== project.id) { + throw notFoundErr( + `boot disk in project '${project.name}' with ID '${body.boot_disk}'` + ) + } + // it must be detached because we're going to attach it + if (disk.state.state !== 'detached') { + throw "When specified boot disk already exists, it must be in state 'detached'" + } + // it must be in the list of disks to attach! disks are attached by name (currently, + // there is an issue to make it NameOrId) so we use the name to make sure it's in the list + const inListOfDisksToAttach = (body.disks || []).some((d) => { + if (d.type === 'create') return false + return disk.name === d.name + }) + if (!inListOfDisksToAttach) { + throw 'Specified boot disk must be in the list of disks to attach' + } + } else { + // otherwise it's a name, and we have to make sure it's somewhere in the list + const inListOfDisks = (body.disks || []).some((d) => body.boot_disk === d.name) + if (!inListOfDisks) { + throw `Specified boot disk '${body.boot_disk}' not found in list of disks` + } + } + } + for (const diskParams of body.disks || []) { if (diskParams.type === 'create') { const { size, name, description, disk_source } = diskParams @@ -505,6 +540,14 @@ export const handlers = makeHandlers({ ...getTimestamps(), run_state: 'creating', time_run_state_updated: new Date().toISOString(), + // Note this relies on the disk already existing. This would be risky + // without the ton of validation before we do anything with disks. + boot_disk_id: body.boot_disk + ? lookup.disk({ + disk: body.boot_disk, + project: isUuid(body.boot_disk) ? undefined : project.id, + }).id + : undefined, } body.external_ips?.forEach((ip) => { From 108e6c250b9b2e7badfe9ce57422787e6303a2f0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 23 Sep 2024 16:14:48 -0500 Subject: [PATCH 06/25] point at the actual omicron PR --- OMICRON_VERSION | 2 +- app/api/__generated__/OMICRON_VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index a7f47b9fef..1cb2132dcf 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -414a0b5191dfa9e28d5ada696b72647fe98a3d99 +1537af9925bb9c2fa931b58d08be82a684bfc12a diff --git a/app/api/__generated__/OMICRON_VERSION b/app/api/__generated__/OMICRON_VERSION index fa425c863d..01a69b73a0 100644 --- a/app/api/__generated__/OMICRON_VERSION +++ b/app/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -414a0b5191dfa9e28d5ada696b72647fe98a3d99 +1537af9925bb9c2fa931b58d08be82a684bfc12a From 0860ce8558501be133357f2b4011d538fc72170e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Sep 2024 14:08:27 -0500 Subject: [PATCH 07/25] split instance disks into two tables --- app/api/util.ts | 3 + .../instances/instance/tabs/StorageTab.tsx | 224 +++++++++++------- mock-api/msw/handlers.ts | 6 +- 3 files changed, 145 insertions(+), 88 deletions(-) diff --git a/app/api/util.ts b/app/api/util.ts index 5d997c02c4..08a24c5e0e 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -96,6 +96,9 @@ const instanceActions: Record = { // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/instance.rs#L865 delete: ['stopped', 'failed'], + // TODO: whence my licence to make such a claim + update: ['stopped'], + // reboot and stop are kind of weird! // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/instance.rs#L790-L798 // https://github.com/oxidecomputer/propolis/blob/b278193/bin/propolis-server/src/lib/vm/request_queue.rs diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index 43331d25e3..b98ca272cd 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -8,6 +8,7 @@ import { createColumnHelper, getCoreRowModel, useReactTable } from '@tanstack/react-table' import { useCallback, useMemo, useState } from 'react' import type { LoaderFunctionArgs } from 'react-router-dom' +import * as R from 'remeda' import { apiQueryClient, @@ -32,19 +33,29 @@ import { addToast } from '~/stores/toast' import { useColsWithActions, type MenuAction } from '~/table/columns/action-col' import { Columns } from '~/table/columns/common' import { Table } from '~/table/Table' -import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' import { EmptyMessage } from '~/ui/lib/EmptyMessage' -import { TableEmptyBox } from '~/ui/lib/Table' +import { TableControls, TableEmptyBox, TableTitle } from '~/ui/lib/Table' import { fancifyStates } from './common' -const EmptyState = () => ( +const BootDiskEmptyState = () => ( } - title="No disks" - body="Attach a disk to this instance to see it here" + title="No boot disk set" + // TODO: boot order docs link + body="Read about boot order LINK HERE" + /> + +) + +const OtherDisksEmptyState = () => ( + + } + title="No other disks" + body="Attach a disk to see it here" /> ) @@ -70,22 +81,11 @@ StorageTab.loader = async ({ params }: LoaderFunctionArgs) => { // row actions menus type InstanceDisk = Disk & { instanceState: InstanceState - isBootDisk: boolean } const colHelper = createColumnHelper() const staticCols = [ - colHelper.accessor('name', { - header: 'Disk', - cell: (info) => { - return ( - <> - {info.getValue()} - {info.row.original.isBootDisk && Boot} - - ) - }, - }), + colHelper.accessor('name', { header: 'Disk' }), colHelper.accessor('size', Columns.size), colHelper.accessor((row) => row.state.state, { header: 'state', @@ -141,67 +141,76 @@ export function StorageTab() { }) const makeActions = useCallback( - (disk: InstanceDisk): MenuAction[] => [ - { - label: 'Snapshot', - disabled: !diskCan.snapshot(disk) && ( - <> - Only disks in state {fancifyStates(diskCan.snapshot.states)} can be snapshotted - - ), - onActivate() { - createSnapshot({ - query: { project }, - body: { - name: genName(disk.name), - disk: disk.name, - description: '', - }, - }) + // TODO: don't do this, just have two separate lists. come on + (isBootDisk: boolean) => + (disk: InstanceDisk): MenuAction[] => [ + { + label: 'Snapshot', + disabled: !diskCan.snapshot(disk) && ( + <> + Only disks in state {fancifyStates(diskCan.snapshot.states)} can be + snapshotted + + ), + onActivate() { + createSnapshot({ + query: { project }, + body: { + name: genName(disk.name), + disk: disk.name, + description: '', + }, + }) + }, }, - }, - { - // don't bother checking disk state: assume that if it is showing up - // in this list, it can be detached - label: 'Detach', - disabled: !instanceCan.detachDisk({ runState: disk.instanceState }) && ( - <> - Instance must be stopped before disk can - be detached - - ), - onActivate() { - detachDisk({ body: { disk: disk.name }, path: { instance: instance.id } }) + { + // don't bother checking disk state: assume that if it is showing up + // in this list, it can be detached + label: 'Detach', + disabled: !instanceCan.detachDisk({ runState: disk.instanceState }) && ( + <> + Instance must be stopped before disk can + be detached + + ), + onActivate() { + detachDisk({ body: { disk: disk.name }, path: { instance: instance.id } }) + }, }, - }, - { - label: disk.isBootDisk ? 'Unset as boot disk' : 'Set as boot disk', - onActivate: () => { - const verb = disk.isBootDisk ? 'unset' : 'set' - return confirmAction({ - doAction: () => - instanceUpdate({ - path: { instance: instance.id }, - body: { bootDisk: disk.isBootDisk ? undefined : disk.id }, - }), - errorTitle: `Could not ${verb} boot disk`, - modalTitle: `Confirm ${verb} boot disk`, - // TODO: link to docs in both cases - modalContent: disk.isBootDisk ? ( -

- Are you sure you want to unset {disk.name} as the boot disk? This - will TODO COPY RE: WHAT HAPPENS HERE -

- ) : ( -

- Are you sure you want to set {disk.name} as the boot disk? -

- ), - actionType: 'primary', - }) + { + label: isBootDisk ? 'Unset boot disk' : 'Set as boot disk', + disabled: !instanceCan.update({ runState: disk.instanceState }) && ( + <> + Instance must be stopped before boot + disk can be changed + + ), + onActivate: () => { + const verb = isBootDisk ? 'unset' : 'set' + return confirmAction({ + doAction: () => + instanceUpdate({ + path: { instance: instance.id }, + body: { bootDisk: isBootDisk ? undefined : disk.id }, + }), + errorTitle: `Could not ${verb} boot disk`, + modalTitle: `Confirm ${verb} boot disk`, + // TODO: copy + link to docs in both cases + modalContent: isBootDisk ? ( +

+ Are you sure you want to unset {disk.name} as the boot disk? This + will TODO COPY RE: WHAT HAPPENS HERE +

+ ) : ( +

+ Are you sure you want to set {disk.name} as the boot disk? +

+ ), + actionType: 'primary', + }) + }, }, - }, - ], + ], [detachDisk, createSnapshot, project, instanceUpdate, instance.id] ) @@ -223,25 +232,66 @@ export function StorageTab() { const { data: disks } = usePrefetchedApiQuery('instanceDiskList', instancePathQuery) - const rows = useMemo( - () => - disks.items.map((disk) => ({ - ...disk, - instanceState: instance.runState, - isBootDisk: instance.bootDiskId === disk.id, - })), - [disks.items, instance.runState, instance.bootDiskId] + const [bootDisks, otherDisks] = useMemo( + () => R.partition(disks.items, (d) => d.id === instance.bootDiskId), + [disks.items, instance.bootDiskId] ) - const table = useReactTable({ - columns: useColsWithActions(staticCols, makeActions), - data: rows, + const bootDisksTable = useReactTable({ + columns: useColsWithActions(staticCols, makeActions(true)), + data: useMemo( + () => bootDisks.map((disk) => ({ ...disk, instanceState: instance.runState })), + [bootDisks, instance.runState] + ), + getCoreRowModel: getCoreRowModel(), + }) + + const otherDisksTable = useReactTable({ + columns: useColsWithActions(staticCols, makeActions(false)), + data: useMemo( + () => otherDisks.map((disk) => ({ ...disk, instanceState: instance.runState })), + [otherDisks, instance.runState] + ), getCoreRowModel: getCoreRowModel(), }) return ( <> - {disks.items.length > 0 ? : } + + Boot disk + + {bootDisks.length > 0 ? ( +
+ ) : ( + + )} +
+
+ +
+
+ + + Other disks + + {otherDisks.length > 0 ? ( +
+ ) : ( + + )} +
- ) : ( - - )} -
-
- - - Other disks - {otherDisks.length > 0 ? ( -
+ {bootDisks.length > 0 ? ( +
) : ( - + )} +
-
+ + Other disks
- + Create disk +
- {!instanceCan.attachDisk(instance) && ( - - The instance must be stopped to add or - attach a disk. - - )} -
+ + {otherDisks.length > 0 ? ( +
+ ) : ( + + )} + {showDiskCreate && ( setShowDiskCreate(false)} diff --git a/test/e2e/instance-disks.e2e.ts b/test/e2e/instance-disks.e2e.ts index 95981b277b..6fbc0f7d1b 100644 --- a/test/e2e/instance-disks.e2e.ts +++ b/test/e2e/instance-disks.e2e.ts @@ -18,9 +18,6 @@ import { test('Attach disk', async ({ page }) => { await page.goto('/projects/mock-project/instances/db1') - const warning = 'The instance must be stopped to add or attach a disk.' - await expect(page.getByText(warning)).toBeVisible() - const row = page.getByRole('row', { name: 'disk-1', exact: false }) await expect(row).toBeVisible() @@ -36,17 +33,20 @@ test('Attach disk', async ({ page }) => { // Have to stop instance to edit disks await stopInstance(page) - await expect(page.getByText(warning)).toBeHidden() - // New disk form - await page.click('role=button[name="Create new disk"]') - await expectVisible(page, [ - 'role=textbox[name="Name"]', - 'role=textbox[name="Description"]', - 'role=radiogroup[name="Block size (Bytes)"]', - 'role=textbox[name="Size (GiB)"]', - 'role=button[name="Create disk"]', - ]) + const createForm = page.getByRole('dialog', { name: 'Create disk' }) + await expect(createForm).toBeHidden() + await page.getByRole('button', { name: 'Create disk' }).click() + await expect(createForm).toBeVisible() + + await expect(createForm.getByRole('textbox', { name: 'Name' })).toBeVisible() + await expect(createForm.getByRole('textbox', { name: 'Description' })).toBeVisible() + await expect( + createForm.getByRole('radiogroup', { name: 'Block size (Bytes)' }) + ).toBeVisible() + await expect(createForm.getByRole('textbox', { name: 'Size (GiB)' })).toBeVisible() + await expect(createForm.getByRole('button', { name: 'Create disk' })).toBeVisible() + await page.click('role=button[name="Cancel"]') // Attach existing disk form @@ -67,6 +67,8 @@ test('Attach disk', async ({ page }) => { await expectVisible(page, ['role=cell[name="disk-3"]']) }) +// TODO: move create form asserts to their own test and actually test the create! + test('Detach disk', async ({ page }) => { await page.goto('/projects/mock-project/instances/db1') @@ -103,3 +105,5 @@ test('Snapshot disk', async ({ page }) => { disk: 'disk-2', }) }) + +// TODO: tests for different combinations of boot and other disks From 2eb1588964ffa7d3c1ea7a6456ec476d3775eab2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Sep 2024 15:00:35 -0500 Subject: [PATCH 09/25] fix one our most pointless e2es --- test/e2e/click-everything.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/click-everything.e2e.ts b/test/e2e/click-everything.e2e.ts index 95db0babe8..e2ead76cc8 100644 --- a/test/e2e/click-everything.e2e.ts +++ b/test/e2e/click-everything.e2e.ts @@ -23,7 +23,7 @@ test('Click through instance page', async ({ page }) => { 'role=cell[name="disk-1"]', 'role=cell[name="disk-2"]', // buttons disabled while instance is running - 'role=button[name="Create new disk"][disabled]', + 'role=button[name="Create disk"][disabled]', 'role=button[name="Attach existing disk"][disabled]', // TODO: assert minitable contents ]) From 4ef587b133f49e5daedf3b3d696f8f90226600e4 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Sep 2024 15:21:24 -0500 Subject: [PATCH 10/25] assert about boot disks after instance create --- test/e2e/instance-create.e2e.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 5ccf320d3b..289f5f115d 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -114,6 +114,22 @@ test('can create an instance', async ({ page }) => { 'text=from space', ]) + // instance goes from creating to starting to running as we poll + const pollingSpinner = page.getByLabel('Spinner') + await expect(pollingSpinner).toBeVisible() + await expect(page.getByText('Creating')).toBeVisible() + await expect(page.getByText('Starting')).toBeVisible() + await expect(page.getByText('Running')).toBeVisible() + await expect(pollingSpinner).toBeHidden() + + // boot disk visible, no other disks attached + await expect( + page + .getByRole('table', { name: 'Boot disk' }) + .getByRole('cell', { name: 'my-boot-disk' }) + ).toBeVisible() + await expect(page.getByText('No other disk')).toBeVisible() + // network tab works await page.getByRole('tab', { name: 'Networking' }).click() const table = page.getByRole('table', { name: 'Network interfaces' }) From 71834dc0c2c36fedc838efa50ab12a7e12e853fc Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Sep 2024 16:01:49 -0500 Subject: [PATCH 11/25] add e2e test for instance create with more disks, test disk create --- .../form/fields/DisksTableField.tsx | 6 +-- test/e2e/instance-create.e2e.ts | 42 ++++++++++++++++++ test/e2e/instance-disks.e2e.ts | 44 ++++++++++++------- 3 files changed, 71 insertions(+), 21 deletions(-) diff --git a/app/components/form/fields/DisksTableField.tsx b/app/components/form/fields/DisksTableField.tsx index f8389ee650..3df422316f 100644 --- a/app/components/form/fields/DisksTableField.tsx +++ b/app/components/form/fields/DisksTableField.tsx @@ -15,7 +15,6 @@ import { CreateDiskSideModalForm } from '~/forms/disk-create' import type { InstanceCreateInput } from '~/forms/instance-create' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' -import { FieldLabel } from '~/ui/lib/FieldLabel' import * as MiniTable from '~/ui/lib/MiniTable' import { bytesToGiB } from '~/util/units' @@ -44,9 +43,8 @@ export function DisksTableField({ return ( <>
- {/* this was empty */} {!!items.length && ( - + Name Type @@ -68,7 +66,7 @@ export function DisksTableField({ {item.type === 'attach' ? ( - '-' + '—' ) : ( <> {bytesToGiB(item.size)} diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 289f5f115d..12a59582c6 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -497,3 +497,45 @@ test('attaching additional disks allows for combobox filtering', async ({ page } await selectADisk.fill('asdf') await expect(page.getByRole('option', { name: 'No items match' })).toBeVisible() }) + +test('create instance with additional disks', async ({ page }) => { + const instanceName = 'more-disks' + await page.goto('/projects/mock-project/instances-new') + await page.getByRole('textbox', { name: 'Name', exact: true }).fill(instanceName) + await selectAProjectImage(page, 'image-1') + + // Create a new disk + await page.getByRole('button', { name: 'Create new disk' }).click() + + const createForm = page.getByRole('dialog', { name: 'Create disk' }) + await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1') + await createForm.getByRole('textbox', { name: 'Size (GiB)' }).fill('5') + await createForm.getByRole('button', { name: 'Create disk' }).click() + + const disksTable = page.getByRole('table', { name: 'Disks' }) + await expectRowVisible(disksTable, { Name: 'new-disk-1', Type: 'create', Size: '5GiB' }) + + // Attach an existing disk + await page.getByRole('button', { name: 'Attach existing disk' }).click() + await page.getByRole('button', { name: 'Disk name' }).click() + await page.getByRole('option', { name: 'disk-3' }).click() + await page.getByRole('button', { name: 'Attach disk' }).click() + + await expectRowVisible(disksTable, { Name: 'disk-3', Type: 'attach', Size: '—' }) + + // Create the instance + await page.getByRole('button', { name: 'Create instance' }).click() + + // Assert we're on the new instance's storage page + await expect(page).toHaveURL(`/projects/mock-project/instances/${instanceName}/storage`) + + // Check for the boot disk + const bootDiskTable = page.getByRole('table', { name: 'Boot disk' }) + // name is generated so it's gnarly + await expect(bootDiskTable.getByRole('cell', { name: /^more-disks-/ })).toBeVisible() + + // Check for the additional disks + const otherDisksTable = page.getByRole('table', { name: 'Other disks' }) + await expectRowVisible(otherDisksTable, { Disk: 'new-disk-1', size: '5 GiB' }) + await expectRowVisible(otherDisksTable, { Disk: 'disk-3', size: '6 GiB' }) +}) diff --git a/test/e2e/instance-disks.e2e.ts b/test/e2e/instance-disks.e2e.ts index 6fbc0f7d1b..ac5d7242fe 100644 --- a/test/e2e/instance-disks.e2e.ts +++ b/test/e2e/instance-disks.e2e.ts @@ -33,22 +33,6 @@ test('Attach disk', async ({ page }) => { // Have to stop instance to edit disks await stopInstance(page) - // New disk form - const createForm = page.getByRole('dialog', { name: 'Create disk' }) - await expect(createForm).toBeHidden() - await page.getByRole('button', { name: 'Create disk' }).click() - await expect(createForm).toBeVisible() - - await expect(createForm.getByRole('textbox', { name: 'Name' })).toBeVisible() - await expect(createForm.getByRole('textbox', { name: 'Description' })).toBeVisible() - await expect( - createForm.getByRole('radiogroup', { name: 'Block size (Bytes)' }) - ).toBeVisible() - await expect(createForm.getByRole('textbox', { name: 'Size (GiB)' })).toBeVisible() - await expect(createForm.getByRole('button', { name: 'Create disk' })).toBeVisible() - - await page.click('role=button[name="Cancel"]') - // Attach existing disk form await page.click('role=button[name="Attach existing disk"]') @@ -67,7 +51,33 @@ test('Attach disk', async ({ page }) => { await expectVisible(page, ['role=cell[name="disk-3"]']) }) -// TODO: move create form asserts to their own test and actually test the create! +test('Create disk', async ({ page }) => { + await page.goto('/projects/mock-project/instances/db1') + + const row = page.getByRole('cell', { name: 'created-disk' }) + await expect(row).toBeHidden() + + // Have to stop instance to edit disks + await stopInstance(page) + + // New disk form + const createForm = page.getByRole('dialog', { name: 'Create disk' }) + await expect(createForm).toBeHidden() + await page.getByRole('button', { name: 'Create disk' }).click() + await expect(createForm).toBeVisible() + + await createForm.getByRole('textbox', { name: 'Name' }).fill('created-disk') + await expect(createForm.getByRole('textbox', { name: 'Description' })).toBeVisible() + + await page.getByRole('radio', { name: 'Snapshot' }).click() + await page.getByRole('button', { name: 'Source snapshot' }).click() + await page.getByRole('option', { name: 'snapshot-heavy' }).click() + + await createForm.getByRole('button', { name: 'Create disk' }).click() + + const otherDisksTable = page.getByRole('table', { name: 'Other disks' }) + await expectRowVisible(otherDisksTable, { Disk: 'created-disk', size: '20 GiB' }) +}) test('Detach disk', async ({ page }) => { await page.goto('/projects/mock-project/instances/db1') From 38ea6e14be14817e9474d792872deb9d1e2890f4 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Sep 2024 16:13:53 -0500 Subject: [PATCH 12/25] move the buttons back under the tables --- .../instances/instance/tabs/StorageTab.tsx | 91 ++++++++++--------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index cf133161a4..d4f13ba462 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -260,66 +260,67 @@ export function StorageTab() { <> Boot disk -
- -
{bootDisks.length > 0 ? (
) : ( )} -
+
+ +
- + Other disks -
- setShowDiskCreate(true)} - disabledReason={ - <> - Instance must be stopped to create and - attach a disk - - } - disabled={!instanceCan.attachDisk(instance)} - > - Create disk - - -
+ {otherDisks.length > 0 ? (
) : ( )} +
+ setShowDiskCreate(true)} + disabledReason={ + <> + Instance must be stopped to create and + attach a disk + + } + disabled={!instanceCan.attachDisk(instance)} + > + Create disk + + +
+ {showDiskCreate && ( setShowDiskCreate(false)} From f493cb639c2fb2232eeaa4cfe369e1b3e61e8281 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 27 Sep 2024 14:21:29 -0500 Subject: [PATCH 13/25] take the button out, always disable detach for boot disk --- .../instances/instance/tabs/StorageTab.tsx | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index d4f13ba462..f0c45b8c2d 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -168,12 +168,14 @@ export function StorageTab() { // don't bother checking disk state: assume that if it is showing up // in this list, it can be detached label: 'Detach', - disabled: !instanceCan.detachDisk({ runState: disk.instanceState }) && ( - <> - Instance must be stopped before disk can - be detached - - ), + disabled: isBootDisk + ? 'Boot disk must be unset before it can be detached' + : !instanceCan.detachDisk({ runState: disk.instanceState }) && ( + <> + Instance must be stopped before disk + can be detached + + ), onActivate() { detachDisk({ body: { disk: disk.name }, path: { instance: instance.id } }) }, @@ -266,23 +268,8 @@ export function StorageTab() { ) : ( )} -
- -
- + Other disks From f7d98d074bce8e9cd8ae02604392faedcc1449d2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 27 Sep 2024 14:42:56 -0500 Subject: [PATCH 14/25] split frankenstein actions array into two, make e2e tests pass --- .../instances/instance/tabs/StorageTab.tsx | 175 ++++++++++-------- test/e2e/instance-disks.e2e.ts | 69 +++++-- 2 files changed, 157 insertions(+), 87 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index f0c45b8c2d..a01b92029a 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -141,80 +141,105 @@ export function StorageTab() { }, }) - const makeActions = useCallback( - // TODO: don't do this, just have two separate lists. come on - (isBootDisk: boolean) => - (disk: InstanceDisk): MenuAction[] => [ - { - label: 'Snapshot', - disabled: !diskCan.snapshot(disk) && ( - <> - Only disks in state {fancifyStates(diskCan.snapshot.states)} can be - snapshotted - - ), - onActivate() { - createSnapshot({ - query: { project }, - body: { - name: genName(disk.name), - disk: disk.name, - description: '', - }, - }) - }, - }, - { - // don't bother checking disk state: assume that if it is showing up - // in this list, it can be detached - label: 'Detach', - disabled: isBootDisk - ? 'Boot disk must be unset before it can be detached' - : !instanceCan.detachDisk({ runState: disk.instanceState }) && ( - <> - Instance must be stopped before disk - can be detached - - ), - onActivate() { - detachDisk({ body: { disk: disk.name }, path: { instance: instance.id } }) - }, - }, - { - label: isBootDisk ? 'Unset boot disk' : 'Set as boot disk', - disabled: !instanceCan.update({ runState: disk.instanceState }) && ( - <> - Instance must be stopped before boot - disk can be changed - - ), - onActivate: () => { - const verb = isBootDisk ? 'unset' : 'set' - return confirmAction({ - doAction: () => - instanceUpdate({ - path: { instance: instance.id }, - body: { bootDisk: isBootDisk ? undefined : disk.id }, - }), - errorTitle: `Could not ${verb} boot disk`, - modalTitle: `Confirm ${verb} boot disk`, - // TODO: copy + link to docs in both cases - modalContent: isBootDisk ? ( -

- Are you sure you want to unset {disk.name} as the boot disk? This - will TODO COPY RE: WHAT HAPPENS HERE -

- ) : ( -

- Are you sure you want to set {disk.name} as the boot disk? -

- ), - actionType: 'primary', - }) - }, + // shared between boot and other disks + const getSnapshotAction = useCallback( + (disk: InstanceDisk) => ({ + label: 'Snapshot', + disabled: !diskCan.snapshot(disk) && ( + <>Only disks in state {fancifyStates(diskCan.snapshot.states)} can be snapshotted + ), + onActivate() { + createSnapshot({ + query: { project }, + body: { name: genName(disk.name), disk: disk.name, description: '' }, + }) + }, + }), + [createSnapshot, project] + ) + + const makeBootDiskActions = useCallback( + (disk: InstanceDisk): MenuAction[] => [ + getSnapshotAction(disk), + { + label: 'Unset as boot disk', + disabled: !instanceCan.update({ runState: disk.instanceState }) && ( + <> + Instance must be stopped before boot disk + can be changed + + ), + onActivate: () => + confirmAction({ + doAction: () => + instanceUpdate({ + path: { instance: instance.id }, + body: { bootDisk: undefined }, + }), + errorTitle: 'Could not unset boot disk', + modalTitle: 'Confirm unset boot disk', + // TODO: copy + link to docs + modalContent: ( +

+ Are you sure you want to unset {disk.name} as the boot disk? This + will TODO COPY RE: WHAT HAPPENS HERE +

+ ), + actionType: 'primary', + }), + }, + { + label: 'Detach', + disabled: 'Boot disk must be unset before it can be detached', + onActivate() {}, // it's always disabled, so noop is ok + }, + ], + [instanceUpdate, instance.id, getSnapshotAction] + ) + + const makeOtherDiskActions = useCallback( + (disk: InstanceDisk): MenuAction[] => [ + getSnapshotAction(disk), + { + label: 'Set as boot disk', + disabled: !instanceCan.update({ runState: disk.instanceState }) && ( + <> + Instance must be stopped before boot disk + can be changed + + ), + onActivate: () => + confirmAction({ + doAction: () => + instanceUpdate({ + path: { instance: instance.id }, + body: { bootDisk: disk.id }, + }), + errorTitle: 'Could not set boot disk', + modalTitle: 'Confirm set boot disk', + // TODO: copy + link to docs + modalContent: ( +

+ Are you sure you want to set {disk.name} as the boot disk? +

+ ), + actionType: 'primary', + }), + }, + { + label: 'Detach', + disabled: !instanceCan.detachDisk({ runState: disk.instanceState }) && ( + <> + Instance must be stopped before disk can + be detached + + ), + onActivate() { + detachDisk({ body: { disk: disk.name }, path: { instance: instance.id } }) }, - ], - [detachDisk, createSnapshot, project, instanceUpdate, instance.id] + }, + ], + [detachDisk, instanceUpdate, instance.id, getSnapshotAction] ) const attachDisk = useApiMutation('instanceDiskAttach', { @@ -241,7 +266,7 @@ export function StorageTab() { ) const bootDisksTable = useReactTable({ - columns: useColsWithActions(staticCols, makeActions(true)), + columns: useColsWithActions(staticCols, makeBootDiskActions), data: useMemo( () => bootDisks.map((disk) => ({ ...disk, instanceState: instance.runState })), [bootDisks, instance.runState] @@ -250,7 +275,7 @@ export function StorageTab() { }) const otherDisksTable = useReactTable({ - columns: useColsWithActions(staticCols, makeActions(false)), + columns: useColsWithActions(staticCols, makeOtherDiskActions), data: useMemo( () => otherDisks.map((disk) => ({ ...disk, instanceState: instance.runState })), [otherDisks, instance.runState] diff --git a/test/e2e/instance-disks.e2e.ts b/test/e2e/instance-disks.e2e.ts index ac5d7242fe..62928b5c9e 100644 --- a/test/e2e/instance-disks.e2e.ts +++ b/test/e2e/instance-disks.e2e.ts @@ -15,21 +15,66 @@ import { test, } from './utils' -test('Attach disk', async ({ page }) => { +test('Disabled actions', async ({ page }) => { await page.goto('/projects/mock-project/instances/db1') - const row = page.getByRole('row', { name: 'disk-1', exact: false }) - await expect(row).toBeVisible() + const bootDiskRow = page.getByRole('row', { name: 'disk-1', exact: false }) + await expect(bootDiskRow).toBeVisible() + + // Both buttons disabled, also test fancy tooltip + + await bootDiskRow.getByRole('button', { name: 'Row actions' }).click() - // can't detach, also test fancy construction of disabled tooltip - await row.getByRole('button', { name: 'Row actions' }).click() - await expect(page.getByRole('menuitem', { name: 'Detach' })).toBeDisabled() - await page.getByRole('menuitem', { name: 'Detach' }).hover() + const detachButton = page.getByRole('menuitem', { name: 'Detach' }) + const unsetButton = page.getByRole('menuitem', { name: 'Unset' }) + + await expect(unsetButton).toBeDisabled() + await page.getByRole('menuitem', { name: 'Unset' }).hover() + await expect( + page.getByText('Instance must be stopped before boot disk can be changed') + ).toBeVisible() + + await expect(detachButton).toBeDisabled() + await detachButton.hover() + await expect( + page.getByText('Boot disk must be unset before it can be detached') + ).toBeVisible() + await page.keyboard.press('Escape') // close menu + + const otherDiskRow = page.getByRole('row', { name: 'disk-2', exact: false }) + await expect(otherDiskRow).toBeVisible() + + await otherDiskRow.getByRole('button', { name: 'Row actions' }).click() + + const setButton = page.getByRole('menuitem', { name: 'Set as boot disk' }) + await expect(setButton).toBeDisabled() + await setButton.hover() + await expect( + page.getByText('Instance must be stopped before boot disk can be changed') + ).toBeVisible() + + await expect(detachButton).toBeDisabled() + await detachButton.hover() await expect( page.getByText('Instance must be stopped before disk can be detached') ).toBeVisible() await page.keyboard.press('Escape') // close menu + // confirm they become enabled when the instance is stopped + await stopInstance(page) + + await bootDiskRow.getByRole('button', { name: 'Row actions' }).click() + await expect(unsetButton).toBeEnabled() + await expect(detachButton).toBeDisabled() // detach is always disabled on boot disk + + await otherDiskRow.getByRole('button', { name: 'Row actions' }).click() + await expect(setButton).toBeEnabled() + await expect(detachButton).toBeEnabled() +}) + +test('Attach disk', async ({ page }) => { + await page.goto('/projects/mock-project/instances/db1') + // Have to stop instance to edit disks await stopInstance(page) @@ -86,11 +131,11 @@ test('Detach disk', async ({ page }) => { await stopInstance(page) const successMsg = page.getByText('Disk detached').nth(0) - const row = page.getByRole('row', { name: 'disk-1' }) + const row = page.getByRole('row', { name: 'disk-2' }) await expect(row).toBeVisible() await expect(successMsg).toBeHidden() - await clickRowAction(page, 'disk-1', 'Detach') + await clickRowAction(page, 'disk-2', 'Detach') await expect(successMsg).toBeVisible() await expect(row).toBeHidden() // disk row goes away }) @@ -102,7 +147,7 @@ test('Snapshot disk', async ({ page }) => { const successMsg = page.getByText('Snapshot created').nth(0) await expect(successMsg).toBeHidden() - await clickRowAction(page, 'disk-2', 'Snapshot') + await clickRowAction(page, 'disk-1', 'Snapshot') await expect(successMsg).toBeVisible() // we see the toast! @@ -111,8 +156,8 @@ test('Snapshot disk', async ({ page }) => { await page.getByRole('button', { name: 'next' }).click() const table = page.getByRole('table') await expectRowVisible(table, { - name: expect.stringMatching(/^disk-2-/), - disk: 'disk-2', + name: expect.stringMatching(/^disk-1-/), + disk: 'disk-1', }) }) From 512f05ff8b414ea064503d103a70bd6a9a2a1a71 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 30 Sep 2024 23:56:01 -0500 Subject: [PATCH 15/25] update omicron to latest commit on PR, fix breakage thereby introduced --- OMICRON_VERSION | 2 +- app/api/__generated__/Api.ts | 49 ++++++++++++++++++++++----- app/api/__generated__/OMICRON_VERSION | 2 +- app/api/__generated__/validate.ts | 16 +++++++-- app/forms/instance-create.tsx | 2 +- mock-api/instance.ts | 19 ++++++----- mock-api/msw/handlers.ts | 26 ++++++++------ 7 files changed, 83 insertions(+), 33 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 1cb2132dcf..491ddfc235 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -1537af9925bb9c2fa931b58d08be82a684bfc12a +899f781136e62361f6c3067ff105884936124b50 diff --git a/app/api/__generated__/Api.ts b/app/api/__generated__/Api.ts index 2d3fac3237..664c62dc54 100644 --- a/app/api/__generated__/Api.ts +++ b/app/api/__generated__/Api.ts @@ -1755,6 +1755,12 @@ export type InstanceState = * View of an Instance */ export type Instance = { + /** The time at which the auto-restart cooldown period for this instance completes, permitting it to be automatically restarted again. If the instance enters the `Failed` state, it will not be restarted until after this time. + +If this is not present, then either the instance has never been automatically restarted, or the cooldown period has already expired, allowing the instance to be restarted immediately if it fails. */ + autoRestartCooldownExpiration?: Date + /** `true` if this instance's auto-restart policy will permit the control plane to automatically restart it if it enters the `Failed` state. */ + autoRestartEnabled: boolean /** the ID of the disk used to boot this Instance, if a specific one is assigned. */ bootDiskId?: string /** human-readable free-form text about a resource */ @@ -1774,11 +1780,25 @@ export type Instance = { runState: InstanceState /** timestamp when this resource was created */ timeCreated: Date + /** The timestamp of the most recent time this instance was automatically restarted by the control plane. + +If this is not present, then this instance has not been automatically restarted. */ + timeLastAutoRestarted?: Date /** timestamp when this resource was last modified */ timeModified: Date timeRunStateUpdated: Date } +/** + * A policy determining when an instance should be automatically restarted by the control plane. + */ +export type InstanceAutoRestartPolicy = + /** The instance should not be automatically restarted by the control plane if it fails. */ + | 'never' + + /** If this instance is running and unexpectedly fails (e.g. due to a host software crash or unexpected host reboot), the control plane will make a best-effort attempt to restart it. The control plane may choose not to restart the instance to preserve the overall availability of the system. */ + | 'best_effort' + /** * Describe the instance's disks at creation time */ @@ -1831,8 +1851,16 @@ If more than one interface is provided, then the first will be designated the pr * Create-time parameters for an `Instance` */ export type InstanceCreate = { - /** Choice of which disk this instance should boot from. */ - bootDisk?: NameOrId + /** The auto-restart policy for this instance. + +This indicates whether the instance should be automatically restarted by the control plane on failure. If this is `null`, no auto-restart policy has been configured for this instance by the user. */ + autoRestartPolicy?: InstanceAutoRestartPolicy + /** The disk this instance should boot into. This disk can either be attached if it already exists, or created, if it should be a new disk. + +It is strongly recommended to either provide a boot disk at instance creation, or update the instance after creation to set a boot disk. + +An instance without an explicit boot disk can be booted: the options are as managed by UEFI, and as controlled by the guest OS, but with some risk. If this instance later has a disk attached or detached, it is possible that boot options can end up reordered, with the intended boot disk moved after the EFI shell in boot priority. This may result in an instance that only boots to the EFI shell until the desired disk is set as an explicit boot disk and the instance rebooted. */ + bootDisk?: InstanceDiskAttachment description: string /** The disks to be created or attached for this instance. */ disks?: InstanceDiskAttachment[] @@ -1944,7 +1972,12 @@ export type InstanceSerialConsoleData = { /** * Parameters of an `Instance` that can be reconfigured after creation. */ -export type InstanceUpdate = { bootDisk?: NameOrId } +export type InstanceUpdate = { + /** Name or ID of the disk the instance should be instructed to boot from. + +If not provided, unset the instance's boot disk. */ + bootDisk?: NameOrId +} /** * A collection of IP ranges. If a pool is linked to a silo, IP addresses from the pool can be allocated within that silo @@ -2588,18 +2621,18 @@ export type RouteConfig = { } /** - * A `RouteDestination` is used to match traffic with a routing rule, on the destination of that traffic. + * A `RouteDestination` is used to match traffic with a routing rule based on the destination of that traffic. * * When traffic is to be sent to a destination that is within a given `RouteDestination`, the corresponding `RouterRoute` applies, and traffic will be forward to the `RouteTarget` for that rule. */ export type RouteDestination = - /** Route applies to traffic destined for a specific IP address */ + /** Route applies to traffic destined for the specified IP address */ | { type: 'ip'; value: string } - /** Route applies to traffic destined for a specific IP subnet */ + /** Route applies to traffic destined for the specified IP subnet */ | { type: 'ip_net'; value: IpNet } - /** Route applies to traffic destined for the given VPC. */ + /** Route applies to traffic destined for the specified VPC */ | { type: 'vpc'; value: Name } - /** Route applies to traffic */ + /** Route applies to traffic destined for the specified VPC subnet */ | { type: 'subnet'; value: Name } /** diff --git a/app/api/__generated__/OMICRON_VERSION b/app/api/__generated__/OMICRON_VERSION index 01a69b73a0..19fd419ac4 100644 --- a/app/api/__generated__/OMICRON_VERSION +++ b/app/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -1537af9925bb9c2fa931b58d08be82a684bfc12a +899f781136e62361f6c3067ff105884936124b50 diff --git a/app/api/__generated__/validate.ts b/app/api/__generated__/validate.ts index 9bac9f6d9c..77402ef0df 100644 --- a/app/api/__generated__/validate.ts +++ b/app/api/__generated__/validate.ts @@ -1680,6 +1680,8 @@ export const InstanceState = z.preprocess( export const Instance = z.preprocess( processResponseBody, z.object({ + autoRestartCooldownExpiration: z.coerce.date().optional(), + autoRestartEnabled: SafeBoolean, bootDiskId: z.string().uuid().optional(), description: z.string(), hostname: z.string(), @@ -1690,11 +1692,20 @@ export const Instance = z.preprocess( projectId: z.string().uuid(), runState: InstanceState, timeCreated: z.coerce.date(), + timeLastAutoRestarted: z.coerce.date().optional(), timeModified: z.coerce.date(), timeRunStateUpdated: z.coerce.date(), }) ) +/** + * A policy determining when an instance should be automatically restarted by the control plane. + */ +export const InstanceAutoRestartPolicy = z.preprocess( + processResponseBody, + z.enum(['never', 'best_effort']) +) + /** * Describe the instance's disks at creation time */ @@ -1744,7 +1755,8 @@ export const InstanceNetworkInterfaceAttachment = z.preprocess( export const InstanceCreate = z.preprocess( processResponseBody, z.object({ - bootDisk: NameOrId.default(null).optional(), + autoRestartPolicy: InstanceAutoRestartPolicy.default(null).optional(), + bootDisk: InstanceDiskAttachment.default(null).optional(), description: z.string(), disks: InstanceDiskAttachment.array().default([]).optional(), externalIps: ExternalIpCreate.array().default([]).optional(), @@ -2485,7 +2497,7 @@ export const RouteConfig = z.preprocess( ) /** - * A `RouteDestination` is used to match traffic with a routing rule, on the destination of that traffic. + * A `RouteDestination` is used to match traffic with a routing rule based on the destination of that traffic. * * When traffic is to be sent to a destination that is within a given `RouteDestination`, the corresponding `RouterRoute` applies, and traffic will be forward to the `RouteTarget` for that rule. */ diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index ebfc62a12c..353902e57f 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -318,7 +318,7 @@ export function CreateInstanceForm() { memory: instance.memory * GiB, ncpus: instance.ncpus, disks: [bootDisk, ...values.disks], - bootDisk: bootDisk.name, + bootDisk, externalIps: values.externalIps, start: values.start, networkInterfaces: values.networkInterfaces, diff --git a/mock-api/instance.ts b/mock-api/instance.ts index 63ced52f5f..c274e1f16c 100644 --- a/mock-api/instance.ts +++ b/mock-api/instance.ts @@ -12,7 +12,15 @@ import { GiB } from '~/util/units' import type { Json } from './json-type' import { project } from './project' +const base = { + time_created: new Date().toISOString(), + time_modified: new Date().toISOString(), + time_run_state_updated: new Date().toISOString(), + auto_restart_enabled: true, +} + export const instance: Json = { + ...base, id: '935499b3-fd96-432a-9c21-83a3dc1eece4', name: 'db1', ncpus: 2, @@ -22,12 +30,10 @@ export const instance: Json = { project_id: project.id, run_state: 'running', boot_disk_id: '7f2309a5-13e3-47e0-8a4c-2a3b3bc992fd', // disk-1 - time_created: new Date().toISOString(), - time_modified: new Date().toISOString(), - time_run_state_updated: new Date().toISOString(), } const failedInstance: Json = { + ...base, id: 'b5946edc-5bed-4597-88ab-9a8beb9d32a4', name: 'you-fail', ncpus: 4, @@ -36,12 +42,10 @@ const failedInstance: Json = { hostname: 'oxide.com', project_id: project.id, run_state: 'failed', - time_created: new Date().toISOString(), - time_modified: new Date().toISOString(), - time_run_state_updated: new Date().toISOString(), } const startingInstance: Json = { + ...base, id: '16737f54-1f76-4c96-8b7c-9d24971c1d62', name: 'not-there-yet', ncpus: 2, @@ -50,9 +54,6 @@ const startingInstance: Json = { hostname: 'oxide.com', project_id: project.id, run_state: 'starting', - time_created: new Date().toISOString(), - time_modified: new Date().toISOString(), - time_run_state_updated: new Date().toISOString(), } export const instances: Json[] = [instance, failedInstance, startingInstance] diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 0e1f925a4a..0b2f94a136 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -442,16 +442,19 @@ export const handlers = makeHandlers({ // Validate boot disk against list of disks to create/attach. This must // happen before disks are created/attached because it can throw. - if (body.boot_disk) { + const bootDisk = body.boot_disk?.name + if (bootDisk) { // if it's an ID, things are annoying - if (isUuid(body.boot_disk)) { + + // Technically, this can't be an ID right now, but I did all this work to + // figure out how to handle IDs before that was changed API side, and I + // expect this to become relevant again soon. + if (isUuid(bootDisk)) { // it must already exist - const disk = lookup.disk({ disk: body.boot_disk }) + const disk = lookup.disk({ disk: bootDisk }) // it must be in this project if (disk.project_id !== project.id) { - throw notFoundErr( - `boot disk in project '${project.name}' with ID '${body.boot_disk}'` - ) + throw notFoundErr(`boot disk in project '${project.name}' with ID '${bootDisk}'`) } // it must be detached because we're going to attach it if (disk.state.state !== 'detached') { @@ -468,9 +471,9 @@ export const handlers = makeHandlers({ } } else { // otherwise it's a name, and we have to make sure it's somewhere in the list - const inListOfDisks = (body.disks || []).some((d) => body.boot_disk === d.name) + const inListOfDisks = (body.disks || []).some((d) => bootDisk === d.name) if (!inListOfDisks) { - throw `Specified boot disk '${body.boot_disk}' not found in list of disks` + throw `Specified boot disk '${bootDisk}' not found in list of disks` } } } @@ -542,12 +545,13 @@ export const handlers = makeHandlers({ time_run_state_updated: new Date().toISOString(), // Note this relies on the disk already existing. This would be risky // without the ton of validation before we do anything with disks. - boot_disk_id: body.boot_disk + boot_disk_id: bootDisk ? lookup.disk({ - disk: body.boot_disk, - project: isUuid(body.boot_disk) ? undefined : project.id, + disk: bootDisk, + project: isUuid(bootDisk) ? undefined : project.id, }).id : undefined, + auto_restart_enabled: true, } body.external_ips?.forEach((ip) => { From 6c393ea23e4617320a431ec2b927690f1b113cf4 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 09:48:25 -0500 Subject: [PATCH 16/25] mock update errors out if boot disk is not attached --- mock-api/msw/handlers.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 0b2f94a136..9f0e9f075c 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -600,6 +600,13 @@ export const handlers = makeHandlers({ project: isUuid(body.boot_disk) ? undefined : query.project, }) + // disk must of course be attached to this instance already to be set as the boot disk + // TODO: update with better link once PR is merged + // https://github.com/oxidecomputer/omicron/pull/6585/files#diff-a234811eb5ee38fdcf626cd6514deb7dbfc8b9e2067cfa3b4a6f111d72a07e53R1079-R1081 + if (!(disk.state.state === 'attached' && disk.state.instance === instance.id)) { + throw 'Boot disk must be attached' + } + instance.boot_disk_id = disk.id } else { // we're clearing the boot disk! From 393ca9c86a07f0e8b4a8d6db4a71b98c354ccf79 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 12:34:41 -0500 Subject: [PATCH 17/25] update to a commit on omicron main --- OMICRON_VERSION | 2 +- app/api/__generated__/OMICRON_VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 491ddfc235..873025fe99 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -899f781136e62361f6c3067ff105884936124b50 +144e91ae570e503a7e3f470efbcbba268afa4ae0 diff --git a/app/api/__generated__/OMICRON_VERSION b/app/api/__generated__/OMICRON_VERSION index 19fd419ac4..750c7737ae 100644 --- a/app/api/__generated__/OMICRON_VERSION +++ b/app/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -899f781136e62361f6c3067ff105884936124b50 +144e91ae570e503a7e3f470efbcbba268afa4ae0 From 9f9b982a5347f3ab829d93e9e434caa50136205d Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 15:05:35 -0500 Subject: [PATCH 18/25] cite omicron source for logic about when you can set a boot disk --- app/api/util.ts | 12 +++++++----- mock-api/msw/handlers.ts | 14 +++++++++----- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/app/api/util.ts b/app/api/util.ts index 08a24c5e0e..52bb61eb54 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -90,14 +90,17 @@ export const genName = (...parts: [string, ...string[]]) => { } const instanceActions: Record = { + // NoVmm maps to to Stopped: + // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-model/src/instance_state.rs#L55 + // https://github.com/oxidecomputer/omicron/blob/0496637/nexus/src/app/instance.rs#L2064 start: ['stopped', 'failed'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/instance.rs#L865 delete: ['stopped', 'failed'], - // TODO: whence my licence to make such a claim - update: ['stopped'], + // https://github.com/oxidecomputer/omicron/blob/3093818/nexus/db-queries/src/db/datastore/instance.rs#L1030-L1043 + update: ['stopped', 'failed', 'creating'], // reboot and stop are kind of weird! // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/instance.rs#L790-L798 @@ -106,9 +109,6 @@ const instanceActions: Record = { reboot: ['running'], // technically rebooting allowed but too weird to say it stop: ['running', 'starting', 'rebooting'], - // NoVmm maps to to Stopped: - // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-model/src/instance_state.rs#L55 - // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L323-L327 detachDisk: ['creating', 'stopped', 'failed'], // only Creating and NoVmm @@ -151,6 +151,8 @@ const diskActions: Record = { attach: ['creating', 'detached'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L313-L314 detach: ['attached'], + // https://github.com/oxidecomputer/omicron/blob/3093818/nexus/db-queries/src/db/datastore/instance.rs#L1077-L1081 + setAsBootDisk: ['attached'], } export const diskCan = R.mapValues(diskActions, (states) => { diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 9f0e9f075c..4e7a98836e 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -592,6 +592,11 @@ export const handlers = makeHandlers({ instanceUpdate({ path, query, body }) { const instance = lookup.instance({ ...path, ...query }) + if (!instanceCan.update({ runState: instance.run_state })) { + const states = instanceCan.update.states + throw `Instance can only be updated if ${commaSeries(states, 'or')}` + } + if (body.boot_disk) { // Only include project if it's a name, otherwise lookup will error. // This will 404 if the disk doesn't exist, which I think is right. @@ -600,11 +605,10 @@ export const handlers = makeHandlers({ project: isUuid(body.boot_disk) ? undefined : query.project, }) - // disk must of course be attached to this instance already to be set as the boot disk - // TODO: update with better link once PR is merged - // https://github.com/oxidecomputer/omicron/pull/6585/files#diff-a234811eb5ee38fdcf626cd6514deb7dbfc8b9e2067cfa3b4a6f111d72a07e53R1079-R1081 - if (!(disk.state.state === 'attached' && disk.state.instance === instance.id)) { - throw 'Boot disk must be attached' + const isAttached = + disk.state.state === 'attached' && disk.state.instance === instance.id + if (!(diskCan.setAsBootDisk(disk) && isAttached)) { + throw 'Boot disk must be attached to the instance' } instance.boot_disk_id = disk.id From 541f95e661ee0cd7671c19852ff00c69928bba47 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 16:03:49 -0500 Subject: [PATCH 19/25] test for changing boot disk --- test/e2e/instance-disks.e2e.ts | 57 +++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/test/e2e/instance-disks.e2e.ts b/test/e2e/instance-disks.e2e.ts index 62928b5c9e..4220ac7a45 100644 --- a/test/e2e/instance-disks.e2e.ts +++ b/test/e2e/instance-disks.e2e.ts @@ -161,4 +161,59 @@ test('Snapshot disk', async ({ page }) => { }) }) -// TODO: tests for different combinations of boot and other disks +test('Change boot disk', async ({ page }) => { + await page.goto('/projects/mock-project/instances/db1') + + // assert disk-1 is boot disk, disk-2 also there + const bootDiskTable = page.getByRole('table', { name: 'Boot disk' }) + const otherDisksTable = page.getByRole('table', { name: 'Other disks' }) + const confirm = page.getByRole('button', { name: 'Confirm' }) + const noBootDisk = page.getByText('No boot disk set') + const noOtherDisks = page.getByText('No other disks') + + const disk1 = { Disk: 'disk-1', size: '2 GiB' } + const disk2 = { Disk: 'disk-2', size: '4 GiB' } + + await expectRowVisible(bootDiskTable, disk1) + await expectRowVisible(otherDisksTable, disk2) + + await stopInstance(page) + + // Set disk-2 as boot disk + await clickRowAction(page, 'disk-2', 'Set as boot disk') + await confirm.click() + + await expectRowVisible(bootDiskTable, disk2) + await expectRowVisible(otherDisksTable, disk1) + + // Unset boot disk + await expect(noBootDisk).toBeHidden() + + await clickRowAction(page, 'disk-2', 'Unset as boot disk') + await confirm.click() + + await expect(noBootDisk).toBeVisible() + await expectRowVisible(otherDisksTable, disk1) + await expectRowVisible(otherDisksTable, disk2) + + // set disk-1 back as boot disk + await clickRowAction(page, 'disk-1', 'Set as boot disk') + await confirm.click() + + await expect(noBootDisk).toBeHidden() + await expect(noOtherDisks).toBeHidden() + + // detach disk-2 + await clickRowAction(page, 'disk-2', 'Detach') + await expect(noOtherDisks).toBeVisible() + + // Remove disk-1 altogether, no disks left + await clickRowAction(page, 'disk-1', 'Unset as boot disk') + await confirm.click() + + await expectRowVisible(otherDisksTable, disk1) + + await clickRowAction(page, 'disk-1', 'Detach') + await expect(noBootDisk).toBeVisible() + await expect(noOtherDisks).toBeVisible() +}) From 4536d50e7f481639f1eb2834a644a704c004e5b7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 16:06:42 -0500 Subject: [PATCH 20/25] ixi's feedback, clean up mock API logic --- app/forms/instance-create.tsx | 2 +- mock-api/msw/handlers.ts | 64 +++++++++-------------------------- 2 files changed, 17 insertions(+), 49 deletions(-) diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 353902e57f..7d3f35a1c2 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -317,7 +317,7 @@ export function CreateInstanceForm() { description: values.description, memory: instance.memory * GiB, ncpus: instance.ncpus, - disks: [bootDisk, ...values.disks], + disks: values.disks, bootDisk, externalIps: values.externalIps, start: values.start, diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 4e7a98836e..72d782b4bc 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -417,12 +417,16 @@ export const handlers = makeHandlers({ * Eagerly check for disk errors. Execution will stop early and prevent orphaned disks from * being created if there's a failure. In omicron this is done automatically via an undo on the saga. */ - for (const diskParams of body.disks || []) { + const otherDisks = body.disks || [] + const allDisks = body.boot_disk ? [body.boot_disk, ...otherDisks] : otherDisks + for (const diskParams of allDisks) { if (diskParams.type === 'create') { errIfExists(db.disks, { name: diskParams.name, project_id: project.id }, 'disk') errIfInvalidDiskSize(diskParams) } else { - lookup.disk({ ...query, disk: diskParams.name }) + const disk = lookup.disk({ ...query, disk: diskParams.name }) + if (disk.state.state !== 'detached') + throw `Disk '${diskParams.name}' is already attached to an instance` } } @@ -440,45 +444,11 @@ export const handlers = makeHandlers({ }) } - // Validate boot disk against list of disks to create/attach. This must - // happen before disks are created/attached because it can throw. - const bootDisk = body.boot_disk?.name - if (bootDisk) { - // if it's an ID, things are annoying - - // Technically, this can't be an ID right now, but I did all this work to - // figure out how to handle IDs before that was changed API side, and I - // expect this to become relevant again soon. - if (isUuid(bootDisk)) { - // it must already exist - const disk = lookup.disk({ disk: bootDisk }) - // it must be in this project - if (disk.project_id !== project.id) { - throw notFoundErr(`boot disk in project '${project.name}' with ID '${bootDisk}'`) - } - // it must be detached because we're going to attach it - if (disk.state.state !== 'detached') { - throw "When specified boot disk already exists, it must be in state 'detached'" - } - // it must be in the list of disks to attach! disks are attached by name (currently, - // there is an issue to make it NameOrId) so we use the name to make sure it's in the list - const inListOfDisksToAttach = (body.disks || []).some((d) => { - if (d.type === 'create') return false - return disk.name === d.name - }) - if (!inListOfDisksToAttach) { - throw 'Specified boot disk must be in the list of disks to attach' - } - } else { - // otherwise it's a name, and we have to make sure it's somewhere in the list - const inListOfDisks = (body.disks || []).some((d) => bootDisk === d.name) - if (!inListOfDisks) { - throw `Specified boot disk '${bootDisk}' not found in list of disks` - } - } - } + ///////////////////////////////////////////////////////////// + // DB write stuff starts here + ///////////////////////////////////////////////////////////// - for (const diskParams of body.disks || []) { + for (const diskParams of allDisks) { if (diskParams.type === 'create') { const { size, name, description, disk_source } = diskParams const newDisk: Json = { @@ -499,6 +469,11 @@ export const handlers = makeHandlers({ } } + // at this point, the boot disk has been created, so just retrieve it again + const bootDiskId = body.boot_disk?.name + ? lookup.disk({ disk: body.boot_disk.name, project: project.id }).id + : undefined + // just use the first VPC in the project and first subnet in the VPC. bit of // a hack but not very important const anyVpc = db.vpcs.find((v) => v.project_id === project.id) @@ -543,14 +518,7 @@ export const handlers = makeHandlers({ ...getTimestamps(), run_state: 'creating', time_run_state_updated: new Date().toISOString(), - // Note this relies on the disk already existing. This would be risky - // without the ton of validation before we do anything with disks. - boot_disk_id: bootDisk - ? lookup.disk({ - disk: bootDisk, - project: isUuid(bootDisk) ? undefined : project.id, - }).id - : undefined, + boot_disk_id: bootDiskId, auto_restart_enabled: true, } From 2f41bafe04f78605f6a89c3683fb7b280f4ecf0b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 17:26:17 -0500 Subject: [PATCH 21/25] moderately deranged boot disk empty state copy --- .../instances/instance/tabs/StorageTab.tsx | 78 +++++++++++++------ app/ui/lib/EmptyMessage.tsx | 12 +-- app/util/links.ts | 2 + 3 files changed, 64 insertions(+), 28 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index a01b92029a..a879189242 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -35,32 +35,12 @@ import { Columns } from '~/table/columns/common' import { Table } from '~/table/Table' import { Button } from '~/ui/lib/Button' import { CreateButton } from '~/ui/lib/CreateButton' -import { EmptyMessage } from '~/ui/lib/EmptyMessage' +import { EMBody, EmptyMessage } from '~/ui/lib/EmptyMessage' import { TableControls, TableEmptyBox, TableTitle } from '~/ui/lib/Table' +import { links } from '~/util/links' import { fancifyStates } from './common' -const BootDiskEmptyState = () => ( - - } - title="No boot disk set" - // TODO: boot order docs link - body="Read about boot order LINK HERE" - /> - -) - -const OtherDisksEmptyState = () => ( - - } - title="No other disks" - body="Attach a disk to see it here" - /> - -) - StorageTab.loader = async ({ params }: LoaderFunctionArgs) => { const { project, instance } = getInstanceSelector(params) const selector = { path: { instance }, query: { project } } @@ -291,7 +271,7 @@ export function StorageTab() { {bootDisks.length > 0 ? (
) : ( - + )} @@ -356,3 +336,55 @@ export function StorageTab() { ) } + +function BootDiskEmptyState({ otherDisks }: { otherDisks: Disk[] }) { + return ( + + } + title="No boot disk set" + body={ + <> + {otherDisks.length > 1 ? ( + + With multiple disks attached and no boot disk, the instance should boot from + the first disk unless this is overridden by the guest. + + ) : otherDisks.length === 1 ? ( + + Instance will boot from {otherDisks[0].name} because it is the only + disk. + + ) : ( + Attach a disk to be able to set a boot disk. + )} + + See the{' '} + + Instances + {' '} + guide to learn about boot order. + + + } + /> + + ) +} + +function OtherDisksEmptyState() { + return ( + + } + title="No other disks" + body="Attach a disk to see it here" + /> + + ) +} diff --git a/app/ui/lib/EmptyMessage.tsx b/app/ui/lib/EmptyMessage.tsx index f22371dfb5..c603b81ea9 100644 --- a/app/ui/lib/EmptyMessage.tsx +++ b/app/ui/lib/EmptyMessage.tsx @@ -6,9 +6,11 @@ * Copyright Oxide Computer Company */ import cn from 'classnames' -import type { ReactElement } from 'react' +import type { ReactElement, ReactNode } from 'react' import { Link } from 'react-router-dom' +import { classed } from '~/util/classed' + import { Button, buttonStyle } from './Button' const buttonStyleProps = { variant: 'ghost', size: 'sm', color: 'secondary' } as const @@ -16,7 +18,7 @@ const buttonStyleProps = { variant: 'ghost', size: 'sm', color: 'secondary' } as type Props = { icon?: ReactElement title: string - body?: string + body?: ReactNode } & ( // only require buttonTo or onClick if buttonText is present | { buttonText: string; buttonTo: string } | { buttonText: string; onClick: () => void } @@ -46,10 +48,10 @@ export function EmptyMessage(props: Props) { )}

{props.title}

- {props.body && ( -

{props.body}

- )} + {typeof props.body === 'string' ? {props.body} : props.body} {button} ) } + +export const EMBody = classed.p`mt-1 text-balance text-sans-md text-secondary` diff --git a/app/util/links.ts b/app/util/links.ts index 65e24673a2..48c88d5eed 100644 --- a/app/util/links.ts +++ b/app/util/links.ts @@ -20,6 +20,8 @@ export const links = { preparingImagesDocs: 'https://docs.oxide.computer/guides/creating-and-sharing-images#_preparing_images_for_import', instanceActionsDocs: 'https://docs.oxide.computer/guides/managing-instances', + // TODO: link to section + instanceBootDiskDocs: 'https://docs.oxide.computer/guides/deploying-workloads', keyConceptsIamPolicyDocs: 'https://docs.oxide.computer/guides/key-entities-and-concepts#iam-policy', keyConceptsProjectsDocs: From d73bba56dbc6aa86129672b0250427a3998d6c3e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 23:11:34 -0500 Subject: [PATCH 22/25] tweak empty state copy and assert about it in e2e test --- .../project/instances/instance/tabs/StorageTab.tsx | 8 ++++---- test/e2e/instance-disks.e2e.ts | 13 +++++++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index a879189242..0e17e94b2d 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -347,8 +347,8 @@ function BootDiskEmptyState({ otherDisks }: { otherDisks: Disk[] }) { <> {otherDisks.length > 1 ? ( - With multiple disks attached and no boot disk, the instance should boot from - the first disk unless this is overridden by the guest. + Setting a boot disk is recommended unless you intend to manage boot order + within the instance. ) : otherDisks.length === 1 ? ( @@ -359,7 +359,7 @@ function BootDiskEmptyState({ otherDisks }: { otherDisks: Disk[] }) { Attach a disk to be able to set a boot disk. )} - See the{' '} + Learn more in the{' '} Instances {' '} - guide to learn about boot order. + guide. } diff --git a/test/e2e/instance-disks.e2e.ts b/test/e2e/instance-disks.e2e.ts index 4220ac7a45..c0c0cfeafa 100644 --- a/test/e2e/instance-disks.e2e.ts +++ b/test/e2e/instance-disks.e2e.ts @@ -196,15 +196,18 @@ test('Change boot disk', async ({ page }) => { await expectRowVisible(otherDisksTable, disk1) await expectRowVisible(otherDisksTable, disk2) + await expect(page.getByText('Setting a boot disk is recommended')).toBeVisible() + + // detach disk so there's only one + await clickRowAction(page, 'disk-2', 'Detach') + + await expect(page.getByText('Instance will boot from disk-1')).toBeVisible() + // set disk-1 back as boot disk await clickRowAction(page, 'disk-1', 'Set as boot disk') await confirm.click() await expect(noBootDisk).toBeHidden() - await expect(noOtherDisks).toBeHidden() - - // detach disk-2 - await clickRowAction(page, 'disk-2', 'Detach') await expect(noOtherDisks).toBeVisible() // Remove disk-1 altogether, no disks left @@ -216,4 +219,6 @@ test('Change boot disk', async ({ page }) => { await clickRowAction(page, 'disk-1', 'Detach') await expect(noBootDisk).toBeVisible() await expect(noOtherDisks).toBeVisible() + + await expect(page.getByText('Attach a disk to be able to set a boot disk')).toBeVisible() }) From 17e9a3797f76aacfb4751d7920c96b8c37d6f30c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 2 Oct 2024 12:37:47 -0500 Subject: [PATCH 23/25] clean up copy TODOs in confirm modals --- .../instances/instance/tabs/StorageTab.tsx | 57 ++++++++++++------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/app/pages/project/instances/instance/tabs/StorageTab.tsx b/app/pages/project/instances/instance/tabs/StorageTab.tsx index 0e17e94b2d..1083b509e5 100644 --- a/app/pages/project/instances/instance/tabs/StorageTab.tsx +++ b/app/pages/project/instances/instance/tabs/StorageTab.tsx @@ -138,6 +138,13 @@ export function StorageTab() { [createSnapshot, project] ) + const { data: disks } = usePrefetchedApiQuery('instanceDiskList', instancePathQuery) + + const [bootDisks, otherDisks] = useMemo( + () => R.partition(disks.items, (d) => d.id === instance.bootDiskId), + [disks.items, instance.bootDiskId] + ) + const makeBootDiskActions = useCallback( (disk: InstanceDisk): MenuAction[] => [ getSnapshotAction(disk), @@ -160,12 +167,20 @@ export function StorageTab() { modalTitle: 'Confirm unset boot disk', // TODO: copy + link to docs modalContent: ( -

- Are you sure you want to unset {disk.name} as the boot disk? This - will TODO COPY RE: WHAT HAPPENS HERE -

+
+

+ Are you sure you want to unset {disk.name} as the boot disk? It + will remain attached to the instance. +

+

+ Setting a boot disk is recommended unless you intend to manage boot order + within the instance. +

+
), actionType: 'primary', + // TODO: add docs link to modal footer similar to LearnMore from + // SettingsGroup. }), }, { @@ -188,23 +203,34 @@ export function StorageTab() { can be changed ), - onActivate: () => - confirmAction({ + onActivate: () => { + const bootDiskName = bootDisks.length > 0 ? bootDisks[0].name : undefined + const verb = bootDiskName ? 'change' : 'set' + return confirmAction({ doAction: () => instanceUpdate({ path: { instance: instance.id }, body: { bootDisk: disk.id }, }), - errorTitle: 'Could not set boot disk', - modalTitle: 'Confirm set boot disk', - // TODO: copy + link to docs - modalContent: ( + errorTitle: `Could not ${verb} boot disk`, + modalTitle: `Confirm ${verb} boot disk`, + modalContent: bootDiskName ? ( +

+ Are you sure you want to change the boot disk to {disk.name}? + Current boot disk {bootDiskName} will remain attached to the + instance. +

+ ) : (

Are you sure you want to set {disk.name} as the boot disk?

), actionType: 'primary', - }), + // TODO: add docs link to modal footer similar to LearnMore + // from SettingsGroup. This probably requires a change to + // `confirmAction`. + }) + }, }, { label: 'Detach', @@ -219,7 +245,7 @@ export function StorageTab() { }, }, ], - [detachDisk, instanceUpdate, instance.id, getSnapshotAction] + [detachDisk, instanceUpdate, instance.id, getSnapshotAction, bootDisks] ) const attachDisk = useApiMutation('instanceDiskAttach', { @@ -238,13 +264,6 @@ export function StorageTab() { }, }) - const { data: disks } = usePrefetchedApiQuery('instanceDiskList', instancePathQuery) - - const [bootDisks, otherDisks] = useMemo( - () => R.partition(disks.items, (d) => d.id === instance.bootDiskId), - [disks.items, instance.bootDiskId] - ) - const bootDisksTable = useReactTable({ columns: useColsWithActions(staticCols, makeBootDiskActions), data: useMemo( From fe3d078c517679540e4e95af6cce777192580e88 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 2 Oct 2024 23:51:26 -0500 Subject: [PATCH 24/25] bump API to latest dogfood version (only doc comment changes) --- OMICRON_VERSION | 2 +- app/api/__generated__/Api.ts | 11 +++++++---- app/api/__generated__/OMICRON_VERSION | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 873025fe99..f7a1d4ae62 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -144e91ae570e503a7e3f470efbcbba268afa4ae0 +c50cf019cd9be35f98266a7f4acacab0236b3a3d diff --git a/app/api/__generated__/Api.ts b/app/api/__generated__/Api.ts index 664c62dc54..e64990f1f9 100644 --- a/app/api/__generated__/Api.ts +++ b/app/api/__generated__/Api.ts @@ -1320,10 +1320,10 @@ export type DiskSource = */ export type DiskCreate = { description: string - /** initial source for this disk */ + /** The initial source for this disk */ diskSource: DiskSource name: Name - /** total size of the Disk in bytes */ + /** The total size of the Disk (in bytes) */ size: ByteCount } @@ -1806,10 +1806,10 @@ export type InstanceDiskAttachment = /** During instance creation, create and attach disks */ | { description: string - /** initial source for this disk */ + /** The initial source for this disk */ diskSource: DiskSource name: Name - /** total size of the Disk in bytes */ + /** The total size of the Disk (in bytes) */ size: ByteCount type: 'create' } @@ -1868,9 +1868,12 @@ An instance without an explicit boot disk can be booted: the options are as mana By default, all instances have outbound connectivity, but no inbound connectivity. These external addresses can be used to provide a fixed, known IP address for making inbound connections to the instance. */ externalIps?: ExternalIpCreate[] + /** The hostname to be assigned to the instance */ hostname: Hostname + /** The amount of RAM (in bytes) to be allocated to the instance */ memory: ByteCount name: Name + /** The number of vCPUs to be allocated to the instance */ ncpus: InstanceCpuCount /** The network interfaces to be created for this instance. */ networkInterfaces?: InstanceNetworkInterfaceAttachment diff --git a/app/api/__generated__/OMICRON_VERSION b/app/api/__generated__/OMICRON_VERSION index 750c7737ae..2c1de3ebde 100644 --- a/app/api/__generated__/OMICRON_VERSION +++ b/app/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -144e91ae570e503a7e3f470efbcbba268afa4ae0 +c50cf019cd9be35f98266a7f4acacab0236b3a3d From fce35a92dc4fa21ce531188c94f12a7cdf771bc6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 3 Oct 2024 09:46:28 -0500 Subject: [PATCH 25/25] rename disks to otherDisks, existing disk test looks at boot vs. other --- app/components/form/fields/DisksTableField.tsx | 2 +- app/forms/instance-create.tsx | 6 +++--- mock-api/msw/handlers.ts | 7 +++++-- test/e2e/instance-create.e2e.ts | 6 +++++- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/components/form/fields/DisksTableField.tsx b/app/components/form/fields/DisksTableField.tsx index 3df422316f..4084c7eacd 100644 --- a/app/components/form/fields/DisksTableField.tsx +++ b/app/components/form/fields/DisksTableField.tsx @@ -38,7 +38,7 @@ export function DisksTableField({ const { field: { value: items, onChange }, - } = useController({ control, name: 'disks' }) + } = useController({ control, name: 'otherDisks' }) return ( <> diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 7d3f35a1c2..fc5d86736b 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -103,7 +103,7 @@ export type InstanceCreateInput = Assign< SetRequired, { presetId: (typeof PRESETS)[number]['id'] - disks: DiskTableItem[] + otherDisks: DiskTableItem[] bootDiskName: string bootDiskSize: number @@ -139,7 +139,7 @@ const baseDefaultValues: InstanceCreateInput = { projectImageSource: '', diskSource: '', - disks: [], + otherDisks: [], networkInterfaces: { type: 'default' }, sshPublicKeys: [], @@ -317,7 +317,7 @@ export function CreateInstanceForm() { description: values.description, memory: instance.memory * GiB, ncpus: instance.ncpus, - disks: values.disks, + disks: values.otherDisks, bootDisk, externalIps: values.externalIps, start: values.start, diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 72d782b4bc..5278bf105c 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -17,6 +17,7 @@ import { INSTANCE_MIN_RAM_GiB, MAX_NICS_PER_INSTANCE, type ApiTypes as Api, + type InstanceDiskAttachment, type SamlIdentityProvider, } from '@oxide/api' @@ -417,8 +418,10 @@ export const handlers = makeHandlers({ * Eagerly check for disk errors. Execution will stop early and prevent orphaned disks from * being created if there's a failure. In omicron this is done automatically via an undo on the saga. */ - const otherDisks = body.disks || [] - const allDisks = body.boot_disk ? [body.boot_disk, ...otherDisks] : otherDisks + const allDisks: Json[] = [] + if (body.disks) allDisks.push(...body.disks) + if (body.boot_disk) allDisks.push(body.boot_disk) + for (const diskParams of allDisks) { if (diskParams.type === 'create') { errIfExists(db.disks, { name: diskParams.name, project_id: project.id }, 'disk') diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 12a59582c6..97af4ec0b5 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -296,7 +296,11 @@ test('create instance with existing disk', async ({ page }) => { await page.getByRole('button', { name: 'Create instance' }).click() await expect(page).toHaveURL(`/projects/mock-project/instances/${instanceName}/storage`) await expectVisible(page, [`h1:has-text("${instanceName}")`, 'text=8 GiB']) - await expectRowVisible(page.getByRole('table'), { Disk: 'disk-3' }) + + const bootDisks = page.getByRole('table', { name: 'Boot disk' }) + await expectRowVisible(bootDisks, { Disk: 'disk-3' }) + + await expect(page.getByText('No other disks')).toBeVisible() }) test('create instance with a silo image', async ({ page }) => {