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

instance spec rework: remove most dependencies on InstanceSpecV0 from propolis-server #757

Merged
merged 11 commits into from
Sep 11, 2024
17 changes: 10 additions & 7 deletions bin/propolis-server/src/lib/initializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,11 @@ 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::SerialPortUser::Standard {
if *user != spec::SerialPortDevice::Uart {
continue;
}

Expand Down Expand Up @@ -509,12 +512,12 @@ impl<'a> MachineInitializer<'a> {
Nvme,
}

'devloop: for (disk_name, disk) in &self.spec.disks {
for (disk_name, disk) in &self.spec.disks {
info!(
self.log,
"Creating storage device {} with properties {:?}",
disk_name,
disk.device_spec
"Creating storage device";
"name" => disk_name,
"spec" => ?disk.device_spec
);

let (device_interface, backend_name, pci_path) = match &disk
Expand Down Expand Up @@ -591,7 +594,7 @@ impl<'a> MachineInitializer<'a> {
virtual disk metrics can't be reported for it";
"disk_id" => %disk_id,
);
continue 'devloop;
continue;
};

// Register the block device as a metric producer, if we've been
Expand All @@ -612,7 +615,7 @@ impl<'a> MachineInitializer<'a> {
"disk_id" => %disk_id,
"error" => ?e,
);
continue 'devloop;
continue;
};

// Set the on-completion callback for the block device, to
Expand Down
4 changes: 2 additions & 2 deletions bin/propolis-server/src/lib/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
serial::history_buffer::SerialHistoryOffset,
spec::{
self,
api_spec_v0::ApiSpecParseError,
api_spec_v0::ApiSpecError,
builder::{SpecBuilder, SpecBuilderError},
Spec,
},
Expand Down Expand Up @@ -325,7 +325,7 @@ async fn instance_spec_ensure(
) -> Result<HttpResponseCreated<api::InstanceEnsureResponse>, HttpError> {
let request = request.into_inner();
let VersionedInstanceSpec::V0(v0_spec) = request.instance_spec;
let spec = Spec::try_from(v0_spec).map_err(|e: ApiSpecParseError| {
let spec = Spec::try_from(v0_spec).map_err(|e: ApiSpecError| {
HttpError::for_bad_request(None, e.to_string())
})?;

Expand Down
26 changes: 12 additions & 14 deletions bin/propolis-server/src/lib/spec/api_spec_v0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ use crate::spec::SoftNpuPort;

use super::{
builder::{SpecBuilder, SpecBuilderError},
Disk, Nic, QemuPvpanic, SerialPortUser, Spec,
Disk, Nic, QemuPvpanic, SerialPortDevice, Spec,
};

#[derive(Debug, Error)]
pub(crate) enum ApiSpecParseError {
pub(crate) enum ApiSpecError {
#[error(transparent)]
BuilderError(#[from] SpecBuilderError),
Builder(#[from] SpecBuilderError),

#[error("backend {backend} not found for device {device}")]
BackendNotFound { backend: String, device: String },
Expand Down Expand Up @@ -79,7 +79,7 @@ impl From<Spec> for InstanceSpecV0 {
}

for (num, user) in val.serial.iter() {
if *user == SerialPortUser::Standard {
if *user == SerialPortDevice::Uart {
let name = match num {
SerialPortNumber::Com1 => "com1",
SerialPortNumber::Com2 => "com2",
Expand Down Expand Up @@ -133,7 +133,7 @@ impl From<Spec> for InstanceSpecV0 {
}

impl TryFrom<InstanceSpecV0> for Spec {
type Error = ApiSpecParseError;
type Error = ApiSpecError;

fn try_from(mut value: InstanceSpecV0) -> Result<Self, Self::Error> {
let mut builder = SpecBuilder::with_board(value.devices.board);
Expand All @@ -150,7 +150,7 @@ impl TryFrom<InstanceSpecV0> for Spec {
.backends
.storage_backends
.remove_entry(backend_name)
.ok_or_else(|| ApiSpecParseError::BackendNotFound {
.ok_or_else(|| ApiSpecError::BackendNotFound {
backend: backend_name.to_owned(),
device: device_name.clone(),
})?;
Expand All @@ -168,7 +168,7 @@ impl TryFrom<InstanceSpecV0> for Spec {
// Once all the devices have been checked, there should be no unpaired
// backends remaining.
if let Some(backend) = value.backends.storage_backends.keys().next() {
return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned()));
return Err(ApiSpecError::BackendNotUsed(backend.to_owned()));
}

// Repeat this process for network devices.
Expand All @@ -179,15 +179,13 @@ impl TryFrom<InstanceSpecV0> for Spec {
.backends
.network_backends
.remove_entry(backend_name)
.ok_or_else(|| ApiSpecParseError::BackendNotFound {
.ok_or_else(|| ApiSpecError::BackendNotFound {
backend: backend_name.to_owned(),
device: device_name.clone(),
})?;

let NetworkBackendV0::Virtio(backend_spec) = backend_spec else {
return Err(ApiSpecParseError::GuestNicInvalidBackend(
device_name,
));
return Err(ApiSpecError::GuestNicInvalidBackend(device_name));
};

builder.add_network_device(
Expand Down Expand Up @@ -218,13 +216,13 @@ impl TryFrom<InstanceSpecV0> for Spec {
.backends
.network_backends
.remove_entry(&port.backend_name)
.ok_or_else(|| ApiSpecParseError::BackendNotFound {
.ok_or_else(|| ApiSpecError::BackendNotFound {
backend: port.backend_name,
device: port_name.clone(),
})?;

let NetworkBackendV0::Dlpi(backend_spec) = backend_spec else {
return Err(ApiSpecParseError::NotDlpiBackend(port_name));
return Err(ApiSpecError::NotDlpiBackend(port_name));
};

builder.add_softnpu_port(
Expand All @@ -235,7 +233,7 @@ impl TryFrom<InstanceSpecV0> for Spec {
}

if let Some(backend) = value.backends.network_backends.keys().next() {
return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned()));
return Err(ApiSpecError::BackendNotUsed(backend.to_owned()));
}

// TODO(#735): Serial ports need to have names like other devices.
Expand Down
6 changes: 3 additions & 3 deletions bin/propolis-server/src/lib/spec/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use propolis_api_types::instance_spec::components::devices::{
P9fs, SoftNpuP9, SoftNpuPciPort,
};

use crate::{config, spec::SerialPortUser};
use crate::{config, spec::SerialPortDevice};

use super::{
api_request::{self, DeviceRequestError},
Expand Down Expand Up @@ -251,7 +251,7 @@ impl SpecBuilder {
&mut self,
port: SerialPortNumber,
) -> Result<&Self, SpecBuilderError> {
if self.spec.serial.insert(port, SerialPortUser::Standard).is_some() {
if self.spec.serial.insert(port, SerialPortDevice::Uart).is_some() {
Err(SpecBuilderError::SerialPortInUse(port))
} else {
Ok(self)
Expand All @@ -273,7 +273,7 @@ impl SpecBuilder {
#[cfg(feature = "falcon")]
pub fn set_softnpu_com4(&mut self) -> Result<&Self, SpecBuilderError> {
let port = SerialPortNumber::Com4;
if self.spec.serial.insert(port, SerialPortUser::SoftNpu).is_some() {
if self.spec.serial.insert(port, SerialPortDevice::SoftNpu).is_some() {
Err(SpecBuilderError::SerialPortInUse(port))
} else {
Ok(self)
Expand Down
7 changes: 4 additions & 3 deletions bin/propolis-server/src/lib/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub(crate) struct Spec {
pub disks: HashMap<String, Disk>,
pub nics: HashMap<String, Nic>,

pub serial: HashMap<SerialPortNumber, SerialPortUser>,
pub serial: HashMap<SerialPortNumber, SerialPortDevice>,

pub pci_pci_bridges: HashMap<String, PciPciBridge>,
pub pvpanic: Option<QemuPvpanic>,
Expand Down Expand Up @@ -143,9 +143,10 @@ pub struct Nic {
pub backend_spec: VirtioNetworkBackend,
}

/// A kind of device to install as the listener on a COM port.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum SerialPortUser {
Standard,
pub enum SerialPortDevice {
Uart,
gjcolombo marked this conversation as resolved.
Show resolved Hide resolved

#[cfg(feature = "falcon")]
SoftNpu,
Expand Down