Return richer enum types from datastore functions #6604
Conversation
Create new enum types and return those to give more information to callers: - `create_region_snapshot_replacement_step` and `insert_region_snapshot_replacement_step` now return `InsertRegionSnapshotReplacementStepResult` - `volume_replace_region` and `volume_replace_snapshot` now return `VolumeReplaceResult` Notably, `VolumeReplaceResult::ExistingVolumeDeleted` replaces the previous error type `TargetVolumeDeleted`, and is not an error, allowing the caller to take action of the existing volume was deleted. This commit was peeled off work in progress to address oxidecomputer#6353.
| @@ -120,6 +120,7 @@ pub use rack::RackInit; | |||
| pub use rack::SledUnderlayAllocationResult; | |||
| pub use region::RegionAllocationFor; | |||
| pub use region::RegionAllocationParameters; | |||
| pub use region_snapshot_replacement::InsertRegionSnapshotReplacementStepResult; | |||
There was a problem hiding this comment.
nitpicky, take it or leave it: if this wasn't pub used and we instead made the region_snapshot_replacement module public, then this could just be region_snapshot_replacement::InsertStepResult or something and code that depends on it could import it to avoid having to type out the whole thing, or not do that if it needs to be disambiguated?
not a big deal either way though.
There was a problem hiding this comment.
I like this, done in d2135f2
| use async_bb8_diesel::AsyncConnection; | ||
| use async_bb8_diesel::AsyncRunQueryDsl; | ||
| use diesel::prelude::*; | ||
| use omicron_common::api::external::Error; | ||
| use uuid::Uuid; | ||
|
|
||
| #[must_use] | ||
| #[derive(Debug, PartialEq)] |
There was a problem hiding this comment.
any reason to not also derive Eq?
| existing_step_id, | ||
| } = result | ||
| else { | ||
| panic!("wrong return type"); |
There was a problem hiding this comment.
could we have this panic include result so that if we hit it, the test's failure output includes the unexpected value?
There was a problem hiding this comment.
good idea, done in d2135f2
| existing_step_id, | ||
| } = result | ||
| else { | ||
| panic!("wrong return type"); |
There was a problem hiding this comment.
similarly, might be good for this panic to show what the wrong thing was?
| let InsertRegionSnapshotReplacementStepResult::Inserted { step_id } = | ||
| result | ||
| else { | ||
| panic!("wrong return type"); |
| Err(ActionError::action_failed(format!( | ||
| "existing volume {} deleted", | ||
| old_volume_id | ||
| ))) |
There was a problem hiding this comment.
should this be Error::conflict? i suppose it doesn't really matter since these sagas are triggered by a background task, right?
There was a problem hiding this comment.
even so, it should be a conflict, yeah: done in d2135f2
| @@ -361,20 +364,40 @@ async fn rsrss_replace_snapshot_in_volume( | |||
| .await | |||
| .map_err(ActionError::action_failed)?; | |||
|
|
|||
| Ok(()) | |||
| info!( | |||
There was a problem hiding this comment.
i wonder whether this should be info or debug?
it might also be nice to include the volume ID and stuff here, unless that's in the saga context?
There was a problem hiding this comment.
changed to debug in d2135f2, and yeah, same answer - it's even more so in the context for this saga because the entire replace params is.
| @@ -554,7 +555,30 @@ async fn srrs_replace_region_in_volume( | |||
| .await | |||
| .map_err(ActionError::action_failed)?; | |||
|
|
|||
| Ok(()) | |||
| info!(log, "replacement returned {:?}", volume_replace_region_result); | |||
There was a problem hiding this comment.
i wonder whether this should be info or debug?
it might also be nice to include the volume ID, region ID, and stuff here, unless that's in the saga context?
There was a problem hiding this comment.
changed to debug in d2135f2
yeah, all that context should be in the saga context
Create new enum types and return those to give more information to callers: - `create_region_snapshot_replacement_step` and `insert_region_snapshot_replacement_step` now return `InsertRegionSnapshotReplacementStepResult` - `volume_replace_region` and `volume_replace_snapshot` now return `VolumeReplaceResult` Notably, `VolumeReplaceResult::ExistingVolumeDeleted` replaces the previous error type `TargetVolumeDeleted`, and is not an error, allowing the caller to take action of the existing volume was deleted. This commit was peeled off work in progress to address #6353.
Create new enum types and return those to give more information to callers:
create_region_snapshot_replacement_stepandinsert_region_snapshot_replacement_stepnow returnInsertRegionSnapshotReplacementStepResultvolume_replace_regionandvolume_replace_snapshotnow returnVolumeReplaceResultNotably,
VolumeReplaceResult::ExistingVolumeDeletedreplaces the previous error typeTargetVolumeDeleted, and is not an error, allowing the caller to take action of the existing volume was deleted.This commit was peeled off work in progress to address #6353.