pkg/helm/controller/reconcile.go: finalizer fix #3039
Conversation
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.
| @@ -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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 | ||
| } |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
| return retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| return r.Client.Update(context.TODO(), o) | ||
| }) |
There was a problem hiding this comment.
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.
|
/cherry-pick v0.17.x |
|
@joelanford: new pull request created: #3046 In response to this:
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. |
Description of the change:
Helm operator now applies its uninstall finalizer only when a release is deployed.
Motivation for the change:
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.