Add support for sidecar container #423

ilayaperumalg · 2021-03-08T09:12:30Z

Ability to pass sidecar container via PodSpec
Applicable whenever PodSpec is created
Add tests

Resolves #414

- Ability to pass sidecar container via PodSpec - Applicable whenever PodSpec is created - Add tests Resolves spring-cloud#414

chrisjs

i think the terminology here can be more generic. a pod can contain multiple containers. "sidecar" is more of a pattern type term. i think a generic deployer property that contains the container info for one or more containers say something like additionalContainers or whatnot would be more appropriate. the property should allow for multiple container definitions to be passed.

ilayaperumalg · 2021-04-15T03:52:47Z

@chrisjs Good point. In fact, when I was getting started with this feature, my original intention was to allow multiple containers instead of just a single additional container. But, since the deployment properties have key/value pairs as String/String, I was thinking maybe we could add the initial support to add a single sidecar container with the specific key named sidecarContainer to avoid this key/value constraint.

Irrespective of this, let's discuss further to find a better approach on this. Thanks!

chrisjs · 2021-04-15T04:02:00Z

@chrisjs Good point. In fact, when I was getting started with this feature, my original intention was to allow multiple containers instead of just a single additional container. But, since the deployment properties have key/value pairs as String/String, I was thinking maybe we could add the initial support to add a single sidecar container with the specific key named sidecarContainer to avoid this key/value constraint.

Irrespective of this, let's discuss further to find a better approach on this. Thanks!

we already map objects beyond key/value pairs, for example:

https://docs.spring.io/spring-cloud-dataflow/docs/current/reference/htmlsingle/#_secret_key_references

https://docs.spring.io/spring-cloud-dataflow/docs/current/reference/htmlsingle/#_configmap_key_references

etc. seems like it would be a similar approach. i would advocate for using the generic "containers" naming rather than "sidecar"

ilayaperumalg · 2021-04-15T04:22:45Z

ok, yeah, I missed that. I will work on it and update the PR.

i would advocate for using the generic "containers" naming rather than "sidecar"

Sure, containers that is :-)

ilayaperumalg · 2021-04-15T07:45:53Z

@chrisjs Updated the PR with the suggested changes. With this set of changes, we can support having multiple containers with all the supported configurations for a container.

- Add list of `Container` objects as a property named `additionalContainers` in K8s deployer property - Use `additionalContainers` as the deployer property to extract the additional container configurations - Add and Update tests

ilayaperumalg · 2021-04-15T12:50:28Z

...ain/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java

