Proposal: Support foreground cascading deletion

[bobh66]

What problem are you facing?

When deleting a Crossplane Claim, the "--cascade=foreground" option to kubectl delete has no impact on the delete process.
All Composite Resources get deleted immediately and all Managed Resources are deleted as soon as possible by their associated providers.

This can cause issues with hung resources as documented in many other issues including:

#1612
#1782
#2439
crossplane-contrib/provider-helm#115
crossplane-contrib/provider-kubernetes#38
crossplane-contrib/provider-terraform#47

and in: #3116

How could Crossplane help solve your problem?

This proposal is the first of two that deal with dependency management, and deals only with supporting foreground cascading deletion. More complex dependency handling will be addressed in a second proposal that is dependent on the changes described here.

Kubernetes uses Owner References and Finalizers to represent and implement dependencies between resources. When a resource has one or more Finalizers and is marked "deleted", Garbage Collection will not reap the resource until the last finalizer has been removed, indicating that all dependencies have been cleaned up by their respective Controllers. Cascading foreground deletion uses the ownerReferences and blockOwnerDeletion flag to generate a dependency graph and adds finalizers to ensure that the set of resources is deleted from the "bottom" up.

https://kubernetes.io/docs/concepts/architecture/garbage-collection/#foreground-deletion

Crossplane has two limitations that cause it to not support foreground cascading deletion:

• the blockOwnerDeletion flag is not set to "true" in the controller owner references that Crossplane adds to the resources it creates

• Crossplane ignores all "external" finalizers and deletes it's resources immediately when it sees the deletionTimestamp attribute added to an object's metadata

These issues can be resolved by the following changes:

• Include the blockOwnerDeletion: true attribute in the controller ownerReferences that Crossplane adds to the resources it creates. This will cause kubernetes to add the foregroundDeletion finalizer to the owner resource when the delete command is executed with "--cascade=foreground" and block it's deletion while there are still resources that reference it. Note that most/all Kubernetes controllers (Deployments, Replicasets, Statefulsets, etc) set this flag for their ownerReferences on the resources they control, such as Pods.

• In the Delete handling for the composite and managed Reconcilers, delay calling the composite and managed external Delete() functions until all "external" (non-Crossplane) finalizers are removed. Crossplane needs to maintain the existence of the composite/managed resources under it's control until the last non-Crossplane finalizer has been deleted, at which time the normal Delete processing can be executed. This will ensure that the remote resources are maintained while the dependencies exist, and they only get removed after all dependencies have been removed.

Coordinated changes in the crossplane and crossplane-runtime projects are required.

The combination of these two changes will allow Crossplane to support foreground cascaded deletion, which is a first step toward a more complete dependency handling solution.

The blockOwnerDeletion change only affects delete handling when cascade=foreground is specified by the user (ArgoCD uses this mode by default). The default "background" mode and "orphan" mode are not affected by this change.

Adding support for external finalizers also enables scenarios where other projects like provider-kubernetes can control delete handling with finalizers, as described in https://github.com/crossplane-contrib/provider-kubernetes/blob/main/docs/enhanced-provider-k8s.md

I can push PRs with these changes and test in a PoC if this is acceptable.

UPDATE

There are two PRs directly related to this issue:

• Support foreground cascading deletion crossplane-runtime#355 - this adds the blockOwnerDeletion attribute to the Controller ownerReferences, updates the managed Reconciler() to requeue if there are additional finalizers when delete is detected, and adds the compositeDeletePolicy attribute to the Claim to enable simulating Foreground Cascading Deletion on Claims

• Add support for compositeDeletePolicy #3321 - this updates the claim and composite Reconcilers() to requeue if there are additioanl finalizers when delete is detected, and adds support for the compositeDeletePolicy to the claim reconciler

[bobh66]

In testing these changes I discovered that the Claim is not defined as an Owner of the Composite - there is no ownerReference in the Composite object pointing at the Claim. I assume this is intentional, but it also disables foreground cascading deletion because Kubernetes garbage collection creates the resource graph using ownerReferences. Since there is no ownerReference, the foregroundDeletion finalizer is never added to the Composite or it's "children" when the Claim is deleted with the "--cascade=foreground" option. @negz @muvaf - is there any history to the absence of an ownerReference on the Composite object? Thanks

[bobh66]

I assume that there is no ownerReference so that the Claim reconciler can handle the composite deletion itself here - https://github.com/crossplane/crossplane/blob/master/internal/controller/apiextensions/claim/reconciler.go#L377

