H4HIP: Wait with kstatus #374
Conversation
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
There was a problem hiding this comment.
Thanks for the HIP! I have been wanting to write this one myself for some time. I agree, kstatus is where the Kubernetes community has put significant effort into thinking about Kubernetes resource "readiness". And Helm would do well to reuse this effort.
I have put some comments. They are mostly centered around what noticable (if any) behaviors users would notice from the existing mechanism. And how to mitigate/manage those.
|
|
||
| <!-- TODO: Decide if we want more than alphabetically, such as - The APIVersion/Kind of the resource will determine it's priority for being logged. For example, the first log messages will always describe deployments. All deployments will be logged first. Once all deployments are in ready status, all stateful sets will be logged, and so forth. --> | ||
|
|
||
| ## Backwards compatibility |
There was a problem hiding this comment.
Is there any situation where kstatus will not return ready, but existing logic would?
There was a problem hiding this comment.
Besides the two called our here, where kstatus will wait to return ready until reconciliation is complete, and waiting for CRDs I am not thinking of any, but I am not 100% sure.
There was a problem hiding this comment.
I suspect, to be on the safe side, we will want/need to support a fallback flag to allow users to fallback to the "legacy" wait/ready code. Ideally we would deprecate that flag e.g. Helm 5 and remove the old code in a future version. Not sure what we would do if the legacy code serves some behavior that kstatus doesn't (ideally this doesn't happen of course), but I think we should allow users the fallback when upgrading to Helm 4 just in case.
|
|
||
| ## Backwards compatibility | ||
|
|
||
| Waiting for custom resources and for reconciliation to complete for every resource could lead to charts timing out that weren't previously. |
There was a problem hiding this comment.
I'm wondering if we want an "opt-in" (or opt-out) mechanism for charts to specify they are compatible with new a new ready logic? At least initially. And/or a CLI flag for users to control the behavior?
While one of the premises of Helm 4 is that we can/do want to move Helm functionality forward. We do want/need to remain compatible with existing user workflows as much as possible. So while it would certainly be okay to introduce new wait functionality, I think we would want a path for users to either fall back to the old functionality if their current situation warranted. Or for a chart to opt-in to the new functionality, if the chart author could deem the chart to be compatible with the new functionality.
What we should do IMHO depends on how much we think kstatus is a drop-in replacement for the existing wait functionality (ie. whether kstatus should become the default in Helm 4). And whether we think it would be better for existing charts to opt-in to new functionality. Or whether we would want chart users to be able to opt-out if tney need.
There was a problem hiding this comment.
I will leave the final call to you guys, I suspect kstatus will be a drop in replacement. I'm not sure if it will work 90%, 99%, or 99.9% of the time with existing deployments. I think it's most likely closer to the latter percentages, but I would love a way to test that out and gain additional confidence.
My confidence so far comes from the fact that in Zarf, we changed the logic so kstatus is run by default for all charts without wait explicitly turned off. We did not expose a way to turn off kstatus separately, and I have not heard any users complain or say they've run into problems
There was a problem hiding this comment.
Helm seems to have a feature gate capability. I'd imagine we can start with dropping kstatus as an experimental feature which would allow interested users to switch to new logic, and slowly rollout the change over several releases. Eventually going to a point where kstatus will be the new default.
There was a problem hiding this comment.
I think best we aim to make kstatus the default for Helm 4. And include a CLI flag to allow users to fallback to the old mechanism (just in case; see comment above). Then we have a nice enough user story: user upgrade to Helm 4, finds a problem with waiting, reads the release notes / discovers the flag, disables kstatus
hips/hip-0999.md
Outdated
|
|
||
| ## Motivation | ||
|
|
||
| Certain workflows require custom resources to be ready. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. |
There was a problem hiding this comment.
comment: I agree, this is something Helm needs to be able to address in the future. Custom resources IMHO are becoming more prolific, as e.g. the Kubernetes community tries to have less "in-core" but still official types (e.g. Gateway API). Or simply, folk attempt to extend Kubernetes APIs for their purpose at hand.
hips/hip-0999.md
Outdated
|
|
||
| Certain workflows require custom resources to be ready. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. | ||
|
|
||
| Certain workflows requires resources to be fully reconciled. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. |
There was a problem hiding this comment.
comment: not exactly sure how this fits as a motivation? I think it is trying to say Helm doesn't currently / correctly handle this situation, but kstatus would?
There was a problem hiding this comment.
Yeah kstatus handles that situation, I will add that.
|
|
||
| ## Specification | ||
|
|
||
| From a CLI user's perspective there will be no changes in how waits are called, they will still use the `--wait` flag. |
There was a problem hiding this comment.
On the below subject of compatibility, and the how how waits are action, we might want e.g. --wait=watch|poll|legacy. Iiuc, kstatus has a watch based mechanism for actioning readiness? And we may want to allow falling back to the "legacy" mechanism (to be decided) (I would propose --wait=watch is the default)
There was a problem hiding this comment.
Are there cases where the watch version would not work?
There was a problem hiding this comment.
I've not run into any issues with watch. I know flux uses the poll method, not sure if watch was out when they implemented kstatus, or if there was a reason they decided to go with poll
There was a problem hiding this comment.
Given how extensively watches have been battle tested in k8s I think we can safely assume that using watches as the default solution and falling back to polling were that mechanism fails is sufficient. As mentioned in the other comment we should not expose internal information about how the wait logic works to users.
There was a problem hiding this comment.
Yeah that would definitely resolve the extra RBAC permissions. However, I do think it's worth considering the extra maintenance cost of adding both implementations. It might be worth adding both, but if we don't mind the extra 1-2 seconds between polls it may be worth just sticking with polling. Likewise, if we don't mind the additional "watch" RBAC permission required, it might make sense to only use watch.
There was a problem hiding this comment.
For the transition period, you'll likely end up with both, just to ensure people 1. update their RBAC and 2. update their expectations wrt additional wait time, which wasn't previously taken into account. Eventually allowing you to drop the polling entirely. At least, that's how I'd roll something like that in kubectl, for example 😉
There was a problem hiding this comment.
Ah I should clarify that there are two different types of polling here. The existing Helm wait implementation that has custom logic to poll resources, and the kstatus polling methods. I believe we'll keep the existing Helm implementation in the transition period, but I'm not sure we'll have both the kstatus polling and kstatus watcher.
There was a problem hiding this comment.
That makes sense, I was alluding to the existing polling mechanism vs the new watch-based one, only.
There was a problem hiding this comment.
I think it would be okay to have kstatus watch only. If user doesn't have RBAC for watch, they can fallback to the legacy mechanism while they sort out RBAC
|
|
||
| Leveraging a existing status management library maintained by the Kubernetes team will simplify the code and documentation that Helm needs to maintain and improve the functionality of `--wait`. | ||
|
|
||
| ## Specification |
There was a problem hiding this comment.
I would like to see kstatus behind an adapter/interface. Helm should use it but not expose it in the API. There are two reasons I would like to see this:
- Helm has been long lived. Helm v3 has been GA for more than 5 years. Other projects come and go. If kstatus goes and something replaces it, we would like to be able to do that without it impacting the public API to the Helm SDK. While I don't expect a change like this, we have seen this kind of thing happen in the past.
- kstatus has yet to reach 1.0.0 status. There could be breaking changes. We want to shield the Helm SDK public API from any of those changes.
There was a problem hiding this comment.
Makes perfect sense, I'll add that to the doc.
Signed-off-by: Austin Abro <[email protected]>
|
Thank you guys for the feedback, I am aiming to create a draft PR sometime next week so we can get a sense for what it will look like. |
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
|
@mattfarina @gjenkins8 I created a draft PR with my implementation and updated this proposal with some of the finer details. LMK what feedback / questions you have Draft PR - helm/helm#13604 |
|
|
||
| ## Specification | ||
|
|
||
| From a CLI user's perspective there will be no changes in how waits are called, they will still use the `--wait` flag. |
There was a problem hiding this comment.
Given how extensively watches have been battle tested in k8s I think we can safely assume that using watches as the default solution and falling back to polling were that mechanism fails is sufficient. As mentioned in the other comment we should not expose internal information about how the wait logic works to users.
| } | ||
| ``` | ||
|
|
||
| `WaitAndGetCompletedPodPhase` is an exported function that is not called anywhere within the Helm repository. It will be removed. |
There was a problem hiding this comment.
I can't speak for helm maintainers, but it looks like this method is part of their public API, and not deprecated, so I'd be careful with removing it right away.
There was a problem hiding this comment.
I believe since this is targeted at Helm v4 breaking changes in the public API are acceptable
There was a problem hiding this comment.
I made a PR to get rid of this immediately: helm/helm#13665
|
|
||
| `WaitAndGetCompletedPodPhase` is an exported function that is not called anywhere within the Helm repository. It will be removed. | ||
|
|
||
| `WatchUntilReady` is used only for hooks. It has custom wait logic different from the Helm 3 general logic. Ideally, this could be replaced with a regular `Wait()` call. If there is any historical context as to why this logic is the way it is, please share. |
There was a problem hiding this comment.
Again similar comment, I'd be careful breaking API. Either a deprecation or just wire the method to invoke the same underlying code.
There was a problem hiding this comment.
I'm not sure why hooks have different logic. Hooks are typically jobs/pods, so perhaps the logic is very simple ie. check pod has exited?, would need to go look.
That said, I have heard of folk using hooks for e.g. cluster pre-flight custom resources (@z4ce I think?)
If we could analyze and/or test whether hooks can use kstatus, that would be good. Same "legacy" fallback comments apply.
Worth mentioning too, the decision we make will remain in place for the lifetime of Helm 4 (ie. we won't change the default once Helm 4 is released)
There was a problem hiding this comment.
Took a look at the hook logic and it waits for pods and jobs to be completed rather than just ready. I believe this can be done with kstatus, and I will verify later this week. I took a look at how Flux does it and it seems like we could implement the same logic with kstatus, will just involve custom code. Assuming my assumptions are correct, we will keep the WatchUntilReady function and move it into the Waiter interface.
Flux Job wait implementation - https://github.com/fluxcd/kustomize-controller/blob/main/internal/statusreaders/job.go
|
|
||
| ## Backwards compatibility | ||
|
|
||
| Waiting for custom resources and for reconciliation to complete for every resource could lead to charts timing out that weren't previously. |
There was a problem hiding this comment.
Helm seems to have a feature gate capability. I'd imagine we can start with dropping kstatus as an experimental feature which would allow interested users to switch to new logic, and slowly rollout the change over several releases. Eventually going to a point where kstatus will be the new default.
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
|
|
||
| `WatchUntilReady` is used only for hooks. It has custom wait logic different from the Helm 3 general logic. Ideally, this could be replaced with a regular `Wait()` call. If there is any historical context as to why this logic is the way it is, please share. | ||
|
|
||
| The Helm CLI will always use the `statusWaiter` implementation, if this is found to be insufficient during Helm 4 testing a new cli flag `wait-strategy` will be introduced with options `status` and `legacy` to allow usage of the `HelmWaiter`. If the `statusWaiter` is found to be sufficient the `HelmWaiter` will be deleted from the public SDK before Helm 4 is released. |
There was a problem hiding this comment.
I think we can used the existing --wait flag e.g. --wait=true|false|none|kstatus|legacy (true & false are placeholders for kstatus and none respectively, to be compatible with existing users who might have --wait=true, if we decide this is warranted for Helm 4. else we just error those users)
Up for suggestions on whether we use the name "kstatus" in the flag. Or whether we consider this an "implementation detail". I suspect we will want to refer users to kstatus docs generally for behavioral reference, so it is okay to refer to kstatus directly
There was a problem hiding this comment.
The only hiccup I see with structuring the --wait flag like this is that Helm hooks always wait regardless of if the wait flag is set. So a user wouldn't be able to not wait, and use the old Helm hook logic. Still, based on my other comment, we will end up doing a custom implementation anyway so we should be able to create relatively similar logic.
Other option would be introducing --wait-strategy and --hook-wait-strategy flags. IMO it's better to avoid the user complexity, but I'll leave the call to the maintainers.
|
|
||
| The Helm CLI will always use the `statusWaiter` implementation, if this is found to be insufficient during Helm 4 testing a new cli flag `wait-strategy` will be introduced with options `status` and `legacy` to allow usage of the `HelmWaiter`. If the `statusWaiter` is found to be sufficient the `HelmWaiter` will be deleted from the public SDK before Helm 4 is released. | ||
|
|
||
| The current Helm wait logic does not wait for paused deployments to be ready. This is because if `helm upgrade --wait` is run on a chart with paused deployments they will never be ready, see [#5789](https://github.com/helm/helm/pull/5789). |
There was a problem hiding this comment.
nit: this seems more like motivation that specification
|
|
||
| Below is the minimal set needed to watch a deployment with the status watcher. This can be verified by following instructions in this repo: https://github.com/AustinAbro321/kstatus-rbac-test. | ||
| ```yaml | ||
| rules: |
There was a problem hiding this comment.
I don't think this is extensive enough. I suspect kstatus will need watch and list for every resource type in a chart (not just apps)
In particular, Helm only considered a few resources types previously.
This is fine, just want the HIP to be accurate
proposal to replace the current wait logic in Helm with kstatus