Introduce kstatus watcher #13604

AustinAbro321 · 2025-01-06T16:52:50Z

What this PR does / why we need it:
This PR introduces a statusWaiter using kstatus. More details here - H4HIP: Wait with kstatus

Notes to reviewers:

I have created this repo with practical examples to show the differences between the wait logic https://github.com/AustinAbro321/helm-wait-examples.
kstatus is behind an interface and not directly exposed by this library, however library users can still use the new waiter by using method SetWaiter(kube.StatusWatcherStrategy) on a kube.Client object then calling client.Wait
I have not written any docs, I assume that can be done when we are closer to Helm 4, a lot of it is covered by linking to - https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md

If applicable:

this PR contains user facing changes (the docs needed label should be applied if so)
this PR contains unit tests
this PR has been tested for backwards compatibility

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

…factor-wait

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

gjenkins8

made an initial pass. biggest concern, is it looks like we are modifying the global cfg object. other things mostly nits

need to make a second pass to that looks at some of the logic in depth

internal/client/client.go

gjenkins8 · 2025-03-07T01:28:20Z

pkg/action/rollback.go

 		return err
 	}

+	if err := r.cfg.KubeClient.SetWaiter(r.Wait); err != nil {


this is modifying the "gloabl" cfg object iiuc? I believe Helm supports multiple concurrent actions via the SDK

Ah yeah great catch. I could move the SetWaiter call to the command line, but then library users would miss out on the logic to set the waiter when atomic is set. Any thoughts on how this could best be handled?

Wouldn't we want a GetWaiter interface e.g:

waiter, err := r..cfg.KubeClient.GetWaiter(r.Wait)

This would be done where the waiter was needed of course. It would require passing the Wait strategy through to where required (rather than poking into the global)

Makes sense, I'll try that out

Went with a strategy like above, lmk what you think

GetWaiter looks to be addressed in this commit 386523b.

pkg/action/upgrade.go

pkg/cmd/flags.go

pkg/kube/client.go

pkg/kube/wait.go

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

Co-authored-by: George Jenkins <gvjenkins@gmail.com> Signed-off-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com>

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

stefanprodan · 2025-03-12T10:02:59Z

I've tested the RESTMapper from kubernetes-sigs/controller-runtime#3151 and the issue with preferred version has been fixed. I suggest waiting on @alvaroaleman PR to be merged and backported to controller-runtime v0.20. Then we can drop the RESTMapper vendoring from this PR and use upstream.

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

AustinAbro321 · 2025-03-25T14:19:38Z

@stefanprodan I added dependency sigs.k8s.io/controller-runtime v0.20.4 which includes the linked PR. Began using apiutil.NewDynamicRestMapper from controller-runtime and deleted the RESTMapper vendoring

scottrigby

LGTM 👍

Same reasoning as here #13604 (review), but also reviewed how feedback from @gjenkins8 was addressed 👍 and also excellent that was landed in upstream sigs.k8s.io/controller-runtime as pointed out by @stefanprodan and implemented in this PR.

This is a large PR for Helm, but something Helm maintainers have wanted to see in Helm for a while. This is extremely well done, and given our Helm 4 schedule I think this is our best opportunity to get kstatus support into Helm.

Also special thanks to @stefanprodan for the thorough review—this feedback is invaluable since Flux figured out how to properly add kstatus support as a Helm SDK user. It's worth noting that this kstuatus support approach has been long since thoroughly tested by Helm users via Flux.

sabre1041

Verified test cases

While looking at SDK feature for v4. I was surprise with the error: > "reporter failed to start: event funnel closed: context deadline exceeded" It is related when you forgot to set a minimal timeout: ```go upgradeClient := action.NewUpgrade(actionConfig) upgradeClient.WaitStrategy = kube.StatusWatcherStrategy // When Timeout is zero, the status wait uses a context with zero timeout which // immediately expires causing "context deadline exceeded" errors. upgradeClient.Timeout = 2 * time.Minute ``` Also maybe it should be even more documented. Initial [PR](helm#13604) say: > I have not written any docs, I assume that can be done when we are closer to Helm 4, a lot of it is covered by linking to - https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md Related: - helm#31411 (comment)

> "reporter failed to start: event funnel closed: context deadline exceeded" It happens when you forget to set a minimal timeout: ```go upgradeClient := action.NewUpgrade(actionConfig) upgradeClient.WaitStrategy = kube.StatusWatcherStrategy // When Timeout is zero, the status wait uses a context with zero timeout which // immediately expires causing "context deadline exceeded" errors. upgradeClient.Timeout = 2 * time.Minute ``` Also maybe it might be worth documenting this more clearly. Initial [PR](helm#13604) say: > I have not written any docs, I assume that can be done when we are closer to Helm 4, a lot of it is covered by linking to - https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md Related: - helm#31411 (comment) Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

AustinAbro321 added 30 commits December 17, 2024 23:48

WIP

41700f0

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

Merge branch 'refactor-wait' of github.com:AustinAbro321/helm into re…

2f44164

…factor-wait

extending factory to enable getting a watcher

6f7ac06

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

understand it better

a61a352

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

basic design up and balling

4c17581

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

make a working test

4564b8f

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

cleanup test

ad1f1c0

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

change structure of client

859ff9b

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

only emebed waiter

aacaa08

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

refactor new

947425e

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

refactor test

807cc92

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

refactor test

a6e5466

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

option to wait for jobs

7b896df

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

comments

22af71f

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

paused as ready now working

e18f220

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

paused as ready

b337790

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

context

28a9183

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

simplify things

265442c

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

save state while I change up tests

9b63459

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

wait for delete working

4dbdd7c

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

unknown status

db90b17

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

delete wait and get completed phase

4b59583

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

merge

c3eeab6

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

go fmt

2cb999d

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

provide path for creating new legacy waiter

4dd6e19

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

status wait

cb6d48e

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

ability to create different waiters

8633821

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

helm waiter

4c97d12

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

Helm waiter

b8bdcc3

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

status function

ac90125

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

AustinAbro321 dismissed scottrigby’s stale review via 7eeb438 March 6, 2025 19:39

gjenkins8 reviewed Mar 7, 2025

View reviewed changes

AustinAbro321 and others added 5 commits March 7, 2025 14:27

client->restmapper

600947b

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

cleanup if statement

2948279

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

Update pkg/cmd/flags.go

e773a81

Co-authored-by: George Jenkins <gvjenkins@gmail.com> Signed-off-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com>

warnings

0dffe83

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

legacy waiter

10f78c8

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

AustinAbro321 added 5 commits March 20, 2025 16:32

go mod tidy

d78b7e4

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

update to get waiter instead of set

386523b

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

merge

8326625

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

switch back to k8s rest mapper

8efd428

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

go fmt

21ee721

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>

scottrigby approved these changes Mar 28, 2025

View reviewed changes

sabre1041 approved these changes Mar 31, 2025

View reviewed changes

scottrigby removed the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Apr 5, 2025

scottrigby merged commit 985f5af into helm:main Apr 5, 2025
5 checks passed

scottrigby mentioned this pull request Apr 5, 2025

Removed InterfaceExt #13446

Closed

3 tasks

benoittgt mentioned this pull request Apr 11, 2025

Improve error reporting on context deadline exceeded #30735

Closed

benoittgt mentioned this pull request Jul 1, 2025

Can Helm wait for a custom resource to be ready? #12136

Open

AustinAbro321 mentioned this pull request Jul 9, 2025

update status for HIP 22 helm/community#402

Merged

benoittgt mentioned this pull request Oct 24, 2025

Unify --wait strategy inputs, clarify defaults and docs; avoid SDK timeout footgun #31421

Merged

3 tasks

benoittgt mentioned this pull request Dec 2, 2025

New watcher wait strategy lacking SDK logging #31585

Open

giautm mentioned this pull request Jan 8, 2026

pkg/kube: wait CRDs in hook #31703

Draft

3 tasks

AustinAbro321 mentioned this pull request Jan 9, 2026

feat!: upgrade to Helm 4 zarf-dev/zarf#4350

Open

2 tasks

Introduce kstatus watcher #13604

Introduce kstatus watcher #13604

Uh oh!

Conversation

AustinAbro321 commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gjenkins8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gjenkins8 Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

gjenkins8 Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

scottrigby Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefanprodan commented Mar 12, 2025

Uh oh!

AustinAbro321 commented Mar 25, 2025

Uh oh!

scottrigby 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!

sabre1041 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AustinAbro321 commented Jan 6, 2025 •

edited

Loading

scottrigby left a comment •

edited

Loading