[sled-agent] Clarify PathInRoot::zpool (#7428)

jgallagher · web-flow · commit 2efb9d5c45fa · 2025-01-30T11:46:50.000-05:00
Fixing #7229 requires sled-agent to know, for any installed zone, what zpool was chosen for that zone's root. It looks like this is tracked by `InstalledZone::zonepath`: https://github.com/oxidecomputer/omicron/blob/5fa0d8e50fd6ab36c211f3f71bebeab3ccf1860c/illumos-utils/src/running_zone.rs#L917-L918 except the `ZpoolName` of `PathInPool` is optional: https://github.com/oxidecomputer/omicron/blob/5fa0d8e50fd6ab36c211f3f71bebeab3ccf1860c/illumos-utils/src/zpool.rs#L193 It's not obvious why the pool could be `None`, particularly when there's a comment on `PathInPool` that says we could derive the pool name from the path. Trying to make it non-optional reveals two spots where this can be set to none, both in `ZoneArgs::root()`, and for two very different reasons: https://github.com/oxidecomputer/omicron/blob/5fa0d8e50fd6ab36c211f3f71bebeab3ccf1860c/sled-agent/src/services.rs#L572-L584 The second arm is where we construct a `PathInPool` for the switch zone. That zone is not in a zpool at all, but is in the ramdisk, so there is truly no zpool involved. The first arm requires `pool` to be optional because `zone_config.zone.filesystem_pool` is optional, and in practice we do have values of `None` there (exactly what #7229 is trying to address!). This one seems dubious at best, because for any given zone we choose to install, we _do_ pick a pool for its filesystem, even if the zone config has `filesystem_pool: None`: https://github.com/oxidecomputer/omicron/blob/5fa0d8e50fd6ab36c211f3f71bebeab3ccf1860c/sled-agent/src/services.rs#L3707-L3720 This PR changes `PathInPool::pool` from `Option<ZpoolName>` to a new enum `ZpoolOrRamdisk`: https://github.com/oxidecomputer/omicron/blob/cd0a87f80301f86661c366979be07bc7ff34f421/illumos-utils/src/zpool.rs#L184-L188 From a Rust-typesystem point of view, this type is basically the same as `Option`, except by renaming the `None` case to `Ramdisk` it becomes obvious that `ZoneArgs::root()` is incorrect: we can't just change the first arm to return `ZpoolOrRamdisk::Ramdisk` in the case where the zone config `filesystem_pool` is `None`, because we're certainly not placing non-switch zones on the ramdisk. This led to removing `ZoneArgs::root()` altogether (the zone config alone is not enough to know the root of the zone, since we choose it randomly in some cases), and instead passing an extra argument into `initialize_zone` (the fully-populated `PathInPool` for the zone). This feels like the smallest change I could make to address the immediate hurdle to fixing #7229, but it might be worth spinning out some separate issues for longer term followup? 1. `PathInPool` is "denormalized" in that the zpool (if there is one) is repeated in the `path` it also stores. There is no enforcement that the two fields are consistent: one can construct or modify a `PathInPool` where the `path` is on some other zpool than the one in `pool`. 2. Should `PathInPool` be used to store paths that aren't in pools at all? (i.e., the switch zone in the ramdisk) 3. This is more tangentially related, but: the `PathInPool::path` value for a zone ends up stored in a sled-agent ledger as part of [`OmicronZoneConfigLocal`](https://github.com/oxidecomputer/omicron/blob/345e095ff9a906ab4f26f3bd623d21b4b4af862a/sled-agent/src/services.rs#L462-L483). The comment on `OmicronZoneConfigLocal` notes that "this struct is less necessary than it has been historically". Once #7229 is fixed, can we make the zone config `filesystem_pool` non-optional and then remove `OmicronZoneConfigLocal` altogether?
diff --git a/illumos-utils/src/running_zone.rs b/illumos-utils/src/running_zone.rs
@@ -13,7 +13,7 @@ use crate::link::{Link, VnicAllocator};
 use crate::opte::{Port, PortTicket};
 use crate::svc::wait_for_service;
 use crate::zone::AddressRequest;
-use crate::zpool::{PathInPool, ZpoolName};
+use crate::zpool::{PathInPool, ZpoolOrRamdisk};
 use camino::{Utf8Path, Utf8PathBuf};
 use camino_tempfile::Utf8TempDir;
 use ipnetwork::IpNetwork;
@@ -358,8 +358,8 @@ impl RunningZone {
     }
 
     /// Returns the zpool on which the filesystem path has been placed.
-    pub fn root_zpool(&self) -> Option<&ZpoolName> {
-        self.inner.zonepath.pool.as_ref()
+    pub fn root_zpool(&self) -> &ZpoolOrRamdisk {
+        &self.inner.zonepath.pool
     }
 
     /// Return the name of a bootstrap VNIC in the zone, if any.
diff --git a/illumos-utils/src/zpool.rs b/illumos-utils/src/zpool.rs
@@ -181,6 +181,12 @@ impl FromStr for ZpoolInfo {
 /// Wraps commands for interacting with ZFS pools.
 pub struct Zpool {}
 
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum ZpoolOrRamdisk {
+    Zpool(ZpoolName),
+    Ramdisk,
+}
+
 /// A path which exists within a pool.
 ///
 /// By storing these types together, it's possible to answer
@@ -190,7 +196,7 @@ pub struct Zpool {}
 // Rather Not.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct PathInPool {
-    pub pool: Option<ZpoolName>,
+    pub pool: ZpoolOrRamdisk,
     pub path: Utf8PathBuf,
 }
 
diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs
@@ -24,6 +24,7 @@ use illumos_utils::opte::{DhcpCfg, PortCreateParams, PortManager};
 use illumos_utils::running_zone::{RunningZone, ZoneBuilderFactory};
 use illumos_utils::svc::wait_for_service;
 use illumos_utils::zone::PROPOLIS_ZONE_PREFIX;
+use illumos_utils::zpool::ZpoolOrRamdisk;
 use omicron_common::api::internal::nexus::{SledVmmState, VmmRuntimeState};
 use omicron_common::api::internal::shared::{
     NetworkInterface, ResolvedVpcFirewallRule, SledIdentifiers, SourceNatConfig,
@@ -1836,7 +1837,10 @@ impl InstanceRunner {
         let Some(run_state) = &self.running_state else {
             return None;
         };
-        run_state.running_zone.root_zpool().map(|p| p.clone())
+        match run_state.running_zone.root_zpool() {
+            ZpoolOrRamdisk::Zpool(zpool_name) => Some(zpool_name.clone()),
+            ZpoolOrRamdisk::Ramdisk => None,
+        }
     }
 
     fn current_state(&self) -> SledVmmState {
diff --git a/sled-agent/src/probe_manager.rs b/sled-agent/src/probe_manager.rs
@@ -6,6 +6,7 @@ use illumos_utils::link::VnicAllocator;
 use illumos_utils::opte::{DhcpCfg, PortCreateParams, PortManager};
 use illumos_utils::running_zone::{RunningZone, ZoneBuilderFactory};
 use illumos_utils::zone::Zones;
+use illumos_utils::zpool::ZpoolOrRamdisk;
 use nexus_client::types::{
     BackgroundTasksActivateRequest, ProbeExternalIp, ProbeInfo,
 };
@@ -126,10 +127,16 @@ impl ProbeManager {
             .zones
             .iter()
             .filter_map(|(id, probe)| {
-                let Some(probe_pool) = probe.root_zpool() else {
-                    // No known pool for this probe
-                    info!(self.inner.log, "use_only_these_disks: Cannot read filesystem pool"; "id" => ?id);
-                    return None;
+                let probe_pool = match probe.root_zpool() {
+                    ZpoolOrRamdisk::Zpool(zpool_name) => zpool_name,
+                    ZpoolOrRamdisk::Ramdisk => {
+                        info!(
+                            self.inner.log,
+                            "use_only_these_disks: removing probe on ramdisk";
+                            "id" => ?id,
+                        );
+                        return None;
+                    }
                 };
 
                 if !u2_set.contains(probe_pool) {
diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs
@@ -55,7 +55,7 @@ use illumos_utils::running_zone::{
 use illumos_utils::smf_helper::SmfHelper;
 use illumos_utils::zfs::ZONE_ZFS_RAMDISK_DATASET_MOUNTPOINT;
 use illumos_utils::zone::AddressRequest;
-use illumos_utils::zpool::{PathInPool, ZpoolName};
+use illumos_utils::zpool::{PathInPool, ZpoolName, ZpoolOrRamdisk};
 use illumos_utils::{execute, PFEXEC};
 use internal_dns_resolver::Resolver;
 use internal_dns_types::names::BOUNDARY_NTP_DNS_NAME;
@@ -533,20 +533,11 @@ impl illumos_utils::smf_helper::Service for SwitchService {
     }
 }
 
-/// Combines the generic `SwitchZoneConfig` with other locally-determined
-/// configuration
-///
-/// This is analogous to `OmicronZoneConfigLocal`, but for the switch zone.
-struct SwitchZoneConfigLocal {
-    zone: SwitchZoneConfig,
-    root: Utf8PathBuf,
-}
-
 /// Describes either an Omicron-managed zone or the switch zone, used for
 /// functions that operate on either one or the other
 enum ZoneArgs<'a> {
     Omicron(&'a OmicronZoneConfigLocal),
-    Switch(&'a SwitchZoneConfigLocal),
+    Switch(&'a SwitchZoneConfig),
 }
 
 impl<'a> ZoneArgs<'a> {
@@ -565,20 +556,7 @@ impl<'a> ZoneArgs<'a> {
     ) -> Box<dyn Iterator<Item = &'a SwitchService> + 'a> {
         match self {
             ZoneArgs::Omicron(_) => Box::new(std::iter::empty()),
-            ZoneArgs::Switch(request) => Box::new(request.zone.services.iter()),
-        }
-    }
-
-    /// Return the root filesystem path for this zone
-    pub fn root(&self) -> PathInPool {
-        match self {
-            ZoneArgs::Omicron(zone_config) => PathInPool {
-                pool: zone_config.zone.filesystem_pool.clone(),
-                path: zone_config.root.clone(),
-            },
-            ZoneArgs::Switch(zone_request) => {
-                PathInPool { pool: None, path: zone_request.root.clone() }
-            }
+            ZoneArgs::Switch(request) => Box::new(request.services.iter()),
         }
     }
 }
@@ -1444,6 +1422,7 @@ impl ServiceManager {
     async fn initialize_zone(
         &self,
         request: ZoneArgs<'_>,
+        zone_root_path: PathInPool,
         filesystems: &[zone::Fs],
         data_links: &[String],
         fake_install_dir: Option<&String>,
@@ -1527,7 +1506,7 @@ impl ServiceManager {
         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_root_path(zone_root_path)
             .with_zone_image_paths(zone_image_paths.as_slice())
             .with_zone_type(zone_type_str)
             .with_datasets(datasets.as_slice())
@@ -2455,10 +2434,7 @@ impl ServiceManager {
                     })?;
                 RunningZone::boot(installed_zone).await?
             }
-            ZoneArgs::Switch(SwitchZoneConfigLocal {
-                zone: SwitchZoneConfig { id, services, addresses },
-                ..
-            }) => {
+            ZoneArgs::Switch(SwitchZoneConfig { id, services, addresses }) => {
                 let info = self.inner.sled_info.get();
 
                 let gw_addr = match info {
@@ -3247,20 +3223,23 @@ impl ServiceManager {
         }
 
         // Ensure that this zone's storage is ready.
-        let root = self
+        let zone_root_path = self
             .validate_storage_and_pick_mountpoint(
                 mount_config,
                 &zone,
                 &all_u2_pools,
             )
             .await?;
 
-        let config =
-            OmicronZoneConfigLocal { zone: zone.clone(), root: root.path };
+        let config = OmicronZoneConfigLocal {
+            zone: zone.clone(),
+            root: zone_root_path.path.clone(),
+        };
 
         let runtime = self
             .initialize_zone(
                 ZoneArgs::Omicron(&config),
+                zone_root_path,
                 // filesystems=
                 &[],
                 // data_links=
@@ -3732,7 +3711,8 @@ impl ServiceManager {
         }
         let path = filesystem_pool
             .dataset_mountpoint(&mount_config.root, ZONE_DATASET);
-        Ok(PathInPool { pool: Some(filesystem_pool), path })
+        let pool = ZpoolOrRamdisk::Zpool(filesystem_pool);
+        Ok(PathInPool { pool, path })
     }
 
     pub async fn cockroachdb_initialize(&self) -> Result<(), Error> {
@@ -4647,12 +4627,18 @@ impl ServiceManager {
         // we could not create the U.2 disks due to lack of encryption. To break
         // the cycle we put the switch zone root fs on the ramdisk.
         let root = Utf8PathBuf::from(ZONE_ZFS_RAMDISK_DATASET_MOUNTPOINT);
-        let zone_request =
-            SwitchZoneConfigLocal { root, zone: request.clone() };
-        let zone_args = ZoneArgs::Switch(&zone_request);
-        info!(self.inner.log, "Starting switch zone",);
+        let zone_root_path =
+            PathInPool { pool: ZpoolOrRamdisk::Ramdisk, path: root.clone() };
+        let zone_args = ZoneArgs::Switch(&request);
+        info!(self.inner.log, "Starting switch zone");
         let zone = self
-            .initialize_zone(zone_args, filesystems, data_links, None)
+            .initialize_zone(
+                zone_args,
+                zone_root_path,
+                filesystems,
+                data_links,
+                None,
+            )
             .await?;
         *sled_zone =
             SwitchZoneState::Running { request: request.clone(), zone };
diff --git a/sled-storage/src/resources.rs b/sled-storage/src/resources.rs
@@ -10,7 +10,7 @@ use crate::disk::{Disk, DiskError, RawDisk};
 use crate::error::Error;
 use camino::Utf8PathBuf;
 use cfg_if::cfg_if;
-use illumos_utils::zpool::{PathInPool, ZpoolName};
+use illumos_utils::zpool::{PathInPool, ZpoolName, ZpoolOrRamdisk};
 use key_manager::StorageKeyRequester;
 use omicron_common::api::external::Generation;
 use omicron_common::disk::{
@@ -134,7 +134,7 @@ impl AllDisks {
             .map(|pool| {
                 let path =
                     pool.dataset_mountpoint(&self.mount_config.root, dataset);
-                PathInPool { pool: Some(pool), path }
+                PathInPool { pool: ZpoolOrRamdisk::Zpool(pool), path }
             })
             .collect()
     }
