[sled-agent] Integrate config-reconciler

Author: jgallagher

clients/sled-agent-client/src/lib.rs

@@ -67,7 +67,6 @@ progenitor::generate_api!(
         OmicronPhysicalDiskConfig = omicron_common::disk::OmicronPhysicalDiskConfig,
         OmicronPhysicalDisksConfig = omicron_common::disk::OmicronPhysicalDisksConfig,
         OmicronSledConfig = nexus_sled_agent_shared::inventory::OmicronSledConfig,
-        OmicronSledConfigResult = nexus_sled_agent_shared::inventory::OmicronSledConfigResult,
         OmicronZoneConfig = nexus_sled_agent_shared::inventory::OmicronZoneConfig,
         OmicronZoneDataset = nexus_sled_agent_shared::inventory::OmicronZoneDataset,
         OmicronZoneImageSource = nexus_sled_agent_shared::inventory::OmicronZoneImageSource,

dev-tools/omdb/src/bin/omdb/sled_agent.rs

@@ -34,14 +34,6 @@ enum SledAgentCommands {
     #[clap(subcommand)]
     Zones(ZoneCommands),
 
-    /// print information about zpools
-    #[clap(subcommand)]
-    Zpools(ZpoolCommands),
-
-    /// print information about datasets
-    #[clap(subcommand)]
-    Datasets(DatasetCommands),
-
     /// print information about the local bootstore node
     #[clap(subcommand)]
     Bootstore(BootstoreCommands),
@@ -97,12 +89,6 @@ impl SledAgentArgs {
             SledAgentCommands::Zones(ZoneCommands::List) => {
                 cmd_zones_list(&client).await
             }
-            SledAgentCommands::Zpools(ZpoolCommands::List) => {
-                cmd_zpools_list(&client).await
-            }
-            SledAgentCommands::Datasets(DatasetCommands::List) => {
-                cmd_datasets_list(&client).await
-            }
             SledAgentCommands::Bootstore(BootstoreCommands::Status) => {
                 cmd_bootstore_status(&client).await
             }
@@ -129,44 +115,6 @@ async fn cmd_zones_list(
     Ok(())
 }
 
-/// Runs `omdb sled-agent zpools list`
-async fn cmd_zpools_list(
-    client: &sled_agent_client::Client,
-) -> Result<(), anyhow::Error> {
-    let response = client.zpools_get().await.context("listing zpools")?;
-    let zpools = response.into_inner();
-
-    println!("zpools:");
-    if zpools.is_empty() {
-        println!("    <none>");
-    }
-    for zpool in &zpools {
-        println!("    {:?}", zpool);
-    }
-
-    Ok(())
-}
-
-/// Runs `omdb sled-agent datasets list`
-async fn cmd_datasets_list(
-    client: &sled_agent_client::Client,
-) -> Result<(), anyhow::Error> {
-    let response = client.datasets_get().await.context("listing datasets")?;
-    let response = response.into_inner();
-
-    println!("dataset configuration @ generation {}:", response.generation);
-    let datasets = response.datasets;
-
-    if datasets.is_empty() {
-        println!("    <none>");
-    }
-    for dataset in &datasets {
-        println!("    {:?}", dataset);
-    }
-
-    Ok(())
-}
-
 /// Runs `omdb sled-agent bootstore status`
 async fn cmd_bootstore_status(
     client: &sled_agent_client::Client,

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

@@ -14,10 +14,7 @@ use omicron_common::{
         external::{ByteCount, Generation},
         internal::shared::{NetworkInterface, SourceNatConfig},
     },
-    disk::{
-        DatasetConfig, DatasetManagementStatus, DiskManagementStatus,
-        DiskVariant, OmicronPhysicalDiskConfig,
-    },
+    disk::{DatasetConfig, DiskVariant, OmicronPhysicalDiskConfig},
     zpool_name::ZpoolName,
 };
 use omicron_uuid_kinds::{DatasetUuid, OmicronZoneUuid};
@@ -130,8 +127,6 @@ pub enum SledRole {
 }
 
 /// Describes the set of Reconfigurator-managed configuration elements of a sled
-// TODO this struct should have a generation number; at the moment, each of
-// the fields has a separete one internally.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq)]
 pub struct OmicronSledConfig {
     pub generation: Generation,
@@ -140,14 +135,6 @@ pub struct OmicronSledConfig {
     pub zones: IdMap<OmicronZoneConfig>,
 }
 
-/// Result of the currently-synchronous `omicron_config_put` endpoint.
-#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-#[must_use = "this `DatasetManagementResult` may contain errors, which should be handled"]
-pub struct OmicronSledConfigResult {
-    pub disks: Vec<DiskManagementStatus>,
-    pub datasets: Vec<DatasetManagementStatus>,
-}
-
 /// Describes the set of Omicron-managed zones running on a sled
 #[derive(
     Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash,

nexus/reconfigurator/execution/src/omicron_sled_config.rs

@@ -10,15 +10,13 @@ use anyhow::anyhow;
 use futures::StreamExt;
 use futures::stream;
 use nexus_db_queries::context::OpContext;
-use nexus_sled_agent_shared::inventory::OmicronSledConfigResult;
 use nexus_types::deployment::BlueprintSledConfig;
 use omicron_uuid_kinds::GenericUuid;
 use omicron_uuid_kinds::SledUuid;
-use slog::Logger;
 use slog::info;
 use slog::warn;
+use slog_error_chain::InlineErrorChain;
 use std::collections::BTreeMap;
-use update_engine::merge_anyhow_list;
 
 /// Idempotently ensure that the specified Omicron sled configs are deployed to
 /// the corresponding sleds
@@ -63,13 +61,14 @@ pub(crate) async fn deploy_sled_configs(
                     format!("Failed to put {config:#?} to sled {sled_id}")
                 });
             match result {
+                Ok(_) => None,
                 Err(error) => {
-                    warn!(log, "{error:#}");
+                    warn!(
+                        log, "failed to put sled config";
+                        InlineErrorChain::new(error.as_ref()),
+                    );
                     Some(error)
                 }
-                Ok(result) => {
-                    parse_config_result(result.into_inner(), &log).err()
-                }
             }
         })
         .collect()
@@ -78,69 +77,6 @@ pub(crate) async fn deploy_sled_configs(
     if errors.is_empty() { Ok(()) } else { Err(errors) }
 }
 
-fn parse_config_result(
-    result: OmicronSledConfigResult,
-    log: &Logger,
-) -> anyhow::Result<()> {
-    let (disk_errs, disk_successes): (Vec<_>, Vec<_>) =
-        result.disks.into_iter().partition(|status| status.err.is_some());
-
-    if !disk_errs.is_empty() {
-        warn!(
-            log,
-            "Failed to deploy disks for sled agent";
-            "successfully configured disks" => disk_successes.len(),
-            "failed disk configurations" => disk_errs.len(),
-        );
-        for err in &disk_errs {
-            warn!(log, "{err:?}");
-        }
-        return Err(merge_anyhow_list(disk_errs.into_iter().map(|status| {
-            anyhow!(
-                "failed to deploy disk {:?}: {:#}",
-                status.identity,
-                // `disk_errs` was partitioned by `status.err.is_some()`, so
-                // this is safe to unwrap.
-                status.err.unwrap(),
-            )
-        })));
-    }
-
-    let (dataset_errs, dataset_successes): (Vec<_>, Vec<_>) =
-        result.datasets.into_iter().partition(|status| status.err.is_some());
-
-    if !dataset_errs.is_empty() {
-        warn!(
-            log,
-            "Failed to deploy datasets for sled agent";
-            "successfully configured datasets" => dataset_successes.len(),
-            "failed dataset configurations" => dataset_errs.len(),
-        );
-        for err in &dataset_errs {
-            warn!(log, "{err:?}");
-        }
-        return Err(merge_anyhow_list(dataset_errs.into_iter().map(
-            |status| {
-                anyhow!(
-                    "failed to deploy dataset {}: {:#}",
-                    status.dataset_name.full_name(),
-                    // `dataset_errs` was partitioned by `status.err.is_some()`,
-                    // so this is safe to unwrap.
-                    status.err.unwrap(),
-                )
-            },
-        )));
-    }
-
-    info!(
-        log,
-        "Successfully deployed config to sled agent";
-        "successfully configured disks" => disk_successes.len(),
-        "successfully configured datasets" => dataset_successes.len(),
-    );
-    Ok(())
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -326,6 +262,9 @@ mod tests {
 
         // Observe the latest configuration stored on the simulated sled agent,
         // and verify that this output matches the input.
+        //
+        // TODO-cleanup Simulated sled-agent should report a unified
+        // `OmicronSledConfig`.
         let observed_disks =
             sim_sled_agent.omicron_physical_disks_list().unwrap();
         let observed_datasets = sim_sled_agent.datasets_config_list().unwrap();

nexus/reconfigurator/planning/src/example.rs

@@ -14,7 +14,6 @@ use crate::planner::rng::PlannerRng;
 use crate::system::SledBuilder;
 use crate::system::SystemDescription;
 use nexus_inventory::CollectionBuilderRng;
-use nexus_sled_agent_shared::inventory::OmicronZonesConfig;
 use nexus_types::deployment::Blueprint;
 use nexus_types::deployment::OmicronZoneNic;
 use nexus_types::deployment::PlanningInput;
@@ -486,15 +485,7 @@ impl ExampleSystemBuilder {
 
         for (sled_id, sled_cfg) in &blueprint.sleds {
             let sled_cfg = sled_cfg.clone().into_in_service_sled_config();
-            system
-                .sled_set_omicron_zones(
-                    *sled_id,
-                    OmicronZonesConfig {
-                        generation: sled_cfg.generation,
-                        zones: sled_cfg.zones.into_iter().collect(),
-                    },
-                )
-                .unwrap();
+            system.sled_set_omicron_config(*sled_id, sled_cfg).unwrap();
         }
 
         // We just ensured that a handful of datasets should exist in

nexus/reconfigurator/planning/src/planner.rs

@@ -996,7 +996,6 @@ pub(crate) mod test {
     use clickhouse_admin_types::ClickhouseKeeperClusterMembership;
     use clickhouse_admin_types::KeeperId;
     use expectorate::assert_contents;
-    use nexus_sled_agent_shared::inventory::OmicronZonesConfig;
     use nexus_types::deployment::BlueprintDatasetDisposition;
     use nexus_types::deployment::BlueprintDiffSummary;
     use nexus_types::deployment::BlueprintPhysicalDiskDisposition;
@@ -1181,18 +1180,15 @@ pub(crate) mod test {
         // example.collection -- this should be addressed via API improvements.
         example
             .system
-            .sled_set_omicron_zones(new_sled_id, {
-                let sled_cfg = blueprint4
+            .sled_set_omicron_config(
+                new_sled_id,
+                blueprint4
                     .sleds
                     .get(&new_sled_id)
                     .expect("blueprint should contain zones for new sled")
                     .clone()
-                    .into_in_service_sled_config();
-                OmicronZonesConfig {
-                    generation: sled_cfg.generation,
-                    zones: sled_cfg.zones.into_iter().collect(),
-                }
-            })
+                    .into_in_service_sled_config(),
+            )
             .unwrap();
         let collection =
             example.system.to_collection_builder().unwrap().build();

nexus/reconfigurator/planning/src/system.rs

@@ -17,6 +17,7 @@ use nexus_sled_agent_shared::inventory::Inventory;
 use nexus_sled_agent_shared::inventory::InventoryDataset;
 use nexus_sled_agent_shared::inventory::InventoryDisk;
 use nexus_sled_agent_shared::inventory::InventoryZpool;
+use nexus_sled_agent_shared::inventory::OmicronSledConfig;
 use nexus_sled_agent_shared::inventory::OmicronZonesConfig;
 use nexus_sled_agent_shared::inventory::SledRole;
 use nexus_types::deployment::ClickhousePolicy;
@@ -353,7 +354,7 @@ impl SystemDescription {
         !self.sleds.is_empty()
     }
 
-    /// Set Omicron zones for a sled.
+    /// Set Omicron config for a sled.
     ///
     /// The zones will be reported in the collection generated by
     /// [`Self::to_collection_builder`].
@@ -362,17 +363,26 @@ impl SystemDescription {
     ///
     /// # Notes
     ///
-    /// It is okay to call `sled_set_omicron_zones` in ways that wouldn't
+    /// It is okay to call `sled_set_omicron_config` in ways that wouldn't
     /// happen in production, such as to test illegal states.
-    pub fn sled_set_omicron_zones(
+    pub fn sled_set_omicron_config(
         &mut self,
         sled_id: SledUuid,
-        omicron_zones: OmicronZonesConfig,
+        sled_config: OmicronSledConfig,
     ) -> anyhow::Result<&mut Self> {
         let sled = self.sleds.get_mut(&sled_id).with_context(|| {
             format!("attempted to access sled {} not found in system", sled_id)
         })?;
-        Arc::make_mut(sled).inventory_sled_agent.omicron_zones = omicron_zones;
+        let sled = Arc::make_mut(sled);
+        // TODO this inventory needs to be more robust for the reconciler;
+        // this is nontrivial so coming in a subsequent PR. For now just
+        // backfill the existing inventory structure.
+        sled.inventory_sled_agent.omicron_zones = OmicronZonesConfig {
+            generation: sled_config.generation,
+            zones: sled_config.zones.into_iter().collect(),
+        };
+        sled.inventory_sled_agent.omicron_physical_disks_generation =
+            sled_config.generation;
         Ok(self)
     }