Repository: crossplane/crossplane

Issue:
Promote Usage API to Beta
### What problem are you facing?

This is the tracking issue for promoting the Usage API (a.k.a. deletion ordering) to Beta.
With the feature recently merged into the master, we don't have any issues yet, but we will list them here as they appear.

### How could Crossplane help solve your problem?

Promote Usage to Beta once we feel confident about it.

```[tasklist]
### Tasks
- [ ] https://github.com/crossplane/crossplane/issues/5042
- [ ] https://github.com/crossplane/crossplane/issues/5193
- [ ] https://github.com/crossplane/crossplane/issues/6140
- [ ] https://github.com/crossplane/crossplane/issues/5880
- [ ] https://github.com/crossplane/crossplane/issues/5962
```


Discussion:
@lsviben: Added an issue based on [Community Slack](https://crossplane.slack.com/archives/CEG3T90A1/p1699996775424049) feedback:
https://github.com/crossplane/crossplane/issues/5042 



@stevendborrelli: I need to be able to duplicate the issue, but I have seen instances where due to long running deletions like for DBs or K8s Clusters when having usage dependences via nested XRs, there can be a 15-minute pause between attempted deletes. We should address it before beta.@bobh66: In my local implementation of this feature using a kopf operator we see this same problem - it's caused by the garbage collector doing an exponential backoff on the delete retries that maxes out at 1000 seconds (16+ minutes).  So when you have something that takes a long time to delete the GC backoff will make it even longer.

I worked around this by explicitly retrying the delete on the "protected" resource when the dependency/usage itself is successfully deleted (the webhook determines that the Usage is no longer required).  That seems to speed things up significantly.

I think we discussed this in the original implementation and decided to defer any solution until we had some data on how it was actually working.  Explicitly retrying the delete feels risky, but if the resource is already marked for deletion (which it should be) then forcing GC to retry again sooner than it's backoff period should not cause any issues.@stevendborrelli: Created issue #5193, this has impacted several users and should be addressed prior to beta graduation. @bobh66: @turkenh any thoughts on adding this to the 1.18 roadmap?  It would be nice to get it promoted.@jbw976: Agreed @bobh66 - looks like it was set to "target release" of v1.18 in the roadmap project, but not set to the v1.18 milestone too.

I tend to set both of these fields, but sometimes I forget. I like the "target release" field more often because it spans across all the Crossplane repos, and milestones are per repo 😇 

Solution patch:
diff --git a/apis/apiextensions/v1alpha1/usage_types.go b/apis/apiextensions/v1alpha1/usage_types.go
index f590b4a10..1cb1ee0e8 100644
--- a/apis/apiextensions/v1alpha1/usage_types.go
+++ b/apis/apiextensions/v1alpha1/usage_types.go
@@ -68,6 +68,9 @@ type UsageSpec struct {
 	// Reason is the reason for blocking deletion of the resource.
 	// +optional
 	Reason *string `json:"reason,omitempty"`
+	// ReplayDeletion will trigger a deletion on the used resource during the deletion of the usage itself, if it was attempted to be deleted at least once.
+	// +optional
+	ReplayDeletion *bool `json:"replayDeletion,omitempty"`
 }
 
 // UsageStatus defines the observed state of Usage.
diff --git a/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go b/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go
index c53d5a361..45148977d 100644
--- a/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/apiextensions/v1alpha1/zz_generated.deepcopy.go
@@ -229,6 +229,11 @@ func (in *UsageSpec) DeepCopyInto(out *UsageSpec) {
 		*out = new(string)
 		**out = **in
 	}
+	if in.ReplayDeletion != nil {
+		in, out := &in.ReplayDeletion, &out.ReplayDeletion
+		*out = new(bool)
+		**out = **in
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UsageSpec.
diff --git a/cluster/crds/apiextensions.crossplane.io_usages.yaml b/cluster/crds/apiextensions.crossplane.io_usages.yaml
index 0cd30d52f..e05a9b444 100644
--- a/cluster/crds/apiextensions.crossplane.io_usages.yaml
+++ b/cluster/crds/apiextensions.crossplane.io_usages.yaml
@@ -138,6 +138,11 @@ spec:
               reason:
                 description: Reason is the reason for blocking deletion of the resource.
                 type: string
+              replayDeletion:
+                description: ReplayDeletion will trigger a deletion on the used resource
+                  during the deletion of the usage itself, if it was attempted to
+                  be deleted at least once.
+                type: boolean
             required:
             - of
             type: object
diff --git a/internal/controller/apiextensions/usage/reconciler.go b/internal/controller/apiextensions/usage/reconciler.go
index f34edfb40..0ef151edd 100644
--- a/internal/controller/apiextensions/usage/reconciler.go
+++ b/internal/controller/apiextensions/usage/reconciler.go
@@ -86,6 +86,7 @@ const (
 	reasonRemoveInUseLabel event.Reason = "RemoveInUseLabel"
 	reasonAddFinalizer     event.Reason = "AddFinalizer"
 	reasonRemoveFinalizer  event.Reason = "RemoveFinalizer"
+	reasonReplayDeletion   event.Reason = "ReplayDeletion"
 
 	reasonUsageConfigured event.Reason = "UsageConfigured"
 	reasonWaitUsing       event.Reason = "WaitingUsingDeleted"
@@ -316,6 +317,23 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 			}
 		}
 
+		if u.Spec.ReplayDeletion != nil && *u.Spec.ReplayDeletion && used.GetAnnotations() != nil {
+			if policy, ok := used.GetAnnotations()[usage.AnnotationKeyDeletionAttempt]; ok {
+				// We have already recorded a deletion attempt and want to replay deletion, let's delete the used resource.
+				go func() {
+					// We do the deletion async and after some delay to make sure the usage is deleted before the
+					// deletion attempt. We remove the finalizer on this Usage right below, so, we know it will disappear
+					// very soon.
+					time.Sleep(2 * time.Second)
+					log.Info("Replaying deletion of the used resource", "apiVersion", used.GetAPIVersion(), "kind", used.GetKind(), "name", used.GetName(), "policy", policy)
+					// We cannot use the context from the reconcile function since it will be cancelled after the reconciliation.
+					if err = r.client.Delete(context.Background(), used, client.PropagationPolicy(policy)); err != nil {
+						log.Info("Error when replaying deletion of the used resource", "apiVersion", used.GetAPIVersion(), "kind", used.GetKind(), "name", used.GetName(), "err", err)
+					}
+				}()
+			}
+		}
+
 		// Remove the finalizer from the usage
 		if err = r.usage.RemoveFinalizer(ctx, u); err != nil {
 			log.Debug(errRemoveFinalizer, "error", err)
diff --git a/internal/usage/handler.go b/internal/usage/handler.go
index 6017f01bf..cbc84f9ca 100644
--- a/internal/usage/handler.go
+++ b/internal/usage/handler.go
@@ -30,10 +30,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/yaml"
 
 	"github.com/crossplane/crossplane-runtime/pkg/controller"
 	"github.com/crossplane/crossplane-runtime/pkg/errors"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
+	xpmeta "github.com/crossplane/crossplane-runtime/pkg/meta"
 	xpunstructured "github.com/crossplane/crossplane-runtime/pkg/resource/unstructured"
 
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
@@ -44,6 +46,11 @@ const (
 	// indexing and retrieving needed CRDs.
 	InUseIndexKey = "inuse.apiversion.kind.name"
 
+	// AnnotationKeyDeletionAttempt is the annotation key used to record whether
+	// a deletion attempt was made and blocked by the Usage. The value stored is
+	// the propagation policy used with the deletion attempt.
+	AnnotationKeyDeletionAttempt = "usage.crossplane.io/deletion-attempt-with-policy"
+
 	// Error strings.
 	errFmtUnexpectedOp = "unexpected operation %q, expected \"DELETE\""
 )
@@ -87,7 +94,7 @@ func SetupWebhookWithManager(mgr ctrl.Manager, options controller.Options) error
 
 // Handler implements the admission Handler for Composition.
 type Handler struct {
-	reader client.Reader
+	client client.Client
 	log    logging.Logger
 }
 
@@ -102,9 +109,9 @@ func WithLogger(l logging.Logger) HandlerOption {
 }
 
 // NewHandler returns a new Handler.
-func NewHandler(reader client.Reader, opts ...HandlerOption) *Handler {
+func NewHandler(client client.Client, opts ...HandlerOption) *Handler {
 	h := &Handler{
-		reader: reader,
+		client: client,
 		log:    logging.NewNopLogger(),
 	}
 
@@ -126,22 +133,44 @@ func (h *Handler) Handle(ctx context.Context, request admission.Request) admissi
 		if err := u.UnmarshalJSON(request.OldObject.Raw); err != nil {
 			return admission.Errored(http.StatusBadRequest, err)
 		}
-		return h.validateNoUsages(ctx, u)
+		opts := &metav1.DeleteOptions{}
+		if err := yaml.Unmarshal(request.Options.Raw, opts); err != nil {
+			return admission.Errored(http.StatusBadRequest, err)
+		}
+		return h.validateNoUsages(ctx, u, opts)
 	default:
 		return admission.Errored(http.StatusBadRequest, errors.Errorf(errFmtUnexpectedOp, request.Operation))
 	}
 }
 
-func (h *Handler) validateNoUsages(ctx context.Context, u *unstructured.Unstructured) admission.Response {
-	h.log.Debug("Validating no usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName())
+func (h *Handler) validateNoUsages(ctx context.Context, u *unstructured.Unstructured, opts *metav1.DeleteOptions) admission.Response {
+	h.log.Debug("Validating no usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "policy", opts.PropagationPolicy)
 	usageList := &v1alpha1.UsageList{}
-	if err := h.reader.List(ctx, usageList, client.MatchingFields{InUseIndexKey: IndexValueForObject(u)}); err != nil {
+	if err := h.client.List(ctx, usageList, client.MatchingFields{InUseIndexKey: IndexValueForObject(u)}); err != nil {
 		h.log.Debug("Error when getting Usages", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "err", err)
 		return admission.Errored(http.StatusInternalServerError, err)
 	}
 	if len(usageList.Items) > 0 {
 		msg := inUseMessage(usageList)
 		h.log.Debug("Usage found, deletion not allowed", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "msg", msg)
+
+		// Use the default propagation policy if not provided
+		policy := metav1.DeletePropagationBackground
+		if opts.PropagationPolicy != nil {
+			policy = *opts.PropagationPolicy
+		}
+		// If the resource is being deleted, we want to record the first deletion attempt
+		// so that we can track whether a deletion was attempted at least once.
+		if u.GetAnnotations() == nil || u.GetAnnotations()[AnnotationKeyDeletionAttempt] != string(policy) {
+			orig := u.DeepCopy()
+			xpmeta.AddAnnotations(u, map[string]string{AnnotationKeyDeletionAttempt: string(policy)})
+			// Patch the resource to add the deletion attempt annotation
+			if err := h.client.Patch(ctx, u, client.MergeFrom(orig)); err != nil {
+				h.log.Debug("Error when patching the resource to add the deletion attempt annotation", "apiVersion", u.GetAPIVersion(), "kind", u.GetKind(), "name", u.GetName(), "err", err)
+				return admission.Errored(http.StatusInternalServerError, err)
+			}
+		}
+
 		return admission.Response{
 			AdmissionResponse: admissionv1.AdmissionResponse{
 				Allowed: false,



Test patch:
diff --git a/internal/controller/apiextensions/usage/reconciler_test.go b/internal/controller/apiextensions/usage/reconciler_test.go
index 2b076325d..3225b79b3 100644
--- a/internal/controller/apiextensions/usage/reconciler_test.go
+++ b/internal/controller/apiextensions/usage/reconciler_test.go
@@ -26,6 +26,7 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -38,6 +39,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
+	"github.com/crossplane/crossplane/internal/usage"
 	"github.com/crossplane/crossplane/internal/xcrd"
 )
 
@@ -723,6 +725,58 @@ func TestReconcile(t *testing.T) {
 				r: reconcile.Result{},
 			},
 		},
+		"SuccessfulDeleteWithReplayDeletion": {
+			reason: "We should replay deletion after usage is gone and replayDeletion is true.",
+			args: args{
+				mgr: &fake.Manager{},
+				opts: []ReconcilerOption{
+					WithClientApplicator(xpresource.ClientApplicator{
+						Client: &test.MockClient{
+							MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+								if o, ok := obj.(*v1alpha1.Usage); ok {
+									o.SetDeletionTimestamp(&now)
+									o.Spec.ReplayDeletion = ptr.To(true)
+									o.Spec.Of.ResourceRef = &v1alpha1.ResourceRef{Name: "cool"}
+									return nil
+								}
+								if o, ok := obj.(*composed.Unstructured); ok {
+									o.SetAnnotations(map[string]string{usage.AnnotationKeyDeletionAttempt: string(metav1.DeletePropagationBackground)})
+									o.SetLabels(map[string]string{inUseLabelKey: "true"})
+									return nil
+								}
+								return errors.New("unexpected object type")
+							}),
+							MockList: test.NewMockListFn(nil, func(_ client.ObjectList) error {
+								return nil
+							}),
+							MockUpdate: test.NewMockUpdateFn(nil, func(obj client.Object) error {
+								if o, ok := obj.(*composed.Unstructured); ok {
+									if o.GetLabels()[inUseLabelKey] != "" {
+										t.Errorf("expected in use label to be removed")
+									}
+									return nil
+								}
+								return errors.New("unexpected object type")
+							}),
+							MockDelete: func(_ context.Context, _ client.Object, _ ...client.DeleteOption) error {
+								return nil
+							},
+						},
+					}),
+					WithSelectorResolver(fakeSelectorResolver{
+						resourceSelectorFn: func(_ context.Context, _ *v1alpha1.Usage) error {
+							return nil
+						},
+					}),
+					WithFinalizer(xpresource.FinalizerFns{RemoveFinalizerFn: func(_ context.Context, _ xpresource.Object) error {
+						return nil
+					}}),
+				},
+			},
+			want: want{
+				r: reconcile.Result{},
+			},
+		},
 		"SuccessfulWaitWhenUsingStillThere": {
 			reason: "We should wait until the using resource is deleted.",
 			args: args{
diff --git a/internal/usage/handler_test.go b/internal/usage/handler_test.go
index 7340cfd23..734f3da30 100644
--- a/internal/usage/handler_test.go
+++ b/internal/usage/handler_test.go
@@ -42,7 +42,7 @@ var errBoom = errors.New("boom")
 func TestHandle(t *testing.T) {
 	protected := "This resource is protected!"
 	type args struct {
-		reader  client.Reader
+		client  client.Client
 		request admission.Request
 	}
 	type want struct {
@@ -121,7 +121,7 @@ func TestHandle(t *testing.T) {
 		"DeleteAllowedNoUsages": {
 			reason: "We should allow a delete request if there is no usages for the given object.",
 			args: args{
-				reader: &test.MockClient{
+				client: &test.MockClient{
 					MockList: func(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error {
 						return nil
 					},
@@ -147,7 +147,7 @@ func TestHandle(t *testing.T) {
 		"DeleteRejectedCannotList": {
 			reason: "We should reject a delete request if we cannot list usages.",
 			args: args{
-				reader: &test.MockClient{
+				client: &test.MockClient{
 					MockList: func(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error {
 						return errBoom
 					},
@@ -173,7 +173,10 @@ func TestHandle(t *testing.T) {
 		"DeleteBlockedWithUsageBy": {
 			reason: "We should reject a delete request if there are usages for the given object with \"by\" defined.",
 			args: args{
-				reader: &test.MockClient{
+				client: &test.MockClient{
+					MockPatch: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
+						return nil
+					},
 					MockList: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error {
 						l := list.(*v1alpha1.UsageList)
 						l.Items = []v1alpha1.Usage{
@@ -231,7 +234,10 @@ func TestHandle(t *testing.T) {
 		"DeleteBlockedWithUsageReason": {
 			reason: "We should reject a delete request if there are usages for the given object with \"reason\" defined.",
 			args: args{
-				reader: &test.MockClient{
+				client: &test.MockClient{
+					MockPatch: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
+						return nil
+					},
 					MockList: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error {
 						l := list.(*v1alpha1.UsageList)
 						l.Items = []v1alpha1.Usage{
@@ -283,7 +289,10 @@ func TestHandle(t *testing.T) {
 		"DeleteBlockedWithUsageNone": {
 			reason: "We should reject a delete request if there are usages for the given object without \"reason\" or \"by\" defined.",
 			args: args{
-				reader: &test.MockClient{
+				client: &test.MockClient{
+					MockPatch: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
+						return nil
+					},
 					MockList: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error {
 						l := list.(*v1alpha1.UsageList)
 						l.Items = []v1alpha1.Usage{
@@ -334,7 +343,7 @@ func TestHandle(t *testing.T) {
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			h := NewHandler(tc.args.reader, WithLogger(logging.NewNopLogger()))
+			h := NewHandler(tc.args.client, WithLogger(logging.NewNopLogger()))
 			got := h.Handle(context.Background(), tc.args.request)
 			if diff := cmp.Diff(tc.want.resp, got); diff != "" {
 				t.Errorf("%s\nHandle(...): -want response, +got:\n%s", tc.reason, diff)
diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml
index 6c54d3b9b..e1cd38bcc 100644
--- a/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml
+++ b/test/e2e/manifests/apiextensions/usage/standalone/with-by/usage.yaml
@@ -3,6 +3,7 @@ kind: Usage
 metadata:
   name: using-uses-used
 spec:
+  replayDeletion: true
   of:
     apiVersion: nop.crossplane.io/v1alpha1
     kind: NopResource
diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml
index da0ec9f17..3fa17b096 100644
--- a/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml
+++ b/test/e2e/manifests/apiextensions/usage/standalone/with-by/used.yaml
@@ -7,9 +7,6 @@ metadata:
 spec:
   forProvider:
     conditionAfter:
-      - conditionType: "Synced"
-        conditionStatus: "True"
-        time: "5s"
       - conditionType: "Ready"
         conditionStatus: "True"
         time: "10s"
\ No newline at end of file
diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml
index b182b1d5a..62a6edcac 100644
--- a/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml
+++ b/test/e2e/manifests/apiextensions/usage/standalone/with-by/using.yaml
@@ -5,9 +5,6 @@ metadata:
 spec:
   forProvider:
     conditionAfter:
-      - conditionType: "Synced"
-        conditionStatus: "True"
-        time: "5s"
       - conditionType: "Ready"
         conditionStatus: "True"
         time: "10s"
\ No newline at end of file
diff --git a/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml b/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml
index ace8b8701..af0060395 100644
--- a/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml
+++ b/test/e2e/manifests/apiextensions/usage/standalone/with-reason/used.yaml
@@ -7,9 +7,6 @@ metadata:
 spec:
   forProvider:
     conditionAfter:
-      - conditionType: "Synced"
-        conditionStatus: "True"
-        time: "5s"
       - conditionType: "Ready"
         conditionStatus: "True"
         time: "10s"
\ No newline at end of file
diff --git a/test/e2e/usage_test.go b/test/e2e/usage_test.go
index 9fcb0608e..065c504b7 100644
--- a/test/e2e/usage_test.go
+++ b/test/e2e/usage_test.go
@@ -59,10 +59,8 @@ func TestUsageStandalone(t *testing.T) {
 				funcs.DeleteResources(manifests, "with-by/using.yaml"),
 				funcs.ResourcesDeletedWithin(30*time.Second, manifests, "with-by/using.yaml"),
 				funcs.ResourcesDeletedWithin(30*time.Second, manifests, "with-by/usage.yaml"),
-
-				// Deletion of used resource should be allowed after usage is cleared.
-				funcs.DeleteResources(manifests, "with-by/used.yaml"),
-				funcs.ResourcesDeletedWithin(30*time.Second, manifests, "with-by/used.yaml"),
+				// We have "replayDeletion: true" on the usage, deletion of used resource should be replayed after usage is cleared.
+				funcs.ResourcesDeletedWithin(1*time.Minute, manifests, "with-by/used.yaml"),
 			),
 		},
 		{