Make size info available during volume creation without requiring activation#557
Make size info available during volume creation without requiring activation#557
Conversation
… to be queried at any time
leftwo
left a comment
leftwo
left a comment
There was a problem hiding this comment.
You've done some good stuff here, and in a much needed area.
I'm trying to break down where the line should be between CrucibleOpts and RegionExtentInfo as well as getting enough working here that you can go back to migration.
However, after typing and re-typing various ideas, I think what you have here is
probably the right place to draw the line between what you need to move forward right
now that won't require major overhaul of Crucible inputs, and also won't be
creating work we would undo later.
upstairs/src/lib.rs
Outdated
| "Block size not available (active: {})", | ||
| up.guest_io_ready().await | ||
| ); | ||
| req.send_err(CrucibleError::UpstairsInactive).await; |
There was a problem hiding this comment.
Let's return some different error here. Maybe just generic error. It's probably UpstairsInactive, but unless we check it, we don't know for sure.
Same for the two other places where we return error below.
upstairs/src/lib.rs
Outdated
| pub async fn up_main( | ||
| opt: CrucibleOpts, | ||
| gen: u64, | ||
| expected_region_def: Option<RegionDefinition>, |
There was a problem hiding this comment.
Small nit: We could probably just call this region_def.
Yes, it is expected, but so is the gen above, and we don't call that expected_gen.
|
I've finally managed to test this with the corresponding Propolis/Omicron changes and things look good so far (Propolis tests pass, Omicron instance starts up). I've pushed the drafts to Github: Propolis, Omicron. These are (IMO) really straightforward changes that suggest that we're taking the right approach here. I'll start incorporating feedback here later today. |
leftwo
left a comment
leftwo
left a comment
There was a problem hiding this comment.
Merge away. Thanks for you work here.
|
Thanks for reviewing! |
2 participants
Allow a
Volumeto have a valid block size and total size from its creation by requiring users ofVolumeConstructionRequest::Regionto pass extent sizes and counts in addition to block sizes. This allows callers to create aVolumeand query its sizes before anyone has activated, which in turn allows Propolis to defer volume activation until late in migration, solving the problems outlined in #440 and #543. Callers who want to create an upstairs without a VolumeConstructionRequest (e.g. tools likecruddandhammer) can still do so (they will get the old "accept information from the first downstairs" behavior).NOTE: Because this changes
VolumeConstructionRequest's definition, this PR is a breaking change for Propolis/Omicron. I have a draft of the Propolis changes needed to adopt the new struct format, but still need to do the corresponding Omicron changes. I'm happy to wait to merge until the Omicron bits are also ready--just let me know (I wanted to go ahead and start getting this reviewed regardless).Open issues to review:
REVIEW(gjc)comment in upstairs/src/lib.rs in a spot where this was important (it affects what sort of checks we can do when processing incoming region definitions).Tests:
cargo testFixes #440. Fixes #543.