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

Boot order UI #2335

Closed
david-crespo opened this issue Jul 22, 2024 · 5 comments · Fixed by #2464
Closed

Boot order UI #2335

david-crespo opened this issue Jul 22, 2024 · 5 comments · Fixed by #2464
Labels
customer For any bug reports or feature requests tied to customer requests
Milestone

Comments

@david-crespo
Copy link
Collaborator

david-crespo commented Jul 22, 2024

API PR: oxidecomputer/omicron#6585

This is looking like a boot_disk: Option<NameOrId> on the instance create body and a boot_disk_id: Uuid on the instance view response. Then (possibly in a followup PR on top of oxidecomputer/omicron#6321) there will also be a field on an instance update endpoint allowing you to change it. You can always remove the configured boot disk ID (I think), which will result in falling back to whatever the (likely undesirable) behavior is now. There will likely be an added constraint on disk detach, namely that you cannot detach a the boot disk — you have to change or clear the specified boot disk first.

So, the work on our end sounds like:

Instance create POST body

We already have a concept of which disk is the boot disk; currently we put it first in the list of disks. All we have to do is add bootDisk: bootDisk.name.

const bootDisk = getBootDiskAttachment(values)
const userData = values.userData
? await readBlobAsBase64(values.userData)
: undefined
await createInstance.mutateAsync({
query: { project },
body: {
name: values.name,
hostname: values.hostname || values.name,
description: values.description,
memory: instance.memory * GiB,
ncpus: instance.ncpus,
disks: [bootDisk, ...values.disks],
externalIps: values.externalIps,
start: values.start,
networkInterfaces: values.networkInterfaces,
sshPublicKeys: values.sshPublicKeys,
userData,
},
})

Instance disks table

  • Show which disk is the boot disk

    • Sort it first in the list
    • Probably with a little badge like we do for primary NIC
    image
  • Add Set as boot disk action to disk row actions

  • Disable disk detach button for boot disk (with helpful tooltip)

    • This is the only place we do disk detach; you can't do it, for example, on the main project disks list
image

Open questions

  • When an instance has no boot disk, should we show that somehow?
    • Put an info box somewhere saying "this instance doesn't have an explicit boot disk, so the behavior might be confusing"?
    • Only do that when there is more than one disk?
@david-crespo david-crespo added this to the 10 milestone Jul 22, 2024
@david-crespo david-crespo modified the milestones: 10, 11 Aug 5, 2024
@iximeow
Copy link
Member

iximeow commented Sep 18, 2024

You can always remove the configured boot disk ID (I think), which will result in falling back to whatever the (likely undesirable) behavior is now.

this is how i've currently got things set up in Nexus+Propolis, yeah. it's in a weird spot: it's kind of desirable - when we're not imposing a boot order, the guest OS can configure boot settings internally! - but at the same time it's possible to end up with unbootable instances like we've seen when changing disks around.

i think we can make some improvements to the no-explicit-boot-disk case such that it's not liable to break itself, but i'm thinking about getting to that after we land the API+Propolis changes for the explicit case.

re. your open questions:

Put an info box somewhere saying "this instance doesn't have an explicit boot disk, so the behavior might be confusing"?

this would become less common over time, since you intend to set bootDevice when creating new instances right? so that seems reasonable. one thing to consider, though, is that we can't easily backfill boot disk choices at the moment. my expectation is that when the API side is landed, everything will still be in the currently-undesirable no-explicit-choice case.

(exception being if there's only one disk attached, which we can probably call the boot disk going forward. i'll make a note on the Omicron PR about that..)

@david-crespo
Copy link
Collaborator Author

I see, that point about letting the guest do its own thing is very helpful because we'll probably want to call that out in UI copy somewhere.

@benjaminleonard
Copy link
Contributor

What you have here works for this first release I think.

One alternative we could consider (we explored this initially) is separating boot disk and remaining disks into separate stacked tables. What this might do for us is give us a place to more clearly call out when a boot disk is not selected and the potential consequences in its empty state. It also becomes a little easier to see which is the boot disk at a glance.

@david-crespo
Copy link
Collaborator Author

I thought about a separate table, and it is cool to have a spot to call out the lack of boot disk, but it might be misleading to make such a strong visual distinction because even when there is no designated boot disk, we will still boot from one of the disks.

@benjaminleonard
Copy link
Contributor

Without a designated disk do we know which one is going to be booted from (aside from any shenanigans in the VM itself)

@twinfees twinfees added the customer For any bug reports or feature requests tied to customer requests label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer For any bug reports or feature requests tied to customer requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants