client: re-export instance spec types for use in sled-agent (#899)

Using generated instance spec types in sled-agent's API has some
substantial downsides:

1. Some component types (e.g. storage backends) have fields that we'd
   like to redact when logging their contents using their `Debug` impls.
   The native versions of these types manually impl `Debug` for these
   types, but their generated counterparts always derive Debug (and
   AFAIK there's no way to ask Progenitor not to add this derive).
2. The Dropshot-generated OpenAPI doc for sled-agent's API winds up
   having very gnarly type descriptions that include fully-escaped
   versions of the Progenitor-generated doc comments from
   propolis-client.

To get around these problems:

- Add a propolis-client submodule that re-exports all instance
  spec-related types from `propolis_api_types`.
- Add `replace` directives to the generated client that replace the
  generated versions of `InstanceSpecV0`, `VersionedInstanceSpec`, and
  `ReplacementComponent` with their re-exported equivalents.

This way, sled-agent can use the re-exported spec types internally and
in its API. This reuses their manual `impl`s when they exist (solving
problem 1) and avoids presenting generated JSON schema documentation
when generating the sled-agent API document (solving problem 2).

Remove the `JsonSchema` implementations for generated spec types, since
they're no longer needed.

Author: gjcolombo

bin/propolis-cli/src/main.rs

@@ -16,15 +16,17 @@ use anyhow::{anyhow, Context};
 use clap::{Args, Parser, Subcommand};
 use futures::{future, SinkExt};
 use newtype_uuid::{GenericUuid, TypedUuid, TypedUuidKind, TypedUuidTag};
-use propolis_client::support::nvme_serial_from_str;
-use propolis_client::types::{
+use propolis_client::instance_spec::{
     BlobStorageBackend, Board, Chipset, ComponentV0, CrucibleStorageBackend,
-    GuestHypervisorInterface, HyperVFeatureFlag, I440Fx, InstanceEnsureRequest,
-    InstanceInitializationMethod, InstanceMetadata, InstanceSpecGetResponse,
-    InstanceSpecV0, NvmeDisk, QemuPvpanic, ReplacementComponent, SerialPort,
+    GuestHypervisorInterface, HyperVFeatureFlag, I440Fx, InstanceSpecV0,
+    NvmeDisk, PciPath, QemuPvpanic, ReplacementComponent, SerialPort,
     SerialPortNumber, SpecKey, VirtioDisk,
 };
-use propolis_client::PciPath;
+use propolis_client::support::nvme_serial_from_str;
+use propolis_client::types::{
+    InstanceEnsureRequest, InstanceInitializationMethod, InstanceMetadata,
+    InstanceSpecGetResponse,
+};
 use propolis_config_toml::spec::SpecConfig;
 use serde::{Deserialize, Serialize};
 use slog::{o, Drain, Level, Logger};
@@ -200,10 +202,17 @@ fn add_component_to_spec(
     id: SpecKey,
     component: ComponentV0,
 ) -> anyhow::Result<()> {
-    if spec.components.insert(id.to_string(), component).is_some() {
-        anyhow::bail!("duplicate component ID {id:?}");
+    use std::collections::btree_map::Entry;
+    match spec.components.entry(id) {
+        Entry::Vacant(vacant_entry) => {
+            vacant_entry.insert(component);
+            Ok(())
+        }
+        Entry::Occupied(occupied_entry) => Err(anyhow::anyhow!(
+            "duplicate component ID {:?}",
+            occupied_entry.key()
+        )),
     }
-    Ok(())
 }
 
 /// A legacy Propolis API disk request, preserved here for compatibility with
@@ -298,11 +307,13 @@ impl VmConfig {
                 cpus: self.vcpus,
                 memory_mb: self.memory,
                 guest_hv_interface: if self.hyperv {
-                    Some(GuestHypervisorInterface::HyperV {
-                        features: vec![HyperVFeatureFlag::ReferenceTsc],
-                    })
+                    GuestHypervisorInterface::HyperV {
+                        features: [HyperVFeatureFlag::ReferenceTsc]
+                            .into_iter()
+                            .collect(),
+                    }
                 } else {
-                    None
+                    Default::default()
                 },
             },
             components: Default::default(),

crates/propolis-api-types/src/instance_spec/components/board.rs

@@ -38,6 +38,12 @@ pub enum Chipset {
     I440Fx(I440Fx),
 }
 
+impl Default for Chipset {
+    fn default() -> Self {
+        Self::I440Fx(I440Fx { enable_pcie: false })
+    }
+}
+
 /// A set of CPUID values to expose to a guest.
 #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
 #[serde(deny_unknown_fields)]

crates/propolis-api-types/src/instance_spec/mod.rs

@@ -217,10 +217,16 @@ impl std::fmt::Display for SpecKey {
 impl std::str::FromStr for SpecKey {
     type Err = core::convert::Infallible;
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        Ok(match Uuid::parse_str(s) {
+        Ok(s.into())
+    }
+}
+
+impl From<&str> for SpecKey {
+    fn from(s: &str) -> Self {
+        match Uuid::parse_str(s) {
             Ok(uuid) => Self::Uuid(uuid),
             Err(_) => Self::Name(s.to_owned()),
-        })
+        }
     }
 }
 

crates/propolis-config-toml/src/spec.rs

@@ -10,14 +10,13 @@ use std::{
 };
 
 use propolis_client::{
-    support::nvme_serial_from_str,
-    types::{
+    instance_spec::{
         ComponentV0, DlpiNetworkBackend, FileStorageBackend,
-        MigrationFailureInjector, NvmeDisk, P9fs, PciPciBridge, SoftNpuP9,
-        SoftNpuPciPort, SoftNpuPort, SpecKey, VirtioDisk, VirtioNetworkBackend,
-        VirtioNic,
+        MigrationFailureInjector, NvmeDisk, P9fs, PciPath, PciPciBridge,
+        SoftNpuP9, SoftNpuPciPort, SoftNpuPort, SpecKey, VirtioDisk,
+        VirtioNetworkBackend, VirtioNic,
     },
-    PciPath,
+    support::nvme_serial_from_str,
 };
 use thiserror::Error;
 

lib/propolis-client/src/lib.rs

@@ -4,9 +4,28 @@
 
 //! A client for the Propolis hypervisor frontend's server API.
 
-// Re-export types from propolis_api_types where callers may want to use
-// constructors or From impls.
-pub use propolis_api_types::instance_spec::{PciPath, SpecKey};
+/// Re-exports of types related to instance specs.
+///
+/// These types are re-exported for the convenience of components like
+/// sled-agent that may wish to expose instance specs in their own APIs.
+/// Defining the sled-agent API in terms of these "native" types allows
+/// sled-agent to reuse their trait implementations (and in particular use
+/// "manual" impls of things that Progenitor would otherwise derive).
+///
+/// In the generated client, the native "top-level" instance spec and component
+/// types ([`VersionedInstanceSpec`], [`InstanceSpecV0`], and
+/// [`ReplacementComponent`]) replace their generated counterparts. This
+/// obviates the need to maintain `From` impls to convert between native and
+/// generated types.
+pub mod instance_spec {
+    pub use propolis_api_types::instance_spec::{
+        components::{backends::*, board::*, devices::*},
+        v0::*,
+        *,
+    };
+
+    pub use propolis_api_types::ReplacementComponent;
+}
 
 // Re-export Crucible client types that appear in their serialized forms in
 // instance specs. This allows clients to ensure they serialize/deserialize
@@ -19,106 +38,21 @@ progenitor::generate_api!(
     interface = Builder,
     tags = Separate,
     replace = {
-        PciPath = crate::PciPath,
+        PciPath = crate::instance_spec::PciPath,
+        ReplacementComponent = crate::instance_spec::ReplacementComponent,
+        InstanceSpecV0 = crate::instance_spec::InstanceSpecV0,
+        VersionedInstanceSpec = crate::instance_spec::VersionedInstanceSpec,
     },
     // Automatically derive JsonSchema for instance spec-related types so that
     // they can be reused in sled-agent's API. This can't be done with a
     // `derives = [schemars::JsonSchema]` directive because the `SpecKey` type
     // needs to implement that trait manually (see below).
     patch = {
-        BlobStorageBackend = { derives = [ schemars::JsonSchema ]},
-        Board = { derives = [ schemars::JsonSchema ]},
-        BootOrderEntry = { derives = [ schemars::JsonSchema ]},
-        BootSettings = { derives = [ schemars::JsonSchema, Default] },
-        ComponentV0 = { derives = [ schemars::JsonSchema ]},
-        Chipset = { derives = [ schemars::JsonSchema ]},
-        CrucibleStorageBackend = { derives = [ schemars::JsonSchema ]},
-        Cpuid = { derives = [ schemars::JsonSchema ]},
-        CpuidEntry = { derives = [ schemars::JsonSchema, PartialEq, Eq, Copy ]},
-        CpuidVendor = { derives = [ schemars::JsonSchema ]},
-        DlpiNetworkBackend = { derives = [ schemars::JsonSchema ]},
-        FileStorageBackend = { derives = [ schemars::JsonSchema ]},
-        GuestHypervisorInterface = { derives = [ schemars::JsonSchema ]},
-        I440Fx = { derives = [ schemars::JsonSchema ]},
-        HyperVFeatureFlag = { derives = [ schemars::JsonSchema ]},
-        MigrationFailureInjector = { derives = [ schemars::JsonSchema ]},
-        NvmeDisk = { derives = [ schemars::JsonSchema ]},
+        BootSettings = { derives = [ Default ] },
+        CpuidEntry = { derives = [ PartialEq, Eq, Copy ] },
         InstanceMetadata = { derives = [ PartialEq ] },
-        InstanceSpecV0 = { derives = [ schemars::JsonSchema ]},
-        PciPciBridge = { derives = [ schemars::JsonSchema ]},
-        P9fs = { derives = [ schemars::JsonSchema ]},
-        QemuPvpanic = { derives = [ schemars::JsonSchema ]},
-        SerialPort = { derives = [ schemars::JsonSchema ]},
-        SerialPortNumber = { derives = [ schemars::JsonSchema ]},
-        SoftNpuP9 = { derives = [ schemars::JsonSchema ]},
-        SoftNpuPort = { derives = [ schemars::JsonSchema ]},
-        SoftNpuPciPort = { derives = [ schemars::JsonSchema ]},
         SpecKey = { derives = [ PartialEq, Eq, Ord, PartialOrd, Hash ] },
-        VirtioDisk = { derives = [ schemars::JsonSchema ]},
-        VirtioNic = { derives = [ schemars::JsonSchema ]},
-        VirtioNetworkBackend = { derives = [ schemars::JsonSchema ]},
     }
 );
 
-// Supply the same JsonSchema implementation for the generated SpecKey type that
-// the native type has. This allows sled-agent (or another consumer) to reuse
-// the generated type in its own API to produce an API document that generates
-// the correct type for sled-agent's (or the other consumer's) clients.
-impl schemars::JsonSchema for types::SpecKey {
-    fn schema_name() -> String {
-        "SpecKey".to_owned()
-    }
-
-    fn json_schema(
-        generator: &mut schemars::gen::SchemaGenerator,
-    ) -> schemars::schema::Schema {
-        use schemars::schema::*;
-        fn label_schema(label: &str, schema: Schema) -> Schema {
-            SchemaObject {
-                metadata: Some(
-                    Metadata {
-                        title: Some(label.to_string()),
-                        ..Default::default()
-                    }
-                    .into(),
-                ),
-                subschemas: Some(
-                    SubschemaValidation {
-                        all_of: Some(vec![schema]),
-                        ..Default::default()
-                    }
-                    .into(),
-                ),
-                ..Default::default()
-            }
-            .into()
-        }
-
-        SchemaObject {
-            metadata: Some(
-                Metadata {
-                    description: Some(
-                        "A key identifying a component in an instance spec."
-                            .to_string(),
-                    ),
-                    ..Default::default()
-                }
-                .into(),
-            ),
-            subschemas: Some(Box::new(SubschemaValidation {
-                one_of: Some(vec![
-                    label_schema(
-                        "uuid",
-                        generator.subschema_for::<uuid::Uuid>(),
-                    ),
-                    label_schema("name", generator.subschema_for::<String>()),
-                ]),
-                ..Default::default()
-            })),
-            ..Default::default()
-        }
-        .into()
-    }
-}
-
 pub mod support;

phd-tests/framework/src/disk/crucible.rs

@@ -13,7 +13,7 @@ use std::{
 
 use anyhow::Context;
 use propolis_client::{
-    types::{ComponentV0, CrucibleStorageBackend},
+    instance_spec::{ComponentV0, CrucibleStorageBackend},
     CrucibleOpts, VolumeConstructionRequest,
 };
 use rand::{rngs::StdRng, RngCore, SeedableRng};

phd-tests/framework/src/disk/file.rs

@@ -5,7 +5,7 @@
 //! Abstractions for disks with a raw file backend.
 
 use camino::{Utf8Path, Utf8PathBuf};
-use propolis_client::types::{ComponentV0, FileStorageBackend};
+use propolis_client::instance_spec::{ComponentV0, FileStorageBackend};
 use tracing::{debug, error, warn};
 use uuid::Uuid;
 

phd-tests/framework/src/disk/in_memory.rs

@@ -4,7 +4,7 @@
 
 //! Abstractions for disks with an in-memory backend.
 
-use propolis_client::types::{BlobStorageBackend, ComponentV0};
+use propolis_client::instance_spec::{BlobStorageBackend, ComponentV0};
 
 use super::DiskConfig;
 use crate::disk::DeviceName;

phd-tests/framework/src/disk/mod.rs

@@ -13,7 +13,7 @@ use std::sync::Arc;
 use anyhow::Context;
 use camino::{Utf8Path, Utf8PathBuf};
 use in_memory::InMemoryDisk;
-use propolis_client::types::ComponentV0;
+use propolis_client::instance_spec::ComponentV0;
 use thiserror::Error;
 
 use crate::{

phd-tests/framework/src/test_vm/config.rs

@@ -7,14 +7,14 @@ use std::sync::Arc;
 use anyhow::Context;
 use cpuid_utils::CpuidIdent;
 use propolis_client::{
-    support::nvme_serial_from_str,
-    types::{
+    instance_spec::{
         Board, BootOrderEntry, BootSettings, Chipset, ComponentV0, Cpuid,
-        CpuidEntry, CpuidVendor, GuestHypervisorInterface, InstanceMetadata,
-        InstanceSpecV0, MigrationFailureInjector, NvmeDisk, SerialPort,
+        CpuidEntry, CpuidVendor, GuestHypervisorInterface, InstanceSpecV0,
+        MigrationFailureInjector, NvmeDisk, PciPath, SerialPort,
         SerialPortNumber, SpecKey, VirtioDisk,
     },
-    PciPath,
+    support::nvme_serial_from_str,
+    types::InstanceMetadata,
 };
 use uuid::Uuid;
 
@@ -299,7 +299,10 @@ impl<'dr> VmConfig<'dr> {
                         cpuid_utils::CpuidVendor::Intel => CpuidVendor::Intel,
                     },
                 }),
-                guest_hv_interface: guest_hv_interface.clone(),
+                guest_hv_interface: guest_hv_interface
+                    .as_ref()
+                    .cloned()
+                    .unwrap_or_default(),
             },
             components: Default::default(),
         };
@@ -338,24 +341,25 @@ impl<'dr> VmConfig<'dr> {
                 }),
             };
 
-            let _old =
-                spec.components.insert(device_name.into_string(), device_spec);
+            let _old = spec
+                .components
+                .insert(device_name.into_string().into(), device_spec);
             assert!(_old.is_none());
             let _old = spec
                 .components
-                .insert(backend_name.into_string(), backend_spec);
+                .insert(backend_name.into_string().into(), backend_spec);
             assert!(_old.is_none());
         }
 
         let _old = spec.components.insert(
-            "com1".to_string(),
+            "com1".into(),
             ComponentV0::SerialPort(SerialPort { num: SerialPortNumber::Com1 }),
         );
         assert!(_old.is_none());
 
         if let Some(boot_order) = boot_order.as_ref() {
             let _old = spec.components.insert(
-                "boot-settings".to_string(),
+                "boot-settings".into(),
                 ComponentV0::BootSettings(BootSettings {
                     order: boot_order
                         .iter()
@@ -370,7 +374,7 @@ impl<'dr> VmConfig<'dr> {
 
         if let Some(mig) = migration_failure.as_ref() {
             let _old = spec.components.insert(
-                "migration-failure".to_string(),
+                "migration-failure".into(),
                 ComponentV0::MigrationFailureInjector(mig.clone()),
             );
             assert!(_old.is_none());