Is there a reason to do the composite delete explicitly in the reconciler instead of letting kubernetes handle it via an ownerReference?

[bobh66]

Since the Claim->Composite relationship is being handled explicitly, what about adding another DeletionPolicy value of "Foreground" that only applies to Claims/Composites (it makes no sense to apply it to Managed Resources) and only gets used in the claim reconciler? If the DeletionPolicy on the Claim/Composite is set to "Foreground", then that option gets passed to the r.client.Delete() command referenced above, and the deletion of the Composite resources is handled as if the user had provided the --cascade=foreground option to kubectl delete. Thoughts?

[bobh66]

I thought it might be possible to detect the cascading foreground deletion request when the Claim was deleted - kubernetes seems to apply the foregroundDeletion finalizer to the claim for a brief period of time, but it's not there when the Claim reconciler gets the object, so we can't call the composite Delete() with the Foreground Propagation Policy which would serve the same purpose as having an ownerReference link.

And since deletionPolicy only applies to Managed resources and not to Composites, that would require a different change to support Foreground deletion.

[bobh66]

It looks like there are a couple of options for allowing foregroundDeletion to work on the Claim:

1 - change the Claim/Composite code to put an ownerReference on the Composite that points at the Claim, and remove the explicit Delete of the Composite from the Claim reconciler. This essentially lets k8s handle the deletion of the composite.

2 - Add an attribute to the Claim/Composite which can be set to cause the Claim reconciler to pass the PropagationPolicy=Foreground parameter to the Delete call on the composite resource

3 - Add a feature flag to enable calling Delete() on the composite with the PropagationPolicy=Foreground parameter in the Claim reconciler, essentially using foreground deletion for all cases

I think I prefer (1) but I don't know all of the background on the current implementation.

@negz @muvaf - Thoughts? Thanks

[negz]

