From 53e1211c192a9132561075c3ec2d382a7caa8020 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Tue, 5 Nov 2024 12:44:51 -0800 Subject: [PATCH 1/8] Target replacement support for VCRs with multiple sub_volumes Updated the volume layer code to support replacement of a target in a VCR that has multiple sub_volumes in it. Before this we only supported a single sub_volume. Most of the changes here are for tests. I've added a loop around vcr replacement tests to create multiple sub volumes and iterate through the test for different VCR configurations. In addition, it is now required that the generation number change for all sub-volumes when any target is changed. --- integration_tests/src/lib.rs | 14 +- upstairs/src/volume.rs | 1395 ++++++++++++++++++++++------------ 2 files changed, 933 insertions(+), 476 deletions(-) diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index 130510d18..003f32978 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -5695,13 +5695,13 @@ mod test { blocks_per_extent: sv_tds.blocks_per_extent(), extent_count: sv_tds.extent_count(), opts: sv_opts.clone(), - gen: 1, + gen: 2, }]; let new_vol = VolumeConstructionRequest::Volume { id: sv_volume_id, block_size: BLOCK_SIZE as u64, - sub_volumes: new_sub_vol.clone(), + sub_volumes: new_sub_vol, read_only_parent: Some(rop), }; @@ -5715,6 +5715,14 @@ mod test { let new_downstairs = tds.new_downstairs().await.unwrap(); info!(log, "A New downstairs: {:?}", new_downstairs.address()); + let more_sub_vol = vec![VolumeConstructionRequest::Region { + block_size: BLOCK_SIZE as u64, + blocks_per_extent: sv_tds.blocks_per_extent(), + extent_count: sv_tds.extent_count(), + opts: sv_opts.clone(), + gen: 3, + }]; + let mut new_opts = tds.opts().clone(); new_opts.target[0] = new_downstairs.address(); info!(log, "Old ops target: {:?}", opts.target); @@ -5732,7 +5740,7 @@ mod test { let replacement = VolumeConstructionRequest::Volume { id: sv_volume_id, block_size: BLOCK_SIZE as u64, - sub_volumes: new_sub_vol.clone(), + sub_volumes: more_sub_vol, read_only_parent: Some(new_rop), }; diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index 17b3737c3..f61f3f530 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -39,7 +39,7 @@ fn build_region_definition( } // Result of comparing two [`VolumeConstructionRequest::Region`] values -#[derive(Debug)] +#[derive(Debug, Clone)] enum VCRDelta { /// The Regions are identical. Same, @@ -50,7 +50,7 @@ enum VCRDelta { } // Result of comparing two [`VolumeConstructionRequest::Volume`] values -#[derive(Debug)] +#[derive(Debug, Clone)] enum CompareResult { Volume { sub_compares: Vec, @@ -1398,7 +1398,6 @@ impl Volume { parts.push_back(read_only_parent_compare); } - CompareResult::Region { delta } => { match delta { VCRDelta::Same => { @@ -1437,9 +1436,6 @@ impl Volume { // Walk the cases. As some deltas are okay for a read only // parent that are not for a sub_volume, we need to look at all // the possible permutations Nexus supports. - // - // TODO also modify this when multiple subvolumes supported! - if targets.is_empty() { // If here, we passed by the `all_same` bail above, meaning // there's a generation delta somewhere. @@ -1462,6 +1458,26 @@ impl Volume { panic!("should only have pushed Target"); }; + // We know we have one target that is different. Verify that + // all the rest of the sub_volumes have a generation number + // difference. + for sc in sub_compares { + match sc { + CompareResult::Region { delta } => match delta { + VCRDelta::Same => { + crucible_bail!( + ReplaceRequestInvalid, + "SubVolumes don't have generation bump" + ); + } + VCRDelta::Generation | VCRDelta::Target { .. } => {} + }, + _ => { + panic!("Unsupported multi level CompareResult"); + } + } + } + Ok(Some((*old, *new))) } @@ -1487,6 +1503,12 @@ impl Volume { } } + // Given two VCRs, compare them to each other, including each sub_volume + // and the read only parent. Return the collection of results in the + // CompareResult struct. Any mismatch results in an error returned + // right away and no further processing. The caller is responsible for + // parsing the CompareResult and deciding what to do if there are + // multiple changes. fn compare_vcr_for_replacement( log: &Logger, o_vol: &VolumeConstructionRequest, @@ -1536,52 +1558,50 @@ impl Volume { ) } - // Presently, we only support one sub_volume for replacement. - // If support for multiple sub_volumes is added, then this - // following section will need to be updated to loop over the - // sub_volume Vec and find the specific one with a difference, - // while verifying that all other sub_volumes are no different. - if n_sub_volumes.len() != 1 { - crucible_bail!( - ReplaceRequestInvalid, - "Only a single sub_volume is supported" - ) + let mut sv_results = Vec::new(); + // Walk the list of SubVolumes, get the compare result from + // each one and add it to list for the final result. + // If we get an error, then that is returned right away and + // we don't bother processing any further. + for (o_sv, new_sv) in + o_sub_volumes.iter().zip(n_sub_volumes.iter()) + { + let sv_res = Self::compare_vcr_for_replacement( + log, o_sv, new_sv, read_only, + )?; + sv_results.push(sv_res); } + let read_only_parent_compare = + Box::new(match (o_read_only_parent, n_read_only_parent) { + (.., None) => { + // The New VCR is none. When comparing + // read_only_parents this is okay. + CompareResult::NewMissing + } - Ok(CompareResult::Volume { - sub_compares: vec![Self::compare_vcr_for_replacement( - log, - &o_sub_volumes[0], - &n_sub_volumes[0], - read_only, - )?], - - read_only_parent_compare: Box::new( - match (o_read_only_parent, n_read_only_parent) { - (.., None) => { - // The New VCR is none. When comparing - // read_only_parents this is okay. - CompareResult::NewMissing - } + (None, Some(..)) => { + // It's never valid when comparing VCRs to have + // the original VCR be missing and the new VCR + // is present. + crucible_bail!( + ReplaceRequestInvalid, + "VCR added where there should not be one" + ); + } - (None, Some(..)) => { - // It's never valid when comparing VCRs to have - // the original VCR be missing and the new VCR - // is present. - crucible_bail!( - ReplaceRequestInvalid, - "VCR added where there should not be one" - ); - } + (Some(o_vol), Some(n_vol)) => { + Self::compare_vcr_for_replacement( + log, o_vol, n_vol, true, + )? + } + }); - (Some(o_vol), Some(n_vol)) => { - Self::compare_vcr_for_replacement( - log, o_vol, n_vol, true, - )? - } - }, - ), - }) + let sv_res = CompareResult::Volume { + sub_compares: sv_results, + read_only_parent_compare, + }; + + Ok(sv_res.clone()) } ( @@ -3490,55 +3510,116 @@ mod test { // A valid replacement VCR is provided with only one target being // different. // Test all three targets for replacement. - for cid in 0..3 { - test_volume_replace_cid(cid).unwrap(); + // Test with 1, 2, and 3 sub_volumes. + // Test with the difference being in each sub_volume. + // Test both with and without a read_only_parent. + for sv in 1..4 { + for sv_changed in 0..sv { + for cid in 0..3 { + test_volume_replace_inner(sv, sv_changed, cid, false) + .unwrap(); + test_volume_replace_inner(sv, sv_changed, cid, true) + .unwrap(); + } + } } } - fn test_volume_replace_cid(cid: usize) -> Result<()> { + // A basic replacement of a target in a sub_volume, with options to + // create multiple sub_volumes, and to select which sub_volume and specific + // target to be different. + // + // sv_count: The number of sub volumes we create in each volume. + // sv_changed: The index (zero based) of the sub volume where we + // want the target to be different. + // cid: The client ID where the change will happen, which is the + // same as the index in the crucible opts target array. + // add_rop: True to create a read_only_parent, false for None + fn test_volume_replace_inner( + sv_count: usize, + sv_changed: usize, + cid: usize, + add_rop: bool, + ) -> Result<()> { + assert!(sv_count > sv_changed); + assert!(cid < 3); // A valid replacement VCR is provided with a larger generation // number and only one target being different. + let vol_id = Uuid::new_v4(); + let opts = generic_crucible_opts(vol_id); + let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); + let rop = if add_rop { + let rop = Box::new(VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 3, + }); + Some(rop) + } else { + None + }; - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], - read_only_parent: None, + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, + read_only_parent: rop.clone(), }; // Change just the minimum things and use the updated values // in the replacement volume. let original_target = opts.target[cid]; let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); - opts.target[cid] = new_target; - let replacement = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let replacement_gen = 3; + + if i == sv_changed { + replacement_opts.target[cid] = new_target; + } + + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts: opts.clone(), - gen: 3, - }], - read_only_parent: None, + opts: replacement_opts.clone(), + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + let replacement = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes: replacement_sub_volumes, + read_only_parent: rop.clone(), }; let log = csl(); - info!(log, "Test replacement of CID {cid}"); + info!(log, + "replacement of CID {cid} with sv_count:{} sv_changed:{} rop_some:{}", + sv_count, sv_changed, rop.is_some() + ); let ReplacementRequestCheck::Valid { old, new } = Volume::compare_vcr_for_target_replacement( original, @@ -3549,76 +3630,116 @@ mod test { panic!("wrong variant returned!"); }; - info!(log, "replace {old} with {new}"); + info!(log, "replaced {old} with {new}"); assert_eq!(original_target, old); assert_eq!(new_target, new); Ok(()) } #[test] - fn volume_replace_rop() { - // A replacement VCR is provided with one target being - // different, both new and old have a read_only_parent + fn test_volume_replace_no_gen() { + // We need at least two sub_volumes for this test. + for sv in 2..4 { + for sv_changed in 0..sv { + test_volume_replace_no_gen_inner(sv, sv_changed, false); + test_volume_replace_no_gen_inner(sv, sv_changed, true); + } + } + } + + fn test_volume_replace_no_gen_inner( + sv_count: usize, + sv_changed: usize, + add_rop: bool, + ) { + assert!(sv_count > sv_changed); + // A replacement VCR is created with a single sub_volume that has + // a new target and a larger generation, but the other sub_volumes do + // not have increased generation numbers, which is not valid + // We require that all sub_volumes have bumped generation numbers. + let vol_id = Uuid::new_v4(); + let opts = generic_crucible_opts(vol_id); + let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let opts = generic_crucible_opts(vol_id); - - let rop = Box::new(VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }); + let rop = if add_rop { + let rop = Box::new(VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 3, + }); + Some(rop) + } else { + None + }; - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], - read_only_parent: Some(rop.clone()), + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, + read_only_parent: rop.clone(), }; - let mut new_opts = opts.clone(); - let original_target = opts.target[1]; + // Change just the target and gen for one subvolume, but not the others. let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); - new_opts.target[1] = new_target; - let replacement = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + + if i == sv_changed { + replacement_opts.target[1] = new_target; + replacement_gen += 1; + } + + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts: new_opts.clone(), - gen: 3, - }], - read_only_parent: Some(rop), + opts: replacement_opts.clone(), + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + let replacement = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes: replacement_sub_volumes, + read_only_parent: rop.clone(), }; let log = csl(); - let ReplacementRequestCheck::Valid { old, new } = - Volume::compare_vcr_for_target_replacement( - original, - replacement, - &log, - ) - .unwrap() - else { - panic!("wrong variant returned!"); - }; - - assert_eq!(original_target, old); - assert_eq!(new_target, new); + info!( + log, + "replacement of with sv_count:{} sv_changed:{} rop_some:{}", + sv_count, + sv_changed, + rop.is_some() + ); + assert!(Volume::compare_vcr_for_target_replacement( + original, + replacement, + &log, + ) + .is_err()); } #[tokio::test] @@ -3688,10 +3809,19 @@ mod test { assert_eq!(new_target, new); } - #[tokio::test] - async fn volume_replace_self() { - // Send the same VCR as both old and new, this should return error - // because the generation number did not change. + #[test] + // Send the same VCR as both old and new, this should return error + // because the generation number did not change. + // Test with 1, 2, and 3 sub_volumes. + // Test both with and without a read_only_parent. + fn volume_replace_self() { + for sv in 1..4 { + test_volume_replace_self_inner(sv, false); + test_volume_replace_self_inner(sv, true); + } + } + + fn test_volume_replace_self_inner(sv_count: usize, add_rop: bool) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -3699,17 +3829,35 @@ mod test { let opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let rop = if add_rop { + let rop = Box::new(VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts, + opts: opts.clone(), + gen: 3, + }); + Some(rop) + } else { + None + }; + + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), gen: 2, - }], - read_only_parent: None, + }; + sub_volumes.push(sv); + } + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, + read_only_parent: rop, }; let log = csl(); @@ -3722,42 +3870,75 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_vcr_no_target() { - // A replacement VCR is provided with differing gen, but the - // same targets. - // This is valid for a migration, but not valid for a target - // replacement. We test both calls here. + #[test] + // A replacement VCR is provided with differing gen, but the same targets. + // This is valid for a migration, but not valid for a target replacement. + // We test both calls here. + // Test with 1, 2, and 3 sub_volumes. + // Test both with and without a read_only_parent. + fn volume_vcr_no_targets() { + for sv in 1..3 { + volume_vcr_no_targets_inner(sv, false); + volume_vcr_no_targets_inner(sv, true); + } + } + + fn volume_vcr_no_targets_inner(sv_count: usize, add_rop: bool) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; let opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let rop = if add_rop { + let rop = Box::new(VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), - gen: 2, - }], - read_only_parent: None, + gen: 3, + }); + Some(rop) + } else { + None }; - let replacement = VolumeConstructionRequest::Volume { + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 2, + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + sub_volumes, + read_only_parent: rop.clone(), + }; + + let mut replacement_sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts, + opts: opts.clone(), gen: 3, - }], - read_only_parent: None, + }; + replacement_sub_volumes.push(sv); + } + + let replacement = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes: replacement_sub_volumes, + read_only_parent: rop, }; let log = csl(); @@ -3776,46 +3957,83 @@ mod test { Volume::compare_vcr_for_migration(original, replacement, &log).unwrap(); } - #[tokio::test] - async fn volume_replace_mismatch_vblock() { - // A replacement VCR is provided with one target being - // different, but with incorrect volume layer block size + #[test] + // A replacement VCR is provided with one target being different, but with + // incorrect volume layer block size + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_mismatch_vblock() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_mismatch_vblock_inner(sv, sv_changed); + } + } + } + + fn volume_replace_mismatch_vblock_inner( + sv_count: usize, + sv_changed: usize, + ) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); + let opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], + }; + sub_volumes.push(sv); + } + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: None, }; - opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; - let replacement = VolumeConstructionRequest::Volume { - id: vol_id, - block_size: 4096, - sub_volumes: vec![VolumeConstructionRequest::Region { + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + } + + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts, - gen: 3, - }], + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + + let replacement = VolumeConstructionRequest::Volume { + id: vol_id, + block_size: 4096, + sub_volumes: replacement_sub_volumes, read_only_parent: None, }; let log = csl(); + info!( + log, + "block_size mismatch with sv_count:{} sv_changed:{}", + sv_count, + sv_changed, + ); + assert!(Volume::compare_vcr_for_target_replacement( original, replacement, @@ -3824,44 +4042,79 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_replace_mismatch_vid() { - // A replacement VCR is provided with one target being - // different, but with incorrect volume layer volume id + #[test] + // A replacement VCR is provided with one target being different, but with + // incorrect volume layer volume id + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_mismatch_vid() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_mismatch_vid_inner(sv, sv_changed); + } + } + } + + fn volume_replace_mismatch_vid_inner(sv_count: usize, sv_changed: usize) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let opts = generic_crucible_opts(vol_id); + + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: None, }; - opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - let replacement = VolumeConstructionRequest::Volume { - id: Uuid::new_v4(), - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + } + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts: opts.clone(), - gen: 3, - }], + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + + let replacement = VolumeConstructionRequest::Volume { + id: Uuid::new_v4(), + block_size, + sub_volumes: replacement_sub_volumes, read_only_parent: None, }; let log = csl(); + info!( + log, + "volume id mismatch with sv_count:{} sv_changed:{}", + sv_count, + sv_changed, + ); assert!(Volume::compare_vcr_for_target_replacement( original, replacement, @@ -3870,42 +4123,70 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_replace_mismatch_vrop() { - // A replacement VCR is provided with one target being - // different, but with the replacement volume having a read only - // parent, which is not allowed. + #[test] + // A replacement VCR is provided with one target being different, but with + // the replacement volume having a read only parent, which is not allowed. + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_mismatch_vrop() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_mismatch_vrop_inner(sv, sv_changed); + } + } + } + + fn volume_replace_mismatch_vrop_inner(sv_count: usize, sv_changed: usize) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let opts = generic_crucible_opts(vol_id); + let mut sub_volumes = Vec::new(); + + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: None, }; - opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + } + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + // Replacement can't have a read_only_parent let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }], + sub_volumes: replacement_sub_volumes, read_only_parent: Some(Box::new( VolumeConstructionRequest::Region { block_size, @@ -3918,6 +4199,12 @@ mod test { }; let log = csl(); + info!( + log, + "volume no new ROP with sv_count:{} sv_changed:{}", + sv_count, + sv_changed, + ); assert!(Volume::compare_vcr_for_target_replacement( original, replacement, @@ -3926,19 +4213,38 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_replace_read_only_parent_ok() { - // Verify VCR replacement passes with a valid difference in - // just the read_only_parents. + #[test] + // Verify VCR replacement passes with a valid difference in + // just the read_only_parents. + // Test with 1, 2, and 3 sub_volumes. + fn volume_replace_read_only_parent_ok() { + for sv in 1..4 { + volume_replace_read_only_parent_ok_inner(sv); + } + } + + fn volume_replace_read_only_parent_ok_inner(sv_count: usize) { let block_size = 512; let vol_id = Uuid::new_v4(); let rop_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - // Make the sub_volume that both VCRs will share. let opts = generic_crucible_opts(vol_id); + // Make the sub_volume(s). + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 2, + }; + sub_volumes.push(sv); + } + // Make the read only parent. let mut rop_opts = generic_crucible_opts(rop_id); @@ -3946,13 +4252,7 @@ mod test { let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }], + sub_volumes, read_only_parent: Some(Box::new( VolumeConstructionRequest::Region { block_size, @@ -3969,18 +4269,23 @@ mod test { let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); rop_opts.target[1] = new_target; - // Make the replacement VCR with the updated target for the - // read_only_parent. - let replacement = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 3, - }], + }; + sub_volumes.push(sv); + } + // Make the replacement VCR with the updated target for the + // read_only_parent. + let replacement = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: Some(Box::new( VolumeConstructionRequest::Region { block_size, @@ -4008,26 +4313,44 @@ mod test { assert_eq!(new_target, new); } - #[tokio::test] - async fn volume_replace_with_both_subvol_and_rop_diff() { - // Verify that a valid diff in both the sub_volumes and the - // read_only_parent returns an error (we can do only one at - // a time). + #[test] + // Verify that a valid diff in both the sub_volumes and the + // read_only_parent returns an error (we can do only one at a time). + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_with_both_subvol_and_rop_diff() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_with_both_subvol_and_rop_diff_inner( + sv, sv_changed, + ); + } + } + } + + fn volume_replace_with_both_subvol_and_rop_diff_inner( + sv_count: usize, + sv_changed: usize, + ) { let block_size = 512; let vol_id = Uuid::new_v4(); let rop_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - // Make the sub_volume that both VCRs will share. - let mut opts = generic_crucible_opts(vol_id); - let sub_vol = vec![VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }]; + // Make the sub_volume(s) that both VCRs will share. + let opts = generic_crucible_opts(vol_id); + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 2, + }; + sub_volumes.push(sv); + } // Make the read only parent. let mut rop_opts = generic_crucible_opts(rop_id); @@ -4043,12 +4366,29 @@ mod test { let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: sub_vol.clone(), + sub_volumes, read_only_parent: Some(Box::new(rop.clone())), }; // Update the sub_volume target with a new downstairs - opts.target[1] = "127.0.0.1:9888".parse().unwrap(); + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + } + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } // Update the ROP target with a new downstairs rop_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); @@ -4058,25 +4398,25 @@ mod test { let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }], + sub_volumes: replacement_sub_volumes, read_only_parent: Some(Box::new( VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: rop_opts.clone(), - gen: 5, + gen: 4, }, )), }; let log = csl(); + info!( + log, + "new ROP and new SV with sv_count:{} sv_changed:{}", + sv_count, + sv_changed, + ); assert!(Volume::compare_vcr_for_target_replacement( original, @@ -4086,45 +4426,80 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_replace_mismatch_sv_bs() { - // A replacement VCR is provided with one target being - // different, but with the replacement volume having a sub_volume - // with a different block size. + #[test] + // A replacement VCR is provided with one target being different, but with + // the replacement volume having a sub_volume with a different block size. + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_mismatch_sv_bs() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_mismatch_sv_bs_inner(sv, sv_changed); + } + } + } + + fn volume_replace_mismatch_sv_bs_inner(sv_count: usize, sv_changed: usize) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let opts = generic_crucible_opts(vol_id); + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: None, }; - opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + let mut replacement_block_size = block_size; + + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + replacement_block_size = 4096; + } + let sv = VolumeConstructionRequest::Region { + block_size: replacement_block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size: 4096, - blocks_per_extent, - extent_count, - opts, - gen: 3, - }], + sub_volumes: replacement_sub_volumes, read_only_parent: None, }; let log = csl(); + info!( + log, + "replacement sub-vol mismatch block_size sv_count:{} sv_changed:{}", + sv_count, + sv_changed, + ); assert!(Volume::compare_vcr_for_target_replacement( original, replacement, @@ -4133,46 +4508,85 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_replace_mismatch_sv_bpe() { - // A replacement VCR is provided with one target being - // different, but with the replacement volume having a sub_volume - // with a different blocks per extent. + #[test] + // A replacement VCR is provided with one target being different, but with + // the replacement volume having a sub_volume with a different blocks per + // extent. + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_mismatch_sv_bpe() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_mismatch_sv_bpe_inner(sv, sv_changed); + } + } + } + + fn volume_replace_mismatch_sv_bpe_inner( + sv_count: usize, + sv_changed: usize, + ) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let opts = generic_crucible_opts(vol_id); + + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: None, }; - opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + let mut replacement_bpe = blocks_per_extent; + + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + replacement_bpe += 2; + } + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent: replacement_bpe, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size, - blocks_per_extent: blocks_per_extent + 2, - extent_count, - opts, - gen: 3, - }], + sub_volumes: replacement_sub_volumes, read_only_parent: None, }; let log = csl(); + info!( + log, + "subvolume mismatch in blocks_per_extent sv_count:{} sv_changed:{}", + sv_count, + sv_changed, + ); assert!(Volume::compare_vcr_for_target_replacement( original, replacement, @@ -4181,41 +4595,68 @@ mod test { .is_err()); } - #[tokio::test] - async fn volume_replace_mismatch_sv_ec() { - // A replacement VCR is provided with one target being - // different, but with the replacement volume having a sub_volume - // with a different extent count. + #[test] + // A replacement VCR is provided with one target being different, but with + // the replacement volume having a sub_volume with a different extent count. + // Test with 1, 2, and 3 sub_volumes. + // Test both with each sub_volume having the difference. + fn volume_replace_mismatch_sv_ec() { + for sv in 1..4 { + for sv_changed in 0..sv { + volume_replace_mismatch_sv_ec_inner(sv, sv_changed); + } + } + } + fn volume_replace_mismatch_sv_ec_inner(sv_count: usize, sv_changed: usize) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; - let mut opts = generic_crucible_opts(vol_id); - let original = VolumeConstructionRequest::Volume { - id: vol_id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let opts = generic_crucible_opts(vol_id); + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, - }], + }; + sub_volumes.push(sv); + } + + let original = VolumeConstructionRequest::Volume { + id: vol_id, + block_size, + sub_volumes, read_only_parent: None, }; - opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; + let mut replacement_ec = extent_count; + + if i == sv_changed { + replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); + replacement_gen += 1; + replacement_ec += 2; + } + let sv = VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count: replacement_ec, + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count: extent_count + 2, - opts, - gen: 3, - }], + sub_volumes: replacement_sub_volumes, read_only_parent: None, }; @@ -4228,6 +4669,9 @@ mod test { // We create two Volumes with the provided information, and use o_opts // for one Volume and n_opts for the other. We return the result of // the compare_vcr_for_target_replacement function. + // sv_count: The number of sub_volumes to create. + // sv_changed: The index for which sub_volume we put the new opts.. + #[allow(clippy::too_many_arguments)] fn test_volume_replace_opts( id: Uuid, block_size: u64, @@ -4235,30 +4679,51 @@ mod test { extent_count: u32, o_opts: CrucibleOpts, n_opts: CrucibleOpts, + sv_count: usize, + sv_changed: usize, ) -> Result<(SocketAddr, SocketAddr), crucible_common::CrucibleError> { - let original = VolumeConstructionRequest::Volume { - id, - block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + let mut sub_volumes = Vec::new(); + for _ in 0..sv_count { + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts: o_opts, + opts: o_opts.clone(), gen: 2, - }], - read_only_parent: None, - }; + }; + sub_volumes.push(sv); + } - let replacement = VolumeConstructionRequest::Volume { + let original = VolumeConstructionRequest::Volume { id, block_size, - sub_volumes: vec![VolumeConstructionRequest::Region { + sub_volumes, + read_only_parent: None, + }; + + let mut replacement_sub_volumes = Vec::new(); + for i in 0..sv_count { + let mut replacement_opts = o_opts.clone(); + let mut replacement_gen = 2; + + if i == sv_changed { + replacement_opts = n_opts.clone(); + replacement_gen += 1; + } + let sv = VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, - opts: n_opts, - gen: 3, - }], + opts: replacement_opts, + gen: replacement_gen, + }; + replacement_sub_volumes.push(sv); + } + + let replacement = VolumeConstructionRequest::Volume { + id, + block_size, + sub_volumes: replacement_sub_volumes, read_only_parent: None, }; @@ -4291,15 +4756,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.id = Uuid::new_v4(); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4307,6 +4778,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.lossy. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4317,15 +4790,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.lossy = true; - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4333,6 +4812,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.flush_timeout. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4343,15 +4824,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.flush_timeout = Some(1.23459); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4359,6 +4846,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.key. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4371,15 +4860,21 @@ mod test { let key_string = engine::general_purpose::STANDARD.encode(key_bytes); n_opts.key = Some(key_string); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4387,6 +4882,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.cert_pem. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4397,15 +4894,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.cert_pem = Some("cert_pem".to_string()); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4413,6 +4916,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.key_pem. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4423,15 +4928,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.key_pem = Some("key_pem".to_string()); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4439,6 +4950,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.root_cert. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4449,15 +4962,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.root_cert_pem = Some("root_pem".to_string()); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4465,6 +4984,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.control. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4475,15 +4996,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.control = Some("127.0.0.1:8888".parse().unwrap()); - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); + } + } } #[tokio::test] @@ -4491,6 +5018,8 @@ mod test { // A replacement VCR is provided with one target being // different, but with the replacement volume having a sub_volume // with a different opts.read_only. + // We test with 1 to 3 sub_volumes and each possible sub_volume having + // the change let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; @@ -4501,101 +5030,21 @@ mod test { n_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); n_opts.read_only = true; - assert!(test_volume_replace_opts( - vol_id, - 512, - blocks_per_extent, - extent_count, - o_opts, - n_opts - ) - .is_err()); - } - - #[test] - fn volume_replace_with_rop() { - let vol_id = Uuid::new_v4(); - let original = VolumeConstructionRequest::Volume { - block_size: 512, - id: vol_id, - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size: 512, - blocks_per_extent: 131072, - extent_count: 128, - gen: 3, - opts: CrucibleOpts { - id: vol_id, - target: vec![ - "[fd00:1122:3344:102::8]:19004".parse().unwrap(), - "[fd00:1122:3344:101::7]:19003".parse().unwrap(), - "[fd00:1122:3344:104::8]:19000".parse().unwrap(), - ], - ..Default::default() - }, - }], - read_only_parent: Some(Box::new( - VolumeConstructionRequest::Volume { - block_size: 512, - id: Uuid::new_v4(), - sub_volumes: vec![VolumeConstructionRequest::Region { - block_size: 512, - blocks_per_extent: 131072, - extent_count: 32, - gen: 2, - opts: CrucibleOpts { - id: Uuid::new_v4(), - target: vec![ - "[fd00:1122:3344:103::7]:19002" - .parse() - .unwrap(), - "[fd00:1122:3344:101::7]:19002" - .parse() - .unwrap(), - "[fd00:1122:3344:102::8]:19002" - .parse() - .unwrap(), - ], - ..Default::default() - }, - }], - read_only_parent: None, - }, - )), - }; - - // Replace one of the subvolume targets and bump the gen number - let mut replacement = original.clone(); - match &mut replacement { - VolumeConstructionRequest::Volume { sub_volumes, .. } => { - match &mut sub_volumes[0] { - VolumeConstructionRequest::Region { opts, gen, .. } => { - opts.target[1] = - "[fd00:1122:3344:111::a]:20000".parse().unwrap(); - *gen += 1; - } - - _ => { - panic!("how?!"); - } - } - } - - _ => { - panic!("how?!"); + for sv in 1..4 { + for sv_changed in 0..sv { + assert!(test_volume_replace_opts( + vol_id, + 512, + blocks_per_extent, + extent_count, + o_opts.clone(), + n_opts.clone(), + sv, + sv_changed, + ) + .is_err()); } } - - let log = csl(); - let result = - Volume::compare_vcr_for_update(original, replacement, &log) - .unwrap(); - assert_eq!( - result, - Some(( - "[fd00:1122:3344:101::7]:19003".parse().unwrap(), - "[fd00:1122:3344:111::a]:20000".parse().unwrap(), - )), - ); } /// Test that changes under a read-only parent work From ef074fc5e64bd73915c9b53a48d7f9e861ba8c14 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 7 Nov 2024 09:20:40 -0800 Subject: [PATCH 2/8] Backout un-needed generation bump --- integration_tests/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index 003f32978..d4ec17717 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -5695,7 +5695,7 @@ mod test { blocks_per_extent: sv_tds.blocks_per_extent(), extent_count: sv_tds.extent_count(), opts: sv_opts.clone(), - gen: 2, + gen: 1, }]; let new_vol = VolumeConstructionRequest::Volume { From 733b511b28c3cffdc4df1e71f71371c6c9037c45 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 7 Nov 2024 09:43:57 -0800 Subject: [PATCH 3/8] No clone needed --- upstairs/src/volume.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index f61f3f530..43520a315 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -39,7 +39,7 @@ fn build_region_definition( } // Result of comparing two [`VolumeConstructionRequest::Region`] values -#[derive(Debug, Clone)] +#[derive(Debug)] enum VCRDelta { /// The Regions are identical. Same, @@ -50,7 +50,7 @@ enum VCRDelta { } // Result of comparing two [`VolumeConstructionRequest::Volume`] values -#[derive(Debug, Clone)] +#[derive(Debug)] enum CompareResult { Volume { sub_compares: Vec, @@ -1601,7 +1601,7 @@ impl Volume { read_only_parent_compare, }; - Ok(sv_res.clone()) + Ok(sv_res) } ( From 2032eea38799608306f737536734e3bccc48cfc8 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 7 Nov 2024 12:23:00 -0800 Subject: [PATCH 4/8] Make an enum for ROP states --- upstairs/src/volume.rs | 252 ++++++++++++++++++++++++++++------------- 1 file changed, 172 insertions(+), 80 deletions(-) diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index 43520a315..3fc444fe7 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -3505,6 +3505,16 @@ mod test { } } + // For tests to decide what mix of read_only_parents the two VCRs should + // be created with. + #[derive(Debug, PartialEq)] + enum ReadOnlyParentMode { + Both, + OnlyOriginal, + OnlyReplacement, + Neither, + } + #[test] fn volume_replace_basic() { // A valid replacement VCR is provided with only one target being @@ -3512,14 +3522,38 @@ mod test { // Test all three targets for replacement. // Test with 1, 2, and 3 sub_volumes. // Test with the difference being in each sub_volume. - // Test both with and without a read_only_parent. + // Test all combinations of read_only_parents. for sv in 1..4 { for sv_changed in 0..sv { for cid in 0..3 { - test_volume_replace_inner(sv, sv_changed, cid, false) - .unwrap(); - test_volume_replace_inner(sv, sv_changed, cid, true) - .unwrap(); + test_volume_replace_inner( + sv, + sv_changed, + cid, + ReadOnlyParentMode::Both, + ) + .unwrap(); + test_volume_replace_inner( + sv, + sv_changed, + cid, + ReadOnlyParentMode::OnlyOriginal, + ) + .unwrap(); + test_volume_replace_inner( + sv, + sv_changed, + cid, + ReadOnlyParentMode::OnlyReplacement, + ) + .unwrap_err(); + test_volume_replace_inner( + sv, + sv_changed, + cid, + ReadOnlyParentMode::Neither, + ) + .unwrap(); } } } @@ -3534,12 +3568,12 @@ mod test { // want the target to be different. // cid: The client ID where the change will happen, which is the // same as the index in the crucible opts target array. - // add_rop: True to create a read_only_parent, false for None + // rop_mode: enum of which VCR will have a ROP and which will not. fn test_volume_replace_inner( sv_count: usize, sv_changed: usize, cid: usize, - add_rop: bool, + rop_mode: ReadOnlyParentMode, ) -> Result<()> { assert!(sv_count > sv_changed); assert!(cid < 3); @@ -3553,17 +3587,21 @@ mod test { let blocks_per_extent = 10; let extent_count = 9; - let rop = if add_rop { - let rop = Box::new(VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }); - Some(rop) - } else { - None + let test_rop = Box::new(VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 3, + }); + + let (original_rop, replacement_rop) = match rop_mode { + ReadOnlyParentMode::Both => { + (Some(test_rop.clone()), Some(test_rop)) + } + ReadOnlyParentMode::OnlyOriginal => (Some(test_rop), None), + ReadOnlyParentMode::OnlyReplacement => (None, Some(test_rop)), + ReadOnlyParentMode::Neither => (None, None), }; let mut sub_volumes = Vec::new(); @@ -3582,7 +3620,7 @@ mod test { id: vol_id, block_size, sub_volumes, - read_only_parent: rop.clone(), + read_only_parent: original_rop, }; // Change just the minimum things and use the updated values @@ -3612,13 +3650,13 @@ mod test { id: vol_id, block_size, sub_volumes: replacement_sub_volumes, - read_only_parent: rop.clone(), + read_only_parent: replacement_rop, }; let log = csl(); info!(log, - "replacement of CID {cid} with sv_count:{} sv_changed:{} rop_some:{}", - sv_count, sv_changed, rop.is_some() + "replacement of CID {} with sv_count:{} sv_changed:{} rop_mode:{:?}", + cid, sv_count, sv_changed, rop_mode ); let ReplacementRequestCheck::Valid { old, new } = Volume::compare_vcr_for_target_replacement( @@ -3641,8 +3679,26 @@ mod test { // We need at least two sub_volumes for this test. for sv in 2..4 { for sv_changed in 0..sv { - test_volume_replace_no_gen_inner(sv, sv_changed, false); - test_volume_replace_no_gen_inner(sv, sv_changed, true); + test_volume_replace_no_gen_inner( + sv, + sv_changed, + ReadOnlyParentMode::Both, + ); + test_volume_replace_no_gen_inner( + sv, + sv_changed, + ReadOnlyParentMode::OnlyOriginal, + ); + test_volume_replace_no_gen_inner( + sv, + sv_changed, + ReadOnlyParentMode::OnlyReplacement, + ); + test_volume_replace_no_gen_inner( + sv, + sv_changed, + ReadOnlyParentMode::Neither, + ); } } } @@ -3650,7 +3706,7 @@ mod test { fn test_volume_replace_no_gen_inner( sv_count: usize, sv_changed: usize, - add_rop: bool, + rop_mode: ReadOnlyParentMode, ) { assert!(sv_count > sv_changed); // A replacement VCR is created with a single sub_volume that has @@ -3665,17 +3721,21 @@ mod test { let blocks_per_extent = 10; let extent_count = 9; - let rop = if add_rop { - let rop = Box::new(VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }); - Some(rop) - } else { - None + let test_rop = Box::new(VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 3, + }); + + let (original_rop, replacement_rop) = match rop_mode { + ReadOnlyParentMode::Both => { + (Some(test_rop.clone()), Some(test_rop)) + } + ReadOnlyParentMode::OnlyOriginal => (Some(test_rop), None), + ReadOnlyParentMode::OnlyReplacement => (None, Some(test_rop)), + ReadOnlyParentMode::Neither => (None, None), }; let mut sub_volumes = Vec::new(); @@ -3694,7 +3754,7 @@ mod test { id: vol_id, block_size, sub_volumes, - read_only_parent: rop.clone(), + read_only_parent: original_rop, }; // Change just the target and gen for one subvolume, but not the others. @@ -3723,16 +3783,16 @@ mod test { id: vol_id, block_size, sub_volumes: replacement_sub_volumes, - read_only_parent: rop.clone(), + read_only_parent: replacement_rop, }; let log = csl(); info!( log, - "replacement of with sv_count:{} sv_changed:{} rop_some:{}", + "replacement of with sv_count:{} sv_changed:{} rop_mode:{:?}", sv_count, sv_changed, - rop.is_some() + rop_mode, ); assert!(Volume::compare_vcr_for_target_replacement( original, @@ -3816,12 +3876,15 @@ mod test { // Test both with and without a read_only_parent. fn volume_replace_self() { for sv in 1..4 { - test_volume_replace_self_inner(sv, false); - test_volume_replace_self_inner(sv, true); + test_volume_replace_self_inner(sv, ReadOnlyParentMode::Both); + test_volume_replace_self_inner(sv, ReadOnlyParentMode::Neither); } } - fn test_volume_replace_self_inner(sv_count: usize, add_rop: bool) { + fn test_volume_replace_self_inner( + sv_count: usize, + rop_mode: ReadOnlyParentMode, + ) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -3829,17 +3892,22 @@ mod test { let opts = generic_crucible_opts(vol_id); - let rop = if add_rop { - let rop = Box::new(VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }); - Some(rop) - } else { - None + let rop = match rop_mode { + ReadOnlyParentMode::Both => { + let test_rop = Box::new(VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 3, + }); + Some(test_rop) + } + ReadOnlyParentMode::Neither => None, + ReadOnlyParentMode::OnlyOriginal + | ReadOnlyParentMode::OnlyReplacement => { + panic!("Unsupported test mode"); + } }; let mut sub_volumes = Vec::new(); @@ -3875,32 +3943,45 @@ mod test { // This is valid for a migration, but not valid for a target replacement. // We test both calls here. // Test with 1, 2, and 3 sub_volumes. - // Test both with and without a read_only_parent. + // Test with all combinations of read_only_parent, knowing that the + // OnlyReplacement case should always fail. fn volume_vcr_no_targets() { for sv in 1..3 { - volume_vcr_no_targets_inner(sv, false); - volume_vcr_no_targets_inner(sv, true); + volume_vcr_no_targets_inner(sv, ReadOnlyParentMode::Both); + volume_vcr_no_targets_inner(sv, ReadOnlyParentMode::OnlyOriginal); + volume_vcr_no_targets_inner( + sv, + ReadOnlyParentMode::OnlyReplacement, + ); + volume_vcr_no_targets_inner(sv, ReadOnlyParentMode::Neither); } } - fn volume_vcr_no_targets_inner(sv_count: usize, add_rop: bool) { + fn volume_vcr_no_targets_inner( + sv_count: usize, + rop_mode: ReadOnlyParentMode, + ) { let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; let extent_count = 9; let opts = generic_crucible_opts(vol_id); - let rop = if add_rop { - let rop = Box::new(VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }); - Some(rop) - } else { - None + let test_rop = Box::new(VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen: 3, + }); + + let (original_rop, replacement_rop) = match rop_mode { + ReadOnlyParentMode::Both => { + (Some(test_rop.clone()), Some(test_rop)) + } + ReadOnlyParentMode::OnlyOriginal => (Some(test_rop), None), + ReadOnlyParentMode::OnlyReplacement => (None, Some(test_rop)), + ReadOnlyParentMode::Neither => (None, None), }; let mut sub_volumes = Vec::new(); @@ -3919,7 +4000,7 @@ mod test { id: vol_id, block_size, sub_volumes, - read_only_parent: rop.clone(), + read_only_parent: original_rop, }; let mut replacement_sub_volumes = Vec::new(); @@ -3938,23 +4019,34 @@ mod test { id: vol_id, block_size, sub_volumes: replacement_sub_volumes, - read_only_parent: rop, + read_only_parent: replacement_rop, }; let log = csl(); // Replacement should return ReplacementMatchesOriginal - assert!(matches!( - Volume::compare_vcr_for_target_replacement( - original.clone(), - replacement.clone(), - &log, - ), - Ok(ReplacementRequestCheck::ReplacementMatchesOriginal), - )); + let res = Volume::compare_vcr_for_target_replacement( + original.clone(), + replacement.clone(), + &log, + ); + if rop_mode == ReadOnlyParentMode::OnlyReplacement { + assert!(res.is_err()); + } else { + assert!(matches!( + res, + Ok(ReplacementRequestCheck::ReplacementMatchesOriginal) + )); + }; // Migration is valid with these VCRs - Volume::compare_vcr_for_migration(original, replacement, &log).unwrap(); + let res = + Volume::compare_vcr_for_migration(original, replacement, &log); + if rop_mode == ReadOnlyParentMode::OnlyReplacement { + assert!(res.is_err()); + } else { + assert!(res.is_ok()); + }; } #[test] From ab9e0223e1dd230f0f275c45313bd09af658f142 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 7 Nov 2024 14:07:39 -0800 Subject: [PATCH 5/8] Helper function to make sub-volumes Cleaned up some comments. --- upstairs/src/volume.rs | 299 +++++++++++++++++++---------------------- 1 file changed, 139 insertions(+), 160 deletions(-) diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index 3fc444fe7..ab8e9910d 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -1574,15 +1574,16 @@ impl Volume { let read_only_parent_compare = Box::new(match (o_read_only_parent, n_read_only_parent) { (.., None) => { - // The New VCR is none. When comparing - // read_only_parents this is okay. + // The replacement VCR has no read_only_parent. This + // is a valid situation as read_only_parents will go + // away after a scrub finishes. CompareResult::NewMissing } (None, Some(..)) => { - // It's never valid when comparing VCRs to have - // the original VCR be missing and the new VCR - // is present. + // It's never valid when comparing VCRs to have the + // original VCR be missing a read_only_parent and the + // new VCR to have a read_only_parent. crucible_bail!( ReplaceRequestInvalid, "VCR added where there should not be one" @@ -3559,6 +3560,26 @@ mod test { } } + // Helper function to build as many sv_count sub-volumes as requested. + fn build_subvolume_vcr( + sv_count: usize, + block_size: u64, + blocks_per_extent: u64, + extent_count: u32, + opts: CrucibleOpts, + gen: u64, + ) -> Vec { + (0..sv_count) + .map(|_| VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: opts.clone(), + gen, + }) + .collect() + } + // A basic replacement of a target in a sub_volume, with options to // create multiple sub_volumes, and to select which sub_volume and specific // target to be different. @@ -3604,17 +3625,14 @@ mod test { ReadOnlyParentMode::Neither => (None, None), }; - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -3738,17 +3756,14 @@ mod test { ReadOnlyParentMode::Neither => (None, None), }; - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -3910,17 +3925,14 @@ mod test { } }; - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -3984,17 +3996,14 @@ mod test { ReadOnlyParentMode::Neither => (None, None), }; - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4073,17 +4082,15 @@ mod test { let opts = generic_crucible_opts(vol_id); - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); + let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -4155,17 +4162,14 @@ mod test { let opts = generic_crucible_opts(vol_id); - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4235,18 +4239,15 @@ mod test { let extent_count = 9; let opts = generic_crucible_opts(vol_id); - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4325,17 +4326,14 @@ mod test { let opts = generic_crucible_opts(vol_id); // Make the sub_volume(s). - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); // Make the read only parent. let mut rop_opts = generic_crucible_opts(rop_id); @@ -4361,17 +4359,14 @@ mod test { let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); rop_opts.target[1] = new_target; - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 3, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 3, + ); // Make the replacement VCR with the updated target for the // read_only_parent. let replacement = VolumeConstructionRequest::Volume { @@ -4432,17 +4427,14 @@ mod test { // Make the sub_volume(s) that both VCRs will share. let opts = generic_crucible_opts(vol_id); - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); // Make the read only parent. let mut rop_opts = generic_crucible_opts(rop_id); @@ -4538,17 +4530,14 @@ mod test { let extent_count = 9; let opts = generic_crucible_opts(vol_id); - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4624,18 +4613,14 @@ mod test { let extent_count = 9; let opts = generic_crucible_opts(vol_id); - - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4706,17 +4691,14 @@ mod test { let extent_count = 9; let opts = generic_crucible_opts(vol_id); - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4774,17 +4756,14 @@ mod test { sv_count: usize, sv_changed: usize, ) -> Result<(SocketAddr, SocketAddr), crucible_common::CrucibleError> { - let mut sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: o_opts.clone(), - gen: 2, - }; - sub_volumes.push(sv); - } + let sub_volumes = build_subvolume_vcr( + sv_count, + block_size, + blocks_per_extent, + extent_count, + o_opts.clone(), + 2, + ); let original = VolumeConstructionRequest::Volume { id, From 1949ae7de5df9bfe5256f718340e3474f353f129 Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Thu, 7 Nov 2024 17:21:22 -0800 Subject: [PATCH 6/8] subvolume helpers Created a function build_replacement_subvol that will create Vec of Regions based on input provided. This should reduce a bunch of common code. Converted a bunch of other replacement subvolume creation to be done immutably. --- upstairs/src/volume.rs | 379 ++++++++++++++++++++--------------------- 1 file changed, 186 insertions(+), 193 deletions(-) diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index ab8e9910d..0d7453797 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -3580,6 +3580,53 @@ mod test { .collect() } + // Take an existing VCR (required to be VCR::Region), and fabricate a + // new set of VCRs from it. We change the generation number of all the + // new sub_volumes, but only change the requested target in one requested + // sub_volume at the requested client ID. + fn build_replacement_subvol( + sv_count: usize, + sv_changed: usize, + source_vcr: VolumeConstructionRequest, + cid: usize, + ) -> Vec { + let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); + + let (block_size, blocks_per_extent, extent_count, opts, gen) = + match source_vcr { + VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts, + gen, + } => { + (block_size, blocks_per_extent, extent_count, opts, gen + 1) + } + _ => { + panic!("Unsupported VCR"); + } + }; + + (0..sv_count) + .map(|i| { + let mut replacement_opts = opts.clone(); + + if i == sv_changed { + replacement_opts.target[cid] = new_target; + } + + VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts.clone(), + gen, + } + }) + .collect() + } + // A basic replacement of a target in a sub_volume, with options to // create multiple sub_volumes, and to select which sub_volume and specific // target to be different. @@ -3637,7 +3684,7 @@ mod test { let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, - sub_volumes, + sub_volumes: sub_volumes.clone(), read_only_parent: original_rop, }; @@ -3646,24 +3693,13 @@ mod test { let original_target = opts.target[cid]; let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let replacement_gen = 3; - - if i == sv_changed { - replacement_opts.target[cid] = new_target; - } + let replacement_sub_volumes = build_replacement_subvol( + sv_count, + sv_changed, + sub_volumes[0].clone(), + cid, + ); - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts.clone(), - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -3775,25 +3811,26 @@ mod test { // Change just the target and gen for one subvolume, but not the others. let new_target: SocketAddr = "127.0.0.1:8888".parse().unwrap(); - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; + let replacement_sub_volumes = (0..sv_count) + .map(|i| { + let mut replacement_opts = opts.clone(); + let mut replacement_gen = 2; - if i == sv_changed { - replacement_opts.target[1] = new_target; - replacement_gen += 1; - } + if i == sv_changed { + replacement_opts.target[1] = new_target; + replacement_gen += 1; + } + + VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts.clone(), + gen: replacement_gen, + } + }) + .collect(); - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts.clone(), - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -4012,17 +4049,15 @@ mod test { read_only_parent: original_rop, }; - let mut replacement_sub_volumes = Vec::new(); - for _ in 0..sv_count { - let sv = VolumeConstructionRequest::Region { + let replacement_sub_volumes = (0..sv_count) + .map(|_| VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 3, - }; - replacement_sub_volumes.push(sv); - } + }) + .collect(); let replacement = VolumeConstructionRequest::Volume { id: vol_id, @@ -4075,6 +4110,7 @@ mod test { sv_count: usize, sv_changed: usize, ) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -4091,6 +4127,13 @@ mod test { 2, ); + let replacement_sub_volumes = build_replacement_subvol( + sv_count, + sv_changed, + sub_volumes[0].clone(), + 1, + ); + let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -4098,26 +4141,6 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - } - - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } - let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size: 4096, @@ -4155,6 +4178,7 @@ mod test { } fn volume_replace_mismatch_vid_inner(sv_count: usize, sv_changed: usize) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -4171,6 +4195,13 @@ mod test { 2, ); + let replacement_sub_volumes = build_replacement_subvol( + sv_count, + sv_changed, + sub_volumes[0].clone(), + 1, + ); + let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -4178,25 +4209,6 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - } - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } - let replacement = VolumeConstructionRequest::Volume { id: Uuid::new_v4(), block_size, @@ -4233,6 +4245,7 @@ mod test { } fn volume_replace_mismatch_vrop_inner(sv_count: usize, sv_changed: usize) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -4249,6 +4262,13 @@ mod test { 2, ); + let replacement_sub_volumes = build_replacement_subvol( + sv_count, + sv_changed, + sub_volumes[0].clone(), + 1, + ); + let original = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -4256,25 +4276,6 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - } - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } - // Replacement can't have a read_only_parent let replacement = VolumeConstructionRequest::Volume { id: vol_id, @@ -4419,6 +4420,7 @@ mod test { sv_count: usize, sv_changed: usize, ) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let rop_id = Uuid::new_v4(); @@ -4446,6 +4448,13 @@ mod test { gen: 4, }; + let replacement_sub_volumes = build_replacement_subvol( + sv_count, + sv_changed, + sub_volumes[0].clone(), + 1, + ); + // Make the original VCR using what we created above. let original = VolumeConstructionRequest::Volume { id: vol_id, @@ -4454,26 +4463,6 @@ mod test { read_only_parent: Some(Box::new(rop.clone())), }; - // Update the sub_volume target with a new downstairs - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - } - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } - // Update the ROP target with a new downstairs rop_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); @@ -4524,6 +4513,7 @@ mod test { } fn volume_replace_mismatch_sv_bs_inner(sv_count: usize, sv_changed: usize) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -4546,26 +4536,26 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - let mut replacement_block_size = block_size; + let replacement_sub_volumes = (0..sv_count) + .map(|i| { + let mut replacement_opts = opts.clone(); + let replacement_gen = 3; + let mut replacement_block_size = block_size; - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - replacement_block_size = 4096; - } - let sv = VolumeConstructionRequest::Region { - block_size: replacement_block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } + if i == sv_changed { + replacement_opts.target[1] = + "127.0.0.1:8888".parse().unwrap(); + replacement_block_size = 4096; + } + VolumeConstructionRequest::Region { + block_size: replacement_block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + } + }) + .collect(); let replacement = VolumeConstructionRequest::Volume { id: vol_id, @@ -4607,6 +4597,7 @@ mod test { sv_count: usize, sv_changed: usize, ) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -4629,26 +4620,26 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - let mut replacement_bpe = blocks_per_extent; + let replacement_sub_volumes = (0..sv_count) + .map(|i| { + let mut replacement_opts = opts.clone(); + let replacement_gen = 3; + let mut replacement_bpe = blocks_per_extent; - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - replacement_bpe += 2; - } - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent: replacement_bpe, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } + if i == sv_changed { + replacement_opts.target[1] = + "127.0.0.1:8888".parse().unwrap(); + replacement_bpe += 2; + } + VolumeConstructionRequest::Region { + block_size, + blocks_per_extent: replacement_bpe, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + } + }) + .collect(); let replacement = VolumeConstructionRequest::Volume { id: vol_id, @@ -4685,6 +4676,7 @@ mod test { } } fn volume_replace_mismatch_sv_ec_inner(sv_count: usize, sv_changed: usize) { + assert!(sv_count > sv_changed); let block_size = 512; let vol_id = Uuid::new_v4(); let blocks_per_extent = 10; @@ -4707,26 +4699,27 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = opts.clone(); - let mut replacement_gen = 2; - let mut replacement_ec = extent_count; + let replacement_sub_volumes = (0..sv_count) + .map(|i| { + let mut replacement_opts = opts.clone(); + let replacement_gen = 3; + let mut replacement_ec = extent_count; + + if i == sv_changed { + replacement_opts.target[1] = + "127.0.0.1:8888".parse().unwrap(); + replacement_ec += 2; + } + VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count: replacement_ec, + opts: replacement_opts, + gen: replacement_gen, + } + }) + .collect(); - if i == sv_changed { - replacement_opts.target[1] = "127.0.0.1:8888".parse().unwrap(); - replacement_gen += 1; - replacement_ec += 2; - } - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count: replacement_ec, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } let replacement = VolumeConstructionRequest::Volume { id: vol_id, block_size, @@ -4756,6 +4749,7 @@ mod test { sv_count: usize, sv_changed: usize, ) -> Result<(SocketAddr, SocketAddr), crucible_common::CrucibleError> { + assert!(sv_count > sv_changed); let sub_volumes = build_subvolume_vcr( sv_count, block_size, @@ -4772,24 +4766,23 @@ mod test { read_only_parent: None, }; - let mut replacement_sub_volumes = Vec::new(); - for i in 0..sv_count { - let mut replacement_opts = o_opts.clone(); - let mut replacement_gen = 2; + let replacement_sub_volumes = (0..sv_count) + .map(|i| { + let mut replacement_opts = o_opts.clone(); + let replacement_gen = 3; - if i == sv_changed { - replacement_opts = n_opts.clone(); - replacement_gen += 1; - } - let sv = VolumeConstructionRequest::Region { - block_size, - blocks_per_extent, - extent_count, - opts: replacement_opts, - gen: replacement_gen, - }; - replacement_sub_volumes.push(sv); - } + if i == sv_changed { + replacement_opts = n_opts.clone(); + } + VolumeConstructionRequest::Region { + block_size, + blocks_per_extent, + extent_count, + opts: replacement_opts, + gen: replacement_gen, + } + }) + .collect(); let replacement = VolumeConstructionRequest::Volume { id, From f755325b9c05f572fa3c053f5aedfe50ba83e8de Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Mon, 11 Nov 2024 11:38:24 -0800 Subject: [PATCH 7/8] A test_all for ReadOnlyParentMode --- upstairs/src/volume.rs | 42 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index 0d7453797..01abaf40b 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -3516,6 +3516,15 @@ mod test { Neither, } + impl ReadOnlyParentMode { + fn test_all(mut f: F) { + f(ReadOnlyParentMode::Both); + f(ReadOnlyParentMode::OnlyOriginal); + f(ReadOnlyParentMode::OnlyReplacement); + f(ReadOnlyParentMode::Neither); + } + } + #[test] fn volume_replace_basic() { // A valid replacement VCR is provided with only one target being @@ -3733,26 +3742,9 @@ mod test { // We need at least two sub_volumes for this test. for sv in 2..4 { for sv_changed in 0..sv { - test_volume_replace_no_gen_inner( - sv, - sv_changed, - ReadOnlyParentMode::Both, - ); - test_volume_replace_no_gen_inner( - sv, - sv_changed, - ReadOnlyParentMode::OnlyOriginal, - ); - test_volume_replace_no_gen_inner( - sv, - sv_changed, - ReadOnlyParentMode::OnlyReplacement, - ); - test_volume_replace_no_gen_inner( - sv, - sv_changed, - ReadOnlyParentMode::Neither, - ); + ReadOnlyParentMode::test_all(|mode| { + test_volume_replace_no_gen_inner(sv, sv_changed, mode); + }); } } } @@ -3996,13 +3988,9 @@ mod test { // OnlyReplacement case should always fail. fn volume_vcr_no_targets() { for sv in 1..3 { - volume_vcr_no_targets_inner(sv, ReadOnlyParentMode::Both); - volume_vcr_no_targets_inner(sv, ReadOnlyParentMode::OnlyOriginal); - volume_vcr_no_targets_inner( - sv, - ReadOnlyParentMode::OnlyReplacement, - ); - volume_vcr_no_targets_inner(sv, ReadOnlyParentMode::Neither); + ReadOnlyParentMode::test_all(|mode| { + volume_vcr_no_targets_inner(sv, mode); + }); } } From 2e2ea368a5183f9f0ae32c84d271faab2664de3f Mon Sep 17 00:00:00 2001 From: Alan Hanson Date: Fri, 10 Jan 2025 11:33:11 -0800 Subject: [PATCH 8/8] Bail don't panic when comparing unsupported VCRs --- upstairs/src/volume.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/upstairs/src/volume.rs b/upstairs/src/volume.rs index 01abaf40b..043c39548 100644 --- a/upstairs/src/volume.rs +++ b/upstairs/src/volume.rs @@ -1472,8 +1472,12 @@ impl Volume { } VCRDelta::Generation | VCRDelta::Target { .. } => {} }, - _ => { - panic!("Unsupported multi level CompareResult"); + r => { + crucible_bail!( + ReplaceRequestInvalid, + "Unsupported multi level CompareResult {:?}", + r + ); } } }