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

Store the container-machine mapping predictably #13858

Merged
merged 7 commits into from
Jul 24, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ public final class Constants {
/** The label that contains a value with volume name. */
public static final String CHE_VOLUME_NAME_LABEL = "che.workspace.volume_name";

/**
* The annotation with the wildcard reserved for container name. Formatted annotation with the
* real container name is used to get machine name.
*/
public static final String MACHINE_NAME_ANNOTATION_FMT =
"org.eclipse.che.container.%s.machine_name";

/** Kubernetes Pod status phase values */
public static final String POD_STATUS_PHASE_RUNNING = "Running";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@

import static java.lang.String.format;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.CHE_ORIGINAL_NAME_LABEL;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;

import com.google.common.collect.Maps;
import io.fabric8.kubernetes.api.model.Container;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.api.model.Pod;
import java.util.HashMap;
import java.util.Map;
import org.eclipse.che.commons.lang.NameGenerator;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment.PodData;
import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesObjectUtil;

/**
* Helps to work with Kubernetes objects names.
Expand All @@ -30,18 +29,14 @@
*/
public class Names {

static final char WORKSPACE_ID_PREFIX_SEPARATOR = '.';
public static final int MAX_CONTAINER_NAME_LENGTH = 63; // K8S container name limit

static final int GENERATED_PART_SIZE = 8;
private static final char WORKSPACE_ID_PREFIX_SEPARATOR = '.';

/**
* Returns machine name for the specified container in the specified pod.
*
* <p>This is a convenience method for {@link #machineName(ObjectMeta, Container)}
*/
public static String machineName(Pod pod, Container container) {
return machineName(pod.getMetadata(), container);
}
private static final int GENERATED_PART_SIZE = 8;

private static final String CONTAINER_DISPLAY_NAMES_FMT =
"org.eclipse.che.container.display-name/%s";

/**
* Returns machine name for the specified container in the specified pod.
Expand All @@ -52,10 +47,60 @@ public static String machineName(PodData podData, Container container) {
return machineName(podData.getMetadata(), container);
}

/**
* Records the machine name used for given container in the annotations of the object metadata.
*
* @param objectMeta the object metadata
* @param containerName the name of the container
* @param machineName the name of the machine of the container
*/
public static void putMachineName(
ObjectMeta objectMeta, String containerName, String machineName) {
KubernetesObjectUtil.putAnnotation(
objectMeta, format(MACHINE_NAME_ANNOTATION_FMT, containerName), machineName);

Map<String, String> annotations = objectMeta.getAnnotations();
if (annotations == null) {
objectMeta.setAnnotations(annotations = new HashMap<>());
}

putMachineName(annotations, containerName, machineName);
}

/**
* Transforms the provided mapping of container names to machine names into a map of annotations
* to be put in some Kubernetes object's metadata.
*
* @param containerToMachineNames the mapping of container names to machine names
* @return a map of annotations
*/
public static Map<String, String> createMachineNameAnnotations(
Map<String, String> containerToMachineNames) {

Map<String, String> ret = new HashMap<>();

containerToMachineNames.forEach((c, m) -> putMachineName(ret, c, m));

return ret;
}

/**
* Similar to {@link #createMachineNameAnnotations(Map)} but only creates annotations for a single
* mapping.
*
* @param containerName the name of the container
* @param machineName the name of the machine
* @return a map of annotations for the container-machine mapping
* @see #createMachineNameAnnotations(Map)
*/
public static Map<String, String> createMachineNameAnnotations(
String containerName, String machineName) {
Map<String, String> ret = Maps.newHashMapWithExpectedSize(2);
putMachineName(ret, containerName, machineName);
return ret;
}

private static void putMachineName(
Map<String, String> annotations, String containerName, String machineName) {
annotations.put(format(CONTAINER_DISPLAY_NAMES_FMT, containerName), machineName);
}

/**
Expand All @@ -75,8 +120,16 @@ public static String machineName(ObjectMeta podMeta, Container container) {
final Map<String, String> annotations = podMeta.getAnnotations();
final String machineName;
final String containerName = container.getName();

if (containerName != null && containerName.length() > MAX_CONTAINER_NAME_LENGTH) {
throw new IllegalArgumentException(
format(
"The container name exceeds the allowed limit of %s characters.",
MAX_CONTAINER_NAME_LENGTH));
}

if (annotations != null
&& (machineName = annotations.get(format(MACHINE_NAME_ANNOTATION_FMT, containerName)))
&& (machineName = annotations.get(format(CONTAINER_DISPLAY_NAMES_FMT, containerName)))
!= null) {
return machineName;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import static org.eclipse.che.api.workspace.server.devfile.Constants.DOCKERIMAGE_COMPONENT_TYPE;
import static org.eclipse.che.api.workspace.server.devfile.Constants.PUBLIC_ENDPOINT_ATTRIBUTE;
import static org.eclipse.che.api.workspace.shared.Constants.PROJECTS_VOLUME_NAME;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -51,6 +50,7 @@
import org.eclipse.che.api.workspace.server.model.impl.ServerConfigImpl;
import org.eclipse.che.api.workspace.server.model.impl.VolumeImpl;
import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
import org.eclipse.che.workspace.infrastructure.kubernetes.util.Containers;

Expand Down Expand Up @@ -202,7 +202,7 @@ private Deployment buildDeployment(
.withNewMetadata()
.withName(name)
.addToLabels(CHE_COMPONENT_NAME_LABEL, name)
.addToAnnotations(format(MACHINE_NAME_ANNOTATION_FMT, name), name)
.addToAnnotations(Names.createMachineNameAnnotations(name, name))
.endMetadata()
.withNewSpec()
.withContainers(container)
Expand Down Expand Up @@ -257,6 +257,14 @@ static String toMachineName(String imageName) throws DevfileException {
return imageName;
}

if (imageName.length() > Names.MAX_CONTAINER_NAME_LENGTH) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it something that can be checked by JSON Schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we can't really restrict the names of the images. What would have to happen is conditional check that would mark alias required if the length of the image name would be > 63. Maybe it is doable in the schema but we have to have the check in the code anyway. So for simplicity's sake I only did it in the code. I believe the PR is writable by contributors so please add the schema changes if required (I don't have access to my computer this week).

throw new DevfileException(
format(
"The image name '%s' is longer than 63 characters and as such cannot be used as a container"
+ " name. Please provide an alias for the component with that image.",
imageName));
}

// the name needs to be both a valid k8s label and a valid machine name.
String clean = imageName.replaceAll("[^-a-zA-Z0-9_]", "-");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
package org.eclipse.che.workspace.infrastructure.kubernetes.environment;

import static java.lang.String.format;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;

import io.fabric8.kubernetes.api.model.Container;
import io.fabric8.kubernetes.api.model.LocalObjectReference;
Expand Down Expand Up @@ -85,11 +84,7 @@ public Deployment merge(List<PodData> podsData) throws ValidationException {
}

// store original recipe machine name
basePodMeta
.getAnnotations()
.put(
format(MACHINE_NAME_ANNOTATION_FMT, containerName),
Names.machineName(podMeta, container));
Names.putMachineName(basePodMeta, containerName, Names.machineName(podMeta, container));

baseSpec.getContainers().add(container);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,17 @@
*/
package org.eclipse.che.workspace.infrastructure.kubernetes.environment.convert;

import static java.lang.String.format;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;

import com.google.common.collect.ImmutableMap;
import io.fabric8.kubernetes.api.model.ContainerBuilder;
import io.fabric8.kubernetes.api.model.Pod;
import io.fabric8.kubernetes.api.model.PodBuilder;
import java.util.HashMap;
import java.util.Map;
import javax.inject.Inject;
import javax.inject.Singleton;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
import org.eclipse.che.workspace.infrastructure.docker.environment.dockerimage.DockerImageEnvironment;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.util.EntryPoint;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.util.EntryPointParser;
Expand Down Expand Up @@ -67,14 +64,11 @@ public KubernetesEnvironment convert(DockerImageEnvironment environment)

applyEntryPoint(machine, container);

final Map<String, String> annotations = new HashMap<>();
annotations.put(format(MACHINE_NAME_ANNOTATION_FMT, CONTAINER_NAME), machineName);

final Pod pod =
new PodBuilder()
.withNewMetadata()
.withName(POD_NAME)
.withAnnotations(annotations)
.withAnnotations(Names.createMachineNameAnnotations(CONTAINER_NAME, machineName))
.endMetadata()
.withNewSpec()
.withContainers(container.build())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.eclipse.che.api.workspace.server.wsplugins.model.ChePluginEndpoint;
import org.eclipse.che.api.workspace.server.wsplugins.model.EnvVar;
import org.eclipse.che.commons.lang.NameGenerator;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.util.Containers;
import org.eclipse.che.workspace.infrastructure.kubernetes.util.KubernetesSize;

Expand All @@ -38,8 +39,6 @@
*/
public class K8sContainerResolver {

static final int MAX_CONTAINER_NAME_LENGTH = 63; // K8S container name limit

private final String imagePullPolicy;
private final CheContainer cheContainer;
private final List<ChePluginEndpoint> containerEndpoints;
Expand Down Expand Up @@ -111,6 +110,6 @@ private List<io.fabric8.kubernetes.api.model.EnvVar> toK8sEnv(List<EnvVar> env)

private String buildContainerName(String cheContainerName) {
String uniqueName = NameGenerator.generate(cheContainerName, 3).toLowerCase();
return uniqueName.substring(0, min(uniqueName.length(), MAX_CONTAINER_NAME_LENGTH));
return uniqueName.substring(0, min(uniqueName.length(), Names.MAX_CONTAINER_NAME_LENGTH));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ private BrokerConfig createBrokerConfig(
configMapVolume = brokerConfig.configMapVolume;
}
brokerConfig.container = newContainer(runtimeId, envVars, image, configMapVolume);
brokerConfig.machineName = Names.machineName(pod, brokerConfig.container);
brokerConfig.machineName = Names.machineName(pod.getMetadata(), brokerConfig.container);
brokerConfig.machineConfig = new InternalMachineConfig();
brokerConfig
.machineConfig
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/
package org.eclipse.che.workspace.infrastructure.kubernetes;

import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Names.createMachineNameAnnotations;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.testng.Assert.assertEquals;
Expand Down Expand Up @@ -59,17 +59,15 @@ public class KubernetesBrokerInitContainerApplierTest {
private static final String WORKSPACE_MACHINE_NAME = "workspaceMachine";
private static final String WORKSPACE_CONTAINER_NAME = "workspaceContainer";
private static final Map<String, String> workspacePodAnnotations =
ImmutableMap.of(
String.format(MACHINE_NAME_ANNOTATION_FMT, WORKSPACE_CONTAINER_NAME),
WORKSPACE_MACHINE_NAME);
createMachineNameAnnotations(WORKSPACE_CONTAINER_NAME, WORKSPACE_MACHINE_NAME);

private static final String BROKER_POD_NAME = "brokerPod";
private static final String BROKER_MACHINE_NAME = "brokerMachine";
private static final String BROKER_CONTAINER_NAME = "brokerContainer";
private static final String BROKER_CONFIGMAP_NAME = "brokerConfigMap";
private static final Map<String, String> brokerPodAnnotations =
ImmutableMap.of(
String.format(MACHINE_NAME_ANNOTATION_FMT, BROKER_CONTAINER_NAME), BROKER_MACHINE_NAME);
createMachineNameAnnotations(BROKER_CONTAINER_NAME, BROKER_MACHINE_NAME);

private static final Map<String, String> brokerConfigMapData =
ImmutableMap.of("brokerConfigKey", "brokerConfigValue");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static org.eclipse.che.api.workspace.server.devfile.Constants.DOCKERIMAGE_COMPONENT_TYPE;
import static org.eclipse.che.api.workspace.server.devfile.Constants.PUBLIC_ENDPOINT_ATTRIBUTE;
import static org.eclipse.che.api.workspace.shared.Constants.PROJECTS_VOLUME_NAME;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;
import static org.eclipse.che.workspace.infrastructure.kubernetes.devfile.DockerimageComponentToWorkspaceApplier.CHE_COMPONENT_NAME_LABEL;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
Expand Down Expand Up @@ -54,6 +53,7 @@
import org.eclipse.che.api.workspace.server.model.impl.devfile.VolumeImpl;
import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
import org.eclipse.che.api.workspace.server.spi.environment.MachineConfigsValidator;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
Expand Down Expand Up @@ -122,12 +122,11 @@ public void setUp() throws Exception {
assertFalse(deploymentSelector.isEmpty());
assertTrue(podMeta.getLabels().entrySet().containsAll(deploymentSelector.entrySet()));

Map<String, String> annotations = podMeta.getAnnotations();
assertEquals(annotations.get(String.format(MACHINE_NAME_ANNOTATION_FMT, "jdk")), "jdk");

Container container = podTemplate.getSpec().getContainers().get(0);
assertEquals(container.getName(), "jdk");
assertEquals(container.getImage(), "eclipse/ubuntu_jdk8:latest");

assertEquals(Names.machineName(podMeta, container), "jdk");
}

@Test
Expand Down Expand Up @@ -163,14 +162,12 @@ public void shouldProvisionK8sEnvironmentWithMachineConfigFromSpecifiedDockerima
assertFalse(deploymentSelector.isEmpty());
assertTrue(podMeta.getLabels().entrySet().containsAll(deploymentSelector.entrySet()));

Map<String, String> annotations = podMeta.getAnnotations();
assertEquals(
annotations.get(String.format(MACHINE_NAME_ANNOTATION_FMT, "eclipse-ubuntu_jdk8-latest")),
"eclipse-ubuntu_jdk8-latest");

Container container = podTemplate.getSpec().getContainers().get(0);
assertEquals(container.getName(), "eclipse-ubuntu_jdk8-latest");
assertEquals(container.getImage(), "eclipse/ubuntu_jdk8:latest");

assertEquals(
Names.machineName(podTemplate.getMetadata(), container), "eclipse-ubuntu_jdk8-latest");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,10 @@
*/
package org.eclipse.che.workspace.infrastructure.kubernetes.environment;

import static java.lang.String.format;
import static java.util.Arrays.asList;
import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonList;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.INGRESSES_IGNORED_WARNING_CODE;
import static org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.INGRESSES_IGNORED_WARNING_MESSAGE;
import static org.eclipse.che.workspace.infrastructure.kubernetes.environment.PodMerger.DEPLOYMENT_NAME_LABEL;
Expand Down Expand Up @@ -64,6 +62,7 @@
import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
import org.eclipse.che.api.workspace.server.spi.environment.InternalRecipe;
import org.eclipse.che.api.workspace.server.spi.environment.MemoryAttributeProvisioner;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment.PodData;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
Expand Down Expand Up @@ -476,8 +475,7 @@ private static PodData createPodData(String machineName, long ramLimit, long ram
when(containerMock.getName()).thenReturn(containerName);
when(containerMock.getResources()).thenReturn(resourcesMock);
when(metadataMock.getAnnotations())
.thenReturn(
ImmutableMap.of(format(MACHINE_NAME_ANNOTATION_FMT, containerName), machineName));
.thenReturn(Names.createMachineNameAnnotations(containerName, machineName));
when(specMock.getContainers()).thenReturn(ImmutableList.of(containerMock));
return new PodData(specMock, metadataMock);
}
Expand Down
Loading