-
Notifications
You must be signed in to change notification settings - Fork 19
Initial snapshots support #1204
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
Changes from all commits
a6f01ba
6271041
d836fca
81814b4
56eb693
5b6623c
8331472
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import type { PathParams, Snapshot, SnapshotCreate } from '@oxide/api' | ||
| import { useApiQuery } from '@oxide/api' | ||
| import { useApiMutation } from '@oxide/api' | ||
| import { useApiQueryClient } from '@oxide/api' | ||
| import { Success16Icon } from '@oxide/ui' | ||
|
|
||
| import { | ||
| DescriptionField, | ||
| ListboxField, | ||
| NameField, | ||
| SideModalForm, | ||
| } from 'app/components/form' | ||
| import { useRequiredParams, useToast } from 'app/hooks' | ||
|
|
||
| import type { CreateSideModalFormProps } from '.' | ||
|
|
||
| const useSnapshotDiskItems = (params: PathParams.Project) => { | ||
| const { data: disks } = useApiQuery('diskList', { ...params, limit: 1000 }) | ||
| return ( | ||
| disks?.items | ||
| .filter((disk) => disk.state.state === 'attached') | ||
| .map((disk) => ({ value: disk.name, label: disk.name })) || [] | ||
| ) | ||
| } | ||
|
|
||
| const values: SnapshotCreate = { | ||
| description: '', | ||
| disk: '', | ||
| name: '', | ||
| } | ||
|
|
||
| export function CreateSnapshotSideModalForm({ | ||
| id = 'create-snapshot-form', | ||
| title = 'Create Snapshot', | ||
| initialValues = values, | ||
| onSubmit, | ||
| onSuccess, | ||
| onError, | ||
| onDismiss, | ||
| ...props | ||
| }: CreateSideModalFormProps<SnapshotCreate, Snapshot>) { | ||
| const queryClient = useApiQueryClient() | ||
| const pathParams = useRequiredParams('orgName', 'projectName') | ||
| const addToast = useToast() | ||
|
|
||
| const diskItems = useSnapshotDiskItems(pathParams) | ||
|
|
||
| const createSnapshot = useApiMutation('snapshotCreate', { | ||
| onSuccess(data) { | ||
| queryClient.invalidateQueries('snapshotList', pathParams) | ||
| addToast({ | ||
| icon: <Success16Icon />, | ||
| title: 'Success!', | ||
| content: 'Your snapshot has been created.', | ||
| }) | ||
| onSuccess?.(data) | ||
| onDismiss() | ||
| }, | ||
| onError, | ||
| }) | ||
|
|
||
| return ( | ||
| <SideModalForm | ||
| id={id} | ||
| title={title} | ||
| initialValues={initialValues} | ||
| onDismiss={onDismiss} | ||
| onSubmit={ | ||
| onSubmit || | ||
| ((values) => { | ||
| createSnapshot.mutate({ | ||
| ...pathParams, | ||
| body: values, | ||
| }) | ||
| }) | ||
| } | ||
| {...props} | ||
| > | ||
| <NameField id="snapshot-name" /> | ||
| <DescriptionField id="snapshot-description" /> | ||
| <ListboxField | ||
| id="snapshot-disk" | ||
| name="disk" | ||
| label="Disk" | ||
| items={diskItems} | ||
| required | ||
| /> | ||
| </SideModalForm> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import type { LoaderFunctionArgs } from 'react-router-dom' | |
| import { Link, useNavigate } from 'react-router-dom' | ||
|
|
||
| import type { Disk } from '@oxide/api' | ||
| import { genName } from '@oxide/api' | ||
| import { apiQueryClient } from '@oxide/api' | ||
| import { useApiMutation, useApiQueryClient } from '@oxide/api' | ||
| import { useApiQuery } from '@oxide/api' | ||
|
|
@@ -14,13 +15,19 @@ import { | |
| PageHeader, | ||
| PageTitle, | ||
| Storage24Icon, | ||
| Success16Icon, | ||
| TableActions, | ||
| buttonStyle, | ||
| } from '@oxide/ui' | ||
|
|
||
| import { DiskStatusBadge } from 'app/components/StatusBadge' | ||
| import CreateDiskSideModalForm from 'app/forms/disk-create' | ||
| import { requireProjectParams, useProjectParams, useRequiredParams } from 'app/hooks' | ||
| import { | ||
| requireProjectParams, | ||
| useProjectParams, | ||
| useRequiredParams, | ||
| useToast, | ||
| } from 'app/hooks' | ||
| import { pb } from 'app/util/path-builder' | ||
|
|
||
| function AttachedInstance({ | ||
|
|
@@ -70,14 +77,41 @@ export function DisksPage({ modal }: DisksPageProps) { | |
| const queryClient = useApiQueryClient() | ||
| const { orgName, projectName } = useRequiredParams('orgName', 'projectName') | ||
| const { Table, Column } = useQueryTable('diskList', { orgName, projectName }) | ||
| const addToast = useToast() | ||
|
|
||
| const deleteDisk = useApiMutation('diskDelete', { | ||
| onSuccess() { | ||
| queryClient.invalidateQueries('diskList', { orgName, projectName }) | ||
| }, | ||
| }) | ||
|
|
||
| const createSnapshot = useApiMutation('snapshotCreate', { | ||
| onSuccess() { | ||
| queryClient.invalidateQueries('snapshotList', { orgName, projectName }) | ||
| addToast({ | ||
| icon: <Success16Icon />, | ||
| title: 'Success!', | ||
| content: 'Snapshot successfully created', | ||
| }) | ||
| }, | ||
| }) | ||
|
|
||
| const makeActions = (disk: Disk): MenuAction[] => [ | ||
| { | ||
| label: 'Snapshot', | ||
| onActivate() { | ||
| createSnapshot.mutate({ | ||
| orgName, | ||
| projectName, | ||
| body: { | ||
| name: genName(disk.name), | ||
| disk: disk.name, | ||
| description: '', | ||
| }, | ||
| }) | ||
| }, | ||
| disabled: disk.state.state !== 'attached', | ||
| }, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This button is awesome. It feels a little abrupt to me — what I expected to happen was it would pull up a side modal with the disk name pre-populated and let me pick a name (with the generated default also pre-populated).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The abruptness is why I mentioned we need a better feedback mechanism.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really there are two issues, I think. In practice the snapshotting will not be instant (you should probably add a couple of seconds delay to the mock endpoint), so there's the feedback on completion, but I also think there's a general "ok what is happening here, what is being created, what is it called, where does it end up?" that might not be answered by the completion confirmation. Or at least, even if we do give some of that info at completion time, and make it easy, for example, to click through to wherever the snapshot lives, I think the user wants some of that info before they click the button so they can feel confident it's what they want to do. |
||
| { | ||
| label: 'Delete', | ||
| onActivate: () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,6 @@ export const pb = { | |
| projectEdit: (params: PP.Project) => `${pb.project(params)}/edit`, | ||
|
|
||
| access: (params: PP.Project) => `${pb.project(params)}/access`, | ||
| snapshots: (params: PP.Project) => `${pb.project(params)}/snapshots`, | ||
| images: (params: PP.Project) => `${pb.project(params)}/images`, | ||
|
|
||
| instances: (params: PP.Project) => `${pb.project(params)}/instances`, | ||
|
|
@@ -24,8 +23,11 @@ export const pb = { | |
|
|
||
| diskNew: (params: PP.Project) => `${pb.project(params)}/disks-new`, | ||
| disks: (params: PP.Project) => `${pb.project(params)}/disks`, | ||
| vpcNew: (params: PP.Project) => `${pb.project(params)}/vpcs-new`, | ||
|
|
||
| snapshotNew: (params: PP.Project) => `${pb.project(params)}/snapshots-new`, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a triviality to follow up on #1210 |
||
| snapshots: (params: PP.Project) => `${pb.project(params)}/snapshots`, | ||
|
|
||
| vpcNew: (params: PP.Project) => `${pb.project(params)}/vpcs-new`, | ||
| vpcs: (params: PP.Project) => `${pb.project(params)}/vpcs`, | ||
| vpc: (params: PP.Vpc) => `${pb.vpcs(params)}/${params.vpcName}`, | ||
| vpcEdit: (params: PP.Vpc) => `${pb.vpc(params)}/edit`, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the API will do it whether it's attached or not. Confirming with James.
https://github.com/oxidecomputer/omicron/blob/5ff619e31001720c6dfed48d0106fddbccfbd52a/nexus/src/app/sagas/snapshot_create.rs#L608-L697
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just kidding! snapshotting unattached disks is unimplemented
https://github.com/oxidecomputer/omicron/blob/74f3ca89af11b0ce6d9f9bd4b5bdcbeb04d1ba3e/sled-agent/src/sled_agent.rs#L421-L433
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I checked with him about this yesterday. Another thing that could fail here is that I believe the instance actually has to be running. That's a much more sophisticated query though, so I just left it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should not do that. Maybe we need a snapshottable disks endpoint (or query param filter on the disks endpoint 🧐).