Ensure finalizer is present before reconciling #218
Conversation
|
ping: @joelanford |
ludydoo
force-pushed
the
ensure-finalizer-before-reconcile
branch
from
df6d950 to
339a185
Compare
|
@joelanford Do you have any more concerns for this one ? |
There was a problem hiding this comment.
Two nitpicks inline, and I'd suggest to reword the PR description to:
The current logic will apply the finalizer to the custom resource after the first reconciliation loop (in a closure). Though, in certain circumstances, the custom resource might get deleted after the first reconciliation loop has installed the chart but before it has applied the finalizer. If the chart created any resources not in the same namespace these will be orphaned without cleanup.
By ensuring that the finalizer is present on the custom resource before proceeding, this will ensure a proper cleanup in all cases.
pkg/reconciler/reconciler.go
Outdated
| @@ -521,6 +522,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| // The finalizer must be present on the CR before we can do anything. Otherwise, if the reconciliation fails, | |||
| // there might be resources created by the chart that will not be garbage-collected | |||
There was a problem hiding this comment.
| // there might be resources created by the chart that will not be garbage-collected | |
| // there might be resources created by the chart that will not be garbage-collected |
pkg/reconciler/reconciler.go
Outdated
| // (cluster-scoped resources or resources in other namespaces, which are not bound by an owner reference). | ||
| // This is a safety measure to ensure that the chart is fully uninstalled before the CR is deleted. | ||
| if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, uninstallFinalizer) { | ||
| log.V(1).Info("Adding uninstall finalizer") |
There was a problem hiding this comment.
| log.V(1).Info("Adding uninstall finalizer") | |
| log.V(1).Info("Adding uninstall finalizer.") |
|
@joelanford can you please approve and merge? This fixes a resource leak which occurs frequently at high CR cardinalities. |
Pull Request Test Coverage Report for Build 5542078207
💛 - Coveralls |
|
@ludydoo could you please rebase this PR with main |
openshift-merge-robot
added
the
needs-rebase
ludydoo
force-pushed
the
ensure-finalizer-before-reconcile
branch
from
47fde81 to
66042cd
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-merge-robot
removed
the
needs-rebase
|
Reopening this to run the CI again |
The current logic will apply the finalizer to the custom resource after the first reconciliation loop (in a closure). Though, in certain circumstances, the custom resource might get deleted after the first reconciliation loop has installed the chart but before it has applied the finalizer. If the chart created any resources not in the same namespace these will be orphaned without cleanup.
By ensuring that the finalizer is present on the custom resource before proceeding, this will ensure a proper cleanup in all cases.