[2/n artifact-config] [sled-agent] ArtifactConfig now has the same ledger versioning structure as OmicronSledConfig

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

@@ -29,9 +29,9 @@ use tufaceous_artifact::ArtifactHash;
 
 use crate::InternalDisksReceiver;
 use crate::SledAgentArtifactStore;
-use ledgered_sled_config_versioning::read_ledgered_sled_config;
+use ledgered_config_versioning::read_ledgered_sled_config;
 
-mod ledgered_sled_config_versioning;
+pub(crate) mod ledgered_config_versioning;
 
 const CONFIG_LEDGER_FILENAME: &str = "omicron-sled-config.json";
 

sled-agent/config-reconciler/src/ledger/ledgered_sled_config_versioning.rs → sled-agent/config-reconciler/src/ledger/ledgered_config_versioning.rs

@@ -2,14 +2,16 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-//! Module for converting older formats of the sled configuration files.
+//! Module for converting older formats of Sled Agent's ledgered config files.
 
 use camino::Utf8PathBuf;
 use omicron_ledger::Ledger;
 use omicron_ledger::Ledgerable;
 use serde::Deserialize;
 use serde::Serialize;
+use sled_agent_types::artifact::ArtifactConfig;
 use sled_agent_types::inventory::OmicronSledConfig;
+use sled_agent_types_versions::v1;
 use sled_agent_types_versions::v4;
 use sled_agent_types_versions::v10;
 use sled_agent_types_versions::v11;
@@ -23,12 +25,12 @@ use slog::warn;
 use slog_error_chain::InlineErrorChain;
 use std::error::Error as StdError;
 
-/// Trait describing an ordered sequence of `OmicronSledConfig` versions, each
+/// Trait describing an ordered sequence of versions of a ledgered config, each
 /// of which can be converted from its previous version.
 ///
-/// When adding a new [`OmicronSledConfig`] version, add your new version to the
-/// `version_conversion_chain!()` invocation below. Use the fully-versioned name
-/// (e.g., `vN::inventory::OmicronSledConfig`), not the [`OmicronSledConfig`]
+/// When adding a new version of a ledgered config, add your new version to the
+/// relevant `version_conversion_chain!()` invocation below. Use the
+/// fully-versioned name (e.g., `vN::inventory::OmicronSledConfig`), not the
 /// alias from `latest`.
 ///
 /// Also update the unit tests at the bottom of this file to cover your new
