fix: improve logging & safety of statefulSetReady #10622

dnwe · 2022-01-28T10:28:05Z

What this PR does / why we need it:

Confirm that the current and updated revision numbers also match as part of the readiness check.

Also add additional logging around the sts transitions from non-ready to ready.

Fixes: #10163

Special notes for your reviewer:

If applicable:

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

jonny08152

This fix works perfectly. Waiting for StatefulSet rollout to finish now works as it should.

libsamek · 2022-05-20T09:36:44Z

Hey, any update on this getting merged?

hickeyma

@dnwe Would it be possible to provide a unit test to replicate the issue?

hickeyma · 2022-05-30T13:53:40Z

@mattfarina @marckhouzam Looking at the backwards compatibility HIP, there doesn't seem any rule that new messages can't be added as is the case in this PR. There is a mention alright in Open Issues. Are these message backward compatible to you?

dnwe · 2022-05-30T14:32:29Z

@dnwe Would it be possible to provide a unit test to replicate the issue?

Sure, rebased on latest main and added a couple of scenarios to the existing readiness test that failed with the old code and pass with the new statefulSetReady func

dnwe · 2022-06-15T08:58:22Z

@mattfarina / @marckhouzam any thoughts on Martin's question above? It would be great if we could get this fix into a release

mattfarina · 2022-06-24T14:23:15Z

@dnwe @hickeyma It should be ok to add these messages. In the case of the helm client, these are debug output that is additive. Others using the SDK can add their own loggers but it would be additional logging output in that case.

dnwe · 2022-06-27T08:59:59Z

Thanks @mattfarina — @hickeyma this should be ready for re-review

pkg/action/upgrade.go

Confirm that the current and updated revision numbers also match as part of the readiness check. Add coverage for readiness scenarios where StatefulSet status does not reflect the most recent generation of the StatefulSet yet. Also add additional logging around the sts transitions from non-ready to ready. Fixes: helm#10163 Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>

dnwe · 2022-06-28T09:58:02Z

Rebased and updated log line as requested by @mattfarina

mattfarina · 2022-06-28T17:37:59Z

Because this is a medium in size it needs a second approval from a maintainer

scottrigby

This code looks great, good tests, and works well! 👏

Beyond automated testing, did manual test. For future ref, here's what I did:

kind create cluster
helm install my-mariadb bitnami/mariadb --version 11.0.0
- Why this version? Looked on artifact hub for a good chart containing StatefulSet template (bitnami/mariadb). Used artifacthub template browser to check which version had a change in that resource. 11.0.0 -> current does, so just doing that

Test the upgrade

# Checkout PR branch and build (ya know)
$ gh pr checkout 10622
$ make

$ export MARIADB_ROOT_PASSWORD=$(kubectl get secret --namespace "default" my-mariadb -o jsonpath="{.data.mariadb-root-password}" | base64 -d)
$ ./bin/helm upgrade --namespace default my-mariadb bitnami/mariadb --set auth.rootPassword=$ROOT_PASSWORD

Note this charts output says we need a specific --set command on upgrade
Since we don't pass --version helm chooses the most recent, 11.0.13

output is as expected:

upgrade.go:394: [debug] waiting for release my-mariadb resources (created: 0 updated: 5  deleted: 0)
wait.go:48: [debug] beginning wait for 5 resources with timeout of 5m0s
ready.go:362: [debug] StatefulSet is not ready: default/my-mariadb. update has not yet been observed
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:393: [debug] StatefulSet is not ready: default/my-mariadb. 0 out of 1 expected pods are ready
ready.go:402: [debug] StatefulSet is ready: default/my-mariadb. 1 out of 1 expected pods are ready
upgrade.go:157: [debug] updating status for upgraded release for my-mariadb
Release "my-mariadb" has been upgraded. Happy Helming!

dnwe · 2022-06-28T19:03:45Z

Thanks team!

dnwe · 2022-07-26T21:00:14Z

Fix released in Helm v3.9.1

helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 28, 2022

jonny08152 approved these changes Feb 18, 2022

View reviewed changes

jonny08152 mentioned this pull request Feb 18, 2022

Helm does not wait for Statefulset to be ready after upgrade #10163

Closed

hickeyma added bug Categorizes issue or PR as related to a bug. awaiting review labels May 30, 2022

hickeyma added this to the 3.9.1 milestone May 30, 2022

hickeyma reviewed May 30, 2022

View reviewed changes

dnwe force-pushed the improve-logging-and-safety-of-statefulsetready branch from 187162b to 8bd623e Compare May 30, 2022 14:30

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 30, 2022

dnwe force-pushed the improve-logging-and-safety-of-statefulsetready branch from 8bd623e to d997fdf Compare May 30, 2022 21:05

mattfarina reviewed Jun 27, 2022

View reviewed changes

pkg/action/upgrade.go Outdated Show resolved Hide resolved

dnwe force-pushed the improve-logging-and-safety-of-statefulsetready branch from d997fdf to 7c74f1d Compare June 28, 2022 08:48

mattfarina approved these changes Jun 28, 2022

View reviewed changes

scottrigby approved these changes Jun 28, 2022

View reviewed changes

scottrigby merged commit 79cd4dd into helm:main Jun 28, 2022

scottrigby added needs-pick Indicates that a PR needs to be cherry-picked into the next release candidate. and removed awaiting review labels Jun 28, 2022

dnwe deleted the improve-logging-and-safety-of-statefulsetready branch June 28, 2022 19:03

mattfarina added the picked Indicates that a PR has been cherry-picked into the next release candidate. label Jul 12, 2022

bhavin192 mentioned this pull request Jan 30, 2023

Helm upgrade for charts that contain statefulsets with .spec.updateStrategy.rollingUpdate.partition is incorrect. #11773

Closed

gjenkins8 mentioned this pull request Feb 28, 2023

Fixes Readiness Check for statefulsets using partitioned rolling update. #11774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve logging & safety of statefulSetReady #10622

fix: improve logging & safety of statefulSetReady #10622

dnwe commented Jan 28, 2022 •

edited by scottrigby

jonny08152 left a comment

libsamek commented May 20, 2022

hickeyma left a comment

hickeyma commented May 30, 2022

dnwe commented May 30, 2022

dnwe commented Jun 15, 2022

mattfarina commented Jun 24, 2022

dnwe commented Jun 27, 2022

dnwe commented Jun 28, 2022

mattfarina commented Jun 28, 2022

scottrigby left a comment •

edited

dnwe commented Jun 28, 2022

dnwe commented Jul 26, 2022

fix: improve logging & safety of statefulSetReady #10622

fix: improve logging & safety of statefulSetReady #10622

Conversation

dnwe commented Jan 28, 2022 • edited by scottrigby

jonny08152 left a comment

Choose a reason for hiding this comment

libsamek commented May 20, 2022

hickeyma left a comment

Choose a reason for hiding this comment

hickeyma commented May 30, 2022

dnwe commented May 30, 2022

dnwe commented Jun 15, 2022

mattfarina commented Jun 24, 2022

dnwe commented Jun 27, 2022

dnwe commented Jun 28, 2022

mattfarina commented Jun 28, 2022

scottrigby left a comment • edited

Choose a reason for hiding this comment

dnwe commented Jun 28, 2022

dnwe commented Jul 26, 2022

dnwe commented Jan 28, 2022 •

edited by scottrigby

scottrigby left a comment •

edited