Skip to content

Commit

Permalink
review feedback:
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
iximeow committed Sep 27, 2024
1 parent ba45929 commit 22b5a08
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 39 deletions.
26 changes: 19 additions & 7 deletions bin/propolis-server/src/lib/initializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1041,26 +1041,38 @@ impl<'a> MachineInitializer<'a> {
match &spec.device_spec {
StorageDevice::Virtio(disk) => {
let bdf = parse_bdf(&disk.pci_path)?;
// TODO: check that bus is 0. only support boot devices
// directly attached to the root bus for now.
if bdf.bus.get() != 0 {
return Err(Error::new(
ErrorKind::InvalidInput,
"Boot device currently must be on PCI bus 0",
));
}

order.add_disk(bdf.location);
}
StorageDevice::Nvme(disk) => {
let bdf = parse_bdf(&disk.pci_path)?;
// TODO: check that bus is 0. only support boot devices
// directly attached to the root bus for now.
//
if bdf.bus.get() != 0 {
return Err(Error::new(
ErrorKind::InvalidInput,
"Boot device currently must be on PCI bus 0",
));
}

// TODO: separately, propolis-standalone passes an eui64
// of 0, so do that here too. is that.. ok?
order.add_nvme(bdf.location, 0);
}
};
} else {
// This should be unreachable - we check that the boot disk is
// valid when constructing the spec we're initializing from.
let message = format!(
"Instance spec included boot entry which does not refer to an existing disk: `{}`",
"Instance spec included boot entry which does not refer \
to an existing disk: `{}`",
boot_entry.name.as_str(),
);
return Err(Error::new(ErrorKind::Other, message));
return Err(Error::new(ErrorKind::InvalidInput, message));
}
}

Expand Down
14 changes: 8 additions & 6 deletions lib/propolis-client/src/instance_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,16 @@ impl SpecBuilderV0 {

/// Sets a boot order. Names here refer to devices included in this spec.
///
/// Permissible to not this if the implicit boot order is desired, but the implicit boot order
/// may be unstable across device addition and removal.
/// Permissible to not set this if the implicit boot order is desired, but
/// the implicit boot order may be unstable across device addition and
/// removal.
///
/// If any devices named in this order are not actually present in the constructed spec,
/// Propolis will return an error when the spec is provided.
/// If any devices named in this order are not actually present in the
/// constructed spec, Propolis will return an error when the spec is
/// provided.
///
/// XXX: this should certainly return `&mut Self` - all the builders here should. check if any
/// of these are chained..?
/// XXX: this should certainly return `&mut Self` - all the builders here
/// should. check if any of these are chained..?
pub fn set_boot_order(
&mut self,
boot_order: Vec<String>,
Expand Down
51 changes: 30 additions & 21 deletions phd-tests/framework/src/test_vm/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,11 @@ impl<'dr> VmConfig<'dr> {
self
}

/// Add a new disk to the VM config, and add it to the front of the VM's boot order.
/// Add a new disk to the VM config, and add it to the front of the VM's
/// boot order.
///
/// The added disk will have the name `boot-disk`, and replace the previous existing
/// `boot-disk`.
/// The added disk will have the name `boot-disk`, and replace the previous
/// existing `boot-disk`.
pub fn boot_disk(
&mut self,
artifact: &'dr str,
Expand Down Expand Up @@ -207,28 +208,37 @@ impl<'dr> VmConfig<'dr> {
})
.context("serializing Propolis server config")?;

// The first disk in the boot list might not be the disk a test *actually* expects to boot.
// 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.
// 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.
let boot_disk =
self.disks.iter().find(|d| d.name == "boot-disk")
.or_else(|| if let Some(boot_order) = self.boot_order.as_ref() {
boot_order.first().and_then(|name| self.disks.iter().find(|d| &d.name == name))
// 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.
let boot_disk = self
.disks
.iter()
.find(|d| d.name == "boot-disk")
.or_else(|| {
if let Some(boot_order) = self.boot_order.as_ref() {
boot_order.first().and_then(|name| {
self.disks.iter().find(|d| &d.name == name)
})
} else {
None
})
.or_else(|| self.disks.first())
.expect("VM config includes at least one disk (and maybe a boot order)?");
}
})
.or_else(|| self.disks.first())
.expect("VM config includes at least one disk");

// XXX: assuming all bootable images are equivalent to the first, or at least the same
// guest OS kind.
// XXX: assuming all bootable images are equivalent to the first, or at
// least the same guest OS kind.
let DiskSource::Artifact(boot_artifact) = boot_disk.source else {
unreachable!("boot disks always use artifacts as sources");
};
Expand All @@ -242,7 +252,6 @@ impl<'dr> VmConfig<'dr> {
let mut disk_handles = Vec::new();
for disk in self.disks.iter() {
disk_handles.push(
// format!("data-disk-{}", idx)
make_disk(disk.name.to_owned(), framework, disk)
.await
.context("creating disk")?,
Expand Down
6 changes: 1 addition & 5 deletions phd-tests/tests/src/boot_order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,11 +353,7 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) {
write_efivar(&vm, &bootvar(0xfff0), &boot_option_bytes).await?;
let reread = read_efivar(&vm, &bootvar(0xfff0)).await?;
if reread.is_empty() {
warn!(
"guest environment drops EFI variable writes! \
exiting test WITHOUT VALIDATING ANYTHING"
);
return Ok(());
phd_skip!("Guest environment drops EFI variable writes");
} else {
assert_eq!(
boot_option_bytes,
Expand Down

0 comments on commit 22b5a08

Please sign in to comment.