Feat/k8s job by Gageperrin · Pull Request #132 · gruntwork-io/helm-kubernetes-services

Gageperrin · 2022-04-21T20:56:03Z

Description

This PR attempts to address Issue #24 by providing a deployment bundle for running a single Kubernetes job. Requested by @rhoboat to review. WIP

Documentation

Inside /helm/charts/k8s-job/README.md

TODOs

Request by @rhoboat

Related Issues

Addresses Issue #24

…e. NOT VALIDATED YET

yorinasub17

This looks like a great start! I found a few minor typos/residuals from Deployment, but overall I think this touches the core!

yorinasub17 · 2022-04-21T20:59:30Z

+
+back to [root README](/README.adoc#day-to-day-operations)
+
+## How do I expose my application internally to the cluster?


This section does not apply to a Job, since a Job by design is for running background tasks and not exposed with an endpoint.

Actually, it looks like almost everything after this doesn't apply to a job, so probably can cut everything after here, and then add new sections that pertain to job. Off the top of my head:

How do I check the status of the Job?

How do I configure my Job to run periodically?

yorinasub17 · 2022-04-21T21:04:50Z

+    {{- range $key, $value := .Values.additionalDeploymentLabels }}
+    {{ $key }}: {{ $value }}
+    {{- end}}
+{{- with .Values.deploymentAnnotations }}


Should be renamed to jobAnnotations

Also, this is missing from the values.yaml file

yorinasub17 · 2022-04-21T21:05:02Z

+    app.kubernetes.io/name: {{ include "k8s-job.name" . }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
+    app.kubernetes.io/managed-by: {{ .Release.Service }}
+    {{- range $key, $value := .Values.additionalDeploymentLabels }}


Should be renamed to additionalJobLabels.

Also, this is missing from the values.yaml file

yorinasub17 · 2022-04-21T21:05:21Z

+      {{- if .isCanary }}
+      gruntwork.io/deployment-type: canary
+      {{- else }}
+      gruntwork.io/deployment-type: main
+      {{- end }}


Since we don't have canaries, can remove.

rhoboat · 2022-04-21T21:08:49Z

Looks like a good start! I want to actually pull and run some of the tests before leaving detailed feedback. Just wanted to let you know that I'll get to that soon!

Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>

…services into feat/k8s-job

rhoboat · 2022-04-21T21:46:13Z

+containerArgs:
+  - "-c"
+  - "while true; do echo hello; sleep 10;done"


👍 I think this makes sense as a trial job to run.

Or.. is this just container start up? Ah, I'm not very familiar with k8s Job, but I was thinking the example could run a very simple job, that the integration test (k8s_job_test.go) would verify was successful.

I believe this should just run for ten seconds while printing "hello" before closing out. Jobs are usually just one-off tasks like db-migrations, so I thought this kind of thing would work. But you're right we still need to sort out how we would do integration testing for verifying a successful finish for a Job.

Yep, that makes sense.

rhoboat · 2022-04-21T21:48:19Z

+	test_structure.RunTestStage(t, "validate_job_deployment", func() {
+		verifyPodsCreatedSuccessfully(t, kubectlOptions, "busybox", releaseName, NumPodsExpected)
+
+	})


Might make sense to also have a stage for "validate_job_successful".

Yeah, if there's enough time I'm going to turn to that for sure

Co-authored-by: Rho <13165182+rhoboat@users.noreply.github.com>

yorinasub17

Looks very close to ready for an initial release! I think the one thing that needs attention is a general cleanup of the chart README and adding one for the example, but overall I think we are very close!

Also, getting the tests to pass. I just attempted a run and ran into the following errors:

=== CONT  TestK8SJobOptionalValuesAreOptional/containerImage.pullPolicy
    template.go:20:
                Error Trace:    template.go:20
                                                        k8s_job_template_test.go:78
                Error:          Received unexpected error:
                                error while running command: exit status 1; WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/yoriy/.kube/config
                                WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/yoriy/.kube/config
                                Error: parse error at (k8s-job/templates/_job_spec.tpl:141): undefined variable "$hasInjectionTypes"

                                Use --debug flag to render out invalid YAML
                Test:           TestK8SJobOptionalValuesAreOptional/containerImage.pullPolicy
--- FAIL: TestK8SJobOptionalValuesAreOptional (0.00s)
    --- FAIL: TestK8SJobOptionalValuesAreOptional/containerImage.pullPolicy (0.05s)


    k8s_job_template_test.go:192:
                Error Trace:    k8s_job_template_test.go:192
                Error:          Not equal:
                                expected: ""
                                actual  : "second-custom-value"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -
                                +second-custom-value
                Test:           TestK8SJobAddingAdditionalLabels
--- FAIL: TestK8SJobAddingAdditionalLabels (0.16s)



    k8s_job_template_test.go:91:
                Error Trace:    k8s_job_template_test.go:91
                Error:          Not equal:
                                expected: 0
                                actual  : 1
                Test:           TestK8SJobAnnotationsRenderCorrectly
    k8s_job_template_test.go:92:
                Error Trace:    k8s_job_template_test.go:92
                Error:          Not equal:
                                expected: ""
                                actual  : "yqDBbJ"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -
                                +yqDBbJ
                Test:           TestK8SJobAnnotationsRenderCorrectly
--- FAIL: TestK8SJobAnnotationsRenderCorrectly (0.16s)

    k8s_job_template_test.go:180:
                Error Trace:    k8s_job_template_test.go:180
                Error:          Not equal:
                                expected: ""
                                actual  : "main"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -
                                +main
                Test:           TestK8SJobMainJobContainersLabeledCorrectly
--- FAIL: TestK8SJobMainJobContainersLabeledCorrectly (0.16s)

yorinasub17 · 2022-04-22T14:12:25Z

+
+back to [root README](/README.adoc#day-to-day-operations)
+
+## How do I set and share configurations with the application?


I think we can actually link to the k8s-service docs for this section, since the content is largely applicable across the two. Something like:

This chart exposes the same parameters as `k8s-service` to set and share configuration variables with your container Pod. Refer to the [corresponding section](/charts/k8s-service/README.md#how-do-i-set-and-share-configurations-with-the-application) in the k8s-service documentation for more details.

Seconding Yori's comment! Where possible, we like to link to the "single source of truth".

yorinasub17 · 2022-04-22T14:13:45Z

+      labels:
+        app.kubernetes.io/name: {{ include "k8s-job.name" . }}
+        app.kubernetes.io/instance: {{ .Release.Name }}
+        gruntwork.io/deployment-type: main


This label is not necessary in k8s-job, as it is primarily used to differentiate with canaries, which doesn't apply for the job use case.

yorinasub17 · 2022-04-22T14:16:47Z

@@ -0,0 +1,164 @@
+#----------------------------------------------------------------------------------------------------------------------


This is good to start! In the short term future right after this, we should also implement support for running multiple copies of the pod with parallelism configuration.

rhoboat

I can give some better feedback based on having pulled the code. I think it's a great start.

If you were to move forward with this PR in the future I would say:

Clean up the charts/k8s-job/README.md first. It would be nice to explain, no more complicated than need be, just what the chart aims to do.
Write the README for examples/k8s-job-busybox, which you've stubbed out.
Then work on getting tests to work. You'll want to wire up the tests using the right render-helper functions. I'd say start with the template tests, then the integration test, but it depends on your workflow. Maybe the integration test makes more sense. 🤷
Part of the above, cull the tests down to only what's needed for k8s-job.
Going beyond if you wanted: Organize the test/ folder to be a little easier to read. Imagine if we started supporting another new chart. This could be a separate PR too.

rhoboat · 2022-04-22T20:33:43Z

+
+	uniqueID := random.UniqueId()
+	// ERROR: Need to find function that can inject annotations into a job
+	job := renderK8SServiceDeploymentWithSetValues(t, map[string]string{"jobAnnotations.unique-id": uniqueID})


I listened to your outro call, and I think you got a little tripped up about this function here. This helper function is actually defined right in this directory in a helper file. You could follow a similar pattern and also define your helper functions for k8s-job that way. Or if you feel another approach is easier to reason about, that could be great.

I can totally understand ramping up on this test as well as Terratest can be a lot in one go. (Punintended)

Nathaniel added 2 commits April 21, 2022 14:03

Initial commit and README

fa0df72

Added Job Helm chart with template test, integration test, and exampl…

3a18e5a

…e. NOT VALIDATED YET

Gageperrin requested review from autero1, rhoboat, yorinasub17 and zackproser as code owners April 21, 2022 20:56

rhoboat reviewed Apr 21, 2022

View reviewed changes

Comment thread charts/k8s-job/README.md Outdated

yorinasub17 reviewed Apr 21, 2022

View reviewed changes

N. G. Perrin and others added 4 commits April 21, 2022 17:16

Update charts/k8s-job/README.md

b87d83d

Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>

Update charts/k8s-job/README.md

049649f

Co-authored-by: Yoriyasu Yano <430092+yorinasub17@users.noreply.github.com>

Commit suggestions

0513ea3

Merge branch 'feat/k8s-job' of github.com:Gageperrin/helm-kubernetes-…

b322176

…services into feat/k8s-job

rhoboat reviewed Apr 21, 2022

View reviewed changes

Comment thread test/k8s_job_template_test.go Outdated

rhoboat reviewed Apr 21, 2022

View reviewed changes

Comment thread test/k8s_job_test.go Outdated

rhoboat reviewed Apr 21, 2022

View reviewed changes

N. G. Perrin and others added 3 commits April 21, 2022 17:50

Update test/k8s_job_template_test.go

b6fe8ef

Co-authored-by: Rho <13165182+rhoboat@users.noreply.github.com>

Update test/k8s_job_test.go

e2d36d0

Co-authored-by: Rho <13165182+rhoboat@users.noreply.github.com>

Commit end of day changes

0293c56

yorinasub17 reviewed Apr 22, 2022

View reviewed changes

rhoboat reviewed Apr 22, 2022

View reviewed changes


		back to [root README](/README.adoc#day-to-day-operations)

		## How do I expose my application internally to the cluster?


		back to [root README](/README.adoc#day-to-day-operations)

		## How do I set and share configurations with the application?

		@@ -0,0 +1,164 @@
		#----------------------------------------------------------------------------------------------------------------------

Uh oh!

Conversation

Gageperrin commented Apr 21, 2022

Description

Documentation

TODOs

Related Issues

Uh oh!

Uh oh!

yorinasub17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhoboat commented Apr 21, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorinasub17 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhoboat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yorinasub17 left a comment •

edited

Loading