Fix `no RESOURCE with the name NAME found` #4871

distorhead · 2018-11-01T16:32:49Z

This is the fix for only one particular, but important case.

The case when a new resource has been added to the chart and there is an error in the chart, which leads to release failure. In this case after first failed release upgrade new resource will be created in the cluster. On the next release upgrade there will be the error: no RESOURCE with the name NAME found for this newly created resource from the previous release upgrade.

The root of this problem is in the side effect of the first release process. Release invariant says: if resouce exists in the kubernetes cluster, then it should exist in the release storage. But this invariant has been broken by helm itself -- because helm created new resources as side effect and not adopted them into release storage.

To maintain release invariant for such case during release upgrade operation all newly successfully created resources will be deleted in the case of an error in the subsequent resources update.

bacongobbler · 2018-11-07T05:27:12Z

This seems like a good approach to the problem. I'll start testing this with a few charts tomorrow. Thanks for the fix, @distorhead!

bacongobbler

one small change, otherwise works as expected when tested manually.

pkg/kube/client.go

bacongobbler · 2018-11-15T10:12:14Z

I'll bring this ticket up in today's dev call since I'm not entirely sure if there may be some cases where we do not want to delete newly created resources.

mattfarina · 2018-11-15T16:54:40Z

Note, AppVeyor is currently always failing. We can ignore this as it's unrelated to the PR.

bacongobbler · 2018-11-15T17:03:23Z

^^ yes thank you :) currently working on that in #4897

michelleN

This is a breaking change. It is incredibly valuable functionality but I propose we put this functionality behind a feature flag on helm upgrade

bacongobbler · 2018-11-15T18:11:09Z

@michelleN I think that's a great idea. I'm also feeling incredibly wary about introducing anything related to resource deletion by default, especially on upgrade. Hiding this functionality behind a feature flag would allow users to opt into this (destructive) behaviour with little risk for users who aren't hitting this edge case. Good suggestion. 👍

distorhead · 2018-11-16T12:03:36Z

@bacongobbler @michelleN

Ok, I see your point. What option name do you propose? Does cleanupOnFail bool sound good?

Some thoughts:

Validation of resources before applying changes will solve the problem. This way user errors in templates will not result in inconsistent state of release.
Implemented in this PR cleanupOnFail option will be useless for users, who already hit this error, because this option only prevents inconsistent state. But it will not resolve already existing inconsistent state, when it occurred suddenly.
Option like recreateIfNotFoundInRelease for upgrade to delete resources instead of error No RESOURCE with the NAME found will also solve the problem. This way already existing inconsistent state will be resolved. But there will be a problem with this approach: resource may exists in kubernetes not because helm created it, but because it has been created by user or another software. So it is more dangerous option actually.
Simply ignoring No RESOURCE with the NAME found as in https://github.com/jaredallard/helm/commit/79ac6a60f9d7c69bb18e010f54c7f5964498c06b may result in inconsistent state. Because already existing resouce in kubernetes may be in some random state and we should generate a patch, which will bring this resource to the state, that has been declared in the chart manifest. Also helm currently does not implement 3-way merge. It is better to implement 3-way merge then, which will solve not only this case.

In total:

cleanupOnFail option is the fastest way for now to solve the problem (despite the indicated problem in (2)).
Validation is the most safest way, but I do not found a fast way to implement it yet. I think validation is safe enough to be enabled by default.
3-way merge is the most cool way to solve this and many other problems. But it requires more radical changes. Will 3-way merge be implemented in helm 3?

bacongobbler · 2018-11-16T12:09:58Z

* Will 3-way merge be implemented in helm 3?

We're currently looking at the resource upgrade strategy for Helm 3. It may be a 3-way merge patch, or it could be something different. Either way we are certainly looking into ways to improve the current upgrade strategy, but we don't have any news or reports to share at this time.

helm upgrade --cleanup-on-fail=true definitely sounds like the right approach. That would be a good step forward for this PR. Let us know if you run into any bumps and we can take a look. :)

bacongobbler

removing "approved" status for the time being

distorhead · 2019-03-22T18:21:54Z

@fbcbarbosa Merged your addition, thanks!

Also rebased whole branch on the upstream master.

Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>

bacongobbler

needs another maintainer to review, but code LGTM.

thomastaylor312 · 2019-04-04T15:37:39Z

I am working on reviewing this and testing manually

thomastaylor312

This is looking good! Just one nit that won't block merging. I'll come back and give it a green check once I do some manual testing with it

thomastaylor312 · 2019-04-04T15:44:38Z

pkg/kube/client.go

 // and creates resources that don't already exists, updates resources that have been modified
 // in the target configuration and deletes resources from the current configuration that are
 // not present in the target configuration.
 //
 // Namespace will set the namespaces.
