sled-agent: move instance configuration generation to Nexus (#8002)

One of the determinations in RFD 505 is that Nexus should be the
component that's in charge of determining how to configure a VM given a
set of database records describing its instance (the Instance itself,
its attached Disks and NetworkInterfaces, etc.). To summarize the
rationale in the RFD, the hope is that this will promote two nice
properties:

- **Local reasoning about virtual platforms**: All the logic that
translates instance descriptions into VM specs now lives in a single
module in Nexus. In past iterations of the code, Nexus transformed
database records into an intermediate sled-agent type, and sled-agent
would transform those into Propolis API types, which Propolis would then
use to fill in virtual hardware details. Understanding where a VM's
configuration came from required the reader to look at all these
components; now all the relevant logic lives in Nexus.
- **Serviceability**: Putting type transformations and platform policies
into sled-agent and Propolis makes them marginally more painful to
update, since updating these components requires the system to migrate
VMs and reboot sleds. Putting the virtual platform policy in Nexus will
make it much less expensive to update in the future.

To achieve this:

- Move sled-agent's virtual platform logic (added in #7211) into a new
Nexus module. Sled-agent needs to hold onto a bit of logic to insert
OPTE port names into instance specs before sending those specs to
Propolis; this needs to live in the agent since it selects the relevant
object names.
- Update the sled-agent instance registration API to take a Propolis
instance spec as a parameter (and rework some other types to distinguish
a bit more clearly between "Propolis VM configuration" and "sled-agent
objects that need to be created to support this VM").

Tested by booting a VM in a dev cluster, booting a comparable VM on
rack2, and comparing their instance specs (as returned by Propolis's
`/instance/spec` API) to make sure they specified the same components
with the same configuration.

Author: gjcolombo

Cargo.lock

@@ -387,10 +387,10 @@ crossterm = { version = "0.28.1", features = ["event-stream"] }
 # NOTE: if you change the pinned revision of the `crucible` dependencies, you
 # must also update the references in package-manifest.toml to match the new
 # revision.
-crucible-agent-client = { git = "https://github.com/oxidecomputer/crucible", rev = "da3cf198a0e000bb89efc3a1c77d7ba09340a600" }
-crucible-pantry-client = { git = "https://github.com/oxidecomputer/crucible", rev = "da3cf198a0e000bb89efc3a1c77d7ba09340a600" }
-crucible-smf = { git = "https://github.com/oxidecomputer/crucible", rev = "da3cf198a0e000bb89efc3a1c77d7ba09340a600" }
-crucible-common = { git = "https://github.com/oxidecomputer/crucible", rev = "da3cf198a0e000bb89efc3a1c77d7ba09340a600" }
+crucible-agent-client = { git = "https://github.com/oxidecomputer/crucible", rev = "45801597f410685015ac2704d044919a41e3ff75" }
+crucible-pantry-client = { git = "https://github.com/oxidecomputer/crucible", rev = "45801597f410685015ac2704d044919a41e3ff75" }
+crucible-smf = { git = "https://github.com/oxidecomputer/crucible", rev = "45801597f410685015ac2704d044919a41e3ff75" }
+crucible-common = { git = "https://github.com/oxidecomputer/crucible", rev = "45801597f410685015ac2704d044919a41e3ff75" }
 # NOTE: See above!
 csv = "1.3.1"
 curve25519-dalek = "4"
@@ -604,10 +604,10 @@ progenitor-client = "0.9.1"
 # NOTE: if you change the pinned revision of the `bhyve_api` and propolis
 # dependencies, you must also update the references in package-manifest.toml to
 # match the new revision.
-bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "e5c85d84b0a51803caffb335a1063612edb02f6d" }
-propolis_api_types = { git = "https://github.com/oxidecomputer/propolis", rev = "e5c85d84b0a51803caffb335a1063612edb02f6d" }
-propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "e5c85d84b0a51803caffb335a1063612edb02f6d" }
-propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "e5c85d84b0a51803caffb335a1063612edb02f6d" }
+bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "060a204d91e401a368c700a09d24510b7cd2b0e4" }
+propolis_api_types = { git = "https://github.com/oxidecomputer/propolis", rev = "060a204d91e401a368c700a09d24510b7cd2b0e4" }
+propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "060a204d91e401a368c700a09d24510b7cd2b0e4" }
+propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "060a204d91e401a368c700a09d24510b7cd2b0e4" }
 # NOTE: see above!
 proptest = "1.6.0"
 qorb = "0.3.1"