+	public static class InitContainer extends ContainerProperties {
+	}
+
+	static class Container extends io.fabric8.kubernetes.api.model.Container {


I am curious if we can use the same approach of using fabric8 api models to other supported properties as well. I believe this can eliminate some explicit models with a fewer properties (for instance the ContainerProperties we have for InitContainer).

Also, I thought having an explicit type named Container could help here. But, I am ok if we can directly use the fabric8 Container type as is in this class.

you could use the fabric8 objects, but thats a bit leakly. theres an open issue around this: #296

chrisjs

minor things but beyond that LGTM

chrisjs · 2021-04-15T16:05:01Z

src/main/java/org/springframework/cloud/deployer/spi/kubernetes/AbstractKubernetesDeployer.java

-
+		List<Container> additionalContainers = this.deploymentPropertiesResolver.getAdditionalContainers(deploymentProperties);
+		for (Container additionalContainer: additionalContainers) {
+			podSpec.addToContainers(additionalContainer);


looks like theres a addAllToContainers that could be used rather than the loop here

chrisjs · 2021-04-15T16:08:48Z

...ain/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java

+	public static class InitContainer extends ContainerProperties {
+	}
+
+	static class Container extends io.fabric8.kubernetes.api.model.Container {


you could use the fabric8 objects, but thats a bit leakly. theres an open issue around this: #296

chrisjs · 2021-04-15T16:09:16Z

...ain/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java

@@ -687,6 +693,11 @@ public void setVolumeMounts(List<VolumeMount> volumeMounts) {
 	 */
 	private InitContainer initContainer;

+	/**
+	 * A side car container one can add to the main application container.


A side car container -> A container

chrisjs · 2021-04-15T16:48:47Z

...rg/springframework/cloud/deployer/spi/kubernetes/KubernetesTaskLauncherIntegrationTests.java

+
+		String taskName = request.getDefinition().getName();
+
+		log.info("Checking job pod spec annotations of {}...", taskName);


looks like this should be removed, or the message changed

chrisjs · 2021-04-16T15:07:10Z

merged in d84e6c7

ilayaperumalg · 2021-04-19T08:39:15Z

@isha1kh @pravinkumarb84 If possible, it would be great to try out the latest SCDF 2.8.0 SNAPSHOT and Skipper 2.7.0-SNAPSHOT and let us know your feedback on supporting additional containers. Please pay attention to the name of the property additionalContainers and you can refer to this test for how to use it. Thanks!

isha1kh · 2021-04-19T12:20:46Z

@ilayaperumalg - Yes am going to try this soon, thanks.

isha1kh · 2021-04-23T06:50:40Z

@ilayaperumalg - I've tried against the above version and although the additional container is getting added. I've noticed an issue i.e. when trying to update the stream via Skipper UI, the additional container properties aren't re-populated.

ilayaperumalg · 2021-04-26T05:35:57Z

On the container restart question, You need to set the restart policy for the container spec .

On the UI, you would only see the additionalContainers as a root property and won't see the nested properties for the container. Please feel free to work on supporting this feature if you are willing to contribute. Thanks!

umutcann · 2021-04-28T08:49:23Z

Hi @ilayaperumalg, @isha1kh

I also want to try this implementation. As I understand, we should pass an container yaml as additionalContainers param, right?

Do you have any sample to try?

Thanks.

ilayaperumalg · 2021-04-28T09:25:41Z

You need to set the container properties similar to how we set here

umutcann · 2021-04-28T10:25:41Z

Thanks @ilayaperumalg , I did not notice before :)

Add support for sidecar container

adc5ec7

- Ability to pass sidecar container via PodSpec - Applicable whenever PodSpec is created - Add tests Resolves spring-cloud#414

ilayaperumalg mentioned this pull request Mar 8, 2021

support kubernetes native deployment yaml. #422

Closed

isha1kh approved these changes Apr 8, 2021

View reviewed changes

pravinkumarb84 approved these changes Apr 13, 2021

View reviewed changes

ilayaperumalg assigned chrisjs Apr 13, 2021

chrisjs suggested changes Apr 15, 2021

View reviewed changes

ilayaperumalg requested a review from chrisjs April 15, 2021 11:47

Add support for having multiple additional containers

8df9f07

- Add list of `Container` objects as a property named `additionalContainers` in K8s deployer property - Use `additionalContainers` as the deployer property to extract the additional container configurations - Add and Update tests

ilayaperumalg force-pushed the GH-414-sidecar-container branch from f77f8aa to 8df9f07 Compare April 15, 2021 12:46

ilayaperumalg commented Apr 15, 2021

View reviewed changes

chrisjs suggested changes Apr 15, 2021

View reviewed changes

Address review comments

3186e3b

ilayaperumalg requested a review from chrisjs April 16, 2021 05:36

chrisjs closed this Apr 16, 2021

umutcann mentioned this pull request Jun 17, 2021

Create containers by order #452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sidecar container #423

Add support for sidecar container #423

ilayaperumalg commented Mar 8, 2021

chrisjs left a comment

ilayaperumalg commented Apr 15, 2021

chrisjs commented Apr 15, 2021 •

edited

Loading

ilayaperumalg commented Apr 15, 2021

ilayaperumalg commented Apr 15, 2021

ilayaperumalg Apr 15, 2021

chrisjs Apr 15, 2021

chrisjs left a comment

chrisjs Apr 15, 2021

chrisjs Apr 15, 2021

chrisjs Apr 15, 2021

chrisjs Apr 15, 2021

chrisjs commented Apr 16, 2021

ilayaperumalg commented Apr 19, 2021

isha1kh commented Apr 19, 2021

isha1kh commented Apr 23, 2021 •

edited

Loading

ilayaperumalg commented Apr 26, 2021

umutcann commented Apr 28, 2021

ilayaperumalg commented Apr 28, 2021

umutcann commented Apr 28, 2021


		String taskName = request.getDefinition().getName();

		log.info("Checking job pod spec annotations of {}...", taskName);

Add support for sidecar container #423

Add support for sidecar container #423

Conversation

ilayaperumalg commented Mar 8, 2021

chrisjs left a comment

Choose a reason for hiding this comment

ilayaperumalg commented Apr 15, 2021

chrisjs commented Apr 15, 2021 • edited Loading

ilayaperumalg commented Apr 15, 2021

ilayaperumalg commented Apr 15, 2021

ilayaperumalg Apr 15, 2021

Choose a reason for hiding this comment

chrisjs Apr 15, 2021

Choose a reason for hiding this comment

chrisjs left a comment

Choose a reason for hiding this comment

chrisjs Apr 15, 2021

Choose a reason for hiding this comment

chrisjs Apr 15, 2021

Choose a reason for hiding this comment

chrisjs Apr 15, 2021

Choose a reason for hiding this comment

chrisjs Apr 15, 2021

Choose a reason for hiding this comment

chrisjs commented Apr 16, 2021

ilayaperumalg commented Apr 19, 2021

isha1kh commented Apr 19, 2021

isha1kh commented Apr 23, 2021 • edited Loading

ilayaperumalg commented Apr 26, 2021

umutcann commented Apr 28, 2021

ilayaperumalg commented Apr 28, 2021

umutcann commented Apr 28, 2021

chrisjs commented Apr 15, 2021 •

edited

Loading

isha1kh commented Apr 23, 2021 •

edited

Loading