helm: use Helm hooks instead of Job unique name #23102

sathieu · 2023-01-16T06:10:41Z

Without this, ArgoCD (and other Gitops tools) thinks there is a diff.

See https://helm.sh/docs/topics/charts_hooks/.

helm: use Helm hooks instead of Job unique name

gandro

Thanks for the PR, I did some local testing and this seems to work nicely!

For anyone curious how this works: Thanks to the job being a hook, Helm will not treat it as part oft the release. Any existing job will be removed by Helm after an upgrade or install due to the "helm.sh/hook-delete-policy": before-hook-creation default value.

Could you also update the clustermesh certificate generation? Thanks

cilium/install/kubernetes/cilium/templates/clustermesh-apiserver/tls-cronjob/job.yaml

Lines 2 to 10 in d914e6c

    
           {{/* 
        
           Because Kubernetes job specs are immutable, Helm will fail patch this job if 
        
           the spec changes between releases. To avoid breaking the upgrade path, we 
        
           generate a name for the job here which is based on the checksum of the spec. 
        
           This will cause the name of the job to change if its content changes, 
        
           and in turn cause Helm to do delete the old job and replace it with a new one. 
        
           */}} 
        
           {{- $jobSpec := include "clustermesh-apiserver-generate-certs.job.spec" . -}} 
        
           {{- $checkSum := $jobSpec | sha256sum | trunc 10 -}}

gandro · 2023-01-16T11:09:18Z

install/kubernetes/cilium/templates/hubble/tls-cronjob/job.yaml

-generate a name for the job here which is based on the checksum of the spec.
-This will cause the name of the job to change if its content changes,
-and in turn cause Helm to do delete the old job and replace it with a new one.
-*/}}
 {{- $jobSpec := include "hubble-generate-certs.job.spec" . -}}
 {{- $checkSum := $jobSpec | sha256sum | trunc 10 -}}


Ah, noticed too late. Please also remove this line, since it's not used anymore. And we can probably inline $jobSpec too, now that there is only one use.

sathieu · 2023-01-16T13:11:18Z

@gandro. Thanks for your review. I've addressed your comments. Backto you!

Could you also update the clustermesh certificate generation? Thanks

Done.

Please also remove this line, since it's not used anymore. And we can probably inline $jobSpec too, now that there is only one use.

Done.

gandro

Thanks!

I've started the first stage CI. Let's also ensure that we get reviews from sig-k8s.

aanm · 2023-01-29T21:56:05Z

@gandro are there any implications on upgrading?

Adding @kaworu as reviewer just for a FYI on the hubble part.

aanm · 2023-01-29T21:56:10Z

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Encapsulation Check connectivity with transparent encryption, VXLAN, and endpoint routes

Failure Output

FAIL: Kubernetes DNS did not become ready in time

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Encapsulation Check connectivity with transparent encryption, VXLAN, and endpoint routes

Failure Output

FAIL: Kubernetes DNS did not become ready in time

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

kaworu

Thanks @sathieu nice cleanup!

install/kubernetes/cilium/templates/clustermesh-apiserver/tls-cronjob/job.yaml

gandro · 2023-01-30T09:44:02Z

@gandro are there any implications on upgrading?

As far as I understand, no. At least not as long as helm upgrade is used (for users who maintain their own YAML based on helm template it will depend on their tooling).

sathieu · 2023-02-01T13:26:18Z

This PR has 3 approvals, however the CI is failing for unrelated reason. Anything missing to merge?

tommyp1ckles · 2023-02-01T16:34:06Z

@sathieu looks like some image pull backoff issues in k8s testing. It's been a couple weeks since the CI images where built so the images probably just expired - I'm just going to rerun the tests.

tommyp1ckles · 2023-02-01T16:34:12Z

/test

pchaigno · 2023-02-01T18:01:45Z

/test

@tommyp1ckles Note that command doesn't rerun the image builds. You need to do that manually (I will).

See https://helm.sh/docs/topics/charts_hooks/. Signed-off-by: Mathieu Parent <math.parent@gmail.com>

Signed-off-by: Mathieu Parent <mathieu.parent@insee.fr>

pchaigno · 2023-02-02T17:04:38Z

@sathieu Was there a merge conflict forcing to rebase? JFYI, we need to rerun the full test suite after each rebase.

pchaigno · 2023-02-02T17:04:58Z

/test

sathieu · 2023-02-02T17:12:34Z

@pchaigno Sorry, I added a commit for #23102 (comment), and rebased for clean history. I thought a merge would require a /retest too...

kaworu · 2023-02-03T11:59:50Z

Looks like k8s-1.26-kernel-net-next CI run hit #20723

kaworu · 2023-02-03T12:00:09Z

/test-1.26-net-next

kaworu · 2023-02-03T12:00:16Z

/ci-l4lb

kaworu · 2023-02-06T08:05:50Z

/ci-verifier

pchaigno · 2023-02-06T22:14:29Z

@kaworu Please provide a rationale when restarting tests. We want to ensure that we don't miss new flakes (i.e., that we create flake issues as soon as the flakes as discovered).

kaworu · 2023-02-07T09:43:22Z

@pchaigno both ci-l4lb and ci-verifier were in a "waiting status" pending state and didn't report anything. I was assuming they didn't run at all, but is there any way to get info?

pchaigno · 2023-02-07T16:43:06Z

Ok, probably some blip caused the job triggering to fail for those. Still good to mention so the TopHat doesn't get scared when they see a lot of reruns 😸

sathieu · 2023-02-20T07:29:34Z

Unfortunately, it's not in 1.13.0. Any chance to have this backported?

