fix(queue): ContinueParentStageHandler should complete if all after…

… stages are complete (spinnaker#2419) This addresses a scenario where an execution would never complete if the set of after stage statuses contained a mix of SUCCEEDED and TERMINAL. See `onFailureStages()` on `CreateServerGroupStage`.
amanya · Sep 24, 2018 · 637d9db · 637d9db
1 parent de29fcc
commit 637d9db
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 20 deletions.
diff --git a/...com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStage.groovy b/...com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStage.groovy
@@ -80,15 +80,17 @@ class CreateServerGroupStage extends AbstractDeployStrategyStage {
 
   @Override
   void onFailureStages(@Nonnull Stage stage, StageGraphBuilder graph) {
-    super.onFailureStages(stage, graph)
-
     def stageData = stage.mapTo(StageData)
     if (!stageData.rollback?.onFailure) {
+      super.onFailureStages(stage, graph)
+
       // rollback on failure is not enabled
       return
     }
 
     if (!stageData.getServerGroup()) {
+      super.onFailureStages(stage, graph)
+
       // did not get far enough to create a new server group
       log.warn("No server group was created, skipping rollback! (executionId: ${stage.execution.id}, stageId: ${stage.id})")
       return
@@ -139,6 +141,9 @@ class CreateServerGroupStage extends AbstractDeployStrategyStage {
         ]
       }
     }
+
+    // any on-failure stages from the parent should be executed _after_ the rollback completes
+    super.onFailureStages(stage, graph)
   }
 
   private static class StageData {

diff --git a/...netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStageSpec.groovy b/...netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStageSpec.groovy
@@ -17,6 +17,9 @@
 package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup
 
 import com.netflix.spinnaker.orca.ExecutionStatus
+import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.AwsDeployStagePreProcessor
+import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.DeployStagePreProcessor
+import com.netflix.spinnaker.orca.pipeline.StageDefinitionBuilder
 import com.netflix.spinnaker.orca.pipeline.model.Task
 
 import java.util.concurrent.TimeUnit
@@ -28,10 +31,13 @@ import spock.lang.Unroll
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage
 
 class CreateServerGroupStageSpec extends Specification {
+  def deployStagePreProcessor = Mock(DeployStagePreProcessor)
+
   @Subject
   def createServerGroupStage = new CreateServerGroupStage(
     rollbackClusterStage: new RollbackClusterStage(),
-    destroyServerGroupStage: new DestroyServerGroupStage()
+    destroyServerGroupStage: new DestroyServerGroupStage(),
+    deployStagePreProcessors: [ deployStagePreProcessor ]
   )
 
   @Unroll
@@ -64,15 +70,23 @@ class CreateServerGroupStageSpec extends Specification {
     then:
     onFailureStageContexts == expectedOnFailureStageContexts
 
+    1 * deployStagePreProcessor.supports(_) >> { return true }
+    1 * deployStagePreProcessor.onFailureStageDefinitions(_) >> {
+      def stageDefinition = new DeployStagePreProcessor.StageDefinition()
+      stageDefinition.stageDefinitionBuilder = Mock(StageDefinitionBuilder)
+      stageDefinition.context = [source: "parent"]
+      return [stageDefinition]
+    }
+
     where:
     shouldRollbackOnFailure | strategy          | deployServerGroups                          | failedTask || expectedOnFailureStageContexts
-    false                   | "rollingredblack" | null                                        | false      || []
-    true                    | "rollingredblack" | null                                        | false      || []
-    false                   | "rollingredblack" | ["us-west-1": ["myapplication-stack-v001"]] | false      || []
-    true                    | "redblack"        | ["us-west-1": ["myapplication-stack-v001"]] | false      || []      // only rollback if task has failed
-    true                    | "highlander"      | ["us-west-1": ["myapplication-stack-v001"]] | false      || []      // highlander is not supported
-    true                    | "rollingredblack" | ["us-west-1": ["myapplication-stack-v001"]] | false      || [expectedRollbackContext([enableAndDisableOnly: true])]
-    true                    | "redblack"        | ["us-west-1": ["myapplication-stack-v001"]] | true       || [expectedRollbackContext([disableOnly: true])]
+    false                   | "rollingredblack" | null                                        | false      || [[source: "parent"]]
+    true                    | "rollingredblack" | null                                        | false      || [[source: "parent"]]
+    false                   | "rollingredblack" | ["us-west-1": ["myapplication-stack-v001"]] | false      || [[source: "parent"]]
+    true                    | "redblack"        | ["us-west-1": ["myapplication-stack-v001"]] | false      || [[source: "parent"]]      // only rollback if task has failed
+    true                    | "highlander"      | ["us-west-1": ["myapplication-stack-v001"]] | false      || [[source: "parent"]]      // highlander is not supported
+    true                    | "rollingredblack" | ["us-west-1": ["myapplication-stack-v001"]] | false      || [expectedRollbackContext([enableAndDisableOnly: true]), [source: "parent"]]
+    true                    | "redblack"        | ["us-west-1": ["myapplication-stack-v001"]] | true       || [expectedRollbackContext([disableOnly: true]), [source: "parent"]]
   }
 
   def "should build DestroyStage when 'rollbackDestroyLatest' is enabled"() {

diff --git a/orca-kotlin/src/main/kotlin/com/netflix/spinnaker/orca/ext/Stage.kt b/orca-kotlin/src/main/kotlin/com/netflix/spinnaker/orca/ext/Stage.kt
@@ -89,10 +89,10 @@ fun Stage.beforeStages(): List<Stage> =
 fun Stage.afterStages(): List<Stage> =
   syntheticStages().filter { it.syntheticStageOwner == STAGE_AFTER }
 
-fun Stage.allBeforeStagesComplete(): Boolean =
+fun Stage.allBeforeStagesSuccessful(): Boolean =
   beforeStages().all { it.status in listOf(SUCCEEDED, FAILED_CONTINUE, SKIPPED) }
 
-fun Stage.allAfterStagesComplete(): Boolean =
+fun Stage.allAfterStagesSuccessful(): Boolean =
   afterStages().all { it.status in listOf(SUCCEEDED, FAILED_CONTINUE, SKIPPED) }
 
 fun Stage.anyBeforeStagesFailed(): Boolean =
@@ -101,6 +101,9 @@ fun Stage.anyBeforeStagesFailed(): Boolean =
 fun Stage.anyAfterStagesFailed(): Boolean =
   afterStages().any { it.status in listOf(TERMINAL, STOPPED, CANCELED) }
 
+fun Stage.allAfterStagesComplete(): Boolean =
+  afterStages().all { it.status.isComplete }
+
 fun Stage.hasTasks(): Boolean =
   tasks.isNotEmpty()
 

diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/admin/HydrateQueueCommand.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/admin/HydrateQueueCommand.kt
@@ -20,8 +20,8 @@ import com.netflix.spinnaker.orca.ExecutionStatus.NOT_STARTED
 import com.netflix.spinnaker.orca.ExecutionStatus.RUNNING
 import com.netflix.spinnaker.orca.RetryableTask
 import com.netflix.spinnaker.orca.ext.afterStages
-import com.netflix.spinnaker.orca.ext.allAfterStagesComplete
-import com.netflix.spinnaker.orca.ext.allBeforeStagesComplete
+import com.netflix.spinnaker.orca.ext.allAfterStagesSuccessful
+import com.netflix.spinnaker.orca.ext.allBeforeStagesSuccessful
 import com.netflix.spinnaker.orca.ext.allUpstreamStagesComplete
 import com.netflix.spinnaker.orca.ext.beforeStages
 import com.netflix.spinnaker.orca.ext.isInitial
@@ -159,9 +159,9 @@ class HydrateQueueCommand(
         stage.afterStages().forEach { actions.addAll(processStage(it)) }
       }
 
-      if (stage.allBeforeStagesComplete() &&
+      if (stage.allBeforeStagesSuccessful() &&
         stage.tasks.all { it.status.isComplete } &&
-        stage.allAfterStagesComplete()) {
+        stage.allAfterStagesSuccessful()) {
         actions.add(Action(
           description = "All tasks and known synthetic stages are complete",
           message = CompleteStage(stage),

diff --git a/...-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ContinueParentStageHandler.kt b/...-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ContinueParentStageHandler.kt
@@ -44,7 +44,7 @@ class ContinueParentStageHandler(
   override fun handle(message: ContinueParentStage) {
     message.withStage { stage ->
       if (message.phase == STAGE_BEFORE) {
-        if (stage.allBeforeStagesComplete()) {
+        if (stage.allBeforeStagesSuccessful()) {
           when {
             stage.hasTasks() -> stage.runFirstTask()
             else             -> queue.push(CompleteStage(stage))
@@ -56,7 +56,7 @@ class ContinueParentStageHandler(
       } else {
         if (stage.allAfterStagesComplete()) {
           queue.push(CompleteStage(stage))
-        } else if (!stage.anyAfterStagesFailed()) {
+        } else {
           log.warn("Re-queuing $message as other ${message.phase} stages are still running")
           queue.push(message, retryDelay)
         }

diff --git a/...ue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/ContinueParentStageHandlerTest.kt b/...ue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/ContinueParentStageHandlerTest.kt
@@ -243,8 +243,8 @@ object ContinueParentStageHandlerTest : SubjectSpek<ContinueParentStageHandler>(
           subject.handle(message)
         }
 
-        it("does nothing") {
-          verifyZeroInteractions(queue)
+        it("tells the stage to complete") {
+          verify(queue).push(CompleteStage(pipeline.stageByRef("1")))
         }
       }