Cargo.toml

@@ -23,5 +23,6 @@ reqwest = { workspace = true, features = [ "json", "rustls-tls", "stream" ] }
 schemars.workspace = true
 serde.workspace = true
 serde_json.workspace = true
+sled-agent-types.workspace = true
 slog.workspace = true
 uuid.workspace = true

clients/sled-agent-client/Cargo.toml

@@ -56,6 +56,7 @@ progenitor::generate_api!(
         DiskVariant = omicron_common::disk::DiskVariant,
         ExternalIpGatewayMap = omicron_common::api::internal::shared::ExternalIpGatewayMap,
         Generation = omicron_common::api::external::Generation,
+        Hostname = omicron_common::api::external::Hostname,
         ImportExportPolicy = omicron_common::api::external::ImportExportPolicy,
         Inventory = nexus_sled_agent_shared::inventory::Inventory,
         InventoryDisk = nexus_sled_agent_shared::inventory::InventoryDisk,
@@ -117,14 +118,6 @@ impl From<omicron_common::api::internal::nexus::VmmState> for types::VmmState {
     }
 }
 
-impl From<omicron_common::api::external::InstanceCpuCount>
-    for types::InstanceCpuCount
-{
-    fn from(s: omicron_common::api::external::InstanceCpuCount) -> Self {
-        Self(s.0)
-    }
-}
-
 impl From<types::VmmState> for omicron_common::api::internal::nexus::VmmState {
     fn from(s: types::VmmState) -> Self {
         use omicron_common::api::internal::nexus::VmmState as Output;
@@ -188,14 +181,6 @@ impl From<types::MigrationState>
     }
 }
 
-impl From<types::InstanceCpuCount>
-    for omicron_common::api::external::InstanceCpuCount
-{
-    fn from(s: types::InstanceCpuCount) -> Self {
-        Self(s.0)
-    }
-}
-
 impl From<omicron_common::api::internal::nexus::DiskRuntimeState>
     for types::DiskRuntimeState
 {

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

@@ -92,6 +92,7 @@ serde_urlencoded.workspace = true
 serde_with.workspace = true
 sha2.workspace = true
 sled-agent-client.workspace = true
+sled-agent-types.workspace = true
 slog.workspace = true
 slog-async.workspace = true
 slog-dtrace.workspace = true
@@ -162,7 +163,6 @@ pretty_assertions.workspace = true
 rcgen.workspace = true
 regex.workspace = true
 similar-asserts.workspace = true
-sled-agent-types.workspace = true
 sp-sim.workspace = true
 rustls.workspace = true
 subprocess.workspace = true

nexus/Cargo.toml

@@ -52,9 +52,3 @@ where
             .map_err(|e| e.into())
     }
 }
-
-impl From<InstanceCpuCount> for sled_agent_client::types::InstanceCpuCount {
-    fn from(i: InstanceCpuCount) -> Self {
-        Self(i.0.0)
-    }
-}

nexus/db-model/src/instance_cpu_count.rs

@@ -14,7 +14,6 @@ use super::MAX_VCPU_PER_INSTANCE;
 use super::MIN_MEMORY_BYTES_PER_INSTANCE;
 use crate::app::sagas;
 use crate::app::sagas::NexusSaga;
-use crate::cidata::InstanceCiData;
 use crate::external_api::params;
 use cancel_safe_futures::prelude::*;
 use futures::future::Fuse;
@@ -41,6 +40,7 @@ use omicron_common::api::external::CreateResult;
 use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::DeleteResult;
 use omicron_common::api::external::Error;
+use omicron_common::api::external::Hostname;
 use omicron_common::api::external::InstanceCpuCount;
 use omicron_common::api::external::InstanceState;
 use omicron_common::api::external::InternalContext;
@@ -64,9 +64,7 @@ use propolis_client::support::tungstenite::protocol::frame::coding::CloseCode;
 use sagas::instance_common::ExternalIpAttach;
 use sagas::instance_start;
 use sagas::instance_update;
-use sled_agent_client::types::InstanceBootSettings;
 use sled_agent_client::types::InstanceMigrationTargetParams;
-use sled_agent_client::types::InstanceProperties;
 use sled_agent_client::types::VmmPutStateBody;
 use std::matches;
 use std::net::SocketAddr;
