feat(moniker): Use moniker in TrafficGuard. #1727
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| */ | ||
| @Component | ||
| public class MonikerHelper { | ||
|
|
||
| public String getAppNameFromStage(Stage stage, String fallbackFriggaName) { | ||
| Names names = Names.parseName(fallbackFriggaName); | ||
| Moniker moniker = monikerFromStage(stage); | ||
|
|
@@ -58,4 +59,20 @@ static public Moniker monikerFromStage(Stage stage) { | |
| return null; | ||
| } | ||
| } | ||
|
|
||
| static public Moniker monikerFromStage(Stage stage, String fallbackFriggaName) { | ||
| Moniker moniker = monikerFromStage(stage); | ||
|
There was a problem hiding this comment. Q: what happens when you have a stage that's dealing with potentially > 1 server group or cluster?
The rollback stages are a current example with > 1 server group specified. There was a problem hiding this comment. Thats a great point. I just tried a rollback and noticed this is what is sent from Deck to Orca: There was a problem hiding this comment. So in the case of TrafficGuard, the moniker: would be passed through. In TrafficGuard itself only There was a problem hiding this comment. @ajordens Do you know an example of a stage that uses multiple clusters? |
||
| if (moniker == null) { | ||
|
There was a problem hiding this comment. If interested, a one liner equivalent: There was a problem hiding this comment. Or |
||
| Names names = Names.parseName(fallbackFriggaName); | ||
| return Moniker.builder() | ||
| .app(names.getApp()) | ||
| .stack(names.getStack()) | ||
| .detail(names.getDetail()) | ||
| .cluster(names.getCluster()) | ||
| .sequence(names.getSequence()) | ||
| .build(); | ||
| } | ||
| return moniker; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,12 +18,11 @@ | |
|
|
||
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
| import com.netflix.spinnaker.moniker.Moniker; | ||
| import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.Location; | ||
| import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.support.TargetServerGroup; | ||
| import com.netflix.spinnaker.orca.front50.Front50Service; | ||
| import com.netflix.spinnaker.orca.front50.model.Application; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
|
|
@@ -47,6 +46,7 @@ public TrafficGuard(OortHelper oortHelper, Optional<Front50Service> front50Servi | |
| } | ||
|
|
||
| public void verifyInstanceTermination(String serverGroupNameFromStage, | ||
| Moniker serverGroupMonikerFromStage, | ||
| List<String> instanceIds, | ||
| String account, | ||
| Location location, | ||
|
|
@@ -67,16 +67,15 @@ public void verifyInstanceTermination(String serverGroupNameFromStage, | |
|
|
||
| instancesPerServerGroup.entrySet().forEach(entry -> { | ||
| String serverGroupName = entry.getKey(); | ||
| if (hasDisableLock(serverGroupMonikerFromStage, account, location)) { | ||
| Optional<TargetServerGroup> targetServerGroup = oortHelper.getTargetServerGroup(account, serverGroupName, location.getValue(), cloudProvider); | ||
|
|
||
| targetServerGroup.ifPresent(serverGroup -> { | ||
| Optional<Map> thisInstance = serverGroup.getInstances().stream().filter(i -> "Up".equals(i.get("healthState"))).findFirst(); | ||
| if (thisInstance.isPresent() && "Up".equals(thisInstance.get().get("healthState"))) { | ||
| long otherActiveInstances = serverGroup.getInstances().stream().filter(i -> "Up".equals(i.get("healthState")) && !entry.getValue().contains(i.get("name"))).count(); | ||
| if (otherActiveInstances == 0) { | ||
| verifyOtherServerGroupsAreTakingTraffic(serverGroupName, serverGroup.getMoniker(), location, account, cloudProvider, operationDescriptor); | ||
| } | ||
| } | ||
| }); | ||
|
|
@@ -91,22 +90,19 @@ private Optional<String> resolveServerGroupNameForInstance(String instanceId, St | |
| return Optional.ofNullable((String) instance.orElse(new HashMap<>()).get("serverGroup")); | ||
| } | ||
|
|
||
| public void verifyTrafficRemoval(String serverGroupName, Moniker serverGroupMoniker, String account, Location location, String cloudProvider, String operationDescriptor) { | ||
| if (!hasDisableLock(serverGroupMoniker, account, location)) { | ||
| return; | ||
| } | ||
|
|
||
| verifyOtherServerGroupsAreTakingTraffic(serverGroupName, serverGroupMoniker,location, account, cloudProvider, operationDescriptor); | ||
|
There was a problem hiding this comment. spacing needed |
||
| } | ||
|
|
||
| private void verifyOtherServerGroupsAreTakingTraffic(String serverGroupName, Moniker serverGroupMoniker, Location location, String account, String cloudProvider, String operationDescriptor) { | ||
|
There was a problem hiding this comment. Do we still need to pass around There was a problem hiding this comment. The server group name isn't contained in the moniker object (only app, stack, detail, cluster, sequence). In the case of the Frigga naming convention, you can reconstruct the name using the moniker object. But that reconstruction won't work for the manifest based k8s provider. There was a problem hiding this comment. Is it silly to ask why we couldn’t have it also contain the serverGroupName? There was a problem hiding this comment. @lwander can probably give you a more thorough explanation. From my POV, I think that it isn't necessary to facilitate the migration away from frigga. It would also require a significant amount of effort. If you search for FYI: This is related to another point. There is now a lot of redundancy with application/stack/detail in the stage data payload. In many places, those fields are already included at the root level of the stage and are now in the moniker as well. A follow up task will probably be to consolidate those. There was a problem hiding this comment. No that's a good question. We don't want the moniker to be the identifier for the object since the point of the moniker is to decouple the name from the relationships (app, cluster) that frigga presented us. The separation of concerns is: When you assign or derive a moniker from an object, that object doesn't even have to provide us with a name, (e.g. ec2 tags), so the idea is really to keep these concepts separate. There was a problem hiding this comment. My point about including the name was somewhat predicated in that when I hear The relationships are really part of the identity of an object, which is why we need to pass application/cluster/region/name/etc. around everywhere. My biggest grief is that we're slowly developing more and more ways to specify the coordinates for an object: serverGroupName, asgName, regions, region, zone, zones, moniker, etc. In the case of moniker it's only dealing with names, so we can ignore regions/region/zone/zones and think about it only as a partial coordinate. I hold out hope that there will be some consolidation, as it's difficult and error prone to keep everything straight. Just my 2c. I think it would be cleaner and possible to pass a Moniker into validateClusterStatus but otherwise this PR fits the existing patterns and can be merged. There was a problem hiding this comment. My hope is this kind of clean up/refactor will point out more places where we can consolidate the many names/coordinates you point out. We'll keep this in mind going forward. |
||
| Optional<Map> cluster = oortHelper.getCluster(serverGroupMoniker.getApp(), account, serverGroupMoniker.getCluster(), cloudProvider); | ||
|
|
||
| if (!cluster.isPresent()) { | ||
| throw new IllegalStateException(format("Could not find cluster '%s' in %s/%s with traffic guard configured.", serverGroupMoniker.getCluster(), account, location.getValue())); | ||
| } | ||
| List<TargetServerGroup> targetServerGroups = ((List<Map<String, Object>>) cluster.get().get("serverGroups")) | ||
| .stream() | ||
|
|
@@ -120,19 +116,18 @@ private void verifyOtherServerGroupsAreTakingTraffic(String serverGroupName, Loc | |
| ); | ||
| if (!otherEnabledServerGroupFound) { | ||
| throw new IllegalStateException(format("This cluster ('%s' in %s/%s) has traffic guards enabled. " + | ||
| "%s %s would leave the cluster with no instances taking traffic.", serverGroupMoniker.getCluster(), account, location.getValue(), operationDescriptor, serverGroupName)); | ||
| } | ||
| } | ||
|
|
||
| public boolean hasDisableLock(Moniker clusterMoniker, String account, Location location) { | ||
| if (front50Service == null) { | ||
| log.warn("Front50 has not been configured, no way to check disable lock. Fix this by setting front50.enabled: true"); | ||
| return false; | ||
| } | ||
| Application application; | ||
| try { | ||
| application = front50Service.get(clusterMoniker.getApp()); | ||
| } catch (RetrofitError e) { | ||
| if (e.getResponse() != null && e.getResponse().getStatus() == 404) { | ||
| application = null; | ||
|
|
@@ -147,6 +142,6 @@ public boolean hasDisableLock(String cluster, String account, Location location) | |
| List<ClusterMatchRule> rules = trafficGuards.stream().map(guard -> | ||
| new ClusterMatchRule(guard.get("account"), guard.get("location"), guard.get("stack"), guard.get("detail"), 1) | ||
| ).collect(Collectors.toList()); | ||
| return ClusterMatcher.getMatchingRule(account, location.getValue(), clusterMoniker, rules) != null; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is indeed a Map ... may be safer to objectMapper.convert(serverGroup.moniker, Moniker) rather than rely on groovy "magic".