refactor(StageDefinitionBuilder): Delete deprecated aroundStages (#3133)

Removing deprecated `aroundStages` (deprecated in early 2018) Also, removing `DeployStage`/`DeployStrategyStage` (marked deprecated in 2015) This logically simplifies the stage flow composition and gives us the opportunity for having smarter deploy strategies (which can plan things on failure, etc) - this is already added to the `Strategy` (e.g. `composeFlow` became `composeBeforeFlow`+`composeAfterFlow`) but it's not really taken advantage of [yet]
spinnaker · Sep 19, 2019 · fba04ec · fba04ec
1 parent 50c7c5a
commit fba04ec
Show file tree

Hide file tree

Showing 34 changed files with 409 additions and 671 deletions.
diff --git a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStage.groovy b/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStage.groovy
@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.orca.bakery.pipeline
 
+import com.netflix.spinnaker.orca.pipeline.graph.StageGraphBuilder
+
 import java.time.Clock
 import javax.annotation.Nonnull
 import com.netflix.spinnaker.orca.ExecutionStatus
@@ -66,16 +68,13 @@ class BakeStage implements StageDefinitionBuilder {
   }
 
   @Override
-  @Nonnull
-  List<Stage> parallelStages(
-    @Nonnull Stage stage
-  ) {
-    if (isTopLevelStage(stage)) {
-      return parallelContexts(stage).collect { context ->
-        newStage(stage.execution, type, "Bake in ${context.region}", context, stage, STAGE_BEFORE)
-      }
-    } else {
-      return Collections.emptyList()
+  void beforeStages(@Nonnull Stage parent, @Nonnull StageGraphBuilder graph) {
+    if (isTopLevelStage(parent)) {
+      parallelContexts(parent)
+        .collect({ context ->
+          newStage(parent.execution, type, "Bake in ${context.region}", context, parent, STAGE_BEFORE)
+        })
+        .forEach({Stage s -> graph.add(s) })
     }
   }
 

diff --git a/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy b/orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy
@@ -16,18 +16,17 @@
 
 package com.netflix.spinnaker.orca.bakery.pipeline
 
+
 import java.time.Clock
-import java.time.Instant
 import com.netflix.spinnaker.orca.ExecutionStatus
+import com.netflix.spinnaker.orca.pipeline.graph.StageGraphBuilder
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import com.netflix.spinnaker.orca.pipeline.util.RegionCollector
 import spock.lang.Specification
 import spock.lang.Unroll
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage
-import static java.time.Clock.systemUTC
 import static java.time.Instant.EPOCH
-import static java.time.Instant.now
 import static java.time.ZoneOffset.UTC
 import static java.time.temporal.ChronoUnit.*
 
@@ -99,7 +98,9 @@ class BakeStageSpec extends Specification {
     }
 
     def bakeStage = pipeline.stageById("1")
-    def parallelStages = new BakeStage(regionCollector: new RegionCollector()).parallelStages(bakeStage)
+    def graph = StageGraphBuilder.beforeStages(bakeStage)
+    new BakeStage(regionCollector: new RegionCollector()).beforeStages(bakeStage, graph)
+    def parallelStages = graph.build()
 
     parallelStages.eachWithIndex { it, idx -> it.context.ami = idx + 1 }
     pipeline.stages.addAll(parallelStages)

diff --git a/...m/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/RollbackServerGroupStage.groovy b/...m/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/RollbackServerGroupStage.groovy
@@ -22,11 +22,14 @@ import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback.Prev
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback.Rollback
 import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback.TestRollback
 import com.netflix.spinnaker.orca.pipeline.StageDefinitionBuilder
+import com.netflix.spinnaker.orca.pipeline.graph.StageGraphBuilder
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.beans.factory.config.AutowireCapableBeanFactory
 import org.springframework.stereotype.Component
 
+import javax.annotation.Nonnull
+
 @Component
 class RollbackServerGroupStage implements StageDefinitionBuilder {
   public static final String PIPELINE_CONFIG_TYPE = "rollbackServerGroup"
@@ -35,7 +38,7 @@ class RollbackServerGroupStage implements StageDefinitionBuilder {
   AutowireCapableBeanFactory autowireCapableBeanFactory
 
   @Override
-  List<Stage> aroundStages(Stage stage) {
+  void afterStages(@Nonnull Stage stage, @Nonnull StageGraphBuilder graph) {
     def stageData = stage.mapTo(StageData)
 
     if (!stageData.rollbackType) {
@@ -44,7 +47,10 @@ class RollbackServerGroupStage implements StageDefinitionBuilder {
 
     def explicitRollback = stage.mapTo("/rollbackContext", stageData.rollbackType.implementationClass) as Rollback
     autowireCapableBeanFactory.autowireBean(explicitRollback)
-    return explicitRollback.buildStages(stage)
+
+    explicitRollback
+      .buildStages(stage)
+      .forEach({ graph.append(it) })
   }
 
   static enum RollbackType {

diff --git a/...naker/orca/clouddriver/pipeline/servergroup/strategies/AbstractDeployStrategyStage.groovy b/...naker/orca/clouddriver/pipeline/servergroup/strategies/AbstractDeployStrategyStage.groovy
@@ -32,6 +32,8 @@ import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 
+import javax.annotation.Nonnull
+
 import static com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.DeployStagePreProcessor.StageDefinition
 import static com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup.Support.locationFromStageData
 import static com.netflix.spinnaker.orca.pipeline.StageDefinitionBuilder.newStage
@@ -104,46 +106,67 @@ abstract class AbstractDeployStrategyStage extends AbstractCloudProviderAwareSta
   }
 
   @Override
-  List<Stage> aroundStages(Stage stage) {
-    correctContext(stage)
-    Strategy strategy = getStrategy(stage)
-    def preProcessors = deployStagePreProcessors.findAll { it.supports(stage) }
-    def stageData = stage.mapTo(StageData)
+  void beforeStages(@Nonnull Stage parent, @Nonnull StageGraphBuilder graph) {
+    correctContext(parent)
+    Strategy strategy = getStrategy(parent)
+    def preProcessors = deployStagePreProcessors.findAll { it.supports(parent) }
+    def stageData = parent.mapTo(StageData)
     def stages = []
-    stages.addAll(strategy.composeFlow(stage))
+    stages.addAll(strategy.composeBeforeStages(parent))
 
     preProcessors.each {
       def defaultContext = [
         credentials  : stageData.account,
         cloudProvider: stageData.cloudProvider
       ]
-      it.beforeStageDefinitions(stage).each {
+      it.beforeStageDefinitions(parent).each {
         stages << newStage(
-          stage.execution,
+          parent.execution,
           it.stageDefinitionBuilder.type,
           it.name,
           defaultContext + it.context,
-          stage,
+          parent,
           SyntheticStageOwner.STAGE_BEFORE
         )
       }
-      it.afterStageDefinitions(stage).each {
+    }
+
+    stages.forEach({ graph.append(it) })
+  }
+
+  @Override
+  void afterStages(@Nonnull Stage parent, @Nonnull StageGraphBuilder graph) {
+    Strategy strategy = getStrategy(parent)
+    def preProcessors = deployStagePreProcessors.findAll { it.supports(parent) }
+    def stageData = parent.mapTo(StageData)
+    List<Stage> stages = new ArrayList<>()
+
+    stages.addAll(strategy.composeAfterStages(parent))
+
+    preProcessors.each {
+      def defaultContext = [
+        credentials  : stageData.account,
+        cloudProvider: stageData.cloudProvider
+      ]
+      it.afterStageDefinitions(parent).each {
         stages << newStage(
-          stage.execution,
+          parent.execution,
           it.stageDefinitionBuilder.type,
           it.name,
           defaultContext + it.context,
-          stage,
+          parent,
           SyntheticStageOwner.STAGE_AFTER
         )
       }
     }
 
-    return stages
+    stages.forEach({ graph.append(it) })
   }
 
   @Override
   void onFailureStages(Stage stage, StageGraphBuilder graph) {
+    // TODO(mvulfson): Strategy on failure
+
     deployStagePreProcessors
       .findAll { it.supports(stage) }
       .collect { it.onFailureStageDefinitions(stage) }

diff --git a/...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/CFRollingRedBlackStrategy.java b/...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/CFRollingRedBlackStrategy.java
@@ -39,6 +39,7 @@
 import groovy.util.logging.Slf4j;
 import java.io.IOException;
 import java.util.*;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import lombok.AllArgsConstructor;
 import lombok.Data;
@@ -77,7 +78,19 @@ public class CFRollingRedBlackStrategy implements Strategy, ApplicationContextAw
       ThreadLocal.withInitial(() -> new Yaml(new SafeConstructor()));
 
   @Override
-  public List<Stage> composeFlow(Stage stage) {
+  public List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent).stream()
+        .filter(s -> s.getSyntheticStageOwner() == SyntheticStageOwner.STAGE_AFTER)
+        .collect(Collectors.toList());
+  }
+
+  public List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent).stream()
+        .filter(s -> s.getSyntheticStageOwner() == SyntheticStageOwner.STAGE_BEFORE)
+        .collect(Collectors.toList());
+  }
+
+  List<Stage> composeFlow(Stage stage) {
     if (!pipelineStage.isPresent()) {
       throw new IllegalStateException(
           "Rolling red/black cannot be run without front50 enabled. Please set 'front50.enabled: true' in your orca config.");

diff --git a/.../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/CustomStrategy.groovy b/.../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/CustomStrategy.groovy
@@ -32,8 +32,18 @@ class CustomStrategy implements Strategy {
   final String name = "custom"
 
   @Override
-  List<Stage> composeFlow(Stage stage) {
+  List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE })
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER })
+  }
 
+  List<Stage> composeFlow(Stage stage) {
     def cleanupConfig = AbstractDeployStrategyStage.CleanupConfig.fromStage(stage)
 
     Map parameters = [

diff --git a/...flix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/HighlanderStrategy.groovy b/...flix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/HighlanderStrategy.groovy
@@ -38,6 +38,17 @@ class HighlanderStrategy implements Strategy, ApplicationContextAware {
   ApplicationContext applicationContext
 
   @Override
+  List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE })
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER })
+  }
+
   List<Stage> composeFlow(Stage stage) {
     def cleanupConfig = AbstractDeployStrategyStage.CleanupConfig.fromStage(stage)
     Map shrinkContext = [

diff --git a/...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/MonitoredDeployStrategy.groovy b/...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/MonitoredDeployStrategy.groovy
@@ -74,6 +74,17 @@ class MonitoredDeployStrategy implements Strategy {
   DeploymentMonitorServiceProvider deploymentMonitorServiceProvider
 
   @Override
+  List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE })
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER })
+  }
+
   List<Stage> composeFlow(Stage stage) {
     def stages = []
     def stageData = stage.mapTo(MonitoredDeployStageData)

diff --git a/.../com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/NoStrategy.groovy b/.../com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/NoStrategy.groovy
@@ -25,7 +25,12 @@ class NoStrategy implements Strategy {
   final String name = "none"
 
   @Override
-  List<Stage> composeFlow(Stage stage) {
+  List<Stage> composeBeforeStages(Stage parent) {
+    return []
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
     return []
   }
 }
diff --git a/...etflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategy.groovy b/...etflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategy.groovy
@@ -52,6 +52,17 @@ class RedBlackStrategy implements Strategy, ApplicationContextAware {
   ApplicationContext applicationContext
 
   @Override
+  List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE })
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER })
+  }
+
   List<Stage> composeFlow(Stage stage) {
     List<Stage> stages = []
     StageData stageData = stage.mapTo(StageData)

diff --git a/...lix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RollingPushStrategy.groovy b/...lix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RollingPushStrategy.groovy
@@ -42,6 +42,17 @@ class RollingPushStrategy implements Strategy {
   SourceResolver sourceResolver
 
   @Override
+  List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE })
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER })
+  }
+
   List<Stage> composeFlow(Stage stage) {
     def stages = []
     def source = sourceResolver.getSource(stage)

diff --git a/...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RollingRedBlackStrategy.groovy b/...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RollingRedBlackStrategy.groovy
@@ -60,6 +60,17 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {
   ScaleDownClusterStage scaleDownClusterStage
 
   @Override
+  List<Stage> composeBeforeStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE })
+  }
+
+  @Override
+  List<Stage> composeAfterStages(Stage parent) {
+    return composeFlow(parent)
+      .findAll({ it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER })
+  }
+
   List<Stage> composeFlow(Stage stage) {
     if (!pipelineStage) {
       throw new IllegalStateException("Rolling red/black cannot be run without front50 enabled. Please set 'front50.enabled: true' in your orca config.")

diff --git a/...oovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/Strategy.java b/...oovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/Strategy.java
@@ -18,10 +18,17 @@
 
 import com.netflix.spinnaker.orca.kato.pipeline.Nameable;
 import com.netflix.spinnaker.orca.pipeline.model.Stage;
+import java.util.Collections;
 import java.util.List;
 
 public interface Strategy extends Nameable {
-  List<Stage> composeFlow(Stage stage);
+  List<Stage> composeBeforeStages(Stage parent);
+
+  List<Stage> composeAfterStages(Stage parent);
+
+  default List<Stage> composeOnFailureStages(Stage parent) {
+    return Collections.emptyList();
+  }
 
   default boolean replacesBasicSteps() {
     return false;

diff --git a/...etflix/spinnaker/orca/clouddriver/tasks/cluster/AbstractClusterWideClouddriverTask.groovy b/...etflix/spinnaker/orca/clouddriver/tasks/cluster/AbstractClusterWideClouddriverTask.groovy
@@ -30,7 +30,6 @@ import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTa
 import com.netflix.spinnaker.orca.clouddriver.utils.OortHelper
 import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard
 import com.netflix.spinnaker.orca.kato.pipeline.CopyLastAsgStage
-import com.netflix.spinnaker.orca.kato.pipeline.DeployStage
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
@@ -212,7 +211,6 @@ abstract class AbstractClusterWideClouddriverTask extends AbstractCloudProviderA
                                                             List<TargetServerGroup> clusterServerGroups) {
     //if we are a synthetic stage child of a deploy, don't operate on what we just deployed
     final Set<String> deployStageTypes = [
-      DeployStage.PIPELINE_CONFIG_TYPE,
       CopyLastAsgStage.PIPELINE_CONFIG_TYPE,
       CloneServerGroupStage.PIPELINE_CONFIG_TYPE,
       CreateServerGroupStage.PIPELINE_CONFIG_TYPE,