Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(moniker): Use moniker in TrafficGuard. #1727

Merged
merged 8 commits into from
Nov 2, 2017
Merged

feat(moniker): Use moniker in TrafficGuard. #1727

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
56804f3
feat(moniker): Use monker in TrafficGuard.
andrewbackes Oct 17, 2017
53a21da
Check for null monikers and fall back on frigga
andrewbackes Oct 25, 2017
51d2dff
Formatting
andrewbackes Oct 25, 2017
b8e76fd
Refactor
andrewbackes Oct 25, 2017
b2360e3
Pass moniker to validateClusterStatus()
andrewbackes Nov 1, 2017
08e5a5f
Resolved merge conflicts
andrewbackes Nov 1, 2017
2ab69cf
Style cleanup
andrewbackes Nov 2, 2017
579f27c
Merge branch 'master' into clouddriver-trafficguard-moniker
ajordens Nov 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions .../netflix/spinnaker/orca/clouddriver/pipeline/servergroup/support/TargetServerGroup.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.orca.clouddriver.utils.MonikerHelper
import groovy.transform.InheritConstructors
import groovy.transform.ToString
import groovy.util.logging.Slf4j
Expand All @@ -29,7 +31,8 @@ import com.netflix.spinnaker.orca.pipeline.model.Stage
*/
class TargetServerGroup {
// Delegates all Map interface calls to this object.
@Delegate private final Map<String, Object> serverGroup
@Delegate
private final Map<String, Object> serverGroup

TargetServerGroup(Map<String, Object> serverGroupData) {
if (serverGroupData.instances && serverGroupData.instances instanceof Collection) {
Expand Down Expand Up @@ -73,6 +76,14 @@ class TargetServerGroup {
return (serverGroup.instances ?: []) as List<Map>
}

/**
* Used in TrafficGuard, which is Java, which doesn't play nice with @Delegate
*/
Moniker getMoniker() {
ObjectMapper objectMapper = new ObjectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should inject an ObjectMapper into the constructor (it'll be autowired) vs explicitly creating one here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since TargetServerGroup is not a bean, I'm not quite sure how to do what you are asking. I pulled it out and made it final static for now.

return objectMapper.convertValue(serverGroup.moniker, Moniker);
}

Map toClouddriverOperationPayload(String account) {
//TODO(cfieber) - add an endpoint on Clouddriver to do provider appropriate conversion of a TargetServerGroup
def op = [
Expand Down Expand Up @@ -173,19 +184,21 @@ class TargetServerGroup {
/**
* "Previous Server Group"
*/
ancestor_asg_dynamic,
ancestor_asg_dynamic,
/**
* "Oldest Server Group"
*/
oldest_asg_dynamic,
oldest_asg_dynamic,
/**
* "(Deprecated) Current Server Group"
*/
@Deprecated current_asg,
@Deprecated
current_asg,
/**
* "(Deprecated) Last Server Group"
*/
@Deprecated ancestor_asg,
@Deprecated
ancestor_asg,

boolean isDynamic() {
return this.name().endsWith("dynamic")
Expand Down
2 changes: 2 additions & 0 deletions ...naker/orca/clouddriver/tasks/instance/TerminateInstanceAndDecrementServerGroupTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.netflix.spinnaker.orca.clouddriver.pipeline.instance.TerminatingInsta
import com.netflix.spinnaker.orca.clouddriver.pipeline.instance.TerminatingInstanceSupport
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.Location
import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTask
import com.netflix.spinnaker.orca.clouddriver.utils.MonikerHelper
import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard
import com.netflix.spinnaker.orca.pipeline.model.Stage
import org.springframework.beans.factory.annotation.Autowired
Expand Down Expand Up @@ -53,6 +54,7 @@ class TerminateInstanceAndDecrementServerGroupTask extends AbstractCloudProvider

trafficGuard.verifyInstanceTermination(
serverGroupName,
MonikerHelper.monikerFromStage(stage, serverGroupName),
[stage.context.instance] as List<String>,
account,
Location.region(stage.context.region as String),
Expand Down
2 changes: 2 additions & 0 deletions ...roovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/TerminateInstancesTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.netflix.spinnaker.orca.clouddriver.pipeline.instance.TerminatingInsta
import com.netflix.spinnaker.orca.clouddriver.pipeline.instance.TerminatingInstanceSupport
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.Location
import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTask
import com.netflix.spinnaker.orca.clouddriver.utils.MonikerHelper
import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard
import com.netflix.spinnaker.orca.pipeline.model.Stage
import org.springframework.beans.factory.annotation.Autowired
Expand Down Expand Up @@ -53,6 +54,7 @@ class TerminateInstancesTask extends AbstractCloudProviderAwareTask implements T

trafficGuard.verifyInstanceTermination(
serverGroupName,
MonikerHelper.monikerFromStage(stage, serverGroupName),
stage.context.instanceIds as List<String>,
account,
Location.region(stage.context.region as String),
Expand Down
7 changes: 4 additions & 3 deletions ...com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/AbstractBulkServerGroupTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public abstract class AbstractBulkServerGroupTask extends AbstractCloudProviderA
@Autowired
protected KatoService katoService;

abstract void validateClusterStatus(Map<String, Object> operation);
abstract void validateClusterStatus(Map<String, Object> operation, Moniker moniker);

abstract String getClouddriverOperation();

Expand Down Expand Up @@ -104,8 +104,9 @@ public TaskResult execute(Stage stage) {
targetServerGroups.forEach( targetServerGroup -> {
Map<String , Map> tmp = new HashMap<>();
Map operation = targetServerGroup.toClouddriverOperationPayload(request.getCredentials());

validateClusterStatus(operation);
Moniker moniker = targetServerGroup.getMoniker() == null ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly cleaner alternative to a somewhat ugly multiline ternary.

Moniker moniker = targetServerGroup.getMoniker();
if (moniker == null) {
  moniker = MonikerHelper.friggaToMoniker(targetServerGroup.getName())
}

MonikerHelper.friggaToMoniker(targetServerGroup.getName()) : targetServerGroup.getMoniker();
validateClusterStatus(operation, moniker);
tmp.put(getClouddriverOperation(), operation);
operations.add(tmp);
});
Expand Down
20 changes: 16 additions & 4 deletions ...y/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/AbstractServerGroupTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup

import com.netflix.spinnaker.moniker.Moniker
import com.netflix.spinnaker.orca.ExecutionStatus
import com.netflix.spinnaker.orca.RetryableTask
import com.netflix.spinnaker.orca.TaskResult
Expand All @@ -25,6 +26,7 @@ import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.Locat
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.clouddriver.tasks.AbstractCloudProviderAwareTask
import com.netflix.spinnaker.orca.clouddriver.utils.MonikerHelper
import com.netflix.spinnaker.orca.pipeline.model.Stage
import org.springframework.beans.factory.annotation.Autowired

Expand All @@ -47,7 +49,7 @@ abstract class AbstractServerGroupTask extends AbstractCloudProviderAwareTask im
false
}

protected void validateClusterStatus(Map operation) {}
protected void validateClusterStatus(Map operation, Moniker moniker) {}

abstract String getServerGroupAction()

Expand All @@ -58,9 +60,9 @@ abstract class AbstractServerGroupTask extends AbstractCloudProviderAwareTask im
TaskResult execute(Stage stage) {
String cloudProvider = getCloudProvider(stage)
String account = getCredentials(stage)

def operation = convert(stage)
validateClusterStatus(operation)
Moniker moniker = convertMoniker(stage)
validateClusterStatus(operation, moniker)
if (!operation) {
// nothing to do but succeed
return new TaskResult(ExecutionStatus.SUCCEEDED)
Expand Down Expand Up @@ -108,6 +110,17 @@ abstract class AbstractServerGroupTask extends AbstractCloudProviderAwareTask im
operation
}

Moniker convertMoniker(Stage stage) {
if (TargetServerGroup.isDynamicallyBound(stage)) {
TargetServerGroup tsg = TargetServerGroupResolver.fromPreviousStage(stage);
return tsg.getMoniker() == null ? MonikerHelper.friggaToMoniker(tsg.getName()) : tsg.getMoniker();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need the else {

and since this is groovy, we can do:

return MonikerHelper.monikerFromStage(stage, serverGroupName ?: asgName)

String serverGroupName = (String) stage.context.serverGroupName;
String asgName = (String) stage.context.asgName;
return MonikerHelper.monikerFromStage(stage, serverGroupName == null ? asgName : serverGroupName);
}
}

/**
* @return a Map of location -> server group name
*/
Expand Down Expand Up @@ -138,5 +151,4 @@ abstract class AbstractServerGroupTask extends AbstractCloudProviderAwareTask im
operation.namespace ? new Location(Type.NAMESPACE, operation.namespace) :
null
}

}
4 changes: 3 additions & 1 deletion .../com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/BulkDestroyServerGroupTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup;

import com.netflix.spinnaker.moniker.Moniker;
import com.netflix.spinnaker.orca.RetryableTask;
import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -36,8 +37,9 @@ String getClouddriverOperation() {
}

@Override
void validateClusterStatus(Map<String, Object> operation) {
void validateClusterStatus(Map<String, Object> operation, Moniker moniker) {
trafficGuard.verifyTrafficRemoval((String) operation.get("serverGroupName"),
moniker,
getCredentials(operation),
getLocation(operation),
getCloudProvider(operation), "Destroying");
Expand Down
4 changes: 3 additions & 1 deletion .../com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/BulkDisableServerGroupTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup;

import com.netflix.spinnaker.moniker.Moniker;
import com.netflix.spinnaker.orca.RetryableTask;
import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -35,8 +36,9 @@ String getClouddriverOperation() {
}

@Override
void validateClusterStatus(Map<String, Object> operation) {
void validateClusterStatus(Map<String, Object> operation, Moniker moniker) {
trafficGuard.verifyTrafficRemoval((String) operation.get("serverGroupName"),
moniker,
getCredentials(operation),
getLocation(operation),
getCloudProvider(operation), "Disabling");
Expand Down
4 changes: 3 additions & 1 deletion ...vy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/DestroyServerGroupTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup

import com.netflix.spinnaker.moniker.Moniker
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DestroyServerGroupStage
import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard
import org.springframework.beans.factory.annotation.Autowired
Expand All @@ -29,8 +30,9 @@ class DestroyServerGroupTask extends AbstractServerGroupTask {
TrafficGuard trafficGuard

@Override
void validateClusterStatus(Map operation) {
void validateClusterStatus(Map operation, Moniker moniker) {
trafficGuard.verifyTrafficRemoval(operation.serverGroupName as String,
moniker,
getCredentials(operation),
getLocation(operation),
getCloudProvider(operation), "Destroying")
Expand Down
4 changes: 3 additions & 1 deletion ...vy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/DisableServerGroupTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup

import com.netflix.spinnaker.moniker.Moniker
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage
import com.netflix.spinnaker.orca.clouddriver.utils.TrafficGuard
import org.springframework.beans.factory.annotation.Autowired
Expand All @@ -34,8 +35,9 @@ class DisableServerGroupTask extends AbstractServerGroupTask {
String serverGroupAction = DisableServerGroupStage.PIPELINE_CONFIG_TYPE

@Override
void validateClusterStatus(Map operation) {
void validateClusterStatus(Map operation, Moniker moniker) {
trafficGuard.verifyTrafficRemoval(operation.serverGroupName as String,
moniker,
getCredentials(operation),
getLocation(operation),
getCloudProvider(operation), "Disabling")
Expand Down
4 changes: 3 additions & 1 deletion ...ovy/com/netflix/spinnaker/orca/clouddriver/tasks/servergroup/ResizeServerGroupTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.servergroup

import com.netflix.spinnaker.moniker.Moniker
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.ResizeServerGroupStage
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.Location
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup
Expand Down Expand Up @@ -63,9 +64,10 @@ class ResizeServerGroupTask extends AbstractServerGroupTask {
}

@Override
void validateClusterStatus(Map operation) {
void validateClusterStatus(Map operation, Moniker moniker) {
if (operation.capacity.desired == 0) {
trafficGuard.verifyTrafficRemoval(operation.serverGroupName as String,
moniker,
getCredentials(operation),
getLocation(operation),
getCloudProvider(operation), "Removal of all instances in ")
Expand Down
10 changes: 4 additions & 6 deletions ...uddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/ClusterMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,21 @@

package com.netflix.spinnaker.orca.clouddriver.utils;

import com.netflix.frigga.Names;
import com.netflix.spinnaker.moniker.Moniker;

import java.util.Comparator;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

public class ClusterMatcher {

public static ClusterMatchRule getMatchingRule(String account, String location, String clusterName, List<ClusterMatchRule> rules) {
public static ClusterMatchRule getMatchingRule(String account, String location, Moniker clusterMoniker, List<ClusterMatchRule> rules) {
if (!Optional.ofNullable(rules).isPresent()) {
return null;
}
Names nameParts = Names.parseName(clusterName);

String stack = nameParts.getStack() == null ? "" : nameParts.getStack();
String detail = nameParts.getDetail() == null ? "" : nameParts.getDetail();
String stack = clusterMoniker.getStack() == null ? "" : clusterMoniker.getStack();
String detail = clusterMoniker.getDetail() == null ? "" : clusterMoniker.getDetail();

List<ClusterMatchRule> candidates = rules.stream().filter(rule -> {
String ruleAccount = rule.getAccount();
Expand Down
20 changes: 20 additions & 0 deletions ...ouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/utils/MonikerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import com.netflix.frigga.Names;
import com.netflix.spinnaker.moniker.Moniker;
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.pipeline.model.Stage;
import org.springframework.stereotype.Component;

Expand All @@ -27,6 +29,7 @@
*/
@Component
public class MonikerHelper {

public String getAppNameFromStage(Stage stage, String fallbackFriggaName) {
Names names = Names.parseName(fallbackFriggaName);
Moniker moniker = monikerFromStage(stage);
Expand Down Expand Up @@ -58,4 +61,21 @@ static public Moniker monikerFromStage(Stage stage) {
return null;
}
}

static public Moniker monikerFromStage(Stage stage, String fallbackFriggaName) {
Moniker moniker = monikerFromStage(stage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: what happens when you have a stage that's dealing with potentially > 1 server group or cluster?

stage.context.moniker seems to imply there only ever being one, and nothing in the context key indicates it's intent.

The rollback stages are a current example with > 1 server group specified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a great point. I just tried a rollback and noticed this is what is sent from Deck to Orca:

{
  "application" : "nginx",
  "name" : "Rollback Server Group: nginx-trafficguards-v011",
  "appConfig" : null,
  "stages" : [ {
    "rollbackType" : "EXPLICIT",
    "rollbackContext" : {
      "rollbackServerGroupName" : "nginx-trafficguards-v011",
      "targetHealthyRollbackPercentage" : 100,
      "restoreServerGroupName" : "nginx-trafficguards-v010"
    },
    "platformHealthOnlyShowOverride" : false,
    "type" : "rollbackServerGroup",
    "moniker" : {
      "app" : "nginx",
      "cluster" : "nginx-trafficguards",
      "detail" : null,
      "sequence" : 11,
      "stack" : "trafficguards"
    },
    "region" : "us-west-2",
    "credentials" : "aws-dev",
    "cloudProvider" : "aws",
    "user" : "anonymous",
    "refId" : "0",
    "requisiteStageRefIds" : [ ]
  } ],
  "origin" : "deck"
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the case of TrafficGuard, the moniker:

"moniker" : {
      "app" : "nginx",
      "cluster" : "nginx-trafficguards",
      "detail" : null,
      "sequence" : 11,
      "stack" : "trafficguards"
    }

would be passed through. In TrafficGuard itself only moniker.cluster is used. In ClusterMatcher only moniker.stack and moniker.detail are used. moniker.sequence is not used. So the moniker is more of a cluster moniker than a server group moniker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajordens Do you know an example of a stage that uses multiple clusters?

return moniker == null ? friggaToMoniker(fallbackFriggaName) : moniker;
}

static public Moniker friggaToMoniker(String friggaName) {
Names names = Names.parseName(friggaName);
return Moniker.builder()
.app(names.getApp())
.stack(names.getStack())
.detail(names.getDetail())
.cluster(names.getCluster())
.sequence(names.getSequence())
.build();
}

}
Loading