@@ -43,14 +45,14 @@ trait VersionConversionChain: Ledgerable {
     // recursing.
     const IS_TERMINAL: bool = false;
 
-    /// The previous [`OmicronSledConfig`] version, which must be convertible
-    /// into this version.
+    /// The previous version of this config, which must be convertible into
+    /// this version.
     type Previous: VersionConversionChain + TryInto<Self, Error: StdError>;
 }
 
 macro_rules! version_conversion_chain {
     // base case
-    ($current:path, $previous:path) => {
+    ($current:path, $previous:path $(,)?) => {
         impl VersionConversionChain for $current {
             const DESCRIPTION: &str = stringify!($current);
             type Previous = $previous;
@@ -64,9 +66,9 @@ macro_rules! version_conversion_chain {
     };
 }
 
-// This list is ordered from newest to oldest; this is the order in which we'll
-// attempt to parse the ledgered config. Add new versions to the top of the
-// list.
+// These lists are ordered from newest to oldest; this is the order in which
+// we'll attempt to parse the ledgered config. Add new versions to the top of
+// the relevant list.
 version_conversion_chain!(
     v51::inventory::OmicronSledConfig,
     v50::inventory::OmicronSledConfig,
@@ -78,6 +80,11 @@ version_conversion_chain!(
     VersionConversionChainTerminal,
 );
 
+version_conversion_chain!(
+    v1::artifact::ArtifactConfig,
+    VersionConversionChainTerminal,
+);
+
 /// Read the ledgered [`OmicronSledConfig`], converting from older versions if
 /// needed.
 ///
@@ -97,25 +104,48 @@ pub(super) async fn read_ledgered_sled_config(
     log: &Logger,
     paths: Vec<Utf8PathBuf>,
 ) -> Option<OmicronSledConfig> {
+    read_ledgered_config(log, paths).await
+}
+
+/// Read the ledgered [`ArtifactConfig`], converting from older versions if
+/// needed.
+///
+/// # Panics
+///
+/// As with `read_ledgered_sled_config`, this panics if we can read a config
+/// of some known older version but cannot convert it to the latest version.
+pub async fn read_ledgered_artifact_config(
+    log: &Logger,
+    paths: Vec<Utf8PathBuf>,
+) -> Option<ArtifactConfig> {
+    read_ledgered_config(log, paths).await
+}
+
+async fn read_ledgered_config<T>(
+    log: &Logger,
+    paths: Vec<Utf8PathBuf>,
+) -> Option<T>
+where
+    T: VersionConversionChain + Clone,
+{
     // Attempt to read the ledger as the current version; if this succeeds,
     // we're done.
-    if let Some(config) = Ledger::new(log, paths.clone()).await {
-        info!(log, "Ledger of sled config exists");
+    if let Some(config) = Ledger::<T>::new(log, paths.clone()).await {
+        info!(log, "Ledger of config exists"; "version" => T::DESCRIPTION);
         return Some(config.into_inner());
     }
 
     // Try to read the config as the previous version; if we have an older
     // version on disk, this will recurse until we get to it, but then convert
     // it up through our previous version before returning.
-    let prev_version = try_ledgered_config_versions_chain::<
-        <OmicronSledConfig as VersionConversionChain>::Previous,
-    >(log, paths.clone())
-    .await?;
+    let prev_version =
+        try_ledgered_config_versions_chain::<T::Previous>(log, paths.clone())
+            .await?;
 
     let current_version = prev_version.try_into().unwrap_or_else(|e| {
         panic!(
             "failed to convert {} to the current version: {}",
-            <OmicronSledConfig as VersionConversionChain>::DESCRIPTION,
+            T::DESCRIPTION,
             InlineErrorChain::new(&e)
         );
     });
@@ -199,12 +229,12 @@ where
     }
 }
 
-async fn write_converted_ledger(
+async fn write_converted_ledger<T: Ledgerable + Clone>(
     log: &Logger,
     paths: Vec<Utf8PathBuf>,
-    sled_config: OmicronSledConfig,
-) -> OmicronSledConfig {
-    let mut config_ledger = Ledger::new_with(log, paths.clone(), sled_config);
+    config: T,
+) -> T {
+    let mut config_ledger = Ledger::new_with(log, paths.clone(), config);
 
     match config_ledger.commit().await {
         Ok(()) => (),
@@ -215,7 +245,7 @@ async fn write_converted_ledger(
             // next time we run.
             warn!(
                 log,
-                "Failed to write new sled config converted from \
+                "Failed to write new config converted from \
                  from older version";
                 InlineErrorChain::new(&err),
             );
@@ -261,6 +291,16 @@ impl TryFrom<VersionConversionChainTerminal>
     }
 }
 
+impl TryFrom<VersionConversionChainTerminal> for v1::artifact::ArtifactConfig {
+    type Error = std::io::Error;
+
+    fn try_from(
+        _: VersionConversionChainTerminal,
+    ) -> Result<Self, Self::Error> {
+        unreachable!("terminal type is uninhabitable")
+    }
+}
+
 #[cfg(test)]
 pub(super) mod tests {
     use super::*;
@@ -269,22 +309,25 @@ pub(super) mod tests {
     use omicron_test_utils::dev;
 
     // v4 config collected from a test system.
-    const V4_CONFIG_PATH: &str = "test-data/v4-sled-config.json";
+    const V4_SLED_CONFIG_PATH: &str = "test-data/v4-sled-config.json";
 
     // paths for expectorate checks
-    const EXPECTORATE_V10_CONFIG_PATH: &str =
+    const EXPECTORATE_V10_SLED_CONFIG_PATH: &str =
         "expectorate/v10-sled-config.json";
-    const EXPECTORATE_V11_CONFIG_PATH: &str =
+    const EXPECTORATE_V11_SLED_CONFIG_PATH: &str =
         "expectorate/v11-sled-config.json";
-    const EXPECTORATE_V14_CONFIG_PATH: &str =
+    const EXPECTORATE_V14_SLED_CONFIG_PATH: &str =
         "expectorate/v14-sled-config.json";
-    const EXPECTORATE_V49_CONFIG_PATH: &str =
+    const EXPECTORATE_V49_SLED_CONFIG_PATH: &str =
         "expectorate/v49-sled-config.json";
-    const EXPECTORATE_V50_CONFIG_PATH: &str =
+    const EXPECTORATE_V50_SLED_CONFIG_PATH: &str =
         "expectorate/v50-sled-config.json";
-    const EXPECTORATE_V51_CONFIG_PATH: &str =
+    const EXPECTORATE_V51_SLED_CONFIG_PATH: &str =
         "expectorate/v51-sled-config.json";
 
+    // v1 artifact config collected from a test system.
+    const V1_ARTIFACT_CONFIG_PATH: &str = "test-data/v1-artifact-config.json";
+
     // This is solely an expectorate test to guarantee:
     //
     // * the conversions for various versions function (at least starting from
@@ -297,7 +340,7 @@ pub(super) mod tests {
 
         let v4 = Ledger::<v4::inventory::OmicronSledConfig>::new(
             log,
-            vec![V4_CONFIG_PATH.into()],
+            vec![V4_SLED_CONFIG_PATH.into()],
         )
         .await
         .expect("read v4 from test-data")
@@ -317,27 +360,27 @@ pub(super) mod tests {
         let v51 = v51::inventory::OmicronSledConfig::from(v50.clone());
 
         expectorate::assert_contents(
-            EXPECTORATE_V10_CONFIG_PATH,
+            EXPECTORATE_V10_SLED_CONFIG_PATH,
             &serde_json::to_string_pretty(&v10).unwrap(),
         );
         expectorate::assert_contents(
-            EXPECTORATE_V11_CONFIG_PATH,
+            EXPECTORATE_V11_SLED_CONFIG_PATH,
             &serde_json::to_string_pretty(&v11).unwrap(),
         );
         expectorate::assert_contents(
-            EXPECTORATE_V14_CONFIG_PATH,
+            EXPECTORATE_V14_SLED_CONFIG_PATH,
             &serde_json::to_string_pretty(&v14).unwrap(),
         );
         expectorate::assert_contents(
-            EXPECTORATE_V49_CONFIG_PATH,
+            EXPECTORATE_V49_SLED_CONFIG_PATH,
             &serde_json::to_string_pretty(&v49).unwrap(),
         );
         expectorate::assert_contents(
-            EXPECTORATE_V50_CONFIG_PATH,
+            EXPECTORATE_V50_SLED_CONFIG_PATH,
             &serde_json::to_string_pretty(&v50).unwrap(),
         );
         expectorate::assert_contents(
-            EXPECTORATE_V51_CONFIG_PATH,
+            EXPECTORATE_V51_SLED_CONFIG_PATH,
             &serde_json::to_string_pretty(&v51).unwrap(),
         );
         logctx.cleanup_successful();
@@ -358,16 +401,16 @@ pub(super) mod tests {
 
         let counts = check_ledger_reads::<LatestConfig, _, _>(
             log,
-            EXPECTORATE_V51_CONFIG_PATH,
-            Some(V4_CONFIG_PATH),
+            EXPECTORATE_V51_SLED_CONFIG_PATH,
+            Some(V4_SLED_CONFIG_PATH),
             &[
-                V4_CONFIG_PATH,
-                EXPECTORATE_V10_CONFIG_PATH,
-                EXPECTORATE_V11_CONFIG_PATH,
-                EXPECTORATE_V14_CONFIG_PATH,
-                EXPECTORATE_V49_CONFIG_PATH,
-                EXPECTORATE_V50_CONFIG_PATH,
-                EXPECTORATE_V51_CONFIG_PATH,
+                V4_SLED_CONFIG_PATH,
+                EXPECTORATE_V10_SLED_CONFIG_PATH,
+                EXPECTORATE_V11_SLED_CONFIG_PATH,
+                EXPECTORATE_V14_SLED_CONFIG_PATH,
+                EXPECTORATE_V49_SLED_CONFIG_PATH,
+                EXPECTORATE_V50_SLED_CONFIG_PATH,
+                EXPECTORATE_V51_SLED_CONFIG_PATH,
             ],
             |paths| read_ledgered_sled_config(log, paths),
         )
@@ -389,6 +432,44 @@ pub(super) mod tests {
         logctx.cleanup_successful();
     }
 
+    #[tokio::test]
+    async fn read_artifact_config_converts_from_older_versions() {
+        let logctx = dev::test_setup_log(
+            "read_artifact_config_converts_from_older_versions",
+        );
+        let log = &logctx.log;
+
+        // Use an explicit type so that adding a new artifact config version
+        // breaks compilation here. Bump the version and add the new version's
+        // path to the array of ledger paths below.
+        type LatestConfig = v1::artifact::ArtifactConfig;
+
+        let counts = check_ledger_reads::<LatestConfig, _, _>(
+            log,
+            V1_ARTIFACT_CONFIG_PATH,
+            None,
+            &[V1_ARTIFACT_CONFIG_PATH],
+            |paths| read_ledgered_artifact_config(log, paths),
+        )
+        .await;
+
+        // For now, the v1 wire format parses correctly as the latest config
+        // version. Once we make an incompatible change, switch this to
+        // `converted > 0`.
+        assert_eq!(
+            counts.converted, 0,
+            "all artifact config versions share a wire format, so nothing \
+             should have needed conversion"
+        );
+        assert!(
+            counts.unchanged > 0,
+            "no fixture parsed as the latest version; \
+             the no-conversion path is untested"
+        );
+
+        logctx.cleanup_successful();
+    }
+
     /// How many fixtures took each branch of [`check_ledger_reads`].
     struct LedgerReadCounts {
         /// Fixtures that did not parse as the latest version, and so had to be

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

@@ -77,6 +77,7 @@ pub use internal_disks::InternalDisksWithBootDisk;
 pub use ledger::LedgerArtifactConfigError;
 pub use ledger::LedgerNewConfigError;
 pub use ledger::LedgerTaskError;
+pub use ledger::ledgered_config_versioning::read_ledgered_artifact_config;
 pub use mupdate_override::ResolverStatusExt;
 pub use raw_disks::RawDisksSender;
 pub use reconciler_task::CurrentlyManagedZpools;

sled-agent/config-reconciler/test-data/v1-artifact-config.json

@@ -0,0 +1,8 @@
+{
+    "generation": 7,
+    "artifacts": [
+        "0d5f5c7b95c30f0a9d0eff0e2a99c8dfa53a1de4d3ba0c67ec5c39e5cf7d0e21",
+        "55e5c1d2b0b60ca1bd0e50a7cbbc22b0aa6b0d4f1c8d9c2a4b3ee9d6d2f0a1c3",
+        "988cab6ea184b7912350e5af151bd18152ee2702bad7dc0b977d414eb8062e27"
+    ]
+}

sled-agent/src/artifact_store.rs

@@ -41,6 +41,7 @@ use sha2::{Digest, Sha256};
 use sled_agent_config_reconciler::ConfigReconcilerHandle;
 use sled_agent_config_reconciler::InternalDisksReceiver;
 use sled_agent_config_reconciler::SledAgentArtifactStore;
+use sled_agent_config_reconciler::read_ledgered_artifact_config;
 use sled_agent_types::artifact::ArtifactConfig;
 use sled_agent_types::artifact::{ArtifactListResponse, ArtifactPutResponse};
 use slog::{Logger, error, info};
@@ -135,9 +136,8 @@ impl<T: DatasetsManager> ArtifactStore<T> {
             }
         }
 
-        let config = Ledger::new(&log, ledger_paths.clone())
-            .await
-            .map(Ledger::into_inner);
+        let config =
+            read_ledgered_artifact_config(&log, ledger_paths.clone()).await;
         let (config_tx, config) = watch::channel(config);
         // Somewhat arbitrary bound size, large enough that we should never hit it.
         let (ledger_tx, ledger_rx) = mpsc::channel(256);