From 23818526491ee75063b1704a8d746dd25dba5e27 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 29 May 2024 13:44:22 -0400 Subject: [PATCH] Don't recurse through VolumeConstructionRequests (#5825) VolumeConstructionRequest objects can be arbitrarily deep, as customers are not restricted in the disk and snapshot layering that they can do. There are a few functions that recurse through these objects: change those to instead use an iterative approach to avoid hitting any recursion limits. Fixes #5815 --- nexus/db-queries/src/db/datastore/volume.rs | 299 +++++++++----------- nexus/src/app/sagas/disk_create.rs | 89 +++--- nexus/src/app/sagas/snapshot_create.rs | 128 ++++----- sled-agent/src/sim/sled_agent.rs | 49 ++-- 4 files changed, 248 insertions(+), 317 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index a7b9273aa8..294cd2decf 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -45,6 +45,7 @@ use serde::Deserialize; use serde::Deserializer; use serde::Serialize; use sled_agent_client::types::VolumeConstructionRequest; +use std::collections::VecDeque; use std::net::SocketAddrV6; use uuid::Uuid; @@ -690,78 +691,56 @@ impl DataStore { pub fn randomize_ids( vcr: &VolumeConstructionRequest, ) -> anyhow::Result { - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size, - sub_volumes, - read_only_parent, - } => Ok(VolumeConstructionRequest::Volume { - id: Uuid::new_v4(), - block_size: *block_size, - sub_volumes: sub_volumes - .iter() - .map( - |subvol| -> anyhow::Result { - Self::randomize_ids(&subvol) - }, - ) - .collect::>>( - )?, - read_only_parent: if let Some(read_only_parent) = - read_only_parent - { - Some(Box::new(Self::randomize_ids(read_only_parent)?)) - } else { - None - }, - }), + let mut new_vcr = vcr.clone(); - VolumeConstructionRequest::Url { id: _, block_size, url } => { - Ok(VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size: *block_size, - url: url.clone(), - }) - } + let mut parts: VecDeque<&mut VolumeConstructionRequest> = + VecDeque::new(); + parts.push_back(&mut new_vcr); - VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts, - gen, - } => { - if !opts.read_only { - // Only one volume can "own" a Region, and that volume's - // UUID is recorded in the region table accordingly. It is - // an error to make a copy of a volume construction request - // that references non-read-only Regions. - bail!( - "only one Volume can reference a Region non-read-only!" - ); + while let Some(vcr_part) = parts.pop_front() { + match vcr_part { + VolumeConstructionRequest::Volume { + id, + sub_volumes, + read_only_parent, + .. + } => { + *id = Uuid::new_v4(); + + for sub_volume in sub_volumes { + parts.push_back(sub_volume); + } + + if let Some(read_only_parent) = read_only_parent { + parts.push_back(read_only_parent); + } } - let mut opts = opts.clone(); - opts.id = Uuid::new_v4(); + VolumeConstructionRequest::Url { id, .. } => { + *id = Uuid::new_v4(); + } - Ok(VolumeConstructionRequest::Region { - block_size: *block_size, - blocks_per_extent: *blocks_per_extent, - extent_count: *extent_count, - opts, - gen: *gen, - }) - } + VolumeConstructionRequest::Region { opts, .. } => { + if !opts.read_only { + // Only one volume can "own" a Region, and that volume's + // UUID is recorded in the region table accordingly. It is + // an error to make a copy of a volume construction request + // that references non-read-only Regions. + bail!( + "only one Volume can reference a Region non-read-only!" + ); + } - VolumeConstructionRequest::File { id: _, block_size, path } => { - Ok(VolumeConstructionRequest::File { - id: Uuid::new_v4(), - block_size: *block_size, - path: path.clone(), - }) + opts.id = Uuid::new_v4(); + } + + VolumeConstructionRequest::File { id, .. } => { + *id = Uuid::new_v4(); + } } } + + Ok(new_vcr) } /// Checkout a copy of the Volume from the database using `volume_checkout`, @@ -1901,48 +1880,40 @@ pub fn read_only_resources_associated_with_volume( vcr: &VolumeConstructionRequest, crucible_targets: &mut CrucibleTargets, ) { - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size: _, - sub_volumes, - read_only_parent, - } => { - for sub_volume in sub_volumes { - read_only_resources_associated_with_volume( - sub_volume, - crucible_targets, - ); - } + let mut parts: VecDeque<&VolumeConstructionRequest> = VecDeque::new(); + parts.push_back(&vcr); - if let Some(read_only_parent) = read_only_parent { - read_only_resources_associated_with_volume( - read_only_parent, - crucible_targets, - ); + while let Some(vcr_part) = parts.pop_front() { + match vcr_part { + VolumeConstructionRequest::Volume { + sub_volumes, + read_only_parent, + .. + } => { + for sub_volume in sub_volumes { + parts.push_back(sub_volume); + } + + if let Some(read_only_parent) = read_only_parent { + parts.push_back(read_only_parent); + } } - } - VolumeConstructionRequest::Url { id: _, block_size: _, url: _ } => { - // no action required - } + VolumeConstructionRequest::Url { .. } => { + // no action required + } - VolumeConstructionRequest::Region { - block_size: _, - blocks_per_extent: _, - extent_count: _, - opts, - gen: _, - } => { - for target in &opts.target { - if opts.read_only { - crucible_targets.read_only_targets.push(target.clone()); + VolumeConstructionRequest::Region { opts, .. } => { + for target in &opts.target { + if opts.read_only { + crucible_targets.read_only_targets.push(target.clone()); + } } } - } - VolumeConstructionRequest::File { id: _, block_size: _, path: _ } => { - // no action required + VolumeConstructionRequest::File { .. } => { + // no action required + } } } } @@ -2005,67 +1976,52 @@ fn replace_region_in_vcr( old_region: SocketAddrV6, new_region: SocketAddrV6, ) -> anyhow::Result { - match vcr { - VolumeConstructionRequest::Volume { - id, - block_size, - sub_volumes, - read_only_parent, - } => Ok(VolumeConstructionRequest::Volume { - id: *id, - block_size: *block_size, - sub_volumes: sub_volumes - .iter() - .map(|subvol| -> anyhow::Result { - replace_region_in_vcr(&subvol, old_region, new_region) - }) - .collect::>>()?, + let mut new_vcr = vcr.clone(); - // Only replacing R/W regions - read_only_parent: read_only_parent.clone(), - }), + let mut parts: VecDeque<&mut VolumeConstructionRequest> = VecDeque::new(); + parts.push_back(&mut new_vcr); - VolumeConstructionRequest::Url { id, block_size, url } => { - Ok(VolumeConstructionRequest::Url { - id: *id, - block_size: *block_size, - url: url.clone(), - }) - } + let mut old_region_found = false; - VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts, - gen, - } => { - let mut opts = opts.clone(); - - for target in &mut opts.target { - let parsed_target: SocketAddrV6 = target.parse()?; - if parsed_target == old_region { - *target = new_region.to_string(); + while let Some(vcr_part) = parts.pop_front() { + match vcr_part { + VolumeConstructionRequest::Volume { sub_volumes, .. } => { + for sub_volume in sub_volumes { + parts.push_back(sub_volume); } + + // Skip looking at read-only parent, this function only replaces + // R/W regions } - Ok(VolumeConstructionRequest::Region { - block_size: *block_size, - blocks_per_extent: *blocks_per_extent, - extent_count: *extent_count, - opts, - gen: *gen + 1, - }) - } + VolumeConstructionRequest::Url { .. } => { + // nothing required + } - VolumeConstructionRequest::File { id, block_size, path } => { - Ok(VolumeConstructionRequest::File { - id: *id, - block_size: *block_size, - path: path.clone(), - }) + 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; + } + } + + // Bump generation number, otherwise update will be rejected + *gen = *gen + 1; + } + + VolumeConstructionRequest::File { .. } => { + // nothing required + } } } + + if !old_region_found { + bail!("old region {old_region} not found!"); + } + + Ok(new_vcr) } /// Find Regions in a Volume's subvolumes list whose target match the argument @@ -2075,31 +2031,36 @@ fn find_matching_rw_regions_in_volume( ip: &std::net::Ipv6Addr, matched_targets: &mut Vec, ) -> anyhow::Result<()> { - match vcr { - VolumeConstructionRequest::Volume { sub_volumes, .. } => { - for sub_volume in sub_volumes { - find_matching_rw_regions_in_volume( - sub_volume, - ip, - matched_targets, - )?; + let mut parts: VecDeque<&VolumeConstructionRequest> = VecDeque::new(); + parts.push_back(vcr); + + while let Some(vcr_part) = parts.pop_front() { + match vcr_part { + VolumeConstructionRequest::Volume { sub_volumes, .. } => { + for sub_volume in sub_volumes { + parts.push_back(sub_volume); + } } - } - VolumeConstructionRequest::Url { .. } => {} + VolumeConstructionRequest::Url { .. } => { + // nothing required + } - 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); + 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); + } } } } - } - VolumeConstructionRequest::File { .. } => {} + VolumeConstructionRequest::File { .. } => { + // nothing required + } + } } Ok(()) diff --git a/nexus/src/app/sagas/disk_create.rs b/nexus/src/app/sagas/disk_create.rs index 5e1d386ed1..ee90f72862 100644 --- a/nexus/src/app/sagas/disk_create.rs +++ b/nexus/src/app/sagas/disk_create.rs @@ -22,6 +22,7 @@ use rand::{rngs::StdRng, RngCore, SeedableRng}; use serde::Deserialize; use serde::Serialize; use sled_agent_client::types::{CrucibleOpts, VolumeConstructionRequest}; +use std::collections::VecDeque; use std::convert::TryFrom; use std::net::SocketAddrV6; use steno::ActionError; @@ -769,65 +770,45 @@ async fn sdc_call_pantry_attach_for_disk_undo( fn randomize_volume_construction_request_ids( input: &VolumeConstructionRequest, ) -> anyhow::Result { - match input { - VolumeConstructionRequest::Volume { - id: _, - block_size, - sub_volumes, - read_only_parent, - } => Ok(VolumeConstructionRequest::Volume { - id: Uuid::new_v4(), - block_size: *block_size, - sub_volumes: sub_volumes - .iter() - .map(|subvol| -> anyhow::Result { - randomize_volume_construction_request_ids(&subvol) - }) - .collect::>>()?, - read_only_parent: if let Some(read_only_parent) = read_only_parent { - Some(Box::new(randomize_volume_construction_request_ids( - read_only_parent, - )?)) - } else { - None - }, - }), + let mut new_vcr = input.clone(); + + let mut parts: VecDeque<&mut VolumeConstructionRequest> = VecDeque::new(); + parts.push_back(&mut new_vcr); + + while let Some(vcr_part) = parts.pop_front() { + match vcr_part { + VolumeConstructionRequest::Volume { + id, + sub_volumes, + read_only_parent, + .. + } => { + *id = Uuid::new_v4(); + + for sub_volume in sub_volumes { + parts.push_back(sub_volume); + } - VolumeConstructionRequest::Url { id: _, block_size, url } => { - Ok(VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size: *block_size, - url: url.clone(), - }) - } + if let Some(read_only_parent) = read_only_parent { + parts.push_back(read_only_parent); + } + } - VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts, - gen, - } => { - let mut opts = opts.clone(); - opts.id = Uuid::new_v4(); - - Ok(VolumeConstructionRequest::Region { - block_size: *block_size, - blocks_per_extent: *blocks_per_extent, - extent_count: *extent_count, - opts, - gen: *gen, - }) - } + VolumeConstructionRequest::Url { id, .. } => { + *id = Uuid::new_v4(); + } - VolumeConstructionRequest::File { id: _, block_size, path } => { - Ok(VolumeConstructionRequest::File { - id: Uuid::new_v4(), - block_size: *block_size, - path: path.clone(), - }) + VolumeConstructionRequest::Region { opts, .. } => { + opts.id = Uuid::new_v4(); + } + + VolumeConstructionRequest::File { id, .. } => { + *id = Uuid::new_v4(); + } } } + + Ok(new_vcr) } #[cfg(test)] diff --git a/nexus/src/app/sagas/snapshot_create.rs b/nexus/src/app/sagas/snapshot_create.rs index 287571cfd5..cca589b758 100644 --- a/nexus/src/app/sagas/snapshot_create.rs +++ b/nexus/src/app/sagas/snapshot_create.rs @@ -117,6 +117,7 @@ use sled_agent_client::types::InstanceIssueDiskSnapshotRequestBody; use sled_agent_client::types::VolumeConstructionRequest; use slog::info; use std::collections::BTreeMap; +use std::collections::VecDeque; use std::net::SocketAddrV6; use steno::ActionError; use steno::Node; @@ -1419,7 +1420,7 @@ async fn ssc_create_volume_record( let snapshot_volume_construction_request: VolumeConstructionRequest = create_snapshot_from_disk( &disk_volume_construction_request, - Some(&replace_sockets_map), + &replace_sockets_map, ) .map_err(|e| { ActionError::action_failed(Error::internal_error(&e.to_string())) @@ -1518,7 +1519,7 @@ async fn ssc_finalize_snapshot_record( /// VolumeConstructionRequest and modifying it accordingly. fn create_snapshot_from_disk( disk: &VolumeConstructionRequest, - socket_map: Option<&BTreeMap>, + socket_map: &BTreeMap, ) -> anyhow::Result { // When copying a disk's VolumeConstructionRequest to turn it into a // snapshot: @@ -1527,78 +1528,73 @@ fn create_snapshot_from_disk( // - set read-only // - remove any control sockets - match disk { - VolumeConstructionRequest::Volume { - id: _, - block_size, - sub_volumes, - read_only_parent, - } => Ok(VolumeConstructionRequest::Volume { - id: Uuid::new_v4(), - block_size: *block_size, - sub_volumes: sub_volumes - .iter() - .map(|subvol| -> anyhow::Result { - create_snapshot_from_disk(&subvol, socket_map) - }) - .collect::>>()?, - read_only_parent: if let Some(read_only_parent) = read_only_parent { - Some(Box::new(create_snapshot_from_disk( - read_only_parent, - // no socket modification required for read-only parents - None, - )?)) - } else { - None - }, - }), + let mut new_vcr = disk.clone(); - VolumeConstructionRequest::Url { id: _, block_size, url } => { - Ok(VolumeConstructionRequest::Url { - id: Uuid::new_v4(), - block_size: *block_size, - url: url.clone(), - }) - } + struct Work<'a> { + vcr_part: &'a mut VolumeConstructionRequest, + socket_modification_required: bool, + } - VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts, - gen, - } => { - let mut opts = opts.clone(); - - if let Some(socket_map) = socket_map { - for target in &mut opts.target { - target.clone_from(socket_map.get(target).ok_or_else( - || anyhow!("target {} not found in map!", target), - )?); + let mut parts: VecDeque = VecDeque::new(); + parts.push_back(Work { + vcr_part: &mut new_vcr, + socket_modification_required: true, + }); + + while let Some(work) = parts.pop_front() { + match work.vcr_part { + VolumeConstructionRequest::Volume { + id, + sub_volumes, + read_only_parent, + .. + } => { + *id = Uuid::new_v4(); + + for sub_volume in sub_volumes { + parts.push_back(Work { + vcr_part: sub_volume, + // Inherit if socket modification is required from the + // parent layer + socket_modification_required: work + .socket_modification_required, + }); + } + + if let Some(read_only_parent) = read_only_parent { + parts.push_back(Work { + vcr_part: read_only_parent, + // no socket modification required for read-only parents + socket_modification_required: false, + }); } } - opts.id = Uuid::new_v4(); - opts.read_only = true; - opts.control = None; + VolumeConstructionRequest::Url { id, .. } => { + *id = Uuid::new_v4(); + } - Ok(VolumeConstructionRequest::Region { - block_size: *block_size, - blocks_per_extent: *blocks_per_extent, - extent_count: *extent_count, - opts, - gen: *gen, - }) - } + VolumeConstructionRequest::Region { opts, .. } => { + opts.id = Uuid::new_v4(); + opts.read_only = true; + opts.control = None; - VolumeConstructionRequest::File { id: _, block_size, path } => { - Ok(VolumeConstructionRequest::File { - id: Uuid::new_v4(), - block_size: *block_size, - path: path.clone(), - }) + 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), + )?); + } + } + } + + VolumeConstructionRequest::File { id, .. } => { + *id = Uuid::new_v4(); + } } } + + Ok(new_vcr) } #[cfg(test)] @@ -1718,7 +1714,7 @@ mod test { ); let snapshot = - create_snapshot_from_disk(&disk, Some(&replace_sockets)).unwrap(); + create_snapshot_from_disk(&disk, &replace_sockets).unwrap(); eprintln!("{:?}", serde_json::to_string(&snapshot).unwrap()); diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index 742639350a..d91b9c9a33 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/sled-agent/src/sim/sled_agent.rs @@ -46,7 +46,7 @@ use propolis_client::{ use propolis_mock_server::Context as PropolisContext; use sled_storage::resources::DisksManagementResult; use slog::Logger; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::str::FromStr; use std::sync::Arc; @@ -93,40 +93,33 @@ fn extract_targets_from_volume_construction_request( // flush. let mut res = vec![]; - match vcr { - VolumeConstructionRequest::Volume { - id: _, - block_size: _, - sub_volumes, - read_only_parent: _, - } => { - for sub_volume in sub_volumes.iter() { - res.extend(extract_targets_from_volume_construction_request( - sub_volume, - )?); + let mut parts: VecDeque<&VolumeConstructionRequest> = VecDeque::new(); + parts.push_back(&vcr); + + while let Some(vcr_part) = parts.pop_front() { + match vcr_part { + VolumeConstructionRequest::Volume { sub_volumes, .. } => { + for sub_volume in sub_volumes { + parts.push_back(sub_volume); + } } - } - VolumeConstructionRequest::Url { .. } => { - // noop - } + VolumeConstructionRequest::Url { .. } => { + // noop + } - VolumeConstructionRequest::Region { - block_size: _, - blocks_per_extent: _, - extent_count: _, - opts, - gen: _, - } => { - for target in &opts.target { - res.push(SocketAddr::from_str(target)?); + VolumeConstructionRequest::Region { opts, .. } => { + for target in &opts.target { + res.push(SocketAddr::from_str(&target)?); + } } - } - VolumeConstructionRequest::File { .. } => { - // noop + VolumeConstructionRequest::File { .. } => { + // noop + } } } + Ok(res) }