fix(rollback): Better support for automatic rollbacks on RRB deploys (#…

…2043) This PR will correctly unpin the source server group when an RRB deployment fails and is automatically rolled back. The previous server group is guaranteed to exist so the automatic rollback _only_ needs to disable the new server group.
spinnaker · Mar 8, 2018 · 7d2e4d6 · 7d2e4d6
1 parent e9afb1a
commit 7d2e4d6
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 14 deletions.
diff --git a/.../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy b/.../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy
@@ -22,6 +22,7 @@ import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.CaptureSour
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.EnableServerGroupStage
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.ResizeServerGroupStage
+import com.netflix.spinnaker.orca.kato.pipeline.strategy.Strategy
 import com.netflix.spinnaker.orca.kato.pipeline.support.ResizeStrategy
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner
@@ -54,9 +55,25 @@ class ExplicitRollback implements Rollback {
   ApplySourceServerGroupCapacityStage applySourceServerGroupCapacityStage
 
   @JsonIgnore
-  def List<Stage> buildStages(Stage parentStage) {
-    def stages = []
+  @Override
+  List<Stage> buildStages(Stage parentStage) {
+    Map disableServerGroupContext = new HashMap(parentStage.context)
+    disableServerGroupContext.serverGroupName = rollbackServerGroupName
+    def disableServerGroupStage = newStage(
+      parentStage.execution, disableServerGroupStage.type, "disable", disableServerGroupContext, parentStage, SyntheticStageOwner.STAGE_AFTER
+    )
 
+    def parentDeployStage = getParentDeployStage(parentStage)
+    def parentDeployStageStrategy = parentDeployStage?.context?.strategy as String
+    if (Strategy.fromStrategy(parentDeployStageStrategy) == Strategy.ROLLING_RED_BLACK) {
+      // no need to do anything but disable the newly deployed (and failing!) server group when dealing with a
+      // rolling red/black deployment
+      return [
+        disableServerGroupStage
+      ]
+    }
+
+    def stages = []
     Map enableServerGroupContext = new HashMap(parentStage.context)
     enableServerGroupContext.targetHealthyDeployPercentage = targetHealthyRollbackPercentage
     enableServerGroupContext.serverGroupName = restoreServerGroupName
@@ -84,12 +101,7 @@ class ExplicitRollback implements Rollback {
       parentStage.execution, resizeServerGroupStage.type, "resize", resizeServerGroupContext, parentStage, SyntheticStageOwner.STAGE_AFTER
     )
 
-    Map disableServerGroupContext = new HashMap(parentStage.context)
-    disableServerGroupContext.serverGroupName = rollbackServerGroupName
-    stages << newStage(
-      parentStage.execution, disableServerGroupStage.type, "disable", disableServerGroupContext, parentStage, SyntheticStageOwner.STAGE_AFTER
-    )
-
+    stages << disableServerGroupStage
     stages << buildApplySourceServerGroupCapacityStage(parentStage, parentStage.mapTo(ResizeStrategy.Source))
     return stages
   }
@@ -138,4 +150,12 @@ class ExplicitRollback implements Rollback {
       SyntheticStageOwner.STAGE_AFTER
     )
   }
+
+  static Stage getParentDeployStage(Stage parentStage) {
+    while (parentStage && !parentStage.context.containsKey("strategy")) {
+      parentStage = parentStage.parent
+    }
+
+    return parentStage
+  }
 }
diff --git a/...ouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/pipeline/strategy/Strategy.java b/...ouddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/pipeline/strategy/Strategy.java
@@ -23,6 +23,7 @@
 
 public enum Strategy implements StrategyFlowComposer{
   RED_BLACK("redblack"),
+  ROLLING_RED_BLACK("rollingredblack"),
   HIGHLANDER("highlander"),
   ROLLING_PUSH("rollingpush"),
   CUSTOM("custom"),
@@ -34,7 +35,7 @@ public enum Strategy implements StrategyFlowComposer{
     this.key = key;
   }
 
-  static Strategy fromStrategy(String key) {
+  public static Strategy fromStrategy(String key) {
     if (key == null) {
       return NONE;
     }

diff --git a/orca-queue-tck/src/main/kotlin/com/netflix/spinnaker/orca/q/Stages.kt b/orca-queue-tck/src/main/kotlin/com/netflix/spinnaker/orca/q/Stages.kt
@@ -64,7 +64,8 @@ val stageWithSyntheticOnFailure = object : StageDefinitionBuilder {
   }
 
   override fun onFailureStages(stage: Stage) = listOf(
-    newStage(stage.execution, singleTaskStage.type, "onFailure1", stage.context, stage, STAGE_AFTER)
+    newStage(stage.execution, singleTaskStage.type, "onFailure1", stage.context, stage, STAGE_AFTER),
+    newStage(stage.execution, singleTaskStage.type, "onFailure2", stage.context, stage, STAGE_AFTER)
   )
 }
 

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
@@ -165,8 +165,15 @@ class CompleteStageHandler(
       }
 
       val notStartedSynthetics = execution.stages.filter { it.parentStageId == id && it.status == NOT_STARTED}
+      val notStartedSyntheticRefIds = notStartedSynthetics.map { it.refId }
 
       builder.buildAfterStages(this, onFailureStages) { it: Stage ->
+        // Avoid having a NOT_STARTED synthetic stage as a prerequisite as it will be subsequently removed
+        // from the execution.
+        it.requisiteStageRefIds = it.requisiteStageRefIds.filter {
+          !notStartedSyntheticRefIds.contains(it)
+        }
+
         repository.addStage(it)
       }
 

diff --git a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/CompleteStageHandlerTest.kt b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/CompleteStageHandlerTest.kt
@@ -932,19 +932,25 @@ object CompleteStageHandlerTest : SubjectSpek<CompleteStageHandler>({
       afterGroup(::resetMocks)
 
       it("plans the first 'OnFailure' stage") {
-        val onFailureStage = pipeline.stages.first { it.name.equals("onFailure1") }
-        verify(queue).push(StartStage(onFailureStage))
+        val onFailure1Stage = pipeline.stages.first { it.name.equals("onFailure1") }
+        assertThat(onFailure1Stage.requisiteStageRefIds).isEmpty()
+
+        val onFailure2Stage = pipeline.stages.first { it.name.equals("onFailure2") }
+        assertThat(onFailure2Stage.requisiteStageRefIds).isEqualTo(setOf(onFailure1Stage.refId))
+
+        verify(queue).push(StartStage(onFailure1Stage))
       }
 
       on("receiving the message again") {
-        val onFailureStage = pipeline.stages.first { it.name.equals("onFailure1") }
-        onFailureStage.status = ExecutionStatus.SUCCEEDED
+        val onFailure1Stage = pipeline.stages.first { it.name.equals("onFailure1") }
+        onFailure1Stage.status = ExecutionStatus.SUCCEEDED
         subject.handle(message)
       }
 
       it("does not re-plan any 'OnFailure' stages") {
         verify(queue).push(CancelStage(message))
       }
+
     }
   }
 })