Verify generation in readiness checks #10920

muang0 · 2022-05-02T05:43:41Z

Signed-off-by: James Oden james.oden.dev@gmail.com

What this PR does / why we need it: This patches a bug where helm preemptively marks resources as ready. This is due to a race condition between the helm readiness check and the associated k8s controller updating the resource. I've added generational validation to readiness checks for all applicable resources.

Special notes for your reviewer:

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

closes #10859
Associated k8s issue
Thanks for the help!

Signed-off-by: James Oden <james.oden.dev@gmail.com>

muang0 · 2022-05-13T18:51:29Z

@joejulian Anything I can do on my end to help get this one triaged? Thanks for the help!

muang0 · 2022-05-18T03:33:55Z

@mattfarina @bacongobbler @yxxhero Anything I can do on my end to help get this one triaged? Thanks for the help!

muang0 · 2022-05-31T02:26:18Z

@zonggen @joejulian @yxxhero Anything I can do on my end to help get this one triaged? Thanks for the help!

yxxhero · 2022-05-31T02:29:15Z

@muang0 need more comments for review. Thanks very much.

muang0 · 2022-05-31T02:48:34Z

@muang0 need more comments for review. Thanks very much.

@yxxhero thanks for the response! Do you mind clarifying what you mean? Do you need me to provide more clarification on this PR in a comment?

A concise summary: During resource readiness checks, helm doesn't utilize the k8s generation/observedGeneration fields. This creates an opening for a race condition between helm and the associated k8s controller. In this case, a resource is incorrectly marked as ready by helm (before the associated k8s controller picks up the changes to the spec from helm). If we add generational checks to helm readiness validation, then helm would notice the generation & observedGeneration are out of sync, and avoid this race condition entirely.

What we are observing as a result: While using the wait flag, helm doesn't wait for the resources to be created but rather immediately marks resource creation as complete and finishes the install.

k8s api-conventions description of observedGeneration & generation: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md?plain=1#L476-L479

muang0 · 2022-06-03T01:04:20Z

Hi @yxxhero any action I should take from my end? Thanks for the help!

yxxhero · 2022-06-03T02:34:19Z

@muang0 In this weekend. I will review this.

muang0 · 2022-06-08T02:04:55Z

Hi @yxxhero, hope you're having a good week! So sorry to bother, just bumping this so it doesn't fall between the cracks :) It's a small change at its core, most of the lines are unit tests.

muang0 · 2022-06-15T00:38:51Z

@yxxhero I hate to bother you 😅 this PR is really a small change at its core, shouldn't take long to review/triage. You'd be helping us out a ton by helping push this one along 😄

yxxhero · 2022-06-15T01:51:32Z

@muang0 those changes are valuable. but The code changes a lot, I need more time to review. Thanks very much.

muang0 · 2022-06-28T14:29:25Z

Gentle bump 😀

Dentrax · 2022-07-06T20:04:29Z

This PR is what exactly I was looking for. I was playing with Kind cluster using Helm and noticed --wait does not as expected, so found the following comment: #10061 (comment)

And ended up this PR eventually. So I tested on my local env:

$ gh pr checkout 10920
$ make build

$ cat <<EOF | kind create cluster --name helm-test --config=-
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
- role: worker
- role: worker
- role: worker
EOF

# The next two commands does not wait
$ helm upgrade fluent-bit fluent/fluent-bit --install --wait
$ helm upgrade --wait --reuse-values --set flush=3 fluent-bit fluent/fluent-bit

# Now test with this PR
$ ./bin/helm upgrade --wait --reuse-values --set flush=1 fluent-bit fluent/fluent-bit

Works like a charm, great job @muang0! 👏

Any possibility to make review process a bit faster to keep time window short to get this merge to main branch?

Kind ping @yxxhero @mattfarina @bacongobbler 🤗

muang0 · 2022-07-14T15:13:25Z

Kind ping :)

muang0 · 2022-07-28T01:10:31Z

Kind ping, plz fam, it's not a complex change 😅

rmendal-Harness · 2022-08-11T15:32:38Z

We've run into this issue too. Would really appreciate it if this got reviewed please and thanks.

muang0 · 2022-08-23T14:45:24Z

@dnwe Thank you greatly for your contribution!! 🎆 Your statefulset generational readiness check is valuable work 😄
7c74f1d

@yxxhero @mattfarina @bacongobbler I think this type of generational check would be valuable for other resources as well. In this PR I've applied generational readiness checks for all applicable resources. If you think this is something helm can benefit from, and willing to commit to reviewing these changes, I'm happy resolve my merge conflicts. Just let me know!

@rmendal-Harness @Dentrax This will likely be the last activity on this PR from me unless maintainers are interested in this PR. Thank you for the shoutouts 💯

sudermanjr

This looks great. The test coverage is great, and I was able to make the tests fail. This seems to me to be the correct way to check for readiness. I left one small nit inline about a comment.

I'm still digging into the logic on the IsRead, specifically the removal of the variable declaration at the top. I don't quite understand why it was there in the first place, so I'm not certain removing it is the right move. Will keep looking

Also, it looks like this could use a rebase. I can bring this to the next community meeting if you want to do that and address the comment.

sudermanjr · 2022-08-27T01:01:10Z

pkg/kube/ready.go

@@ -272,6 +284,16 @@ func (c *ReadyChecker) volumeReady(v *corev1.PersistentVolumeClaim) bool {
 }

 func (c *ReadyChecker) deploymentReady(rs *appsv1.ReplicaSet, dep *appsv1.Deployment) bool {
+	// Verify the generation observed by the replicaSet controller matches the spec generation


Seems like a duplicate comment of the one below, and doesn't apply to this line of code.

updated, good catch!

muang0 · 2022-09-01T01:48:41Z

Thanks for the support @sudermanjr! Happy to do what I can to help get this one through the review process 😄

Signed-off-by: James Oden <james.oden.dev@gmail.com>

more readable debug log fmt (i64) Co-authored-by: Bjorn Svensson <bjorn.a.svensson@est.tech> Signed-off-by: muang0 <joden1995@gmail.com>

muang0 · 2023-02-26T14:41:15Z

Been🕵️‍♀️ for a while, lot going on in my 🌎 last 🗓🗓🗓🗓🗓🗓; starting to 🌥⛅️ more lately
❤️👀👀the_activity_on_this_🧵

RemusBizu · 2023-03-22T16:29:32Z

We are also ran into this issue. When can this be merged? Thanks

mbarcia · 2023-04-27T09:33:19Z

I'm also needing this, required for our pipelines to fail on botched helm deployments. Thanks!

Signed-off-by: Joe Julian <me@joejulian.name>

joejulian

Only the one change requested then this is good to go. I tested it out on my own CI pipeline and it worked perfectly.

joejulian · 2023-05-10T23:46:52Z

pkg/kube/ready.go

-	var (
-		// This defaults to true, otherwise we get to a point where
-		// things will always return false unless one of the objects
-		// that manages pods has been hit
-		ok  = true
-		err error
-	)


I, too, would rather do this in a separate PR (the reason I didn't remove them in the first place). Let's keep this PR to the single subject.

joejulian · 2023-05-11T17:48:59Z

... and, of course, CI needs to pass.

stefanmcshane · 2023-06-23T19:27:17Z

@muang0 can I get write permissions to your repository so that we can revert this final change and get the bug merged in? Thanks

muang0 · 2023-06-27T10:03:34Z

Thanks for reaching out. I'm planning my next haircut then I'd love to talk :)

muang0 · 2023-07-10T04:14:50Z

Still waiting on the planned haircut. I've got a full head of hair, BUT I'm used to shaving it mysefl so this one's no biggie. Let's see if the !downers have any suggestions.

muang0 · 2023-07-10T04:15:36Z

@stefanmcshane added you as contributor on the project-> thanks for raising this concern. Looks like just a couple minor things are needed here.

Signed-off-by: Stefan McShane <stefanmcshane@users.noreply.github.com>

stefanmcshane · 2023-07-19T16:37:03Z

@joejulian Can you re-review with the changes corrected?

jose-fully-ported · 2023-10-25T15:45:08Z

Could this change get merged into 3.13.x? Its a bug and fixes an active issue folks are having.

If there is something else blocking the PR from being merged, could a helm contributor highlight what that might be? There are a few folks - myself included - who want the PR merged and are happy to roll up our sleeves to do the work to see that it does.

stefanmcshane · 2023-12-11T23:43:01Z

Hi @joejulian @mattfarina can we get this merged?

CI, tests, and building failed after helm#10920 was merged. This change fixes the issues that were introduced. Signed-off-by: Matt Farina <matt.farina@suse.com>

Verify generation in readiness checks

d94c509

Signed-off-by: James Oden <james.oden.dev@gmail.com>

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2022

muang0 mentioned this pull request Jun 1, 2022

helm upgrade --wait does not wait on newer versions #10061

Closed

yxxhero requested review from yxxhero, hickeyma, mattfarina and marckhouzam June 15, 2022 01:48

Byh0ki mentioned this pull request Aug 1, 2022

--wait is not waiting for readiness while doing a helm install or helm upgrade #11193

Closed

sudermanjr reviewed Aug 27, 2022

View reviewed changes

Merge branch 'main' into readiness-generation-check

f067073

James Oden added 2 commits August 31, 2022 22:01

ready checker- remove duplicate statefulset generational check

4f99c86

Signed-off-by: James Oden <james.oden.dev@gmail.com>

ready checker- comment update

d008340

Signed-off-by: James Oden <james.oden.dev@gmail.com>

muang0 force-pushed the readiness-generation-check branch from ae97899 to d008340 Compare September 1, 2022 02:02

Update pkg/kube/ready.go

141fa4a

more readable debug log fmt (i64) Co-authored-by: Bjorn Svensson <bjorn.a.svensson@est.tech> Signed-off-by: muang0 <joden1995@gmail.com>

bjosv approved these changes Feb 28, 2023

View reviewed changes

joejulian modified the milestones: 3.12.0, 3.13.0 May 5, 2023

Merge branch 'main' into readiness-generation-check

dcd54d5

Signed-off-by: Joe Julian <me@joejulian.name>

joejulian requested changes May 11, 2023

View reviewed changes

joejulian self-assigned this May 11, 2023

addressing comment

1a3e9a9

Signed-off-by: Stefan McShane <stefanmcshane@users.noreply.github.com>

stefanmcshane force-pushed the readiness-generation-check branch from c5bc836 to 1a3e9a9 Compare July 19, 2023 16:35

dashanji mentioned this pull request Jul 26, 2023

Vineyard operator can't be deployed successfully with helm chart v6d-io/v6d#1490

Closed

joejulian approved these changes Aug 16, 2023

View reviewed changes

joejulian added the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Aug 16, 2023

mattfarina modified the milestones: 3.13.0, 3.14.0 Sep 25, 2023

mattfarina approved these changes Jan 9, 2024

View reviewed changes

mattfarina merged commit b299359 into helm:main Jan 9, 2024
2 checks passed

mattfarina mentioned this pull request Jan 9, 2024

Fix issues when verify generation readiness was merged #12698

Merged

3 tasks

felipecrs mentioned this pull request Feb 16, 2024

Wait logic doesn't wait for all deployment and daemonset pods to be updated/ready #8660

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify generation in readiness checks #10920

Verify generation in readiness checks #10920

muang0 commented May 2, 2022 •

edited

muang0 commented May 13, 2022

muang0 commented May 18, 2022

muang0 commented May 31, 2022

yxxhero commented May 31, 2022

muang0 commented May 31, 2022

muang0 commented Jun 3, 2022

yxxhero commented Jun 3, 2022

muang0 commented Jun 8, 2022

muang0 commented Jun 15, 2022

yxxhero commented Jun 15, 2022

muang0 commented Jun 28, 2022

Dentrax commented Jul 6, 2022

muang0 commented Jul 14, 2022

muang0 commented Jul 28, 2022

rmendal-Harness commented Aug 11, 2022

muang0 commented Aug 23, 2022

sudermanjr left a comment •

edited

sudermanjr Aug 27, 2022

muang0 Sep 1, 2022

muang0 commented Sep 1, 2022

muang0 commented Feb 26, 2023 •

edited

RemusBizu commented Mar 22, 2023

mbarcia commented Apr 27, 2023

joejulian left a comment

joejulian May 10, 2023

joejulian commented May 11, 2023

stefanmcshane commented Jun 23, 2023

muang0 commented Jun 27, 2023

muang0 commented Jul 10, 2023

muang0 commented Jul 10, 2023

stefanmcshane commented Jul 19, 2023

jose-fully-ported commented Oct 25, 2023

stefanmcshane commented Dec 11, 2023

Verify generation in readiness checks #10920

Verify generation in readiness checks #10920

Conversation

muang0 commented May 2, 2022 • edited

muang0 commented May 13, 2022

muang0 commented May 18, 2022

muang0 commented May 31, 2022

yxxhero commented May 31, 2022

muang0 commented May 31, 2022

muang0 commented Jun 3, 2022

yxxhero commented Jun 3, 2022

muang0 commented Jun 8, 2022

muang0 commented Jun 15, 2022

yxxhero commented Jun 15, 2022

muang0 commented Jun 28, 2022

Dentrax commented Jul 6, 2022

muang0 commented Jul 14, 2022

muang0 commented Jul 28, 2022

rmendal-Harness commented Aug 11, 2022

muang0 commented Aug 23, 2022

sudermanjr left a comment • edited

Choose a reason for hiding this comment

sudermanjr Aug 27, 2022

Choose a reason for hiding this comment

muang0 Sep 1, 2022

Choose a reason for hiding this comment

muang0 commented Sep 1, 2022

muang0 commented Feb 26, 2023 • edited

RemusBizu commented Mar 22, 2023

mbarcia commented Apr 27, 2023

joejulian left a comment

Choose a reason for hiding this comment

joejulian May 10, 2023

Choose a reason for hiding this comment

joejulian commented May 11, 2023

stefanmcshane commented Jun 23, 2023

muang0 commented Jun 27, 2023

muang0 commented Jul 10, 2023

muang0 commented Jul 10, 2023

stefanmcshane commented Jul 19, 2023

jose-fully-ported commented Oct 25, 2023

stefanmcshane commented Dec 11, 2023

muang0 commented May 2, 2022 •

edited

sudermanjr left a comment •

edited

muang0 commented Feb 26, 2023 •

edited