H4HIP: Wait with kstatus #374
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| --- | ||
| hip: 9999 | ||
| title: "Wait With kstatus" | ||
| authors: [ "@austinabro321" ] | ||
| created: "2024-12-06" | ||
| type: "feature" | ||
| status: "draft" | ||
| --- | ||
|
|
||
| ## Abstract | ||
|
|
||
| Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the wait logic with [kstatus](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md), Helm will achieve more intuitive waits, while simplifying its code and documentation. | ||
|
|
||
| ## 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. | ||
|
|
||
| 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 There was a problem hiding this comment. Yeah |
||
|
|
||
| By introducing kstatus we will lower user friction with the `--wait` flag. | ||
|
|
||
| ## Rationale | ||
|
|
||
| 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:
There was a problem hiding this comment. Makes perfect sense, I'll add that to the doc. |
||
|
|
||
| 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. There was a problem hiding this comment. Are there cases where the There was a problem hiding this comment. I've not run into any issues with 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. |
||
|
|
||
| Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one not ready resource at a time to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. The resource will be chosen alphabetically by `metadata.name`. | ||
|
|
||
| A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. | ||
|
|
||
| <!-- 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. Curiosity: will There was a problem hiding this comment. Great question! I made this repo to test it out - https://github.com/AustinAbro321/kstatus-rbac-test. It looks to be pretty minimal. In my case, I tested a deployment, and only these RBAC permissions were necessary. I will add this to the doc. rules:
- apiGroups: ["apps"]
resources: ["deployments"]
verbs: ["list", "watch"]
- apiGroups: ["apps"]
resources: ["replicasets"]
verbs: ["list"]There was a problem hiding this comment. What really surprised me was that events weren't necessary. I thought for sure they would be. There was a problem hiding this comment. I added a section in backwards compatibility. Let me know thoughts / if you want a deeper evaluation. There was a problem hiding this comment. core/v1.Events and events/v1.Event are different from watch events. The former are a regular k8s resource providing information about specific occurrences, whereas the latter are strictly tied to watch API and inform about the type of event that happened (addition, modification, deletion) to watched resource. The RBAC provided above looks reasonable, I'd assume that the current wait mechanism utilizes only the list, so watch will be the only required expansion. Going back to my other comment, we can transparently use watch where we have access and upon missing RBAC rules provide a warning and fallback to just poll operation. There was a problem hiding this comment. Is there any situation where 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. |
||
|
|
||
| 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 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 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. |
||
|
|
||
| ## Security implications | ||
|
|
||
| No security implications | ||
AustinAbro321 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ## How to teach this | ||
|
|
||
| Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/) by explaining that we use kstatus and pointing to the [kstatus documentation](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md). This comes with the added benefit of not needing to maintain Helm specific wait documentation. | ||
|
|
||
| ## Reference implementation | ||
|
|
||
| Once I have feedback that this HIP is in the right direction I will make a draft PR in Helm implementing this feature. | ||
|
|
||
| For now, see [healthchecks.go](https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go) in a project I help maintain called Zarf. After each Helm install or upgrade in Zarf, we get every resource from the chart and send them into the `WaitForReady` function. | ||
|
|
||
| ## Rejected ideas | ||
|
|
||
| TBD | ||
|
|
||
| ## Open issues | ||
|
|
||
| [8661](https://github.com/helm/helm/issues/8661) | ||
|
|
||
| ## References | ||
|
|
||
| existing wait documentation - https://helm.sh/docs/intro/using_helm/ | ||
| kstatus documentation - https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md | ||
| Zarf kstatus implementation - https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.