sled-agent: validate host phase 2 artifacts when ledgering configs

Author: jgallagher

nexus-sled-agent-shared/src/inventory.rs

@@ -597,6 +597,16 @@ pub enum HostPhase2DesiredContents {
     Artifact { hash: ArtifactHash },
 }
 
+impl HostPhase2DesiredContents {
+    /// The artifact hash described by `self`, if it has one.
+    pub fn artifact_hash(&self) -> Option<ArtifactHash> {
+        match self {
+            Self::CurrentContents => None,
+            Self::Artifact { hash } => Some(*hash),
+        }
+    }
+}
+
 /// Describes the desired contents for both host phase 2 slots.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq)]
 #[serde(rename_all = "snake_case")]

sled-agent/config-reconciler/src/ledger.rs

@@ -17,7 +17,7 @@ use slog::error;
 use slog::info;
 use slog::warn;
 use slog_error_chain::InlineErrorChain;
-use std::collections::BTreeMap;
+use std::collections::BTreeSet;
 use std::convert::Infallible;
 use std::ops::Deref as _;
 use tokio::sync::mpsc;
@@ -98,7 +98,7 @@ pub enum LedgerArtifactConfigError {
         "Artifacts in use by ledgered sled config are not present \
          in new artifact config: {0:?}"
     )]