kaworu · 2023-02-28T12:47:10Z

Seems safe enough to backport to v1.13, @gandro do you agree?

gandro · 2023-02-28T12:53:15Z

I'd like to avoid such changes in patch releases, there is the potential danger of it breaking peoples workflow (e.g. if they manually use helm template), which is why I added the minor release note label.

Since this is not a bugfix, I don't think it satisfies the Backport Criteria for Current Minor Release

sathieu requested review from a team as code owners January 16, 2023 06:10

sathieu requested review from tommyp1ckles and gandro January 16, 2023 06:10

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 16, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 16, 2023

gandro approved these changes Jan 16, 2023

View reviewed changes

gandro requested changes Jan 16, 2023

View reviewed changes

sathieu force-pushed the patch-1 branch from 1a7b866 to bea60f0 Compare January 16, 2023 13:04

sathieu requested review from gandro and removed request for tommyp1ckles January 16, 2023 13:11

gandro requested a review from tommyp1ckles January 16, 2023 15:25

gandro approved these changes Jan 16, 2023

View reviewed changes

kaworu approved these changes Jan 30, 2023

View reviewed changes

install/kubernetes/cilium/templates/clustermesh-apiserver/tls-cronjob/job.yaml Show resolved Hide resolved

tommyp1ckles approved these changes Jan 30, 2023

View reviewed changes

sathieu force-pushed the patch-1 branch from bea60f0 to 8354281 Compare February 2, 2023 16:29

sathieu and others added 2 commits February 2, 2023 17:34

helm: use Helm hooks instead of Job unique name

7aa1062

See https://helm.sh/docs/topics/charts_hooks/. Signed-off-by: Mathieu Parent <math.parent@gmail.com>

helm: use annotations in clustermesh cronjob too

9c58a74

Signed-off-by: Mathieu Parent <mathieu.parent@insee.fr>

sathieu force-pushed the patch-1 branch from 8354281 to 9c58a74 Compare February 2, 2023 16:38

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 6, 2023

pchaigno merged commit 9af18a1 into cilium:master Feb 6, 2023

sathieu deleted the patch-1 branch July 1, 2023 20:17

seb-lafond mentioned this pull request Sep 14, 2023

Cilium v1.14.x - Deployment Failure of Helm Chart on fresh Cluster #28167

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm: use Helm hooks instead of Job unique name #23102

helm: use Helm hooks instead of Job unique name #23102

sathieu commented Jan 16, 2023 •

edited

gandro left a comment

gandro Jan 16, 2023

sathieu Feb 2, 2023

sathieu commented Jan 16, 2023

gandro left a comment •

edited

aanm commented Jan 29, 2023

aanm commented Jan 29, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

kaworu left a comment

gandro commented Jan 30, 2023

sathieu commented Feb 1, 2023

tommyp1ckles commented Feb 1, 2023

tommyp1ckles commented Feb 1, 2023

pchaigno commented Feb 1, 2023

pchaigno commented Feb 2, 2023

pchaigno commented Feb 2, 2023

sathieu commented Feb 2, 2023

kaworu commented Feb 3, 2023

kaworu commented Feb 3, 2023

kaworu commented Feb 3, 2023

kaworu commented Feb 6, 2023

pchaigno commented Feb 6, 2023

kaworu commented Feb 7, 2023

pchaigno commented Feb 7, 2023

sathieu commented Feb 20, 2023

kaworu commented Feb 28, 2023

gandro commented Feb 28, 2023 •

edited

	{{/*
	Because Kubernetes job specs are immutable, Helm will fail patch this job if
	the spec changes between releases. To avoid breaking the upgrade path, we
	generate a name for the job here which is based on the checksum of the spec.
	This will cause the name of the job to change if its content changes,
	and in turn cause Helm to do delete the old job and replace it with a new one.
	*/}}
	{{- $jobSpec := include "clustermesh-apiserver-generate-certs.job.spec" . -}}
	{{- $checkSum := $jobSpec \| sha256sum \| trunc 10 -}}

helm: use Helm hooks instead of Job unique name #23102

helm: use Helm hooks instead of Job unique name #23102

Conversation

sathieu commented Jan 16, 2023 • edited

gandro left a comment

Choose a reason for hiding this comment

gandro Jan 16, 2023

Choose a reason for hiding this comment

sathieu Feb 2, 2023

Choose a reason for hiding this comment

sathieu commented Jan 16, 2023

gandro left a comment • edited

Choose a reason for hiding this comment

aanm commented Jan 29, 2023

aanm commented Jan 29, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

kaworu left a comment

Choose a reason for hiding this comment

gandro commented Jan 30, 2023

sathieu commented Feb 1, 2023

tommyp1ckles commented Feb 1, 2023

tommyp1ckles commented Feb 1, 2023

pchaigno commented Feb 1, 2023

pchaigno commented Feb 2, 2023

pchaigno commented Feb 2, 2023

sathieu commented Feb 2, 2023

kaworu commented Feb 3, 2023

kaworu commented Feb 3, 2023

kaworu commented Feb 3, 2023

kaworu commented Feb 6, 2023

pchaigno commented Feb 6, 2023

kaworu commented Feb 7, 2023

pchaigno commented Feb 7, 2023

sathieu commented Feb 20, 2023

kaworu commented Feb 28, 2023

gandro commented Feb 28, 2023 • edited

sathieu commented Jan 16, 2023 •

edited

gandro left a comment •

edited

aanm commented Jan 29, 2023 •

edited by maintainer-s-little-helper bot

gandro commented Feb 28, 2023 •

edited