@@ -1080,7 +1078,7 @@ impl super::Nexus {
         // TODO-cleanup: This can be removed when we are confident that no
         // instances exist prior to the addition of strict hostname validation
         // in the API.
-        let Ok(hostname) = db_instance.hostname.parse() else {
+        let Ok(hostname) = db_instance.hostname.parse::<Hostname>() else {
             let msg = format!(
                 "The instance hostname '{}' is no longer valid. \
                 To access the data on its disks, this instance \
@@ -1107,57 +1105,6 @@ impl super::Nexus {
             )
             .await?;
 
-        let mut disk_reqs = vec![];
-        for disk in &disks {
-            // Disks that are attached to an instance should always have a slot
-            // assignment, but if for some reason this one doesn't, return an
-            // error instead of taking down the whole process.
-            let slot = match disk.slot {
-                Some(s) => s,
-                None => {
-                    error!(self.log, "attached disk has no PCI slot assignment";
-                       "disk_id" => %disk.id(),
-                       "disk_name" => disk.name().to_string(),
-                       "instance_id" => ?disk.runtime_state.attach_instance_id);
-
-                    return Err(Error::internal_error(&format!(
-                        "disk {} is attached but has no PCI slot assignment",
-                        disk.id()
-                    ))
-                    .into());
-                }
-            };
-
-            let volume = self
-                .db_datastore
-                .volume_checkout(
-                    disk.volume_id(),
-                    match operation {
-                        InstanceRegisterReason::Start { vmm_id } =>
-                            db::datastore::VolumeCheckoutReason::InstanceStart { vmm_id },
-                        InstanceRegisterReason::Migrate { vmm_id, target_vmm_id } =>
-                            db::datastore::VolumeCheckoutReason::InstanceMigrate { vmm_id, target_vmm_id },
-                    }
-                )
-                .await?;
-
-            disk_reqs.push(sled_agent_client::types::InstanceDisk {
-                disk_id: disk.id(),
-                name: disk.name().to_string(),
-                slot: slot.0,
-                read_only: false,
-                vcr_json: volume.data().to_owned(),
-            });
-        }
-
-        // The routines that maintain an instance's boot options are supposed to
-        // guarantee that the boot disk ID, if present, is the ID of an attached
-        // disk. If this invariant isn't upheld, Propolis will catch the failure
-        // when it processes its received VM configuration.
-        let boot_settings = db_instance
-            .boot_disk_id
-            .map(|id| InstanceBootSettings { order: vec![id] });
-
         let nics = self
             .db_datastore
             .derive_guest_network_interface_info(&opctx, &authz_instance)
@@ -1271,28 +1218,25 @@ impl super::Nexus {
                         .unwrap(),
                 }),
             )
-            .await?
-            .into_iter();
+            .await?;
 
-        let ssh_keys: Vec<String> =
-            ssh_keys.map(|ssh_key| ssh_key.public_key).collect();
+        let vmm_spec = self
+            .generate_vmm_spec(
+                &operation,
+                db_instance,
+                &disks,
+                &nics,
+                &ssh_keys,
+            )
+            .await?;
 
         let metadata = sled_agent_client::types::InstanceMetadata {
             silo_id: authz_silo.id(),
             project_id: authz_project.id(),
         };
 
-        // Ask the sled agent to begin the state change.  Then update the
-        // database to reflect the new intermediate state.  If this update is
-        // not the newest one, that's fine.  That might just mean the sled agent
-        // beat us to it.
-
-        let instance_hardware = sled_agent_client::types::InstanceHardware {
-            properties: InstanceProperties {
-                ncpus: db_instance.ncpus.into(),
-                memory: db_instance.memory.into(),
-                hostname,
-            },
+        let local_config = sled_agent_client::types::InstanceSledLocalConfig {
+            hostname,
             nics,
             source_nat,
             ephemeral_ip,
@@ -1304,12 +1248,6 @@ impl super::Nexus {
                 host_domain: None,
                 search_domains: Vec::new(),
             },
-            disks: disk_reqs,
-            boot_settings,
-            cloud_init_bytes: Some(base64::Engine::encode(
-                &base64::engine::general_purpose::STANDARD,
-                db_instance.generate_cidata(&ssh_keys)?,
-            )),
         };
 
         let instance_id = InstanceUuid::from_untyped_uuid(db_instance.id());
@@ -1338,7 +1276,8 @@ impl super::Nexus {
             .vmm_register(
                 propolis_id,
                 &sled_agent_client::types::InstanceEnsureBody {
-                    hardware: instance_hardware,
+                    vmm_spec,
+                    local_config,
                     migration_id: db_instance.runtime().migration_id,
                     vmm_runtime,
                     instance_id,