Remove reconciliation resource version cache

internal/controllers/reconciliation/controller.go

@@ -153,7 +153,7 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request)
 	}
 
 	// Fetch the current resource
-	current, hasChanged, err := c.getCurrent(ctx, resource)
+	current, err := c.getCurrent(ctx, resource)
 	if client.IgnoreNotFound(err) != nil && !isErrMissingNS(err) {
 		return ctrl.Result{}, fmt.Errorf("getting current state: %w", err)
 	}
@@ -195,30 +195,19 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request)
 		}
 	}
 
-	// Nil current struct means the resource version hasn't changed since it was last observed
-	// Skip without logging since this is a very hot path
-	var modified bool
-	if hasChanged {
-		resource.ObserveVersion("") // in case reconciliation fails, invalidate the cache first to avoid skipping the next attempt
-		modified, err = c.reconcileResource(ctx, comp, prev, resource, current)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
+	modified, err := c.reconcileResource(ctx, comp, prev, resource, current)
+	if err != nil {
+		return ctrl.Result{}, err
 	}
-
-	// We requeue to make sure the resource is in sync before updating our cache's resource version
-	// Otherwise the next sync would just hit the cache without actually diffing the resource.
+	// If we modified the resource, we should also re-evaluate readiness
+	// without waiting for the interval.
 	if modified {
 		return ctrl.Result{Requeue: true}, nil
 	}
-	if current != nil {
-		if rv := current.GetResourceVersion(); rv != "" {
-			resource.ObserveVersion(rv)
-		}
-	}
 
-	// Store the results
-	deleted := current == nil || current.GetDeletionTimestamp() != nil
+	deleted := current == nil ||
+		current.GetDeletionTimestamp() != nil ||
+		(resource.Deleted() && comp.Annotations["eno.azure.io/deletion-strategy"] == "orphan") // orphaning should be reflected on the status.
 	c.writeBuffer.PatchStatusAsync(ctx, &resource.ManifestRef, patchResourceState(deleted, ready))
 	if ready == nil {
 		return ctrl.Result{RequeueAfter: wait.Jitter(c.readinessPollInterval, 0.1)}, nil
@@ -354,33 +343,17 @@ func (c *Controller) buildPatch(ctx context.Context, prev, next *reconstitution.
 	return patch, types.StrategicMergePatchType, err
 }
 
-func (c *Controller) getCurrent(ctx context.Context, resource *reconstitution.Resource) (*unstructured.Unstructured, bool, error) {
-	if resource.HasBeenSeen() && !resource.Deleted() {
-		meta := &metav1.PartialObjectMetadata{}
-		meta.Name = resource.Ref.Name
-		meta.Namespace = resource.Ref.Namespace
-		meta.Kind = resource.GVK.Kind
-		meta.APIVersion = resource.GVK.GroupVersion().String()
-		err := c.upstreamClient.Get(ctx, client.ObjectKeyFromObject(meta), meta)
-		if err != nil {
-			return nil, false, err
-		}
-		if resource.MatchesLastSeen(meta.ResourceVersion) {
-			return nil, false, nil
-		}
-		resourceVersionChanges.Inc()
-	}
-
+func (c *Controller) getCurrent(ctx context.Context, resource *reconstitution.Resource) (*unstructured.Unstructured, error) {
 	current := &unstructured.Unstructured{}
 	current.SetName(resource.Ref.Name)
 	current.SetNamespace(resource.Ref.Namespace)
 	current.SetKind(resource.GVK.Kind)
 	current.SetAPIVersion(resource.GVK.GroupVersion().String())
 	err := c.upstreamClient.Get(ctx, client.ObjectKeyFromObject(current), current)
 	if err != nil {
-		return nil, true, err
+		return nil, err
 	}
-	return current, true, nil
+	return current, nil
 }
 
 func mungePatch(patch []byte, rv string) ([]byte, error) {

internal/controllers/reconciliation/crud_test.go

@@ -21,6 +21,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	apiv1 "github.com/Azure/eno/api/v1"
+	v1 "github.com/Azure/eno/api/v1"
 	testv1 "github.com/Azure/eno/internal/controllers/reconciliation/fixtures/v1"
 	"github.com/Azure/eno/internal/controllers/synthesis"
 	"github.com/Azure/eno/internal/execution"
@@ -755,6 +756,72 @@ func TestOrphanedCompositionDeletion(t *testing.T) {
 	})
 }
 
+func TestOrphanedResources(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
+
+	registerControllers(t, mgr)
+	testutil.WithFakeExecutor(t, mgr, func(ctx context.Context, s *apiv1.Synthesizer, input *krmv1.ResourceList) (*krmv1.ResourceList, error) {
+		output := &krmv1.ResourceList{}
+		output.Items = []*unstructured.Unstructured{{
+			Object: map[string]any{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]any{
+					"name":      "test-obj",
+					"namespace": "default",
+					"annotations": map[string]string{
+						"eno.azure.io/reconcile-interval": "100ms",
+					},
+				},
+				"data": map[string]string{"foo": "bar"},
+			},
+		}}
+		return output, nil
+	})
+
+	setupTestSubject(t, mgr)
+	mgr.Start(t)
+	_, comp := writeComposition(t, upstream, true)
+
+	// Wait for resource to be created.
+	obj := &corev1.ConfigMap{}
+	testutil.Eventually(t, func() bool {
+		obj.SetName("test-obj")
+		obj.SetNamespace("default")
+		err := downstream.Get(ctx, client.ObjectKeyFromObject(obj), obj)
+		return err == nil
+	})
+
+	// Delete the composition.
+	require.NoError(t, upstream.Delete(ctx, comp))
+	t.Logf("deleted composition")
+	testutil.Eventually(t, func() bool {
+		return errors.IsNotFound(upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp))
+	})
+
+	// Ensure that the slice was deleted before checking the actual resource.
+	testutil.Eventually(t, func() bool {
+		rll := &v1.ResourceSliceList{}
+		err := upstream.List(ctx, rll, client.InNamespace(metav1.NamespaceAll))
+		if err != nil {
+			return false
+		}
+		return len(rll.Items) == 0
+	})
+
+	// The resource should be orphaned after the composition is gone.
+	err := downstream.Get(ctx, client.ObjectKeyFromObject(obj), obj)
+	require.NoError(t, err)
+
+}
+
 // TestResourceDefaulting proves that resources which define properties equal to the field's default will eventually converge.
 func TestResourceDefaulting(t *testing.T) {
 	scheme := runtime.NewScheme()

internal/controllers/reconciliation/helpers_test.go

@@ -35,6 +35,10 @@ func registerControllers(t *testing.T, mgr *testutil.Manager) {
 }
 
 func writeGenericComposition(t *testing.T, client client.Client) (*apiv1.Synthesizer, *apiv1.Composition) {
+	return writeComposition(t, client, false)
+}
+
+func writeComposition(t *testing.T, client client.Client, orphan bool) (*apiv1.Synthesizer, *apiv1.Composition) {
 	syn := &apiv1.Synthesizer{}
 	syn.Name = "test-syn"
 	syn.Spec.Image = "create"
@@ -44,6 +48,9 @@ func writeGenericComposition(t *testing.T, client client.Client) (*apiv1.Synthes
 	comp.Name = "test-comp"
 	comp.Namespace = "default"
 	comp.Spec.Synthesizer.Name = syn.Name
+	if orphan {
+		comp.Annotations = map[string]string{"eno.azure.io/deletion-strategy": "orphan"}
+	}
 	require.NoError(t, client.Create(context.Background(), comp))
 
 	return syn, comp

internal/resource/resource.go

@@ -43,7 +43,6 @@ type ManifestRef struct {
 
 // Resource is the controller's internal representation of a single resource out of a ResourceSlice.
 type Resource struct {
-	lastSeenMeta
 	lastReconciledMeta
 
 	Ref               Ref
@@ -257,29 +256,6 @@ type patchMeta struct {
 	Ops        jsonpatch.Patch `json:"ops"`
 }
 
-type lastSeenMeta struct {
-	lock            sync.Mutex
-	resourceVersion string
-}
-
-func (l *lastSeenMeta) ObserveVersion(rv string) {
-	l.lock.Lock()
-	defer l.lock.Unlock()
-	l.resourceVersion = rv
-}
-
-func (l *lastSeenMeta) HasBeenSeen() bool {
-	l.lock.Lock()
-	defer l.lock.Unlock()
-	return l.resourceVersion != ""
-}
-
-func (l *lastSeenMeta) MatchesLastSeen(rv string) bool {
-	l.lock.Lock()
-	defer l.lock.Unlock()
-	return l.resourceVersion == rv
-}
-
 type lastReconciledMeta struct {
 	lock           sync.Mutex
 	lastReconciled *time.Time