change: (helm) - keep attempting to helm uninstall even if the first helm uninstall was not successful

[mikeshng]

Signed-off-by: Mike Ng ming@redhat.com

Description of the change:
For Helm-based operators delete resource, if the Helm uninstall completed with error, then keep trying to uninstall until there is no error on the Helm uninstall.

Motivation for the change:
For the Helm client, the helm uninstall command removes the release record even if the uninstall completed with error(s). This is fine behaviour for regular CLI execution because a user is informed that there was an error with the uninstall and the user will perform the proper recovery procedures.

However, this is problematic with the current implementation of the Helm operator. The Helm uninstall might have error and not deleted anything but since the release record is removed, the Helm operator thinks the release is gone and proceed to remove it's finalizer. So unless the user double check the log, the uninstall error is hidden away. See #3407 (comment)

This change is to enhance the behaviour of the Helm operator uninstall scenario, where the error will persist on the resource status for user to debug.

Closes: #3407

before this change:

# resource is gone from k8s, only in the log, we can see there was an error
{"level":"error","ts":1608568023.8352385,"logger":"controller-runtime.manager.controller.nginx-controller","msg":"Reconciler error","name":"nginx-sample","namespace":"default","error":"uninstallation completed with 1 error(s): ....
{"level":"info","ts":1608568023.8640625,"logger":"helm.controller","msg":"Release not found, removing finalizer","namespace":"default","name":"nginx-sample","apiVersion":"example.com/v1alpha1","kind":"Nginx","release":"nginx-sample"}

after this change:

$ oc get nginx/nginx-sample -o yaml 
...
  - lastTransitionTime: "2020-12-21T19:40:48Z"
    message: 'uninstallation completed with 1 error(s): ...
    reason: UninstallError
    status: "True"
    type: ReleaseFailed
...

# continuously attempt uninstall until the problem is fixed.
# when the problem is fixed, the log will show:

{"level":"info","ts":1608580477.2366333,"logger":"helm.controller","msg":"Uninstalled release","namespace":"default","name":"nginx-sample","apiVersion":"example.com/v1alpha1","kind":"Nginx","release":"nginx-sample"}
----
-# Source: nginx/templates/serviceaccount.yaml
-apiVersion: v1
-kind: ServiceAccount
-metadata:

Checklist

If the pull request includes user-facing changes, extra documentation is required:

• Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
• Add or update relevant sections of the docs website in website/content/en/docs

[camilamacedo86]

Shows that it is more bug fix. Am I right?
@joelanford. Could you please give a hand with this one? Do you think that we should remove the finalizer if Helm uninstallation completed with an error? WDYT?

[camilamacedo86]

/retest

[mikeshng]

Rebased but I don't feel too confident about this PR anymore.

• This is a default behavior change.
• It's a hack to muck with the helm release storage status just to trigger the helm uninstall logic again. I think if helm has an option like helm uninstall --fail-on-error see proposal here https://lists.cncf.io/g/cncf-helm/message/351 then the helm operator can call leverage that to better handle uninstall failures.
• Just because the uninstall didn't error, doesn't mean all the resources are actually successfully deleted so it doesn't really solve Deleting a Helm-based operator CR doesn't guarantee all it's associated resources are deleted #3407
• enh: (helm) - add annotation to wait for all resources to be deleted before removing CR finalizer #4487 has been merged so there is already an option for user to wait for all resources are deleted before the CR is gone.

Dear operator-sdk maintainers, please feel free to close this PR. Thanks.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci]

@openshift-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.