Fix a race condition on reconciler finalizer removal

[nan-yu]

Previously, the reconciler is responsible for removing the reconciler finalizer from the RSync object, but if the reconciler fails to update RSync to remove the finalizer (e.g. the reconciler is deleted or the reconciler doesn't have permissions to update RSync), RSync would stuck in deletion forever.

This commit updates the reconciler-manager to remove the reconciler finalizer to unblock RSync deletion if RSync's Namespace is terminating. A side-effect of this update is that managed resouces couldn't be deleted even though the deletion propagation is set to Foreground, but since the reconciler couldn't function, managed resources couldn't be deleted anyway.

b/332940374

[karlkfi]

/hold

I need to think about this carefully and I don't really have time today.

I think there's some problems with deletion ordering that might crop up in this situation. My general suggestion would be for users not to delete namespaces with RootSyncs in them before carefully deleting the RootSync, but I understand that might be counter intuitive or surprising since everyone takes namespace garbage collection for granted.

A potentially better solution might be to add webhook that blocks namespace deletion when a RepoSync is present. Or perhaps a finalizer on the Namespace that executes before the namespace garbage collector.

[karlkfi]

After digging into this a bit more my concern is this:

Removing the finalizer on the RepoSync when its parent Namespace is being deleted DOES solve the case where deletion ordering is unimportant.

The problem with this solution is that it prevents the reconciler from even attempting to solve the harder problem where deletion ordering IS important.

Additionally, in both cases, the ResourceGroup inventory is left behind.

My counter-proposal would be to only remove the finalizer if there aren't any explicit dependencies defines in the resources being managed by the RepoSync reconciler.

To do that, the reconciler itself would need to stay in charge of removing the RSync finalizer, because only the reconciler has access to the state of the objects without having to query them all. Plus, having only one code path in charge of a specific update is a good idea anyway to avoid race conditions and simplify debugging. Since the reconciler already deletes its RSync's finalizer, we could just skip the Destroy call all together and let the namespace garbage collector handle it.

This proposal has one notable downside: The reconciler deployment needs a RoleBinding to have permission to remove the RepoSync finalizer, and that RoleBinding is also deleted by the namespace garbage collector. To avoid this I have two ideas:

1. Add a webhook (or a new endpoint on the admission-controller) to block deletion of the RoleBinding until after the reconciler Deployment is NotFound.
2. Add a finalizer to the RoleBinding until after the reconciler Deployment is NotFound.

Both of these options have pros and cons, but the webhook could be more generally useful. For example:

• A webhook could block deletion of Namespaces if they contain any objects with incoming dependencies, allowing the RepoSync reconciler to delete package contents in the right order first.
• A webhook could block deletion of any object with an incoming dependency, enforcing deletion ordering more widely.

In the case of namespace garbage collection, temporary deletion errors are ok, because it will just keep retrying, giving the RepoSync reconciler a chance to do it's work.

In the case where the RepoSync reconciler needs an additional user-managed RoleBinding to manage other resource types, the user could add a dependency on the RepoSync to make it depend on their RoleBinding. This way, the user's RoleBinding would also be protected the same way the default RoleBinding is protected by the webhook.

[nan-yu]

Removing the finalizer on the RepoSync when its parent Namespace is being deleted DOES solve the case where deletion ordering is unimportant.

That is the intent of this fix.

The problem with this solution is that it prevents the reconciler from even attempting to solve the harder problem where deletion ordering IS important.

It might be helpful to clarify that depends-on and deletion order shouldn't be treated as identical concepts. If deletion ordering IS important, users should leverage custom finalizers on their resources. An example is CR depends on CRD, but the deletion of CRD should not be blocked by CR.

Additionally, in both cases, the ResourceGroup inventory is left behind.

This is not correct. ResourceGroup is in the same namespace as the RepoSync. It would be deleted as well.

[nan-yu]

In the case of namespace garbage collection, temporary deletion errors are ok, because it will just keep retrying, giving the RepoSync reconciler a chance to do it's work.

In this case, no deletion ordering is guaranteed.

In the case where the RepoSync reconciler needs an additional user-managed RoleBinding to manage other resource types, the user could add a dependency on the RepoSync to make it depend on their RoleBinding. This way, the user's RoleBinding would also be protected the same way the default RoleBinding is protected by the webhook.

It is also not guaranteed that RoleBinding and RepoSync are managed in the same repo, and I don't think Config Sync supports external dependency.

[karlkfi]

ResourceGroup is in the same namespace as the RepoSync. It would be deleted as well.

I stand corrected. You're right, only the RootSync ResourceGroups are in the config-management-system.

In that case, the RepoSync & ResourceGroup being deleted at approximately the same time by the namespace garbage collector is probably good enough.

However, the other related problem is that the UI & CLI will show the RepoSync/package as deleted, but the package contents may not have all been fully deleted if they're blocked by a finalizer. So the user won't be able to use the Console Package UI or nomos status to watch the status of those objects if they have finalizers.

It might be helpful to clarify that depends-on and deletion order shouldn't be treated as identical concepts.

Yes. The primary issue here is with explicit dependencies, with the depends-on annotation, not implicit dependencies, like namespaces and CRDs.

In this case, no deletion ordering is guaranteed.

The namespace garbage collector doesn't ensure dependency deletion ordering, but we do with RSync uninstall (with the deletion-propagation annotation). In this case the two policies conflict and we lose. But it doesn't have to be that way. Just because it is this way doesn't mean it should be or that users want it this way. We can make it better!

It is also not guaranteed that RoleBinding and RepoSync are managed in the same repo, and I don't think Config Sync supports external dependency.

I'm not implying they would be or need to be. My proposal is that the webhook detect depends-on annotations and protect the dependency from deletion, even if the object wasn't synced by Config Sync. That way the reconciler-manager can add one on the reconciler Deployment -> default RoleBinding and the user can add one for RepoSync -> user RoleBinding.

In the future, we might want to figure out how to allow multiple clients to manage separate depends-on annotations on the same object to define different dependencies, but for this use case we don't need that yet.

[mikebz]

Hi everyone! Sorry for chiming in on this when I have not been involved in much of the technical reasoning. We have an upcoming code freeze and therefore need to evaluate some of these decision in the light that we have deadlines pending.

Question:

1. Do we have ready alternatives to this?
2. The UX of namespace being deleted and removing all resources out of order could lead to issues, is this a risk we can take on right now and think about it later?
3. Is this change irreversible? Meaning will customers take dependency on this behavior and then we are locked into this choice?

If the answers are:

1. We don't have alternatives at hand.
2. The risk is small (meaning someone who was relying on Deployment A to be deleted before Deployment B, and are upset that this is happening when namespace is deleted).
3. We can improve the behavior later.

I suggest merging this in and using the few days we have before the code freeze for additional testing. It doesn't seem like we want to ship the bug that we currently have "RSync would stuck in deletion forever."

[karlkfi]

I don't really understand why y'all are in a hurry to get this half-fix in with side effects, for an issue that's existed for years, right before a code freeze, but I agree that the side effect is less bad than the current behavior.

I don't think this will be un-reversable. It's just more tech debt that will have to be reverted if we decide to later apply the more complete solution.

Anyway, I'll make another review pass today.

[janetkuo]

A few comments:

1. Deletion ordering is orthogonal to RSync garbage collection, because deletion can be performed by other controllers/users in the cluster, and it could happen before the RSync is being deleted. Reconciler can make sure it deletes things in order, but it can't stop others from deleting things out of order.
2. Apply ordering (depends-on) doesn't necessary imply deletion ordering. An example is that you want to apply CRD before applying CR, but we don't necessary want to block CRD from being deleted just because CR hasn't been deleted.
3. When a Namespace is being terminated, Namespace controller already handles resource deletion of everything managed by the RepoSync. It's safe and easier for reconcilers to let Namespace controller handle garbage collection.

This is the right fix.

[karlkfi]

/lgtm

I suspect we'll eventually need to revert this later when we add a more general solution that works for deletion dependencies and RootSyncs, but for now it seems like an improvement for RepoSyncs at least.

I left some minor comments, but overall it LGTM.

I would also suggest including the NamespaceTerminating part of https://github.com/GoogleContainerTools/kpt-config-sync/pull/1194 in this PR, so that they can be pulled out together, if necessary, since they cause the same side effect. But it's not necessarily a blocker.

Feel free to remove the hold when the comments are addressed.

[nan-yu]

/unhold

#1194 is related, but a different issue. It shouldn't block this.

[karlkfi]

/lgtm

[google-oss-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, karlkfi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [janetkuo,karlkfi]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment