Improve support for sidecar containers #1202

nicolaferraro · 2018-02-19T12:21:39Z

This fixes #968, and its implementation is based on a discussion with @kameshsampath a long time ago.

This changes how the fragments are merged with generated resources. If a container in a fragment has a name (different from the f-m-p generated one) it's considered a sidecar and not merged, creating 2 containers. Fragments that don't declare a name (or with a matching one) are considered an addition to the default container.

I've changed the health-check enrichment process to enrich only generated containers and not sidecars. In order to do this I needed to add a env variable to generated containers (there are no annotation/labels at container level) to recognize it during enrichment.

nicolaferraro · 2018-02-20T11:15:44Z

retest this please

ghost · 2018-02-20T17:08:36Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/AbstractHealthCheckEnricher.java


 /**
 * Enriches containers with health check probes.
 */
 public abstract class AbstractHealthCheckEnricher extends BaseEnricher {

+    private enum Config implements Configs.Key {
+        enrichAllContainers {{ d = "false"; }},


Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.
Move the contents of this initializer to a standard constructor or to field initializers.
Use another way to initialize this instance.

ghost · 2018-02-20T17:08:37Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/AbstractHealthCheckEnricher.java


 /**
 * Enriches containers with health check probes.
 */
 public abstract class AbstractHealthCheckEnricher extends BaseEnricher {

+    private enum Config implements Configs.Key {
+        enrichAllContainers {{ d = "false"; }},
+        enrichContainers    {{ d = null; }};


Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.
Move the contents of this initializer to a standard constructor or to field initializers.
Use another way to initialize this instance.

ghost · 2018-02-20T17:08:37Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/AbstractHealthCheckEnricher.java

@@ -80,6 +87,46 @@ private String describe(Probe probe) {
        return desc.toString();
    }

+    protected List<ContainerBuilder> getContainersToEnrich(KubernetesListBuilder builder) {


Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.

ghost · 2018-02-20T17:08:38Z

SonarQube analysis reported 7 issues

3 critical
2 major
2 minor

Watch the comments in this conversation to review them.

nicolaferraro · 2018-02-20T17:10:34Z

Finally.. JMockit was not working well..

hrishin · 2018-03-01T04:12:53Z

@nicolaferraro Thanks for the PR. Will review it soon :)

nicolaferraro · 2018-03-27T06:19:12Z

@hrishin did you have time to have a look?

hrishin · 2018-04-03T09:09:52Z

@nicolaferraro sorry to say I got sidetracked from FMP these days. I can give initial look but @rohanKanojia @piyush1594 can help here to review it.

@piyush1594 @rohanKanojia lets priorities this PR review ASAP

geoand · 2018-05-28T07:18:14Z

Hi,

Is there any news on this? I ran into this issue as well

piyush-garg · 2018-06-07T09:20:28Z

@nicolaferraro Would you like to rebase and push again.

hrishin · 2018-06-18T14:38:51Z

@nicolaferraro we have rebased it. Could you please verify it?

nicolaferraro · 2018-06-22T09:39:40Z

Rebased...

piyush-garg · 2018-06-22T09:45:52Z

hey, @nicolaferraro 3.5.40 got released two days, you need to put it in 3.5.41. Thanks

rhuss

I really like and support the intention of the PR. Although I haven't much time to dig into the details, the only thing I dislike a bit is the introduction of a kind of 'processing instruction' in form of a container env which doesn't get remove afterwards.

Actually, it would be nice if the Kubernetes Model would allow for such kind of transient meta data, so it might be worth to open an issue over there. In the meantime, I would add a postprocess step to remove the "FABRIC8_GENERATED" env vzr.

rhuss · 2018-07-18T09:33:42Z

core/src/main/java/io/fabric8/maven/core/handler/EnvVarHandler.java

@@ -53,6 +53,12 @@
                .endValueFrom()
                .build());

+        ret.add(
+            new EnvVarBuilder()
+                    .withName("FABRIC8_GENERATED")


I wonder whether we could use some other kind of meta-data for a container to indicate that it's auto generated? As this env var is visible within the container it might have side effects (although unlikely) but at least it pollutes the env namespace and increases the descriptors size. I understand that a container within a pod cannot carry any annotations/labels.

However, we need this information only during the (internal) build up of the model, so it sounds a bit over the top to make this information persistent. I suspect that the Kubernetes model objects don't carry any 'transient' meta information (which would be useful here), but maybe we could, in a postrpocess step, remove our markers before writing out the resource files ?

Yes, it was my concern too. I think having a enricher that removes the env variable at the end of the build is ok. However, I think that I can also use a annotation at pod level to specify the name of the container that is generated by fabric8. Let's keep this on hold and I'll try to provide a different implementation. I don't like the added env variable either.

codecov · 2018-11-02T05:50:58Z

Codecov Report

Merging #1202 into master will increase coverage by 0.3%.
The diff coverage is 56.6%.

@@             Coverage Diff             @@
##             master    #1202     +/-   ##
===========================================
+ Coverage     35.76%   36.06%   +0.3%     
- Complexity     1110     1131     +21     
===========================================
  Files           180      180             
  Lines          9969    10041     +72     
  Branches       1617     1632     +15     
===========================================
+ Hits           3565     3621     +56     
- Misses         5976     5987     +11     
- Partials        428      433      +5

nicolaferraro

@rohanKanojia I think removing the env variable at the end is the easiest way to implement the functionality without affecting the end user. This way, the variable is just a marker within the enrichment process.

LGTM

lordofthejars · 2019-01-17T15:41:43Z

plugin/src/main/java/io/fabric8/maven/plugin/converter/DeploymentOpenShiftConverter.java


-    public DeploymentOpenShiftConverter(PlatformMode mode, Long openshiftDeployTimeoutSeconds) {
+    public DeploymentOpenShiftConverter(PlatformMode mode, Long openshiftDeployTimeoutSeconds, MavenProject project) {


Instead of passing the MavenProject, can we only pass the artifactId?

devang-gaur · 2019-03-12T21:43:44Z

fixes #1037

lordofthejars · 2019-03-13T09:24:26Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/BaseEnricher.java

@@ -38,7 +38,7 @@

    private final EnricherConfig config;
    private final String name;
-    private EnricherContext enricherContext;
+    public EnricherContext enricherContext;


Don't like to have a public field, maybe protected? Or create a getter?

lordofthejars · 2019-03-13T09:47:00Z

...her/standard/src/main/java/io/fabric8/maven/enricher/standard/DefaultControllerEnricher.java

                        deploymentConfigBuilder.editSpec().addNewTrigger().withType("ConfigChange").endTrigger().endSpec();
                    }
                }
            });
        }
    }

