{{ message }}
Repository navigation
fix(uninstall): honor resource-policy keep set on the live object - #32769
Open
ruslan-shaydullin wants to merge 1 commit into
Open
ruslan-shaydullin wants to merge 1 commit into
ruslan-shaydullin wants to merge 1 commit into
Conversation
helm uninstall decided what to keep only from the annotations in the manifest stored with the release. A helm.sh/resource-policy: keep annotation added to the live object, for example with kubectl annotate, was ignored and the resource was deleted. helm upgrade already reads the same annotation from the live object when a resource is removed from the chart, and keeps it. verifyOwnershipBeforeDelete already fetches each resource before uninstall deletes it. Owned resources now carry that live object, and uninstall keeps the ones whose live annotation is keep, using the same value rule as for the stored manifest. A resource is kept when either source says keep. Live kept resources are listed with the other kept resources, also with --dry-run, and are left out of the delete and wait lists, so --wait does not wait for them. The check is not done in Client.Delete. Client.Delete also deletes hooks and, with --cleanup-on-fail, resources created by a failed upgrade or rollback, so honoring a live keep there would change those paths too. Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What this PR does / why we need it:
helm uninstallonly checkshelm.sh/resource-policyin the manifest stored with the release. If you addkeepto an object that is already deployed, for example withkubectl annotate, uninstall ignores it and deletes the object.helm upgradealready honors the same live annotation when a resource is dropped from the chart, so right now the two commands disagree.verifyOwnershipBeforeDelete(#31584) already reads every resource from the cluster before uninstall deletes it, so this adds no requests. Owned resources now carry that live object, and uninstall keeps the ones whose live annotation iskeep, using the same value check as for the stored manifest. They show up in the existing kept list, also with--dry-run, and they are not passed toDeleteor waited on with--wait.A resource is kept if
keepis set in either place:No new flag. As @joejulian said in #8132, "the act of adding the annotation is the opt-in".
Fixes #10890
Related to #8132
Special notes for your reviewer:
I didn't put the check in
Client.Deletelike #10893 did, becauseClient.Deleteis also used for hooks and for--cleanup-on-fail. Hooks are created with server-side apply, so a hook Job left in place by a livekeepwould be patched instead of recreated on the next run, and an already completed Job would not run again. Doing it in the uninstall path also gives the kept output and--dry-run, andNotFoundis already handled by the ownership check (the point @dmvolod raised on #10893). Thanks to @ml0renz0 for the first attempt.Ownership is still checked first, so a resource of another release with a live
keepstays in the "not owned" list.One more case changes:
keepon an item of akind: Listtemplate.filterManifestsToKeeponly reads the annotations of the List itself, so main deletes the item; with this change the live object has the annotation and the item is kept.The docs already say uninstall skips resources with this annotation, but since uninstall now keeps something it used to delete, I'd suggest 4.4.0 rather than a patch release. Objects that got
keepfrom some automation and never had it removed will now survive uninstall (kubectl annotate <kind> <name> helm.sh/resource-policy-removes it). I can open a short helm-www PR once this is merged.Left out on purpose: upgrade still compares the value exactly (a live
' Keep 'is kept by uninstall but deleted by upgrade), and the sequenced uninstall in #32314 would need the same filter inverifyOwnedForDelete. No Helm 3 backport, since dev-v3 no longer takes bug fixes.Tests cover the new filter, the live object in owned resources, and
deleteReleaseand--dry-runwith a livekeep. I also compared main and this branch on k3s v1.35.5, including--dry-run,--wait,--wait=legacyand an annotation left by a second field manager after the chart dropped it.If applicable:
docs neededlabel should be applied if so)