CDAP-19300 adding container injection feature and Dockerfile for unit testing on Docker image #89

mariogiuffrida · 2022-05-18T09:47:58Z

This change refers to the ticket CDAP-19300 - https://cdap.atlassian.net/browse/CDAP-19300.

In this change, we introduced:

the possibility of injecting containers inside Stateful and Deployment services.
the possibility to override the service ports (i.e. router listening on non-default ports)
a new Dockerfile.test specifically used to run unit test on a Docker image

google-cla · 2022-05-18T09:48:09Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

dli357 · 2022-07-11T22:28:55Z

api/v1alpha1/cdapmaster_types.go

+	// Containers define any additional containers a service has
+	// This is a list of containers and can be left blank
+	// A typical use is to add sidecars for a deployment
+	Containers []*corev1.Container `json:"containers,omitempty"`


Can we move this to the CDAPServiceSpec? That way we don't have to duplicate code in statefulset and deployment.

Thanks for your review @dli357 I have now moved the Containers definitions to the CDAPServiceSpec struct. Please let me know if there is anything else you would like me to change. Thank you

mariogiuffrida · 2022-07-14T16:35:57Z

Thank you @dli357 I have now uploaded a new commit version

arjan-bal

Thanks for contributing! Left some comments.

arjan-bal · 2022-09-12T04:30:22Z

controllers/cdapmaster_controller.go

+	if spec.Config[confRouterServerAddress] == "" {
+		spec.Config[confRouterServerAddress] = fmt.Sprintf("cdap-%s-%s", r.Name, strings.ToLower(string(serviceRouter)))
+	}
+


nit: Can we remove blank lines between "if" blocks if they're related. In this case, there are 3 if blocks that "Set the configMapCConf entry for the router and UI service and ports". I think a blank line should come after the 3 if blocks.

arjan-bal · 2022-09-12T06:30:23Z

controllers/deployment.go

-		ss, err := getCDAPServiceSpec(master, s)
+		statefulSet, err := getCDAPStatefulServiceSpec(master, s)


Why is this change required? Since Containers field is present in CDAPServiceSpec, we could access it without getting a statefulSet spec or ScalableServiceSpec.

arjan-bal · 2022-09-12T06:35:13Z

controllers/deployment.go

 			return nil, err
 		}
-		if _, err := spec.addAdditionalVolumeMounts(ss.AdditionalVolumeMounts); err != nil {
+		if _, err := spec.addAdditionalVolumeMounts(scalableSpec.CDAPServiceSpec.AdditionalVolumeMounts); err != nil {


This if block seems to be duplicated on line 360.

arjan-bal · 2022-09-12T06:35:37Z

controllers/deployment.go

 			return nil, err
 		}
-		if _, err := spec.addAdditionalVolumes(ss.AdditionalVolumes); err != nil {
+		if _, err := spec.addAdditionalVolumes(scalableSpec.CDAPServiceSpec.AdditionalVolumes); err != nil {


This if block seems to be duplicated on line 357.

arjan-bal · 2022-09-12T06:37:08Z

controllers/testdata/additional_appfabric_container_a.json

+        "protocol":"test-protocol"
+      }
+    ]
+  }


nit: missing newline at end of file.

arjan-bal · 2022-09-12T06:37:15Z

controllers/testdata/additional_appfabric_container_b.json

+        "protocol":"test-protocol"
+      }
+    ]
+  }


nit: missing newline at end of file. Check in other test files also.

arjan-bal · 2022-09-12T06:37:25Z

controllers/testdata/additional_router_container_b.json

+        "protocol":"test-protocol"
+      }
+    ]
+  }


nit: missing newline at end of file.

arjan-bal · 2022-09-12T07:16:28Z

templates/cdap-deployment.yaml

+              httpHeaders:
+                {{range $h := $c.LivenessProbe.Handler.HTTPGet.HTTPHeaders }}
+              - name: {{$h.Name}}
+                value: {{$h.Value}}
+                {{end}}
+              {{end}}
+            {{end}}
+
+            {{if $c.LivenessProbe.Handler.Exec}}
+            exec:
+              command:
+              {{range $v := $c.LivenessProbe.Handler.Exec.Command }}
+              - {{$v}}
+              {{end}}
+            {{end}}
+
+            {{if $c.LivenessProbe.Handler.TCPSocket}}
+            tcpSocket:
+              port: {{$c.LivenessProbe.Handler.TCPSocket.Port}}
+            {{end}}
+
+            {{end}}
+
+          {{end}}
+
+          {{if $c.ReadinessProbe}}
+          readinessProbe:
+            {{if $c.ReadinessProbe.InitialDelaySeconds }}
+            initialDelaySeconds: {{$c.ReadinessProbe.InitialDelaySeconds }}
+            {{end}}
+            {{if $c.ReadinessProbe.TimeoutSeconds }}
+            timeoutSeconds: {{$c.ReadinessProbe.TimeoutSeconds }}
+            {{end}}
+            {{if $c.ReadinessProbe.PeriodSeconds }}
+            periodSeconds: {{$c.ReadinessProbe.PeriodSeconds }}
+            {{end}}
+            {{if $c.ReadinessProbe.SuccessThreshold }}
+            successThreshold: {{$c.ReadinessProbe.SuccessThreshold }}
+            {{end}}
+            {{if $c.ReadinessProbe.FailureThreshold }}
+            failureThreshold: {{$c.ReadinessProbe.FailureThreshold }}
+            {{end}}
+            {{if $c.ReadinessProbe.Handler}}
+
+            {{if $c.ReadinessProbe.Handler.HTTPGet}}
+            httpGet:
+              {{if $c.ReadinessProbe.Handler.HTTPGet.Path}}
+              path: {{$c.ReadinessProbe.Handler.HTTPGet.Path}} 
+              {{end}}
+              port: {{$c.ReadinessProbe.Handler.HTTPGet.Port}}
+              {{if $c.ReadinessProbe.Handler.HTTPGet.Scheme}}
+              scheme: {{$c.ReadinessProbe.Handler.HTTPGet.Scheme}}
+              {{end}}
+              {{if $c.ReadinessProbe.Handler.HTTPGet.Host}}
+              host: {{$c.ReadinessProbe.Handler.HTTPGet.Host}}
+              {{end}}
+              {{if $c.ReadinessProbe.Handler.HTTPGet.HTTPHeaders}}
+              httpHeaders:
+                {{range $h := $c.ReadinessProbe.Handler.HTTPGet.HTTPHeaders }}
+              - name: {{$h.Name}}
+                value: {{$h.Value}}
+                {{end}}
+              {{end}}
+            {{end}}
+
+            {{if $c.ReadinessProbe.Handler.Exec}}
+            exec:
+              command:
+              {{range $v := $c.ReadinessProbe.Handler.Exec.Command }}
+              - {{$v}}
+              {{end}}
+            {{end}}
+
+            {{if $c.ReadinessProbe.Handler.TCPSocket}}
+            tcpSocket:
+              port: {{$c.ReadinessProbe.Handler.TCPSocket.Port}}
+            {{end}}
+
+            {{end}}
+
+          {{end}}
+
+          {{if $c.Ports}}
+          ports:
+            {{range $cp := $c.Ports}}
+          - containerPort: {{$cp.ContainerPort}}
+            {{if $cp.Name}}
+            name: {{$cp.Name}}
+            {{end}}
+            {{if $cp.Protocol}}
+            protocol: {{$cp.Protocol}}
+            {{end}}
+            {{if $cp.HostPort}}
+            hostPort: {{$cp.HostPort}}
+            {{end}}
+            {{if $cp.HostIP}}
+            hostIP: {{$cp.HostIP}}
+            {{end}}
+            {{end}}
+          {{end}}


Instead of adding these properties through templates, I think it will be lesser code and more flexible if you merge the fields using go code.
Eg PR: https://github.com/cdapio/cdap-operator/pull/79/files
Code that needs to be changed: https://github.com/cdapio/cdap-operator/blob/develop/controllers/deployment.go#L460-L465

arjan-bal · 2022-09-12T08:28:52Z

controllers/deployment_test.go