@bobh66 Thanks for looking into this. I generally like the approach of enabling foreground deletion. I thought that we had ruled this approach out as something we couldn't make work, but that was a long time ago and I don't recall why. Perhaps @muvaf does (though I believe he's out on vacation)?

I discovered that the Claim is not defined as an Owner of the Composite

Unfortunately it's not possible for a claim to be the owner or controller of an XR because claims are namespaced while XRs are cluster scoped. From https://kubernetes.io/docs/concepts/architecture/garbage-collection/#owners-dependents:

Cluster-scoped dependents can only specify cluster-scoped owners. In v1.20+, if a cluster-scoped dependent specifies a namespaced kind as an owner, it is treated as having an unresolvable owner reference, and is not able to be garbage collected.

Crossplane needs to maintain the existence of the composite/managed resources under it's control until the last non-Crossplane finalizer has been deleted, at which time the normal Delete processing can be executed.

I'm fairly sure Crossplane doesn't get a choice here - the API server ensures that deletions are never "complete" until all finalizers are removed. The process is something like:

1. Crossplane (or any entity really) calls Delete, which sets meta.deletionTimestamp.
2. The API server waits for all metadata.finalizers to be removed.
3. The API server completes the deletion.

The resource does not actually disappear from the API server until step 3.

I thought it might be possible to detect the cascading foreground deletion request when the Claim was deleted - kubernetes seems to apply the foregroundDeletion finalizer to the claim for a brief period of time, but it's not there when the Claim reconciler gets the object, so we can't call the composite Delete() with the Foreground Propagation Policy which would serve the same purpose as having an ownerReference link.

That's a shame - that would be my preferred approach. I'm guessing the finalizer is only there for a moment because the claim has no dependents blocking its deletion.

2 - Add an attribute to the Claim/Composite which can be set to cause the Claim reconciler to pass the PropagationPolicy=Foreground parameter to the Delete call on the composite resource

This seems like the best option to me. If we had a do-over I'd be tempted to make foreground deletion the default, despite the fact that it diverges from typical Kubernetes defaults. Unfortunately the divergence combined with the behaviour change makes me think we need to stick with background by default.

[bobh66]

Thanks @negz - I knew there had to be a good reason for the way it is designed.

I'm fairly sure Crossplane doesn't get a choice here - the API server ensures that deletions are never "complete" until all finalizers are removed.

That's true, the Crossplane resource instance (Claim, Composite or Managed) is preserved until the last finalizer is removed, but the Crossplane reconcilers execute their Delete logic without regard for the external finalizers, and so the "external" state of the resources is not maintained while the finalizers exist.

This seems like the best option to me

Thanks - I'll add a compositeDeletePolicy attribute to the Claim - it doesn't need to be on the Composite since the only place it will be used is in the claim reconciler. It will default to Background, and we can use the value of the attribute for the Propagation Policy argument to the Delete() call for the composite.

[MisterMX]

I am not sure if that relates to the core of this issue but I think the deletion process could be "improved" by keeping the finalizer alive until all subresources are gone. Meaning, the XR stays alive until the XRC is gone and the XRC stays alive until all composed resources are gone.

I get a lot of complains from users that the XRs instantly vanish while MRs stay around until deletion (if you don't use orphan delete policy). To them the way Crossplane handles it feels like an antipattern.

[bobh66]

I believe this change will do what you are asking, with the exception of the XRC->XR., though that could also be implemented if so desired.

In the existing implementation of these changes, all of the resources that "belong" to an XR (MRs and other XRs) will all be deleted before the XR is deleted, working from the edge nodes towards the top-level Composite.

However since the cluster-scoped Composite cannot have an ownerReference that points at a namespaced Claim object, the Claim gets deleted without waiting for the Composite to go away. The Claim reconciler would need to be modified to not remove the finalizer on the Claim until it detects that the Composite has been deleted.

@negz - what do you think about using the compositeDeletePolicy value of "Foreground" to also cause the Claim reconciler to not remove the Claim's finalizer until the Composite has been deleted? It seems like it would be consistent with the desire to use Foreground deletion.

[negz]

@negz - what do you think about using the compositeDeletePolicy value of "Foreground" to also cause the Claim reconciler to not remove the Claim's finalizer until the Composite has been deleted? It seems like it would be consistent with the desire to use Foreground deletion.

@bobh66 That sounds reasonable.

[MisterMX]

Does it makes sense to let the claim deletion behave like defined in spec.compositeDeletionPolicy (Foreground, Background or Orphan), ignoring any additional finalizers and the --cascade option (which we cannot access anyway, appearently)?

And let all composed resources have blockOwnerDeletion: true set, so the deletion behaves as intended by the --cascade option?

Additionally we add a spec.forcedCompositeDeletionPolicy to the XRD, to overwrite the user settings.

This might not be "by the book", but I am seeing this from the perspectives of a platform provider:

• Users (aka XRC writers) do not have access to XRs and MRs, in fact they shouldn't know about them.
• XRCs define an abstract API that hides all complexity. A user should not care (and know) that there are security groups, subnets and more created for his "Database".
• Users in many cases do not have very deep knowledge about K8s in general and Kubectl's --cascade option in specific. When a user deletes his "Database" claim and its gone, then he considers it to be actually, physically, deleted, not just hidden. The platform engineer needs to be able to consider this. The --cascade option is written with the image of a power user in mind, who has access to both Deployment and Pods and knows exactly what he is doing.
• Platform operators need to be able to prevent users from using background orphan deletion. Otherwise it would allow them to create chaos with failed and orphan resources that don't belong to anything.

I hope this makes it clear what I have in mind.

[bobh66]

Does it makes sense to let the claim deletion behave like defined in spec.compositeDeletionPolicy (Foreground, Background or Orphan), ignoring any additional finalizers and the --cascade option (which we cannot access anyway, appearently)?

compositeDeletePolicy does not have an Orphan option as it is currently defined - I don't think it makes sense to "Orphan" a Composite object the way we Orphan Managed Resources.

And let all composed resources have blockOwnerDeletion: true set, so the deletion behaves as intended by the --cascade option?

That is set in the crossplane-runtime code change:
https://github.com/crossplane/crossplane-runtime/pull/355/files#diff-898270f422155d6d5b46facaf0aaafe21c07e22a7ee4544b2c5ae0f7ed9eb3c6R117

Additionally we add a spec.forcedCompositeDeletionPolicy to the XRD, to overwrite the user settings.

Is this intended to prevent the user from specifying "Orphan" deletion of the claim?

Platform operators need to be able to prevent users from using background orphan deletion. Otherwise it would allow them to create chaos with failed and orphan resources that don't belong to anything.

I believe the cascade=orphan handling relies on the ownerReferences in the dependent objects the same way that foreground deletion does, so it cannot be applied to a Claim because there are no ownerReferences pointing at the Claim. So I don't think this scenario is a problem since even if the user specifies cascade=orphan when they delete a claim it will not impact the deletion of the Composite and Managed resources.