-
Notifications
You must be signed in to change notification settings - Fork 40
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
Handle region snapshot replacement volume deletes #7046
Merged
jmpesp
merged 31 commits into
oxidecomputer:main
from
jmpesp:region_snapshot_replacement_account_for_deleted_volumes
Dec 19, 2024
Merged
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
92da0a9
Handle region snapshot replacement volume deletes
jmpesp bee3cca
slow CI machines may not have started the sagas after running backgro…
jmpesp 309742d
THIS WAS A TREMENDOUS OVERSIGHT
jmpesp c9455ec
emit VolumeReplaceResult as saga node data
jmpesp 4630e03
fix a bunch of tests that were locking non-existent volumes
jmpesp 4c729e6
fix a bunch more tests that were locking non-existent volumes
jmpesp 1c50f37
fix another test that was locking non-existent volumes
jmpesp 63cb56e
the TREMENDOUS oversight continues
jmpesp 0ce2029
Merge branch 'main' into region_snapshot_replacement_account_for_dele…
jmpesp 79f8ad9
fix after merge
jmpesp 8b0a677
account for relocking a volume
jmpesp b2a740f
add comment about validating volume exists
jmpesp 18efdd3
disambiguate between soft and hard delete
jmpesp 640e678
cover the case that the region snapshot finish saga kicks off in the …
jmpesp 1a60b15
wait for state to transition to complete
jmpesp 9fc46ab
just an optimization
jmpesp 2cd7881
add missing unwind edge
jmpesp a78cabf
remove likely
jmpesp c869cad
handle if region snapshot is hard deleted while replacement request i…
jmpesp 231764d
Merge branch 'main' into region_snapshot_replacement_account_for_dele…
jmpesp 7b93e2e
fmt
jmpesp cb1c2fe
fix compile time errors from merge
jmpesp fdf800a
conflicts are not errors!
jmpesp abf522e
add dummy region snapshot
jmpesp 715c9f7
fmt
jmpesp e4d0844
comment for volume_repair_insert_in_txn
jmpesp 518a86c
cargo fmt missed this one!
jmpesp ec9bb9a
when completing a region snapshot replacement from the start background
jmpesp 34db989
address the unfinished comment
jmpesp 4931f58
rework wait_for_request_state to be clearer
jmpesp 2a37e9c
mroe -> more!
jmpesp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ impl_enum_type!( | |
ReplacementDone => b"replacement_done" | ||
DeletingOldVolume => b"deleting_old_volume" | ||
Running => b"running" | ||
Completing => b"completing" | ||
Complete => b"complete" | ||
); | ||
|
||
|
@@ -43,6 +44,7 @@ impl std::str::FromStr for RegionSnapshotReplacementState { | |
Ok(RegionSnapshotReplacementState::DeletingOldVolume) | ||
} | ||
"running" => Ok(RegionSnapshotReplacementState::Running), | ||
"completing" => Ok(RegionSnapshotReplacementState::Completing), | ||
"complete" => Ok(RegionSnapshotReplacementState::Complete), | ||
_ => Err(format!("unrecognized value {} for enum", s)), | ||
} | ||
|
@@ -77,8 +79,13 @@ impl std::str::FromStr for RegionSnapshotReplacementState { | |
/// v --- | ||
/// --- | ||
/// Running | | ||
/// | set in region snapshot replacement | ||
/// | | finish background task | ||
/// | | ||
/// | | | ||
/// v | | ||
/// | responsibility of region snapshot | ||
/// Completing | replacement finish saga | ||
/// | | ||
/// | | | ||
/// v | | ||
/// | | ||
/// Complete --- | ||
|
@@ -130,6 +137,12 @@ pub struct RegionSnapshotReplacement { | |
pub replacement_state: RegionSnapshotReplacementState, | ||
|
||
pub operating_saga_id: Option<Uuid>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not related to this PR, but a doc comment explaining the use of this field would be a welcome addition for slow-on-the-uptake readers like me :) |
||
|
||
/// In order for the newly created region not to be deleted inadvertently, | ||
/// an additional reference count bump is required. This volume should live | ||
/// as long as this request so that all necessary replacements can be | ||
/// completed. | ||
pub new_region_volume_id: Option<Uuid>, | ||
} | ||
|
||
impl RegionSnapshotReplacement { | ||
|
@@ -154,6 +167,7 @@ impl RegionSnapshotReplacement { | |
old_snapshot_id, | ||
old_snapshot_volume_id: None, | ||
new_region_id: None, | ||
new_region_volume_id: None, | ||
replacement_state: RegionSnapshotReplacementState::Requested, | ||
operating_saga_id: None, | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there's another version of this diagram (in the saga comments, I think) that has a back-edge from completing to running, though I think this is only if it unwinds, and I'm not sure if you're including those edges here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a nit, that's important! added in 2cd7881