teach propolis-server to understand configurable boot order #756

iximeow · 2024-09-03T21:31:17Z

brings the existing fw_cfg-based boot order configuration that we already have in propolis-standalone over to propolis-server as well.

pairs with oxidecomputer/omicron#6585 to actually configure instances' boot settings and get them to Propolis.

while we accept a list of boot options here, note that Nexus intentionally has a more reductive view, maintaining only a single boot_disk. this is heavily based on what i've learned writing tests for how OVMF+guests behave with different mixes of boot device orders and sources of boot configuration. this conversation on RFD 470 gets at the crux of it - we don't have like a platform NVRAM device for guests to persist EFI variables in, so boot configuration is persisted in the EFI system partition (so, associated with a disk, not an instance). because we're not going to go modify the EFI system partition in user disks to effect boot order preferences, we're using the QEMU-style fw_cfg mechanism to indicate boot order. this, in turn, means that guest settings are blown away and replaced with what OVMF determines at boot time.

taken together, this isn't an ideal spot to be in if we were to support more complex boot behavior logic, and it's probably not how we want to work multi-device orderings into propolis-server. it also means that guest-managed boot ordering just don't reliably persist if the instance is configured with a specific boot device!

in the future with managed nonvolatile storage for guest firmware to persist settings in, we'll be in a somewhat better position to extend boot logic, and so this PR should leave us able to do so without much API hassle. that will probably be more interesting on the control plane side, which is why i'm permissive on the Propolis side, but restrictive on the Nexus side.

bin/propolis-server/src/lib/initializer.rs

phd-tests/framework/src/test_vm/config.rs

phd-tests/tests/src/boot_order.rs

bin/propolis-server/src/lib/initializer.rs

bin/propolis-standalone/src/main.rs

gjcolombo

@iximeow I've taken a stab at answering your other questions here; let me know if I can clarify any of those answers or get you any more info about how these parts of Propolis are (meant to be) arranged.

Thanks for putting this together and for asking thoughtful questions about the existing design! There are definitely some pre-existing things we can go improve here.

bin/propolis-server/src/lib/initializer.rs

lib/propolis-client/src/instance_spec.rs

crates/propolis-api-types/src/instance_spec/v0/mod.rs

phd-tests/framework/src/test_vm/config.rs

phd-tests/tests/src/boot_order.rs

this could have been made to work, but i'm walking this back as it would land fairly untested at first

iximeow

somewhat closer to landing, almost all the changes here are to the new boot_order.rs tests as i've been checking that guests behave in a useful way. i've one last test i want to write on that front: what does a guest see if we configure a boot order via EFI variable, stop the VM, configure a different boot order via fw_cfg, then restart a VM?

phd-tests/tests/src/boot_order.rs

iximeow · 2024-09-10T16:20:07Z

phd-tests/tests/src/boot_order.rs

+    // The entry we booted from is clearly valid, so we should be able to insert a few duplicate
+    // entries. We won't boot into them, but if something bogus happens and we did boot one of
+    // these, at least it'll work and we can detect the misbehavior.
+    //
+    // But here's a weird one: if we just append these to the end, on reboot they'll be moved
+    // somewhat up the boot order. This occurrs both if setting variables through `efibootmgr` or
+    // by writing to /sys/firmware/efi/efivars/BootOrder-* directly. As an example, say we had a
+    // boot order of "0004,0001,0003,0000" where boot options were as follows:
+    // * 0000: UiApp
+    // * 0001: PCI device 4, function 0
+    // * 0003: EFI shell (Firmware volume+file)
+    // * 0004: Ubuntu (HD partition 15, GPT formatted)
+    //
+    // If we duplicate entry 4 to new options FFF0 and FFFF, reset the boot order to
+    // "0004,0001,0003,0000,FFF0,FFFF", then reboot the VM, the boot order when it comes back
+    // up will be "0004,0001,FFF0,FFFF,0003,0000".
+    //
+    // This almost makes sense, but with other devices in the mix I've seen reorderings like
+    // `0004,0001,<PCI 16.0>,0003,0000,FFF0,FFFF` turning into
+    // `0004,0001,FFF0,FFFF,<PCI 16.0>,0003,0000`. This is particularly strange in that the new
+    // options were reordered around some other PCI device. It's not the boot order we set!
+    //
+    // So, to at least confirm we *can* modify the boot order in a stable way, make a somewhat less
+    // ambitious change: insert the duplicate boot options in the order directly after the option
+    // they are duplicates of. This seems to not get reordered.


i cannot make sense of this, but it doesn't seem to be a bug resulting from anything we're doing. i'd describe the overall behavior as "kind of works, and probably not something to debug in Propolis, maybe something to better understand in OVMF"..

I think this is because of ovmf trying to reconcile the EFI boot order with what's in the qemu fwcfg - https://github.com/oxidecomputer/edk2/blob/edd72391086a9a297bf9aa011fe53b449165e556/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c#L1911

Ah, basically what Greg had called out on the RFD

yeah exactly. the specifically confusing thing about this, to me, is that the duplicate FFF0 and FFFF entries remain in the boot order at all. i would expect them to get dropped late in SetBootOrderFromQemu, when it calls into BootOrderComplete and gets here.

between this and fwcfg overriding guest-configured boot order generally, i'm leaning towards actual configurations Propolis receives only naming a single boot device. if any of this changes because of a change in OVMF at some point, i want the test to fail as an indicator that Something Changed, but it feels like we shouldn't be relying on specific semantics here

iximeow · 2024-09-10T16:22:43Z

phd-tests/tests/src/boot_order.rs

+///
+/// For Linux guests: variables automatically have their prior attributes prepended. Provide only
+/// the variable's data.
+async fn write_efivar(


boring but delayed realization: writing to NvVars only actually matters if the guest OS has an EFI partition to persist to. Alpine virt images, for example, do not, so these changes are always lost on reboot. The "guest can reconfigure boot order" test will fail because of this, and rightfully so because the guest image simply does not support reconfiguring the boot order via EFI.

i think that's all basically fine, since we'd expect Alpine to basically be fine for other functionality, but now i'm curious what CI thinks of this..

FWIW there's a guest_os_has_read_only_fs helper method on TestVm that's meant to help with cases like this--see crucible::smoke::shutdown_persistence_test for an example. (Of course, in that case you've just ended up not running the test, which makes CI pass but doesn't add much else of value.)

that'd need a related fs_is_read_readonly() helper or something, since EFI variables are on a different filesystem, which would be fine.. but the Alpine image that doesn't seem to persist NvVars also has /sys/firmware/efi/efivars mounted rw. i'm not confident enough to assert this yet, but i suspect somewhere in efivarfs the actual persistence to disk is silently lost..?

lol: bcb6824#diff-3a20a9ed692f02614280c3438c2e7368597cb587487dcc28be5c6bac8affc549R321-R340

the Alpine ISO 9660-backed virt image, for reference, looks like this:

localhost:~# mount | grep efivarfs efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)

note it even says rw!! i think this is because technically the filesystem could be reasonably written to even without a writable ESP - it could be configuring real non-volatile storage. just, it's not here. it's unfortunate the write gets lost as a result, but it is what it is.

these still depend on having a guest artifact with an EFI partition to persist NvVars into, but with such a partition they actually show useful behavior!

functionally the guest adaptations for 22.04 and 24.04 are the same. these "24.04" definitions are actually in support of my weird 24.04 base image that has a user "root" instead of ubuntu-with-a-password

this makes the tests kind of legible..

don't *really* need the file present for any test purpose

openapi/propolis-server.json

iximeow · 2024-09-13T22:15:34Z

phd-tests/framework/src/guest_os/ubuntu24_04.rs

+    fn get_shell_prompt(&self) -> &'static str {
+        "root@ubuntu:~#"
+    }


this was useful for my inconsistently-configured 24.04 test image, but probably not how any other 24.04 test image would be set up. or at least, it's different from the existing 22.04 personality (which does not expect to be root). so i'll drop this from the PR and make sure we've got something like "how to get or assemble test images for PHD" written up.

is 24.04 actually necessary for any particular reason? IIRC on CI we run these tests against an Alpine image, but if we need to start running certain tests against Ubuntu 24.04, presumably we can start doing that as well?

not at this point, this is partially an artifact of at one point using efibootmgr (which is not in the base Alpine image) for configuration.

i did notice that guest_can_adjust_boot_order does not pass with the Alpine CI image, but does with my 24.04 image. i'm not exactly sure why yet, but i don't think it works out in the direction of "need 24.04 in CI".

make sure we've got something like "how to get or assemble test images for PHD" written up

I'd almost forgotten about it, but a ways back I created the guest-os-image-configs repo as a place to put stuff like this.

bin/propolis-server/src/lib/initializer.rs

hawkw · 2024-09-13T22:31:00Z

phd-tests/framework/src/guest_os/ubuntu24_04.rs

+    fn get_shell_prompt(&self) -> &'static str {
+        "root@ubuntu:~#"
+    }


is 24.04 actually necessary for any particular reason? IIRC on CI we run these tests against an Alpine image, but if we need to start running certain tests against Ubuntu 24.04, presumably we can start doing that as well?

hawkw · 2024-09-13T22:35:38Z

phd-tests/tests/src/boot_order/efi_utils.rs

+    // parsing here brought to you by rereading
+    // * https://uefi.org/specs/UEFI/2.10/10_Protocols_Device_Path_Protocol.html
+    // * https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html


<3 thanks for including the references :)

phd-tests/tests/src/boot_order/efi_utils.rs

iximeow · 2024-09-16T23:44:23Z

from bmat:

4349  2024-09-16T23:34:32.081Z  failures:
4350  2024-09-16T23:34:32.081Z      phd_tests::boot_order::boot_order_source_priority
4351  2024-09-16T23:34:32.081Z      phd_tests::boot_order::guest_can_adjust_boot_order
4352  2024-09-16T23:34:32.081Z      phd_tests::boot_order::unbootable_disk_skipped
4353  2024-09-16T23:34:32.081Z      phd_tests::boot_order::configurable_boot_order

this is somewhat surprising. i expected guest_can_adjust_boot_order to fail for the same reason as mentioned here but the rest all passed locally with the same Alpine image..

edit: finally figured it out. didn't pull boot_order through the new TryFrom<InstanceSpecV0> for Spec...

also rustfmt has different opinions now?

this drops the untested support from booting from NICs as well

this test does not rely on an EFI partition existing at all!

gjcolombo

This looks fantastic. Thanks for putting in all the work to make it happen (especially all the digging into the UEFI spec and EDK2)! I found a few small bolts to tighten before merging but overall this LGTM.

bin/propolis-server/src/lib/initializer.rs

bin/propolis-server/src/lib/spec/builder.rs

phd-tests/framework/src/test_vm/config.rs

phd-tests/tests/src/boot_order.rs

Co-authored-by: Greg Colombo <[email protected]>

bin/propolis-cli/src/main.rs

bin/propolis-server/src/lib/initializer.rs

bin/propolis-server/src/lib/spec/builder.rs

hawkw · 2024-09-25T21:14:10Z

bin/propolis-server/src/lib/spec/mod.rs

