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.

Sign up for GitHub

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

Jump to bottom

pkg/helm/controller/reconcile.go: finalizer fix #3039

Merged
merged 1 commit into from May 14, 2020
Merged

pkg/helm/controller/reconcile.go: finalizer fix #3039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions changelog/fragments/helm-fix-finalizer.yaml
View file
Open in desktop
@@ -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
4 changes: 1 addition & 3 deletions pkg/helm/controller/controller.go
View file
Open in desktop
Expand Up @@ -35,7 +35,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")
Expand Down Expand Up @@ -79,8 +78,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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GenerationChangedPredicate needed to be removed. Without it, the helm operator can now detect that the finalizer has been changed/deleted and reconcile so that it is re-added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this predicate preventing detection of changes to the finalizer? Wouldn't a finalizer update change a CR's generation and pass through the predicate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure generation is only changed for spec updates (and maybe status updates). It is not changed when metadata changes. I'm like 95% certain on this, so I'll double check to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep just confirmed that on master I can manually remove the finalizer and it doesn't show back up until I make another edit that changes the spec.

if err := c.Watch(&source.Kind{Type: o}, &crthandler.EnqueueRequestForObject{}); err != nil {
return err
}

Expand Down
88 changes: 46 additions & 42 deletions pkg/helm/controller/reconcile.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Comment on lines -97 to -107
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is moved so that we only add the finalizer after a successful install and then we ensure it is present when reconciling.


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)
Comment on lines -109 to -125
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is moved to after the deletion check and uninstall code. This is necessary to ensure that a CR can always be deleted. There were cases where the CR had a deletion timestamp, but this call to manager.Sync was failing so we always errored out before getting to the manager.Uninstall call.


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
}
Expand Down Expand Up @@ -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
Expand All @@ -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",
Expand All @@ -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")
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
})
Comment on lines +346 to +348
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're no longer using the GenerationChangedPredicate, we get more back-to-back reconciliations, so we need to retry on conflict since the cache may not have updated in time to return the latest version of the CR.

}

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 {
Expand Down