Skip to content

Commit

Permalink
fix(RRB): fix NPE introduced in #3031 (#3050)
Browse files Browse the repository at this point in the history 
When there is no source server group and a validation pipeline has been specified
RRB flow composition will fail (erroneously) trying to construct parameters to be
passed into the validation pipeline (even though the source isn't even necessary!).

This fix does two things:
1. Correctly calculate parameters passed into the validation pipeline
2. Don't even bother composing the flow if we are being called during the `beforeStages`
   since all these stages will be discared anyway (none of the stages have `STAGE_BEFORE` synthetic)

Medium term: all of this should just go away - we shouldn't be using `aroundStages` which is a
source of lots of confusion. I will be refactoring this code to take advantage of newer
`beforeStages` and `afterStages`
  • Loading branch information
@marchello2000
marchello2000 committed Jul 21, 2019
1 parent 2d2f924 commit 6eb1718
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 21 deletions.
41 changes: 25 additions & 16 deletions ...spinnaker/orca/clouddriver/pipeline/servergroup/strategies/RollingRedBlackStrategy.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies

import com.netflix.spinnaker.orca.ExecutionStatus
import com.netflix.spinnaker.orca.clouddriver.pipeline.cluster.ScaleDownClusterStage
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.CreateServerGroupStage
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage
Expand Down Expand Up @@ -105,15 +106,6 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {
targetLocation: cleanupConfig.location,
]

stages << newStage(
stage.execution,
determineTargetServerGroupStage.type,
"Determine Deployed Server Group",
findContext,
stage,
SyntheticStageOwner.STAGE_AFTER
)

// Get source ASG from prior determineSourceServerGroupTask
def source = null

Expand All @@ -122,6 +114,14 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {

Stage parentCreateServerGroupStage = stage.directAncestors().find() { it.type == CreateServerGroupStage.PIPELINE_CONFIG_TYPE }

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)
sourceServerGroup = parentStageData.source

Expand All @@ -138,6 +138,15 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {
throw new IllegalStateException("Failed to determine source server group from parent stage while planning RRB flow", e)
}

stages << newStage(
stage.execution,
determineTargetServerGroupStage.type,
"Determine Deployed Server Group",
findContext,
stage,
SyntheticStageOwner.STAGE_AFTER
)

if (source == null) {
log.warn("no source server group -- will perform RRB to exact fallback capacity $savedCapacity with no disableCluster or scaleDownCluster stages")
}
Expand Down Expand Up @@ -180,7 +189,7 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {

// an expression to grab newly deployed server group at runtime (ie. the server group being resized up)
def deployedServerGroupName = '${' + "#stage('${resizeStage.id}')['context']['asgName']" + '}'.toString()
stages.addAll(getBeforeCleanupStages(stage, stageData, source, deployedServerGroupName, p))
stages.addAll(getBeforeCleanupStages(stage, stageData, cleanupConfig, source?.serverGroupName, deployedServerGroupName, p))

// only generate the "disable p% of traffic" stages if we have something to disable
if (source) {
Expand Down Expand Up @@ -236,7 +245,8 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {

List<Stage> getBeforeCleanupStages(Stage parentStage,
RollingRedBlackStageData stageData,
ResizeStrategy.Source source,
AbstractDeployStrategyStage.CleanupConfig cleanupConfig,
String sourceServerGroupName,
String deployedServerGroupName,
int percentageComplete) {
def stages = []
Expand All @@ -255,10 +265,9 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {

if (stageData.pipelineBeforeCleanup?.application && stageData.pipelineBeforeCleanup?.pipelineId) {
def serverGroupCoordinates = [
region : source.region,
serverGroupName: source.serverGroupName,
account : source.credentials,
cloudProvider : source.cloudProvider
region : cleanupConfig.location.value,
account : cleanupConfig.account,
cloudProvider : cleanupConfig.cloudProvider
]

def pipelineContext = [
Expand All @@ -270,7 +279,7 @@ class RollingRedBlackStrategy implements Strategy, ApplicationContextAware {
serverGroupName: deployedServerGroupName
],
"sourceServerGroup" : serverGroupCoordinates + [
serverGroupName: source.serverGroupName
serverGroupName: sourceServerGroupName
],
"percentageComplete" : percentageComplete
]
Expand Down
46 changes: 41 additions & 5 deletions ...naker/orca/clouddriver/pipeline/servergroup/strategies/RollingRedBlackStrategySpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ 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.ScaleDownClusterStage
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.ResizeServerGroupStage
Expand Down Expand Up @@ -56,8 +57,6 @@ class RollingRedBlackStrategySpec extends Specification {
moniker : moniker,
cloudProvider : "aws",
region : "north",
availabilityZones : [north: ["pole-1a"]],
source : [:], // pretend there is no pre-existing server group
capacity : fallbackCapacity,
targetHealthyDeployPercentage: 90
]
Expand All @@ -74,13 +73,32 @@ class RollingRedBlackStrategySpec extends Specification {
moniker : moniker,
cloudProvider : "aws",
region : "north",
availabilityZones : [north: ["pole-1a"]],
source : [:], // pretend there is no pre-existing server group
capacity : fallbackCapacity,
targetHealthyDeployPercentage: 90,
targetPercentages : [50]
]
}

stage {
refId = "2-deploy3"
parent
context = [
refId : "stage",
account : "testAccount",
application : "unit",
stack : "tests",
moniker : moniker,
cloudProvider : "aws",
region : "north",
capacity : fallbackCapacity,
targetHealthyDeployPercentage: 90,
targetPercentages : [50, 100],
pipelineBeforeCleanup : [
application: "serverlabmvulfson",
pipelineId: "d054a10b-79fd-498b-8b0d-52b339e5643e"
]
]
}
}
}

Expand All @@ -93,9 +111,16 @@ class RollingRedBlackStrategySpec extends Specification {
determineTargetServerGroupStage: determineTargetServerGroupStage,
)

when: 'planning with no targetPercentages'
when: 'planning without having run createServerGroup'
def stage = pipeline.stageByRef("2-deploy1")
def syntheticStages = strat.composeFlow(stage)

then:
syntheticStages.size() == 0

when: 'planning with no targetPercentages'
pipeline.stageByRef("1-create").status = ExecutionStatus.RUNNING
syntheticStages = strat.composeFlow(stage)
def beforeStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_BEFORE }
def afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER }

Expand Down Expand Up @@ -139,5 +164,16 @@ class RollingRedBlackStrategySpec extends Specification {
afterStages.get(2).context.action == ResizeStrategy.ResizeAction.scale_exact
afterStages.get(2).context.capacity == fallbackCapacity

when: 'planning with cleanup pipeline'
stage = pipeline.stageByRef("2-deploy3")
syntheticStages = strat.composeFlow(stage)
afterStages = syntheticStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER }

then:
noExceptionThrown()
afterStages.size() == 5
afterStages.first().type == determineTargetServerGroupStage.type
afterStages[2].type == pipelineStage.type
afterStages[4].type == pipelineStage.type
}
}

0 comments on commit 6eb1718

Please sign in to comment.