From 30d41911f3682e21f34ec041a651c3f206600894 Mon Sep 17 00:00:00 2001 From: liffy <629075+lifning@users.noreply.github.com> Date: Tue, 28 Nov 2023 12:02:08 -0800 Subject: [PATCH] Refactor InstalledZone::install to use a builder pattern, per TODO. (#4325) Additionally, make a builder-factory with an option to create fake builders, in service of refactoring some things to enable some unit tests being written. --- Cargo.lock | 1 + illumos-utils/Cargo.toml | 1 + illumos-utils/src/running_zone.rs | 238 +++++++++++++++++++++++++---- sled-agent/src/instance.rs | 48 +++--- sled-agent/src/instance_manager.rs | 9 ++ sled-agent/src/services.rs | 41 ++--- sled-agent/src/sled_agent.rs | 2 + 7 files changed, 272 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 76107c8f4e..108c8b182d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3072,6 +3072,7 @@ dependencies = [ "bhyve_api", "byteorder", "camino", + "camino-tempfile", "cfg-if 1.0.0", "crucible-smf", "futures", diff --git a/illumos-utils/Cargo.toml b/illumos-utils/Cargo.toml index 497454e047..8296eace5c 100644 --- a/illumos-utils/Cargo.toml +++ b/illumos-utils/Cargo.toml @@ -11,6 +11,7 @@ async-trait.workspace = true bhyve_api.workspace = true byteorder.workspace = true camino.workspace = true +camino-tempfile.workspace = true cfg-if.workspace = true crucible-smf.workspace = true futures.workspace = true diff --git a/illumos-utils/src/running_zone.rs b/illumos-utils/src/running_zone.rs index ba8cd009e8..ea80a6d34b 100644 --- a/illumos-utils/src/running_zone.rs +++ b/illumos-utils/src/running_zone.rs @@ -11,10 +11,12 @@ use crate::opte::{Port, PortTicket}; use crate::svc::wait_for_service; use crate::zone::{AddressRequest, IPADM, ZONE_PREFIX}; use camino::{Utf8Path, Utf8PathBuf}; +use camino_tempfile::Utf8TempDir; use ipnetwork::IpNetwork; use omicron_common::backoff; use slog::{error, info, o, warn, Logger}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; +use std::sync::Arc; #[cfg(target_os = "illumos")] use std::sync::OnceLock; #[cfg(target_os = "illumos")] @@ -1043,7 +1045,7 @@ pub struct ServiceProcess { pub log_file: Utf8PathBuf, } -/// Errors returned from [`InstalledZone::install`]. +/// Errors returned from [`ZoneBuilder::install`]. #[derive(thiserror::Error, Debug)] pub enum InstallZoneError { #[error("Cannot create '{zone}': failed to create control VNIC: {err}")] @@ -1063,6 +1065,9 @@ pub enum InstallZoneError { #[error("Failed to find zone image '{image}' from {paths:?}")] ImageNotFound { image: String, paths: Vec }, + + #[error("Attempted to call install() on underspecified ZoneBuilder")] + IncompleteBuilder, } pub struct InstalledZone { @@ -1119,24 +1124,208 @@ impl InstalledZone { &self.zonepath } - // TODO: This would benefit from a "builder-pattern" interface. - #[allow(clippy::too_many_arguments)] - pub async fn install( - log: &Logger, - underlay_vnic_allocator: &VnicAllocator, - zone_root_path: &Utf8Path, - zone_image_paths: &[Utf8PathBuf], - zone_type: &str, - unique_name: Option, - datasets: &[zone::Dataset], - filesystems: &[zone::Fs], - data_links: &[String], - devices: &[zone::Device], - opte_ports: Vec<(Port, PortTicket)>, - bootstrap_vnic: Option, - links: Vec, - limit_priv: Vec, - ) -> Result { + pub fn site_profile_xml_path(&self) -> Utf8PathBuf { + let mut path: Utf8PathBuf = self.zonepath().into(); + path.push("root/var/svc/profile/site.xml"); + path + } +} + +#[derive(Clone)] +pub struct FakeZoneBuilderConfig { + temp_dir: Arc, +} + +#[derive(Clone, Default)] +pub struct ZoneBuilderFactory { + // Why this is part of this builder/factory and not some separate builder + // type: At time of writing, to the best of my knowledge: + // - If we want builder pattern, we need to return some type of `Self`. + // - If we have a trait that returns `Self` type, we can't turn it into a + // trait object (i.e. Box). + // - Plumbing concrete types as generics through every other type that + // needs to construct zones (and anything else with a lot of parameters) + // seems like a worse idea. + fake_cfg: Option, +} + +impl ZoneBuilderFactory { + /// For use in unit tests that don't require actual zone creation to occur. + pub fn fake() -> Self { + Self { + fake_cfg: Some(FakeZoneBuilderConfig { + temp_dir: Arc::new(Utf8TempDir::new().unwrap()), + }), + } + } + + /// Create a [ZoneBuilder] that inherits this factory's fakeness. + pub fn builder<'a>(&self) -> ZoneBuilder<'a> { + ZoneBuilder { fake_cfg: self.fake_cfg.clone(), ..Default::default() } + } +} + +/// Builder-pattern construct for creating an [InstalledZone]. +/// Created by [ZoneBuilderFactory]. +#[derive(Default)] +pub struct ZoneBuilder<'a> { + log: Option, + underlay_vnic_allocator: Option<&'a VnicAllocator>, + zone_root_path: Option<&'a Utf8Path>, + zone_image_paths: Option<&'a [Utf8PathBuf]>, + zone_type: Option<&'a str>, + unique_name: Option, // actually optional + datasets: Option<&'a [zone::Dataset]>, + filesystems: Option<&'a [zone::Fs]>, + data_links: Option<&'a [String]>, + devices: Option<&'a [zone::Device]>, + opte_ports: Option>, + bootstrap_vnic: Option, // actually optional + links: Option>, + limit_priv: Option>, + fake_cfg: Option, +} + +impl<'a> ZoneBuilder<'a> { + pub fn with_log(mut self, log: Logger) -> Self { + self.log = Some(log); + self + } + + pub fn with_underlay_vnic_allocator( + mut self, + vnic_allocator: &'a VnicAllocator, + ) -> Self { + self.underlay_vnic_allocator = Some(vnic_allocator); + self + } + + pub fn with_zone_root_path(mut self, root_path: &'a Utf8Path) -> Self { + self.zone_root_path = Some(root_path); + self + } + + pub fn with_zone_image_paths( + mut self, + image_paths: &'a [Utf8PathBuf], + ) -> Self { + self.zone_image_paths = Some(image_paths); + self + } + + pub fn with_zone_type(mut self, zone_type: &'a str) -> Self { + self.zone_type = Some(zone_type); + self + } + + pub fn with_unique_name(mut self, uuid: Uuid) -> Self { + self.unique_name = Some(uuid); + self + } + + pub fn with_datasets(mut self, datasets: &'a [zone::Dataset]) -> Self { + self.datasets = Some(datasets); + self + } + + pub fn with_filesystems(mut self, filesystems: &'a [zone::Fs]) -> Self { + self.filesystems = Some(filesystems); + self + } + + pub fn with_data_links(mut self, links: &'a [String]) -> Self { + self.data_links = Some(links); + self + } + + pub fn with_devices(mut self, devices: &'a [zone::Device]) -> Self { + self.devices = Some(devices); + self + } + + pub fn with_opte_ports(mut self, ports: Vec<(Port, PortTicket)>) -> Self { + self.opte_ports = Some(ports); + self + } + + pub fn with_bootstrap_vnic(mut self, vnic: Link) -> Self { + self.bootstrap_vnic = Some(vnic); + self + } + + pub fn with_links(mut self, links: Vec) -> Self { + self.links = Some(links); + self + } + + pub fn with_limit_priv(mut self, limit_priv: Vec) -> Self { + self.limit_priv = Some(limit_priv); + self + } + + fn fake_install(self) -> Result { + let zone = self + .zone_type + .ok_or(InstallZoneError::IncompleteBuilder)? + .to_string(); + let control_vnic = self + .underlay_vnic_allocator + .ok_or(InstallZoneError::IncompleteBuilder)? + .new_control(None) + .map_err(move |err| InstallZoneError::CreateVnic { zone, err })?; + let fake_cfg = self.fake_cfg.unwrap(); + let temp_dir = fake_cfg.temp_dir.path().to_path_buf(); + (|| { + let full_zone_name = InstalledZone::get_zone_name( + self.zone_type?, + self.unique_name, + ); + let zonepath = temp_dir + .join(self.zone_root_path?.strip_prefix("/").unwrap()) + .join(&full_zone_name); + let iz = InstalledZone { + log: self.log?, + zonepath, + name: full_zone_name, + control_vnic, + bootstrap_vnic: self.bootstrap_vnic, + opte_ports: self.opte_ports?, + links: self.links?, + }; + let xml_path = iz.site_profile_xml_path().parent()?.to_path_buf(); + std::fs::create_dir_all(&xml_path) + .unwrap_or_else(|_| panic!("ZoneBuilder::fake_install couldn't create site profile xml path {:?}", xml_path)); + Some(iz) + })() + .ok_or(InstallZoneError::IncompleteBuilder) + } + + pub async fn install(self) -> Result { + if self.fake_cfg.is_some() { + return self.fake_install(); + } + + let Self { + log: Some(log), + underlay_vnic_allocator: Some(underlay_vnic_allocator), + zone_root_path: Some(zone_root_path), + zone_image_paths: Some(zone_image_paths), + zone_type: Some(zone_type), + unique_name, + datasets: Some(datasets), + filesystems: Some(filesystems), + data_links: Some(data_links), + devices: Some(devices), + opte_ports: Some(opte_ports), + bootstrap_vnic, + links: Some(links), + limit_priv: Some(limit_priv), + .. + } = self + else { + return Err(InstallZoneError::IncompleteBuilder); + }; + let control_vnic = underlay_vnic_allocator.new_control(None).map_err(|err| { InstallZoneError::CreateVnic { @@ -1145,7 +1334,8 @@ impl InstalledZone { } })?; - let full_zone_name = Self::get_zone_name(zone_type, unique_name); + let full_zone_name = + InstalledZone::get_zone_name(zone_type, unique_name); // Looks for the image within `zone_image_path`, in order. let image = format!("{}.tar.gz", zone_type); @@ -1183,7 +1373,7 @@ impl InstalledZone { net_device_names.dedup(); Zones::install_omicron_zone( - log, + &log, &zone_root_path, &full_zone_name, &zone_image_path, @@ -1210,12 +1400,6 @@ impl InstalledZone { links, }) } - - pub fn site_profile_xml_path(&self) -> Utf8PathBuf { - let mut path: Utf8PathBuf = self.zonepath().into(); - path.push("root/var/svc/profile/site.xml"); - path - } } /// Return true if the service with the given FMRI appears to be an diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index a6f022f5f2..c37f0ffde6 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -26,7 +26,7 @@ use futures::lock::{Mutex, MutexGuard}; use illumos_utils::dladm::Etherstub; use illumos_utils::link::VnicAllocator; use illumos_utils::opte::{DhcpCfg, PortManager}; -use illumos_utils::running_zone::{InstalledZone, RunningZone}; +use illumos_utils::running_zone::{RunningZone, ZoneBuilderFactory}; use illumos_utils::svc::wait_for_service; use illumos_utils::zone::Zones; use illumos_utils::zone::PROPOLIS_ZONE_PREFIX; @@ -226,6 +226,9 @@ struct InstanceInner { // Storage resources storage: StorageHandle, + // Used to create propolis zones + zone_builder_factory: ZoneBuilderFactory, + // Object used to collect zone bundles from this instance when terminated. zone_bundler: ZoneBundler, @@ -611,6 +614,7 @@ impl Instance { port_manager, storage, zone_bundler, + zone_builder_factory, } = services; let mut dhcp_config = DhcpCfg { @@ -678,6 +682,7 @@ impl Instance { running_state: None, nexus_client, storage, + zone_builder_factory, zone_bundler, instance_ticket: ticket, }; @@ -904,31 +909,28 @@ impl Instance { .choose(&mut rng) .ok_or_else(|| Error::U2NotFound)? .clone(); - let installed_zone = InstalledZone::install( - &inner.log, - &inner.vnic_allocator, - &root, - &["/opt/oxide".into()], - "propolis-server", - Some(*inner.propolis_id()), - // dataset= - &[], - // filesystems= - &[], - // data_links= - &[], - &[ + let installed_zone = inner + .zone_builder_factory + .builder() + .with_log(inner.log.clone()) + .with_underlay_vnic_allocator(&inner.vnic_allocator) + .with_zone_root_path(&root) + .with_zone_image_paths(&["/opt/oxide".into()]) + .with_zone_type("propolis-server") + .with_unique_name(*inner.propolis_id()) + .with_datasets(&[]) + .with_filesystems(&[]) + .with_data_links(&[]) + .with_devices(&[ zone::Device { name: "/dev/vmm/*".to_string() }, zone::Device { name: "/dev/vmmctl".to_string() }, zone::Device { name: "/dev/viona".to_string() }, - ], - opte_ports, - // physical_nic= - None, - vec![], - vec![], - ) - .await?; + ]) + .with_opte_ports(opte_ports) + .with_links(vec![]) + .with_limit_priv(vec![]) + .install() + .await?; let gateway = inner.port_manager.underlay_ip(); diff --git a/sled-agent/src/instance_manager.rs b/sled-agent/src/instance_manager.rs index fa40a876f0..c1b7e402a4 100644 --- a/sled-agent/src/instance_manager.rs +++ b/sled-agent/src/instance_manager.rs @@ -17,6 +17,7 @@ use crate::zone_bundle::ZoneBundler; use illumos_utils::dladm::Etherstub; use illumos_utils::link::VnicAllocator; use illumos_utils::opte::PortManager; +use illumos_utils::running_zone::ZoneBuilderFactory; use illumos_utils::vmm_reservoir; use omicron_common::api::external::ByteCount; use omicron_common::api::internal::nexus::InstanceRuntimeState; @@ -76,6 +77,7 @@ struct InstanceManagerInternal { port_manager: PortManager, storage: StorageHandle, zone_bundler: ZoneBundler, + zone_builder_factory: ZoneBuilderFactory, } pub(crate) struct InstanceManagerServices { @@ -84,6 +86,7 @@ pub(crate) struct InstanceManagerServices { pub port_manager: PortManager, pub storage: StorageHandle, pub zone_bundler: ZoneBundler, + pub zone_builder_factory: ZoneBuilderFactory, } /// All instances currently running on the sled. @@ -100,6 +103,7 @@ impl InstanceManager { port_manager: PortManager, storage: StorageHandle, zone_bundler: ZoneBundler, + zone_builder_factory: ZoneBuilderFactory, ) -> Result { Ok(InstanceManager { inner: Arc::new(InstanceManagerInternal { @@ -113,6 +117,7 @@ impl InstanceManager { port_manager, storage, zone_bundler, + zone_builder_factory, }), }) } @@ -266,6 +271,10 @@ impl InstanceManager { port_manager: self.inner.port_manager.clone(), storage: self.inner.storage.clone(), zone_bundler: self.inner.zone_bundler.clone(), + zone_builder_factory: self + .inner + .zone_builder_factory + .clone(), }; let state = crate::instance::InstanceInitialState { diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index b87c91768b..2caa640e22 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -53,7 +53,7 @@ use illumos_utils::dladm::{ use illumos_utils::link::{Link, VnicAllocator}; use illumos_utils::opte::{DhcpCfg, Port, PortManager, PortTicket}; use illumos_utils::running_zone::{ - InstalledZone, RunCommandError, RunningZone, + InstalledZone, RunCommandError, RunningZone, ZoneBuilderFactory, }; use illumos_utils::zfs::ZONE_ZFS_RAMDISK_DATASET_MOUNTPOINT; use illumos_utils::zone::AddressRequest; @@ -1103,23 +1103,28 @@ impl ServiceManager { .push(boot_zpool.dataset_mountpoint(INSTALL_DATASET)); } - let installed_zone = InstalledZone::install( - &self.inner.log, - &self.inner.underlay_vnic_allocator, - &request.root, - zone_image_paths.as_slice(), - &request.zone.zone_type.to_string(), - unique_name, - datasets.as_slice(), - &filesystems, - &data_links, - &devices, - opte_ports, - bootstrap_vnic, - links, - limit_priv, - ) - .await?; + let mut zone_builder = ZoneBuilderFactory::default().builder(); + if let Some(uuid) = unique_name { + zone_builder = zone_builder.with_unique_name(uuid); + } + if let Some(vnic) = bootstrap_vnic { + zone_builder = zone_builder.with_bootstrap_vnic(vnic); + } + let installed_zone = zone_builder + .with_log(self.inner.log.clone()) + .with_underlay_vnic_allocator(&self.inner.underlay_vnic_allocator) + .with_zone_root_path(&request.root) + .with_zone_image_paths(zone_image_paths.as_slice()) + .with_zone_type(&request.zone.zone_type.to_string()) + .with_datasets(datasets.as_slice()) + .with_filesystems(&filesystems) + .with_data_links(&data_links) + .with_devices(&devices) + .with_opte_ports(opte_ports) + .with_links(links) + .with_limit_priv(limit_priv) + .install() + .await?; // TODO(https://github.com/oxidecomputer/omicron/issues/1898): // diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index cfa8c5d7ca..f5b71106cd 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -68,6 +68,7 @@ use std::sync::Arc; use tokio::sync::oneshot; use uuid::Uuid; +use illumos_utils::running_zone::ZoneBuilderFactory; #[cfg(not(test))] use illumos_utils::{dladm::Dladm, zone::Zones}; #[cfg(test)] @@ -382,6 +383,7 @@ impl SledAgent { port_manager.clone(), storage_manager.clone(), long_running_task_handles.zone_bundler.clone(), + ZoneBuilderFactory::default(), )?; // Configure the VMM reservoir as either a percentage of DRAM or as an