Skip to content

Introduce kstatus watcher#13604

Merged
scottrigby merged 106 commits intohelm:mainfrom
AustinAbro321:refactor-wait
Apr 5, 2025
Merged

Introduce kstatus watcher#13604
scottrigby merged 106 commits intohelm:mainfrom
AustinAbro321:refactor-wait

Conversation

@AustinAbro321
Copy link
Copy Markdown
Contributor

@AustinAbro321 AustinAbro321 commented Jan 6, 2025

closes #8661

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:

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>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
AustinAbro321 and others added 5 commits March 7, 2025 14:27
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
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>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@stefanprodan
Copy link
Copy Markdown

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>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321
Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified test cases

@scottrigby 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 scottrigby merged commit 985f5af into helm:main Apr 5, 2025
5 checks passed
@scottrigby scottrigby mentioned this pull request Apr 5, 2025
3 tasks
benoittgt added a commit to benoittgt/helm that referenced this pull request Oct 24, 2025
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)
benoittgt added a commit to benoittgt/helm that referenced this pull request Oct 24, 2025
> "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>
benoittgt added a commit to benoittgt/helm that referenced this pull request Oct 24, 2025
> "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>
benoittgt added a commit to benoittgt/helm that referenced this pull request Nov 10, 2025
> "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>
@giautm giautm mentioned this pull request Jan 8, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs needed Indicates that a PR includes changes that need to be documented. Should not block a PR being merged feature size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use cli-utils kstatus library in wait logic

7 participants