fix(rollback): Ensure that rollback stages are injected correctly #2072
Conversation
|
|
There was a problem hiding this comment.
I'd like to update the existing regression test you added.
Specifically looking to reproduce the scenario where
(a) there are stages that previously SUCCEEDED and (b) more than one on failure stage.
(a) will validate that we're not duplicating a refId.
(b) will validate that we're correctly connecting stages.
| @@ -106,7 +106,6 @@ class CreateServerGroupStage extends AbstractDeployStrategyStage { | |||
| } | |||
|
|
|||
| if (strategySupportsRollback) { | |||
| // TODO: should this append after any existing stages in the graph? | |||
There was a problem hiding this comment.
The answer was yes ... I didn't see anything on StageGraphBuilder that would let you connect() to the last added node.
There was a problem hiding this comment.
Yeah, I think StageGraphBuilder could be a little easier to use for certain common scenarios.
| @@ -148,11 +148,18 @@ public void connect(@Nonnull Stage previous, @Nonnull Stage next) { | |||
| } | |||
|
|
|||
| private String generateRefId() { | |||
| long offset = parent | |||
There was a problem hiding this comment.
If we're adding stages to an already built execution, it's important to not re-use a refId.
| .getExecution() | ||
| .getStages() | ||
| .stream() | ||
| .filter(i -> parent.getId().equals(i.getParentStageId()) && type == i.getSyntheticStageOwner()) |
There was a problem hiding this comment.
not necessary the most elegant, alternatives?
There was a problem hiding this comment.
Seems reasonable. I can't think of a neater way off the top of my head.
| onFailureStages.forEachIndexed { index, stage -> | ||
| if (index > 0) { | ||
| // all on failure stages should be run linearly | ||
| graph.connect(onFailureStages.get(index - 1), stage) |
There was a problem hiding this comment.
Without this, we run the risk of one of the stages completing and notifying it's parent ... and completing the execution with another stage still running in parallel.
My expectation is that > 1 onFailure stages will run linearly (they did previously).
There was a problem hiding this comment.
I'd like to address the root issue as well (not in this PR). The execution should not complete if anything is still in a RUNNING state and if it does that's a bug.
ajordens
force-pushed
the
fix-deploy-rollbacks
branch
2 times, most recently
from
9b86d29
to
be51c8e
Compare
This PR handles generating a `refId` for a synthetic stage that has pre-existing siblings. In such cases, the `refId` should be unique and offset by the number of siblings. All `onFailureStages()` should be run linearly otherwise you run the risk of a parent stage being notified prematurely.
ajordens
force-pushed
the
fix-deploy-rollbacks
branch
from
be51c8e
to
c97d0b0
Compare
| .getExecution() | ||
| .getStages() | ||
| .stream() | ||
| .filter(i -> parent.getId().equals(i.getParentStageId()) && type == i.getSyntheticStageOwner()) |
There was a problem hiding this comment.
Seems reasonable. I can't think of a neater way off the top of my head.
| onFailureStages.forEachIndexed { index, stage -> | ||
| if (index > 0) { | ||
| // all on failure stages should be run linearly | ||
| graph.connect(onFailureStages.get(index - 1), stage) |
There was a problem hiding this comment.
I'd like to address the root issue as well (not in this PR). The execution should not complete if anything is still in a RUNNING state and if it does that's a bug.
This PR handles generating a
refIdfor a synthetic stage that haspre-existing siblings.
In such cases, the
refIdshould be unique and offset by the number ofsiblings.
All
onFailureStages()should be run linearly otherwise you run therisk of a parent stage being notified prematurely.