Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make crutest know extent info for each sub volume. #1474

Closed
wants to merge 5 commits into from

Conversation

leftwo
Copy link
Contributor

@leftwo leftwo commented Sep 26, 2024

Updates for crutest to know more about the sub-volumes.

Replaced BlockIO query_extent_size() with query_extent_info(). This now returns a Vec instead of a Block.

ExtentInfo is:

    pub extent_size: Block,
    pub extent_count: u32,

Having this query will enable more test coverage, and allow at test to know the configuration of all the sub_volumes that make up a volume.

Added a function to VolumeConstructionRequest to copy out all the targets for all the sub-volumes.

Crutest updates:
Use the new query_extent_info() to build a RegionInfo struct that captures each of the sub_volumes info.

New "pleasant summary" output for RegionInfo that shows all the information from all sub-volumes.:

----------------------------------------------
random write with 4 KiB chunks (1 block)
region info:
  block size:                   4096 bytes
  sub_volume 0 blocks / extent: 100
  sub_volume 0 extent size:     400 KiB
  sub_volume 0 extent count:    15
  sub_volume 1 blocks / extent: 100
  sub_volume 1 extent size:     400 KiB
  sub_volume 1 extent count:    15
  sub_volume 2 blocks / extent: 100
  sub_volume 2 extent size:     400 KiB
  sub_volume 2 extent count:    15
  total blocks:                 4500
  total size:                   17.6 MiB
  encryption:                   no
----------------------------------------------

Use the new pleasant summary output elsewhere in crutest.
Updated sparse_fill_workload to handle multiple sub-volumes.
Updated span_workload to operate on each sub-volume.
Updated crutest tests ReplaceBeforeActive and ReplaceReconcile to use the global dsc option and not require their own unique option.

Add FillSparse command to crutest cli

Updated test_replace_special.sh to use dsc instead of targets.
Updated test_live_repair.sh to use dsc instead of targets.
Updated most of test_up.sh to use dsc instead of targets, though there are a few places where we are not free of them yet. Also hammer test will need updates to support dsc instead of targets.

leftwo and others added 5 commits September 26, 2024 16:42
Replaced BlockIO query_extent_size() with query_extent_info().  This now
returns a Vec<ExtentInfo> instead of a Block.

ExtentInfo is:
    pub extent_size: Block,
    pub extent_count: u32,

Having this query will enable more test coverage, and allow at test to know
the configuration of all the sub_volumes that make up a volume.

Added a function to VolumeConstructionRequest to copy out all the targets
for all the sub-volumes.

Crutest:
Use the new query_extent_info() to build a RegionInfo struct that captures
each of the sub_volumes info.

New "pleasant summary" output for RegionInfo that shows all the information
from all sub-volumes.:

```
----------------------------------------------
random write with 4 KiB chunks (1 block)
region info:
  block size:                   4096 bytes
  sub_volume 0 blocks / extent: 100
  sub_volume 0 extent size:     400 KiB
  sub_volume 0 extent count:    15
  sub_volume 1 blocks / extent: 100
  sub_volume 1 extent size:     400 KiB
  sub_volume 1 extent count:    15
  sub_volume 2 blocks / extent: 100
  sub_volume 2 extent size:     400 KiB
  sub_volume 2 extent count:    15
  total blocks:                 4500
  total size:                   17.6 MiB
  encryption:                   no
----------------------------------------------
```

Use the new pleasant summary output elsewhere in crutest.

Updated sparse_fill_workload to handle multiple sub-volumes.
Updated span_workload to operate on each sub-volume.

Updated crutest tests ReplaceBeforeActive and ReplaceReconcile to use the
global dsc option and not require their own unique option.

Add FillSparse command to crutest cli

