PodSpecable Integration #1861

mmelko · 2020-12-11T14:34:11Z

This is still WIP. Currently the test is missing and it's only possible to specify the template from the file. Trait is applied only when IntegrationPhaseRunning state. I had to move util_content and util_sources under util/source to being able to use those also in the trait package.

Fixes #1657.
Fixes #2096.

pkg/trait/pod.go

mmelko · 2021-02-18T15:40:41Z

I've created custom method for merging resources based on map[string]interface{}. I still need to add more tests and sort out the configmaps. If this will be accepted method could be moved to some *utils.go

pkg/trait/pod.go

astefanutti · 2021-03-04T16:33:47Z

@mmelko I've created #2096, that would be the corner stone place to store the PodSpec.

astefanutti · 2021-03-05T14:21:03Z

Now that we validated strategic merge patch is a good candidate, we should find the place where the pod template should be stored.

It may be worth exploring #2096 as a possible solution, embedding a PodSpec field inside the Integration resource, and adding an option to the kamel run CLI command, that would source the template into this field.

mmelko · 2021-03-18T13:08:07Z

I updated the PR also with adding PodTemplateSpec into integration spec (#2096). Pod trait is now enabled when template will be passed. That is possible with --pod-template param from CLI e.g kamel run integration.yaml --pod-template template.yaml. Template is merged with the deployment one using kubernetes strategic merge.

astefanutti · 2021-03-18T13:35:59Z

Thanks a lot. Here are a couple of points:

I wonder if we should embed PodSpec instead of PodTemplateSpec. Annotations can already be passed via the Integration, and that would avoid opening the door to Finalizers, OwnerReferences and the like?
Should we filter out fields from the embedded PodTemplateSpec / PodSpec? Are there any that do not functionally make sense embedding?
We have to add e2e tests covering the first use cases we have in mind, like:
- Setting environment variables to the Integration container
- Adding an init or side-car container
- Setting TopologySpreadConstraints
- ...

mmelko · 2021-03-18T13:59:04Z

That's a valid point. If we don't need to update/configure metadata for the template (which we probably don't) embedding only thePodSpec would be probably much more safe.
I guess it depends on what we want to allow to configure. I can imagine that we won't allow to modify for example the network properties...I need to investigate it more to understand how/where the pod is being created
Adding into my TODO..

mmelko · 2021-04-21T12:32:23Z

I've created a custom PodTemplateSpec based on v1core.PodTemplateSpec https://github.com/apache/camel-k/pull/1861/files#diff-df704f21201b6d30fe851b7bee054f77855f8b5720e77d93e8462fffb9c71cb3R225 to reduce the modification posibilities. I've also added simple e2e test to update ENV properties and add sidecar container.

astefanutti

Awesome! Simple and powerful 👍🏼.

I've left a couple of comments. Once cleared, the PR will be good to go 🥳!

go.mod

pkg/apis/camel/v1/integration_types.go

pkg/cmd/run.go

pkg/trait/pod.go

e2e/common/traits/pod_test.go

pkg/trait/pod.go

astefanutti

The changes to deploy/olm-catalog/camel-k-dev/1.4.0/ must be removed.

go.mod

pkg/cmd/run.go

pkg/trait/pod.go

astefanutti · 2021-04-28T08:51:31Z

pkg/trait/pod_test.go

+
+func testPodTemplateSpec(t *testing.T, template string) corev1.PodTemplateSpec {
+	trait, environment, _ := createPodTest(template)
+	//trait.Template = template


This can be removed.

pkg/trait/pod.go

docs/modules/traits/pages/pod.adoc

astefanutti · 2021-05-04T08:47:48Z

@nicolaferraro @lburgazzoli would you be kind enough for a quick review now this is ready?

lburgazzoli · 2021-05-04T09:38:47Z

docs/modules/traits/pages/pod.adoc

+[source,yaml]
+----
+containers:
+  - name: integration


How would this work when you have competing changes to the integration container ?
i.e.

what would be the result of kamel run --env TEST_VARIABLE=abc --pod-template template.yaml ?

what woudl happen if the user sets a custom entry point for the integration container ?

TEST_VARIABLE=abc will be overwritten by values from the template, due to changes from the template are actually applied by pod trait in later stages.

I've tried to add there command from the example: command: [ "/bin/sh" , "-c", "while true; do echo $(date -u) 'Content from the sidecar altered container' > /var/log/file.txt; sleep 1;done" ] and nothing really happened. Command hasn't been overwritten due to jvm trait is executed after the pod trait.

So I think the behavior is ok for the sake of this PR to get merged but I think we need to open some follow up issues and iterate a little bit over the expected behavior as:

the pod-template is well, a template so I'd expect it not to be "immutable" so, in the cli example I provided above, I'd expect the TEST_VARIABLE to reflect what it is set by the --env flag and not the value from the template. Of course in this case, you know what the template content is so you could workaround this but I'm thinking about a case where the template is configured at an higher level (i.e. IntegrationPlatform) and you don't know if you are overwriting, or if your changes have no effect because the template is overriding some of your config

the fact that a TEST_VARIABLE is taken from the template and the command not, make things a little confusing so we should probably limit/document the relation between the template and the integration container

Right, we need to decide what is the precedence. I see three layers:

operator

user provided pod template

any other user provided options (that affects the integration pod)

The first decision to make is whether the operator is prescriptive or not, that is whether it's possible to override the operator defined spec by the template. Then whether the pod template overwrites other user defined options, or the other way around. For the later, I would be inclined to think it's better from an end-user standpoint to have the specific options have precedence over the more general pod template. For the former, that depends on the level of power we want to delegate to the users.

So for now I will add notes into the doc about env variables and the command option. And if this will get merged we might create a follow-up issue to address env variables and other options that could be possibly overwritten by the template option.

close apache#1657

reflect changes in the pod trait Add e2e tests to cover pod template with deployment and knative strategy

astefanutti · 2021-05-05T06:39:37Z

Thanks!

astefanutti reviewed Dec 14, 2020

View reviewed changes

pkg/trait/pod.go Outdated Show resolved Hide resolved

pkg/trait/pod.go Show resolved Hide resolved

mmelko force-pushed the podtrait branch from eb2c839 to 6e0c0df Compare February 18, 2021 15:38

mmelko force-pushed the podtrait branch 2 times, most recently from d7f0fdf to 792c943 Compare February 22, 2021 09:33

astefanutti reviewed Feb 22, 2021

View reviewed changes

pkg/trait/pod.go Outdated Show resolved Hide resolved

mmelko force-pushed the podtrait branch 2 times, most recently from 977b42d to 1a27a01 Compare March 5, 2021 14:11

mmelko force-pushed the podtrait branch 2 times, most recently from 6344db7 to 3afff7a Compare March 17, 2021 15:36

astefanutti added the status/wip Work in progress label Mar 17, 2021

mmelko force-pushed the podtrait branch from 3afff7a to 1f57870 Compare March 18, 2021 12:59

mmelko mentioned this pull request Mar 18, 2021

PodSpecable Integration #2096

Closed

mmelko force-pushed the podtrait branch from 1f57870 to 9551d38 Compare April 21, 2021 12:27

astefanutti approved these changes Apr 21, 2021

View reviewed changes

astefanutti changed the title ~~WIP: Add podTrait to specify customPodTemplateSpec~~ PodSpecable Integration Apr 21, 2021

mmelko force-pushed the podtrait branch from 9551d38 to 37eb18f Compare April 28, 2021 08:34

astefanutti reviewed Apr 28, 2021

View reviewed changes

mmelko force-pushed the podtrait branch 6 times, most recently from 17c97cc to 255a76c Compare May 3, 2021 19:32

astefanutti removed the status/wip Work in progress label May 4, 2021

lburgazzoli reviewed May 4, 2021

View reviewed changes

mmelko added 2 commits May 4, 2021 15:35

Add podTrait to specify customPodTemplateSpec

18289e9

close apache#1657

Make integration podspec-able

71aedac

reflect changes in the pod trait Add e2e tests to cover pod template with deployment and knative strategy

mmelko force-pushed the podtrait branch from 255a76c to 71aedac Compare May 4, 2021 13:37

astefanutti approved these changes May 4, 2021

View reviewed changes

lburgazzoli approved these changes May 4, 2021

View reviewed changes

astefanutti merged commit bd0a199 into apache:main May 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PodSpecable Integration #1861

PodSpecable Integration #1861

mmelko commented Dec 11, 2020 •

edited by astefanutti

Loading

mmelko commented Feb 18, 2021

astefanutti commented Mar 4, 2021

astefanutti commented Mar 5, 2021

mmelko commented Mar 18, 2021 •

edited

Loading

astefanutti commented Mar 18, 2021

mmelko commented Mar 18, 2021

mmelko commented Apr 21, 2021

astefanutti left a comment

astefanutti left a comment

astefanutti Apr 28, 2021

astefanutti commented May 4, 2021

lburgazzoli May 4, 2021

mmelko May 4, 2021 •

edited

Loading

lburgazzoli May 4, 2021

astefanutti May 4, 2021

mmelko May 4, 2021

astefanutti commented May 5, 2021

PodSpecable Integration #1861

PodSpecable Integration #1861

Conversation

mmelko commented Dec 11, 2020 • edited by astefanutti Loading

mmelko commented Feb 18, 2021

astefanutti commented Mar 4, 2021

astefanutti commented Mar 5, 2021

mmelko commented Mar 18, 2021 • edited Loading

astefanutti commented Mar 18, 2021

mmelko commented Mar 18, 2021

mmelko commented Apr 21, 2021

astefanutti left a comment

Choose a reason for hiding this comment

astefanutti left a comment

Choose a reason for hiding this comment

astefanutti Apr 28, 2021

Choose a reason for hiding this comment

astefanutti commented May 4, 2021

lburgazzoli May 4, 2021

Choose a reason for hiding this comment

mmelko May 4, 2021 • edited Loading

Choose a reason for hiding this comment

lburgazzoli May 4, 2021

Choose a reason for hiding this comment

astefanutti May 4, 2021

Choose a reason for hiding this comment

mmelko May 4, 2021

Choose a reason for hiding this comment

astefanutti commented May 5, 2021

mmelko commented Dec 11, 2020 •

edited by astefanutti

Loading

mmelko commented Mar 18, 2021 •

edited

Loading

mmelko May 4, 2021 •

edited

Loading