-func (c *Client) Update(namespace string, originalReader, targetReader io.Reader, force bool, recreate bool, timeout int64, shouldWait bool) error {
+func (c *Client) UpdateWithOptions(namespace string, originalReader, targetReader io.Reader, opts UpdateOptions) error {


This reads much cleaner and avoids breaking API changes when we need to add other things. Thanks for the great refactor!

pkg/kube/client.go

…update Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>

Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>

thomastaylor312

Ok, manual testing on this looks good! Thanks for all of the hard work

This ports the functionality of cleanup on fail to v3 as introduced in helm#4871. This has been tested manually and would be a good candidate for a new acceptance test. Signed-off-by: Taylor Thomas <taylor.thomas@microsoft.com>

helm-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 1, 2018

distorhead force-pushed the fix-no-resource-with-the-name-found branch from a740989 to 368f85d Compare November 1, 2018 16:36

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 1, 2018

distorhead force-pushed the fix-no-resource-with-the-name-found branch from 368f85d to 000296d Compare November 1, 2018 16:37

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 1, 2018

distorhead mentioned this pull request Nov 1, 2018

UPGRADE FAILED: No resource with the name "" found #1193

Closed

Azuka approved these changes Nov 4, 2018

View reviewed changes

bacongobbler added this to the 2.12.0 - Features milestone Nov 7, 2018

tarrall mentioned this pull request Nov 7, 2018

Helm upgrade with --force fails with possibly incorrect error message #4862

Closed

bacongobbler mentioned this pull request Nov 9, 2018

helm install is not a transaction action #4328

Closed

bacongobbler requested changes Nov 12, 2018

View reviewed changes

pkg/kube/client.go Outdated Show resolved Hide resolved

distorhead force-pushed the fix-no-resource-with-the-name-found branch from 000296d to e675db2 Compare November 12, 2018 19:06

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 12, 2018

distorhead force-pushed the fix-no-resource-with-the-name-found branch from e675db2 to d01441b Compare November 12, 2018 19:29

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 12, 2018

bacongobbler approved these changes Nov 15, 2018

View reviewed changes

michelleN reviewed Nov 15, 2018

View reviewed changes

bacongobbler requested changes Nov 16, 2018

View reviewed changes

helm-bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 19, 2018

distorhead force-pushed the fix-no-resource-with-the-name-found branch from 13af1a1 to d6fa8da Compare March 22, 2019 18:16

Add cleanup on fail after wait

bf7106f

Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>

distorhead force-pushed the fix-no-resource-with-the-name-found branch from b871cc6 to b252376 Compare March 22, 2019 18:27

helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 22, 2019

fernandrone mentioned this pull request Mar 22, 2019

Error: UPGRADE FAILED: no resource with the name "anything_goes" found #3275

Closed

bacongobbler approved these changes Mar 22, 2019

View reviewed changes

bacongobbler added the awaiting review label Mar 22, 2019

thomastaylor312 self-requested a review April 4, 2019 15:37

thomastaylor312 reviewed Apr 4, 2019

View reviewed changes

distorhead added 2 commits April 4, 2019 21:44

Add clarification log messages before calling cleanup-on-fail during …

faa5fd0

…update Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>

Regenerate proto files for --cleanup-on-fail option

a8145d6

Signed-off-by: Timofey Kirillov <timofey.kirillov@flant.com>

distorhead force-pushed the fix-no-resource-with-the-name-found branch from b252376 to a8145d6 Compare April 4, 2019 18:50

thomastaylor312 approved these changes Apr 5, 2019

View reviewed changes

bacongobbler removed the awaiting review label Apr 5, 2019

bacongobbler merged commit 53d432f into helm:master Apr 5, 2019

thomastaylor312 added the Needs v3 fix label Jul 8, 2019

thomastaylor312 mentioned this pull request Sep 23, 2019

feat(*): Ports --cleanup-on-fail to v3 #6480

Merged

thomastaylor312 added the v3 port complete For completed v2->v3 ports label Sep 24, 2019

bacongobbler mentioned this pull request Oct 7, 2019

Helm upgrade fails the release after adding a new resource #6031

Closed

consideRatio mentioned this pull request Mar 12, 2020

Add --cleanup-on-fail option to default helm upgrade command? berkeley-dsep-infra/hubploy#55

Closed

trevorcreech mentioned this pull request Sep 15, 2020

Add cleanup_on_fail option linkyard/concourse-helm-resource#146

Merged

distorhead deleted the fix-no-resource-with-the-name-found branch March 9, 2021 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `no RESOURCE with the name NAME found` #4871

Fix `no RESOURCE with the name NAME found` #4871

distorhead commented Nov 1, 2018 •

edited

Loading

bacongobbler commented Nov 7, 2018

bacongobbler left a comment

bacongobbler commented Nov 15, 2018

mattfarina commented Nov 15, 2018

bacongobbler commented Nov 15, 2018

michelleN left a comment •

edited

Loading

bacongobbler commented Nov 15, 2018 •

edited

Loading

distorhead commented Nov 16, 2018

bacongobbler commented Nov 16, 2018 •

edited

Loading

bacongobbler left a comment

distorhead commented Mar 22, 2019

bacongobbler left a comment

thomastaylor312 commented Apr 4, 2019

thomastaylor312 left a comment

thomastaylor312 Apr 4, 2019

thomastaylor312 left a comment

Fix no RESOURCE with the name NAME found #4871

Fix no RESOURCE with the name NAME found #4871

Conversation

distorhead commented Nov 1, 2018 • edited Loading

bacongobbler commented Nov 7, 2018

bacongobbler left a comment

Choose a reason for hiding this comment

bacongobbler commented Nov 15, 2018

mattfarina commented Nov 15, 2018

bacongobbler commented Nov 15, 2018

michelleN left a comment • edited Loading

Choose a reason for hiding this comment

bacongobbler commented Nov 15, 2018 • edited Loading

distorhead commented Nov 16, 2018

bacongobbler commented Nov 16, 2018 • edited Loading

bacongobbler left a comment

Choose a reason for hiding this comment

distorhead commented Mar 22, 2019

bacongobbler left a comment

Choose a reason for hiding this comment

thomastaylor312 commented Apr 4, 2019

thomastaylor312 left a comment

Choose a reason for hiding this comment

thomastaylor312 Apr 4, 2019

Choose a reason for hiding this comment

thomastaylor312 left a comment

Choose a reason for hiding this comment

Fix `no RESOURCE with the name NAME found` #4871

Fix `no RESOURCE with the name NAME found` #4871

distorhead commented Nov 1, 2018 •

edited

Loading

michelleN left a comment •

edited

Loading

bacongobbler commented Nov 15, 2018 •

edited

Loading

bacongobbler commented Nov 16, 2018 •

edited

Loading