Helm: fix misleading "release not found" errors during CR deletion (#2359)

* pkg/helm: handle helm v3 ErrReleaseNotFound correctly

* pkg/helm: wait for CR deletion after uninstall and finalizer removal

* CHANGELOG.md: update for #2359

Author: joelanford

CHANGELOG.md

@@ -25,7 +25,9 @@
 ### Removed
 
 ### Bug Fixes
+
 - Fix `operator-sdk build`'s `--image-build-args` to support spaces within quotes like `--label some.name="First Last"`. ([#2312](https://github.com/operator-framework/operator-sdk/pull/2312))
+- Fix misleading Helm operator "release not found" errors during CR deletion. ([#2359](https://github.com/operator-framework/operator-sdk/pull/2359))
 
 ## v0.13.0
 

pkg/helm/controller/reconcile.go

@@ -16,14 +16,17 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
 	rpb "helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/storage/driver"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -126,7 +129,7 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 		}
 
 		uninstalledRelease, err := manager.UninstallRelease(context.TODO())
-		if err != nil && err != release.ErrNotFound {
+		if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
 			log.Error(err, "Failed to uninstall release")
 			status.SetCondition(types.HelmAppCondition{
 				Type:    types.ConditionReleaseFailed,
@@ -139,7 +142,7 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 		}
 		status.RemoveCondition(types.ConditionReleaseFailed)
 
-		if err == release.ErrNotFound {
+		if errors.Is(err, driver.ErrReleaseNotFound) {
 			log.Info("Release not found, removing finalizer")
 		} else {
 			log.Info("Uninstalled release")
@@ -154,6 +157,7 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 			status.DeployedRelease = nil
 		}
 		if err := r.updateResourceStatus(o, status); err != nil {
+			log.Info("Failed to update CR status")
 			return reconcile.Result{}, err
 		}
 
@@ -164,10 +168,21 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile.
 			}
 		}
 		o.SetFinalizers(finalizers)
-		err = r.updateResource(o)
+		if err := r.updateResource(o); err != nil {
+			log.Info("Failed to remove CR uninstall finalizer")
+			return reconcile.Result{}, err
+		}
 
-		// Need to requeue because finalizer update does not change metadata.generation
-		return reconcile.Result{Requeue: true}, err
+		// Since the client is hitting a cache, waiting for the
+		// deletion here will guarantee that the next reconciliation
+		// will see that the CR has been deleted and that there's
+		// nothing left to do.
+		if err := r.waitForDeletion(o); err != nil {
+			log.Info("Failed waiting for CR deletion")
+			return reconcile.Result{}, err
+		}
+
+		return reconcile.Result{}, nil
 	}
 
 	if !manager.IsInstalled() {
@@ -313,6 +328,26 @@ func (r HelmOperatorReconciler) updateResourceStatus(o *unstructured.Unstructure
 	return r.Client.Status().Update(context.TODO(), o)
 }
 
+func (r HelmOperatorReconciler) waitForDeletion(o runtime.Object) error {
+	key, err := client.ObjectKeyFromObject(o)
+	if err != nil {
+		return err
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+	defer cancel()
+	return wait.PollImmediateUntil(time.Millisecond*10, func() (bool, error) {
+		err := r.Client.Get(ctx, key, o)
+		if apierrors.IsNotFound(err) {
+			return true, nil
+		}
+		if err != nil {
+			return false, err
+		}
+		return false, nil
+	}, ctx.Done())
+}
+
 func contains(l []string, s string) bool {
 	for _, elem := range l {
 		if elem == s {

pkg/helm/release/manager.go

@@ -27,6 +27,7 @@ import (
 	"helm.sh/helm/v3/pkg/kube"
 	rpb "helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/storage"
+	"helm.sh/helm/v3/pkg/storage/driver"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -38,11 +39,6 @@ import (
 	"github.com/operator-framework/operator-sdk/pkg/helm/internal/types"
 )
 
-var (
-	// ErrNotFound indicates the release was not found.
-	ErrNotFound = errors.New("release not found")
-)
-
 // Manager manages a Helm release. It can install, update, reconcile,
 // and uninstall a release.
 type Manager interface {
@@ -109,7 +105,7 @@ func (m *manager) Sync(ctx context.Context) error {
 
 	// Load the most recently deployed release from the storage backend.
 	deployedRelease, err := m.getDeployedRelease()
-	if err == ErrNotFound {
+	if errors.Is(err, driver.ErrReleaseNotFound) {
 		return nil
 	}
 	if err != nil {
@@ -138,7 +134,7 @@ func (m manager) getDeployedRelease() (*rpb.Release, error) {
 	deployedRelease, err := m.storageBackend.Deployed(m.releaseName)
 	if err != nil {
 		if strings.Contains(err.Error(), "has no deployed releases") {
-			return nil, ErrNotFound
+			return nil, driver.ErrReleaseNotFound
 		}
 		return nil, err
 	}
@@ -299,13 +295,13 @@ func (m manager) UninstallRelease(ctx context.Context) (*rpb.Release, error) {
 	// Get history of this release
 	h, err := m.storageBackend.History(m.releaseName)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get release history: %s", err)
+		return nil, fmt.Errorf("failed to get release history: %w", err)
 	}
 
 	// If there is no history, the release has already been uninstalled,
-	// so return ErrNotFound.
+	// so return ErrReleaseNotFound.
 	if len(h) == 0 {
-		return nil, ErrNotFound
+		return nil, driver.ErrReleaseNotFound
 	}
 
 	uninstall := action.NewUninstall(m.actionConfig)