Skip to content

Commit

Permalink
fix(RRB): Do not consider traffic guards during RRB deploy (#3054)
Browse files Browse the repository at this point in the history
As a result of #3037, the RRB flow now no longer bypasses traffic guards
(it used to do that before, here: https://github.com/spinnaker/orca/blob/master/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/AbstractClusterWideClouddriverTask.groovy#L145 `shouldSkipTrafficGuards`.)

Instead, now when we run the first disable step in RRB (e.g. disable 5% of source), the traffic guards think
the whole ASG is going away as it is unaware of the percentage disable flag and thus it terminates the operation
since "disabling entire ASG while new is still not fully up" is most certainly unsafe.

For now, add back the logic we had in the `AbstractClusterWideTask` to `DisableServerGroupTask` to skip check on RRB deploys.
In the future, we should not have artisanal checks like this and instead have traffic guards understand that
a percentage of an ASG is going away and validate against that percentage.
  • Loading branch information
marchello2000 committed Jul 22, 2019
1 parent aef16bf commit 39e6068
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ abstract class AbstractClusterWideClouddriverTask extends AbstractCloudProviderA
]).build()
}

// TOOD: mvulfson: this is technically not needed since RRB doesn't call DisableCluster anymore
private static boolean shouldSkipTrafficGuardCheck(List<Map<String, Map>> katoOps) {
// if any operation has a non-null desiredPercentage that indicates an entire server group is not going away
// (e.g. in the case of rolling red/black), let's bypass the traffic guard check
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ class DisableServerGroupTask extends AbstractServerGroupTask {

@Override
void validateClusterStatus(Map operation, Moniker moniker) {
// if any operation has a non-null desiredPercentage that indicates an entire server group is not going away
// (e.g. in the case of rolling red/black deploy), let's bypass the traffic guard check for now.
// In the future, we should actually use this percentage to correctly determine if the disable operation is safe
if (operation.desiredPercentage && operation.desiredPercentage < 100) {
return
}

trafficGuard.verifyTrafficRemoval(operation.serverGroupName as String,
moniker,
getCredentials(operation),
Expand Down

0 comments on commit 39e6068

Please sign in to comment.