From 97d1e4037c5077304a181617805e8473ad1285fa Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 20 Dec 2024 01:48:58 +0000 Subject: [PATCH] PR feedback: SocketAddrV6, misc cleanup --- nexus/db-queries/src/db/datastore/volume.rs | 23 ++++++---- .../tasks/region_snapshot_replacement_step.rs | 30 ++++++------ nexus/src/app/instance.rs | 46 +++---------------- nexus/src/app/sagas/snapshot_create.rs | 27 +++++++---- .../integration_tests/volume_management.rs | 15 +++--- sled-agent/src/instance.rs | 11 ++--- 6 files changed, 66 insertions(+), 86 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index 9c85586a70..e55c374073 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -2578,11 +2578,15 @@ fn region_in_vcr( VolumeConstructionRequest::Region { opts, .. } => { for target in &opts.target { - if let SocketAddr::V6(target) = target { - if *target == *region { + match target { + SocketAddr::V6(t) if *t == *region => { region_found = true; break; } + SocketAddr::V6(_) => {} + SocketAddr::V4(_) => { + bail!("region target contains an IPv4 address"); + } } } } @@ -2644,10 +2648,16 @@ fn read_only_target_in_vcr( } for target in &opts.target { - if let SocketAddr::V6(target) = target { - if *target == *read_only_target && opts.read_only { + 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"); + } } } } @@ -4986,10 +4996,7 @@ mod tests { .unwrap(); let volumes = datastore - .find_volumes_referencing_socket_addr( - &opctx, - address_1.into(), - ) + .find_volumes_referencing_socket_addr(&opctx, address_1.into()) .await .unwrap(); 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 1893271df5..c8010dd90d 100644 --- a/nexus/src/app/background/tasks/region_snapshot_replacement_step.rs +++ b/nexus/src/app/background/tasks/region_snapshot_replacement_step.rs @@ -568,6 +568,7 @@ mod test { use omicron_uuid_kinds::DatasetUuid; 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.parse().unwrap()], + 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/instance.rs b/nexus/src/app/instance.rs index 86a971846a..109c0d8521 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -1052,7 +1052,6 @@ impl super::Nexus { ) .await?; - let mut found_boot_disk = None; let mut disk_reqs = vec![]; for disk in &disks { // Disks that are attached to an instance should always have a slot @@ -1087,12 +1086,6 @@ impl super::Nexus { ) .await?; - if let Some(wanted_id) = &db_instance.boot_disk_id { - if disk.id() == *wanted_id { - found_boot_disk = Some(*wanted_id); - } - } - disk_reqs.push(sled_agent_client::types::InstanceDisk { disk_id: disk.id(), name: disk.name().to_string(), @@ -1102,38 +1095,13 @@ impl super::Nexus { }); } - let boot_settings = if let Some(boot_disk_id) = found_boot_disk { - Some(InstanceBootSettings { order: vec![boot_disk_id] }) - } 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 found_boot_disk.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/snapshot_create.rs b/nexus/src/app/sagas/snapshot_create.rs index 694cee6bfe..cceec7d70e 100644 --- a/nexus/src/app/sagas/snapshot_create.rs +++ b/nexus/src/app/sagas/snapshot_create.rs @@ -125,6 +125,8 @@ use steno::ActionError; use steno::Node; use uuid::Uuid; +type ReplaceSocketsMap = BTreeMap; + // snapshot create saga: input parameters #[derive(Debug, Deserialize, Serialize)] @@ -1384,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::()?; @@ -1410,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 { @@ -1454,7 +1456,7 @@ async fn ssc_start_running_snapshot( ); info!(log, "map {} to {}", region_addr, snapshot_addr); - map.insert(region_addr.into(), snapshot_addr.into()); + 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 @@ -1564,8 +1566,8 @@ async fn ssc_create_volume_record( // The volume construction request must then be modified to point to the // read-only crucible agent downstairs (corresponding to this snapshot) // launched through this saga. - let replace_sockets_map = sagactx - .lookup::>("replace_sockets_map")?; + let replace_sockets_map = + sagactx.lookup::("replace_sockets_map")?; let snapshot_volume_construction_request: VolumeConstructionRequest = create_snapshot_from_disk( &disk_volume_construction_request, @@ -1689,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: @@ -1751,6 +1753,16 @@ fn create_snapshot_from_disk( if work.socket_modification_required { for target in &mut opts.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) })?; @@ -1869,8 +1881,7 @@ mod test { ], }; - let mut replace_sockets: BTreeMap = - BTreeMap::new(); + let mut replace_sockets = ReplaceSocketsMap::new(); // Replacements for top level Region only replace_sockets.insert( diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index e33ec6db42..b059aae12b 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -3567,7 +3567,7 @@ struct TestReadOnlyRegionReferenceUsage { datastore: Arc, region: db::model::Region, - region_address: SocketAddr, + region_address: SocketAddrV6, first_volume_id: Uuid, second_volume_id: Uuid, @@ -3628,9 +3628,8 @@ impl TestReadOnlyRegionReferenceUsage { // so fill in a random port here. datastore.region_set_port(region.id(), 12345).await.unwrap(); - let region_address = SocketAddr::V6( - datastore.region_addr(region.id()).await.unwrap().unwrap(), - ); + let region_address = + datastore.region_addr(region.id()).await.unwrap().unwrap(); let region = datastore.get_region(region.id()).await.unwrap(); @@ -3661,7 +3660,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -3791,7 +3790,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -3824,7 +3823,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, @@ -3859,7 +3858,7 @@ impl TestReadOnlyRegionReferenceUsage { gen: 1, opts: CrucibleOpts { id: Uuid::new_v4(), - target: vec![self.region_address], + target: vec![self.region_address.into()], lossy: false, flush_timeout: None, key: None, diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index 854be1abd2..8b327ddd9a 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -869,8 +869,8 @@ impl InstanceRunner { // components (e.g., converting device slot numbers into concrete PCI // bus/device/function numbers). // - // Today's Propolis VM creation API (the one used below) differs from - // this scheme in two important ways: + // 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). @@ -974,12 +974,12 @@ impl InstanceRunner { let nics: Vec = running_zone .opte_ports() .map(|port| -> Result { - let pos = self + 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(), @@ -987,7 +987,6 @@ impl InstanceRunner { ), ))?; - let nic = &self.requested_nics[pos]; Ok(NicComponents { id: nic.id, device: VirtioNic { @@ -1012,7 +1011,7 @@ impl InstanceRunner { // - 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, which devices may have different names + // 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