Add API for injecting 'repetitions' into saga executor#88
Conversation
andrewjstone
left a comment
There was a problem hiding this comment.
This looks good to me and will be very helpful for testing! I like that you re-used the error injection pathway :)
| // | ||
| // This helper intends to reduce some of that boilerplate. | ||
| async fn saga_runner_helper( | ||
| repeat: Option<(NodeIndex, RepeatInjected)>, |
There was a problem hiding this comment.
I find the tests using the helper somewhat hard to read. The repeat and fail parameters are tagged with comments and it's not immediately clear what the counts represent. What if instead we passed a struct to the saga runner helper so we could have named parameters? This would obviate the need for comments and on top of that give us a way to tie the counts to the named parameters. We could optionally add names to the counts by separating them into node_redos and node_undos in the struct.
There was a problem hiding this comment.
Thanks @smklein. While, verbose, I think it makes the individual tests easier to read!
|
|
||
| #[derive(Debug, Copy, Clone)] | ||
| pub enum RepeatInjected { | ||
| Action, |
There was a problem hiding this comment.
I wonder if there would be any value in adding counts the values here to allow repeating more than once. While it shouldn't matter, it may be possible that executing Action, say 2 times, and Undo 3 times has a perverse effect.
There was a problem hiding this comment.
Done in d9a4de1 ; this forced the input parameters to change a bit, since I didn't want the caller to need to call for the "action" and "undo" counts separately (which would introduce contention on the node-as-key implementation).
- Makes use of oxidecomputer/steno#88 to test idempotency of instance creation actions + undo actions - Fixes all the ways in which it was *not* idempotent, including... - (Action) Inserting the same network interface failed with a VPC subnet error, instead of an "already exists" error. - (Action) Inserting the instance record twice fell victim to #1168 - (Undo) Deleting the instance record failed to execute twice, as it could not find the instance record for the second invocation. Part of #2094
- Makes use of oxidecomputer/steno#88 to test idempotency of instance deletion actions - Fixes the way in which it was not idempotent: deleting the instance record Part of #2094
|
Neat! This looks great. What happens with the saga log when an action is repeated? I think you want only the log entries from one of the repetitions in the log -- otherwise it seems like not a valid log. It'd be great to get coverage for this in the demo example like we have for injecting errors. I think this should be pretty easy, adding a block like this one: steno/examples/demo-provision.rs Lines 228 to 238 in 04d7c09 It should be easy to add a test too like this one: Lines 64 to 72 in 04d7c09 and you could make sure recovery works after using it with something like this: Line 105 in 04d7c09 |
- Makes use of oxidecomputer/steno#88 to test idempotency of disk creation actions - Fixes the way in which it was not idempotent: creating the disk record Part of #2094
- Makes use of oxidecomputer/steno#88 to test idempotency of disk deletion actions Part of #2094
Provides a simple API for instructing a node to "execute twice".
This provides a "bare-minimum" helper API for testing idempotency within a saga. When combined with #67 - which was used to test unwind safety - it should be possible to test that all actions / undo actions within a saga are idempotent, at least across being called twice.
Part of #31