From c1281a9b98a576395d886630167c0dac22ef4024 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 20 Dec 2024 15:57:23 -0800 Subject: [PATCH] ingest new Propolis VM creation API (#7211) Update Omicron to use the new Propolis VM creation API defined in oxidecomputer/propolis#813 and oxidecomputer/propolis#816. This API requires clients to pass instance specs to create new VMs and component replacement lists to migrate existing VMs. Construct these in sled agent for now; in the future this logic can move to Nexus and become part of a robust virtual platform abstraction. For now the goal is just to keep everything working for existing VMs while adapting to the new Propolis API. Slightly adjust the sled agent instance APIs so that Nexus specifies disks and boot orderings using sled-agent-defined types and not re-exported Propolis types. Finally, adapt Nexus to the fact that Crucible's `VolumeConstructionRequest` and `CrucibleOpts` types no longer appear in Propolis's generated client (and so don't appear in sled agent's client either). Instead, the `propolis-client` crate re-exports these types directly from its `crucible-client-types` dependency. For the most part, this involves updating `use` directives and storing `SocketAddr`s in their natively typed form instead of converting them to and from strings. Tests: cargo nextest, plus ad hoc testing in a dev cluster as described in the PR comments. --- Cargo.lock | 22 +- Cargo.toml | 8 +- clients/sled-agent-client/Cargo.toml | 1 + clients/sled-agent-client/src/lib.rs | 2 + dev-tools/ls-apis/tests/api_dependencies.out | 4 +- dev-tools/omdb/src/bin/omdb/db.rs | 2 +- .../src/db/datastore/region_replacement.rs | 2 +- .../datastore/region_snapshot_replacement.rs | 2 +- nexus/db-queries/src/db/datastore/volume.rs | 278 ++++++------ .../src/db/datastore/volume_repair.rs | 2 +- .../background/tasks/region_replacement.rs | 4 +- .../tasks/region_replacement_driver.rs | 2 +- .../region_snapshot_replacement_finish.rs | 2 +- ...on_snapshot_replacement_garbage_collect.rs | 2 +- .../region_snapshot_replacement_start.rs | 9 +- .../tasks/region_snapshot_replacement_step.rs | 34 +- nexus/src/app/image.rs | 2 +- nexus/src/app/instance.rs | 63 +-- nexus/src/app/sagas/disk_create.rs | 6 +- nexus/src/app/sagas/instance_migrate.rs | 1 - .../src/app/sagas/region_replacement_drive.rs | 2 - .../app/sagas/region_replacement_finish.rs | 4 +- .../src/app/sagas/region_replacement_start.rs | 8 +- ...on_snapshot_replacement_garbage_collect.rs | 4 +- .../region_snapshot_replacement_start.rs | 13 +- .../sagas/region_snapshot_replacement_step.rs | 9 +- ...apshot_replacement_step_garbage_collect.rs | 4 +- nexus/src/app/sagas/snapshot_create.rs | 93 ++-- nexus/src/app/sagas/volume_delete.rs | 2 +- nexus/src/app/sagas/volume_remove_rop.rs | 2 +- .../integration_tests/volume_management.rs | 84 ++-- openapi/sled-agent.json | 327 +++----------- package-manifest.toml | 4 +- sled-agent/src/instance.rs | 401 +++++++++++++++--- sled-agent/src/sim/http_entrypoints_pantry.rs | 2 +- sled-agent/src/sim/sled_agent.rs | 44 +- sled-agent/src/sim/storage.rs | 2 +- sled-agent/types/src/instance.rs | 41 +- 38 files changed, 799 insertions(+), 695 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5048914c74..1aec1ccc89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -693,7 +693,7 @@ dependencies = [ [[package]] name = "bhyve_api" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739#220a6f367c18f2452dbc4fa9086f3fe73b961739" +source = "git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b#d4529fd8247386b422b78e1203315d5baea5ea8b" dependencies = [ "bhyve_api_sys", "libc", @@ -703,7 +703,7 @@ dependencies = [ [[package]] name = "bhyve_api_sys" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739#220a6f367c18f2452dbc4fa9086f3fe73b961739" +source = "git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b#d4529fd8247386b422b78e1203315d5baea5ea8b" dependencies = [ "libc", "strum", @@ -7023,7 +7023,7 @@ dependencies = [ "pq-sys", "pretty_assertions", "progenitor-client", - "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739)", + "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b)", "qorb", "rand", "rcgen", @@ -7289,7 +7289,7 @@ dependencies = [ "oximeter-producer", "oxnet", "pretty_assertions", - "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739)", + "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b)", "propolis-mock-server", "propolis_api_types", "rand", @@ -9004,12 +9004,14 @@ dependencies = [ [[package]] name = "propolis-client" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739#220a6f367c18f2452dbc4fa9086f3fe73b961739" +source = "git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b#d4529fd8247386b422b78e1203315d5baea5ea8b" dependencies = [ "async-trait", "base64 0.21.7", + "crucible-client-types", "futures", "progenitor", + "propolis_api_types", "rand", "reqwest", "schemars", @@ -9046,7 +9048,7 @@ dependencies = [ [[package]] name = "propolis-mock-server" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739#220a6f367c18f2452dbc4fa9086f3fe73b961739" +source = "git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b#d4529fd8247386b422b78e1203315d5baea5ea8b" dependencies = [ "anyhow", "atty", @@ -9088,12 +9090,13 @@ dependencies = [ [[package]] name = "propolis_api_types" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739#220a6f367c18f2452dbc4fa9086f3fe73b961739" +source = "git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b#d4529fd8247386b422b78e1203315d5baea5ea8b" dependencies = [ "crucible-client-types", "propolis_types", "schemars", "serde", + "serde_with", "thiserror 1.0.69", "uuid", ] @@ -9101,7 +9104,7 @@ dependencies = [ [[package]] name = "propolis_types" version = "0.0.0" -source = "git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739#220a6f367c18f2452dbc4fa9086f3fe73b961739" +source = "git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b#d4529fd8247386b422b78e1203315d5baea5ea8b" dependencies = [ "schemars", "serde", @@ -10739,6 +10742,7 @@ dependencies = [ "omicron-workspace-hack", "oxnet", "progenitor", + "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b)", "regress 0.9.1", "reqwest", "schemars", @@ -10764,7 +10768,7 @@ dependencies = [ "omicron-uuid-kinds", "omicron-workspace-hack", "oxnet", - "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=220a6f367c18f2452dbc4fa9086f3fe73b961739)", + "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=d4529fd8247386b422b78e1203315d5baea5ea8b)", "rcgen", "schemars", "serde", diff --git a/Cargo.toml b/Cargo.toml index 3d29f61cf9..8906ab4d70 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -547,10 +547,10 @@ prettyplease = { version = "0.2.25", features = ["verbatim"] } proc-macro2 = "1.0" progenitor = "0.8.0" progenitor-client = "0.8.0" -bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "220a6f367c18f2452dbc4fa9086f3fe73b961739" } -propolis_api_types = { git = "https://github.com/oxidecomputer/propolis", rev = "220a6f367c18f2452dbc4fa9086f3fe73b961739" } -propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "220a6f367c18f2452dbc4fa9086f3fe73b961739" } -propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "220a6f367c18f2452dbc4fa9086f3fe73b961739" } +bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "d4529fd8247386b422b78e1203315d5baea5ea8b" } +propolis_api_types = { git = "https://github.com/oxidecomputer/propolis", rev = "d4529fd8247386b422b78e1203315d5baea5ea8b" } +propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "d4529fd8247386b422b78e1203315d5baea5ea8b" } +propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "d4529fd8247386b422b78e1203315d5baea5ea8b" } proptest = "1.5.0" qorb = "0.2.1" quote = "1.0" diff --git a/clients/sled-agent-client/Cargo.toml b/clients/sled-agent-client/Cargo.toml index 770588d6b4..e6c77fe24a 100644 --- a/clients/sled-agent-client/Cargo.toml +++ b/clients/sled-agent-client/Cargo.toml @@ -17,6 +17,7 @@ omicron-uuid-kinds.workspace = true omicron-workspace-hack.workspace = true oxnet.workspace = true progenitor.workspace = true +propolis-client.workspace = true regress.workspace = true reqwest = { workspace = true, features = [ "json", "rustls-tls", "stream" ] } schemars.workspace = true diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index f2eb957650..6a9c721587 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/clients/sled-agent-client/src/lib.rs @@ -12,6 +12,8 @@ use serde::Serialize; use std::convert::TryFrom; use uuid::Uuid; +pub use propolis_client::{CrucibleOpts, VolumeConstructionRequest}; + progenitor::generate_api!( spec = "../../openapi/sled-agent.json", derives = [schemars::JsonSchema, PartialEq], diff --git a/dev-tools/ls-apis/tests/api_dependencies.out b/dev-tools/ls-apis/tests/api_dependencies.out index 8e061f2906..d2b672e62c 100644 --- a/dev-tools/ls-apis/tests/api_dependencies.out +++ b/dev-tools/ls-apis/tests/api_dependencies.out @@ -67,8 +67,8 @@ Oximeter (client: oximeter-client) consumed by: omicron-nexus (omicron/nexus) via 2 paths Propolis (client: propolis-client) - consumed by: omicron-nexus (omicron/nexus) via 2 paths - consumed by: omicron-sled-agent (omicron/sled-agent) via 1 path + consumed by: omicron-nexus (omicron/nexus) via 3 paths + consumed by: omicron-sled-agent (omicron/sled-agent) via 2 paths Crucible Repair (client: repair-client) consumed by: crucible-downstairs (crucible/downstairs) via 1 path diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index e501e650b1..629e465212 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -135,7 +135,7 @@ use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::PropolisUuid; use omicron_uuid_kinds::SledUuid; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::VolumeConstructionRequest; use std::borrow::Cow; use std::cmp::Ordering; use std::collections::BTreeMap; diff --git a/nexus/db-queries/src/db/datastore/region_replacement.rs b/nexus/db-queries/src/db/datastore/region_replacement.rs index 8aad7f2cfd..e4a7d78224 100644 --- a/nexus/db-queries/src/db/datastore/region_replacement.rs +++ b/nexus/db-queries/src/db/datastore/region_replacement.rs @@ -924,7 +924,7 @@ mod test { use crate::db::pub_test_utils::TestDatabase; use omicron_test_utils::dev; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; #[tokio::test] async fn test_one_replacement_per_volume() { diff --git a/nexus/db-queries/src/db/datastore/region_snapshot_replacement.rs b/nexus/db-queries/src/db/datastore/region_snapshot_replacement.rs index 90b014c582..5568822e07 100644 --- a/nexus/db-queries/src/db/datastore/region_snapshot_replacement.rs +++ b/nexus/db-queries/src/db/datastore/region_snapshot_replacement.rs @@ -1371,7 +1371,7 @@ mod test { use crate::db::pub_test_utils::TestDatabase; use omicron_test_utils::dev; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; #[tokio::test] async fn test_one_replacement_per_volume() { diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index 505533da50..e55c374073 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -58,7 +58,7 @@ use omicron_uuid_kinds::UpstairsRepairKind; use serde::Deserialize; use serde::Deserializer; use serde::Serialize; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::VolumeConstructionRequest; use std::collections::HashSet; use std::collections::VecDeque; use std::net::AddrParseError; @@ -1118,7 +1118,7 @@ impl DataStore { ) -> CreateResult { let volume = self.volume_checkout(volume_id, reason).await?; - let vcr: sled_agent_client::types::VolumeConstructionRequest = + let vcr: sled_agent_client::VolumeConstructionRequest = serde_json::from_str(volume.data())?; let randomized_vcr = serde_json::to_string( @@ -2578,10 +2578,15 @@ fn region_in_vcr( VolumeConstructionRequest::Region { opts, .. } => { for target in &opts.target { - let parsed_target: SocketAddrV6 = target.parse()?; - if parsed_target == *region { - region_found = true; - break; + match target { + SocketAddr::V6(t) if *t == *region => { + region_found = true; + break; + } + SocketAddr::V6(_) => {} + SocketAddr::V4(_) => { + bail!("region target contains an IPv4 address"); + } } } } @@ -2643,9 +2648,16 @@ fn read_only_target_in_vcr( } for target in &opts.target { - let parsed_target: SocketAddrV6 = target.parse()?; - if parsed_target == *read_only_target && opts.read_only { - return Ok(true); + match target { + SocketAddr::V6(t) + if *t == *read_only_target && opts.read_only => + { + return Ok(true); + } + SocketAddr::V6(_) => {} + SocketAddr::V4(_) => { + bail!("region target contains an IPv4 address"); + } } } } @@ -3177,9 +3189,9 @@ impl DataStore { blocks_per_extent: 1, extent_count: 1, gen: 1, - opts: sled_agent_client::types::CrucibleOpts { + opts: sled_agent_client::CrucibleOpts { id: volume_to_delete_id.0, - target: vec![existing.0.to_string()], + target: vec![existing.0.into()], lossy: false, flush_timeout: None, key: None, @@ -3442,7 +3454,9 @@ pub fn read_only_resources_associated_with_volume( VolumeConstructionRequest::Region { opts, .. } => { for target in &opts.target { if opts.read_only { - crucible_targets.read_only_targets.push(target.clone()); + crucible_targets + .read_only_targets + .push(target.to_string()); } } } @@ -3481,7 +3495,7 @@ pub fn read_write_resources_associated_with_volume( VolumeConstructionRequest::Region { opts, .. } => { if !opts.read_only { for target in &opts.target { - targets.push(target.clone()); + targets.push(target.to_string()); } } } @@ -3597,10 +3611,11 @@ fn replace_region_in_vcr( VolumeConstructionRequest::Region { opts, gen, .. } => { for target in &mut opts.target { - let parsed_target: SocketAddrV6 = target.parse()?; - if parsed_target == old_region { - *target = new_region.to_string(); - old_region_found = true; + if let SocketAddr::V6(target) = target { + if *target == old_region { + *target = new_region; + old_region_found = true; + } } } @@ -3681,10 +3696,11 @@ fn replace_read_only_target_in_vcr( } for target in &mut opts.target { - let parsed_target: SocketAddrV6 = target.parse()?; - if parsed_target == old_target.0 && opts.read_only { - *target = new_target.0.to_string(); - replacements += 1; + if let SocketAddr::V6(target) = target { + if *target == old_target.0 && opts.read_only { + *target = new_target.0; + replacements += 1; + } } } } @@ -3727,9 +3743,10 @@ fn find_matching_rw_regions_in_volume( VolumeConstructionRequest::Region { opts, .. } => { if !opts.read_only { for target in &opts.target { - let parsed_target: SocketAddrV6 = target.parse()?; - if parsed_target.ip() == ip { - matched_targets.push(parsed_target); + if let SocketAddr::V6(target) = target { + if target.ip() == ip { + matched_targets.push(*target); + } } } } @@ -4006,7 +4023,7 @@ mod tests { use nexus_types::external_api::params::DiskSource; use omicron_common::api::external::ByteCount; use omicron_test_utils::dev; - use sled_agent_client::types::CrucibleOpts; + use sled_agent_client::CrucibleOpts; // Assert that Nexus will not fail to deserialize an old version of // CrucibleResources that was serialized before schema update 6.0.0. @@ -4145,7 +4162,7 @@ mod tests { .await .unwrap(); - let mut region_addresses: Vec = + let mut region_addresses: Vec = Vec::with_capacity(datasets_and_regions.len()); for (i, (_, region)) in datasets_and_regions.iter().enumerate() { @@ -4162,7 +4179,7 @@ mod tests { let address: SocketAddrV6 = datastore.region_addr(region.id()).await.unwrap().unwrap(); - region_addresses.push(address.to_string()); + region_addresses.push(address); } // Manually create a replacement region at the first dataset @@ -4209,9 +4226,9 @@ mod tests { id: volume_id, target: vec![ // target to replace - region_addresses[0].clone(), - region_addresses[1].clone(), - region_addresses[2].clone(), + region_addresses[0].into(), + region_addresses[1].into(), + region_addresses[2].into(), ], lossy: false, flush_timeout: None, @@ -4238,7 +4255,7 @@ mod tests { db::datastore::VolumeReplacementParams { volume_id, region_id: datasets_and_regions[0].1.id(), - region_addr: region_addresses[0].parse().unwrap(), + region_addr: region_addresses[0], }, /* replacement */ db::datastore::VolumeReplacementParams { @@ -4271,9 +4288,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - replacement_region_addr.to_string(), // replaced - region_addresses[1].clone(), - region_addresses[2].clone(), + replacement_region_addr.into(), // replaced + region_addresses[1].into(), + region_addresses[2].into(), ], lossy: false, flush_timeout: None, @@ -4302,7 +4319,7 @@ mod tests { db::datastore::VolumeReplacementParams { volume_id: volume_to_delete_id, region_id: datasets_and_regions[0].1.id(), - region_addr: region_addresses[0].parse().unwrap(), + region_addr: region_addresses[0], }, ) .await @@ -4329,9 +4346,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - region_addresses[0].clone(), // back to what it was - region_addresses[1].clone(), - region_addresses[2].clone(), + region_addresses[0].into(), // back to what it was + region_addresses[1].into(), + region_addresses[2].into(), ], lossy: false, flush_timeout: None, @@ -4383,7 +4400,7 @@ mod tests { .await .unwrap(); - let mut region_addresses: Vec = + let mut region_addresses: Vec = Vec::with_capacity(datasets_and_regions.len()); for (i, (_, region)) in datasets_and_regions.iter().enumerate() { @@ -4400,7 +4417,7 @@ mod tests { let address: SocketAddrV6 = datastore.region_addr(region.id()).await.unwrap().unwrap(); - region_addresses.push(address.to_string()); + region_addresses.push(address); } // Manually create a replacement region at the first dataset @@ -4435,28 +4452,31 @@ mod tests { // need to add region snapshot objects to satisfy volume create // transaction's search for resources - let address_1 = String::from("[fd00:1122:3344:104::1]:400"); - let address_2 = String::from("[fd00:1122:3344:105::1]:401"); - let address_3 = String::from("[fd00:1122:3344:106::1]:402"); + let address_1: SocketAddrV6 = + "[fd00:1122:3344:104::1]:400".parse().unwrap(); + let address_2: SocketAddrV6 = + "[fd00:1122:3344:105::1]:401".parse().unwrap(); + let address_3: SocketAddrV6 = + "[fd00:1122:3344:106::1]:402".parse().unwrap(); let region_snapshots = [ RegionSnapshot::new( DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - address_1.clone(), + address_1.to_string(), ), RegionSnapshot::new( DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - address_2.clone(), + address_2.to_string(), ), RegionSnapshot::new( DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - address_3.clone(), + address_3.to_string(), ), ]; @@ -4493,9 +4513,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - region_addresses[0].clone(), - region_addresses[1].clone(), - region_addresses[2].clone(), + region_addresses[0].into(), + region_addresses[1].into(), + region_addresses[2].into(), ], lossy: false, flush_timeout: None, @@ -4517,9 +4537,9 @@ mod tests { id: rop_id, target: vec![ // target to replace - address_1.clone(), - address_2.clone(), - address_3.clone(), + address_1.into(), + address_2.into(), + address_3.into(), ], lossy: false, flush_timeout: None, @@ -4587,7 +4607,7 @@ mod tests { let volume_replace_snapshot_result = datastore .volume_replace_snapshot( VolumeWithTarget(volume_id), - ExistingTarget(address_1.parse().unwrap()), + ExistingTarget(address_1), ReplacementTarget(replacement_region_addr), VolumeToDelete(volume_to_delete_id), ) @@ -4616,9 +4636,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - region_addresses[0].clone(), - region_addresses[1].clone(), - region_addresses[2].clone(), + region_addresses[0].into(), + region_addresses[1].into(), + region_addresses[2].into(), ], lossy: false, flush_timeout: None, @@ -4640,9 +4660,9 @@ mod tests { id: rop_id, target: vec![ // target replaced - replacement_region_addr.to_string(), - address_2.clone(), - address_3.clone(), + replacement_region_addr.into(), + address_2.into(), + address_3.into(), ], lossy: false, flush_timeout: None, @@ -4682,7 +4702,7 @@ mod tests { id: volume_to_delete_id, target: vec![ // replaced target stashed here - address_1.clone(), + address_1.into(), ], lossy: false, flush_timeout: None, @@ -4745,7 +4765,7 @@ mod tests { .volume_replace_snapshot( VolumeWithTarget(volume_id), ExistingTarget(replacement_region_addr), - ReplacementTarget(address_1.parse().unwrap()), + ReplacementTarget(address_1), VolumeToDelete(volume_to_delete_id), ) .await @@ -4772,9 +4792,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - region_addresses[0].clone(), - region_addresses[1].clone(), - region_addresses[2].clone(), + region_addresses[0].into(), + region_addresses[1].into(), + region_addresses[2].into(), ], lossy: false, flush_timeout: None, @@ -4796,7 +4816,9 @@ mod tests { id: rop_id, target: vec![ // back to what it was - address_1, address_2, address_3, + address_1.into(), + address_2.into(), + address_3.into(), ], lossy: false, flush_timeout: None, @@ -4836,7 +4858,7 @@ mod tests { id: volume_to_delete_id, target: vec![ // replacement stashed here - replacement_region_addr.to_string(), + replacement_region_addr.into(), ], lossy: false, flush_timeout: None, @@ -4899,16 +4921,19 @@ mod tests { // need to add region snapshot objects to satisfy volume create // transaction's search for resources - let address_1 = String::from("[fd00:1122:3344:104::1]:400"); - let address_2 = String::from("[fd00:1122:3344:105::1]:401"); - let address_3 = String::from("[fd00:1122:3344:106::1]:402"); + let address_1: SocketAddrV6 = + "[fd00:1122:3344:104::1]:400".parse().unwrap(); + let address_2: SocketAddrV6 = + "[fd00:1122:3344:105::1]:401".parse().unwrap(); + let address_3: SocketAddrV6 = + "[fd00:1122:3344:106::1]:402".parse().unwrap(); datastore .region_snapshot_create(RegionSnapshot::new( DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - address_1.clone(), + address_1.to_string(), )) .await .unwrap(); @@ -4917,7 +4942,7 @@ mod tests { DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - address_2.clone(), + address_2.to_string(), )) .await .unwrap(); @@ -4926,7 +4951,7 @@ mod tests { DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - address_3.clone(), + address_3.to_string(), )) .await .unwrap(); @@ -4949,9 +4974,9 @@ mod tests { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - address_1.clone(), - address_2, - address_3, + address_1.into(), + address_2.into(), + address_3.into(), ], lossy: false, flush_timeout: None, @@ -4971,10 +4996,7 @@ mod tests { .unwrap(); let volumes = datastore - .find_volumes_referencing_socket_addr( - &opctx, - address_1.parse().unwrap(), - ) + .find_volumes_referencing_socket_addr(&opctx, address_1.into()) .await .unwrap(); @@ -5014,9 +5036,9 @@ mod tests { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - String::from("[fd00:1122:3344:105::1]:401"), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + "[fd00:1122:3344:105::1]:401".parse().unwrap(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5050,9 +5072,9 @@ mod tests { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - String::from("[fd00:1122:3344:105::1]:401"), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + "[fd00:1122:3344:105::1]:401".parse().unwrap(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5089,9 +5111,9 @@ mod tests { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - String::from("[fd00:1122:3344:105::1]:401"), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + "[fd00:1122:3344:105::1]:401".parse().unwrap(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5133,9 +5155,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - String::from("[fd00:1122:3344:105::1]:401"), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + "[fd00:1122:3344:105::1]:401".parse().unwrap(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5175,9 +5197,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - new_target.0.to_string(), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + new_target.0.into(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5210,9 +5232,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd55:1122:3344:204::1]:1000"), - String::from("[fd55:1122:3344:205::1]:1001"), - String::from("[fd55:1122:3344:206::1]:1002"), + "[fd55:1122:3344:204::1]:1000".parse().unwrap(), + "[fd55:1122:3344:205::1]:1001".parse().unwrap(), + "[fd55:1122:3344:206::1]:1002".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5233,9 +5255,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd33:1122:3344:304::1]:2000"), - String::from("[fd33:1122:3344:305::1]:2001"), - String::from("[fd33:1122:3344:306::1]:2002"), + "[fd33:1122:3344:304::1]:2000".parse().unwrap(), + "[fd33:1122:3344:305::1]:2001".parse().unwrap(), + "[fd33:1122:3344:306::1]:2002".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5258,9 +5280,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - String::from("[fd00:1122:3344:105::1]:401"), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + "[fd00:1122:3344:105::1]:401".parse().unwrap(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5301,9 +5323,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd55:1122:3344:204::1]:1000"), - String::from("[fd55:1122:3344:205::1]:1001"), - String::from("[fd55:1122:3344:206::1]:1002"), + "[fd55:1122:3344:204::1]:1000".parse().unwrap(), + "[fd55:1122:3344:205::1]:1001".parse().unwrap(), + "[fd55:1122:3344:206::1]:1002".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5324,13 +5346,13 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from( - "[fd33:1122:3344:304::1]:2000" - ), - String::from( - "[fd33:1122:3344:305::1]:2001" - ), - new_target.0.to_string(), + "[fd33:1122:3344:304::1]:2000" + .parse() + .unwrap(), + "[fd33:1122:3344:305::1]:2001" + .parse() + .unwrap(), + new_target.0.into(), ], lossy: false, flush_timeout: None, @@ -5353,9 +5375,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd00:1122:3344:104::1]:400"), - String::from("[fd00:1122:3344:105::1]:401"), - String::from("[fd00:1122:3344:106::1]:402"), + "[fd00:1122:3344:104::1]:400".parse().unwrap(), + "[fd00:1122:3344:105::1]:401".parse().unwrap(), + "[fd00:1122:3344:106::1]:402".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5383,9 +5405,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd33:1122:3344:304::1]:2000"), - String::from("[fd33:1122:3344:305::1]:2001"), - String::from("[fd33:1122:3344:306::1]:2002"), + "[fd33:1122:3344:304::1]:2000".parse().unwrap(), + "[fd33:1122:3344:305::1]:2001".parse().unwrap(), + "[fd33:1122:3344:306::1]:2002".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5412,9 +5434,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd55:1122:3344:204::1]:1000"), - String::from("[fd55:1122:3344:205::1]:1001"), - String::from("[fd55:1122:3344:206::1]:1002"), + "[fd55:1122:3344:204::1]:1000".parse().unwrap(), + "[fd55:1122:3344:205::1]:1001".parse().unwrap(), + "[fd55:1122:3344:206::1]:1002".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5450,9 +5472,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - new_target.0.to_string(), - String::from("[fd33:1122:3344:305::1]:2001"), - String::from("[fd33:1122:3344:306::1]:2002"), + new_target.0.into(), + "[fd33:1122:3344:305::1]:2001".parse().unwrap(), + "[fd33:1122:3344:306::1]:2002".parse().unwrap(), ], lossy: false, flush_timeout: None, @@ -5481,9 +5503,9 @@ mod tests { opts: CrucibleOpts { id: volume_id, target: vec![ - String::from("[fd55:1122:3344:204::1]:1000"), - String::from("[fd55:1122:3344:205::1]:1001"), - String::from("[fd55:1122:3344:206::1]:1002"), + "[fd55:1122:3344:204::1]:1000".parse().unwrap(), + "[fd55:1122:3344:205::1]:1001".parse().unwrap(), + "[fd55:1122:3344:206::1]:1002".parse().unwrap(), ], lossy: false, flush_timeout: None, diff --git a/nexus/db-queries/src/db/datastore/volume_repair.rs b/nexus/db-queries/src/db/datastore/volume_repair.rs index 598d9d77a2..976c7f756a 100644 --- a/nexus/db-queries/src/db/datastore/volume_repair.rs +++ b/nexus/db-queries/src/db/datastore/volume_repair.rs @@ -169,7 +169,7 @@ mod test { use crate::db::pub_test_utils::TestDatabase; use omicron_test_utils::dev; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; #[tokio::test] async fn volume_lock_conflict_error_returned() { diff --git a/nexus/src/app/background/tasks/region_replacement.rs b/nexus/src/app/background/tasks/region_replacement.rs index caa262bc8c..acadf3dee3 100644 --- a/nexus/src/app/background/tasks/region_replacement.rs +++ b/nexus/src/app/background/tasks/region_replacement.rs @@ -306,8 +306,8 @@ mod test { use nexus_db_model::RegionReplacement; use nexus_db_model::Volume; use nexus_test_utils_macros::nexus_test; - use sled_agent_client::types::CrucibleOpts; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::CrucibleOpts; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/background/tasks/region_replacement_driver.rs b/nexus/src/app/background/tasks/region_replacement_driver.rs index 6cc28f9dfd..c08cd2edfc 100644 --- a/nexus/src/app/background/tasks/region_replacement_driver.rs +++ b/nexus/src/app/background/tasks/region_replacement_driver.rs @@ -258,7 +258,7 @@ mod test { use omicron_uuid_kinds::UpstairsKind; use omicron_uuid_kinds::UpstairsRepairKind; use omicron_uuid_kinds::UpstairsSessionKind; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/background/tasks/region_snapshot_replacement_finish.rs b/nexus/src/app/background/tasks/region_snapshot_replacement_finish.rs index 61a84c579d..a4351e0573 100644 --- a/nexus/src/app/background/tasks/region_snapshot_replacement_finish.rs +++ b/nexus/src/app/background/tasks/region_snapshot_replacement_finish.rs @@ -198,7 +198,7 @@ mod test { use nexus_db_queries::db::datastore::region_snapshot_replacement; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/background/tasks/region_snapshot_replacement_garbage_collect.rs b/nexus/src/app/background/tasks/region_snapshot_replacement_garbage_collect.rs index 57bbf3741c..88999efeb9 100644 --- a/nexus/src/app/background/tasks/region_snapshot_replacement_garbage_collect.rs +++ b/nexus/src/app/background/tasks/region_snapshot_replacement_garbage_collect.rs @@ -155,7 +155,7 @@ mod test { use nexus_db_model::RegionSnapshotReplacementState; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/background/tasks/region_snapshot_replacement_start.rs b/nexus/src/app/background/tasks/region_snapshot_replacement_start.rs index f2b82a3943..412a437250 100644 --- a/nexus/src/app/background/tasks/region_snapshot_replacement_start.rs +++ b/nexus/src/app/background/tasks/region_snapshot_replacement_start.rs @@ -355,8 +355,8 @@ mod test { use omicron_common::api::external; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::GenericUuid; - use sled_agent_client::types::CrucibleOpts; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::CrucibleOpts; + use sled_agent_client::VolumeConstructionRequest; use std::collections::BTreeMap; use uuid::Uuid; @@ -701,10 +701,7 @@ mod test { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![ - // the region snapshot - String::from("[::1]:12345"), - ], + target: vec!["[::1]:12345".parse().unwrap()], lossy: false, flush_timeout: None, key: None, diff --git a/nexus/src/app/background/tasks/region_snapshot_replacement_step.rs b/nexus/src/app/background/tasks/region_snapshot_replacement_step.rs index f481126312..c8010dd90d 100644 --- a/nexus/src/app/background/tasks/region_snapshot_replacement_step.rs +++ b/nexus/src/app/background/tasks/region_snapshot_replacement_step.rs @@ -566,8 +566,9 @@ mod test { use nexus_db_model::Volume; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::CrucibleOpts; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::CrucibleOpts; + use sled_agent_client::VolumeConstructionRequest; + use std::net::SocketAddrV6; use uuid::Uuid; type ControlPlaneTestContext = @@ -575,7 +576,7 @@ mod test { async fn add_fake_volume_for_snapshot_addr( datastore: &DataStore, - snapshot_addr: String, + snapshot_addr: SocketAddrV6, ) -> Uuid { let new_volume_id = Uuid::new_v4(); @@ -587,7 +588,7 @@ mod test { DatasetUuid::new_v4(), Uuid::new_v4(), Uuid::new_v4(), - snapshot_addr.clone(), + snapshot_addr.to_string(), )) .await .unwrap(); @@ -604,7 +605,7 @@ mod test { gen: 0, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![snapshot_addr], + target: vec![snapshot_addr.into()], lossy: false, flush_timeout: None, key: None, @@ -656,13 +657,14 @@ mod test { let dataset_id = DatasetUuid::new_v4(); let region_id = Uuid::new_v4(); let snapshot_id = Uuid::new_v4(); - let snapshot_addr = String::from("[fd00:1122:3344::101]:9876"); + let snapshot_addr: SocketAddrV6 = + "[fd00:1122:3344::101]:9876".parse().unwrap(); let fake_region_snapshot = RegionSnapshot::new( dataset_id, region_id, snapshot_id, - snapshot_addr.clone(), + snapshot_addr.to_string(), ); datastore.region_snapshot_create(fake_region_snapshot).await.unwrap(); @@ -746,28 +748,22 @@ mod test { // Add some fake volumes that reference the region snapshot being // replaced - let new_volume_1_id = add_fake_volume_for_snapshot_addr( - &datastore, - snapshot_addr.clone(), - ) - .await; - let new_volume_2_id = add_fake_volume_for_snapshot_addr( - &datastore, - snapshot_addr.clone(), - ) - .await; + let new_volume_1_id = + add_fake_volume_for_snapshot_addr(&datastore, snapshot_addr).await; + let new_volume_2_id = + add_fake_volume_for_snapshot_addr(&datastore, snapshot_addr).await; // Add some fake volumes that do not let other_volume_1_id = add_fake_volume_for_snapshot_addr( &datastore, - String::from("[fd00:1122:3344::101]:1000"), + "[fd00:1122:3344::101]:1000".parse().unwrap(), ) .await; let other_volume_2_id = add_fake_volume_for_snapshot_addr( &datastore, - String::from("[fd12:5544:3344::912]:3901"), + "[fd12:5544:3344::912]:3901".parse().unwrap(), ) .await; diff --git a/nexus/src/app/image.rs b/nexus/src/app/image.rs index a3fa722d36..d46da647d5 100644 --- a/nexus/src/app/image.rs +++ b/nexus/src/app/image.rs @@ -155,7 +155,7 @@ impl super::Nexus { let image_id = Uuid::new_v4(); let volume_construction_request = - sled_agent_client::types::VolumeConstructionRequest::File { + sled_agent_client::VolumeConstructionRequest::File { id: image_id, block_size, path: "/opt/oxide/propolis-server/blob/alpine.iso" diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index e916974332..109c0d8521 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -63,8 +63,7 @@ use propolis_client::support::WebSocketStream; use sagas::instance_common::ExternalIpAttach; use sagas::instance_start; use sagas::instance_update; -use sled_agent_client::types::BootOrderEntry; -use sled_agent_client::types::BootSettings; +use sled_agent_client::types::InstanceBootSettings; use sled_agent_client::types::InstanceMigrationTargetParams; use sled_agent_client::types::InstanceProperties; use sled_agent_client::types::VmmPutStateBody; @@ -1053,8 +1052,6 @@ impl super::Nexus { ) .await?; - let mut boot_disk_name = None; - let mut disk_reqs = vec![]; for disk in &disks { // Disks that are attached to an instance should always have a slot @@ -1089,58 +1086,22 @@ impl super::Nexus { ) .await?; - // Propolis wants the name of the boot disk rather than ID, because we send names - // rather than IDs in the disk requsts as assembled below. - if db_instance.boot_disk_id == Some(disk.id()) { - boot_disk_name = Some(disk.name().to_string()); - } - - disk_reqs.push(sled_agent_client::types::DiskRequest { + disk_reqs.push(sled_agent_client::types::InstanceDisk { + disk_id: disk.id(), name: disk.name().to_string(), - slot: sled_agent_client::types::Slot(slot.0), + slot: slot.0, read_only: false, - device: "nvme".to_string(), - volume_construction_request: serde_json::from_str( - &volume.data(), - ) - .map_err(Error::from)?, + vcr_json: volume.data().to_owned(), }); } - let boot_settings = if let Some(boot_disk_name) = boot_disk_name { - Some(BootSettings { - order: vec![BootOrderEntry { name: boot_disk_name }], - }) - } else { - if let Some(instance_boot_disk_id) = - db_instance.boot_disk_id.as_ref() - { - // This should never occur: when setting the boot disk we ensure it is - // attached, and when detaching a disk we ensure it is not the boot - // disk. If this error is seen, the instance somehow had a boot disk - // that was not attached anyway. - // - // When Propolis accepts an ID rather than name, and we don't need to - // look up a name when assembling the Propolis request, we might as well - // remove this check; we can just pass the ID and rely on Propolis' own - // check that the boot disk is attached. - if boot_disk_name.is_none() { - error!(self.log, "instance boot disk is not attached"; - "boot_disk_id" => ?instance_boot_disk_id, - "instance id" => %db_instance.id()); - - return Err(InstanceStateChangeError::Other( - Error::internal_error(&format!( - "instance {} has boot disk {:?} but it is not attached", - db_instance.id(), - db_instance.boot_disk_id.as_ref(), - )), - )); - } - } - - None - }; + // 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 diff --git a/nexus/src/app/sagas/disk_create.rs b/nexus/src/app/sagas/disk_create.rs index 51f6d29de1..5dcb3a0616 100644 --- a/nexus/src/app/sagas/disk_create.rs +++ b/nexus/src/app/sagas/disk_create.rs @@ -20,10 +20,10 @@ use omicron_common::api::external::Error; use rand::{rngs::StdRng, RngCore, SeedableRng}; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::{CrucibleOpts, VolumeConstructionRequest}; -use std::collections::VecDeque; +use sled_agent_client::{CrucibleOpts, VolumeConstructionRequest}; use std::convert::TryFrom; use std::net::SocketAddrV6; +use std::{collections::VecDeque, net::SocketAddr}; use steno::ActionError; use steno::Node; use uuid::Uuid; @@ -506,7 +506,7 @@ async fn sdc_regions_ensure( )), ) }) - .map(|addr| addr.to_string()) + .map(SocketAddr::V6) }) .collect::, ActionError>>()?, diff --git a/nexus/src/app/sagas/instance_migrate.rs b/nexus/src/app/sagas/instance_migrate.rs index 2ddbe5b05c..644f0c42e3 100644 --- a/nexus/src/app/sagas/instance_migrate.rs +++ b/nexus/src/app/sagas/instance_migrate.rs @@ -539,7 +539,6 @@ async fn sim_instance_migrate( InstanceStateChangeRequest::Migrate( InstanceMigrationTargetParams { src_propolis_addr: src_vmm_addr.to_string(), - src_propolis_id, }, ), ) diff --git a/nexus/src/app/sagas/region_replacement_drive.rs b/nexus/src/app/sagas/region_replacement_drive.rs index 81f9a9a7eb..30cb76f7fb 100644 --- a/nexus/src/app/sagas/region_replacement_drive.rs +++ b/nexus/src/app/sagas/region_replacement_drive.rs @@ -148,7 +148,6 @@ use crate::app::{authn, authz, db}; use chrono::DateTime; use chrono::Utc; use nexus_db_model::VmmState; -use nexus_types::identity::Resource; use omicron_common::api::external::Error; use propolis_client::types::ReplaceResult; use serde::Deserialize; @@ -1502,7 +1501,6 @@ async fn execute_propolis_drive_action( .instance_issue_crucible_vcr_request() .id(disk.id()) .body(propolis_client::types::InstanceVcrReplace { - name: disk.name().to_string(), vcr_json: disk_new_volume_vcr, }) .send() diff --git a/nexus/src/app/sagas/region_replacement_finish.rs b/nexus/src/app/sagas/region_replacement_finish.rs index 2212e6fdf3..fcbf40db84 100644 --- a/nexus/src/app/sagas/region_replacement_finish.rs +++ b/nexus/src/app/sagas/region_replacement_finish.rs @@ -219,8 +219,8 @@ pub(crate) mod test { use nexus_db_queries::context::OpContext; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::CrucibleOpts; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::CrucibleOpts; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/sagas/region_replacement_start.rs b/nexus/src/app/sagas/region_replacement_start.rs index aa9e83c037..bbd68d3aac 100644 --- a/nexus/src/app/sagas/region_replacement_start.rs +++ b/nexus/src/app/sagas/region_replacement_start.rs @@ -58,8 +58,8 @@ use nexus_db_queries::db::datastore::REGION_REDUNDANCY_THRESHOLD; use omicron_common::api::external::Error; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::CrucibleOpts; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::CrucibleOpts; +use sled_agent_client::VolumeConstructionRequest; use std::net::SocketAddrV6; use steno::ActionError; use steno::Node; @@ -688,7 +688,7 @@ async fn srrs_create_fake_volume( gen: 0, opts: CrucibleOpts { id: new_volume_id, - target: vec![old_region_address.to_string()], + target: vec![old_region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -793,7 +793,7 @@ pub(crate) mod test { use nexus_types::identity::Asset; use omicron_common::api::internal::shared::DatasetKind; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs b/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs index 675b2b0cb3..89c3ec2364 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs @@ -219,8 +219,8 @@ pub(crate) mod test { use nexus_db_queries::context::OpContext; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; - use sled_agent_client::types::CrucibleOpts; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::CrucibleOpts; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/sagas/region_snapshot_replacement_start.rs b/nexus/src/app/sagas/region_snapshot_replacement_start.rs index b9ed75c288..4919919c99 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_start.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_start.rs @@ -68,8 +68,9 @@ use omicron_common::api::external::Error; use omicron_uuid_kinds::DatasetUuid; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::CrucibleOpts; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::CrucibleOpts; +use sled_agent_client::VolumeConstructionRequest; +use std::net::SocketAddr; use std::net::SocketAddrV6; use steno::ActionError; use steno::Node; @@ -731,12 +732,12 @@ async fn rsrss_new_region_volume_create( ))); }; - let new_region_address = SocketAddrV6::new( + let new_region_address = SocketAddr::V6(SocketAddrV6::new( *new_dataset_address.ip(), ensured_region.port_number, 0, 0, - ); + )); // Create a volume to inflate the reference count of the newly created // read-only region. If this is not done it's possible that a user could @@ -753,7 +754,7 @@ async fn rsrss_new_region_volume_create( gen: 0, opts: CrucibleOpts { id: new_region_volume_id, - target: vec![new_region_address.to_string()], + target: vec![new_region_address], lossy: false, flush_timeout: None, key: None, @@ -1149,7 +1150,7 @@ pub(crate) mod test { use nexus_types::external_api::views; use nexus_types::identity::Asset; use omicron_uuid_kinds::GenericUuid; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::VolumeConstructionRequest; type ControlPlaneTestContext = nexus_test_utils::ControlPlaneTestContext; diff --git a/nexus/src/app/sagas/region_snapshot_replacement_step.rs b/nexus/src/app/sagas/region_snapshot_replacement_step.rs index a236fcf62c..fd34e80712 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_step.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_step.rs @@ -56,13 +56,12 @@ use crate::app::db::lookup::LookupPath; use crate::app::sagas::declare_saga_actions; use crate::app::{authn, authz, db}; use nexus_db_model::VmmState; -use nexus_types::identity::Resource; use omicron_common::api::external::Error; use propolis_client::types::ReplaceResult; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::CrucibleOpts; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::CrucibleOpts; +use sled_agent_client::VolumeConstructionRequest; use std::net::SocketAddrV6; use steno::ActionError; use steno::Node; @@ -555,11 +554,13 @@ async fn rsrss_notify_upstairs( "vmm id" => ?vmm.id, ); + // N.B. The ID passed to this request must match the disk backend ID that + // sled agent supplies to Propolis when it creates the instance. Currently, + // sled agent uses the disk ID as the backend ID. let result = client .instance_issue_crucible_vcr_request() .id(disk.id()) .body(propolis_client::types::InstanceVcrReplace { - name: disk.name().to_string(), vcr_json: new_volume_vcr, }) .send() diff --git a/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs b/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs index 15c6a39651..f67a1ee31f 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs @@ -131,8 +131,8 @@ pub(crate) mod test { use nexus_db_queries::context::OpContext; use nexus_db_queries::db::datastore::region_snapshot_replacement; use nexus_test_utils_macros::nexus_test; - use sled_agent_client::types::CrucibleOpts; - use sled_agent_client::types::VolumeConstructionRequest; + use sled_agent_client::CrucibleOpts; + use sled_agent_client::VolumeConstructionRequest; use uuid::Uuid; type ControlPlaneTestContext = diff --git a/nexus/src/app/sagas/snapshot_create.rs b/nexus/src/app/sagas/snapshot_create.rs index 4dd958c50b..cceec7d70e 100644 --- a/nexus/src/app/sagas/snapshot_create.rs +++ b/nexus/src/app/sagas/snapshot_create.rs @@ -112,18 +112,21 @@ use omicron_uuid_kinds::{GenericUuid, PropolisUuid, SledUuid}; use rand::{rngs::StdRng, RngCore, SeedableRng}; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::CrucibleOpts; use sled_agent_client::types::VmmIssueDiskSnapshotRequestBody; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::CrucibleOpts; +use sled_agent_client::VolumeConstructionRequest; use slog::info; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; use std::collections::VecDeque; +use std::net::SocketAddr; use std::net::SocketAddrV6; use steno::ActionError; use steno::Node; use uuid::Uuid; +type ReplaceSocketsMap = BTreeMap; + // snapshot create saga: input parameters #[derive(Debug, Deserialize, Serialize)] @@ -539,7 +542,7 @@ async fn ssc_regions_ensure( )), ) }) - .map(|addr| addr.to_string()) + .map(SocketAddr::V6) }) .collect::, ActionError>>()?, @@ -1383,7 +1386,7 @@ async fn ssc_detach_disk_from_pantry( async fn ssc_start_running_snapshot( sagactx: NexusActionContext, -) -> Result, ActionError> { +) -> Result { let log = sagactx.user_data().log(); let osagactx = sagactx.user_data(); let params = sagactx.saga_params::()?; @@ -1409,7 +1412,7 @@ async fn ssc_start_running_snapshot( .await .map_err(ActionError::action_failed)?; - let mut map: BTreeMap = BTreeMap::new(); + let mut map: ReplaceSocketsMap = BTreeMap::new(); for (dataset, region) in datasets_and_regions { let Some(dataset_addr) = dataset.address() else { @@ -1438,28 +1441,22 @@ async fn ssc_start_running_snapshot( ); // Map from the region to the snapshot - let region_addr = format!( - "{}", - SocketAddrV6::new( - *dataset_addr.ip(), - crucible_region.port_number, - 0, - 0 - ) + let region_addr = SocketAddrV6::new( + *dataset_addr.ip(), + crucible_region.port_number, + 0, + 0, ); - let snapshot_addr = format!( - "{}", - SocketAddrV6::new( - *dataset_addr.ip(), - crucible_running_snapshot.port_number, - 0, - 0 - ) + let snapshot_addr = SocketAddrV6::new( + *dataset_addr.ip(), + crucible_running_snapshot.port_number, + 0, + 0, ); info!(log, "map {} to {}", region_addr, snapshot_addr); - map.insert(region_addr, snapshot_addr.clone()); + map.insert(region_addr, snapshot_addr); // Once snapshot has been validated, and running snapshot has been // started, add an entry in the region_snapshot table to correspond to @@ -1470,7 +1467,7 @@ async fn ssc_start_running_snapshot( dataset_id: dataset.id().into(), region_id: region.id(), snapshot_id, - snapshot_addr, + snapshot_addr: snapshot_addr.to_string(), volume_references: 0, // to be filled later deleting: false, }) @@ -1570,7 +1567,7 @@ async fn ssc_create_volume_record( // read-only crucible agent downstairs (corresponding to this snapshot) // launched through this saga. let replace_sockets_map = - sagactx.lookup::>("replace_sockets_map")?; + sagactx.lookup::("replace_sockets_map")?; let snapshot_volume_construction_request: VolumeConstructionRequest = create_snapshot_from_disk( &disk_volume_construction_request, @@ -1694,7 +1691,7 @@ async fn ssc_release_volume_lock( /// VolumeConstructionRequest and modifying it accordingly. fn create_snapshot_from_disk( disk: &VolumeConstructionRequest, - socket_map: &BTreeMap, + socket_map: &ReplaceSocketsMap, ) -> anyhow::Result { // When copying a disk's VolumeConstructionRequest to turn it into a // snapshot: @@ -1756,9 +1753,19 @@ fn create_snapshot_from_disk( if work.socket_modification_required { for target in &mut opts.target { - target.clone_from(socket_map.get(target).ok_or_else( - || anyhow!("target {} not found in map!", target), - )?); + let target = match target { + SocketAddr::V6(v6) => v6, + SocketAddr::V4(_) => { + anyhow::bail!( + "unexpected IPv4 address in VCR: {:?}", + work.vcr_part + ) + } + }; + + *target = *socket_map.get(target).ok_or_else(|| { + anyhow!("target {} not found in map!", target) + })?; } } } @@ -1799,7 +1806,7 @@ mod test { use omicron_common::api::external::InstanceCpuCount; use omicron_common::api::external::Name; use omicron_common::api::external::NameOrId; - use sled_agent_client::types::CrucibleOpts; + use sled_agent_client::CrucibleOpts; use sled_agent_client::TestInterfaces as SledAgentTestInterfaces; use std::str::FromStr; @@ -1834,9 +1841,9 @@ mod test { lossy: false, read_only: true, target: vec![ - "[fd00:1122:3344:101::8]:19001".into(), - "[fd00:1122:3344:101::7]:19001".into(), - "[fd00:1122:3344:101::6]:19001".into(), + "[fd00:1122:3344:101::8]:19001".parse().unwrap(), + "[fd00:1122:3344:101::7]:19001".parse().unwrap(), + "[fd00:1122:3344:101::6]:19001".parse().unwrap(), ], cert_pem: None, key_pem: None, @@ -1860,34 +1867,34 @@ mod test { lossy: false, read_only: false, target: vec![ - "[fd00:1122:3344:101::8]:19002".into(), - "[fd00:1122:3344:101::7]:19002".into(), - "[fd00:1122:3344:101::6]:19002".into(), + "[fd00:1122:3344:101::8]:19002".parse().unwrap(), + "[fd00:1122:3344:101::7]:19002".parse().unwrap(), + "[fd00:1122:3344:101::6]:19002".parse().unwrap(), ], cert_pem: None, key_pem: None, root_cert_pem: None, flush_timeout: None, - control: Some("127.0.0.1:12345".into()), + control: Some("127.0.0.1:12345".parse().unwrap()), } }, ], }; - let mut replace_sockets: BTreeMap = BTreeMap::new(); + let mut replace_sockets = ReplaceSocketsMap::new(); // Replacements for top level Region only replace_sockets.insert( - "[fd00:1122:3344:101::6]:19002".into(), - "[XXXX:1122:3344:101::6]:9000".into(), + "[fd00:1122:3344:101::6]:19002".parse().unwrap(), + "[fd01:1122:3344:101::6]:9000".parse().unwrap(), ); replace_sockets.insert( - "[fd00:1122:3344:101::7]:19002".into(), - "[XXXX:1122:3344:101::7]:9000".into(), + "[fd00:1122:3344:101::7]:19002".parse().unwrap(), + "[fd01:1122:3344:101::7]:9000".parse().unwrap(), ); replace_sockets.insert( - "[fd00:1122:3344:101::8]:19002".into(), - "[XXXX:1122:3344:101::8]:9000".into(), + "[fd00:1122:3344:101::8]:19002".parse().unwrap(), + "[fd01:1122:3344:101::8]:9000".parse().unwrap(), ); let snapshot = diff --git a/nexus/src/app/sagas/volume_delete.rs b/nexus/src/app/sagas/volume_delete.rs index a8ded4e33c..89d8306265 100644 --- a/nexus/src/app/sagas/volume_delete.rs +++ b/nexus/src/app/sagas/volume_delete.rs @@ -336,7 +336,7 @@ async fn svd_delete_crucible_snapshot_records( /// It's insufficient to rely on the struct of CrucibleResources to clean up /// that is returned as part of svd_decrease_crucible_resource_count. Imagine a /// disk that is composed of three regions (a subset of -/// [`sled_agent_client::types::VolumeConstructionRequest`] is shown here): +/// [`sled_agent_client::VolumeConstructionRequest`] is shown here): /// /// ```json /// { diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index b614495615..0f81356365 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -10,7 +10,7 @@ use nexus_db_queries::db; use omicron_common::api::external::Error; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::VolumeConstructionRequest; +use sled_agent_client::VolumeConstructionRequest; use steno::{ActionError, Node}; use uuid::Uuid; diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index f0eb294e58..b059aae12b 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -55,6 +55,7 @@ use omicron_common::api::external::Name; use omicron_common::api::internal; use omicron_test_utils::dev::poll::wait_for_condition; use omicron_test_utils::dev::poll::CondCheckError; +use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::DownstairsKind; use omicron_uuid_kinds::DownstairsRegionKind; use omicron_uuid_kinds::GenericUuid; @@ -64,9 +65,9 @@ use omicron_uuid_kinds::UpstairsRepairKind; use omicron_uuid_kinds::UpstairsSessionKind; use rand::prelude::SliceRandom; use rand::{rngs::StdRng, SeedableRng}; -use sled_agent_client::types::{CrucibleOpts, VolumeConstructionRequest}; +use sled_agent_client::{CrucibleOpts, VolumeConstructionRequest}; use std::collections::HashSet; -use std::net::SocketAddrV6; +use std::net::{SocketAddr, SocketAddrV6}; use std::sync::Arc; use uuid::Uuid; @@ -2206,44 +2207,44 @@ async fn test_keep_your_targets_straight(cptestctx: &ControlPlaneTestContext) { // insert those here manually. // (dataset_id, region_id, snapshot_id, snapshot_addr) - let region_snapshots = vec![ + let region_snapshots: Vec<(DatasetUuid, Uuid, Uuid, SocketAddr)> = vec![ // first snapshot-create ( zpool0.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:101::7]:19016"), + "[fd00:1122:3344:101::7]:19016".parse().unwrap(), ), ( zpool1.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:102::7]:19016"), + "[fd00:1122:3344:102::7]:19016".parse().unwrap(), ), ( zpool2.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:103::7]:19016"), + "[fd00:1122:3344:103::7]:19016".parse().unwrap(), ), // second snapshot-create ( zpool0.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:101::7]:19016"), // duplicate! + "[fd00:1122:3344:101::7]:19016".parse().unwrap(), ), ( zpool3.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:104::7]:19016"), + "[fd00:1122:3344:104::7]:19016".parse().unwrap(), ), ( zpool2.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:103::7]:19017"), + "[fd00:1122:3344:103::7]:19017".parse().unwrap(), ), ]; @@ -2258,7 +2259,7 @@ async fn test_keep_your_targets_straight(cptestctx: &ControlPlaneTestContext) { dataset_id: (*dataset_id).into(), region_id: *region_id, snapshot_id: *snapshot_id, - snapshot_addr: snapshot_addr.clone(), + snapshot_addr: snapshot_addr.to_string(), volume_references: 0, deleting: false, }) @@ -2283,9 +2284,9 @@ async fn test_keep_your_targets_straight(cptestctx: &ControlPlaneTestContext) { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - region_snapshots[0].3.clone(), - region_snapshots[1].3.clone(), - region_snapshots[2].3.clone(), + region_snapshots[0].3, + region_snapshots[1].3, + region_snapshots[2].3, ], lossy: false, flush_timeout: None, @@ -2379,7 +2380,7 @@ async fn test_keep_your_targets_straight(cptestctx: &ControlPlaneTestContext) { dataset_id: (*dataset_id).into(), region_id: *region_id, snapshot_id: *snapshot_id, - snapshot_addr: snapshot_addr.clone(), + snapshot_addr: snapshot_addr.to_string(), volume_references: 0, deleting: false, }) @@ -2404,9 +2405,9 @@ async fn test_keep_your_targets_straight(cptestctx: &ControlPlaneTestContext) { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - region_snapshots[3].3.clone(), - region_snapshots[4].3.clone(), - region_snapshots[5].3.clone(), + region_snapshots[3].3, + region_snapshots[4].3, + region_snapshots[5].3, ], lossy: false, flush_timeout: None, @@ -3659,7 +3660,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address.to_string()], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -3789,7 +3790,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address.to_string()], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -3822,7 +3823,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address.to_string()], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -3857,7 +3858,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address.to_string()], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -5377,30 +5378,30 @@ async fn test_migrate_to_ref_count_with_records_region_snapshot_deleting( let zpool3 = iter.next().expect("Expected four zpools"); // (dataset_id, region_id, snapshot_id, snapshot_addr) - let region_snapshots = vec![ + let region_snapshots: Vec<(DatasetUuid, Uuid, Uuid, SocketAddr)> = vec![ ( zpool0.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:101::7]:19016"), + "[fd00:1122:3344:101::7]:19016".parse().unwrap(), ), ( zpool1.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:102::7]:19016"), + "[fd00:1122:3344:102::7]:19016".parse().unwrap(), ), ( zpool2.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:103::7]:19016"), + "[fd00:1122:3344:103::7]:19016".parse().unwrap(), ), ( zpool3.datasets[0].id, Uuid::new_v4(), Uuid::new_v4(), - String::from("[fd00:1122:3344:104::7]:19016"), + "[fd00:1122:3344:104::7]:19016".parse().unwrap(), ), ]; @@ -5413,7 +5414,7 @@ async fn test_migrate_to_ref_count_with_records_region_snapshot_deleting( dataset_id: to_db_typed_uuid(*dataset_id), region_id: *region_id, snapshot_id: *snapshot_id, - snapshot_addr: snapshot_addr.clone(), + snapshot_addr: snapshot_addr.to_string(), volume_references: 0, deleting: false, }) @@ -5441,9 +5442,9 @@ async fn test_migrate_to_ref_count_with_records_region_snapshot_deleting( opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - region_snapshots[0].3.clone(), - region_snapshots[1].3.clone(), - region_snapshots[2].3.clone(), + region_snapshots[0].3, + region_snapshots[1].3, + region_snapshots[2].3, ], lossy: false, flush_timeout: None, @@ -5479,9 +5480,9 @@ async fn test_migrate_to_ref_count_with_records_region_snapshot_deleting( opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - region_snapshots[1].3.clone(), - region_snapshots[2].3.clone(), - region_snapshots[3].3.clone(), + region_snapshots[1].3, + region_snapshots[2].3, + region_snapshots[3].3, ], lossy: false, flush_timeout: None, @@ -5519,7 +5520,10 @@ async fn test_migrate_to_ref_count_with_records_region_snapshot_deleting( ); assert_eq!(region_snapshot_to_delete.region_id, region_snapshots[0].1); assert_eq!(region_snapshot_to_delete.snapshot_id, region_snapshots[0].2); - assert_eq!(region_snapshot_to_delete.snapshot_addr, region_snapshots[0].3); + assert_eq!( + region_snapshot_to_delete.snapshot_addr.parse::().unwrap(), + region_snapshots[0].3 + ); assert_eq!(region_snapshot_to_delete.volume_references, 0); assert_eq!(region_snapshot_to_delete.deleting, true); @@ -6050,9 +6054,9 @@ async fn test_no_zombie_read_only_regions(cptestctx: &ControlPlaneTestContext) { opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - region_addrs[0].to_string(), - region_addrs[1].to_string(), - region_addrs[2].to_string(), + region_addrs[0].into(), + region_addrs[1].into(), + region_addrs[2].into(), ], lossy: false, flush_timeout: None, @@ -6236,9 +6240,9 @@ async fn test_no_zombie_read_write_regions( opts: CrucibleOpts { id: Uuid::new_v4(), target: vec![ - region_addrs[0].to_string(), - region_addrs[1].to_string(), - region_addrs[2].to_string(), + region_addrs[0].into(), + region_addrs[1].into(), + region_addrs[2].into(), ], lossy: false, flush_timeout: None, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index c7d2b36de4..81d9211104 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2570,36 +2570,6 @@ } ] }, - "BootOrderEntry": { - "description": "An entry in the boot order stored in a [`BootSettings`] component.\n\n
JSON schema\n\n```json { \"description\": \"An entry in the boot order stored in a [`BootSettings`] component.\", \"type\": \"object\", \"required\": [ \"name\" ], \"properties\": { \"name\": { \"description\": \"The name of another component in the spec that Propolis should try to boot from.\\n\\nCurrently, only disk device components are supported.\", \"type\": \"string\" } } } ```
", - "type": "object", - "properties": { - "name": { - "description": "The name of another component in the spec that Propolis should try to boot from.\n\nCurrently, only disk device components are supported.", - "type": "string" - } - }, - "required": [ - "name" - ] - }, - "BootSettings": { - "description": "Settings supplied to the guest's firmware image that specify the order in which it should consider its options when selecting a device to try to boot from.\n\n
JSON schema\n\n```json { \"description\": \"Settings supplied to the guest's firmware image that specify the order in which it should consider its options when selecting a device to try to boot from.\", \"type\": \"object\", \"required\": [ \"order\" ], \"properties\": { \"order\": { \"description\": \"An ordered list of components to attempt to boot from.\", \"type\": \"array\", \"items\": { \"$ref\": \"#/components/schemas/BootOrderEntry\" } } }, \"additionalProperties\": false } ```
", - "type": "object", - "properties": { - "order": { - "description": "An ordered list of components to attempt to boot from.", - "type": "array", - "items": { - "$ref": "#/components/schemas/BootOrderEntry" - } - } - }, - "required": [ - "order" - ], - "additionalProperties": false - }, "BootstoreStatus": { "type": "object", "properties": { @@ -2891,59 +2861,6 @@ } ] }, - "CrucibleOpts": { - "description": "CrucibleOpts\n\n
JSON schema\n\n```json { \"type\": \"object\", \"required\": [ \"id\", \"lossy\", \"read_only\", \"target\" ], \"properties\": { \"cert_pem\": { \"type\": [ \"string\", \"null\" ] }, \"control\": { \"type\": [ \"string\", \"null\" ] }, \"flush_timeout\": { \"type\": [ \"number\", \"null\" ], \"format\": \"float\" }, \"id\": { \"type\": \"string\", \"format\": \"uuid\" }, \"key\": { \"type\": [ \"string\", \"null\" ] }, \"key_pem\": { \"type\": [ \"string\", \"null\" ] }, \"lossy\": { \"type\": \"boolean\" }, \"read_only\": { \"type\": \"boolean\" }, \"root_cert_pem\": { \"type\": [ \"string\", \"null\" ] }, \"target\": { \"type\": \"array\", \"items\": { \"type\": \"string\" } } } } ```
", - "type": "object", - "properties": { - "cert_pem": { - "nullable": true, - "type": "string" - }, - "control": { - "nullable": true, - "type": "string" - }, - "flush_timeout": { - "nullable": true, - "type": "number", - "format": "float" - }, - "id": { - "type": "string", - "format": "uuid" - }, - "key": { - "nullable": true, - "type": "string" - }, - "key_pem": { - "nullable": true, - "type": "string" - }, - "lossy": { - "type": "boolean" - }, - "read_only": { - "type": "boolean" - }, - "root_cert_pem": { - "nullable": true, - "type": "string" - }, - "target": { - "type": "array", - "items": { - "type": "string" - } - } - }, - "required": [ - "id", - "lossy", - "read_only", - "target" - ] - }, "DatasetConfig": { "description": "Configuration information necessary to request a single dataset.\n\nThese datasets are tracked directly by Nexus.", "type": "object", @@ -3250,34 +3167,6 @@ "identity" ] }, - "DiskRequest": { - "description": "DiskRequest\n\n
JSON schema\n\n```json { \"type\": \"object\", \"required\": [ \"device\", \"name\", \"read_only\", \"slot\", \"volume_construction_request\" ], \"properties\": { \"device\": { \"type\": \"string\" }, \"name\": { \"type\": \"string\" }, \"read_only\": { \"type\": \"boolean\" }, \"slot\": { \"$ref\": \"#/components/schemas/Slot\" }, \"volume_construction_request\": { \"$ref\": \"#/components/schemas/VolumeConstructionRequest\" } } } ```
", - "type": "object", - "properties": { - "device": { - "type": "string" - }, - "name": { - "type": "string" - }, - "read_only": { - "type": "boolean" - }, - "slot": { - "$ref": "#/components/schemas/Slot" - }, - "volume_construction_request": { - "$ref": "#/components/schemas/VolumeConstructionRequest" - } - }, - "required": [ - "device", - "name", - "read_only", - "slot", - "volume_construction_request" - ] - }, "DiskRuntimeState": { "description": "Runtime state of the Disk, which includes its attach state and some minimal metadata", "type": "object", @@ -3866,12 +3755,65 @@ } ] }, + "InstanceBootSettings": { + "description": "Configures how an instance is told to try to boot.", + "type": "object", + "properties": { + "order": { + "description": "Propolis should tell guest firmware to try to boot from devices in this order.", + "type": "array", + "items": { + "type": "string", + "format": "uuid" + } + } + }, + "required": [ + "order" + ] + }, "InstanceCpuCount": { "description": "The number of CPUs in an Instance", "type": "integer", "format": "uint16", "minimum": 0 }, + "InstanceDisk": { + "description": "A request to attach a disk to an instance.", + "type": "object", + "properties": { + "disk_id": { + "description": "The disk's UUID.", + "type": "string", + "format": "uuid" + }, + "name": { + "description": "The disk's name, used to generate the serial number for the virtual disk exposed to the guest.", + "type": "string" + }, + "read_only": { + "description": "True if the disk is read-only.", + "type": "boolean" + }, + "slot": { + "description": "The logical slot number assigned to the disk in its database record.", + "type": "integer", + "format": "uint8", + "minimum": 0 + }, + "vcr_json": { + "description": "A JSON representation of the Crucible volume construction request for this attachment.", + "type": "string" + } + }, + "required": [ + "disk_id", + "name", + "read_only", + "slot", + "vcr_json" + ] + }, "InstanceEnsureBody": { "description": "The body of a request to ensure that a instance and VMM are known to a sled agent.", "type": "object", @@ -3978,7 +3920,7 @@ "nullable": true, "allOf": [ { - "$ref": "#/components/schemas/BootSettings" + "$ref": "#/components/schemas/InstanceBootSettings" } ] }, @@ -3992,7 +3934,7 @@ "disks": { "type": "array", "items": { - "$ref": "#/components/schemas/DiskRequest" + "$ref": "#/components/schemas/InstanceDisk" } }, "ephemeral_ip": { @@ -4062,16 +4004,10 @@ "src_propolis_addr": { "description": "The address of the Propolis server that will serve as the migration source.", "type": "string" - }, - "src_propolis_id": { - "description": "The Propolis ID of the migration source.", - "type": "string", - "format": "uuid" } }, "required": [ - "src_propolis_addr", - "src_propolis_id" + "src_propolis_addr" ] }, "InstanceProperties": { @@ -5682,12 +5618,6 @@ "vmm_state" ] }, - "Slot": { - "description": "A stable index which is translated by Propolis into a PCI BDF, visible to the guest.\n\n
JSON schema\n\n```json { \"description\": \"A stable index which is translated by Propolis into a PCI BDF, visible to the guest.\", \"type\": \"integer\", \"format\": \"uint8\", \"minimum\": 0.0 } ```
", - "type": "integer", - "format": "uint8", - "minimum": 0 - }, "SourceNatConfig": { "description": "An IP address and port range used for source NAT, i.e., making outbound network connections from guests or services.", "type": "object", @@ -6269,151 +6199,6 @@ "format": "uint32", "minimum": 0 }, - "VolumeConstructionRequest": { - "description": "VolumeConstructionRequest\n\n
JSON schema\n\n```json { \"oneOf\": [ { \"type\": \"object\", \"required\": [ \"block_size\", \"id\", \"sub_volumes\", \"type\" ], \"properties\": { \"block_size\": { \"type\": \"integer\", \"format\": \"uint64\", \"minimum\": 0.0 }, \"id\": { \"type\": \"string\", \"format\": \"uuid\" }, \"read_only_parent\": { \"allOf\": [ { \"$ref\": \"#/components/schemas/VolumeConstructionRequest\" } ] }, \"sub_volumes\": { \"type\": \"array\", \"items\": { \"$ref\": \"#/components/schemas/VolumeConstructionRequest\" } }, \"type\": { \"type\": \"string\", \"enum\": [ \"volume\" ] } } }, { \"type\": \"object\", \"required\": [ \"block_size\", \"id\", \"type\", \"url\" ], \"properties\": { \"block_size\": { \"type\": \"integer\", \"format\": \"uint64\", \"minimum\": 0.0 }, \"id\": { \"type\": \"string\", \"format\": \"uuid\" }, \"type\": { \"type\": \"string\", \"enum\": [ \"url\" ] }, \"url\": { \"type\": \"string\" } } }, { \"type\": \"object\", \"required\": [ \"block_size\", \"blocks_per_extent\", \"extent_count\", \"gen\", \"opts\", \"type\" ], \"properties\": { \"block_size\": { \"type\": \"integer\", \"format\": \"uint64\", \"minimum\": 0.0 }, \"blocks_per_extent\": { \"type\": \"integer\", \"format\": \"uint64\", \"minimum\": 0.0 }, \"extent_count\": { \"type\": \"integer\", \"format\": \"uint32\", \"minimum\": 0.0 }, \"gen\": { \"type\": \"integer\", \"format\": \"uint64\", \"minimum\": 0.0 }, \"opts\": { \"$ref\": \"#/components/schemas/CrucibleOpts\" }, \"type\": { \"type\": \"string\", \"enum\": [ \"region\" ] } } }, { \"type\": \"object\", \"required\": [ \"block_size\", \"id\", \"path\", \"type\" ], \"properties\": { \"block_size\": { \"type\": \"integer\", \"format\": \"uint64\", \"minimum\": 0.0 }, \"id\": { \"type\": \"string\", \"format\": \"uuid\" }, \"path\": { \"type\": \"string\" }, \"type\": { \"type\": \"string\", \"enum\": [ \"file\" ] } } } ] } ```
", - "oneOf": [ - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "read_only_parent": { - "nullable": true, - "allOf": [ - { - "$ref": "#/components/schemas/VolumeConstructionRequest" - } - ] - }, - "sub_volumes": { - "type": "array", - "items": { - "$ref": "#/components/schemas/VolumeConstructionRequest" - } - }, - "type": { - "type": "string", - "enum": [ - "volume" - ] - } - }, - "required": [ - "block_size", - "id", - "sub_volumes", - "type" - ] - }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "type": { - "type": "string", - "enum": [ - "url" - ] - }, - "url": { - "type": "string" - } - }, - "required": [ - "block_size", - "id", - "type", - "url" - ] - }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "blocks_per_extent": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "extent_count": { - "type": "integer", - "format": "uint32", - "minimum": 0 - }, - "gen": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "opts": { - "$ref": "#/components/schemas/CrucibleOpts" - }, - "type": { - "type": "string", - "enum": [ - "region" - ] - } - }, - "required": [ - "block_size", - "blocks_per_extent", - "extent_count", - "gen", - "opts", - "type" - ] - }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "path": { - "type": "string" - }, - "type": { - "type": "string", - "enum": [ - "file" - ] - } - }, - "required": [ - "block_size", - "id", - "path", - "type" - ] - } - ] - }, "VpcFirewallRuleAction": { "type": "string", "enum": [ diff --git a/package-manifest.toml b/package-manifest.toml index b28ac7d59f..8140ce43af 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -624,10 +624,10 @@ service_name = "propolis-server" only_for_targets.image = "standard" source.type = "prebuilt" source.repo = "propolis" -source.commit = "220a6f367c18f2452dbc4fa9086f3fe73b961739" +source.commit = "d4529fd8247386b422b78e1203315d5baea5ea8b" # The SHA256 digest is automatically posted to: # https://buildomat.eng.oxide.computer/public/file/oxidecomputer/propolis/image//propolis-server.sha256.txt -source.sha256 = "964bf262677496118f8cea95c257d0a57c76ddca70733217b0666657b53bd6e6" +source.sha256 = "3e5995281e2b222fbfa3537fcc846e0706361db5ab57de6656811871bcc04cc3" output.type = "zone" [package.mg-ddm-gz] diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index 21832fde23..8b327ddd9a 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -443,8 +443,8 @@ struct InstanceRunner { dhcp_config: DhcpCfg, // Disk related properties - requested_disks: Vec, - boot_settings: Option, + requested_disks: Vec, + boot_settings: Option, cloud_init_bytes: Option>, // Internal State management @@ -847,84 +847,365 @@ impl InstanceRunner { res.map(|_| ()) } - /// Sends an instance ensure request to this instance's Propolis. + /// Sends an instance ensure request to this instance's Propolis, + /// constructing its configuration from the fields in `self` that describe + /// the instance's virtual hardware configuration. async fn propolis_ensure_inner( &self, client: &PropolisClient, running_zone: &RunningZone, migrate: Option, ) -> Result<(), Error> { - let nics = running_zone + // A bit of history helps to explain the workings of the rest of this + // function. + // + // In the past, the Propolis API accepted an InstanceEnsureRequest + // struct that described a VM's hardware configuration at more or less + // the level of specificity used in Nexus's database. Callers passed + // this struct irrespective of whether they were starting a brand new VM + // migrating from an existing VM in some other Propolis. It was + // Propolis's job to convert any insufficiently-specific parameters + // passed in its API into the concrete details needed to set up VM + // components (e.g., converting device slot numbers into concrete PCI + // bus/device/function numbers). + // + // The Propolis VM creation API used below differs from this scheme in + // two important ways: + // + // 1. Callers are responsible for filling in all the details of a VM's + // configuration (e.g., choosing PCI BDFs for PCI devices). + // 2. During a live migration, the migration target inherits most of its + // configuration directly from the migration source. Propolis only + // allows clients to specify new configurations for the specific set + // of components that it expects to be reconfigured over a migration. + // These are described further below. + // + // This scheme aims to + // + // 1. prevent bugs where an instance can't migrate because the control + // plane has specified conflicting configurations to the source and + // target, and + // 2. maximize the updateability of VM configurations by allowing the + // control plane, which (at least in Nexus) is relatively easy to + // update, to make the rules about how Nexus instance configurations + // are realized in Propolis VMs. + // + // See propolis#804 for even more context on this API's design. + // + // A "virtual platform" is a set of rules that describe how to realize a + // Propolis VM configuration from a control plane instance description. + // The logic below encodes the "Oxide MVP" virtual platform that + // Propolis implicitly implemented in its legacy instance-ensure API. In + // the future there will be additional virtual platforms that may encode + // different rules and configuration options. + // + // TODO(#615): Eventually this logic should move to a robust virtual + // platform library in Nexus. + + use propolis_client::{ + types::{ + BlobStorageBackend, Board, BootOrderEntry, BootSettings, + Chipset, ComponentV0, CrucibleStorageBackend, + InstanceInitializationMethod, NvmeDisk, QemuPvpanic, + ReplacementComponent, SerialPort, SerialPortNumber, VirtioDisk, + VirtioNetworkBackend, VirtioNic, + }, + PciPath, SpecKey, + }; + + // Define some local helper structs for unpacking hardware descriptions + // into the types Propolis wants to see in its specifications. + struct DiskComponents { + id: Uuid, + device: NvmeDisk, + backend: CrucibleStorageBackend, + } + + struct NicComponents { + id: Uuid, + device: VirtioNic, + backend: VirtioNetworkBackend, + } + + // Assemble the list of NVMe disks associated with this instance. + let disks: Vec = self + .requested_disks + .iter() + .map(|disk| -> Result { + // One of the details Propolis asks clients to fill in is the + // serial number for each NVMe disk. It's important that this + // number remain stable because it can be written to an + // instance's nonvolatile EFI variables, specifically its boot + // order variables, which can be undercut if a serial number + // changes. + // + // The old version of the Propolis API generated serial numbers + // by taking the control plane disk name and padding it with + // zero bytes. Match this behavior here. + // + // Note that this scheme violates version 1.2.1 of the NVMe + // specification: section 1.5 says that string fields like this + // one should be left-justified and padded with spaces, not + // zeroes. Future versions of this logic may switch to this + // behavior. + let serial_number = + propolis_client::support::nvme_serial_from_str( + &disk.name, 0, + ); + + Ok(DiskComponents { + id: disk.disk_id, + device: NvmeDisk { + backend_id: SpecKey::Uuid(disk.disk_id), + // The old Propolis API added 16 to disk slot numbers to + // get their PCI device numbers. + pci_path: PciPath::new(0, disk.slot + 0x10, 0)?, + serial_number, + }, + backend: CrucibleStorageBackend { + readonly: disk.read_only, + request_json: disk.vcr_json.0.clone(), + }, + }) + }) + .collect::, _>>()?; + + // Next, assemble the list of guest NICs. + let nics: Vec = running_zone .opte_ports() - .map(|port| { - self.requested_nics + .map(|port| -> Result { + let nic = self + .requested_nics .iter() // We expect to match NIC slots to OPTE port slots. // Error out if we can't find a NIC for a port. - .position(|nic| nic.slot == port.slot()) + .find(|nic| nic.slot == port.slot()) .ok_or(Error::Opte( illumos_utils::opte::Error::NoNicforPort( port.name().into(), port.slot().into(), ), + ))?; + + Ok(NicComponents { + id: nic.id, + device: VirtioNic { + backend_id: SpecKey::Uuid(nic.id), + interface_id: nic.id, + // The old Propolis API added 8 to NIC slot numbers to + // get their PCI device numbers. + pci_path: PciPath::new(0, nic.slot + 8, 0)?, + }, + backend: VirtioNetworkBackend { + vnic_name: port.name().to_string(), + }, + }) + }) + .collect::, _>>()?; + + // When a VM migrates, the migration target inherits most of its + // configuration directly from the migration source. The exceptions are + // cases where the target VM needs new parameters in order to interface + // with services on its sled or in the rest of the rack. These include + // + // - Crucible disks: the target needs to connect to its downstairs + // instances with new generation numbers supplied from Nexus + // - OPTE ports: the target needs to bind its VNICs to the correct + // devices for its new host; those devices may have different names + // than their counterparts on the source host + // + // If this is a request to migrate, construct a list of source component + // specifications that this caller intends to replace. Otherwise, + // construct a complete instance specification for a new VM. + let request = if let Some(params) = migrate { + // TODO(#6073): The migration ID is sent in the VMM registration + // request and isn't part of the migration target params (despite + // being known when the migration-start request is sent). If it were + // sent here it would no longer be necessary to read the ID back + // from the saved VMM/instance state. + // + // In practice, Nexus should (by the construction of the instance + // migration saga) always initialize a migration-destination VMM + // with a valid migration ID. But since that invariant is + // technically part of a different component, return an error here + // instead of unwrapping if it's violated. + let migration_id = self + .state + .migration_in() + .ok_or_else(|| { + Error::Migration(anyhow::anyhow!( + "migration requested but no migration ID was \ + supplied when this VMM was registered" )) - .map(|pos| { - let nic = &self.requested_nics[pos]; - propolis_client::types::NetworkInterfaceRequest { - interface_id: nic.id, - name: port.name().to_string(), - slot: propolis_client::types::Slot(port.slot()), - } + })? + .migration_id; + + // Add the new Crucible backends to the list of source instance spec + // elements that should be replaced in the target's spec. + let mut elements_to_replace: std::collections::HashMap<_, _> = + disks + .into_iter() + .map(|disk| { + ( + // N.B. This ID must match the one supplied when the + // instance was started. + disk.id.to_string(), + ReplacementComponent::CrucibleStorageBackend( + disk.backend, + ), + ) }) - }) - .collect::, _>>()?; - - let migrate = match migrate { - Some(params) => { - let migration_id = self.state - .migration_in() - // TODO(eliza): This is a bit of an unfortunate dance: the - // initial instance-ensure-registered request is what sends - // the migration ID, but it's the subsequent - // instance-ensure-state request (which we're handling here) - // that includes migration the source VMM's UUID and IP - // address. Because the API currently splits the migration - // IDs between the instance-ensure-registered and - // instance-ensure-state requests, we have to stash the - // migration ID in an `Option` and `expect()` it here, - // panicking if we get an instance-ensure-state request with - // a source Propolis ID if the instance wasn't registered - // with a migration in ID. - // - // This is kind of a shame. Eventually, we should consider - // reworking the API ensure-state request contains the - // migration ID, and we don't have to unwrap here. See: - // https://github.com/oxidecomputer/omicron/issues/6073 - .expect("if we have migration target params, we should also have a migration in") - .migration_id; - Some(propolis_client::types::InstanceMigrateInitiateRequest { - src_addr: params.src_propolis_addr.to_string(), - src_uuid: params.src_propolis_id, + .collect(); + + // Add new OPTE backend configuration to the replacement list. + elements_to_replace.extend(nics.into_iter().map(|nic| { + ( + // N.B. This ID must match the one supplied when the + // instance was started. + nic.id.to_string(), + ReplacementComponent::VirtioNetworkBackend(nic.backend), + ) + })); + + propolis_client::types::InstanceEnsureRequest { + properties: self.properties.clone(), + init: InstanceInitializationMethod::MigrationTarget { migration_id, - }) + replace_components: elements_to_replace, + src_addr: params.src_propolis_addr.to_string(), + }, } - None => None, - }; + } else { + // This is not a request to migrate, so construct a brand new spec + // to send to Propolis. + // + // Spec components must all have unique names. This routine ensures + // that names are unique as follows: + // + // 1. Backend components corresponding to specific control plane + // objects (e.g. Crucible disks, network interfaces) are + // identified by their control plane record IDs, which are UUIDs. + // (If these UUIDs collide, Nexus has many other problems.) + // 2. Devices attached to those backends are identified by a string + // that includes the backend UUID; see `id_to_device_name` below. + // 3. Other components that are implicitly added to all VMs are + // assigned unique component names within this function. + // + // Because *Nexus object names* (which *can* alias) are never used + // directly to name spec components, there should never be a + // conflict, so this helper asserts that all keys in the component + // map are unique. + fn add_component( + spec: &mut propolis_client::types::InstanceSpecV0, + key: String, + component: ComponentV0, + ) { + assert!(spec.components.insert(key, component).is_none()); + } + + fn id_to_device_name(id: &Uuid) -> String { + format!("{id}:device") + } + + let mut spec = propolis_client::types::InstanceSpecV0 { + board: Board { + chipset: Chipset::default(), + cpus: self.vcpus, + memory_mb: self.memory_mib, + cpuid: None, + }, + components: Default::default(), + }; + + for (name, num) in [ + ("com1", SerialPortNumber::Com1), + ("com2", SerialPortNumber::Com2), + ("com3", SerialPortNumber::Com3), + ("com4", SerialPortNumber::Com4), + ] { + add_component( + &mut spec, + name.to_string(), + ComponentV0::SerialPort(SerialPort { num }), + ); + } + + for DiskComponents { id, device, backend } in disks.into_iter() { + add_component( + &mut spec, + id_to_device_name(&id), + ComponentV0::NvmeDisk(device), + ); - let request = propolis_client::types::InstanceEnsureRequest { - properties: self.properties.clone(), - vcpus: self.vcpus, - memory: self.memory_mib, - nics, - disks: self - .requested_disks - .iter() - .cloned() - .map(Into::into) - .collect(), - boot_settings: self.boot_settings.clone(), - migrate, - cloud_init_bytes: self.cloud_init_bytes.clone().map(|x| x.0), + add_component( + &mut spec, + id.to_string(), + ComponentV0::CrucibleStorageBackend(backend), + ); + } + + for NicComponents { id, device, backend } in nics.into_iter() { + add_component( + &mut spec, + id_to_device_name(&id), + ComponentV0::VirtioNic(device), + ); + add_component( + &mut spec, + id.to_string(), + ComponentV0::VirtioNetworkBackend(backend), + ); + } + + add_component( + &mut spec, + "pvpanic".to_owned(), + ComponentV0::QemuPvpanic(QemuPvpanic { enable_isa: true }), + ); + + if let Some(settings) = &self.boot_settings { + // The boot order contains a list of disk IDs. Propolis matches + // boot order entries based on device component names, so pass + // the ID through `id_to_device_name` when generating the + // Propolis boot order. + let settings = ComponentV0::BootSettings(BootSettings { + order: settings + .order + .iter() + .map(|id| BootOrderEntry { + id: SpecKey::Name(id_to_device_name(&id)), + }) + .collect(), + }); + + add_component(&mut spec, "boot_settings".to_string(), settings); + } + + if let Some(cloud_init) = &self.cloud_init_bytes { + let device_name = "cloud-init-dev"; + let backend_name = "cloud-init-backend"; + + // The old Propolis API (and sled-agent's arguments to it) + // always attached cloud-init drives at BDF 0.24.0. + let device = ComponentV0::VirtioDisk(VirtioDisk { + backend_id: SpecKey::Name(backend_name.to_string()), + pci_path: PciPath::new(0, 0x18, 0).unwrap(), + }); + + let backend = + ComponentV0::BlobStorageBackend(BlobStorageBackend { + base64: cloud_init.0.clone(), + readonly: true, + }); + + add_component(&mut spec, device_name.to_string(), device); + add_component(&mut spec, backend_name.to_string(), backend); + } + + propolis_client::types::InstanceEnsureRequest { + properties: self.properties.clone(), + init: InstanceInitializationMethod::Spec { spec }, + } }; debug!(self.log, "Sending ensure request to propolis: {:?}", request); diff --git a/sled-agent/src/sim/http_entrypoints_pantry.rs b/sled-agent/src/sim/http_entrypoints_pantry.rs index c98c7db665..e879cea70f 100644 --- a/sled-agent/src/sim/http_entrypoints_pantry.rs +++ b/sled-agent/src/sim/http_entrypoints_pantry.rs @@ -9,7 +9,7 @@ use dropshot::{ HttpResponseDeleted, HttpResponseOk, HttpResponseUpdatedNoContent, Path as TypedPath, RequestContext, TypedBody, }; -use propolis_client::types::VolumeConstructionRequest; +use propolis_client::VolumeConstructionRequest; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::sync::Arc; diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index 1f099fc036..0653b52508 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/sled-agent/src/sim/sled_agent.rs @@ -45,7 +45,11 @@ use omicron_uuid_kinds::{ }; use oxnet::Ipv6Net; use propolis_client::{ - types::VolumeConstructionRequest, Client as PropolisClient, + types::{ + Board, Chipset, ComponentV0, InstanceInitializationMethod, + InstanceSpecV0, SerialPort, SerialPortNumber, + }, + Client as PropolisClient, VolumeConstructionRequest, }; use sled_agent_api::SupportBundleMetadata; use sled_agent_api::SupportBundleState; @@ -125,7 +129,7 @@ fn extract_targets_from_volume_construction_request( VolumeConstructionRequest::Region { opts, .. } => { for target in &opts.target { - res.push(SocketAddr::from_str(&target)?); + res.push(*target); } } @@ -301,10 +305,7 @@ impl SledAgent { // Ensure that any disks that are in this request are attached to // this instance. - let id = match disk.volume_construction_request { - VolumeConstructionRequest::Volume { id, .. } => id, - _ => panic!("Unexpected construction type"), - }; + let id = disk.disk_id; self.disks .sim_ensure( &id, @@ -353,15 +354,30 @@ impl SledAgent { description: "sled-agent-sim created instance".to_string(), metadata, }; + let body = propolis_client::types::InstanceEnsureRequest { properties, - memory: hardware.properties.memory.to_whole_mebibytes(), - vcpus: hardware.properties.ncpus.0 as u8, - nics: vec![], - disks: vec![], - boot_settings: None, - migrate: None, - cloud_init_bytes: None, + init: InstanceInitializationMethod::Spec { + spec: InstanceSpecV0 { + board: Board { + cpus: hardware.properties.ncpus.0 as u8, + chipset: Chipset::default(), + memory_mb: hardware + .properties + .memory + .to_whole_mebibytes(), + cpuid: None, + }, + components: [( + "com1".to_string(), + ComponentV0::SerialPort(SerialPort { + num: SerialPortNumber::Com1, + }), + )] + .into_iter() + .collect(), + }, + }, }; // Try to create the instance client.instance_ensure().body(body).send().await.map_err( @@ -397,7 +413,7 @@ impl SledAgent { .await?; for disk_request in &hardware.disks { - let vcr = &disk_request.volume_construction_request; + let vcr = serde_json::from_str(&disk_request.vcr_json.0)?; self.map_disk_ids_to_region_ids(&vcr).await?; } diff --git a/sled-agent/src/sim/storage.rs b/sled-agent/src/sim/storage.rs index 2299ba9db9..c706c05b14 100644 --- a/sled-agent/src/sim/storage.rs +++ b/sled-agent/src/sim/storage.rs @@ -36,7 +36,7 @@ use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::PropolisUuid; use omicron_uuid_kinds::SupportBundleUuid; use omicron_uuid_kinds::ZpoolUuid; -use propolis_client::types::VolumeConstructionRequest; +use propolis_client::VolumeConstructionRequest; use serde::Serialize; use sled_agent_api::SupportBundleMetadata; use sled_agent_api::SupportBundleState; diff --git a/sled-agent/types/src/instance.rs b/sled-agent/types/src/instance.rs index 39726030b0..3ff73925e4 100644 --- a/sled-agent/types/src/instance.rs +++ b/sled-agent/types/src/instance.rs @@ -47,6 +47,39 @@ pub struct InstanceEnsureBody { pub metadata: InstanceMetadata, } +/// A request to attach a disk to an instance. +#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] +pub struct InstanceDisk { + /// The disk's UUID. + pub disk_id: Uuid, + /// The logical slot number assigned to the disk in its database record. + pub slot: u8, + /// True if the disk is read-only. + pub read_only: bool, + /// A JSON representation of the Crucible volume construction request for + /// this attachment. + // + // This is marked as `NoDebug` because the VCR contains the volume's + // encryption keys. + pub vcr_json: NoDebug, + + /// The disk's name, used to generate the serial number for the virtual disk + /// exposed to the guest. + // + // TODO(#7153): Making this depend on the disk name means that a disk's ID + // may change if it is renamed or if a snapshot of it is used to create a + // new disk. + pub name: String, +} + +/// Configures how an instance is told to try to boot. +#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] +pub struct InstanceBootSettings { + /// Propolis should tell guest firmware to try to boot from devices in this + /// order. + pub order: Vec, +} + /// Describes the instance hardware. #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] pub struct InstanceHardware { @@ -59,9 +92,8 @@ pub struct InstanceHardware { pub floating_ips: Vec, pub firewall_rules: Vec, pub dhcp_config: DhcpConfig, - // TODO: replace `propolis_client::*` with locally-modeled request type - pub disks: Vec, - pub boot_settings: Option, + pub disks: Vec, + pub boot_settings: Option, pub cloud_init_bytes: Option>, } @@ -152,9 +184,6 @@ pub struct VmmUnregisterResponse { /// migration. #[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct InstanceMigrationTargetParams { - /// The Propolis ID of the migration source. - pub src_propolis_id: Uuid, - /// The address of the Propolis server that will serve as the migration /// source. pub src_propolis_addr: SocketAddr,