diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index 42ea458c..bfb32d9d 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/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 diff --git a/internal/controllers/resourceslice/cleanup.go b/internal/controllers/resourceslice/cleanup.go index 605485f4..dc17a532 100644 --- a/internal/controllers/resourceslice/cleanup.go +++ b/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 +} diff --git a/internal/controllers/resourceslice/cleanup_test.go b/internal/controllers/resourceslice/cleanup_test.go index f55ea311..cf3ae151 100644 --- a/internal/controllers/resourceslice/cleanup_test.go +++ b/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)