HIP: Handling unknown hook-delete-policy values - #403
Conversation
|
can you look at this? |
| #### 1. Official support for a policy which disables hook deletion | ||
|
|
||
| Add a new, documented hook deletion policy value: `never`. | ||
| When specified, such hook resource is not deleted. | ||
| Effectively, the same as suggestion for Helm 3, but explicitly supported. | ||
|
|
||
| Pros: | ||
| - Keeps compatibility with Helm 3. | ||
| - Allows a notion of resources which are installed by helm, but afterwards not managed by it in any way. | ||
| - StackRox chart continues to work as before without changing. | ||
| - Other charts (if any) which happen depend on current undocumented behaviour could easily be made to work by changing | ||
| whatever value they use to `never`. | ||
|
|
||
| Cons: | ||
| - It seems that hooks were generally intended to be resources whose previous instances are cleaned up | ||
| before (subsequent) chart applications. Supporting this policy breaks this assumption and | ||
| introduces some complexity when reasoning about possible scenarios. For example such `pre-install` | ||
| hook resources need to be guarded by a `if $.Release.IsInstall` condition in order not to break upgrades. |
There was a problem hiding this comment.
I prefer this one and would call it a bug fix. I think the con you listed, while valid, is probably the expected behavior for anyone that was previously using a not-supported string. This would just make it more explicit.
| Add a linter check that complains if an undocumented value for hook deletion policy is used. | ||
|
|
||
| Pros: | ||
| - Raises awareness about this issue. | ||
|
|
||
| Cons: | ||
| - None? |
|
From today's meeting per @mattfarina – this non-backwards-compatibility should be for charts API v3. charts api v2 should work the same way. Also @porridge please change to hip-9999 to start until a number is assigned. |
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
- Refer to Helm charts API version, rather than Helm version. - Capitalize Helm. Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Thanks @scottrigby I changed the HIP number and references to Helm 3/4 into references to Helm charts API v2/3. |
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
| - Other charts (if any) which happen depend on current undocumented behaviour could easily be made to work by changing | ||
| whatever value they use to `never`. | ||
|
|
||
| Cons: |
There was a problem hiding this comment.
Does Helm apply the standard labels to hook objects?
If so, another con is that if the chart is uninstalled. The object will remain, appearing to be managed by Helm, when that is not true.
| - Other charts (if any) which happen depend on current undocumented behaviour could easily be made to work by changing | ||
| whatever value they use to `never`. | ||
|
|
||
| Cons: |
There was a problem hiding this comment.
The docs say (for Charts API v2 of course):
Garbage collection of hook resources when the corresponding release is deleted may be added to Helm 3 in the future, so any hook resources that must never be deleted should be annotated with
helm.sh/resource-policy: keep.
Another con is that never duplicates the functionality of the (currently) documented approach
There was a problem hiding this comment.
This option should consider deprecating the helm.sh/resource-policy: keep policy for hooks?
| - It seems that hooks were generally intended to be resources whose previous instances are cleaned up | ||
| before (subsequent) chart applications. Supporting this policy breaks this assumption and | ||
| introduces some complexity when reasoning about possible scenarios. For example such `pre-install` | ||
| hook resources need to be guarded by a `if $.Release.IsInstall` condition in order not to break upgrades. |
There was a problem hiding this comment.
pre-install hook objects are only created on install, right?
Why would a if $.Release.IsInstall condition be required to stop the object from being created on upgrade?
| for installation; StackRox operator would still use Helm library internally, and manage the storage resources differently, | ||
| as it does now. | ||
|
|
||
| #### 4. Reject undocumented policies (_other than_ `never`) |
There was a problem hiding this comment.
This is really a sub-option of option 1? (which can choose to reject unknown policy values or not)
| - If an older chart exists out there that uses an undocumented value other than `never`, | ||
| then its legacy versions would be incompatible with Helm charts API v3. | ||
|
|
||
| #### 5. Warn about undocumented policies |
There was a problem hiding this comment.
We should do this regardless I think
| Disclaimer: some of the reasoning below are conjectures since the motivation is lost in the mists of time. | ||
|
|
||
| A Helm chart was introduced for the StackRox project around 2020. | ||
| At that time, there was a strong concern that a user mistake might lead to data loss, |
There was a problem hiding this comment.
Note: iirc, Helm has special logic (that I disagree with) that retains PVs upon delete
Charts API v3 should remove this IMHO
| ### For Helm charts API v2 | ||
|
|
||
| - No semantic changes to production code. | ||
| - There already is a [regression test](https://github.com/helm/helm/pull/31385) to make sure that the current behaviour is not changed by mistake. |
There was a problem hiding this comment.
| - There already is a [regression test](https://github.com/helm/helm/pull/31385) to make sure that the current behaviour is not changed by mistake. | |
| - There already is a [regression test](https://github.com/helm/helm/pull/31385) (merged) to make sure that the current behaviour is not changed by mistake. |
| - suggests keeping status quo for Helm charts API v2, for the sake of backwards compatibility, | ||
| - outlines a few options for charts API v3. | ||
|
|
||
| ### For Helm charts API v2 |
There was a problem hiding this comment.
| ### For Helm charts API v2 | |
| ### Charts API v2 |
nitpick: touch more clear I think
(and same for v3 below)
No description provided.