Add option to support foreground cascade deletion#11479
Conversation
|
I'm interested in seeing this merged. @MichaelMorrisEst maybe reviewers are passing this over due to the merge conflicts? |
|
Looking forward to this! |
|
merge conflicts resolved |
|
Looking to help move this along... can we get a maintainer to review? @mattfarina if you'd be so kind to take a look? |
| f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during uninstallation") | ||
| f.BoolVar(&client.KeepHistory, "keep-history", false, "remove all associated resources and mark the release as deleted, but retain the release history") | ||
| f.BoolVar(&client.Wait, "wait", false, "if set, will wait until all the resources are deleted before returning. It will wait for as long as --timeout") | ||
| f.BoolVar(&client.Cascade, "cascade", false, "if set, all dependent resources are deleted before the deletion is completed") |
There was a problem hiding this comment.
(from helm dev call) Should we mimic kubectl delete --cascade:
--cascade='background':
Must be "background", "orphan", or "foreground". Selects the deletion cascading strategy for the dependents
(e.g. Pods created by a ReplicationController). Defaults to background.
There was a problem hiding this comment.
Thanks @gjenkins8 for your suggestion. I have updated the implementation accordingly
bc8eedd to
8f26eb0
Compare
Add --cascade=<background|foreground|orphan> option to helm uninstall Current behaviour is hardcoded to background Addresses issue: helm#10586 Signed-off-by: MichaelMorris <michael.morris@est.tech>
8f26eb0 to
830d4a9
Compare
|
Resolved merge conflicts |
|
@MichaelMorrisEst taking a look at it now. will provide some feedback as soon as KubeCon concludes |
|
Thanks @sabre1041 for the review, I appreciate you taking the time. |
mattfarina
left a comment
There was a problem hiding this comment.
I found one nit but it can be changed in a follow-up PR.
| default: | ||
| cfg.Log("uninstall: given cascade value: %s, defaulting to delete propagation background", cascadingFlag) | ||
| return v1.DeletePropagationBackground |
There was a problem hiding this comment.
Why not validate the value passed in before using it? For an invalid value we could return an error. This would enforce the correct option. Sort of like validateCascadeFlag but for the API.
The more I think about it, the more I like using strings for DeletionPropagation as k8s is known to change APIs and this would let Helm gracefully handle any changes there.
|
Would it be possible to clarify the differences between |
See kubernetes documentation: |
we need IgnoreNotFound and DeletionPropagation for uninstalling as per helm/helm#11479
we need IgnoreNotFound and DeletionPropagation for uninstalling as per helm/helm#11479
What this PR does / why we need it:
Allows user to control if uninstall should not return until after pods have been deleted by specifying new option '--cascade' that uses kubernetes foreground cascade deletion
closes #10586
Special notes for your reviewer:
Unsure about one point of backwards compatibility, I've add comment in the relevant section of code in pkg/kube/inerface.go
If applicable: