[12/n] [sled-agent] don't start install dataset zones on zone manifest error (#8237)

For mupdate overrides, in order to be safe, we must know that the data
stored in the JSON is consistent with the images stored on disk. We
currently apply this logic to install dataset hashes, and will apply it
to artifact hashes in the future.

Questions:

* [x] ~~Should we apply this logic to more critical zones like the
switch zone? They're always going to be part of the ramdisk maybe?~~ We
unconditionally succeed for RAMdisk zones, and in particular the switch
zone, now.
* [ ] How does this interact with config-reconciler work? On error, that
would permanently be blocked it seems to me. Note: we decide errors
per-zone now.
* [ ] We ask for the boot zpool to be passed in but also cache it as
part of constructing the info... how do we reconcile the relative
presence or absence of this info?

Depends on #8235.

Author: sunshowers

Cargo.lock

@@ -1339,7 +1339,7 @@ impl<'a> ZoneBuilder<'a> {
 }
 
 /// Places to look for a zone's image.
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub struct ZoneImageFileSource {
     /// The file name to look for.
     pub file_name: String,

illumos-utils/src/running_zone.rs

@@ -64,7 +64,7 @@ use internal_dns_types::names::BOUNDARY_NTP_DNS_NAME;
 use internal_dns_types::names::DNS_ZONE;
 use nexus_config::{ConfigDropshotWithTls, DeploymentConfig};
 use nexus_sled_agent_shared::inventory::{
-    OmicronZoneConfig, OmicronZoneImageSource, OmicronZoneType, ZoneKind,
+    OmicronZoneConfig, OmicronZoneType, ZoneKind,
 };
 use omicron_common::address::AZ_PREFIX;
 use omicron_common::address::DENDRITE_PORT;
@@ -94,6 +94,7 @@ use omicron_uuid_kinds::OmicronZoneUuid;
 use sled_agent_config_reconciler::InternalDisksReceiver;
 use sled_agent_types::sled::SWITCH_ZONE_BASEBOARD_FILE;
 use sled_agent_zone_images::ZoneImageSourceResolver;
+use sled_agent_zone_images::{MupdateOverrideReadError, ZoneImageSource};
 use sled_hardware::DendriteAsic;
 use sled_hardware::SledMode;
 use sled_hardware::is_gimlet;
@@ -262,6 +263,9 @@ pub enum Error {
     #[error("Unexpected zone config: zone {zone_id} is running on ramdisk ?!")]
     ZoneIsRunningOnRamdisk { zone_id: OmicronZoneUuid },
 
+    #[error("failed to read mupdate override info, not starting zones")]
+    MupdateOverrideRead(#[source] MupdateOverrideReadError),
+
     #[error(
         "Couldn't find requested zone image ({hash}) for \
         {zone_kind:?} {id} in artifact store: {err}"
@@ -1363,20 +1367,31 @@ impl ServiceManager {
         };
 
         // TODO: `InstallDataset` should be renamed to something more accurate
-        // when all the major changes here have landed. Some zones are
-        // distributed from the host OS image and are never placed in the
-        // install dataset; that enum variant more accurately reflects that we
-        // are falling back to searching `/opt/oxide` in addition to the install
-        // datasets.
+        // when all the major changes here have landed.
+        //
+        // Some zones are distributed from the host OS image and are never
+        // placed in the install dataset; the Ramdisk enum variant more
+        // accurately reflects that we are only search `/opt/oxide` for those
+        // zones.
+        //
+        // (Currently, only the switch zone goes through this code path. Other
+        // ramdisk zones like the probe zone construct the file source
+        // directly.)
         let image_source = match &request {
-            ZoneArgs::Omicron(zone_config) => &zone_config.image_source,
-            ZoneArgs::Switch(_) => &OmicronZoneImageSource::InstallDataset,
+            ZoneArgs::Omicron(zone_config) => {
+                ZoneImageSource::Omicron(zone_config.image_source.clone())
+            }
+            ZoneArgs::Switch(_) => ZoneImageSource::Ramdisk,
         };
-        let file_source = self.inner.zone_image_resolver.file_source_for(
-            zone_type_str,
-            image_source,
-            self.inner.internal_disks_rx.current(),
-        );
+        let file_source = self
+            .inner
+            .zone_image_resolver
+            .file_source_for(
+                zone_type_str,
+                &image_source,
+                self.inner.internal_disks_rx.current(),
+            )
+            .map_err(|error| Error::MupdateOverrideRead(error))?;
 
         // We use the fake initialiser for testing
         let mut zone_builder = match self.inner.system_api.fake_install_dir() {

sled-agent/src/services.rs

@@ -8,7 +8,6 @@ license = "MPL-2.0"
 workspace = true
 
 [dependencies]
-anyhow.workspace = true
 camino.workspace = true
 iddqd.workspace = true
 illumos-utils.workspace = true

sled-agent/zone-images/Cargo.toml

@@ -6,19 +6,33 @@
 
 use crate::AllMupdateOverrides;
 use crate::AllZoneManifests;
+use crate::MupdateOverrideReadError;
 use crate::MupdateOverrideStatus;
 use crate::RAMDISK_IMAGE_PATH;
 use crate::ZoneManifestStatus;
 use crate::install_dataset_file_name;
+use crate::ramdisk_file_source;
 use camino::Utf8PathBuf;
 use illumos_utils::running_zone::ZoneImageFileSource;
 use nexus_sled_agent_shared::inventory::OmicronZoneImageSource;
 use sled_agent_config_reconciler::InternalDisks;
 use sled_agent_config_reconciler::InternalDisksWithBootDisk;
+use slog::error;
 use slog::o;
+use slog_error_chain::InlineErrorChain;
 use std::sync::Arc;
 use std::sync::Mutex;
 
+/// A zone image source.
+#[derive(Clone, Debug)]
+pub enum ZoneImageSource {
+    /// An Omicron zone.
+    Omicron(OmicronZoneImageSource),
+
+    /// A RAM disk-based zone.
+    Ramdisk,
+}
+
 /// Resolves [`OmicronZoneImageSource`] instances into file names and search
 /// paths.
 ///
@@ -57,11 +71,19 @@ impl ZoneImageSourceResolver {
     pub fn file_source_for(
         &self,
         zone_type: &str,
-        image_source: &OmicronZoneImageSource,
+        image_source: &ZoneImageSource,
         internal_disks: InternalDisks,
-    ) -> ZoneImageFileSource {
-        let inner = self.inner.lock().unwrap();
-        inner.file_source_for(zone_type, image_source, internal_disks)
+    ) -> Result<ZoneImageFileSource, MupdateOverrideReadError> {
+        match image_source {
+            ZoneImageSource::Ramdisk => {
+                // RAM disk images are always stored on the RAM disk path.
+                Ok(ramdisk_file_source(zone_type))
+            }
+            ZoneImageSource::Omicron(image_source) => {
+                let inner = self.inner.lock().unwrap();
+                inner.file_source_for(zone_type, image_source, internal_disks)
+            }
+        }
     }
 }
 
@@ -77,7 +99,6 @@ pub struct ResolverStatus {
 
 #[derive(Debug)]
 struct ResolverInner {
-    #[expect(unused)]
     log: slog::Logger,
     image_directory_override: Option<Utf8PathBuf>,
     // Store all collected information for zones -- we're going to need to
@@ -112,41 +133,222 @@ impl ResolverInner {
         zone_type: &str,
         image_source: &OmicronZoneImageSource,
         internal_disks: InternalDisks,
-    ) -> ZoneImageFileSource {
+    ) -> Result<ZoneImageFileSource, MupdateOverrideReadError> {
         match image_source {
             OmicronZoneImageSource::InstallDataset => {
-                // Look for the image in the ramdisk first
+                let file_name = install_dataset_file_name(zone_type);
+                // Look for the image in the RAM disk first. Note that install
+                // dataset images are not stored on the RAM disk in production,
+                // just in development or test workflows.
                 let mut zone_image_paths =
                     vec![Utf8PathBuf::from(RAMDISK_IMAGE_PATH)];
+
                 // Inject an image path if requested by a test.
                 if let Some(path) = &self.image_directory_override {
                     zone_image_paths.push(path.clone());
                 };
 
-                // If the boot disk exists, look for the image in the "install"
-                // dataset on the boot zpool.
+                // Any zones not part of the RAM disk are managed via the
+                // zone manifest.
+                //
+                // XXX: we ask for the boot zpool to be passed in here. But
+                // `AllZoneImages` also caches the boot zpool. How should we
+                // reconcile the two?
                 if let Some(path) = internal_disks.boot_disk_install_dataset() {
-                    zone_image_paths.push(path);
+                    match self.zone_manifests.boot_disk_result() {
+                        Ok(result) => {
+                            match result.data.get(file_name.as_str()) {
+                                Some(result) => {
+                                    if result.is_valid() {
+                                        zone_image_paths.push(path);
+                                    } else {
+                                        // If the zone is not valid, we refuse to start
+                                        // it.
+                                        error!(
+                                            self.log,
+                                            "zone {} is not valid in the zone manifest, \
+                                             not returning it as a source",
+                                            file_name;
+                                            "error" => %result.display()
+                                        );
+                                    }
+                                }
+                                None => {
+                                    error!(
+                                        self.log,
+                                        "zone {} is not present in the boot disk zone manifest",
+                                        file_name,
+                                    );
+                                }
+                            }
+                        }
+                        Err(error) => {
+                            error!(
+                                self.log,
+                                "error parsing boot disk zone manifest, not returning \
+                                 install dataset as a source";
+                                "error" => InlineErrorChain::new(error),
+                            );
+                        }
+                    }
                 }
 
-                ZoneImageFileSource {
-                    file_name: install_dataset_file_name(zone_type),
+                Ok(ZoneImageFileSource {
+                    file_name,
                     search_paths: zone_image_paths,
-                }
+                })
             }
             OmicronZoneImageSource::Artifact { hash } => {
+                // TODO: implement mupdate override here.
+                //
                 // Search both artifact datasets. This iterator starts with the
                 // dataset for the boot disk (if it exists), and then is followed
                 // by all other disks.
                 let search_paths =
                     internal_disks.all_artifact_datasets().collect();
-                ZoneImageFileSource {
-                    // Images in the artifact store are named by just their
-                    // hash.
+                Ok(ZoneImageFileSource {
                     file_name: hash.to_string(),
                     search_paths,
-                }
+                })
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use crate::test_utils::{
+        BOOT_PATHS, BOOT_ZPOOL, WriteInstallDatasetContext,
+        make_internal_disks_rx,
+    };
+
+    use camino_tempfile_ext::prelude::*;
+    use dropshot::{ConfigLogging, ConfigLoggingLevel, test_util::LogContext};
+
+    /// Test source resolver behavior when the zone manifest is invalid.
+    #[test]
+    fn file_source_zone_manifest_invalid() {
+        let logctx = LogContext::new(
+            "source_resolver_file_source_zone_manifest_invalid",
+            &ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Debug },
+        );
+        let dir = Utf8TempDir::new().unwrap();
+        dir.child(&BOOT_PATHS.install_dataset).create_dir_all().unwrap();
+
+        let internal_disks_rx =
+            make_internal_disks_rx(dir.path(), BOOT_ZPOOL, &[]);
+        let resolver = ZoneImageSourceResolver::new(
+            &logctx.log,
+            internal_disks_rx.current_with_boot_disk(),
+        );
+
+        // RAM disk image sources should work as expected.
+        let ramdisk_source = resolver
+            .file_source_for(
+                "zone1",
+                &ZoneImageSource::Ramdisk,
+                internal_disks_rx.current(),
+            )
+            .unwrap();
+        assert_eq!(ramdisk_source, ramdisk_file_source("zone1"));
+
+        let file_source = resolver
+            .file_source_for(
+                "zone1",
+                &ZoneImageSource::Omicron(
+                    OmicronZoneImageSource::InstallDataset,
+                ),
+                internal_disks_rx.current(),
+            )
+            .unwrap();
+
+        // Because the zone manifest is missing, the file source should not
+        // return the install dataset.
+        assert_eq!(
+            file_source,
+            ZoneImageFileSource {
+                file_name: install_dataset_file_name("zone1"),
+                search_paths: vec![Utf8PathBuf::from(RAMDISK_IMAGE_PATH)]
             }
+        );
+
+        logctx.cleanup_successful();
+    }
+
+    /// Test source resolver behavior when the zone manifest detects errors.
+    #[test]
+    fn file_source_with_errors() {
+        let logctx = LogContext::new(
+            "source_resolver_file_source_with_errors",
+            &ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Debug },
+        );
+        let dir = Utf8TempDir::new().unwrap();
+        let mut cx = WriteInstallDatasetContext::new_basic();
+        cx.make_error_cases();
+
+        cx.write_to(&dir.child(&BOOT_PATHS.install_dataset)).unwrap();
+
+        let internal_disks_rx =
+            make_internal_disks_rx(dir.path(), BOOT_ZPOOL, &[]);
+        let resolver = ZoneImageSourceResolver::new(
+            &logctx.log,
+            internal_disks_rx.current_with_boot_disk(),
+        );
+
+        // The resolver should not fail for ramdisk images.
+        let file_source = resolver
+            .file_source_for(
+                "fake-zone",
+                &ZoneImageSource::Ramdisk,
+                internal_disks_rx.current(),
+            )
+            .unwrap();
+        assert_eq!(file_source, ramdisk_file_source("fake-zone"));
+
+        // zone1.tar.gz is valid.
+        let file_source = resolver
+            .file_source_for(
+                "zone1",
+                &ZoneImageSource::Omicron(
+                    OmicronZoneImageSource::InstallDataset,
+                ),
+                internal_disks_rx.current(),
+            )
+            .unwrap();
+        assert_eq!(
+            file_source,
+            ZoneImageFileSource {
+                file_name: "zone1.tar.gz".to_string(),
+                search_paths: vec![
+                    Utf8PathBuf::from(RAMDISK_IMAGE_PATH),
+                    dir.path().join(&BOOT_PATHS.install_dataset)
+                ]
+            },
+        );
+
+        // zone2, zone3, zone4 and zone5 aren't valid, and none of them will
+        // return the install dataset path.
+        for zone_name in ["zone2", "zone3", "zone4", "zone5"] {
+            let file_source = resolver
+                .file_source_for(
+                    zone_name,
+                    &ZoneImageSource::Omicron(
+                        OmicronZoneImageSource::InstallDataset,
+                    ),
+                    internal_disks_rx.current(),
+                )
+                .unwrap();
+            assert_eq!(
+                file_source,
+                ZoneImageFileSource {
+                    file_name: install_dataset_file_name(zone_name),
+                    search_paths: vec![Utf8PathBuf::from(RAMDISK_IMAGE_PATH)]
+                }
+            );
         }
+
+        logctx.cleanup_successful();
     }
 }