Skip to content
This repository has been archived by the owner on Jun 19, 2024. It is now read-only.

Issue#968 - Multi Container Support with Resource Fragments #969

Closed
wants to merge 4 commits into from
Closed
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 @@ -719,9 +719,20 @@ public static void mergePodSpec(PodSpecBuilder builder, PodSpec defaultPodSpec,
int idx = 0;
for (Container defaultContainer : defaultContainers) {
Container container;
boolean isNewContainerAdded = false;
if (idx < containers.size()) {
container = containers.get(idx);
} else {
isNewContainerAdded = true;
container = new Container();
containers.add(container);
}
//TODO: SK remove it after review
//Though we have more containers in the builder, but if existing one does not match
// default container name then we assume it to be new one or the one to be added
String containerName = container.getImage();
String defaultContainerName = defaultContainer.getImage();
if (!containerName.equalsIgnoreCase(defaultContainerName) && !isNewContainerAdded) {
Copy link
Member

@nicolaferraro nicolaferraro Jul 4, 2017

Choose a reason for hiding this comment

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

Here you're correlating over the image name. This works with sidecars, but it may have problems with pods having multiple containers from the same base image (with different startup options).

I think a better approach would be to correlate over the container name, i.e.

  • If a fragment has a name, and the name is different the one that would be used for the current project, then add another container
  • If the fragment does not declare a name, or the name matches the one that would be used for the current project, then use that as container for the project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolaferraro - do you mean this

...
for (Container defaultContainer : defaultContainers) {
                    Container container;
                    boolean isNewContainerAdded = false;
                    if (idx < containers.size()) {
                        container = containers.get(idx);
                    } else {
                        isNewContainerAdded = true;
                        container = new Container();
                        containers.add(container);
                    }
                    String containerName = container.getName();
                    String defaultContainerName = defaultContainer.getName() == null?defaultName:defaultContainer.getName();
                    **if (!defaultContainerName.equalsIgnoreCase(containerName) && !isNewContainerAdded)** {
                        container = new Container();
                        containers.add(container);
                    }
                    mergeSimpleFields(container, defaultContainer);
 ....

Copy link
Member

Choose a reason for hiding this comment

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

Something like that @kameshsampath, but I see that code adds another container if the fragment does not declare a name (that is the standard case when adding fragments). I'd add another container only if the one declared in the fragments uses a name that is different from the default container name for the project (e.g. it's called proxy). Please, also fix the dependencies of the 'it' tests as they refer snapshot artifacts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolaferraro - am not getting what you mean " I see that code adds another container if the fragment does not declare a name" - can you please point to the line# in the code for it ?

container = new Container();
containers.add(container);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
package io.fabric8.maven.enricher.api;

import io.fabric8.kubernetes.api.builder.TypedVisitor;
import io.fabric8.kubernetes.api.model.ContainerBuilder;
import io.fabric8.kubernetes.api.model.KubernetesListBuilder;
import io.fabric8.kubernetes.api.model.Probe;
import io.fabric8.kubernetes.api.model.*;
import io.fabric8.kubernetes.api.model.extensions.Deployment;
import io.fabric8.maven.core.util.Configs;

import java.util.List;

/**
* Enriches containers with health check probes.
Expand All @@ -30,25 +32,67 @@ public AbstractHealthCheckEnricher(EnricherContext buildContext, String name) {
super(buildContext, name);
}

// Available configuration keys
protected enum Config implements Configs.Key {

probeMode {{ d = "all";}};
Copy link
Member

Choose a reason for hiding this comment

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

We can decide to enrich all containers here, or we can rely here on the concept of "project's main container", or list the containers that should be enriched using a configuration option (also a global one if this enricher is not the only one that should be scoped to a specific container). I'd not go for "first" / "last" as it relies too much on what the current implementation does but it may change in the future (with plugin updates or with "fragments" added in other ways e.g. #941).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolaferraro i agree with you on the first/last/all, how does something like this

<config>
  <probes>
       <container-name-1/>
       <container-name-2/>
 </probes>
</config> 

when the probes is empty then we assume "all" and add probes to all containers

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep the configuration inside the enricher config, but a list of container names can be used (I don't know if the configuration utils support lists as you have declared them in the example, there should be other cases of list configs). About the default when no configuration is given: both 'all containers' or 'just the one having the default name' are reasonable in my opinion.


protected String d;

public String def() {
return d;
}
}

@Override
public void addMissingResources(KubernetesListBuilder builder) {

final List<HasMetadata> buildItems = builder.buildItems();

builder.accept(new TypedVisitor<ContainerBuilder>() {
@Override
public void visit(ContainerBuilder container) {
if (!container.hasReadinessProbe()) {
Probe probe = getReadinessProbe(container);
if (probe != null) {
log.info("Adding readiness " + describe(probe));
container.withReadinessProbe(probe);
Container matchedContainer = null;
int idx = 0;

// by default true - as probeMode will be all containers
boolean matches = true;

//do probe mode check only for first and last probe modes
if ("first".equalsIgnoreCase(getConfig(Config.probeMode)) ||
"last".equalsIgnoreCase(getConfig(Config.probeMode))) {
for (HasMetadata buildItem : buildItems) {
//TODO: SK check with ROL on whether kind to be for Replication Controllers as well??
if ("Deployment".equals(buildItem.getKind())) {
Deployment deployment = (Deployment) buildItem;
List<Container> containers = deployment.getSpec().getTemplate().getSpec().getContainers();
if ("first".equalsIgnoreCase(getConfig(Config.probeMode))) {
idx = 0;
} else if ("last".equalsIgnoreCase(getConfig(Config.probeMode))) {
idx = containers.size() - 1;
}
matchedContainer = containers.get(idx);
}
}
matches = matchedContainer != null &&
container.build().getImage().equals(matchedContainer.getImage());
}

if (!container.hasLivenessProbe()) {
Probe probe = getLivenessProbe(container);
if (probe != null) {
log.info("Adding liveness " + describe(probe));
container.withLivenessProbe(probe);
if (matches) {
if (!container.hasReadinessProbe()) {
Probe probe = getReadinessProbe(container);
if (probe != null) {
log.info("Adding readiness " + describe(probe));
container.withReadinessProbe(probe);
}
}

if (!container.hasLivenessProbe()) {
Probe probe = getLivenessProbe(container);
if (probe != null) {
log.info("Adding liveness " + describe(probe));
container.withLivenessProbe(probe);
}
}
}
}
Expand Down
1 change: 0 additions & 1 deletion enricher/fabric8/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@
<groupId>com.jayway.jsonpath</groupId>
<artifactId>json-path-assert</artifactId>
</dependency>

</dependencies>

</project>
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.jayway.jsonpath.matchers.JsonPathMatchers;
import io.fabric8.kubernetes.api.model.KubernetesList;
import io.fabric8.kubernetes.api.model.KubernetesListBuilder;
import io.fabric8.kubernetes.api.model.extensions.Deployment;
import io.fabric8.maven.core.config.ProcessorConfig;
import io.fabric8.maven.core.util.KubernetesResourceUtil;
import io.fabric8.maven.docker.config.BuildImageConfiguration;
Expand All @@ -37,8 +38,7 @@
import java.util.Collections;
import java.util.TreeMap;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.*;

/**
* @author kamesh
Expand Down Expand Up @@ -66,6 +66,53 @@ public void checkDefaultReplicaCount() throws Exception {
enrichAndAssert(1, 1);
}

@Test
public void checkMultipleDeployments() throws Exception {

final BuildImageConfiguration buildConfig =
new BuildImageConfiguration.Builder()
.ports(Arrays.asList("8080"))
.build();

final ImageConfiguration imageConfiguration1 = new ImageConfiguration.Builder()
.buildConfig(buildConfig)
.alias("img-1")
.name("img-1")
.build();

final BuildImageConfiguration buildConfig2 =
new BuildImageConfiguration.Builder()
.ports(Arrays.asList("8080"))
.build();

final ImageConfiguration imageConfiguration2 = new ImageConfiguration.Builder()
.buildConfig(buildConfig2)
.alias("img-2")
.name("img-2")
.build();


final TreeMap controllerConfig = new TreeMap();
controllerConfig.put("replicaCount", String.valueOf(1));

multipleDeploymentExpectations(buildConfig, imageConfiguration1, buildConfig2, imageConfiguration2, controllerConfig);

// Enrich
KubernetesList list = enrichAndBuild();

assertEquals(1, list.getItems().size());

String json = KubernetesResourceUtil.toJson(list.getItems().get(0));
assertThat(json, JsonPathMatchers.isJson());
assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.replicas", Matchers.equalTo(1)));

Deployment deployment = (Deployment) list.getItems().get(0);
assertNotNull(deployment);
assertEquals(2, deployment.getSpec().getTemplate().getSpec().getContainers().size());

}


protected void enrichAndAssert(int sizeOfObjects, int replicaCount) throws com.fasterxml.jackson.core.JsonProcessingException {
// Setup a sample docker build configuration
final BuildImageConfiguration buildConfig =
Expand All @@ -77,18 +124,62 @@ protected void enrichAndAssert(int sizeOfObjects, int replicaCount) throws com.f
controllerConfig.put("replicaCount", String.valueOf(replicaCount));

setupExpectations(buildConfig, controllerConfig);
KubernetesList list = enrichAndBuild();


assertEquals(sizeOfObjects, list.getItems().size());

String json = KubernetesResourceUtil.toJson(list.getItems().get(0));
assertThat(json, JsonPathMatchers.isJson());
assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.replicas", Matchers.equalTo(replicaCount)));

}

private KubernetesList enrichAndBuild() {
// Enrich
DefaultControllerEnricher controllerEnricher = new DefaultControllerEnricher(context);
KubernetesListBuilder builder = new KubernetesListBuilder();
controllerEnricher.addMissingResources(builder);

// Validate that the generated resource contains
KubernetesList list = builder.build();
assertEquals(sizeOfObjects, list.getItems().size());
return builder.build();
}

String json = KubernetesResourceUtil.toJson(list.getItems().get(0));
assertThat(json, JsonPathMatchers.isJson());
assertThat(json, JsonPathMatchers.hasJsonPath("$.spec.replicas", Matchers.equalTo(replicaCount)));
private void multipleDeploymentExpectations(final BuildImageConfiguration buildConfig,
final ImageConfiguration imageConfiguration1,
final BuildImageConfiguration buildConfig2,
final ImageConfiguration imageConfiguration2,
final TreeMap controllerConfig) {
new Expectations() {{

project.getArtifactId();
result = "fmp-controller-test";

project.getBuild().getOutputDirectory();
result = Files.createTempDir().getAbsolutePath();

context.getProject();
result = project;

context.getConfig();
result = new ProcessorConfig(null, null,
Collections.singletonMap("fmp-controller", controllerConfig));

imageConfiguration1.getBuildConfiguration();
result = buildConfig;

imageConfiguration1.getName();
result = "img-1";

imageConfiguration2.getBuildConfiguration();
result = buildConfig2;

imageConfiguration2.getName();
result = "img-2";

context.getImages();
result = Arrays.asList(imageConfiguration1, imageConfiguration2);
}};
}

protected void setupExpectations(final BuildImageConfiguration buildConfig, final TreeMap controllerConfig) {
Expand Down
137 changes: 137 additions & 0 deletions it/src/it/hello-world-probemode-all/expected/kubernetes.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
---
apiVersion: v1
kind: List
items:
- apiVersion: v1
kind: Service
metadata:
annotations:
fabric8.io/git-commit: "@ignore@"
fabric8.io/iconUrl: img/icons/spring-boot.svg
fabric8.io/git-branch: "@ignore@"
prometheus.io/scrape: "@ignore@"
prometheus.io/port: "@ignore@"
labels:
expose: "true"
provider: fabric8
project: hello-world
version: 0.0.1-SNAPSHOT
group: io.fabric8
name: hello-world
spec:
ports:
- name: http
port: 8080
protocol: TCP
targetPort: 8080
selector:
project: hello-world
provider: fabric8
group: io.fabric8
- apiVersion: extensions/v1beta1
kind: Deployment
metadata:
annotations:
fabric8.io/git-commit: "@ignore@"
fabric8.io/iconUrl: img/icons/spring-boot.svg
fabric8.io/git-branch: "@ignore@"
fabric8.io/metrics-path: dashboard/file/kubernetes-pods.json/?var-project=hello-world&var-version=0.0.1-SNAPSHOT
labels:
provider: fabric8
project: hello-world
version: 0.0.1-SNAPSHOT
group: io.fabric8
name: hello-world
spec:
replicas: 1
selector:
matchLabels:
project: hello-world
provider: fabric8
group: io.fabric8
template:
metadata:
annotations:
alpha.istio.io/sidecar: injected
alpha.istio.io/version: [email protected]
fabric8.io/git-commit: "@ignore@"
fabric8.io/iconUrl: img/icons/spring-boot.svg
fabric8.io/git-branch: "@ignore@"
fabric8.io/metrics-path: dashboard/file/kubernetes-pods.json/?var-project=hello-world&var-version=0.0.1-SNAPSHOT
labels:
provider: fabric8
project: hello-world
version: 0.0.1-SNAPSHOT
group: io.fabric8
spec:
containers:
- args:
- proxy
- sidecar
- -v
- "2"
- --passthrough
- "8080"
env:
- name: POD_NAME
valueFrom:
fieldRef:
fieldPath: metadata.name
- name: POD_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: POD_IP
valueFrom:
fieldRef:
fieldPath: status.podIP
image: docker.io/istio/proxy_debug:0.1
imagePullPolicy: Always
livenessProbe:
httpGet:
path: /health
port: 8080
scheme: HTTP
initialDelaySeconds: 180
name: proxy
readinessProbe:
httpGet:
path: /health
port: 8080
scheme: HTTP
initialDelaySeconds: 10
resources: {}
securityContext:
runAsUser: 1337
- env:
- name: KUBERNETES_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
image: "@matches('fabric8/hello-world:.*$')@"
imagePullPolicy: IfNotPresent
livenessProbe:
httpGet:
path: /health
port: 8080
scheme: HTTP
initialDelaySeconds: 180
name: spring-boot
ports:
- containerPort: 8080
name: http
protocol: TCP
- containerPort: 9779
name: prometheus
protocol: TCP
- containerPort: 8778
name: jolokia
protocol: TCP
readinessProbe:
httpGet:
path: /health
port: 8080
scheme: HTTP
initialDelaySeconds: 10
securityContext:
privileged: false
Loading