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.