api: clean up Instance and InstanceProperties types (#803)

Clean up unused fields and structures in the server API:

- Remove the unused `image_id` and `bootrom_id` fields from
  `InstanceProperties`.
- Remove the `disks` and `nics` fields from the `Instance` struct returned
  by an instance GET and all their supporting types. These are also not
  used today. Callers who are interested in finding out about an instance's
  disks and NICs can query the instance spec GET endpoint instead.
- Remove the CPU count and memory size fields from `InstanceProperties` and
  put them in `InstanceEnsureRequest` instead. This eliminates an extra
  copy of these data in the server (they were available in both the
  properties and the instance spec; now they're just available in the
  spec).

bin/mock-server/src/lib/lib.rs

@@ -232,8 +232,6 @@ async fn instance_get(
     let instance_info = api::Instance {
         properties: instance.properties.clone(),
         state: instance.state,
-        disks: vec![],
-        nics: vec![],
     };
     Ok(HttpResponseOk(api::InstanceGetResponse { instance: instance_info }))
 }

bin/propolis-cli/src/main.rs

@@ -15,7 +15,7 @@ use anyhow::{anyhow, Context};
 use clap::{Parser, Subcommand};
 use futures::{future, SinkExt};
 use newtype_uuid::{GenericUuid, TypedUuid, TypedUuidKind, TypedUuidTag};
-use propolis_client::types::InstanceMetadata;
+use propolis_client::types::{InstanceMetadata, VersionedInstanceSpec};
 use slog::{o, Drain, Level, Logger};
 use tokio::io::{AsyncReadExt, AsyncWriteExt};
 use tokio_tungstenite::tungstenite::{
@@ -253,16 +253,12 @@ async fn new_instance(
         name,
         description: "propolis-cli generated instance".to_string(),
         metadata,
-        // TODO: Use real UUID
-        image_id: Uuid::default(),
-        // TODO: Use real UUID
-        bootrom_id: Uuid::default(),
-        memory,
-        vcpus,
     };
 
     let request = InstanceEnsureRequest {
         properties,
+        vcpus,
+        memory,
         // TODO: Allow specifying NICs
         nics: vec![],
         disks,
@@ -504,17 +500,20 @@ async fn migrate_instance(
 ) -> anyhow::Result<()> {
     // Grab the instance details
     let src_instance =
-        src_client.instance_get().send().await.with_context(|| {
+        src_client.instance_spec_get().send().await.with_context(|| {
             anyhow!("failed to get src instance properties")
         })?;
-    let src_uuid = src_instance.instance.properties.id;
+    let src_uuid = src_instance.properties.id;
+    let VersionedInstanceSpec::V0(spec) = &src_instance.spec;
 
     let request = InstanceEnsureRequest {
         properties: InstanceProperties {
             // Use a new ID for the destination instance we're creating
             id: dst_uuid,
-            ..src_instance.instance.properties.clone()
+            ..src_instance.properties.clone()
         },
+        vcpus: spec.board.cpus,
+        memory: spec.board.memory_mb,
         // TODO: Handle migrating NICs
         nics: vec![],
         disks,

bin/propolis-server/src/lib/initializer.rs

@@ -191,6 +191,7 @@ pub struct MachineInitializer<'a> {
     pub(crate) producer_registry: Option<ProducerRegistry>,
     pub(crate) state: MachineInitializerState,
     pub(crate) kstat_sampler: Option<KstatSampler>,
+    pub(crate) stats_vm: crate::stats::VirtualMachine,
 }
 
 impl<'a> MachineInitializer<'a> {
@@ -744,7 +745,7 @@ impl<'a> MachineInitializer<'a> {
             track_network_interface_kstats(
                 &self.log,
                 sampler,
-                self.properties,
+                &self.stats_vm,
                 interface_ids.unwrap(),
             )
             .await
@@ -1031,15 +1032,15 @@ impl<'a> MachineInitializer<'a> {
             // unknown
             proc_upgrade: 0x2,
             // make core and thread counts equal for now
-            core_count: self.properties.vcpus,
-            core_enabled: self.properties.vcpus,
-            thread_count: self.properties.vcpus,
+            core_count: self.spec.board.cpus,
+            core_enabled: self.spec.board.cpus,
+            thread_count: self.spec.board.cpus,
             proc_characteristics: type4::Characteristics::IS_64_BIT
                 | type4::Characteristics::MULTI_CORE,
             ..Default::default()
         };
 
-        let memsize_bytes = (self.properties.memory as usize) * MB;
+        let memsize_bytes = (self.spec.board.memory_mb as usize) * MB;
         let mut smb_type16 = smbios::table::Type16 {
             location: type16::Location::SystemBoard,
             array_use: type16::ArrayUse::System,
@@ -1228,7 +1229,7 @@ impl<'a> MachineInitializer<'a> {
             self.devices.insert(format!("vcpu-{}", vcpu.id), vcpu.clone());
         }
         if let Some(sampler) = self.kstat_sampler.as_ref() {
-            track_vcpu_kstats(&self.log, sampler, self.properties).await;
+            track_vcpu_kstats(&self.log, sampler, &self.stats_vm).await;
         }
         Ok(())
     }

bin/propolis-server/src/lib/server.rs

@@ -117,7 +117,7 @@ fn instance_spec_from_request(
     request: &api::InstanceEnsureRequest,
     toml_config: &VmTomlConfig,
 ) -> Result<Spec, SpecBuilderError> {
-    let mut spec_builder = SpecBuilder::new(&request.properties);
+    let mut spec_builder = SpecBuilder::new(request.vcpus, request.memory);
 
     spec_builder.add_devices_from_config(toml_config)?;
 
@@ -376,8 +376,6 @@ async fn instance_get(
             instance: api::Instance {
                 properties: full.properties,
                 state: full.state,
-                disks: vec![],
-                nics: vec![],
             },
         })
     })

bin/propolis-server/src/lib/spec/builder.rs

@@ -15,7 +15,7 @@ use propolis_api_types::{
         },
         PciPath,
     },
-    DiskRequest, InstanceProperties, NetworkInterfaceRequest,
+    DiskRequest, NetworkInterfaceRequest,
 };
 use thiserror::Error;
 
@@ -78,10 +78,10 @@ pub(crate) struct SpecBuilder {
 }
 
 impl SpecBuilder {
-    pub fn new(properties: &InstanceProperties) -> Self {
+    pub fn new(cpus: u8, memory_mb: u64) -> Self {
         let board = Board {
-            cpus: properties.vcpus,
-            memory_mb: properties.memory,
+            cpus,
+            memory_mb,
             chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }),
         };
 
@@ -455,36 +455,16 @@ mod test {
             backends::{BlobStorageBackend, VirtioNetworkBackend},
             devices::{VirtioDisk, VirtioNic},
         },
-        InstanceMetadata, Slot, VolumeConstructionRequest,
+        Slot, VolumeConstructionRequest,
     };
     use uuid::Uuid;
 
     use crate::spec::{StorageBackend, StorageDevice};
 
     use super::*;
 
-    fn test_metadata() -> InstanceMetadata {
-        InstanceMetadata {
-            silo_id: uuid::uuid!("556a67f8-8b14-4659-bd9f-d8f85ecd36bf"),
-            project_id: uuid::uuid!("75f60038-daeb-4a1d-916a-5fa5b7237299"),
-            sled_id: uuid::uuid!("43a789ac-a0dd-4e1e-ac33-acdada142faa"),
-            sled_serial: "some-gimlet".into(),
-            sled_revision: 1,
-            sled_model: "abcd".into(),
-        }
-    }
-
     fn test_builder() -> SpecBuilder {
-        SpecBuilder::new(&InstanceProperties {
-            id: Default::default(),
-            name: Default::default(),
-            description: Default::default(),
-            metadata: test_metadata(),
-            image_id: Default::default(),
-            bootrom_id: Default::default(),
-            memory: 512,
-            vcpus: 4,
-        })
+        SpecBuilder::new(4, 512)
     }
 
     #[test]

bin/propolis-server/src/lib/stats/mod.rs

@@ -14,7 +14,6 @@ use oximeter::{
 };
 use oximeter_instruments::kstat::KstatSampler;
 use oximeter_producer::{Config, Error, Server};
-use propolis_api_types::InstanceProperties;
 use slog::Logger;
 use uuid::Uuid;
 
@@ -188,11 +187,10 @@ pub fn start_oximeter_server(
 /// Create an object that can be used to sample kstat-based metrics.
 pub(crate) fn create_kstat_sampler(
     log: &Logger,
-    properties: &InstanceProperties,
     spec: &Spec,
 ) -> Option<KstatSampler> {
     let kstat_limit = usize::try_from(
-        (u32::from(properties.vcpus) * KSTAT_LIMIT_PER_VCPU)
+        (u32::from(spec.board.cpus) * KSTAT_LIMIT_PER_VCPU)
             + (spec.nics.len() as u32 * SAMPLE_BUFFER),
     )
     .unwrap();
@@ -216,7 +214,7 @@ pub(crate) fn create_kstat_sampler(
 pub(crate) async fn track_vcpu_kstats(
     log: &Logger,
     _: &KstatSampler,
-    _: &InstanceProperties,
+    _: &VirtualMachine,
 ) {
     slog::error!(log, "vCPU stats are not supported on this platform");
 }
@@ -226,13 +224,12 @@ pub(crate) async fn track_vcpu_kstats(
 pub(crate) async fn track_vcpu_kstats(
     log: &Logger,
     sampler: &KstatSampler,
-    properties: &InstanceProperties,
+    virtual_machine: &VirtualMachine,
 ) {
-    let virtual_machine = VirtualMachine::from(properties);
     let details = oximeter_instruments::kstat::CollectionDetails::never(
         VCPU_KSTAT_INTERVAL,
     );
-    if let Err(e) = sampler.add_target(virtual_machine, details).await {
+    if let Err(e) = sampler.add_target(virtual_machine.clone(), details).await {
         slog::error!(
             log,
             "failed to add VirtualMachine target, \
@@ -247,7 +244,7 @@ pub(crate) async fn track_vcpu_kstats(
 pub(crate) async fn track_network_interface_kstats(
     log: &Logger,
     _: &KstatSampler,
-    _: &InstanceProperties,
+    _: &VirtualMachine,
     _: NetworkInterfaceIds,
 ) {
     slog::error!(
@@ -261,10 +258,10 @@ pub(crate) async fn track_network_interface_kstats(
 pub(crate) async fn track_network_interface_kstats(
     log: &Logger,
     sampler: &KstatSampler,
-    properties: &InstanceProperties,
+    virtual_machine: &VirtualMachine,
     interface_ids: NetworkInterfaceIds,
 ) {
-    let nics = InstanceNetworkInterfaces::new(properties, &interface_ids);
+    let nics = InstanceNetworkInterfaces::new(virtual_machine, &interface_ids);
     let details = oximeter_instruments::kstat::CollectionDetails::never(
         NETWORK_INTERFACE_SAMPLE_INTERVAL,
     );

bin/propolis-server/src/lib/stats/network_interface.rs

@@ -151,7 +151,7 @@ impl InstanceNetworkInterfaces {
     /// metrics from.
     #[cfg(all(not(test), target_os = "illumos"))]
     pub(crate) fn new(
-        properties: &propolis_api_types::InstanceProperties,
+        virtual_machine: &super::VirtualMachine,
         interface_ids: &NetworkInterfaceIds,
     ) -> Self {
         Self {
@@ -160,9 +160,9 @@ impl InstanceNetworkInterfaces {
                 // multiple targets in the `to_samples` method and override
                 // this.
                 interface_id: uuid::Uuid::nil(),
-                instance_id: properties.id,
-                project_id: properties.metadata.project_id,
-                silo_id: properties.metadata.silo_id,
+                instance_id: virtual_machine.target.instance_id,
+                project_id: virtual_machine.target.project_id,
+                silo_id: virtual_machine.target.silo_id,
             },
             interface_ids: interface_ids.to_vec(),
         }

bin/propolis-server/src/lib/stats/virtual_machine.rs

@@ -61,8 +61,11 @@ pub struct VirtualMachine {
     vm_name: String,
 }
 
-impl From<&propolis_api_types::InstanceProperties> for VirtualMachine {
-    fn from(properties: &propolis_api_types::InstanceProperties) -> Self {
+impl VirtualMachine {
+    pub fn new(
+        n_vcpus: u8,
+        properties: &propolis_api_types::InstanceProperties,
+    ) -> Self {
         Self {
             target: VirtualMachineTarget {
                 silo_id: properties.metadata.silo_id,
@@ -73,7 +76,7 @@ impl From<&propolis_api_types::InstanceProperties> for VirtualMachine {
                 sled_revision: properties.metadata.sled_revision,
                 sled_serial: properties.metadata.sled_serial.clone().into(),
             },
-            n_vcpus: properties.vcpus.into(),
+            n_vcpus: u32::from(n_vcpus),
             vm_name: properties.vm_name(),
         }
     }

bin/propolis-server/src/lib/vm/ensure.rs

@@ -40,7 +40,7 @@ use crate::{
         build_instance, MachineInitializer, MachineInitializerState,
     },
     spec::Spec,
-    stats::create_kstat_sampler,
+    stats::{create_kstat_sampler, VirtualMachine},
     vm::request_queue::InstanceAutoStart,
 };
 
@@ -187,10 +187,10 @@ impl<'a> VmEnsureNotStarted<'a> {
             state: MachineInitializerState::default(),
             kstat_sampler: initialize_kstat_sampler(
                 self.log,
-                properties,
                 self.instance_spec(),
                 options.oximeter_registry.clone(),
             ),
+            stats_vm: VirtualMachine::new(spec.board.cpus, properties),
         };
 
         init.initialize_rom(options.toml_config.bootrom.as_path())?;
@@ -205,7 +205,10 @@ impl<'a> VmEnsureNotStarted<'a> {
         let com1 = Arc::new(init.initialize_uart(&chipset));
         let ps2ctrl = init.initialize_ps2(&chipset);
         init.initialize_qemu_debug_port()?;
-        init.initialize_qemu_pvpanic(properties.into())?;
+        init.initialize_qemu_pvpanic(VirtualMachine::new(
+            self.instance_spec().board.cpus,
+            properties,
+        ))?;
         init.initialize_network_devices(&chipset).await?;
 
         #[cfg(not(feature = "omicron-build"))]
@@ -378,12 +381,11 @@ impl<'a> VmEnsureActive<'a> {
 /// Create an object used to sample kstats.
 fn initialize_kstat_sampler(
     log: &slog::Logger,
-    properties: &InstanceProperties,
     spec: &Spec,
     producer_registry: Option<ProducerRegistry>,
 ) -> Option<KstatSampler> {
     let registry = producer_registry?;
-    let sampler = create_kstat_sampler(log, properties, spec)?;
+    let sampler = create_kstat_sampler(log, spec)?;
 
     match registry.register_producer(sampler.clone()) {
         Ok(_) => Some(sampler),

bin/propolis-server/src/lib/vm/services.rs

@@ -14,6 +14,7 @@ use slog::{error, info, Logger};
 use crate::{
     serial::SerialTaskControlMessage,
     server::MetricsEndpointConfig,
+    spec::Spec,
     stats::{ServerStats, VirtualMachine},
     vnc::VncServer,
 };
@@ -56,16 +57,23 @@ impl VmServices {
         vm_properties: &InstanceProperties,
         ensure_options: &super::EnsureOptions,
     ) -> Self {
+        let vm_objects = vm_objects.lock_shared().await;
         let oximeter_state = if let Some(cfg) = &ensure_options.metrics_config {
             let registry = ensure_options.oximeter_registry.as_ref().expect(
                 "should have a producer registry if metrics are configured",
             );
-            register_oximeter_producer(log, cfg, registry, vm_properties).await
+            register_oximeter_producer(
+                log,
+                cfg,
+                registry,
+                vm_objects.instance_spec(),
+                vm_properties,
+            )
+            .await
         } else {
             OximeterState::default()
         };
 
-        let vm_objects = vm_objects.lock_shared().await;
         let vnc_server = ensure_options.vnc_server.clone();
         if let Some(ramfb) = vm_objects.framebuffer() {
             vnc_server.attach(vm_objects.ps2ctrl().clone(), ramfb.clone());
@@ -110,10 +118,11 @@ async fn register_oximeter_producer(
     log: &slog::Logger,
     cfg: &MetricsEndpointConfig,
     registry: &ProducerRegistry,
+    spec: &Spec,
     vm_properties: &InstanceProperties,
 ) -> OximeterState {
     let mut oximeter_state = OximeterState::default();
-    let virtual_machine = VirtualMachine::from(vm_properties);
+    let virtual_machine = VirtualMachine::new(spec.board.cpus, vm_properties);
 
     // Create the server itself.
     //