Skip to content

Commit

Permalink
perf(kubernetes): Reduce memory allocation during caching cycles (#3736)
Browse files Browse the repository at this point in the history 
* perf(kubernetes): Reduce memory allocation during caching cycles

The caching logic for the kubernetes v2 provider allocates a
lot of short-term memory during each caching cycle, putting
pressure on the garbage collector, in some cases exceeding
the garbage collection overhead.

A significant contributor is the logic to compute relationships
between kubernetes objects. We currently create a CacheData object
to hold each relationship and rely on downstream (and inefficient)
logic to merge these into one CacheData per object, containing all
of its relationships.

Improve this by having invertRelationships return one CacheData
per object (containing all of its relationships) rather than on
CacheData per relationship. Likewise, have getClusterRelationships
return a single CacheData for each application, rather than a
separate one for each application-object relationship.

* test(kubernetes): Add tests to getClusterRelationships

Also simplify the logic in the invertRelationships test
  • Loading branch information
@ezimanyi
ezimanyi committed Jun 3, 2019
1 parent 823e771 commit 00d249a
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 97 deletions.
150 changes: 74 additions & 76 deletions ...tflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCacheDataConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,7 @@
import com.netflix.spinnaker.moniker.Moniker;
import com.netflix.spinnaker.moniker.Namer;
import io.kubernetes.client.JSON;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.*;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -400,55 +392,88 @@ static Map<String, Collection<String>> ownerReferenceRelationships(
* To ensure the entire relationship graph is bidirectional, invert any relationship entries here
* to point back at the resource being cached (key).
*/
static List<CacheData> invertRelationships(CacheData cacheData) {
String key = cacheData.getId();
Keys.CacheKey parsedKey =
Keys.parseKey(key)
.orElseThrow(
() ->
new IllegalStateException(
"Cache data produced with illegal key format " + key));
String group = parsedKey.getGroup();
Map<String, Collection<String>> relationshipGroupings = cacheData.getRelationships();
List<CacheData> result = new ArrayList<>();

for (Collection<String> relationships : relationshipGroupings.values()) {
for (String relationship : relationships) {
invertSingleRelationship(group, key, relationship)
.flatMap(
cd -> {
result.add(cd);
return Optional.empty();
});
}
static List<CacheData> invertRelationships(List<CacheData> resourceData) {
Map<String, Set<String>> inverted = new HashMap<>();
resourceData.forEach(
cacheData ->
cacheData.getRelationships().values().stream()
.flatMap(Collection::stream)
.forEach(
r -> inverted.computeIfAbsent(r, k -> new HashSet<>()).add(cacheData.getId())));

return inverted.entrySet().stream()
.map(e -> KubernetesCacheDataConverter.buildInverseRelationship(e.getKey(), e.getValue()))
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());
}

private static Optional<CacheData> buildInverseRelationship(
String key, Set<String> relationshipKeys) {
Map<String, Collection<String>> relationships = new HashMap<>();
for (String relationshipKey : relationshipKeys) {
Keys.CacheKey parsedKey =
Keys.parseKey(relationshipKey)
.orElseThrow(
() ->
new IllegalStateException(
"Cache data produced with illegal key format " + relationshipKey));
relationships
.computeIfAbsent(parsedKey.getGroup(), k -> new HashSet<>())
.add(relationshipKey);
}

return result;
/*
* Worth noting the strange behavior here. If we are inverting a relationship to create a cache data for
* either a cluster or an application we need to insert attributes to ensure the cache data gets entered into
* the cache. If we are caching anything else, we don't want competing agents to overwrite attributes, so
* we leave them blank.
*/
return Keys.parseKey(key)
.map(
k -> {
Map<String, Object> attributes;
int ttl;
if (Keys.LogicalKind.isLogicalGroup(k.getGroup())) {
ttl = logicalTtlSeconds;
attributes =
new ImmutableMap.Builder<String, Object>().put("name", k.getName()).build();
} else {
ttl = infrastructureTtlSeconds;
attributes = new HashMap<>();
}
return defaultCacheData(key, ttl, attributes, relationships);
});
}

static CacheData getClusterRelationships(String account, CacheData cacheData) {
Moniker moniker = getMoniker(cacheData);
static List<CacheData> getClusterRelationships(String accountName, List<CacheData> resourceData) {
Map<String, List<Moniker>> monikers =
resourceData.stream()
.map(KubernetesCacheDataConverter::getMoniker)
.filter(Objects::nonNull)
.filter(m -> m.getApp() != null && m.getCluster() != null)
.collect(Collectors.groupingBy(Moniker::getApp));

if (moniker == null) {
return null;
}

String cluster = moniker.getCluster();
String application = moniker.getApp();
return monikers.entrySet().stream()
.map(
e ->
KubernetesCacheDataConverter.getApplicationClusterRelationships(
accountName, e.getKey(), e.getValue()))
.collect(Collectors.toList());
}

if (cluster == null || application == null) {
return null;
}
private static CacheData getApplicationClusterRelationships(
String account, String application, List<Moniker> monikers) {
Set<String> clusterRelationships =
monikers.stream()
.map(m -> Keys.cluster(account, application, m.getCluster()))
.collect(Collectors.toSet());

Map<String, Object> attributes = new HashMap<>();
Map<String, Collection<String>> relationships = new HashMap<>();
relationships.put(
CLUSTERS.toString(),
Collections.singletonList(Keys.cluster(account, application, cluster)));
CacheData appToCluster =
defaultCacheData(
Keys.application(application), logicalTtlSeconds, attributes, relationships);
return appToCluster;
relationships.put(CLUSTERS.toString(), clusterRelationships);
return defaultCacheData(
Keys.application(application), logicalTtlSeconds, attributes, relationships);
}

static void logStratifiedCacheData(
Expand Down Expand Up @@ -543,31 +568,4 @@ static Map<String, Collection<CacheData>> stratifyCacheDataByGroup(
kp -> kp.key.getGroup(),
Collectors.mapping(kp -> kp.cacheData, Collectors.toCollection(ArrayList::new))));
}

/*
* Worth noting the strange behavior here. If we are inverting a relationship to create a cache data for
* either a cluster or an application we need to insert attributes to ensure the cache data gets entered into
* the cache. If we are caching anything else, we don't want competing agents to overwrite attributes, so
* we leave them blank.
*/
private static Optional<CacheData> invertSingleRelationship(
String group, String key, String relationship) {
Map<String, Collection<String>> relationships = new HashMap<>();
relationships.put(group, Collections.singletonList(key));
return Keys.parseKey(relationship)
.map(
k -> {
Map<String, Object> attributes;
int ttl;
if (Keys.LogicalKind.isLogicalGroup(k.getGroup())) {
ttl = logicalTtlSeconds;
attributes =
new ImmutableMap.Builder<String, Object>().put("name", k.getName()).build();
} else {
ttl = infrastructureTtlSeconds;
attributes = new HashMap<>();
}
return defaultCacheData(relationship, ttl, attributes, relationships);
});
}
}
5 changes: 1 addition & 4 deletions ...tflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesMetricCachingAgent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,7 @@ public CacheResult loadData(ProviderCache providerCache) {
.collect(Collectors.toList());

List<CacheData> invertedRelationships =
cacheData.stream()
.map(KubernetesCacheDataConverter::invertRelationships)
.flatMap(Collection::stream)
.collect(Collectors.toList());
KubernetesCacheDataConverter.invertRelationships(cacheData);

cacheData.addAll(invertedRelationships);

Expand Down
13 changes: 2 additions & 11 deletions ...m/netflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesV2CachingAgent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,7 @@ protected CacheResult buildCacheResult(Map<KubernetesKind, List<KubernetesManife
.filter(Objects::nonNull)
.collect(Collectors.toList());

List<CacheData> invertedRelationships =
resourceData.stream()
.map(KubernetesCacheDataConverter::invertRelationships)
.flatMap(Collection::stream)
.collect(Collectors.toList());
resourceData.addAll(KubernetesCacheDataConverter.invertRelationships(resourceData));

resourceData.addAll(
resources.values().stream()
Expand All @@ -196,13 +192,8 @@ protected CacheResult buildCacheResult(Map<KubernetesKind, List<KubernetesManife
.filter(Objects::nonNull)
.collect(Collectors.toList()));

resourceData.addAll(invertedRelationships);

resourceData.addAll(
resourceData.stream()
.map(rs -> KubernetesCacheDataConverter.getClusterRelationships(accountName, rs))
.filter(Objects::nonNull)
.collect(Collectors.toList()));
KubernetesCacheDataConverter.getClusterRelationships(accountName, resourceData));

Map<String, Collection<CacheData>> entries =
KubernetesCacheDataConverter.stratifyCacheDataByGroup(
Expand Down
43 changes: 37 additions & 6 deletions ...x/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCacheDataConvertSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,16 @@ metadata:
def cacheData = new DefaultCacheData(id, null, relationships)

when:
def result = KubernetesCacheDataConverter.invertRelationships(cacheData)
def result = KubernetesCacheDataConverter.invertRelationships([cacheData])

then:
relationships.collect {
group, keys -> keys.collect {
relationships.every {
group, keys -> keys.every {
key -> result.find {
data -> data.id == key && data.relationships.get(kind.toString()) == [id]
data -> data.id == key && data.relationships.get(kind.toString()) == [id] as Set
} != null
}.inject true, { a, b -> a && b }
}.inject true, { a, b -> a && b }
}
}

where:
kind | version | relationships
Expand All @@ -144,6 +144,37 @@ metadata:
KubernetesKind.SERVICE | KubernetesApiVersion.V1 | ["cluster": [Keys.cluster("account", "app", "name")], "application": [Keys.application("blarg"), Keys.application("asdfasdf")]]
}

@Unroll
def "given a cache data entry, determines cluster relationships"() {
setup:
def account = "account"
def application = "app"
def id = Keys.infrastructure(kind, account, "namespace", cluster)
def attributes = [
moniker: [
app: application,
cluster: cluster
]
]
def cacheData = new DefaultCacheData(id, attributes, [:])

when:
def result = KubernetesCacheDataConverter.getClusterRelationships(account, [cacheData])

then:
result.size() == 1
result[0].id == Keys.application(application)
result[0].relationships.clusters == [
Keys.cluster(account, application, cluster)
] as Set

where:
kind | cluster
KubernetesKind.REPLICA_SET | "my-app-321"
KubernetesKind.DEPLOYMENT | "my-app"
KubernetesKind.POD | "my-app-321-abcd"
}

@Unroll
def "correctly builds cache data entry for pod metrics"() {
setup:
Expand Down

0 comments on commit 00d249a

Please sign in to comment.