@@ -57,6 +57,7 @@ pub(crate) struct Spec {
    pub board: Board,
    pub disks: HashMap<String, Disk>,
    pub nics: HashMap<String, Nic>,
+    pub boot_order: Option<Vec<BootOrderEntry>>,


nitpicky: maybe worth a comment explaining the semantic difference between None and Some([])?

lib/propolis-client/src/instance_spec.rs

hawkw · 2024-09-25T21:18:41Z

phd-tests/framework/src/test_vm/config.rs

-        self.boot_disk = DiskRequest {
+        let boot_order = self.boot_order.get_or_insert(Vec::new());
+        if let Some(prev_boot_item) =
+            boot_order.iter().position(|d| *d == "boot-disk")


super goofy, unimportant nitpick that nobody besides me will ever care about: the string "boot-disk" shows up enough here that i might const-ify it...

hawkw · 2024-09-25T21:20:17Z

phd-tests/framework/src/test_vm/config.rs

+        // The first disk in the boot list might not be the disk a test *actually* expects to boot.
+        //
+        // If there are multiple bootable disks in the boot order, we'll assume they're all
+        // the same guest OS kind. So look for `boot-disk` - if there isn't a disk named
+        // `boot-disk` then fall back to hoping the first disk in the boot order is a bootable
+        // disk, and if *that* isn't a bootable disk, maybe the first disk is.
+        //
+        // TODO: theoretically we might want to accept configuration of a specific guest OS adapter
+        // and avoid the guessing games. So far the above supports existing tests and makes them
+        // "Just Work", but a more complicated test may want more control here.


the magic name "boot-disk" and its behavior might be worth documenting in the API docs for...one of the methods in this module?

phd-tests/tests/src/boot_order.rs

hawkw · 2024-09-25T22:11:43Z

phd-tests/tests/src/boot_order.rs

+    let boot_num_bytes = read_efivar(&vm, BOOT_CURRENT_VAR).await?;
+
+    let boot_num: u16 = u16::from_le_bytes(boot_num_bytes.try_into().unwrap());
+
+    let boot_option_bytes = read_efivar(&vm, &bootvar(boot_num)).await?;
+
+    let mut cursor = Cursor::new(boot_option_bytes.as_slice());
+
+    let load_option = EfiLoadOption::parse_from(&mut cursor).unwrap();


i wonder if this ought to be a function, 'cause it also shows up twice in the configurable_boot_order test?

propolis-server was correct with respect to the scheme PHD uses to name disks and their backends (user-provided name is the name of the backend, derived name is the name of the disk), but disk requests from `parse_disk_from_request` use the user-provided name as the device name, with the derived name as the backend name. this means that propolis-server is wrong when used with Nexus, and real boot disk names never match against any of the `<name>-backend` backends they were being compared against. this probably makes the whole thing work though! yay!

* a few typos * a few 80col violations * actually enforce that boot devices are on PCI bus 0 * emphasize that propolis-server should never see a Spec with invalid boot disk * vestigial line * phd_skip!() instead of warn!+return Ok

phd-tests/framework/src/test_vm/config.rs

these newtypes are useful for other kinds of devices, and in propolis-api-types generally, but threading that through is more time than i can spend on this right now

hawkw · 2024-09-28T02:12:38Z

you love to see it 🎉

) the Omicron side of adding explicit boot order selection to instances (counterpart to [propolis#756](oxidecomputer/propolis#756)). first, this extends `params::InstanceCreate` to take a new `boot_disk: Option<params::InstanceDiskAttachment>`. additionally, this adds a `PUT /v1/instances/{instance}` to update instances. the only property that can be updated at the moment is `boot_disk`, pick a new boot disk or unset it entirely. this also partially subsumes #6321. finally, this updates Omicron to reference a recent enough Propolis that #756 is included. a surprising discovery along the way: you can specify a disk to be attached multiple times in `disks` today, when creating an instance, and we're fine with it! this carries through with the new `boot_disk` field: if you specify the disk as `boot_disk` and in `disks`, it is technically listing the disk for attachment twice but this will succeed.

iximeow commented Sep 3, 2024

View reviewed changes

iximeow force-pushed the ixi/propolis-bootorder branch from d0d140a to f5437f6 Compare September 3, 2024 23:18

iximeow commented Sep 3, 2024

View reviewed changes

bin/propolis-server/src/lib/initializer.rs Outdated Show resolved Hide resolved

gjcolombo self-assigned this Sep 3, 2024

pfmooney reviewed Sep 3, 2024

View reviewed changes

bin/propolis-standalone/src/main.rs Outdated Show resolved Hide resolved

wip: teach propolis-server to understand configurable boot order

f5437f6

gjcolombo reviewed Sep 4, 2024

View reviewed changes

hawkw self-requested a review September 4, 2024 18:11

Merge branch 'master' into ixi/propolis-bootorder

2032c1b

iximeow mentioned this pull request Sep 5, 2024

move smbios table creation to propolis (out of server+standalone) #759

Open

iximeow added 2 commits September 6, 2024 02:29

log plumbing

ca97278

shelve the first_boot_only attribute for a moment

d5e43c4

this could have been made to work, but i'm walking this back as it would land fairly untested at first

iximeow commented Sep 10, 2024

View reviewed changes

iximeow added 9 commits September 10, 2024 23:03

rustfmt and log tweaks

3f921f0

include actually-usable tests

e0c7c71

these still depend on having a guest artifact with an EFI partition to persist NvVars into, but with such a partition they actually show useful behavior!

actually include guest OS definitions for 24.04 (shouldn't land)

54bda84

functionally the guest adaptations for 22.04 and 24.04 are the same. these "24.04" definitions are actually in support of my weird 24.04 base image that has a user "root" instead of ubuntu-with-a-password

move all the EFI variable stuff to a module

cc485b0

this makes the tests kind of legible..

lose dependency on specific test images

7ddf735

fill out other half of test "boot_order_source_priority"

b112aa6

use the implicit boot-disk entry like every other test

f0a09db

use empty FAT filesystems for unbootable disks

ed4a3e8

don't *really* need the file present for any test purpose

log formatting consistency

7484eb4

iximeow commented Sep 13, 2024

View reviewed changes

openapi/propolis-server.json Outdated Show resolved Hide resolved

iximeow commented Sep 13, 2024

View reviewed changes

hawkw reviewed Sep 13, 2024

View reviewed changes

iximeow mentioned this pull request Sep 16, 2024

Add a boot device field to Instance, forward along as appropriate oxidecomputer/omicron#6585

Merged

iximeow added 3 commits September 17, 2024 01:59

review feedback: stray eprintln, include new boot_order in api-types

ed455ea

also rustfmt has different opinions now?

Merge remote-tracking branch 'github/master' into ixi/propolis-bootorder

ee46c91

extracted that into another PR

09583fb

iximeow added 13 commits September 25, 2024 00:27

resolve merge conflict with refactors

2e01169

this drops the untested support from booting from NICs as well

one less todo

9af0380

settle on an actual approach for the test "boot_disk" helper fn

617dd0c

ok, keep all tests passing with framework changes

fcfa0a2

rustfmt

247f1e3

speed limit 80

52aa0fe

dont want to wrap those lines, avoid the problem

c9a522b

rustfmt

bbeafcf

rustfmt

a636216

remove unneeded (and not-generally-correct) ubuntu 24.04 adapter

ab9743e

cargo clippy --workspace...

458b740

remove unnecessary warning about EFI partition

6d0077e

this test does not rely on an EFI partition existing at all!

fix migration tests for this non-API-breaking change

cee2adc

gjcolombo mentioned this pull request Sep 25, 2024

instance spec rework: flatten InstanceSpecV0 #767

Merged

gjcolombo approved these changes Sep 25, 2024

View reviewed changes

Update phd-tests/framework/src/test_vm/config.rs

93e33e4

Co-authored-by: Greg Colombo <[email protected]>

hawkw reviewed Sep 25, 2024

View reviewed changes

gjcolombo removed their assignment Sep 25, 2024

iximeow added 2 commits September 27, 2024 12:48

review feedback:

22b5a08

* a few typos * a few 80col violations * actually enforce that boot devices are on PCI bus 0 * emphasize that propolis-server should never see a Spec with invalid boot disk * vestigial line * phd_skip!() instead of warn!+return Ok

gjcolombo reviewed Sep 27, 2024

View reviewed changes

phd-tests/framework/src/test_vm/config.rs Outdated Show resolved Hide resolved

use newtypes to delineate DeviceName/BackendName in PHD

3a3e753

these newtypes are useful for other kinds of devices, and in propolis-api-types generally, but threading that through is more time than i can spend on this right now

iximeow force-pushed the ixi/propolis-bootorder branch from 59b6bfc to 3a3e753 Compare September 28, 2024 01:01

iximeow merged commit 11371b0 into master Sep 28, 2024
11 checks passed

iximeow deleted the ixi/propolis-bootorder branch September 28, 2024 01:55

This was referenced Sep 30, 2024

Prefer component IDs to names #772

Closed

long lines confuse PHD's serial listener #773

Open

iximeow mentioned this pull request Oct 7, 2024

propolis should allow for control over boot order #697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

teach propolis-server to understand configurable boot order #756

teach propolis-server to understand configurable boot order #756

iximeow commented Sep 3, 2024 •

edited

Loading

gjcolombo left a comment

iximeow left a comment

iximeow Sep 10, 2024

luqmana Sep 12, 2024

luqmana Sep 12, 2024

iximeow Sep 12, 2024

iximeow Sep 10, 2024

gjcolombo Sep 10, 2024

iximeow Sep 12, 2024

iximeow Sep 23, 2024

iximeow Sep 13, 2024 •

edited

Loading

hawkw Sep 13, 2024

iximeow Sep 13, 2024

gjcolombo Sep 16, 2024

hawkw Sep 13, 2024

hawkw Sep 13, 2024

iximeow commented Sep 16, 2024 •

edited

Loading

gjcolombo left a comment

hawkw Sep 25, 2024

hawkw Sep 25, 2024

hawkw Sep 25, 2024

hawkw Sep 25, 2024

hawkw commented Sep 28, 2024

teach propolis-server to understand configurable boot order #756

teach propolis-server to understand configurable boot order #756

Conversation

iximeow commented Sep 3, 2024 • edited Loading

gjcolombo left a comment

Choose a reason for hiding this comment

iximeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iximeow Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iximeow commented Sep 16, 2024 • edited Loading

gjcolombo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Sep 28, 2024

iximeow commented Sep 3, 2024 •

edited

Loading

iximeow Sep 13, 2024 •

edited

Loading

iximeow commented Sep 16, 2024 •

edited

Loading