Apply updates from operator-sdk helm-operator

[joelanford]

• Uninstall bug: Bug fix: helm operator uninstall is not properly checking for existing release operator-sdk#3431
  • Not applicable. The uninstall code in this repo already directly calls uninstall.Run without any pre-checks for the existence of the release.
• Build info metric: internal/cmd: add build info metric in helm/ansible operators operator-sdk#4220
  • Done in add build info metric #67
• Plugin docker invocation: *: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' operator-sdk#4226
  • Done in Added default Dockerfile path to end of command #68
• Don't inject owner references for helm keep resources: fix: (helm) - do not add owner references to resources that contain the Helm keep resource-policy annotation operator-sdk#4389
  • Done in fix: (helm) - do not add owner references to resources that contain the Helm keep resource-policy annotation #71
• fix: (helm) - prevent sync from deleting all release records if Helm failed to transition state properly during upgrade operator-sdk#4358 when all non-deployed release records are purged, it appears that the helm app was never installed. To recover, the helm operator performed an install but failed so it did an uninstall as rollback which lead to data loss. Related to change: (helm) - if the release is not installed but status.DeployedRelease is populated, skip the rollback(uninstall) if the install errors operator-sdk#4297
  • Done in pkg/reconciler: force upgrade if release is failed or superseded #81
• Prevent nil pointer during uninstall: fix: (helm) - prevent a possible nil pointer in the helm uninstall operator-sdk#4288
  • Not applicable. See https://github.com/joelanford/helm-operator/issues/63#issuecomment-767068736
• change: (helm) - keep attempting to helm uninstall even if the first helm uninstall was not successful operator-sdk#4347 in a more busy env like production, the helm operator delete doesn't cleanup all the resources. This PR is to hopefully improve on that.
• change: (helm) - if the release is not installed but status.DeployedRelease is populated, skip the rollback(uninstall) if the install errors operator-sdk#4297 - skip rollback/uninstall if status.DeployedRelease is set, but install/upgrade failed
• Improve help text: ansible-operator,helm-operator: improve executable help text operator-sdk#4187
• Possible: improve handling of install errors: change: (helm) - if the release is not installed but status.DeployedRelease is populated, skip the rollback(uninstall) if the install errors operator-sdk#4297 (still unmerged in SDK repo as of 1/12/2021)

[anmol372]

PR for Build info metric: 67
PR to add correct help text: kb-1830
PR for Plugin docker invocation: 68

[mikeshng]

FYI There is also this operator-framework/operator-sdk#4288 that was merged

[mikeshng]

If possible, could you please also consider operator-framework/operator-sdk#4297
Due to the unintended uninstall rollback, some of our customers were hit with unintended data loss.

[joelanford]

Thanks @mikeshng if there are any other Helm-related PRs that you're aware of, please post them.

We're fairly confident that this repo has everything up to SDK v1.0.0.

Our goal is to eventually replace the SDK helm-operator implementation with this one, and we want to make sure all the bug fixes and added features from the SDK repo make it here.

[mikeshng]

Thanks @joelanford for all your help. Two PRs that I am less confident about and I am looking for your team's expert opinions on:

• fix: (helm) - prevent sync from deleting all release records if Helm failed to transition state properly during upgrade operator-sdk#4358 I think this is what some of my customers ran into: all the release records were purged making it seems like the helm app was never installed. To recover, the helm operator performed an install but failed so it did an uninstall as rollback which lead to data loss. Related to change: (helm) - if the release is not installed but status.DeployedRelease is populated, skip the rollback(uninstall) if the install errors operator-sdk#4297
• change: (helm) - keep attempting to helm uninstall even if the first helm uninstall was not successful operator-sdk#4347 in a more busy env like production, the helm operator delete doesn't cleanup all the resources. This PR is to hopefully improve on that.

[joelanford]

For operator-framework/operator-sdk#4288, is this just a defensive check, or is there actually a code path that results in a nil error and a nil UninstallReleaseResponse or nil Release? I'm not seeing a way for the existing Helm libraries to return both a nil Release and a nil error.

[joelanford]

I don't think operator-framework/operator-sdk#4358 is applicable here. In this repo, there is nothing that deletes releases from the storage backend outside of running through an uninstall. manager.Sync has been mostly replaced by getReleaseState.

When a non-deployed release exists, it looks like this repo will attempt an upgrade rather than pre-deleting it and then attempting a clean install. If the last release is one of Deployed, Failed, or Superseded, then the upgrade will proceed. That seems reasonable in the context of the Helm operator too I think. I'll submit a PR here to make sure we attempt the upgrade in this scenario.

[mikeshng]

For operator-framework/operator-sdk#4288, is this just a defensive check, or is there actually a code path that results in a nil error and a nil UninstallReleaseResponse or nil Release? I'm not seeing a way for the existing Helm libraries to return both a nil Release and a nil error.

As I remember, I ran into a nil pointer in the old code. It was only doing:

uninstallResponse, err := uninstall.Run(m.releaseName)
return uninstallResponse.Release, err

So the primary goal of the fix was to avoid this nilpointer when uninstallResponse is nil.

As for the change if log.V(0).Enabled() && uninstalledRelease != nil { Yes, I think that's a defensive check that Eric suggested.

[joelanford]

Re: operator-framework/operator-sdk#4288 - It looks like this is also not applicable in this repo since the client returns the uninstall response and then the caller correctly checks for errors before assuming the embedded release field is valid.

[mikeshng]

@joelanford if you have some cycles, could you please see my idea here and post a feedback message: https://lists.cncf.io/g/cncf-helm/message/351
I am trying to enhance the helm uninstall so the helm operator can leverage the new functionality and perform a more reliable resource cleanup upon CR delete. Thanks.

[jmrodri]

@varshaprasad96 please review to see where we are with this, close when done.

[varshaprasad96]

Went through the updates with respect to helm-operator in operator-sdk after SDK 1.0 release till v1.15.0. These are the following updates:

• Add predicates for filtering : Closes #4949: Label-based filtering for Helm via predicates operator-sdk#4997 - Taken care by Remove legacy Helm code base #115
• Add option to watch list types : Add option to watch List object types #138
• Helm operator reconciliation diffs are now logged only at the zap debug level : https://github.com/operator-framework/operator-sdk/pull/5307/files - Discussed in Unable to shows the diff between previous manifest and the updated one (action.upgrade) #44
• support go text/template evaluation in override values : helm-operator: support go text/template evaluation in override values operator-sdk#5105
• fix sending empty patch requests to api server: https://github.com/operator-framework/operator-sdk/pull/4957/files (fixed by fix(helm-operator): do not send empty patch requests to kube apiserver #139)
• add annotation to wait for to wait for all resources to be deleted before removing CR finalizer : https://github.com/operator-framework/operator-sdk/pull/4487/files

PS: These are updates made to the library, the plugin was updated recently and is already upto date.