Skip to content

Commit

Permalink
instance spec rework: tighten up component naming (#761)
Browse files Browse the repository at this point in the history 
Fix a few problems with the way components are named in propolis-server
`Spec`s:

- Don't duplicate backend names in disk and NIC elements
- Use names as keys in the serial port map instead of deriving names from
  serial port numbers
- Properly register the names of bridges when adding them to the spec
  builder
- Make the builder reject requests to add a device/backend pair where the
  device and backend have the same name (and stop requesting this for
  cloud-init disks...)
  • Loading branch information
@gjcolombo
gjcolombo authored Sep 17, 2024
1 parent eaef107 commit 004df03
Show file tree
Hide file tree
Showing 7 changed files with 185 additions and 159 deletions.
27 changes: 8 additions & 19 deletions bin/propolis-server/src/lib/initializer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,34 +298,23 @@ impl<'a> MachineInitializer<'a> {
chipset: &RegisteredChipset,
) -> Result<Serial<LpcUart>, Error> {
let mut com1 = None;

// Create UART devices for all of the serial ports in the spec that
// requested one.
for (num, user) in self.spec.serial.iter() {
if *user != spec::SerialPortDevice::Uart {
for (name, desc) in self.spec.serial.iter() {
if desc.device != spec::SerialPortDevice::Uart {
continue;
}

let (name, irq, port) = match num {
SerialPortNumber::Com1 => {
("com1", ibmpc::IRQ_COM1, ibmpc::PORT_COM1)
}
SerialPortNumber::Com2 => {
("com2", ibmpc::IRQ_COM2, ibmpc::PORT_COM2)
}
SerialPortNumber::Com3 => {
("com3", ibmpc::IRQ_COM3, ibmpc::PORT_COM3)
}
SerialPortNumber::Com4 => {
("com4", ibmpc::IRQ_COM4, ibmpc::PORT_COM4)
}
let (irq, port) = match desc.num {
SerialPortNumber::Com1 => (ibmpc::IRQ_COM1, ibmpc::PORT_COM1),
SerialPortNumber::Com2 => (ibmpc::IRQ_COM2, ibmpc::PORT_COM2),
SerialPortNumber::Com3 => (ibmpc::IRQ_COM3, ibmpc::PORT_COM3),
SerialPortNumber::Com4 => (ibmpc::IRQ_COM4, ibmpc::PORT_COM4),
};

let dev = LpcUart::new(chipset.irq_pin(irq).unwrap());
dev.set_autodiscard(true);
LpcUart::attach(&dev, &self.machine.bus_pio, port);
self.devices.insert(name.to_owned(), dev.clone());
if *num == SerialPortNumber::Com1 {
if desc.num == SerialPortNumber::Com1 {
assert!(com1.is_none());
com1 = Some(dev);
}
Expand Down
14 changes: 7 additions & 7 deletions bin/propolis-server/src/lib/server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,19 +133,19 @@ fn instance_spec_from_request(
spec_builder.add_cloud_init_from_request(base64.clone())?;
}

for port in [
instance_spec::components::devices::SerialPortNumber::Com1,
instance_spec::components::devices::SerialPortNumber::Com2,
instance_spec::components::devices::SerialPortNumber::Com3,
for (name, port) in [
("com1", instance_spec::components::devices::SerialPortNumber::Com1),
("com2", instance_spec::components::devices::SerialPortNumber::Com2),
("com3", instance_spec::components::devices::SerialPortNumber::Com3),
// SoftNpu uses this port for ASIC management.
#[cfg(not(feature = "falcon"))]
instance_spec::components::devices::SerialPortNumber::Com4,
("com4", instance_spec::components::devices::SerialPortNumber::Com4),
] {
spec_builder.add_serial_port(port)?;
spec_builder.add_serial_port(name.to_owned(), port)?;
}

#[cfg(feature = "falcon")]
spec_builder.set_softnpu_com4()?;
spec_builder.set_softnpu_com4("com4".to_owned())?;

spec_builder.add_pvpanic_device(spec::QemuPvpanic {
name: "pvpanic".to_string(),
Expand Down
85 changes: 11 additions & 74 deletions bin/propolis-server/src/lib/spec/api_request.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,13 @@ pub(super) fn parse_disk_from_request(
disk: &DiskRequest,
) -> Result<ParsedDiskRequest, DeviceRequestError> {
let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?;
let device_name = disk.name.clone();
let backend_name = format!("{}-backend", disk.name);
let device_spec = match disk.device.as_ref() {
"virtio" => StorageDevice::Virtio(VirtioDisk {
backend_name: backend_name.clone(),
pci_path,
}),
"nvme" => StorageDevice::Nvme(NvmeDisk {
backend_name: backend_name.clone(),
pci_path,
}),
"virtio" => {
StorageDevice::Virtio(VirtioDisk { backend_name, pci_path })
}
"nvme" => StorageDevice::Nvme(NvmeDisk { backend_name, pci_path }),
_ => {
return Err(DeviceRequestError::InvalidStorageInterface(
disk.device.clone(),
Expand All @@ -90,7 +87,6 @@ pub(super) fn parse_disk_from_request(
}
};

let device_name = disk.name.clone();
let backend_spec = StorageBackend::Crucible(CrucibleStorageBackend {
request_json: serde_json::to_string(&disk.volume_construction_request)
.map_err(|e| {
Expand All @@ -101,7 +97,7 @@ pub(super) fn parse_disk_from_request(

Ok(ParsedDiskRequest {
name: device_name,
disk: Disk { device_spec, backend_name, backend_spec },
disk: Disk { device_spec, backend_spec },
})
}

Expand All @@ -110,18 +106,16 @@ pub(super) fn parse_cloud_init_from_request(
) -> Result<ParsedDiskRequest, DeviceRequestError> {
let name = "cloud-init";
let pci_path = slot_to_pci_path(Slot(0), SlotType::CloudInit)?;
let backend_name = name.to_string();
let backend_name = "cloud-init-backend".to_string();
let backend_spec =
StorageBackend::Blob(BlobStorageBackend { base64, readonly: true });

let device_spec = StorageDevice::Virtio(VirtioDisk {
backend_name: name.to_string(),
pci_path,
});
let device_spec =
StorageDevice::Virtio(VirtioDisk { backend_name, pci_path });

Ok(ParsedDiskRequest {
name: name.to_owned(),
disk: Disk { device_spec, backend_name, backend_spec },
disk: Disk { device_spec, backend_spec },
})
}

Expand All @@ -139,63 +133,6 @@ pub(super) fn parse_nic_from_request(
let backend_spec = VirtioNetworkBackend { vnic_name: nic.name.to_string() };
Ok(ParsedNicRequest {
name: device_name,
nic: Nic { device_spec, backend_name, backend_spec },
nic: Nic { device_spec, backend_spec },
})
}

#[cfg(test)]
mod test {
use propolis_api_types::VolumeConstructionRequest;
use uuid::Uuid;

use super::*;

fn check_parsed_storage_device_backend_pointer(parsed: &ParsedDiskRequest) {
let device_to_backend = match &parsed.disk.device_spec {
StorageDevice::Virtio(d) => d.backend_name.clone(),
StorageDevice::Nvme(d) => d.backend_name.clone(),
};

assert_eq!(device_to_backend, parsed.disk.backend_name);
}

#[test]
fn parsed_disk_devices_point_to_backends() {
let vcr = VolumeConstructionRequest::File {
id: Uuid::nil(),
block_size: 512,
path: "".to_string(),
};

let req = DiskRequest {
name: "my-disk".to_string(),
slot: Slot(0),
read_only: false,
device: "nvme".to_string(),
volume_construction_request: vcr,
};

let parsed = parse_disk_from_request(&req).unwrap();
check_parsed_storage_device_backend_pointer(&parsed);
}

#[test]
fn parsed_network_devices_point_to_backends() {
let req = NetworkInterfaceRequest {
name: "vnic".to_string(),
interface_id: uuid::Uuid::new_v4(),
slot: Slot(0),
};

let parsed = parse_nic_from_request(&req).unwrap();
let VirtioNic { backend_name, .. } = &parsed.nic.device_spec;
assert_eq!(*backend_name, parsed.nic.backend_name);
}

#[test]
fn parsed_cloud_init_devices_point_to_backends() {
let base64 = "AAAAAAAAAAAAAAAAAAAAAAAAAAA".to_string();
let parsed = parse_cloud_init_from_request(base64).unwrap();
check_parsed_storage_device_backend_pointer(&parsed);
}
}
35 changes: 14 additions & 21 deletions bin/propolis-server/src/lib/spec/api_spec_v0.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use std::collections::HashMap;

use propolis_api_types::instance_spec::{
components::devices::{SerialPort as SerialPortDesc, SerialPortNumber},
components::devices::SerialPort as SerialPortDesc,
v0::{InstanceSpecV0, NetworkBackendV0, NetworkDeviceV0, StorageDeviceV0},
};
use thiserror::Error;
Expand Down Expand Up @@ -68,6 +68,7 @@ impl From<Spec> for InstanceSpecV0 {
let mut spec = InstanceSpecV0::default();
spec.devices.board = val.board;
for (disk_name, disk) in val.disks {
let backend_name = disk.device_spec.backend_name().to_owned();
insert_component(
&mut spec.devices.storage_devices,
disk_name,
Expand All @@ -76,12 +77,13 @@ impl From<Spec> for InstanceSpecV0 {

insert_component(
&mut spec.backends.storage_backends,
disk.backend_name,
backend_name,
disk.backend_spec.into(),
);
}

for (nic_name, nic) in val.nics {
let backend_name = nic.device_spec.backend_name.clone();
insert_component(
&mut spec.devices.network_devices,
nic_name,
Expand All @@ -90,24 +92,17 @@ impl From<Spec> for InstanceSpecV0 {

insert_component(
&mut spec.backends.network_backends,
nic.backend_name,
backend_name,
NetworkBackendV0::Virtio(nic.backend_spec),
);
}

for (num, user) in val.serial.iter() {
if *user == SerialPortDevice::Uart {
let name = match num {
SerialPortNumber::Com1 => "com1",
SerialPortNumber::Com2 => "com2",
SerialPortNumber::Com3 => "com3",
SerialPortNumber::Com4 => "com4",
};

for (name, desc) in val.serial {
if desc.device == SerialPortDevice::Uart {
insert_component(
&mut spec.devices.serial_ports,
name.to_owned(),
SerialPortDesc { num: *num },
name,
SerialPortDesc { num: desc.num },
);
}
}
Expand Down Expand Up @@ -163,7 +158,7 @@ impl TryFrom<InstanceSpecV0> for Spec {
StorageDeviceV0::NvmeDisk(disk) => &disk.backend_name,
};

let (backend_name, backend_spec) = value
let (_, backend_spec) = value
.backends
.storage_backends
.remove_entry(backend_name)
Expand All @@ -176,7 +171,6 @@ impl TryFrom<InstanceSpecV0> for Spec {
device_name,
Disk {
device_spec: device_spec.into(),
backend_name,
backend_spec: backend_spec.into(),
},
)?;
Expand All @@ -192,7 +186,7 @@ impl TryFrom<InstanceSpecV0> for Spec {
for (device_name, device_spec) in value.devices.network_devices {
let NetworkDeviceV0::VirtioNic(device_spec) = device_spec;
let backend_name = &device_spec.backend_name;
let (backend_name, backend_spec) = value
let (_, backend_spec) = value
.backends
.network_backends
.remove_entry(backend_name)
Expand All @@ -207,7 +201,7 @@ impl TryFrom<InstanceSpecV0> for Spec {

builder.add_network_device(
device_name,
Nic { device_spec, backend_name, backend_spec },
Nic { device_spec, backend_spec },
)?;
}

Expand Down Expand Up @@ -253,9 +247,8 @@ impl TryFrom<InstanceSpecV0> for Spec {
return Err(ApiSpecError::BackendNotUsed(backend.to_owned()));
}

// TODO(#735): Serial ports need to have names like other devices.
for serial_port in value.devices.serial_ports.values() {
builder.add_serial_port(serial_port.num)?;
for (name, serial_port) in value.devices.serial_ports {
builder.add_serial_port(name, serial_port.num)?;
}

for (name, bridge) in value.devices.pci_pci_bridges {
Expand Down
Loading

0 comments on commit 004df03

Please sign in to comment.