-    InUseArtifactedMissing(BTreeMap<ArtifactHash, &'static str>),
+    InUseArtifactedMissing(BTreeSet<ArtifactHash>),
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -442,10 +442,7 @@ impl<T: SledAgentArtifactStore> LedgerTask<T> {
         // thing we confirm is that any referenced artifacts are present in the
         // artifact store.
         let mut artifact_validation_errors = Vec::new();
-        for zone in &new_config.zones {
-            let Some(artifact_hash) = zone.image_source.artifact_hash() else {
-                continue;
-            };
+        for artifact_hash in config_artifact_hashes(new_config) {
             match self
                 .artifact_store
                 .validate_artifact_exists_in_storage(artifact_hash)
@@ -491,12 +488,10 @@ impl<T: SledAgentArtifactStore> LedgerTask<T> {
             CurrentSledConfig::Ledgered(sled_config) => sled_config,
         };
 
-        let mut missing = BTreeMap::new();
-        for zone in &sled_config.zones {
-            if let Some(hash) = zone.image_source.artifact_hash() {
-                if !new_config.artifacts.contains(&hash) {
-                    missing.insert(hash, "current zone configuration");
-                }
+        let mut missing = BTreeSet::new();
+        for artifact_hash in config_artifact_hashes(sled_config) {
+            if !new_config.artifacts.contains(&artifact_hash) {
+                missing.insert(artifact_hash);
             }
         }
 
@@ -589,6 +584,18 @@ impl<T: SledAgentArtifactStore> LedgerTask<T> {
     }
 }
 
+// Helper to return all the `ArtifactHash`es contained in a config.
+fn config_artifact_hashes(
+    config: &OmicronSledConfig,
+) -> impl Iterator<Item = ArtifactHash> + '_ {
+    config
+        .zones
+        .iter()
+        .filter_map(|zone| zone.image_source.artifact_hash())
+        .chain(config.host_phase_2.slot_a.artifact_hash())
+        .chain(config.host_phase_2.slot_b.artifact_hash())
+}
+
 async fn load_sled_config(
     config_datasets: &[Utf8PathBuf],
     log: &Logger,
@@ -648,6 +655,7 @@ mod tests {
     use camino_tempfile::Utf8TempDir;
     use id_map::IdMap;
     use illumos_utils::zpool::ZpoolName;
+    use nexus_sled_agent_shared::inventory::HostPhase2DesiredContents;
     use nexus_sled_agent_shared::inventory::HostPhase2DesiredSlots;
     use nexus_sled_agent_shared::inventory::OmicronZoneConfig;
     use nexus_sled_agent_shared::inventory::OmicronZoneImageSource;
@@ -1054,7 +1062,7 @@ mod tests {
 
         // Create a config that references a zone with a different artifact
         // hash.
-        let config = OmicronSledConfig {
+        let mut config = OmicronSledConfig {
             generation: Generation::new().next(),
             disks: IdMap::new(),
             datasets: IdMap::new(),
@@ -1070,7 +1078,7 @@ mod tests {
         // The ledger task should reject this config due to a missing artifact.
         let err = test_harness
             .task_handle
-            .set_new_config(config)
+            .set_new_config(config.clone())
             .await
             .expect("can communicate with task")
             .expect_err("config should fail");
@@ -1081,20 +1089,48 @@ mod tests {
 
         // Change the config to reference the artifact that does exist; this one
         // should be accepted.
-        let config = OmicronSledConfig {
-            generation: Generation::new().next(),
-            disks: IdMap::new(),
-            datasets: IdMap::new(),
-            zones: [make_dummy_zone_config_using_artifact_hash(
-                existing_artifact_hash,
-            )]
-            .into_iter()
-            .collect(),
-            remove_mupdate_override: None,
-            host_phase_2: HostPhase2DesiredSlots::current_contents(),
+        config.zones = [make_dummy_zone_config_using_artifact_hash(
+            existing_artifact_hash,
+        )]
+        .into_iter()
+        .collect();
+
+        test_harness
+            .task_handle
+            .set_new_config(config.clone())
+            .await
+            .expect("can communicate with task")
+            .expect("config should be ledgered");
+
+        // Try a config that references a host phase 2 artifact that isn't in
+        // the store; this should be rejected.
+        config.generation = config.generation.next();
+        config.zones = IdMap::new();
+        config.host_phase_2 = HostPhase2DesiredSlots {
+            slot_a: HostPhase2DesiredContents::CurrentContents,
+            slot_b: HostPhase2DesiredContents::Artifact {
+                hash: nonexisting_artifact_hash,
+            },
         };
+        let err = test_harness
+            .task_handle
+            .set_new_config(config.clone())
+            .await
+            .expect("can communicate with task")
+            .expect_err("config should fail");
+        match err {
+            LedgerNewConfigError::ArtifactStoreValidationFailed(_) => (),
+            _ => panic!("unexpected error {}", InlineErrorChain::new(&err)),
+        }
 
-        // TODO-john also test host phase 2 artifacts!
+        // Change the config to reference the artifact that does exist; this one
+        // should be accepted.
+        config.host_phase_2 = HostPhase2DesiredSlots {
+            slot_a: HostPhase2DesiredContents::CurrentContents,
+            slot_b: HostPhase2DesiredContents::Artifact {
+                hash: existing_artifact_hash,
+            },
+        };
 
         test_harness
             .task_handle
@@ -1112,54 +1148,84 @@ mod tests {
             "reject_artifact_configs_removing_referenced_artifacts",
         );
 
-        // Claim we have a zone using the all-zeroes hash.
-        let used_artifact_hash = ArtifactHash([0; 32]);
-        let unused_artifact_hash = ArtifactHash([1; 32]);
+        // Construct some artifacts hashes we'll claim to be using and one we
+        // won't.
+        let used_zone_artifact_hash = ArtifactHash([0; 32]);
+        let used_host_artifact_hash = ArtifactHash([1; 32]);
+        let unused_artifact_hash = ArtifactHash([2; 32]);
 
+        // Claim we have a host phase 2
         // Set up the ledger task with an initial config.
         let mut sled_config = make_nonempty_sled_config();
         sled_config.zones.insert(make_dummy_zone_config_using_artifact_hash(
-            used_artifact_hash,
+            used_zone_artifact_hash,
         ));
+        sled_config.host_phase_2.slot_a = HostPhase2DesiredContents::Artifact {
+            hash: used_host_artifact_hash,
+        };
+
         let test_harness =
             TestHarness::with_initial_config(logctx.log.clone(), &sled_config)
                 .await;
 
-        // Construct an `ArtifactConfig` that doesn't contain
-        // `used_artifact_hash`; this should be rejected.
         let mut artifact_config = ArtifactConfig {
             generation: Generation::new(),
-            artifacts: [unused_artifact_hash].into_iter().collect(),
+            artifacts: BTreeSet::new(),
         };
-        let err = test_harness
-            .task_handle
-            .validate_artifact_config(artifact_config.clone())
-            .await
-            .expect("no ledger task error")
-            .expect_err("config should be rejected");
 
-        match err {
-            LedgerArtifactConfigError::InUseArtifactedMissing(artifacts) => {
-                assert!(
-                    artifacts.contains_key(&used_artifact_hash),
-                    "unexpected error contents: {artifacts:?}"
-                );
+        // Try some ArtifactConfigs that don't contain both `used_*` artifact
+        // hashes; all of these should be rejected.
+        for incomplete_artifacts in [
+            &[used_zone_artifact_hash] as &[ArtifactHash],
+            &[used_host_artifact_hash],
+            &[unused_artifact_hash],
+            &[used_zone_artifact_hash, unused_artifact_hash],
+            &[used_host_artifact_hash, unused_artifact_hash],
+        ] {
+            artifact_config.artifacts =
+                incomplete_artifacts.iter().cloned().collect();
+
+            let err = test_harness
+                .task_handle
+                .validate_artifact_config(artifact_config.clone())
+                .await
+                .expect("no ledger task error")
+                .expect_err("config should be rejected");
+
+            match err {
+                LedgerArtifactConfigError::InUseArtifactedMissing(
+                    artifacts,
+                ) => {
+                    if !incomplete_artifacts.contains(&used_zone_artifact_hash)
+                    {
+                        assert!(
+                            artifacts.contains(&used_zone_artifact_hash),
+                            "unexpected error contents: {artifacts:?}"
+                        );
+                    }
+                    if !incomplete_artifacts.contains(&used_host_artifact_hash)
+                    {
+                        assert!(
+                            artifacts.contains(&used_host_artifact_hash),
+                            "unexpected error contents: {artifacts:?}"
+                        );
+                    }
+                }
+                _ => panic!("unexpected error {}", InlineErrorChain::new(&err)),
             }
-            _ => panic!("unexpected error {}", InlineErrorChain::new(&err)),
         }
 
-        // Put the used artifact hash into the artifact config; now it should
+        // Put both used artifact hashes into the artifact config; now it should
         // pass validation.
-        artifact_config.artifacts.insert(used_artifact_hash);
+        artifact_config.artifacts.insert(used_zone_artifact_hash);
+        artifact_config.artifacts.insert(used_host_artifact_hash);
         test_harness
             .task_handle
             .validate_artifact_config(artifact_config)
             .await
             .expect("no ledger task error")
             .expect("config is valid");
 
-        // TODO-john also test host phase 2 artifacts!
-
         logctx.cleanup_successful();
     }
 

sled-agent/config-reconciler/test-data/expectorate/merged-sled-config.json

@@ -810,5 +810,9 @@
       }
     }
   },
-  "remove_mupdate_override": null
+  "remove_mupdate_override": null,
+  "host_phase_2": {
+    "slot_a": "CurrentContents",
+    "slot_b": "CurrentContents"
+  }
 }