+			expectedContainerJson := readExpectedJson(expectedJsonFilename)
+			diffJson(actualContainerJson, expectedContainerJson)
+		}
+		It("Multiple Additional containers for Router", func() {


nit: I think this should read like a sentence: "It should add multiple containers fro Router". Similarly for other tests.

mariogiuffrida · 2022-09-12T17:33:44Z

Thank you for all your great feedback @arjan-bal! I have now uploaded a new commit version.

dli357 · 2022-10-26T04:05:48Z

templates/cdap-deployment.yaml

@@ -97,7 +97,17 @@ spec:
          env:
          {{range $e := $c.Env }}
          - name: "{{$e.Name}}"
-            value: "{{$e.Value}}"
+            {{if $e.Value}}


Like the other comment regarding moving the container to go code, we should do the same for environment variables if we want to support ValueFrom. This will significantly simplify the generation logic since we can just directly add the container/envvar to the YAML instead of relying on templating.

Additionally, can we also add this to cdap-sts.yaml to keep environment variables consistent across all containers? Thanks!

Thanks for your review @dli357 . I have now moved the environment variables to go code. Could you please have a further look and let me know? Thank you

mariogiuffrida · 2022-11-17T11:50:24Z

@arjan-bal @dli357 I have uploaded a new version which considers all your comments (thank you). Could you please review it?

dli357

Should we be including kubebuilder_1.0.8_darwin_amd64.tar.gz? Otherwise, everything else looks mostly good to me aside from some minor nit comments.

dli357 · 2023-02-21T20:00:08Z

controllers/cdapmaster_controller.go

-/////////////////////////////////////////////////////////
-///// Handling reconciling ConfigMapHandler objects /////
-/////////////////////////////////////////////////////////
+// ///////////////////////////////////////////////////////


nit: no additional space

dli357 · 2023-02-21T20:00:27Z

controllers/cdapmaster_controller.go

-///////////////////////////////////////////////////////////
-///// Handling reconciling deployment of all services /////
-///////////////////////////////////////////////////////////
+// /////////////////////////////////////////////////////////


nit: no spaces

dli357 · 2023-02-21T20:00:34Z

controllers/cdapmaster_controller.go

-///////////////////////////////////////////////////////
-///// Handler for image version upgrade/downgrade /////
-///////////////////////////////////////////////////////
+// /////////////////////////////////////////////////////


nit: no spaces

mariogiuffrida changed the base branch from develop to master May 18, 2022 10:21

mariogiuffrida changed the base branch from master to develop May 18, 2022 10:21

mariogiuffrida force-pushed the CDAP-19300-container-injection-feature branch from 73c000c to 9fb33d2 Compare May 24, 2022 10:23

mariogiuffrida changed the title ~~Cdap 19300 container injection feature~~ CDAP-19300 adding container injection feature and Dockerfile for unit testing on Docker image May 24, 2022

mariogiuffrida force-pushed the CDAP-19300-container-injection-feature branch 2 times, most recently from 2d8e131 to 3e2c1a8 Compare May 24, 2022 11:18

mariogiuffrida marked this pull request as ready for review May 24, 2022 11:29

mariogiuffrida force-pushed the CDAP-19300-container-injection-feature branch 2 times, most recently from e3d60e4 to e5ad0e8 Compare May 24, 2022 14:02

dli357 reviewed Jul 11, 2022

View reviewed changes

arjan-bal self-requested a review August 30, 2022 17:00

arjan-bal reviewed Sep 12, 2022

View reviewed changes

dli357 reviewed Oct 26, 2022

View reviewed changes

mariogiuffrida closed this Nov 15, 2022

mariogiuffrida force-pushed the CDAP-19300-container-injection-feature branch from 57d0ed0 to 21cf8d0 Compare November 15, 2022 15:49

added new line

4261efb

mariogiuffrida reopened this Nov 15, 2022

mariogiuffrida and others added 3 commits November 15, 2022 17:09

CDAP-19300 adding container injection feature

ba08f93

merged from develop

5309e8a

added CRD option to remove desc

088ed10

dli357 reviewed Feb 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDAP-19300 adding container injection feature and Dockerfile for unit testing on Docker image #89

CDAP-19300 adding container injection feature and Dockerfile for unit testing on Docker image #89

mariogiuffrida commented May 18, 2022 •

edited

Loading

google-cla bot commented May 18, 2022

dli357 Jul 11, 2022

mariogiuffrida Jul 12, 2022

mariogiuffrida commented Jul 14, 2022

arjan-bal left a comment

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

arjan-bal Sep 12, 2022

mariogiuffrida commented Sep 12, 2022

dli357 Oct 26, 2022

mariogiuffrida Nov 7, 2022

mariogiuffrida commented Nov 17, 2022

dli357 left a comment •

edited

Loading

dli357 Feb 21, 2023

dli357 Feb 21, 2023

dli357 Feb 21, 2023

		ss, err := getCDAPServiceSpec(master, s)
		statefulSet, err := getCDAPStatefulServiceSpec(master, s)

CDAP-19300 adding container injection feature and Dockerfile for unit testing on Docker image #89

Are you sure you want to change the base?

CDAP-19300 adding container injection feature and Dockerfile for unit testing on Docker image #89

Conversation

mariogiuffrida commented May 18, 2022 • edited Loading

google-cla bot commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariogiuffrida commented Jul 14, 2022

arjan-bal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariogiuffrida commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariogiuffrida commented Nov 17, 2022

dli357 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariogiuffrida commented May 18, 2022 •

edited

Loading

dli357 left a comment •

edited

Loading