Skip to content

Commit

Permalink
refactor: don't duplicate backend names
Browse files Browse the repository at this point in the history
  • Loading branch information
@gjcolombo
gjcolombo committed Sep 11, 2024
1 parent 0cb04bb commit 67dc7e4
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 99 deletions.
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);
}
}
13 changes: 7 additions & 6 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 @@ -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,7 +92,7 @@ impl From<Spec> for InstanceSpecV0 {

insert_component(
&mut spec.backends.network_backends,
nic.backend_name,
backend_name,
NetworkBackendV0::Virtio(nic.backend_spec),
);
}
Expand Down Expand Up @@ -156,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 @@ -169,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 @@ -185,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 @@ -200,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
14 changes: 8 additions & 6 deletions bin/propolis-server/src/lib/spec/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,15 +195,15 @@ impl SpecBuilder {
return Err(SpecBuilderError::ComponentNameInUse(disk_name));
}

if self.component_names.contains(&disk.backend_name) {
if self.component_names.contains(disk.device_spec.backend_name()) {
return Err(SpecBuilderError::ComponentNameInUse(
disk.backend_name,
disk.device_spec.backend_name().to_owned(),
));
}

self.register_pci_device(disk.device_spec.pci_path())?;
self.component_names.insert(disk_name.clone());
self.component_names.insert(disk.backend_name.clone());
self.component_names.insert(disk.device_spec.backend_name().to_owned());
let _old = self.spec.disks.insert(disk_name, disk);
assert!(_old.is_none());
Ok(self)
Expand All @@ -219,13 +219,15 @@ impl SpecBuilder {
return Err(SpecBuilderError::ComponentNameInUse(nic_name));
}

if self.component_names.contains(&nic.backend_name) {
return Err(SpecBuilderError::ComponentNameInUse(nic.backend_name));
if self.component_names.contains(&nic.device_spec.backend_name) {
return Err(SpecBuilderError::ComponentNameInUse(
nic.device_spec.backend_name,
));
}

self.register_pci_device(nic.device_spec.pci_path)?;
self.component_names.insert(nic_name.clone());
self.component_names.insert(nic.backend_name.clone());
self.component_names.insert(nic.device_spec.backend_name.clone());
let _old = self.spec.nics.insert(nic_name, nic);
assert!(_old.is_none());
Ok(self)
Expand Down
16 changes: 5 additions & 11 deletions bin/propolis-server/src/lib/spec/config_toml.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,29 +113,23 @@ impl TryFrom<&config::Config> for ParsedConfig {
let device_spec =
parse_storage_device_from_config(device_name, device)?;

let backend_name = match &device_spec {
StorageDevice::Virtio(disk) => {
disk.backend_name.clone()
}
StorageDevice::Nvme(disk) => disk.backend_name.clone(),
};

let backend_name = device_spec.backend_name();
let backend_config =
config.block_devs.get(&backend_name).ok_or_else(
config.block_devs.get(backend_name).ok_or_else(
|| ConfigTomlError::StorageDeviceBackendNotFound {
device: device_name.to_owned(),
backend: backend_name.to_owned(),
},
)?;

let backend_spec = parse_storage_backend_from_config(
&backend_name,
backend_name,
backend_config,
)?;

parsed.disks.push(ParsedDiskRequest {
name: device_name.to_owned(),
disk: Disk { device_spec, backend_name, backend_spec },
disk: Disk { device_spec, backend_spec },
});
}
"pci-virtio-viona" => {
Expand Down Expand Up @@ -298,7 +292,7 @@ pub(super) fn parse_network_device_from_config(

Ok(ParsedNicRequest {
name: device_name,
nic: Nic { device_spec, backend_name, backend_spec },
nic: Nic { device_spec, backend_spec },
})
}

Expand Down
9 changes: 7 additions & 2 deletions bin/propolis-server/src/lib/spec/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ impl StorageDevice {
StorageDevice::Nvme(disk) => disk.pci_path,
}
}

pub fn backend_name(&self) -> &str {
match self {
StorageDevice::Virtio(disk) => &disk.backend_name,
StorageDevice::Nvme(disk) => &disk.backend_name,
}
}
}

impl From<StorageDevice> for StorageDeviceV0 {
Expand Down Expand Up @@ -132,14 +139,12 @@ impl From<StorageBackendV0> for StorageBackend {
#[derive(Clone, Debug)]
pub struct Disk {
pub device_spec: StorageDevice,
pub backend_name: String,
pub backend_spec: StorageBackend,
}

#[derive(Clone, Debug)]
pub struct Nic {
pub device_spec: VirtioNic,
pub backend_name: String,
pub backend_spec: VirtioNetworkBackend,
}

Expand Down

0 comments on commit 67dc7e4

Please sign in to comment.