Consider executing "reconcileDelete" only if finalizer is set

[sbueringer]

Today our controllers are implemented like this:

• if object has no deletionTimestamp
  • => Ensure finalizer is set
• else
  • => execute "reconcileDelete" (remove finalizer at the end of reconcileDelete)

In later reconciles we continue to run "reconcileDelete" even if the finalizer is not set anymore.

I wonder if this should be changed to only execute reconcileDelete as long as the finalizer is on the object.

Pro: always running reconcileDelete:

• reconcileDelete code will always make sure everything is deleted even if the finalizer is not set (but this also means that the object could be removed from etcd in the middle of reconcileDelete)

Pro: running reconcileDelete only when finalizer is set:

• reconcileDelete will only be executed until the finalizer has been removed
  • Example: Machine deletion will stop executing reconcileDelete as soon as it completed successfully once (so we stop continuously trying to drain Pods, waiting for volume detach , ...)

[k8s-ci-robot]

This issue is currently awaiting triage.

If CAPI contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

[sbueringer]

@fabriziopandini @vincepri @enxebre @chrischdi Do you have any opinions on this?

[vincepri]

What's the underlying problem we're trying to solve? The assumption that the reconcileDelete always gets executed is generally fine, although I can see the point of only running actions when the finalizer is present. That said, in terms of re-entrance and ongoing correctness leaning towards more extra reconciliation is probably preferred to rely on the finalizer being present

[fabriziopandini]

I agree with Vince.
For the machine use case, we should probably stop doing actions like drain when the infra machine is deleted (before the removing the finalizer).

[sbueringer]

What's the underlying problem we're trying to solve?

Mostly to avoid unnecessary redundant work.

The finalizer exists because there is work to do for a specific controller, once that is done and the finalizer is removed there should be nothing else to do (otherwise we shouldn't have removed the finalizer).

No strong opinion though if we prefer to continue running reconcileDelete until the object is gone from etcd.

Just seems easier to change the pattern everywhere to stop running reconcileDelete once there's nothing more to do over tactical fixes across controllers to stop doing certain things that won't work anymore once reconcileDelete succeeded.

[sbueringer]

@fabriziopandini

For the machine use case, we should probably stop doing actions like drain when the infra machine is deleted (before the removing the finalizer).

Not sure I understand, why does drain stop working?
(I might be wrong with the assumption that drain stopped working, but I inferred this from this comment + #11457 (comment), but I"m wondering what the problem is that we've hit that lead to 11457)

If there are other Machines the wl cluster kube-apiserver is still reachable. During Cluster deletion drain is not executed at all.

[enxebre]

Just seems easier to change the pattern everywhere to stop running reconcileDelete once there's nothing more to do over tactical fixes across controllers to stop doing certain things that won't work anymore once reconcileDelete succeeded.

I have no strong opinions either. I agree that not running after dropping the finalizer should be fine unless we have a bigger issue. Do we have a list of these tactical fixes / existing issues caused by rerunning?

[sbueringer]

I think I don't have any examples for cases where reconcileDelete was entirely completed and then additional reconcileDelete's failed.

I guess all the cases we had up until now are around re-entrancy when reconcileDelete is not entirely done yet (e.g. Machine reconcileDelete deleted the InfraMachine => then drain doesn't make sense anymore).

But I think there are reasons why we don't encounter these:

• Who would notice errors in CAPI controller logs after we removed our finalizer? We don't block the deletion anymore so also nothing breaks
• I think we don't have a lot of experience with someone else adding additional finalizers to our objects

In general, I'm fine with simply closing this issue.

It just came to mind that technically we shouldn't need any more reconcileDelete's after we're done and we have 0 test coverage that they actually continue to work (but also probably it doesn't matter if they continue to work or just log errors).