Get close to a working resource cleanup implementation

internal/controllers/reconciliation/controller.go

@@ -24,8 +24,6 @@ import (
 	"github.com/go-logr/logr"
 )
 
-// TODO: Block ResourceSlice deletion until resources have been cleaned up
-
 type Controller struct {
 	client         client.Client
 	resourceClient reconstitution.Client

internal/controllers/resourceslice/cleanup.go

@@ -16,10 +16,6 @@ import (
 
 // TODO: Should we limit the max number of resource slices per composition to protect etcd in the face of partial resource slice write failures?
 
-// TODO: Make sure it isn't possible for slices to be deleted before the tombstones are passed down
-
-// TODO: Another controller/finalizer to prevent compositions from being deleted before any tombstones have been reconciled
-
 type cleanupController struct {
 	client client.Client
 }
@@ -52,6 +48,10 @@ func (c *cleanupController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	comp.Name = owner.Name
 	comp.Namespace = slice.Namespace
 	err = c.client.Get(ctx, client.ObjectKeyFromObject(comp), comp)
+	// TODO: This actually shouldn't use tombstones
+	if slice.DeletionTimestamp != nil && (errors.IsNotFound(err) || comp.DeletionTimestamp != nil) && tombstonesRemain(slice) {
+		return ctrl.Result{}, nil // wait for tombstones to be reconciled to avoid leaking resources
+	}
 	if errors.IsNotFound(err) {
 		return c.ensureFinalizerRemoved(ctx, slice)
 	}
@@ -101,3 +101,20 @@ func (c *cleanupController) ensureFinalizerRemoved(ctx context.Context, slice *a
 	logger.V(0).Info("released unused resource slice")
 	return ctrl.Result{}, nil
 }
+
+func tombstonesRemain(slice *apiv1.ResourceSlice) bool {
+	for i, manifest := range slice.Spec.Resources {
+		if !manifest.Deleted {
+			continue
+		}
+		if slice.Status.Resources == nil {
+			return true
+		}
+
+		state := slice.Status.Resources[i]
+		if !state.Reconciled {
+			return true
+		}
+	}
+	return false
+}

internal/controllers/resourceslice/cleanup_test.go

@@ -59,6 +59,49 @@ func TestSliceControllerHappyPath(t *testing.T) {
 	})
 }
 
+func TestSliceControllerCompositionCleanup(t *testing.T) {
+	ctx := testutil.NewContext(t)
+	mgr := testutil.NewManager(t)
+	require.NoError(t, NewCleanupController(mgr.Manager))
+	mgr.Start(t)
+
+	comp := &apiv1.Composition{}
+	comp.Name = "test-1"
+	comp.Namespace = "default"
+	require.NoError(t, mgr.GetClient().Create(ctx, comp))
+
+	slice := &apiv1.ResourceSlice{}
+	slice.Name = "test-1"
+	slice.Namespace = "default"
+	slice.Finalizers = []string{"eno.azure.io/cleanup"}
+	slice.Spec.Resources = []apiv1.Manifest{{Manifest: "test-tombstone", Deleted: true}}
+	require.NoError(t, controllerutil.SetControllerReference(comp, slice, mgr.GetScheme()))
+	require.NoError(t, mgr.GetClient().Create(ctx, slice))
+
+	comp.Status.CurrentState = &apiv1.Synthesis{
+		ResourceSlices: []*apiv1.ResourceSliceRef{{Name: slice.Name}},
+		Synthesized:    true,
+	}
+	require.NoError(t, mgr.GetClient().Status().Update(ctx, comp))
+	require.NoError(t, mgr.GetClient().Delete(ctx, comp))
+	require.NoError(t, mgr.GetClient().Delete(ctx, slice))
+
+	// Slice should not be deleted
+	time.Sleep(time.Millisecond * 50)
+	testutil.Eventually(t, func() bool {
+		return mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(slice), slice) == nil
+	})
+
+	// Reconcile the tombstone
+	slice.Status.Resources = []apiv1.ResourceState{{Reconciled: true}}
+	require.NoError(t, mgr.GetClient().Status().Update(ctx, slice))
+
+	// Slice should eventually be released
+	testutil.Eventually(t, func() bool {
+		return errors.IsNotFound(mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(slice), slice))
+	})
+}
+
 func TestSliceControllerMissingComposition(t *testing.T) {
 	ctx := testutil.NewContext(t)
 	mgr := testutil.NewManager(t)