Skip to content

Commit

Permalink
perf(kubernetes): Improve performance of API group and version (#3831)
Browse files Browse the repository at this point in the history
* fix(kubernetes): Fix equality check in KubernetesKind

We're incorrectly comparing two objects using == instead of
.equals(). This is working currently because the we only ever
have one KubernetesApiGroup object for each class, but the next
commit will change that.

* perf(kubernetes): Improve performance of API group and version

These two classes each use a static variable to hold all instances
of the class that are ever created. Before creating a new instance
of the class, they check to see if there is already a matching
instance and if so return that instance instead of creating a
new one.

While this might be a reasonable pattern for objects that are expensive
to create, here the cost of keeping this object pool is extremely high
vs. the cost of just creating an object each time we need it.

Partly this is because we don't use an efficient data structure for
storing exiting objects, and need to loop over an array of all existing
objects each time we want to fetch/create a new one.  This is compounded
by the fact that that loop is in a synchronized block for thread safety.

While we could make this object pool more efficient by using a better
data structure and handling synchronization better, as noted above these
objects are so cheap to create that it's not worth the effort.

Also add tests to the class as part of this refactor.
  • Loading branch information
ezimanyi authored Jul 1, 2019
1 parent 5f4ee56 commit 80f9fc6
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 73 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,43 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;
import com.google.common.collect.ImmutableSet;
import java.util.Collections;
import java.util.Map;
import java.util.TreeMap;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import lombok.EqualsAndHashCode;
import org.apache.commons.lang3.StringUtils;

@EqualsAndHashCode
public class KubernetesApiGroup {
private static final Map<String, KubernetesApiGroup> values =
Collections.synchronizedMap(new TreeMap<>(String.CASE_INSENSITIVE_ORDER));
// from https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.12/
public static KubernetesApiGroup NONE = new KubernetesApiGroup("");
public static KubernetesApiGroup CORE = new KubernetesApiGroup("core");
public static KubernetesApiGroup BATCH = new KubernetesApiGroup("batch");
public static KubernetesApiGroup APPS = new KubernetesApiGroup("apps");
public static KubernetesApiGroup EXTENSIONS = new KubernetesApiGroup("extensions");
public static KubernetesApiGroup STORAGE_K8S_IO = new KubernetesApiGroup("storage.k8s.io");
public static KubernetesApiGroup APIEXTENSIONS_K8S_IO =
public static final KubernetesApiGroup NONE = new KubernetesApiGroup("");
public static final KubernetesApiGroup CORE = new KubernetesApiGroup("core");
public static final KubernetesApiGroup BATCH = new KubernetesApiGroup("batch");
public static final KubernetesApiGroup APPS = new KubernetesApiGroup("apps");
public static final KubernetesApiGroup EXTENSIONS = new KubernetesApiGroup("extensions");
public static final KubernetesApiGroup STORAGE_K8S_IO = new KubernetesApiGroup("storage.k8s.io");
public static final KubernetesApiGroup APIEXTENSIONS_K8S_IO =
new KubernetesApiGroup("apiextensions.k8s.io");
public static KubernetesApiGroup APIREGISTRATION_K8S_IO =
public static final KubernetesApiGroup APIREGISTRATION_K8S_IO =
new KubernetesApiGroup("apiregistration.k8s.io");
public static KubernetesApiGroup AUTOSCALING = new KubernetesApiGroup("autoscaling");
public static KubernetesApiGroup ADMISSIONREGISTRATION_K8S_IO =
public static final KubernetesApiGroup AUTOSCALING = new KubernetesApiGroup("autoscaling");
public static final KubernetesApiGroup ADMISSIONREGISTRATION_K8S_IO =
new KubernetesApiGroup("admissionregistration.k8s.io");
public static KubernetesApiGroup POLICY = new KubernetesApiGroup("policy");
public static KubernetesApiGroup SCHEDULING_K8S_IO = new KubernetesApiGroup("scheduling.k8s.io");
public static KubernetesApiGroup SETTINGS_K8S_IO = new KubernetesApiGroup("settings.k8s.io");
public static KubernetesApiGroup AUTHORIZATION_K8S_IO =
public static final KubernetesApiGroup POLICY = new KubernetesApiGroup("policy");
public static final KubernetesApiGroup SCHEDULING_K8S_IO =
new KubernetesApiGroup("scheduling.k8s.io");
public static final KubernetesApiGroup SETTINGS_K8S_IO =
new KubernetesApiGroup("settings.k8s.io");
public static final KubernetesApiGroup AUTHORIZATION_K8S_IO =
new KubernetesApiGroup("authorization.k8s.io");
public static KubernetesApiGroup AUTHENTICATION_K8S_IO =
public static final KubernetesApiGroup AUTHENTICATION_K8S_IO =
new KubernetesApiGroup("authentication.k8s.io");
public static KubernetesApiGroup RBAC_AUTHORIZATION_K8S_IO =
public static final KubernetesApiGroup RBAC_AUTHORIZATION_K8S_IO =
new KubernetesApiGroup("rbac.authorization.k8s.io");
public static KubernetesApiGroup CERTIFICATES_K8S_IO =
public static final KubernetesApiGroup CERTIFICATES_K8S_IO =
new KubernetesApiGroup("certificates.k8s.io");
public static KubernetesApiGroup NETWORKING_K8S_IO = new KubernetesApiGroup("networking.k8s.io");
public static final KubernetesApiGroup NETWORKING_K8S_IO =
new KubernetesApiGroup("networking.k8s.io");

private final String name;
@Nonnull private final String name;

// including NONE since it seems like any resource without an api group would have to be native
private static final ImmutableSet<KubernetesApiGroup> NATIVE_GROUPS =
Expand All @@ -64,9 +63,8 @@ public class KubernetesApiGroup {
NETWORKING_K8S_IO,
NONE);

protected KubernetesApiGroup(String name) {
this.name = name;
values.put(name, this);
private KubernetesApiGroup(@Nonnull String name) {
this.name = name.toLowerCase();
}

@Override
Expand All @@ -80,13 +78,11 @@ public boolean isNativeGroup() {
}

@JsonCreator
public static KubernetesApiGroup fromString(String name) {
if (StringUtils.isEmpty(name)) {
return null;
}

synchronized (values) {
return values.computeIfAbsent(name, KubernetesApiGroup::new);
@Nonnull
public static KubernetesApiGroup fromString(@Nullable String name) {
if (name == null) {
return KubernetesApiGroup.NONE;
}
return new KubernetesApiGroup(name);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,31 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import lombok.EqualsAndHashCode;
import org.apache.commons.lang3.StringUtils;
import lombok.Getter;

@EqualsAndHashCode
public class KubernetesApiVersion {
public static KubernetesApiVersion V1 = new KubernetesApiVersion("v1");
public static KubernetesApiVersion EXTENSIONS_V1BETA1 =
public static final KubernetesApiVersion V1 = new KubernetesApiVersion("v1");
public static final KubernetesApiVersion EXTENSIONS_V1BETA1 =
new KubernetesApiVersion("extensions/v1beta1");
public static KubernetesApiVersion NETWORKING_K8S_IO_V1 =
public static final KubernetesApiVersion NETWORKING_K8S_IO_V1 =
new KubernetesApiVersion("network.k8s.io/v1");
public static KubernetesApiVersion NETWORKING_K8S_IO_V1BETA1 =
public static final KubernetesApiVersion NETWORKING_K8S_IO_V1BETA1 =
new KubernetesApiVersion("network.k8s.io/v1beta1");
public static KubernetesApiVersion APPS_V1BETA1 = new KubernetesApiVersion("apps/v1beta1");
public static KubernetesApiVersion APPS_V1BETA2 = new KubernetesApiVersion("apps/v1beta2");
public static KubernetesApiVersion BATCH_V1 = new KubernetesApiVersion("batch/v1");
public static final KubernetesApiVersion APPS_V1BETA1 = new KubernetesApiVersion("apps/v1beta1");
public static final KubernetesApiVersion APPS_V1BETA2 = new KubernetesApiVersion("apps/v1beta2");
public static final KubernetesApiVersion BATCH_V1 = new KubernetesApiVersion("batch/v1");
public static final KubernetesApiVersion NONE = new KubernetesApiVersion("");

private final String name;
@Nonnull private final String name;
@Getter @Nonnull @EqualsAndHashCode.Exclude private final KubernetesApiGroup apiGroup;

private static List<KubernetesApiVersion> values;

protected KubernetesApiVersion(String name) {
if (values == null) {
values = Collections.synchronizedList(new ArrayList<>());
}

this.name = name;
values.add(this);
private KubernetesApiVersion(@Nonnull String name) {
this.name = name.toLowerCase();
this.apiGroup = parseApiGroup(this.name);
}

@Override
Expand All @@ -58,26 +52,21 @@ public String toString() {
return name;
}

public KubernetesApiGroup getApiGroup() {
final String[] split = name.split("/");
if (split.length > 1) {
return KubernetesApiGroup.fromString(split[0]);
@Nonnull
private static KubernetesApiGroup parseApiGroup(@Nonnull String name) {
int index = name.indexOf('/');
if (index > 0) {
return KubernetesApiGroup.fromString(name.substring(0, index));
}
return KubernetesApiGroup.NONE;
}

@JsonCreator
public static KubernetesApiVersion fromString(String name) {
if (StringUtils.isEmpty(name)) {
return null;
}

synchronized (values) {
Optional<KubernetesApiVersion> versionOptional =
values.stream().filter(v -> v.name.equalsIgnoreCase(name)).findAny();

// separate from the above chain to avoid concurrent modification of the values list
return versionOptional.orElseGet(() -> new KubernetesApiVersion(name));
@Nonnull
public static KubernetesApiVersion fromString(@Nullable String name) {
if (name == null) {
return KubernetesApiVersion.NONE;
}
return new KubernetesApiVersion(name);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -204,7 +205,7 @@ public static KubernetesKind getOrRegisterKind(
Predicate<KubernetesKind> groupMatches =
kind -> {
// Exact match
if (kind.apiGroup == apiGroup) {
if (Objects.equals(kind.apiGroup, apiGroup)) {
return true;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2019 Google, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License")
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.clouddriver.kubernetes.v2.description

import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiGroup
import spock.lang.Specification
import spock.lang.Unroll

class KubernetesApiGroupSpec extends Specification {
@Unroll
void "creates built-in API groups by name"() {
when:
def apiGroup = KubernetesApiGroup.fromString(name)

then:
apiGroup.equals(expectedApiGroup)

where:
name | expectedApiGroup
null | KubernetesApiGroup.NONE
"" | KubernetesApiGroup.NONE
"batch" | KubernetesApiGroup.BATCH
"BATCH" | KubernetesApiGroup.BATCH
"settings.k8s.io" | KubernetesApiGroup.SETTINGS_K8S_IO
"seTtiNgs.k8S.IO" | KubernetesApiGroup.SETTINGS_K8S_IO
}

@Unroll
void "creates custom API groups"() {
when:
def apiGroup = KubernetesApiGroup.fromString(name)

then:
noExceptionThrown()
apiGroup.toString() == expectedName

where:
name | expectedName
"test.api.group" | "test.api.group"
"TEST.api.Group" | "test.api.group"
}

@Unroll
void "returns whether an API group is a native group"() {
when:
def apiGroup = KubernetesApiGroup.fromString(name)

then:
apiGroup.isNativeGroup() == isNative

where:
name | isNative
"test.api.group" | false
"batch" | true
"apps" | true
"" | true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright 2019 Google, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License")
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.clouddriver.kubernetes.v2.description

import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiGroup
import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiVersion
import spock.lang.Specification
import spock.lang.Unroll

class KubernetesApiVersionSpec extends Specification {
@Unroll
void "creates built-in API versions by name"() {
when:
def apiVersion = KubernetesApiVersion.fromString(name)

then:
apiVersion.equals(expectedApiGroup)

where:
name | expectedApiGroup
null | KubernetesApiVersion.NONE
"" | KubernetesApiVersion.NONE
"v1" | KubernetesApiVersion.V1
"network.k8s.io/v1beta1" | KubernetesApiVersion.NETWORKING_K8S_IO_V1BETA1
"neTwoRk.k8s.io/v1beTA1" | KubernetesApiVersion.NETWORKING_K8S_IO_V1BETA1
}

@Unroll
void "creates custom API versions"() {
when:
def apiVersion = KubernetesApiVersion.fromString(name)

then:
noExceptionThrown()
apiVersion.toString() == expectedName

where:
name | expectedName
"test.api.group" | "test.api.group"
"test.api.group/version" | "test.api.group/version"
}

@Unroll
void "correctly parses the group from the version"() {
when:
def apiVersion = KubernetesApiVersion.fromString(name)

then:
apiVersion.getApiGroup().equals(expectedGroup)

where:
name | expectedGroup
null | KubernetesApiGroup.NONE
"" | KubernetesApiGroup.NONE
"test.api.group" | KubernetesApiGroup.NONE
"test.api.group/version" | KubernetesApiGroup.fromString("test.api.group")
"apps/v1beta1" | KubernetesApiGroup.APPS
}
}

0 comments on commit 80f9fc6

Please sign in to comment.