diff --git a/api/v1/config/crd/eno.azure.io_resourceslices.yaml b/api/v1/config/crd/eno.azure.io_resourceslices.yaml index 4a28acff..0f9a140d 100644 --- a/api/v1/config/crd/eno.azure.io_resourceslices.yaml +++ b/api/v1/config/crd/eno.azure.io_resourceslices.yaml @@ -56,6 +56,11 @@ spec: spec.resources at the observed generation. items: properties: + deleted: + description: Deleted is true when the resource has been cleaned + up, either because spec.deleted == true or the parent composition + has been deleted. + type: boolean ready: description: nil if Eno is unable to determine the readiness of this resource. Otherwise it is true when the resource is diff --git a/api/v1/resourceslice.go b/api/v1/resourceslice.go index 14bdf4a9..df4ae279 100644 --- a/api/v1/resourceslice.go +++ b/api/v1/resourceslice.go @@ -48,6 +48,9 @@ type ResourceState struct { // Otherwise it is true when the resource is ready, false otherwise. // Like Reconciled, it latches and will never transition from true->false. Ready *bool `json:"ready,omitempty"` + + // Deleted is true when the resource has been cleaned up, either because spec.deleted == true or the parent composition has been deleted. + Deleted bool `json:"deleted,omitempty"` } type ResourceSliceRef struct { diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index bfb32d9d..1c24d0e5 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -111,12 +111,17 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request) return ctrl.Result{}, err } - c.resourceClient.PatchStatusAsync(ctx, &req.Manifest, func(rs *apiv1.ResourceState) bool { - if rs.Reconciled { - return false // already in sync + c.resourceClient.PatchStatusAsync(ctx, &req.Manifest, func(rs *apiv1.ResourceState) (modified bool) { + deleted := current.GetDeletionTimestamp() != nil || resource.Manifest.Deleted + if deleted && !rs.Deleted { + rs.Deleted = true + modified = true } - rs.Reconciled = true - return true + if !rs.Reconciled { + rs.Reconciled = true + modified = true + } + return }) if resource != nil && resource.Manifest.ReconcileInterval != nil { @@ -133,6 +138,8 @@ func (c *Controller) reconcileResource(ctx context.Context, prev, resource *reco return nil // already deleted - nothing to do } + // TODO: How to get resource slice deletion state here? + err := c.upstreamClient.Delete(ctx, resource.Object) if err != nil { return client.IgnoreNotFound(fmt.Errorf("deleting resource: %w", err)) diff --git a/internal/controllers/reconciliation/integration_test.go b/internal/controllers/reconciliation/integration_test.go index e31e49c4..77dd42a2 100644 --- a/internal/controllers/reconciliation/integration_test.go +++ b/internal/controllers/reconciliation/integration_test.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" apiv1 "github.com/Azure/eno/api/v1" testv1 "github.com/Azure/eno/internal/controllers/reconciliation/fixtures/v1" @@ -349,7 +350,7 @@ func TestReconcileInterval(t *testing.T) { comp.Spec.Synthesizer.Name = syn.Name require.NoError(t, upstream.Create(ctx, comp)) - // Wait for service to be created + // Wait for resource to be created obj := &corev1.ConfigMap{} testutil.Eventually(t, func() bool { obj.SetName("test-obj") @@ -446,3 +447,50 @@ func TestReconcileCacheRace(t *testing.T) { time.Sleep(time.Millisecond * 50) } } + +func TestReconcileStatus(t *testing.T) { + scheme := runtime.NewScheme() + corev1.SchemeBuilder.AddToScheme(scheme) + testv1.SchemeBuilder.AddToScheme(scheme) + + ctx := testutil.NewContext(t) + mgr := testutil.NewManager(t) + upstream := mgr.GetClient() + downstream := mgr.DownstreamClient + + rm, err := reconstitution.New(mgr.Manager, time.Millisecond) + require.NoError(t, err) + + err = New(rm, mgr.DownstreamRestConfig, 5, testutil.AtLeastVersion(t, 15)) + require.NoError(t, err) + mgr.Start(t) + + comp := &apiv1.Composition{} + comp.Name = "test-comp" + comp.Namespace = "default" + require.NoError(t, upstream.Create(ctx, comp)) + + slice := &apiv1.ResourceSlice{} + slice.Name = "test-slice" + slice.Namespace = comp.Namespace + require.NoError(t, controllerutil.SetControllerReference(comp, slice, upstream.Scheme())) + slice.Spec.Resources = []apiv1.Manifest{ + {Manifest: `{ "kind": "ConfigMap", "apiVersion": "v1", "metadata": { "name": "test", "namespace": "default" } }`}, + {Deleted: true, Manifest: `{ "kind": "ConfigMap", "apiVersion": "v1", "metadata": { "name": "test-deleted", "namespace": "default" } }`}, + } + require.NoError(t, upstream.Create(ctx, slice)) + + comp.Status.CurrentState = &apiv1.Synthesis{ + Synthesized: true, + ResourceSlices: []*apiv1.ResourceSliceRef{{Name: slice.Name}}, + } + require.NoError(t, upstream.Status().Update(ctx, comp)) + + // Status should eventually reflect the reconciliation state + testutil.Eventually(t, func() bool { + err = downstream.Get(context.Background(), client.ObjectKeyFromObject(slice), slice) + return err == nil && len(slice.Status.Resources) == 2 && + slice.Status.Resources[0].Reconciled && !slice.Status.Resources[0].Deleted && + slice.Status.Resources[1].Reconciled && slice.Status.Resources[1].Deleted + }) +} diff --git a/internal/controllers/resourceslice/cleanup.go b/internal/controllers/resourceslice/cleanup.go index dc17a532..a1e9840b 100644 --- a/internal/controllers/resourceslice/cleanup.go +++ b/internal/controllers/resourceslice/cleanup.go @@ -48,8 +48,7 @@ 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) { + if slice.DeletionTimestamp != nil && (errors.IsNotFound(err) || comp.DeletionTimestamp != nil) && resourcesRemain(slice) { return ctrl.Result{}, nil // wait for tombstones to be reconciled to avoid leaking resources } if errors.IsNotFound(err) { @@ -102,17 +101,12 @@ func (c *cleanupController) ensureFinalizerRemoved(ctx context.Context, slice *a 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 { +func resourcesRemain(slice *apiv1.ResourceSlice) bool { + if len(slice.Status.Resources) == 0 && len(slice.Spec.Resources) > 0 { + return true // status is lagging behind + } + for _, state := range slice.Status.Resources { + if !state.Deleted { return true } } diff --git a/internal/controllers/resourceslice/cleanup_test.go b/internal/controllers/resourceslice/cleanup_test.go index cf3ae151..c54ee0a6 100644 --- a/internal/controllers/resourceslice/cleanup_test.go +++ b/internal/controllers/resourceslice/cleanup_test.go @@ -74,7 +74,7 @@ func TestSliceControllerCompositionCleanup(t *testing.T) { slice.Name = "test-1" slice.Namespace = "default" slice.Finalizers = []string{"eno.azure.io/cleanup"} - slice.Spec.Resources = []apiv1.Manifest{{Manifest: "test-tombstone", Deleted: true}} + slice.Spec.Resources = []apiv1.Manifest{{Manifest: "test-resource"}} require.NoError(t, controllerutil.SetControllerReference(comp, slice, mgr.GetScheme())) require.NoError(t, mgr.GetClient().Create(ctx, slice)) @@ -92,8 +92,8 @@ func TestSliceControllerCompositionCleanup(t *testing.T) { return mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(slice), slice) == nil }) - // Reconcile the tombstone - slice.Status.Resources = []apiv1.ResourceState{{Reconciled: true}} + // Remove the resource + slice.Status.Resources = []apiv1.ResourceState{{Deleted: true}} require.NoError(t, mgr.GetClient().Status().Update(ctx, slice)) // Slice should eventually be released