From 9817373163608f0f227569d616aed5013e175eb7 Mon Sep 17 00:00:00 2001 From: Maggie Neterval Date: Wed, 18 Sep 2019 18:17:34 -0400 Subject: [PATCH] fix(google): fix deploying with red/black into an empty cluster (#3173) * fix(google): fix deploying with red/black into an empty cluster * fix(google): replace catch with null check, handle clone case --- .../gce/GceDeployStagePreProcessor.java | 58 +++++++----- .../AbstractDeployStrategyStage.groovy | 11 +++ .../strategies/RedBlackStrategy.groovy | 37 +++++--- .../strategies/RedBlackStrategySpec.groovy | 92 +++++++++++++------ .../gce/GceDeployStagePreProcessorTest.java | 30 ++++++ 5 files changed, 165 insertions(+), 63 deletions(-) diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessor.java b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessor.java index ed801fa746..1dfc7d587c 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessor.java +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessor.java @@ -24,14 +24,15 @@ import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.ResizeServerGroupStage; import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.AbstractDeployStrategyStage; import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.DeployStagePreProcessor; +import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup; import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroupResolver; import com.netflix.spinnaker.orca.kato.pipeline.strategy.Strategy; import com.netflix.spinnaker.orca.kato.pipeline.support.ResizeStrategy; import com.netflix.spinnaker.orca.kato.pipeline.support.StageData; import com.netflix.spinnaker.orca.pipeline.model.Stage; import java.util.Collections; -import java.util.HashMap; import java.util.Map; +import java.util.Optional; import javax.annotation.Nullable; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; @@ -66,7 +67,13 @@ public ImmutableList beforeStageDefinitions(Stage stage) { return ImmutableList.of(); } - Map resizeContext = getResizeContext(stageData); + Optional> optionalResizeContext = getResizeContext(stageData); + if (!optionalResizeContext.isPresent()) { + // no source server group, no need to resize + return ImmutableList.of(); + } + + Map resizeContext = optionalResizeContext.get(); resizeContext.put("pinMinimumCapacity", true); return ImmutableList.of( @@ -107,26 +114,25 @@ private static boolean shouldPinSourceServerGroup(String strategy) { return Strategy.fromStrategyKey(strategy) == Strategy.RED_BLACK; } - private Map getResizeContext(StageData stageData) { - AbstractDeployStrategyStage.CleanupConfig cleanupConfig = - AbstractDeployStrategyStage.CleanupConfig.fromStage(stageData); - Map resizeContext = new HashMap<>(); - - resizeContext.put( - cleanupConfig.getLocation().singularType(), cleanupConfig.getLocation().getValue()); - resizeContext.put("cluster", cleanupConfig.getCluster()); - resizeContext.put("moniker", cleanupConfig.getMoniker()); - resizeContext.put("credentials", cleanupConfig.getAccount()); - resizeContext.put("cloudProvider", cleanupConfig.getCloudProvider()); - - ResizeStrategy.Source source = getSource(targetServerGroupResolver, stageData, resizeContext); - resizeContext.put("serverGroupName", source.getServerGroupName()); - resizeContext.put("action", ResizeStrategy.ResizeAction.scale_to_server_group); - resizeContext.put("source", source); - resizeContext.put( - "useNameAsLabel", true); // hint to deck that it should _not_ override the name - - return resizeContext; + private Optional> getResizeContext(StageData stageData) { + Map resizeContext = + AbstractDeployStrategyStage.CleanupConfig.toContext(stageData); + + try { + ResizeStrategy.Source source = getSource(targetServerGroupResolver, stageData, resizeContext); + if (source == null) { + return Optional.empty(); + } + + resizeContext.put("serverGroupName", source.getServerGroupName()); + resizeContext.put("action", ResizeStrategy.ResizeAction.scale_to_server_group); + resizeContext.put("source", source); + resizeContext.put( + "useNameAsLabel", true); // hint to deck that it should _not_ override the name + return Optional.of(resizeContext); + } catch (TargetServerGroup.NotFoundException e) { + return Optional.empty(); + } } @Nullable @@ -140,7 +146,13 @@ private StageDefinition buildUnpinServerGroupStage(StageData stageData) { return null; } - Map resizeContext = getResizeContext(stageData); + Optional> optionalResizeContext = getResizeContext(stageData); + if (!optionalResizeContext.isPresent()) { + // no source server group, no need to unpin + return null; + } + + Map resizeContext = optionalResizeContext.get(); resizeContext.put("unpinMinimumCapacity", true); return new StageDefinition( diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/AbstractDeployStrategyStage.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/AbstractDeployStrategyStage.groovy index 73c9de720b..34bcbe69dc 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/AbstractDeployStrategyStage.groovy +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/AbstractDeployStrategyStage.groovy @@ -200,5 +200,16 @@ abstract class AbstractDeployStrategyStage extends AbstractCloudProviderAwareSta location: loc ) } + + static Map toContext(StageData stageData) { + CleanupConfig cleanupConfig = fromStage(stageData) + Map context = new HashMap<>() + context.put(cleanupConfig.getLocation().singularType(), cleanupConfig.getLocation().getValue()) + context.put("cloudProvider", cleanupConfig.getCloudProvider()) + context.put("cluster", cleanupConfig.getCluster()) + context.put("credentials", cleanupConfig.getAccount()) + context.put("moniker", cleanupConfig.getMoniker()) + return context + } } } diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategy.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategy.groovy index a1adaa7e44..d27456d896 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategy.groovy +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategy.groovy @@ -16,9 +16,12 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies +import com.netflix.spinnaker.orca.ExecutionStatus import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.DisableClusterStage import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.ScaleDownClusterStage import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.ShrinkClusterStage +import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.CloneServerGroupStage +import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.CreateServerGroupStage import com.netflix.spinnaker.orca.kato.pipeline.support.StageData import com.netflix.spinnaker.orca.pipeline.WaitStage import com.netflix.spinnaker.orca.pipeline.model.Stage @@ -50,17 +53,29 @@ class RedBlackStrategy implements Strategy, ApplicationContextAware { @Override List composeFlow(Stage stage) { - def stages = [] - def stageData = stage.mapTo(StageData) - def cleanupConfig = AbstractDeployStrategyStage.CleanupConfig.fromStage(stage) - - Map baseContext = [ - (cleanupConfig.location.singularType()): cleanupConfig.location.value, - cluster : cleanupConfig.cluster, - credentials : cleanupConfig.account, - moniker : cleanupConfig.moniker, - cloudProvider : cleanupConfig.cloudProvider, - ] + List stages = [] + StageData stageData = stage.mapTo(StageData) + Map baseContext = AbstractDeployStrategyStage.CleanupConfig.toContext(stageData) + + Stage parentCreateServerGroupStage = stage.directAncestors().find() { it.type == CreateServerGroupStage.PIPELINE_CONFIG_TYPE || it.type == CloneServerGroupStage.PIPELINE_CONFIG_TYPE } + if (parentCreateServerGroupStage == null) { + throw new IllegalStateException("Failed to determine source server group from parent stage while planning red/black flow") + } + if (parentCreateServerGroupStage.status == ExecutionStatus.NOT_STARTED) { + // No point in composing the flow if we are called to plan "beforeStages" since we don't have any STAGE_BEFOREs. + // Also, we rely on the the source server group task to have run already. + // In the near future we will move composeFlow into beforeStages and afterStages instead of the + // deprecated aroundStages + return [] + } + + StageData parentStageData = parentCreateServerGroupStage.mapTo(StageData) + StageData.Source sourceServerGroup = parentStageData.source + + // Short-circuit if there is no source server group + if (sourceServerGroup == null || sourceServerGroup.serverGroupName == null) { + return [] + } // We don't want the key propagated if interestingHealthProviderNames isn't defined, since this prevents // health providers from the stage's 'determineHealthProviders' task to be added to the context. diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategySpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategySpec.groovy index 044bf92e69..96f9130d9c 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategySpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RedBlackStrategySpec.groovy @@ -19,17 +19,19 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService import com.netflix.spinnaker.kork.dynamicconfig.SpringDynamicConfigService import com.netflix.spinnaker.moniker.Moniker +import com.netflix.spinnaker.orca.ExecutionStatus import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.DisableClusterStage import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.ScaleDownClusterStage import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.ShrinkClusterStage import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard import com.netflix.spinnaker.orca.pipeline.WaitStage -import com.netflix.spinnaker.orca.pipeline.model.Execution -import com.netflix.spinnaker.orca.pipeline.model.Stage import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner import org.springframework.mock.env.MockEnvironment import spock.lang.Specification +import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline +import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage + class RedBlackStrategySpec extends Specification { def trafficGuard = Stub(TrafficGuard) @@ -44,28 +46,64 @@ class RedBlackStrategySpec extends Specification { def "should compose flow"() { given: - Moniker moniker = new Moniker(app: "unit", stack: "tests"); - def ctx = [ - account : "testAccount", - application : "unit", - stack : "tests", - moniker : moniker, - cloudProvider : "aws", - region : "north", - availabilityZones : [ - north: ["pole-1a"] - ] - ] - def stage = new Stage(Execution.newPipeline("orca"), "whatever", ctx) + Moniker moniker = new Moniker(app: "unit", stack: "tests") + + def pipeline = pipeline { + application = "orca" + stage { + refId = "1-create" + type = "createServerGroup" + context = [ + refId: "stage_createASG" + ] + + stage { + refId = "2-deploy1" + parent + context = [ + refId : "stage", + account : "testAccount", + application : "unit", + stack : "tests", + moniker : moniker, + cloudProvider : "aws", + region : "north", + availabilityZones : [ + north: ["pole-1a"] + ] + ] + } + } + } + def strat = new RedBlackStrategy( shrinkClusterStage: shrinkClusterStage, scaleDownClusterStage: scaleDownClusterStage, disableClusterStage: disableClusterStage, waitStage: waitStage ) + def stage = pipeline.stageByRef("2-deploy1") - when: + when: 'planning without having run createServerGroup' def syntheticStages = strat.composeFlow(stage) + + then: + syntheticStages.size() == 0 + + when: 'deploying into a new cluster (no source server group)' + pipeline.stageByRef("1-create").status = ExecutionStatus.RUNNING + + then: + syntheticStages.size() == 0 + + when: 'deploying into an existing cluster' + pipeline.stageByRef("1-create").context.source = [ + account: "testAccount", + region: "north", + serverGroupName: "unit-tests-v000", + asgName: "unit-tests-v000" + ] + syntheticStages = strat.composeFlow(stage) def beforeStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE } def afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER } @@ -83,9 +121,8 @@ class RedBlackStrategySpec extends Specification { preferLargerOverNewer : false, ] - when: - ctx.maxRemainingAsgs = 10 - stage = new Stage(Execution.newPipeline("orca"), "whatever", ctx) + when: 'maxRemainingAsgs is specified' + pipeline.stageByRef("2-deploy1").context.maxRemainingAsgs = 10 syntheticStages = strat.composeFlow(stage) beforeStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE } afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER } @@ -97,9 +134,8 @@ class RedBlackStrategySpec extends Specification { afterStages.first().context.shrinkToSize == 10 afterStages.last().type == disableClusterStage.type - when: - ctx.scaleDown = true - stage = new Stage(Execution.newPipeline("orca"), "whatever", ctx) + when: 'scaleDown is true' + pipeline.stageByRef("2-deploy1").context.scaleDown = true syntheticStages = strat.composeFlow(stage) beforeStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE } afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER } @@ -112,9 +148,8 @@ class RedBlackStrategySpec extends Specification { afterStages[2].type == scaleDownClusterStage.type afterStages[2].context.allowScaleDownActive == false - when: - ctx.interestingHealthProviderNames = ["Google"] - stage = new Stage(Execution.newPipeline("orca"), "whatever", ctx) + when: 'interestingHealthProviderNames is specified' + pipeline.stageByRef("2-deploy1").context.interestingHealthProviderNames = ["Google"] syntheticStages = strat.composeFlow(stage) beforeStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE } afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER } @@ -125,10 +160,9 @@ class RedBlackStrategySpec extends Specification { afterStages.first().type == shrinkClusterStage.type afterStages.first().context.interestingHealthProviderNames == ["Google"] - when: - ctx.delayBeforeDisableSec = 5 - ctx.delayBeforeScaleDownSec = 10 - stage = new Stage(Execution.newPipeline("orca"), "resize", ctx) + when: 'delayBeforeDisableSec and delayBeforeScaleDownSec are specified' + pipeline.stageByRef("2-deploy1").context.delayBeforeDisableSec = 5 + pipeline.stageByRef("2-deploy1").context.delayBeforeScaleDownSec = 10 syntheticStages = strat.composeFlow(stage) beforeStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE } afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER } diff --git a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessorTest.java b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessorTest.java index 74e7dba3fc..4aabfa21ea 100644 --- a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessorTest.java +++ b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/pipeline/providers/gce/GceDeployStagePreProcessorTest.java @@ -95,6 +95,36 @@ void redBlackStrategyTest() { .containsExactly(resizeServerGroupStage); } + @Test + void redBlackStrategyNoExistingServerGroupTest() { + when(targetServerGroupResolver.resolve(any())) + .thenThrow(TargetServerGroup.NotFoundException.class); + Stage stage = new Stage(); + Map context = createDefaultContext(); + context.put("strategy", "redblack"); + stage.setContext(context); + + // Before Stages + List beforeStages = preProcessor.beforeStageDefinitions(stage); + assertThat(beforeStages).isEmpty(); + + // Additional Steps + List additionalSteps = preProcessor.additionalSteps(stage); + assertThat(additionalSteps) + .extracting(StepDefinition::getTaskClass) + .containsExactly(CaptureSourceServerGroupCapacityTask.class); + + // After Stages + List afterStages = preProcessor.afterStageDefinitions(stage); + assertThat(afterStages) + .extracting(stageDefinition -> stageDefinition.stageDefinitionBuilder) + .containsExactly(applySourceServerGroupCapacityStage); + + // On Failure Stages + List failureStages = preProcessor.onFailureStageDefinitions(stage); + assertThat(failureStages).isEmpty(); + } + @Test void noneStrategyTest() { Stage stage = new Stage();