Skip to content
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

Poll instance state when instance is transitioning #2360

Merged
merged 10 commits into from
Aug 16, 2024
11 changes: 10 additions & 1 deletion app/api/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,20 @@ const instanceActions: Record<string, InstanceState[]> = {
// while also making the states available directly

export const instanceCan = R.mapValues(instanceActions, (states) => {
const test = (i: Instance) => states.includes(i.runState)
const test = (i: { runState: InstanceState }) => states.includes(i.runState)
test.states = states
return test
})

export function instanceTransitioning({ runState }: Instance) {
return (
runState === 'creating' ||
runState === 'starting' ||
runState === 'stopping' ||
runState === 'rebooting'
)
}

const diskActions: Record<string, DiskState['state'][]> = {
// https://github.com/oxidecomputer/omicron/blob/4970c71e/nexus/db-queries/src/db/datastore/disk.rs#L578-L582.
delete: ['detached', 'creating', 'faulted'],
Expand Down
39 changes: 26 additions & 13 deletions app/pages/project/instances/actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,29 +29,35 @@ type Options = {
}

export const useMakeInstanceActions = (
projectSelector: { project: string },
{ project }: { project: string },
options: Options = {}
): MakeActions<Instance> => {
const navigate = useNavigate()

// if you also pass onSuccess to mutate(), this one is not overridden — this
// one runs first, then the one passed to mutate()
// one runs first, then the one passed to mutate().
//
// We pull out the mutate functions because they are referentially stable,
// while the whole useMutation result object is not. The async ones are used
// when we need to confirm because the confirm modals want that.
const opts = { onSuccess: options.onSuccess }
const startInstance = useApiMutation('instanceStart', opts)
const stopInstance = useApiMutation('instanceStop', opts)
const rebootInstance = useApiMutation('instanceReboot', opts)
const { mutate: startInstance } = useApiMutation('instanceStart', opts)
const { mutateAsync: stopInstanceAsync } = useApiMutation('instanceStop', opts)
const { mutate: rebootInstance } = useApiMutation('instanceReboot', opts)
// delete has its own
const deleteInstance = useApiMutation('instanceDelete', { onSuccess: options.onDelete })
const { mutateAsync: deleteInstanceAsync } = useApiMutation('instanceDelete', {
onSuccess: options.onDelete,
})

return useCallback(
(instance) => {
const instanceSelector = { ...projectSelector, instance: instance.name }
const instanceParams = { path: { instance: instance.name }, query: projectSelector }
const instanceSelector = { project, instance: instance.name }
const instanceParams = { path: { instance: instance.name }, query: { project } }
return [
{
label: 'Start',
onActivate() {
startInstance.mutate(instanceParams, {
startInstance(instanceParams, {
onSuccess: () => addToast({ title: `Starting instance '${instance.name}'` }),
onError: (error) =>
addToast({
Expand All @@ -71,7 +77,7 @@ export const useMakeInstanceActions = (
confirmAction({
actionType: 'danger',
doAction: () =>
stopInstance.mutateAsync(instanceParams, {
stopInstanceAsync(instanceParams, {
onSuccess: () =>
addToast({ title: `Stopping instance '${instance.name}'` }),
}),
Expand All @@ -93,7 +99,7 @@ export const useMakeInstanceActions = (
{
label: 'Reboot',
onActivate() {
rebootInstance.mutate(instanceParams, {
rebootInstance(instanceParams, {
onSuccess: () => addToast({ title: `Rebooting instance '${instance.name}'` }),
onError: (error) =>
addToast({
Expand All @@ -117,7 +123,7 @@ export const useMakeInstanceActions = (
label: 'Delete',
onActivate: confirmDelete({
doDelete: () =>
deleteInstance.mutateAsync(instanceParams, {
deleteInstanceAsync(instanceParams, {
onSuccess: () =>
addToast({ title: `Deleting instance '${instance.name}'` }),
}),
Expand All @@ -132,6 +138,13 @@ export const useMakeInstanceActions = (
},
]
},
[projectSelector, deleteInstance, navigate, rebootInstance, startInstance, stopInstance]
[
project,
navigate,
deleteInstanceAsync,
rebootInstance,
startInstance,
stopInstanceAsync,
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file are no longer strictly needed for this PR because I've taken the polling on the list page back out, but they are still very good to do.

)
}
31 changes: 26 additions & 5 deletions app/pages/project/instances/instance/InstancePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
} from '@oxide/api'
import { Instances16Icon, Instances24Icon } from '@oxide/design-system/icons/react'

import { instanceTransitioning } from '~/api/util'
import { DocsPopover } from '~/components/DocsPopover'
import { ExternalIps } from '~/components/ExternalIps'
import { MoreActionsMenu } from '~/components/MoreActionsMenu'
Expand All @@ -28,6 +29,8 @@ import { EmptyCell } from '~/table/cells/EmptyCell'
import { DateTime } from '~/ui/lib/DateTime'
import { PageHeader, PageTitle } from '~/ui/lib/PageHeader'
import { PropertiesTable } from '~/ui/lib/PropertiesTable'
import { Spinner } from '~/ui/lib/Spinner'
import { Tooltip } from '~/ui/lib/Tooltip'
import { Truncate } from '~/ui/lib/Truncate'
import { docLinks } from '~/util/links'
import { pb } from '~/util/path-builder'
Expand Down Expand Up @@ -94,10 +97,19 @@ export function InstancePage() {
onDelete: () => navigate(pb.instances(instanceSelector)),
})

const { data: instance } = usePrefetchedApiQuery('instanceView', {
path: { instance: instanceSelector.instance },
query: { project: instanceSelector.project },
})
const { data: instance } = usePrefetchedApiQuery(
'instanceView',
{
path: { instance: instanceSelector.instance },
query: { project: instanceSelector.project },
},
{
refetchInterval: ({ state: { data: instance } }) =>
instance && instanceTransitioning(instance) ? 1000 : false,
}
)

const polling = instanceTransitioning(instance)

david-crespo marked this conversation as resolved.
Show resolved Hide resolved
const { data: nics } = usePrefetchedApiQuery('instanceNetworkInterfaceList', {
query: {
Expand Down Expand Up @@ -157,7 +169,16 @@ export function InstancePage() {
<span className="ml-1 text-quaternary"> {memory.unit}</span>
</PropertiesTable.Row>
<PropertiesTable.Row label="status">
<InstanceStatusBadge status={instance.runState} />
<div className="flex">
<InstanceStatusBadge status={instance.runState} />
{polling && (
<Tooltip content="Auto-refreshing while state changes" delay={150}>
<button type="button">
<Spinner className="ml-2" />
</button>
</Tooltip>
)}
</div>
</PropertiesTable.Row>
<PropertiesTable.Row label="vpc">
{vpc ? (
Expand Down
66 changes: 33 additions & 33 deletions app/pages/project/instances/instance/tabs/NetworkingTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,31 @@ const staticCols = [

const updateNicStates = fancifyStates(instanceCan.updateNic.states)

const ipColHelper = createColumnHelper<ExternalIp>()
const staticIpCols = [
ipColHelper.accessor('ip', {
cell: (info) => <CopyableIp ip={info.getValue()} />,
}),
ipColHelper.accessor('kind', {
header: () => (
<>
Kind
<TipIcon className="ml-2">
Floating IPs can be detached from this instance and attached to another.
</TipIcon>
</>
),
cell: (info) => <Badge color="neutral">{info.getValue()}</Badge>,
}),
ipColHelper.accessor('name', {
cell: (info) => (info.getValue() ? info.getValue() : <EmptyCell />),
}),
ipColHelper.accessor((row) => ('description' in row ? row.description : undefined), {
header: 'description',
cell: (info) => <DescriptionCell text={info.getValue()} />,
}),
]

export function NetworkingTab() {
const instanceSelector = useInstanceSelector()
const { instance: instanceName, project } = instanceSelector
Expand All @@ -157,13 +182,13 @@ export function NetworkingTab() {
setCreateModalOpen(false)
},
})
const deleteNic = useApiMutation('instanceNetworkInterfaceDelete', {
const { mutateAsync: deleteNic } = useApiMutation('instanceNetworkInterfaceDelete', {
onSuccess() {
queryClient.invalidateQueries('instanceNetworkInterfaceList')
addToast({ content: 'Network interface deleted' })
},
})
const editNic = useApiMutation('instanceNetworkInterfaceUpdate', {
const { mutate: editNic } = useApiMutation('instanceNetworkInterfaceUpdate', {
onSuccess() {
queryClient.invalidateQueries('instanceNetworkInterfaceList')
},
Expand All @@ -180,7 +205,7 @@ export function NetworkingTab() {
{
label: 'Make primary',
onActivate() {
editNic.mutate({
editNic({
path: { interface: nic.name },
query: instanceSelector,
body: { ...nic, primary: true },
Expand Down Expand Up @@ -211,7 +236,7 @@ export function NetworkingTab() {
label: 'Delete',
onActivate: confirmDelete({
doDelete: () =>
deleteNic.mutateAsync({
deleteNic({
path: { interface: nic.name },
query: instanceSelector,
}),
Expand Down Expand Up @@ -243,32 +268,7 @@ export function NetworkingTab() {
query: { project },
})

const ipColHelper = createColumnHelper<ExternalIp>()
const staticIpCols = [
ipColHelper.accessor('ip', {
cell: (info) => <CopyableIp ip={info.getValue()} />,
}),
ipColHelper.accessor('kind', {
header: () => (
<>
Kind
<TipIcon className="ml-2">
Floating IPs can be detached from this instance and attached to another.
</TipIcon>
</>
),
cell: (info) => <Badge color="neutral">{info.getValue()}</Badge>,
}),
ipColHelper.accessor('name', {
cell: (info) => (info.getValue() ? info.getValue() : <EmptyCell />),
}),
ipColHelper.accessor((row) => ('description' in row ? row.description : undefined), {
header: 'description',
cell: (info) => <DescriptionCell text={info.getValue()} />,
}),
]

const ephemeralIpDetach = useApiMutation('instanceEphemeralIpDetach', {
const { mutateAsync: ephemeralIpDetach } = useApiMutation('instanceEphemeralIpDetach', {
onSuccess() {
queryClient.invalidateQueries('instanceExternalIpList')
addToast({ content: 'Your ephemeral IP has been detached' })
Expand All @@ -278,7 +278,7 @@ export function NetworkingTab() {
},
})

const floatingIpDetach = useApiMutation('floatingIpDetach', {
const { mutateAsync: floatingIpDetach } = useApiMutation('floatingIpDetach', {
onSuccess() {
queryClient.invalidateQueries('floatingIpList')
queryClient.invalidateQueries('instanceExternalIpList')
Expand All @@ -301,12 +301,12 @@ export function NetworkingTab() {
const doAction =
externalIp.kind === 'floating'
? () =>
floatingIpDetach.mutateAsync({
floatingIpDetach({
path: { floatingIp: externalIp.name },
query: { project },
})
: () =>
ephemeralIpDetach.mutateAsync({
ephemeralIpDetach({
path: { instance: instanceName },
query: { project },
})
Expand Down
14 changes: 8 additions & 6 deletions app/pages/project/instances/instance/tabs/StorageTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export function StorageTab() {
[instanceName, project]
)

const detachDisk = useApiMutation('instanceDiskDetach', {
const { mutate: detachDisk } = useApiMutation('instanceDiskDetach', {
onSuccess() {
queryClient.invalidateQueries('instanceDiskList')
addToast({ content: 'Disk detached' })
Expand All @@ -86,7 +86,7 @@ export function StorageTab() {
})
},
})
const createSnapshot = useApiMutation('snapshotCreate', {
const { mutate: createSnapshot } = useApiMutation('snapshotCreate', {
onSuccess() {
queryClient.invalidateQueries('snapshotList')
addToast({ content: 'Snapshot created' })
Expand All @@ -112,7 +112,7 @@ export function StorageTab() {
</>
),
onActivate() {
createSnapshot.mutate({
createSnapshot({
query: { project },
body: {
name: genName(disk.name),
Expand All @@ -124,18 +124,20 @@ export function StorageTab() {
},
{
label: 'Detach',
disabled: !instanceCan.detachDisk(instance) && (
disabled: !instanceCan.detachDisk({ runState: instance.runState }) && (
<>
Instance must be <span className="text-default">stopped</span> before disk can
be detached
</>
),
onActivate() {
detachDisk.mutate({ body: { disk: disk.name }, ...instancePathQuery })
detachDisk({ body: { disk: disk.name }, ...instancePathQuery })
},
},
],
[detachDisk, instance, instancePathQuery, createSnapshot, project]
// important to pass instance.runState because instance is not referentially
// stable when we are polling when instance is in transition
[detachDisk, instance.runState, instancePathQuery, createSnapshot, project]
)

const attachDisk = useApiMutation('instanceDiskAttach', {
Expand Down
4 changes: 4 additions & 0 deletions app/ui/styles/components/spinner.css
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
stroke: var(--content-default);
}

.spinner-secondary .path {
stroke: var(--content-secondary);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided against using this, but this fixes the variant="secondary" spinner to make the brighter part gray instead of green.

image

.spinner-primary .bg {
stroke: var(--content-accent);
}
Expand Down
2 changes: 1 addition & 1 deletion mock-api/msw/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ export const handlers = makeHandlers({

setTimeout(() => {
instance.run_state = 'running'
}, 1000)
}, 3000)

return json(instance, { status: 202 })
},
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/instance-serial.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ test('serial console can connect while starting', async ({ page }) => {
await page.getByRole('link', { name: 'Connect' }).click()

// The message goes from creating to starting and then disappears once
// the instance is running
await expect(page.getByText('The instance is creating')).toBeVisible()
// the instance is running. skip the check for "creating" because it can
// go by quickly
await expect(page.getByText('Waiting for the instance to start')).toBeVisible()
await expect(page.getByText('The instance is starting')).toBeVisible()
await expect(page.getByText('The instance is')).toBeHidden()
Expand Down
Loading