Split starting running snapshots into two steps

[jmpesp]

When Saga nodes fail, the corresponding undo function is not executed. If a saga node can partially succeed before failing, the undo function has to be split into a separate saga step in order to properly unwind the created resources. This wasn't being done for running snapshots, so this commit corrects that and adds a test for it.

Fixes #3085

[davepacheco]

I'm a little confused by the pattern of having a node in the DAG with no action, just an undo, followed by a node with just an action, no undo. Usually, the undo action is supposed to go with the corresponding action. I gather doing that here didn't do the right thing because the action is neither succeeding nor failing, but partially completing. Would that be better split it into multiple actions that themselves would either succeed or fail? But maybe that's not possible here because the number of actions is runtime-dependent (depending on the number of allocated regions)?

I don't know that it causes a correctness problem here.

[jmpesp]

Usually, the undo action is supposed to go with the corresponding action. I gather doing that here didn't do the right thing because the action is neither succeeding nor failing, but partially completing. Would that be better split it into multiple actions that themselves would either succeed or fail? But maybe that's not possible here because the number of actions is runtime-dependent (depending on the number of allocated regions)?

Yeah, having the undo action go with the corresponding action is only valid when the saga node's action is atomic. If the saga node can partially succeed then this two step process is required to ensure that the unwind is executed. Of course, the unwind may also partially succeed, which is another problem entirely.

It would be better to split it into multiple actions depending on what is actually required (each with only one idempotent call to a corresponding crucible agent) but you're correct in that this is runtime-dependent, and the code that builds the saga DAG does not have access to the database and cannot make complicated runtime decisions. get_allocated_regions could return any number of regions. It's true that we currently do not stitch multiple subregions together or have complicated topologies, but in the future we probably will.

[davepacheco]

Makes sense. Thanks for that explanation. I wonder if in the future we might be able to fetch those regions when we create the DAG (which I think is usually a time we could hit the database, since it's usually happening in some API call), store them as a "constant" node or something like that, and then reference it later. Anyway, that seems beyond the scope of this change.