Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Instance start saga's undo steps refer to nonexistent "instance_id" node #3894

Closed
gjcolombo opened this issue Aug 17, 2023 · 2 comments · Fixed by #3895
Closed

Instance start saga's undo steps refer to nonexistent "instance_id" node #3894

gjcolombo opened this issue Aug 17, 2023 · 2 comments · Fixed by #3895
Assignees
@gjcolombo
Labels
nexus Related to nexus

Comments

@gjcolombo
Copy link
Contributor

Seen in dogfood. sis_v2p_ensure_undo and sis_ensure_registered_undo both try to get an instance ID from a sagactx.lookup that refers to a nonexistent node. They should get the ID from the saga params instead like all the other nodes.

The fix itself is very simple; a more interesting problem is why the action unwind test for this saga didn't catch the problem. Investigating that now.
@gjcolombo gjcolombo added the nexus Related to nexus label Aug 17, 2023
@gjcolombo gjcolombo self-assigned this Aug 17, 2023
@gjcolombo
Copy link
Contributor Author

The fix itself is very simple; a more interesting problem is why the action unwind test for this saga didn't catch the problem. Investigating that now.

You might think that, as the author of #3309, I would have been able to avoid recreating a similar problem in the start saga. If so, you (and I) thought incorrectly: the bug here is that the start saga unwind test is failing ever to get past the first node and so is not actually exercising the undo steps for later nodes. This points to a bug in the "set instance to starting" logic or its undo step that I'm now investigating.

@gjcolombo
Copy link
Contributor Author

This points to a bug in the "set instance to starting" logic or its undo step that I'm now investigating.

The problem is actually a test issue: prior iterations of the saga are changing the parameters that need to be passed to subsequent iterations, but the test isn't regenerating those parameters. (IOW, new test iterations aren't starting from a clean slate the way a new API call to the start endpoint would.) After fixing this issue I can reproduce the actual bug and am now testing the fix.

This was referenced Aug 17, 2023
Merged
Closed
gjcolombo added a commit that referenced this issue Aug 18, 2023
@gjcolombo
Get instance ID from parameters in start saga undo (#3895)
41839b5 
The instance start saga has two undo actions that try to read an
instance ID from a nonexistent prior action (I incorrectly copied them
from the instance create saga). The saga unwind test missed this because
it erroneously checked only that its saga invocations were failing and
not that they were failing at the correct step.

Fix the test so that it can reproduce this issue, then make the undo
nodes read the saga IDs from the correct place.

Tested: cargo test, both with the test fix and no code fix (fails as
expected) and with both fixes (passes as expected).

Fixes #3894.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gjcolombo gjcolombo

Labels
nexus Related to nexus
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Get instance ID from parameters in start saga undo oxidecomputer/omicron
1 participant
@gjcolombo