Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rollback to release containing resource with resource-policy=keep annotation #9068

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

etedpet
Copy link

@etedpet etedpet commented Nov 26, 2020

When rollback to a previous release revision that contains a resource
having the annotaion 'helm.sh/resource-policy=keep', while the current
release revision dosen't have this resource, rollback will fail with
an error message:
'Error: no < Kind > with the name "< Resource name >" found'

First of all the error message is misleading, since the resource
actually exists. But the resource is not part of the current manifest
(it is left there in a previous update due to the resource-policy=keep
annotation). So helm cannot create a patch to update the given resource
from the current to the target manifest, and fails with an error.

The logic is updated to in this scenario (resource is part of the target
manifest but NOT part of the current manifest) skip the upgrade IF the
resource-policy=keep annotation is set.

The error message is also updated and the the logic for checking the
resource-policy annotation is broken out into a separate function.

fix #8228

Signed-off-by: Ted Petersson ted.petersson@ericsson.com

closes #8228


I've tested upgrade/rollback between the following 2 simple charts:

chartA MANIFEST:

---
# Source: chartA/templates/service1.yaml
apiVersion: v1
kind: Service
metadata:
  name: my-service1
spec:
  selector:
    app: MyApp1
  ports:
    - port: 71
---
# Source: chartA/templates/service2.yaml
apiVersion: v1
kind: Service
metadata:
  name: my-service2
  annotations:
    helm.sh/resource-policy: "keep"
spec:
  selector:
    app: MyApp2
  ports:
    - port: 81
---
# Source: chartA/templates/service3.yaml
apiVersion: v1
kind: Service
metadata:
  name: my-service3
  annotations:
    helm.sh/resource-policy: "keep"
spec:
  selector:
    app: MyApp3
  ports:
    - port: 91

Note - my-service2 and my-service3 have helm.sh/resource-policy: "keep" annotation

chartB MANIFEST:

---
# Source: chartB/templates/service1.yaml
apiVersion: v1
kind: Service
metadata:
  name: my-service1
spec:
  selector:
    app: MyApp1
  ports:
    - port: 72
---
# Source: chartB/templates/service3.yaml
apiVersion: v1
kind: Service
metadata:
  name: my-service3
spec:
  selector:
    app: MyApp3
  ports:
    - port: 92

Note1 - my-service2 not part of this manifest
Note2 - my-service1/3 have updated config (new ports)


To reproduce / verify the issue:

Step 1 - Install

> helm install app chartA

> kubectl get all
NAME                  TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)   AGE
service/my-service1   ClusterIP   10.99.14.140     <none>        71/TCP    68s
service/my-service2   ClusterIP   10.102.47.214    <none>        81/TCP    68s
service/my-service3   ClusterIP   10.104.118.105   <none>        91/TCP    69s

Step 2 - Upgrade

> helm upgrade app chartB

> kubectl get all
NAME                  TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)   AGE
service/my-service1   ClusterIP   10.99.14.140     <none>        72/TCP    2m54s
service/my-service2   ClusterIP   10.102.47.214    <none>        81/TCP    2m54s
service/my-service3   ClusterIP   10.104.118.105   <none>        92/TCP    2m55s

Note - my-service1/3 are updated (new port config) and my-service2 is kept (even though not part of the manifest)

Step 3A - Rollback without fix

> helm rollback app 
Error: no Service with the name "my-service2" found

Step 3B - Rollback with fix

> helm rollback app

> kubectl get all
NAME                  TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)   AGE
service/my-service1   ClusterIP   10.99.14.140     <none>        71/TCP    4m47s
service/my-service2   ClusterIP   10.102.47.214    <none>        81/TCP    4m47s
service/my-service3   ClusterIP   10.104.118.105   <none>        91/TCP    4m48s

Note - We're back to the original (chartA) config

I've also tested upgrading to "other way around" and it works fine:
Install chartB -> Upgrade chartA -> Rollback

…p annotation

When rollback to a previous release revision that contains a resource
having the annotaion 'helm.sh/resource-policy=keep', while the current
release revision dosen't have this resource, rollback will fail with
an error message:
'Error: no <Kind> with the name "<Resource name>" found'

First of all the error message is misleading, since the resource
actually exists. But the resource is not part of the current manifest
(it is left there in a previous update due to the resource-policy=keep
annotation). So helm cannot create a patch to update the given resource
from the current to the target manifest, and fails with an error.

The logic is updated to in this scenario (resource is part of the target
manifest but NOT part of the current manifest) skip the upgrade IF the
resource-policy=keep annotation is set.

The error message is also updated and the the logic for checking the
resource-policy annotation is broken out into a separate function.

fix helm#8228

Signed-off-by: Ted Petersson <ted.petersson@ericsson.com>
@etedpet
Copy link
Author

etedpet commented Nov 27, 2020

Is there some setup/configuration needed for ci/circleci to run on my pull request?

@mattfarina
Copy link
Collaborator

@etedpet There was an issue with CircleCI. I re-fired the webhook and it appears to be processing now.

fix helm#8228

Signed-off-by: Ted Petersson <ted.petersson@ericsson.com>
@etedpet etedpet force-pushed the fix-rollback-to-release-containing-resource-policy-keep branch from a7b2f47 to 5587ddb Compare December 15, 2020 10:47
@DanTulovsky
Copy link

Is this planned to be merged by chance?

@drew-eero
Copy link

@mattfarina This is a pretty old PR at this point, but I'm still running into this issue. Any update on if it can be merged in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
5 participants