Updated test_replace_special.sh to use dsc instead of targets.
Updated test_live_repair.sh to use dsc instead of targets.
Updated most of test_up.sh to use dsc instead of targets, though there are
a few places where we are not free of them yet.  Also hammer test will
need updates to support dsc instead of targets.
@leftwo leftwo requested review from mkeeter, jmpesp and bnaecker and removed request for bnaecker September 27, 2024 05:06
@leftwo leftwo marked this pull request as ready for review September 27, 2024 05:06
pub async fn query_extent_size(&self) -> Result<Block, CrucibleError> {
self.send_and_wait(|done| BlockOp::QueryExtentSize { done })
.await
pub async fn query_extent_info(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this function implemented under both impl Guest and impl BlockIO for Guest? We should be able to just implement it for the latter case (then import the BlockIO trait if we need to call it).

(this is a pre-existing condition, but might as well clean it up since we're touching it here)

Ok(ei) => {
for sv_ei in ei.iter() {
extent_info.push(*sv_ei);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a pre-existing condition, but why is it acceptable to skip subvolumes if they return an error here? It seems like we should return an error.

@@ -406,6 +406,13 @@ impl Default for RegionOptions {
}
}

#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
/// Info about extents in a region.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This data structure gives me bad vibes for a few reasons.

  • I have a pre-existing dislike for Block, because its value field is ambiguous and used in different ways
  • It looks like we're usually returning a Vec<ExtentInfo>, in which all of the extent_size.value must be identical (we don't support volumes with mixed block sizes). However, this isn't prevented by the data structure!
  • Pedantically, this isn't info about an extent; it's info about a region / volume / subvolume
  • We already have crucible_client_types::RegionExtentInfo, which contains the same data as this struct

If we want an unambiguous data structure, we could do something like the following:

pub struct VolumeInfo {
    pub block_size: u64,
    pub volumes: Vec<SubVolumeInfo>,
}

struct SubVolumeInfo {
    pub blocks_per_extent: u64,
    pub extent_count: u32,
}

Using this data structure, we're promising that we've checked and all of the block sizes are the same; we would need to add that check to Volume::query_extent_info. Volumes without any subvolumes could still return the top-level VolumeInfo with a single value in volumes (equivalen to return a single-item Vec<ExtentInfo>).

Comment on lines +43 to +74
pub fn targets(&self) -> Vec<SocketAddr> {
let mut targets = Vec::new();
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => {
for subreq in sub_volumes {
let new_targets = subreq.targets();
for nt in new_targets {
targets.push(nt);
}
}
}
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => {
for nt in &opts.target {
targets.push(*nt);
}
}
_ => {}
}
targets
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to manually accumulate into the Vec<SocketAddr>:

Suggested change
pub fn targets(&self) -> Vec<SocketAddr> {
let mut targets = Vec::new();
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => {
for subreq in sub_volumes {
let new_targets = subreq.targets();
for nt in new_targets {
targets.push(nt);
}
}
}
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => {
for nt in &opts.target {
targets.push(*nt);
}
}
_ => {}
}
targets
}
}
pub fn targets(&self) -> Vec<SocketAddr> {
match self {
VolumeConstructionRequest::Volume {
id: _,
block_size: _,
sub_volumes,
read_only_parent: _,
} => sub_volumes.iter().flat_map(|s| s.targets()).collect(),
VolumeConstructionRequest::Region {
block_size: _,
blocks_per_extent: _,
extent_count: _,
opts,
gen: _,
} => opts.target.clone(),
_ => vec![],
}
}

@@ -883,13 +905,10 @@ async fn process_cli_command<T: BlockIO + Send + Sync + 'static>(
Err(e) => fw.send(CliMessage::Error(e)).await,
},
CliMessage::InfoPlease => {
let new_ri = get_region_info(block_io).await;
let new_ri = get_region_info(block_io, false).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know is_encrypted is false here?

total_size,
total_blocks,
write_log,
max_block_io,
})
};
print_region_description(&ri, is_encrypted);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird to print the region description as a side effect here (and this is the only part of the function which uses is_encrypted); could we make print_region_description the callers' responsibility?

@@ -902,6 +888,7 @@ async fn make_a_volume(
bail!("Can't determine extent info to build a Volume");
}
};
let targets = crucible_opts.target.clone().into_iter().collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let targets = crucible_opts.target.clone().into_iter().collect();
let targets = crucible_opts.target.clone();

@@ -1045,7 +1035,7 @@ async fn main() -> Result<()> {
* Build the region info struct that all the tests will use.
* This includes importing and verifying from a write log, if requested.
*/
let mut region_info = match get_region_info(&block_io).await {
let mut region_info = match get_region_info(&block_io, is_encrypted).await {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but this could be written as

    let mut region_info = get_region_info(&block_io, is_encrypted)
        .await
        .context("failed to get region info")?;

(instead of a match; requires importing anyhow::Context)

// If we have more than one sub-volume, this performance test will
// not compute extent size properly.
if ri.sub_volume_info.len() > 1 {
println!("WARNING: Multiple Sub_Volumes seen in perf test");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bail! instead of logging?

@leftwo
Copy link
Contributor Author

leftwo commented Sep 27, 2024

Okay, so after some side band discussions, I'm tabling this work in favor of another route to the same outcome.

Instead of leaking more details about the underlying structure of things through BlockIO, it makes
better sense to just make crutest commit to Volume.
Then, volume details (sub-volume, extent counts, extent sizes) can be specific Volume things.

I'm moving this back to WIP, but will probalby discard it after I've taken pieces into another PR where
I'll end up at the same place, but with a different path to get there.

@leftwo leftwo removed the request for review from jmpesp September 27, 2024 15:57
@leftwo leftwo marked this pull request as draft September 27, 2024 15:57
@leftwo
Copy link
Contributor Author

leftwo commented Oct 16, 2024

This work was scattered across a few other PRs, but the largest part ended up here: #1508

I'm closing this one now, as 1508 takes its place. Comments and suggestions that were not applied elsewhere and still were relevant were taken over in 1508

@leftwo leftwo closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants