[nexus] Make instance creation actions/undo actions idempotent #2095
Conversation
| @@ -1004,7 +1023,7 @@ mod test { | |||
| name: DISK_NAME.parse().unwrap(), | |||
| }, | |||
| )], | |||
| start: true, | |||
| start: false, | |||
There was a problem hiding this comment.
I admittedly did flip this bit - otherwise, I need to poke the instance and update state to "stopped" before I can delete it anyway in the tests below.
| let params = new_test_params(&opctx, project_id); | ||
| let dag = create_saga_dag::<SagaInstanceCreate>(params).unwrap(); | ||
|
|
||
| // The "undo_node" should always be immediately preceding the |
There was a problem hiding this comment.
While perfectly reasonable with our current implementation, it makes me a tad uncomfortable that we are expecting a steno internal Dag structure to look a certain way for this test.
I'm not really sure the best way to handle this, except for perhaps first classing this type of injection in steno. I"m curious to hear @davepacheco's thoughts when he gets back, although I don't think we should hold up the PR for this.
There was a problem hiding this comment.
We aren't expecting anything of the internal DAG structure, other than "it's ordered". "Which node is which" is entirely up to the test -- the test is making the call "whenever we fail, the node preceding the failure node should have an undo action called twice to test idempotency".
We can arbitrarily change the saga DAG, and this test should be able to remain unchanged.
All nodes we're iterating over are either actions, constants, or subsagas, as discussed in oxidecomputer/steno#67 .
I'm not really sure the best way to handle this, except for perhaps first classing this type of injection in steno.
What would this mean to you? I thought the APIs I added to steno to iterate over the dag, expose indices which could be targets for injection, and add the ability to inject repetitions alongside the already-existing "error injection" counted as first-class support?
There was a problem hiding this comment.
We aren't expecting anything of the internal DAG structure, other than "it's ordered". "Which node is which" is entirely up to the test -- the test is making the call "whenever we fail, the node preceding the failure node should have an undo action called twice to test idempotency".
We can arbitrarily change the saga DAG, and this test should be able to remain unchanged.
Yup, thinking about this more, you are 100% correct.
What would this mean to you? I thought the APIs I added to steno to iterate over the dag, expose indices which could be targets for injection, and add the ability to inject repetitions alongside the already-existing "error injection" counted as first-class support?
I was overthinking this. What you did makes sense. My initial thought was something unnecessarily complicated. We'd inject errors then iterate again, reading the annotated error nodes and getting the prior nodes affected by the errors. However, as you point out that's unnecessary, since we know based on the dag that the node immediately prior to the error node will be undone.
Sorry for unnecessarily getting your hackles up.
There was a problem hiding this comment.
No worries! Just want to make sure I wasn't missing a cleaner solution - probing at what's going on is always appreciated
Part of #2094