Target replacement support for VCRs with multiple sub_volumes #1551

leftwo · 2024-11-05T21:42:33Z

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 the 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.

Some tests that were duplicates have been removed.

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

upstairs/src/volume.rs

mkeeter · 2024-11-07T16:07:36Z

upstairs/src/volume.rs

+                let read_only_parent_compare =
+                    Box::new(match (o_read_only_parent, n_read_only_parent) {
+                        (.., None) => {
+                            // The New VCR is none. When comparing


"The New VCR is none" is confusing. I think it means "The new VCR has no read-only parent"? Also, saying why this is okay would be helpful (I think we need to support the case because it occurs after scrubbing is complete?)

Updated the comment in ab9e022

mkeeter · 2024-11-07T16:08:09Z

upstairs/src/volume.rs

+                for (o_sv, new_sv) in
+                    o_sub_volumes.iter().zip(n_sub_volumes.iter())
+                {
+                    let sv_res = Self::compare_vcr_for_replacement(


Any worries about unbounded recursion blowing up the stack here?

Yes. But, I'm not sure what to do about it.
We currently don't have any limits in Nexus, so you could craft VCRs forever until something broke. I don't know what the limit is currently and what will break first.

upstairs/src/volume.rs

mkeeter · 2024-11-07T16:16:25Z

upstairs/src/volume.rs

-            read_only_parent: None,
+            };
+            sub_volumes.push(sv);
+        }


Nit: this could be slightly shorter + immutable

let sub_volumes = (0..sv_count) .map(|_| VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: opts.clone(), gen: 2, }) .collect();

ab9e022 has this immutable update, plus I made a specific function and called it wherever I could.

mkeeter · 2024-11-07T16:19:45Z

upstairs/src/volume.rs

+            id: vol_id,
+            block_size,
+            sub_volumes: replacement_sub_volumes,
+            read_only_parent: rop.clone(),


Nit: similarly, this could be

let replacement_sub_volumes = (0..sv_count) .map(|i| { let mut replacement_opts = opts.clone(); let replacement_gen = 3; if i == sv_changed { replacement_opts.target[cid] = new_target; } VolumeConstructionRequest::Region { block_size, blocks_per_extent, extent_count, opts: replacement_opts.clone(), gen: replacement_gen, } }) .collect();

In 1949ae7 I did this as well as made a few into a common function.

mkeeter · 2024-11-07T16:32:11Z

upstairs/src/volume.rs

+        sv_count: usize,
+        sv_changed: usize,
+        cid: usize,
+        add_rop: bool,


Nit: add_rop is confusing, because we don't change the ROP state. Maybe with_rop?

Also, should we test removing the read-only parent? We could use a three-variant enum in these tests, e.g.

enum ReadOnlyParentMode { Absent, Present, Remove, }

Since the loops are doing all the work here, we could even do:

ReadOnlyParentMode { Both, OnlyOriginal, OnlyReplacement, Neither, }

The OnlyReplacement should fail 100% of the time.

I did this in 2032eea

mkeeter · 2024-11-07T16:33:39Z

upstairs/src/volume.rs

-            id: vol_id,
-            block_size,
-            sub_volumes: vec![VolumeConstructionRequest::Region {
+        let mut sub_volumes = Vec::new();


Same suggestion about building this immutably with (0..sv_count).map(...)

Done in ab9e022

mkeeter · 2024-11-07T16:34:16Z

upstairs/src/volume.rs

+        }
+    }
+
+    fn test_volume_replace_self_inner(sv_count: usize, add_rop: bool) {


Same comment here about add_rop

With 2032eea, no more add_rop.

mkeeter · 2024-11-07T16:34:22Z

upstairs/src/volume.rs

+    fn test_volume_replace_no_gen_inner(
+        sv_count: usize,
+        sv_changed: usize,
+        add_rop: bool,


Same comment here about add_rop

With 2032eea, no more add_rop.

mkeeter · 2024-11-07T16:36:35Z

upstairs/src/volume.rs

+        let opts = generic_crucible_opts(vol_id);
+
+        let mut sub_volumes = Vec::new();
+        for _ in 0..sv_count {


You know, we should probably just have a build_subvolume_vcr helper function instead of copy-pasting this into every single function...

Done in ab9e022

upstairs/src/volume.rs

Cleaned up some comments.

Created a function build_replacement_subvol that will create Vec<VolumeConstructionRequest> 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.

mkeeter

LGTM

The only follow-up suggestion (optional) is to consolidate "test on every ReadOnlyParentMode", e.g.

impl ReadOnlyParentMode
    fn test_all<F: FnMut(Self)>(mut f: F) {
        f(ReadOnlyParentMode::OnlyOriginal);
        f(ReadOnlyParentMode::OnlyReplacement);
        f(ReadOnlyParentMode::Neither);
        f(ReadOnlyParentMode::Both);
    }
}

...which you could then use like

        for sv in 1..4 {
            for sv_changed in 0..sv {
                for cid in 0..3 {
                    ReadOnlyParentMode::test_all(|mode|
                        test_volume_replace_inner(
                            sv,
                            sv_changed,
                            cid,
                            mode,
                        )
                    );
                }
            }
        }

jmpesp · 2024-12-20T21:00:58Z

upstairs/src/volume.rs

+                            VCRDelta::Generation | VCRDelta::Target { .. } => {}
+                        },
+                        _ => {
+                            panic!("Unsupported multi level CompareResult");


I think this should return ReplaceRequestInvalid instead of panicking

jmpesp · 2024-12-20T21:01:57Z

upstairs/src/volume.rs

+                block_size,
+                blocks_per_extent,
+                extent_count,
+                opts: opts.clone(),


The targets should be randomized here so that all sub volumes are not the same, using something like https://docs.rs/ipgen/1.0.2/ipgen/fn.ip.html

I'm holding it wrong, somehow...

I get back:
Err(InvalidIpNetwork("[BUG] failed to parse the generated IP address a00:67 as IPv4"))

And, the source says:

// This error should never happen but I'm not a fan of panicking in libraries .map_err(|_| Error::InvalidIpNetwork(format!("[BUG] failed to parse the generated IP address `{}` as IPv4", ipv6_addr.trim_start_matches(':'))))

Ah, it is a bug in the latest release 1.0.2.
Fixed in main, but they have not rolled a new release out that contains the fix.

jmpesp · 2024-12-20T21:02:30Z

upstairs/src/volume.rs

+                    block_size,
+                    blocks_per_extent,
+                    extent_count,
+                    opts: replacement_opts.clone(),


This too will have multiple subvolume targets that are the same

jmpesp · 2024-12-20T21:03:24Z

upstairs/src/volume.rs

        // 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);


maybe generic_crucible_opts should also randomize the addresses?

leftwo requested review from jmpesp and mkeeter November 5, 2024 21:42