From cea728cec4503c098b537e5e219c0ca1c14d49ac Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Sat, 25 Apr 2020 00:07:46 -0400 Subject: [PATCH] pkg/helm/controller/reconcile.go: finalizer fix This PR fixes a bug in the helm operator that caused the CR to be unable to be deleted without manually intervening to delete a prematurely added finalizer. Helm operator now applies its uninstall finalizer only when a release is deployed. --- changelog/fragments/helm-fix-finalizer.yaml | 7 ++ pkg/helm/controller/controller.go | 4 +- pkg/helm/controller/reconcile.go | 88 +++++++++++---------- 3 files changed, 54 insertions(+), 45 deletions(-) create mode 100644 changelog/fragments/helm-fix-finalizer.yaml diff --git a/changelog/fragments/helm-fix-finalizer.yaml b/changelog/fragments/helm-fix-finalizer.yaml new file mode 100644 index 00000000000..83d9f0b0fdf --- /dev/null +++ b/changelog/fragments/helm-fix-finalizer.yaml @@ -0,0 +1,7 @@ +entries: + - description: > + Helm operator now applies its uninstall finalizer only when + a release is deployed. This fixes a bug that caused the + CR to be unable to be deleted without manually intervening + to delete a prematurely added finalizer. + kind: bugfix diff --git a/pkg/helm/controller/controller.go b/pkg/helm/controller/controller.go index 7587e05cea4..0cbc8bd820f 100644 --- a/pkg/helm/controller/controller.go +++ b/pkg/helm/controller/controller.go @@ -36,7 +36,6 @@ import ( "github.com/operator-framework/operator-sdk/pkg/helm/release" "github.com/operator-framework/operator-sdk/pkg/internal/predicates" - "github.com/operator-framework/operator-sdk/pkg/predicate" ) var log = logf.Log.WithName("helm.controller") @@ -80,8 +79,7 @@ func Add(mgr manager.Manager, options WatchOptions) error { o := &unstructured.Unstructured{} o.SetGroupVersionKind(options.GVK) - if err := c.Watch(&source.Kind{Type: o}, &crthandler.EnqueueRequestForObject{}, - predicate.GenerationChangedPredicate{}); err != nil { + if err := c.Watch(&source.Kind{Type: o}, &crthandler.EnqueueRequestForObject{}); err != nil { return err } diff --git a/pkg/helm/controller/reconcile.go b/pkg/helm/controller/reconcile.go index 4c091104e0e..b6f1d4f369a 100644 --- a/pkg/helm/controller/reconcile.go +++ b/pkg/helm/controller/reconcile.go @@ -29,7 +29,9 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/operator-framework/operator-sdk/internal/util/diffutil" @@ -94,38 +96,8 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. status := types.StatusFor(o) log = log.WithValues("release", manager.ReleaseName()) - deleted := o.GetDeletionTimestamp() != nil - pendingFinalizers := o.GetFinalizers() - if !deleted && !contains(pendingFinalizers, finalizer) { - log.V(1).Info("Adding finalizer", "finalizer", finalizer) - finalizers := append(pendingFinalizers, finalizer) - o.SetFinalizers(finalizers) - err = r.updateResource(o) - - // Need to requeue because finalizer update does not change metadata.generation - return reconcile.Result{Requeue: true}, err - } - - status.SetCondition(types.HelmAppCondition{ - Type: types.ConditionInitialized, - Status: types.StatusTrue, - }) - - if err := manager.Sync(context.TODO()); err != nil { - log.Error(err, "Failed to sync release") - status.SetCondition(types.HelmAppCondition{ - Type: types.ConditionIrreconcilable, - Status: types.StatusTrue, - Reason: types.ReasonReconcileError, - Message: err.Error(), - }) - _ = r.updateResourceStatus(o, status) - return reconcile.Result{}, err - } - status.RemoveCondition(types.ConditionIrreconcilable) - - if deleted { - if !contains(pendingFinalizers, finalizer) { + if o.GetDeletionTimestamp() != nil { + if !contains(o.GetFinalizers(), finalizer) { log.Info("Resource is terminated, skipping reconciliation") return reconcile.Result{}, nil } @@ -163,13 +135,7 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. return reconcile.Result{}, err } - finalizers := []string{} - for _, pendingFinalizer := range pendingFinalizers { - if pendingFinalizer != finalizer { - finalizers = append(finalizers, pendingFinalizer) - } - } - o.SetFinalizers(finalizers) + controllerutil.RemoveFinalizer(o, finalizer) if err := r.updateResource(o); err != nil { log.Info("Failed to remove CR uninstall finalizer") return reconcile.Result{}, err @@ -187,6 +153,24 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. return reconcile.Result{}, nil } + status.SetCondition(types.HelmAppCondition{ + Type: types.ConditionInitialized, + Status: types.StatusTrue, + }) + + if err := manager.Sync(context.TODO()); err != nil { + log.Error(err, "Failed to sync release") + status.SetCondition(types.HelmAppCondition{ + Type: types.ConditionIrreconcilable, + Status: types.StatusTrue, + Reason: types.ReasonReconcileError, + Message: err.Error(), + }) + _ = r.updateResourceStatus(o, status) + return reconcile.Result{}, err + } + status.RemoveCondition(types.ConditionIrreconcilable) + if !manager.IsInstalled() { for k, v := range r.OverrideValues { r.EventRecorder.Eventf(o, "Warning", "OverrideValuesInUse", @@ -206,6 +190,13 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. } status.RemoveCondition(types.ConditionReleaseFailed) + log.V(1).Info("Adding finalizer", "finalizer", finalizer) + controllerutil.AddFinalizer(o, finalizer) + if err := r.updateResource(o); err != nil { + log.Info("Failed to add CR uninstall finalizer") + return reconcile.Result{}, err + } + if r.releaseHook != nil { if err := r.releaseHook(installedRelease); err != nil { log.Error(err, "Failed to run release hook") @@ -236,6 +227,15 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. return reconcile.Result{RequeueAfter: r.ReconcilePeriod}, err } + if !contains(o.GetFinalizers(), finalizer) { + log.V(1).Info("Adding finalizer", "finalizer", finalizer) + controllerutil.AddFinalizer(o, finalizer) + if err := r.updateResource(o); err != nil { + log.Info("Failed to add CR uninstall finalizer") + return reconcile.Result{}, err + } + } + if manager.IsUpdateRequired() { for k, v := range r.OverrideValues { r.EventRecorder.Eventf(o, "Warning", "OverrideValuesInUse", @@ -343,12 +343,16 @@ func hasHelmUpgradeForceAnnotation(o *unstructured.Unstructured) bool { } func (r HelmOperatorReconciler) updateResource(o runtime.Object) error { - return r.Client.Update(context.TODO(), o) + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + return r.Client.Update(context.TODO(), o) + }) } func (r HelmOperatorReconciler) updateResourceStatus(o *unstructured.Unstructured, status *types.HelmAppStatus) error { - o.Object["status"] = status - return r.Client.Status().Update(context.TODO(), o) + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + o.Object["status"] = status + return r.Client.Status().Update(context.TODO(), o) + }) } func (r HelmOperatorReconciler) waitForDeletion(o runtime.Object) error {