+    private void setProcessingInstruction(List<String> containerNames) {
+        Map<String, String> pi = new HashMap<>();


Can you rename to a meaningful var name?

We can have a varible called Processing Instructions(pi) in EnricherContext which would be propagated through the chain of enrichers rather than polluting application's container environment.

Don't add ImageChange triggers to containers other than main application containers. In Openshift mode, ImageStream is used by default. This is just a workaround for fabric8io#1037

… images

…ariable name

nicolaferraro requested review from kameshsampath and hrishin February 19, 2018 12:21

nicolaferraro mentioned this pull request Feb 20, 2018

spring-boot-healthcheck adds probes to initContainers #1178

Closed

nicolaferraro force-pushed the sidecars branch 3 times, most recently from f33dd5a to 4cf8190 Compare February 20, 2018 17:04

ghost reviewed Feb 20, 2018

View reviewed changes

nicolaferraro force-pushed the sidecars branch from b71fa42 to 4efb72c Compare June 22, 2018 09:39

nicolaferraro force-pushed the sidecars branch from 4efb72c to 1e0c2c9 Compare June 22, 2018 10:13

rohanKanojia requested a review from rhuss July 13, 2018 05:26

rhuss reviewed Jul 18, 2018

View reviewed changes

nicolaferraro mentioned this pull request Jul 20, 2018

When initContainers defined on deployment, readinessProbe must not be generated #1283

Closed

rhuss assigned nicolaferraro Jul 24, 2018

rhuss added the pr/change-requested Reviewer requested a change label Jul 24, 2018

rohanKanojia added the target/3.5 PR targeted to 3.5 label Jul 24, 2018

rhuss added WIP target/4.0 PR for targeted to 4.0.x labels Jul 31, 2018

rohanKanojia force-pushed the sidecars branch from 9087d2b to 038244b Compare October 30, 2018 10:36

rohanKanojia requested a review from lordofthejars October 30, 2018 10:37

rohanKanojia force-pushed the sidecars branch 2 times, most recently from 93acc9c to d4c5dd0 Compare November 2, 2018 05:47

lordofthejars approved these changes Nov 6, 2018

View reviewed changes

nicolaferraro commented Nov 26, 2018

View reviewed changes

lordofthejars mentioned this pull request Nov 29, 2018

Do not add ImageChange change triggers for Docker images #1037

Closed

rohanKanojia force-pushed the sidecars branch from d4c5dd0 to e4344fc Compare January 16, 2019 14:50

rohanKanojia added pr/waiting-on-feedback Waiting on feedback and removed pr/change-requested Reviewer requested a change pr/wip Work in Progress, do not merge labels Jan 17, 2019

lordofthejars suggested changes Jan 17, 2019

View reviewed changes

rohanKanojia force-pushed the sidecars branch from 1cfc192 to 640a4b5 Compare March 1, 2019 12:18

rohanKanojia requested a review from devang-gaur March 11, 2019 13:34

rohanKanojia force-pushed the sidecars branch from 36d5043 to 9693e61 Compare March 11, 2019 13:35

devang-gaur approved these changes Mar 12, 2019

View reviewed changes

lordofthejars suggested changes Mar 13, 2019

View reviewed changes

nicolaferraro and others added 7 commits March 13, 2019 17:27

Fix fabric8io#968: improve support for sidecar containers

57416f6

Use Processing instructions rather than environment variables.

98bb042

We can have a varible called Processing Instructions(pi) in EnricherContext which would be propagated through the chain of enrichers rather than polluting application's container environment.

Do not add ImageChange triggers for sidecar containers

c96602d

Don't add ImageChange triggers to containers other than main application containers. In Openshift mode, ImageStream is used by default. This is just a workaround for fabric8io#1037

Changes in AbstractHealthCheckEnricher after Enricher refactoring

8ad2aaf

Fix fabric8io#1037: Do not add ImageChange change triggers for Docker…

7824e76

… images

Made enricherContext field protected rather than public, refactored v…

75c2f62

…ariable name

Changes in tests after rebasing with master

70e4b42

rohanKanojia force-pushed the sidecars branch from 9693e61 to 70e4b42 Compare March 13, 2019 12:38

lordofthejars approved these changes Mar 13, 2019

View reviewed changes

lordofthejars merged commit 3a640a4 into fabric8io:master Mar 13, 2019

rohanKanojia mentioned this pull request Mar 27, 2019

4.0.0 is generating an unwanted sidecar copy of application instance #1579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve support for sidecar containers #1202

Improve support for sidecar containers #1202

nicolaferraro commented Feb 19, 2018

nicolaferraro commented Feb 20, 2018

ghost Feb 20, 2018

ghost Feb 20, 2018

ghost Feb 20, 2018

ghost commented Feb 20, 2018

nicolaferraro commented Feb 20, 2018

hrishin commented Mar 1, 2018

nicolaferraro commented Mar 27, 2018

hrishin commented Apr 3, 2018

geoand commented May 28, 2018

piyush-garg commented Jun 7, 2018

hrishin commented Jun 18, 2018

nicolaferraro commented Jun 22, 2018

piyush-garg commented Jun 22, 2018 •

edited

Loading

rhuss left a comment

rhuss Jul 18, 2018

nicolaferraro Jul 18, 2018

codecov bot commented Nov 2, 2018 •

edited

Loading

nicolaferraro left a comment

lordofthejars Jan 17, 2019

devang-gaur commented Mar 12, 2019

lordofthejars Mar 13, 2019

lordofthejars Mar 13, 2019


		public DeploymentOpenShiftConverter(PlatformMode mode, Long openshiftDeployTimeoutSeconds) {
		public DeploymentOpenShiftConverter(PlatformMode mode, Long openshiftDeployTimeoutSeconds, MavenProject project) {

Improve support for sidecar containers #1202

Improve support for sidecar containers #1202

Conversation

nicolaferraro commented Feb 19, 2018

nicolaferraro commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 20, 2018

nicolaferraro commented Feb 20, 2018

hrishin commented Mar 1, 2018

nicolaferraro commented Mar 27, 2018

hrishin commented Apr 3, 2018

geoand commented May 28, 2018

piyush-garg commented Jun 7, 2018

hrishin commented Jun 18, 2018

nicolaferraro commented Jun 22, 2018

piyush-garg commented Jun 22, 2018 • edited Loading

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2018 • edited Loading

Codecov Report

nicolaferraro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devang-gaur commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-garg commented Jun 22, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading