Harmonize finalizer addition across all controllers

[timebertt]

How to categorize this issue?

/area robustness
/kind enhancement

What would you like to be added:

From #4696 (comment):

In some controllers (e.g. shoot controller) we return nil after adding the finalizer and wait for an update watch event to prevent duplicated reconciliations (see #2364).
However, a majority of the other controllers behaves differently and presumably does duplicated/unwanted reconciliations.

We should harmonize the behavior in all our controllers to

• prevent duplicated reconciliations
• cause less confusion along developers and have a good template for new controllers

Ideas would be

• a) adopt the logic from the shoot controller everywhere (return nil after adding finalizer)
• b) changing the update event handlers to enqueue objects only when the generation changed (instead of comparing metadata.generation and status.observedGeneration), this is also what controller-runtime supports out of the box with predicate.GenerationChangedPredicate (see Optimize finalizer addition #4696 (comment))

[BeckerMax]

/priority 4

[shafeeqes]

/assign

[rfranzke]

@shafeeqes Can you please describe what is missing here after #5683?

[shafeeqes]

b) changing the update event handlers to enqueue objects only when the generation changed (instead of comparing metadata.generation and status.observedGeneration), this is also what controller-runtime supports out of the box with predicate.GenerationChangedPredicate

since all the controllers are not yet refactored.

[rfranzke]

Thanks @shafeeqes. As discussed earlier in our meeting, let's
/close
this issue for now since the main motivation (preventing duplicate reconciliations) was tackled. We might optimize the other suggestion as part of #4251, however there is no immediate necessity to pursue this task outside of the planned controller-runtime refactoring activities.

[gardener-prow]

@rfranzke: Closing this issue.

In response to this:

Thanks @shafeeqes. As discussed earlier in our meeting, let's
/close
this issue for now since the main motivation (preventing duplicate reconciliations) was tackled. We might optimize the other suggestion as part of #4251, however there is no immediate necessity to pursue this task outside of the planned controller-runtime refactoring activities.

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.

[oliver-goetz]

@timebertt @rfranzke after implementing b) finalizers are not enforced anymore. When finalizers are removed by someone/something else the reconciler does not add them unless a change which affects the generation happens or the controller is restarting.
Is this an accepted side effect or a bug?

[timebertt]

Is there a difference in this regard between a) and b)?
I think forcefully removing the finalizers is always a dangerous operation and is generally not prevented (e.g., LB services, PVCs, PVs).
It should not be protected because sometimes that's the only way to get rid of resources, however you should know what you're doing 😄

[oliver-goetz]

Other controllers do the same, that's true 👍
I just checked for b) because it is on my agenda now. However, there might be not much to do anyway. I have the impression most of our controllers are already adapted this way.

[rfranzke]

This sounds good @oliver-goetz, please confirm :) If there is nothing to be done, even better!

[oliver-goetz]

It is harmonized at most refactored controllers, but the more I check, the more places I find, where it's not done yet 😄