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.

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

perf(ecs): Narrowing the cache search for the ECS provider on views #6256

Merged
merged 2 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ public static String getTaskDefinitionKey(
return buildKey(Namespace.TASK_DEFINITIONS.ns, account, region, taskDefinitionArn);
}

public static String getAlarmKey(String account, String region, String alarmArn) {
return buildKey(Namespace.ALARMS.ns, account, region, alarmArn);
public static String getAlarmKey(String account, String region, String alarmArn, String cluster) {
return buildKey(Namespace.ALARMS.ns, account, region, alarmArn + SEPARATOR + cluster);
}

public static String getScalableTargetKey(String account, String region, String resourceId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ public Collection<T> getAll(String account, String region) {
return convertAll(data);
}

public Collection<T> getAll(Collection<String> identifiers) {
Collection<CacheData> allData = cacheView.getAll(keyNamespace, identifiers);
jasonmcintosh marked this conversation as resolved.
Show resolved Hide resolved
if (allData == null) {
return Collections.emptyList();
}
return convertAll(allData);
}

/**
* @param key A key within the key namespace that will be used to retrieve the object.
* @return An object of the generic type that is associated to the key.
Expand Down Expand Up @@ -110,4 +118,8 @@ private Collection<CacheData> fetchFromCache(String account, String region) {

return allData;
}

public Collection<String> filterIdentifiers(String glob) {
return cacheView.filterIdentifiers(keyNamespace, glob);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.netflix.spinnaker.cats.cache.Cache;
import com.netflix.spinnaker.cats.cache.CacheData;
import com.netflix.spinnaker.clouddriver.ecs.cache.Keys;
import com.netflix.spinnaker.clouddriver.ecs.cache.model.EcsMetricAlarm;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -71,31 +72,19 @@ protected EcsMetricAlarm convert(CacheData cacheData) {
}

public List<EcsMetricAlarm> getMetricAlarms(
String serviceName, String accountName, String region) {
String serviceName, String accountName, String region, String ecsClusterName) {
List<EcsMetricAlarm> metricAlarms = new LinkedList<>();
Collection<EcsMetricAlarm> allMetricAlarms = getAll(accountName, region);
Copy link
Member Author

Choose a reason for hiding this comment

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

Before All the alarms for an ECS account/region where fetched and iterated through to match the service. This is extremely costly.

After the change the ECSCluster is added during the caching cycles to the cache key id for the ECS provider in the alarms. We retrieve the IDs with ECS account/region/EcsClusterName and then try to match the service.


outLoop:
for (EcsMetricAlarm metricAlarm : allMetricAlarms) {
for (String action : metricAlarm.getAlarmActions()) {
if (action.contains(serviceName)) {
metricAlarms.add(metricAlarm);
continue outLoop;
}
}
String glob = Keys.getAlarmKey(accountName, region, "*", ecsClusterName);
Collection<String> metricAlarmsIds = filterIdentifiers(glob);
Collection<EcsMetricAlarm> allMetricAlarms = getAll(metricAlarmsIds);

for (String action : metricAlarm.getOKActions()) {
if (action.contains(serviceName)) {
metricAlarms.add(metricAlarm);
continue outLoop;
}
}

for (String action : metricAlarm.getInsufficientDataActions()) {
if (action.contains(serviceName)) {
metricAlarms.add(metricAlarm);
continue outLoop;
}
for (EcsMetricAlarm metricAlarm : allMetricAlarms) {
if (metricAlarm.getAlarmActions().stream().anyMatch(action -> action.contains(serviceName))
Copy link
Member Author

Choose a reason for hiding this comment

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

Small refactoring here to make it more readable

|| metricAlarm.getOKActions().stream().anyMatch(action -> action.contains(serviceName))
|| metricAlarm.getInsufficientDataActions().stream()
.anyMatch(action -> action.contains(serviceName))) {
metricAlarms.add(metricAlarm);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ public Void operate(List priorOutputs) {

updateTaskStatus("Removing MetricAlarms from " + description.getServerGroupName() + ".");
ecsCloudMetricService.deleteMetrics(
description.getServerGroupName(), description.getAccount(), description.getRegion());
description.getServerGroupName(),
description.getAccount(),
description.getRegion(),
ecsClusterName);
updateTaskStatus("Done removing MetricAlarms from " + description.getServerGroupName() + ".");

UpdateServiceRequest updateServiceRequest = new UpdateServiceRequest();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,16 @@ public class EcsApplication implements Application {
private String name;
Map<String, String> attributes;
Map<String, Set<String>> clusterNames;
Map<String, Set<String>> clusterNameMetadata;

public EcsApplication(
String name, Map<String, String> attributes, Map<String, Set<String>> clusterNames) {
String name,
Map<String, String> attributes,
Map<String, Set<String>> clusterNames,
Map<String, Set<String>> getClusterNameMetadata) {
this.name = name;
this.attributes = attributes;
this.clusterNames = clusterNames;
this.clusterNameMetadata = getClusterNameMetadata;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,13 @@ Map<String, Collection<CacheData>> generateFreshData(Set<MetricAlarm> cacheableM
Map<String, Collection<CacheData>> newDataMap = new HashMap<>();

for (MetricAlarm metricAlarm : cacheableMetricAlarm) {
String key = Keys.getAlarmKey(accountName, region, metricAlarm.getAlarmArn());
String cluster =
metricAlarm.getDimensions().stream()
Copy link
Member Author

Choose a reason for hiding this comment

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

Based on the AWS SDK a cloudwatch alarm for the ECS contains 2 dimensions depending for the type:

  • Service alarm contains the dimension ECSCluster and ServiceName
  • Autoscaling group alarm of an ECS cluster contains the ECSCluster and the Capacity provider.

This change includes the ECSClusterName in the cached key id to make the search less costly

.filter(t -> t.getName().equalsIgnoreCase("ClusterName"))
.map(t -> t.getValue())
.collect(Collectors.joining());

String key = Keys.getAlarmKey(accountName, region, metricAlarm.getAlarmArn(), cluster);
Map<String, Object> attributes =
convertMetricAlarmToAttributes(metricAlarm, accountName, region);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.common.collect.Lists;
import com.netflix.spinnaker.cats.cache.Cache;
import com.netflix.spinnaker.clouddriver.aws.security.AmazonClientProvider;
import com.netflix.spinnaker.clouddriver.ecs.cache.Keys;
import com.netflix.spinnaker.clouddriver.ecs.cache.client.EcsClusterCacheClient;
import com.netflix.spinnaker.clouddriver.ecs.cache.model.EcsCluster;
import com.netflix.spinnaker.clouddriver.ecs.security.NetflixECSCredentials;
Expand Down Expand Up @@ -55,9 +56,11 @@ public Collection<EcsCluster> getAllEcsClusters() {
// TODO include[] input of Describe Cluster is not a part of this implementation, need to
// implement in the future if additional properties are needed.
public Collection<Cluster> getEcsClusterDescriptions(String account, String region) {
String glob = Keys.getClusterKey(account, region, "*");
Collection<String> ecsClustersIdentifiers = ecsClusterCacheClient.filterIdentifiers(glob);
Collection<Cluster> clusters = new ArrayList<>();
List<String> filteredEcsClusters =
ecsClusterCacheClient.getAll().stream()
ecsClusterCacheClient.getAll(ecsClustersIdentifiers).stream()
.filter(
cluster ->
account.equals(cluster.getAccount()) && region.equals(cluster.getRegion()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.netflix.spinnaker.clouddriver.aws.AmazonCloudProvider;
import com.netflix.spinnaker.clouddriver.aws.data.ArnUtils;
import com.netflix.spinnaker.clouddriver.ecs.EcsCloudProvider;
import com.netflix.spinnaker.clouddriver.ecs.cache.Keys;
import com.netflix.spinnaker.clouddriver.ecs.cache.client.EcsLoadbalancerCacheClient;
import com.netflix.spinnaker.clouddriver.ecs.cache.client.EcsTargetGroupCacheClient;
import com.netflix.spinnaker.clouddriver.ecs.cache.client.ServiceCacheClient;
Expand Down Expand Up @@ -118,8 +119,13 @@ public List<Details> byAccountAndRegionAndName(String account, String region, St
@Override
public Set<EcsLoadBalancer> getApplicationLoadBalancers(String application) {
// Find the load balancers currently in use by ECS services in this application
String glob =
application != null
? Keys.getServiceKey("*", "*", application + "*")
: Keys.getServiceKey("*", "*", "*");
Collection<String> ecsServices = ecsServiceCacheClient.filterIdentifiers(glob);
Set<Service> services =
ecsServiceCacheClient.getAll().stream()
ecsServiceCacheClient.getAll(ecsServices).stream()
.filter(service -> service.getApplicationName().equals(application))
.collect(Collectors.toSet());
log.debug("Retrieved {} services for application '{}'", services.size(), application);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,17 @@ public EcsServerClusterProvider(

private Map<String, Set<EcsServerCluster>> findClusters(
Map<String, Set<EcsServerCluster>> clusterMap, AmazonCredentials credentials) {
return findClusters(clusterMap, credentials, null);
return findClusters(clusterMap, credentials, null, true);
}

private Map<String, Set<EcsServerCluster>> findClusters(
Map<String, Set<EcsServerCluster>> clusterMap,
AmazonCredentials credentials,
String application) {
String application,
boolean inludeDetails) {
for (AmazonCredentials.AWSRegion awsRegion : credentials.getRegions()) {
clusterMap = findClustersForRegion(clusterMap, credentials, awsRegion, application);
clusterMap =
findClustersForRegion(clusterMap, credentials, awsRegion, application, inludeDetails);
}

return clusterMap;
Expand All @@ -118,10 +120,16 @@ private Map<String, Set<EcsServerCluster>> findClustersForRegion(
Map<String, Set<EcsServerCluster>> clusterMap,
AmazonCredentials credentials,
AmazonCredentials.AWSRegion awsRegion,
String application) {
String application,
boolean includeDetails) {

Collection<Service> services =
serviceCacheClient.getAll(credentials.getName(), awsRegion.getName());
String glob =
application != null
? Keys.getServiceKey(credentials.getName(), awsRegion.getName(), application + "*")
: Keys.getServiceKey(credentials.getName(), awsRegion.getName(), "*");

Collection<String> ecsServices = serviceCacheClient.filterIdentifiers(glob);
Collection<Service> services = serviceCacheClient.getAll(ecsServices);
Collection<Task> allTasks = taskCacheClient.getAll(credentials.getName(), awsRegion.getName());

for (Service service : services) {
Expand Down Expand Up @@ -167,7 +175,8 @@ private Map<String, Set<EcsServerCluster>> findClustersForRegion(
service.getClusterName(),
taskDefinition,
service.getSubnets(),
service.getSecurityGroups());
service.getSecurityGroups(),
includeDetails);

if (ecsServerGroup == null) {
continue;
Expand Down Expand Up @@ -308,13 +317,9 @@ private EcsServerGroup buildEcsServerGroup(
String ecsClusterName,
com.amazonaws.services.ecs.model.TaskDefinition taskDefinition,
List<String> eniSubnets,
List<String> eniSecurityGroups) {
List<String> eniSecurityGroups,
boolean includeDetails) {
ServerGroup.InstanceCounts instanceCounts = buildInstanceCount(instances);
TaskDefinition ecsTaskDefinition = buildTaskDefinition(taskDefinition);
EcsServerGroup.Image image = new EcsServerGroup.Image();
image.setImageId(ecsTaskDefinition.getContainerImage());
image.setName(ecsTaskDefinition.getContainerImage());

String scalableTargetId = "service/" + ecsClusterName + "/" + serviceName;
String scalableTargetKey = Keys.getScalableTargetKey(account, region, scalableTargetId);
ScalableTarget scalableTarget = scalableTargetCacheClient.get(scalableTargetKey);
Expand Down Expand Up @@ -368,31 +373,52 @@ private EcsServerGroup buildEcsServerGroup(
}
}
}

Set<String> metricAlarmNames =
ecsCloudWatchAlarmCacheClient.getMetricAlarms(serviceName, account, region).stream()
ecsCloudWatchAlarmCacheClient
.getMetricAlarms(serviceName, account, region, ecsClusterName)
.stream()
.map(EcsMetricAlarm::getAlarmName)
.collect(Collectors.toSet());

EcsServerGroup serverGroup =
new EcsServerGroup()
.setDisabled(capacity.getDesired() == 0)
.setName(serviceName)
.setCloudProvider(EcsCloudProvider.ID)
.setType(EcsCloudProvider.ID)
.setRegion(region)
.setInstances(instances)
.setCapacity(capacity)
.setImage(image)
.setInstanceCounts(instanceCounts)
.setCreatedTime(creationTime)
.setEcsCluster(ecsClusterName)
.setTaskDefinition(ecsTaskDefinition)
.setVpcId(vpcId)
.setSecurityGroups(securityGroups)
.setMetricAlarms(metricAlarmNames)
.setMoniker(moniker);

EcsServerGroup serverGroup = new EcsServerGroup();
if (includeDetails) {
Copy link
Member Author

Choose a reason for hiding this comment

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

includeDetails is false only for the getSummaries. The rest of the logic remains the same

TaskDefinition ecsTaskDefinition = buildTaskDefinition(taskDefinition);
EcsServerGroup.Image image = new EcsServerGroup.Image();
image.setImageId(ecsTaskDefinition.getContainerImage());
image.setName(ecsTaskDefinition.getContainerImage());
serverGroup
.setDisabled(capacity.getDesired() == 0)
.setName(serviceName)
.setCloudProvider(EcsCloudProvider.ID)
.setType(EcsCloudProvider.ID)
.setRegion(region)
.setInstances(instances)
.setCapacity(capacity)
.setImage(image)
.setInstanceCounts(instanceCounts)
.setCreatedTime(creationTime)
.setEcsCluster(ecsClusterName)
.setTaskDefinition(ecsTaskDefinition)
.setVpcId(vpcId)
.setSecurityGroups(securityGroups)
.setMetricAlarms(metricAlarmNames)
.setMoniker(moniker);
} else {
serverGroup
.setDisabled(capacity.getDesired() == 0)
.setName(serviceName)
.setCloudProvider(EcsCloudProvider.ID)
.setType(EcsCloudProvider.ID)
.setRegion(region)
.setInstances(instances)
.setCapacity(capacity)
.setInstanceCounts(instanceCounts)
.setCreatedTime(creationTime)
.setEcsCluster(ecsClusterName)
.setVpcId(vpcId)
.setSecurityGroups(securityGroups)
.setMetricAlarms(metricAlarmNames)
.setMoniker(moniker);
}
EcsServerGroup.AutoScalingGroup asg =
new EcsServerGroup.AutoScalingGroup()
.setDesiredCapacity(scalableTarget.getMaxCapacity())
Expand Down Expand Up @@ -456,26 +482,30 @@ private AmazonCredentials getEcsCredentials(String account) {

@Override
public Map<String, Set<EcsServerCluster>> getClusterSummaries(String application) {
return getClusters();
return getClusters0(application, false);
}

@Override
public Map<String, Set<EcsServerCluster>> getClusterDetails(String application) {
return getClusters0(application, true);
}

@Override
public Map<String, Set<EcsServerCluster>> getClusters() {
Map<String, Set<EcsServerCluster>> clusterMap = new HashMap<>();

for (AmazonCredentials credentials : getEcsCredentials()) {
clusterMap = findClusters(clusterMap, credentials, application);
clusterMap = findClusters(clusterMap, credentials);
}

return clusterMap;
}

@Override
public Map<String, Set<EcsServerCluster>> getClusters() {
private Map<String, Set<EcsServerCluster>> getClusters0(
String application, boolean includeDetails) {
Map<String, Set<EcsServerCluster>> clusterMap = new HashMap<>();

for (AmazonCredentials credentials : getEcsCredentials()) {
clusterMap = findClusters(clusterMap, credentials);
clusterMap = findClusters(clusterMap, credentials, application, includeDetails);
}
return clusterMap;
}
Expand All @@ -485,7 +515,7 @@ public Map<String, Set<EcsServerCluster>> getClusters() {
public Set<EcsServerCluster> getClusters(String application, String account) {
try {
AmazonCredentials credentials = getEcsCredentials(account);
return findClusters(new HashMap<>(), credentials, application).get(application);
return findClusters(new HashMap<>(), credentials, application, true).get(application);
} catch (NoSuchElementException exception) {
log.info("No ECS Credentials were found for account " + account);
return null;
Expand Down Expand Up @@ -536,7 +566,7 @@ public ServerGroup getServerGroup(
AmazonCredentials credentials = getEcsCredentials(account);
Moniker moniker = MonikerHelper.applicationNameToMoniker(serverGroupName);
log.debug("App Name is: " + moniker.getApp());
clusterMap = findClusters(clusterMap, credentials, moniker.getApp());
clusterMap = findClusters(clusterMap, credentials, moniker.getApp(), true);
} catch (NoSuchElementException exception) {
/* This is ugly, but not sure how else to do it. If we don't have creds due
* to not being an ECS account, there's nothing to do here, and we should
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@ public class EcsCloudMetricService {

private final Logger log = LoggerFactory.getLogger(getClass());

public void deleteMetrics(String serviceName, String account, String region) {
public void deleteMetrics(
String serviceName, String account, String region, String ecsClusterName) {
List<EcsMetricAlarm> metricAlarms =
metricAlarmCacheClient.getMetricAlarms(serviceName, account, region);
metricAlarmCacheClient.getMetricAlarms(ecsClusterName, serviceName, account, region);

if (metricAlarms.isEmpty()) {
return;
Expand Down
Loading
Loading