fix(rollback): Ensure that rollback stages are injected correctly (#2072

) 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.
spinnaker · Mar 22, 2018 · 18a9b26 · 18a9b26
1 parent b9b786e
commit 18a9b26
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 7 deletions.
diff --git a/...com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStage.groovy b/...com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStage.groovy
@@ -106,7 +106,6 @@ class CreateServerGroupStage extends AbstractDeployStrategyStage {
     }
 
     if (strategySupportsRollback) {
-      // TODO: should this append after any existing stages in the graph?
       graph.add {
         it.type = rollbackClusterStage.type
         it.name = "Rollback ${stageData.cluster}"

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/graph/StageGraphBuilder.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/graph/StageGraphBuilder.java
@@ -148,11 +148,18 @@ public void connect(@Nonnull Stage previous, @Nonnull Stage next) {
   }
 
   private String generateRefId() {
+    long offset = parent
+      .getExecution()
+      .getStages()
+      .stream()
+      .filter(i -> parent.getId().equals(i.getParentStageId()) && type == i.getSyntheticStageOwner())
+      .count();
+
     return format(
       "%s%s%d",
       parent.getRefId(),
       type == STAGE_BEFORE ? "<" : ">",
-      graph.nodes().size()
+      offset + graph.nodes().size()
     );
   }
 

diff --git a/orca-queue-tck/src/main/kotlin/com/netflix/spinnaker/orca/q/QueueIntegrationTest.kt b/orca-queue-tck/src/main/kotlin/com/netflix/spinnaker/orca/q/QueueIntegrationTest.kt
@@ -40,6 +40,7 @@ import com.netflix.spinnaker.orca.pipeline.TaskNode.Builder
 import com.netflix.spinnaker.orca.pipeline.graph.StageGraphBuilder
 import com.netflix.spinnaker.orca.pipeline.model.Execution.ExecutionType.PIPELINE
 import com.netflix.spinnaker.orca.pipeline.model.Stage
+import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner
 import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner.STAGE_BEFORE
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository
 import com.netflix.spinnaker.orca.pipeline.persistence.jedis.JedisConfiguration
@@ -733,6 +734,13 @@ class QueueIntegrationTest {
       stage {
         refId = "1"
         type = "syntheticFailure"
+
+        stage {
+          refId = "1>1"
+          syntheticStageOwner = SyntheticStageOwner.STAGE_AFTER
+          type = "dummy"
+          status = SUCCEEDED
+        }
       }
     }
     repository.store(pipeline)
@@ -753,10 +761,12 @@ class QueueIntegrationTest {
       assertSoftly {
         assertThat(status).isEqualTo(TERMINAL)
         assertThat(stageByRef("1").status).isEqualTo(TERMINAL)
-        assertThat(stageByRef("1>1").name).isEqualTo("onFailure1")
-        assertThat(stageByRef("1>1").status).isEqualTo(SUCCEEDED)
-        assertThat(stageByRef("1>2").name).isEqualTo("onFailure2")
+        assertThat(stageByRef("1>2").name).isEqualTo("onFailure1")
         assertThat(stageByRef("1>2").status).isEqualTo(SUCCEEDED)
+
+        assertThat(stageByRef("1>3").requisiteStageRefIds).isEqualTo(setOf("1>2"))
+        assertThat(stageByRef("1>3").name).isEqualTo("onFailure2")
+        assertThat(stageByRef("1>3").status).isEqualTo(SUCCEEDED)
       }
     }
   }
@@ -814,12 +824,13 @@ class TestConfig {
     }
 
     override fun onFailureStages(stage: Stage, graph: StageGraphBuilder) {
-      val stage1 = graph.add {
+      graph.add {
         it.type = "dummy"
         it.name = "onFailure1"
         it.context = stage.context
       }
-      graph.connect(stage1) {
+
+      graph.add {
         it.type = "dummy"
         it.name = "onFailure2"
         it.context = stage.context

diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/CompleteStageHandler.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/CompleteStageHandler.kt
@@ -168,7 +168,14 @@ class CompleteStageHandler(
 
     val graph = StageGraphBuilder.afterStages(this)
     builder().onFailureStages(this, graph)
+
     val onFailureStages = graph.build().toList()
+    onFailureStages.forEachIndexed { index, stage ->
+      if (index > 0) {
+        // all on failure stages should be run linearly
+        graph.connect(onFailureStages.get(index - 1), stage)
+      }
+    }
 
     val alreadyPlanned = onFailureStages.any { previouslyPlannedAfterStageNames.contains(it.name) }