Feat/k8s job #132

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

charts/k8s-job/README.md

yorinasub17

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

charts/k8s-job/README.md

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

charts/k8s-job/README.md

+
+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

charts/k8s-job/templates/_job_spec.tpl

+    {{- 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

charts/k8s-job/templates/_job_spec.tpl

+    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

charts/k8s-job/templates/_job_spec.tpl

+      {{- 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 <[email protected]>

…services into feat/k8s-job

test/k8s_job_template_test.go

test/k8s_job_test.go

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

examples/k8s-job-busybox/values.yaml

+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/k8s_job_test.go

+	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 <[email protected]>

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

charts/k8s-job/README.md

+
+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

charts/k8s-job/templates/_job_spec.tpl

+      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

charts/k8s-job/values.yaml

@@ -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

test/k8s_job_template_test.go

+
+	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 yorinasub17, rhoboat, zackproser and autero1 as code owners April 21, 2022 20:56

rhoboat reviewed Apr 21, 2022

View reviewed changes

charts/k8s-job/README.md Outdated Show resolved Hide resolved

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 <[email protected]>

Update charts/k8s-job/README.md

049649f

Co-authored-by: Yoriyasu Yano <[email protected]>

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

test/k8s_job_template_test.go Outdated Show resolved Hide resolved

rhoboat reviewed Apr 21, 2022

View reviewed changes

test/k8s_job_test.go Outdated Show resolved Hide resolved

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 <[email protected]>

Update test/k8s_job_test.go

e2d36d0

Co-authored-by: Rho <[email protected]>

Commit end of day changes

0293c56

yorinasub17 reviewed Apr 22, 2022

View reviewed changes

rhoboat reviewed Apr 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/k8s job #132

Feat/k8s job #132

Gageperrin commented Apr 21, 2022

yorinasub17 left a comment

yorinasub17 Apr 21, 2022

yorinasub17 Apr 21, 2022

yorinasub17 Apr 21, 2022

yorinasub17 Apr 21, 2022

yorinasub17 Apr 21, 2022

yorinasub17 Apr 21, 2022

yorinasub17 Apr 21, 2022

rhoboat commented Apr 21, 2022

rhoboat Apr 21, 2022

rhoboat Apr 21, 2022

Gageperrin Apr 21, 2022

rhoboat Apr 22, 2022

rhoboat Apr 21, 2022

Gageperrin Apr 21, 2022

yorinasub17 left a comment •

edited

Loading

yorinasub17 Apr 22, 2022

rhoboat Apr 22, 2022

yorinasub17 Apr 22, 2022

yorinasub17 Apr 22, 2022

rhoboat left a comment

rhoboat Apr 22, 2022

rhoboat Apr 22, 2022


		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 @@
		#----------------------------------------------------------------------------------------------------------------------

Feat/k8s job #132

Are you sure you want to change the base?

Feat/k8s job #132

Conversation

Gageperrin commented Apr 21, 2022

Description

Documentation

TODOs

Related Issues

yorinasub17 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

rhoboat commented Apr 21, 2022

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

yorinasub17 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

Choose a reason for hiding this comment

rhoboat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorinasub17 left a comment •

edited

Loading