From f32494269cc0d9183b3dbc0bf348fd6c711ce953 Mon Sep 17 00:00:00 2001 From: Ruinan Liu <72321026+ruinan-liu@users.noreply.github.com> Date: Mon, 16 Mar 2026 10:26:51 -0700 Subject: [PATCH] Revert "[eno] Add cross-reconciler deletion ordering fix (#571)" This reverts commit 1187a7ee25d534c32c765cc7519f91ead79db7b9. --- .../controllers/reconciliation/controller.go | 12 +- internal/resource/cache.go | 14 +- internal/resource/cache_test.go | 205 ------------- ...-both-crd-and-cr-and-readiness-groups.json | 12 +- ...tree-builder-both-crd-and-cr-conflict.json | 6 +- ...ee-builder-crd-and-cr-during-deletion.json | 6 +- .../fixtures/tree-builder-crd-and-cr.json | 6 +- .../tree-builder-deletion-groups.json | 12 +- ...ee-builder-several-overlapping-groups.json | 9 +- ...tree-builder-several-readiness-groups.json | 12 +- .../tree-builder-single-basic-resource.json | 3 +- internal/resource/resource.go | 2 - internal/resource/tree.go | 20 +- internal/resource/tree_test.go | 287 ------------------ 14 files changed, 28 insertions(+), 578 deletions(-) diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index bd000dd3..7959c4ae 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -151,15 +151,9 @@ func (c *Controller) Reconcile(ctx context.Context, req resource.Request) (ctrl. snap, current, ready, modified, err := c.reconcileResource(ctx, comp, prev, resource) failingOpen := c.shouldFailOpen(resource) if failingOpen { - // Don't fail open for resources with deletion ordering constraints - - // suppressing errors would break deletion group / foreground deletion ordering - if snap != nil && snap.Deleted() && (snap.ForegroundDeletion || resource.HasDeletionGroup()) { - failingOpen = false - } else { - logger.Info("FailOpen - suppressing errors") - err = nil - modified = false - } + logger.Info("FailOpen - suppressing errors") + err = nil + modified = false } if err != nil { logger.Error(err, "resource reconciliation failed") diff --git a/internal/resource/cache.go b/internal/resource/cache.go index e8e39ea8..81138d58 100644 --- a/internal/resource/cache.go +++ b/internal/resource/cache.go @@ -118,12 +118,6 @@ func (c *Cache) Fill(ctx context.Context, comp *apiv1.Composition, synUUID strin continue } if !matches { - logger.Info("adding shadow resource to cache", - "resourceKind", res.Ref.Kind, - "resourceName", res.Ref.Name, - "resourceNamespace", res.Ref.Namespace, - "compositionDeleted", res.compositionDeleted) - builder.AddShadow(res) continue } } @@ -139,13 +133,7 @@ func (c *Cache) Fill(ctx context.Context, comp *apiv1.Composition, synUUID strin c.syntheses[synUUID] = tree c.synByComp[compNSN] = append(c.synByComp[compNSN], synUUID) c.mut.Unlock() - shadowCount := 0 - for _, idx := range tree.byRef { - if idx.Shadow { - shadowCount++ - } - } - logger.Info("resource cache filled", "synthesisUUID", synUUID, "totalResources", len(tree.byRef), "shadowResources", shadowCount, "realResources", len(tree.byRef)-shadowCount) + logger.Info("resource cache filled", "synthesisUUID", synUUID) } // Purge removes all syntheses from the cache that are not part of the given composition. diff --git a/internal/resource/cache_test.go b/internal/resource/cache_test.go index 55df8e6d..7526ffd8 100644 --- a/internal/resource/cache_test.go +++ b/internal/resource/cache_test.go @@ -424,208 +424,3 @@ func dumpQueue(q workqueue.TypedRateLimitingInterface[Request]) (slice []string) q.Forget(req) } } - -func TestCacheShadowResourceFilterAddsShadows(t *testing.T) { - // Filtered-out resources should be added as shadows (not found via Get, but present in tree for ordering) - ctx := context.Background() - var c Cache - queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedControllerRateLimiter[Request]()) - c.SetQueue(queue) - - comp := &apiv1.Composition{} - comp.Name = "test-comp" - comp.Namespace = "test-ns" - - slices := []apiv1.ResourceSlice{{ - ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, - Spec: apiv1.ResourceSliceSpec{ - Resources: []apiv1.Manifest{ - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "real-resource", "namespace": "default", "labels": {"env": "prod"}}}`}, - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "shadow-resource", "namespace": "default", "labels": {"env": "dev"}}}`}, - }, - }, - }} - - filter, err := enocel.Parse("self.metadata.labels.env == 'prod'") - require.NoError(t, err) - c.ResourceFilter = filter - - const synUUID = "test-syn" - c.Fill(ctx, comp, synUUID, slices) - - // Real resource is found - res, visible, found := c.Get(ctx, synUUID, Ref{Name: "real-resource", Namespace: "default", Kind: "ConfigMap"}) - assert.NotNil(t, res) - assert.True(t, visible) - assert.True(t, found) - - // Shadow resource is NOT found via Get - res, visible, found = c.Get(ctx, synUUID, Ref{Name: "shadow-resource", Namespace: "default", Kind: "ConfigMap"}) - assert.Nil(t, res) - assert.False(t, visible) - assert.False(t, found) - - // Visit still processes shadow resources (no panic, returns true) - assert.True(t, c.Visit(ctx, comp, synUUID, slices)) - - // Only the real resource should be enqueued on first Visit - requests := dumpQueue(queue) - assert.ElementsMatch(t, []string{"(.ConfigMap)/default/real-resource"}, requests) -} - -func TestCacheShadowDeletionGroupOrdering(t *testing.T) { - // End-to-end cross-reconciler deletion ordering via cache: - // Shadow CRD at deletion-group -1, real Deployment at deletion-group 0 - // Deployment should only become visible after CRD's Deleted status is written by Visit. - ctx := context.Background() - var c Cache - queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedControllerRateLimiter[Request]()) - c.SetQueue(queue) - - comp := &apiv1.Composition{} - comp.Name = "test-comp" - comp.Namespace = "test-ns" - now := metav1.Now() - comp.DeletionTimestamp = &now - - slices := []apiv1.ResourceSlice{{ - ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, - Spec: apiv1.ResourceSliceSpec{ - Resources: []apiv1.Manifest{ - // index 0: CRD with deletion-group -1 and addon label (will be filtered out -> shadow) - {Manifest: `{"apiVersion": "apiextensions.k8s.io/v1", "kind": "CustomResourceDefinition", "metadata": {"name": "test-crd", "namespace": "default", "annotations": {"eno.azure.io/deletion-group": "-1"}, "labels": {"eno.azure.io/overlaymgr-component-type": "addon"}}}`}, - // index 1: Deployment with deletion-group 0 and ccp label (will be kept -> real) - {Manifest: `{"apiVersion": "apps/v1", "kind": "Deployment", "metadata": {"name": "test-deploy", "namespace": "default", "annotations": {"eno.azure.io/deletion-group": "0"}, "labels": {"eno.azure.io/overlaymgr-component-type": "ccp"}}}`}, - }, - }, - }} - - // Filter: only resources with ccp label (Deployment passes, CRD is shadow) - filter, err := enocel.Parse("has(self.metadata.labels) && 'eno.azure.io/overlaymgr-component-type' in self.metadata.labels && self.metadata.labels['eno.azure.io/overlaymgr-component-type'] == 'ccp'") - require.NoError(t, err) - c.ResourceFilter = filter - - const synUUID = "test-syn" - c.Fill(ctx, comp, synUUID, slices) - - // Deployment should be found but NOT visible (blocked by shadow CRD) - res, visible, found := c.Get(ctx, synUUID, Ref{Name: "test-deploy", Namespace: "default", Kind: "Deployment", Group: "apps"}) - assert.NotNil(t, res) - assert.False(t, visible, "Deployment should be blocked behind shadow CRD") - assert.True(t, found) - - // CRD is a shadow, not found via Get - res, visible, found = c.Get(ctx, synUUID, Ref{Name: "test-crd", Namespace: "default", Kind: "CustomResourceDefinition", Group: "apiextensions.k8s.io"}) - assert.Nil(t, res) - assert.False(t, visible) - assert.False(t, found) - - // Visit with no status changes - c.Visit(ctx, comp, synUUID, slices) - dumpQueue(queue) // drain first-visit enqueue - - // Simulate the other reconciler marking the CRD as deleted in the slice status - slices[0].Status.Resources = []apiv1.ResourceState{ - {Deleted: true}, // CRD status - {}, // Deployment status - } - c.Visit(ctx, comp, synUUID, slices) - - // The Deployment should be enqueued (unblocked by shadow CRD deletion) - requests := dumpQueue(queue) - assert.Contains(t, requests, "(apps.Deployment)/default/test-deploy", "Deployment should be enqueued after shadow CRD deletion") - - // Deployment should now be visible - res, visible, found = c.Get(ctx, synUUID, Ref{Name: "test-deploy", Namespace: "default", Kind: "Deployment", Group: "apps"}) - assert.NotNil(t, res) - assert.True(t, visible, "Deployment should be visible after shadow CRD is deleted") - assert.True(t, found) -} - -func TestCacheShadowVisitUpdatesState(t *testing.T) { - // Verify that Visit processes shadow resources and updates their state in the tree - ctx := context.Background() - var c Cache - queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedControllerRateLimiter[Request]()) - c.SetQueue(queue) - - comp := &apiv1.Composition{} - comp.Name = "test-comp" - comp.Namespace = "test-ns" - - slices := []apiv1.ResourceSlice{{ - ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, - Spec: apiv1.ResourceSliceSpec{ - Resources: []apiv1.Manifest{ - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "shadow-cm", "namespace": "default", "labels": {"env": "dev"}}}`}, - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "real-cm", "namespace": "default", "labels": {"env": "prod"}}}`}, - }, - }, - }} - - filter, err := enocel.Parse("self.metadata.labels.env == 'prod'") - require.NoError(t, err) - c.ResourceFilter = filter - - const synUUID = "test-syn" - c.Fill(ctx, comp, synUUID, slices) - - // First visit to establish baseline - c.Visit(ctx, comp, synUUID, slices) - dumpQueue(queue) - - // Visit with updated status for shadow — should not panic and should not enqueue shadow - slices[0].Status.Resources = []apiv1.ResourceState{ - {Ready: &metav1.Time{}, Reconciled: true}, // shadow - {Ready: &metav1.Time{}, Reconciled: true}, // real - } - c.Visit(ctx, comp, synUUID, slices) - requests := dumpQueue(queue) - - // Only the real resource should be enqueued (due to its status change) - assert.Contains(t, requests, "(.ConfigMap)/default/real-cm") - // Shadow should NOT appear (not self-enqueued) - for _, r := range requests { - assert.NotEqual(t, "(.ConfigMap)/default/shadow-cm", r, "shadow should not self-enqueue") - } -} - -func TestCacheShadowPurge(t *testing.T) { - // Shadows should be purged alongside real resources - ctx := context.Background() - var c Cache - queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedControllerRateLimiter[Request]()) - c.SetQueue(queue) - - comp := &apiv1.Composition{} - comp.Name = "test-comp" - comp.Namespace = "test-ns" - compNSN := types.NamespacedName{Name: comp.Name, Namespace: comp.Namespace} - - slices := []apiv1.ResourceSlice{{ - ObjectMeta: metav1.ObjectMeta{Name: "slice-1"}, - Spec: apiv1.ResourceSliceSpec{ - Resources: []apiv1.Manifest{ - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "shadow-cm", "namespace": "default", "labels": {"env": "dev"}}}`}, - {Manifest: `{"apiVersion": "v1", "kind": "ConfigMap", "metadata": {"name": "real-cm", "namespace": "default", "labels": {"env": "prod"}}}`}, - }, - }, - }} - - filter, err := enocel.Parse("self.metadata.labels.env == 'prod'") - require.NoError(t, err) - c.ResourceFilter = filter - - const synUUID = "test-syn" - c.Fill(ctx, comp, synUUID, slices) - - // Verify cache is populated - assert.True(t, c.Visit(ctx, comp, synUUID, slices)) - dumpQueue(queue) - - // Purge everything - c.Purge(ctx, compNSN, nil) - - // Cache should be empty now (Visit returns false) - assert.False(t, c.Visit(ctx, comp, synUUID, slices)) -} diff --git a/internal/resource/fixtures/tree-builder-both-crd-and-cr-and-readiness-groups.json b/internal/resource/fixtures/tree-builder-both-crd-and-cr-and-readiness-groups.json index 4d185f0f..77733a50 100644 --- a/internal/resource/fixtures/tree-builder-both-crd-and-cr-and-readiness-groups.json +++ b/internal/resource/fixtures/tree-builder-both-crd-and-cr-and-readiness-groups.json @@ -5,8 +5,7 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/not-a-crd": { "dependencies": [], @@ -14,8 +13,7 @@ "(test.group.TestKind)/default/test-crd" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-cr": { "dependencies": [ @@ -25,8 +23,7 @@ "(test.group.TestKind)/default/also-not-a-crd" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-crd": { "dependencies": [ @@ -36,7 +33,6 @@ "(test.group.TestKind)/default/test-cr" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-both-crd-and-cr-conflict.json b/internal/resource/fixtures/tree-builder-both-crd-and-cr-conflict.json index 5134fc43..cceea996 100644 --- a/internal/resource/fixtures/tree-builder-both-crd-and-cr-conflict.json +++ b/internal/resource/fixtures/tree-builder-both-crd-and-cr-conflict.json @@ -7,8 +7,7 @@ "(test.group.TestKind)/default/test-crd" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-crd": { "dependencies": [ @@ -18,7 +17,6 @@ "(test.group.TestKind)/default/test-cr" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-crd-and-cr-during-deletion.json b/internal/resource/fixtures/tree-builder-crd-and-cr-during-deletion.json index 88128885..73a8829f 100644 --- a/internal/resource/fixtures/tree-builder-crd-and-cr-during-deletion.json +++ b/internal/resource/fixtures/tree-builder-crd-and-cr-during-deletion.json @@ -5,8 +5,7 @@ "(test.group.TestKind)/default/test-crd" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-crd": { "dependencies": [ @@ -14,7 +13,6 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-crd-and-cr.json b/internal/resource/fixtures/tree-builder-crd-and-cr.json index 1da3701a..778c18fe 100644 --- a/internal/resource/fixtures/tree-builder-crd-and-cr.json +++ b/internal/resource/fixtures/tree-builder-crd-and-cr.json @@ -5,8 +5,7 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-crd": { "dependencies": [], @@ -14,7 +13,6 @@ "(test.group.TestKind)/default/test-cr" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-deletion-groups.json b/internal/resource/fixtures/tree-builder-deletion-groups.json index 025664e9..d340c1ce 100644 --- a/internal/resource/fixtures/tree-builder-deletion-groups.json +++ b/internal/resource/fixtures/tree-builder-deletion-groups.json @@ -5,8 +5,7 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/low-deletion-group": { "dependencies": [], @@ -14,21 +13,18 @@ "(test.group.TestKind)/default/high-deletion-group" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/no-deletion-group": { "dependencies": [], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/no-deletion-or-readiness-group": { "dependencies": [], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-several-overlapping-groups.json b/internal/resource/fixtures/tree-builder-several-overlapping-groups.json index dd1db96f..4fd7f32a 100644 --- a/internal/resource/fixtures/tree-builder-several-overlapping-groups.json +++ b/internal/resource/fixtures/tree-builder-several-overlapping-groups.json @@ -6,8 +6,7 @@ "(test.group.TestKind)/default/test-2-b" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-2-a": { "dependencies": [ @@ -15,8 +14,7 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-2-b": { "dependencies": [ @@ -24,7 +22,6 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-several-readiness-groups.json b/internal/resource/fixtures/tree-builder-several-readiness-groups.json index ae65b585..24edbf37 100644 --- a/internal/resource/fixtures/tree-builder-several-readiness-groups.json +++ b/internal/resource/fixtures/tree-builder-several-readiness-groups.json @@ -7,8 +7,7 @@ "(test.group.TestKind)/default/test-1" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-1": { "dependencies": [ @@ -18,8 +17,7 @@ "(test.group.TestKind)/default/test-4" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-4": { "dependencies": [ @@ -27,8 +25,7 @@ ], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false }, "(test.group.TestKind)/default/test-negative-2": { "dependencies": [], @@ -36,7 +33,6 @@ "(test.group.TestKind)/default/test-0" ], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/fixtures/tree-builder-single-basic-resource.json b/internal/resource/fixtures/tree-builder-single-basic-resource.json index a982a5d6..b3b8b164 100644 --- a/internal/resource/fixtures/tree-builder-single-basic-resource.json +++ b/internal/resource/fixtures/tree-builder-single-basic-resource.json @@ -3,7 +3,6 @@ "dependencies": [], "dependents": [], "ready": false, - "reconciled": false, - "shadow": false + "reconciled": false } } \ No newline at end of file diff --git a/internal/resource/resource.go b/internal/resource/resource.go index a28c9ab5..2c664ba2 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -247,8 +247,6 @@ func newResource(ctx context.Context, parsed *unstructured.Unstructured, strict func (r *Resource) State() *apiv1.ResourceState { return r.latestKnownState.Load() } -func (r *Resource) HasDeletionGroup() bool { return r.deletionGroup != nil } - // Less returns true when r < than. // Used to establish determinstic ordering for conflicting resources. func (r *Resource) Less(than *Resource) bool { diff --git a/internal/resource/tree.go b/internal/resource/tree.go index 43f29e20..c5d78170 100644 --- a/internal/resource/tree.go +++ b/internal/resource/tree.go @@ -12,7 +12,6 @@ import ( type indexedResource struct { Resource *Resource Seen bool - Shadow bool // tracked for cross reconciler ordering but not reconciled PendingDependencies map[Ref]struct{} Dependents map[Ref]*indexedResource } @@ -56,14 +55,6 @@ func (b *treeBuilder) init() { } func (b *treeBuilder) Add(resource *Resource) { - b.add(resource, false) -} - -func (b *treeBuilder) AddShadow(resource *Resource) { - b.add(resource, true) -} - -func (b *treeBuilder) add(resource *Resource, shadow bool) { b.init() // Handle conflicting refs deterministically @@ -74,7 +65,6 @@ func (b *treeBuilder) add(resource *Resource, shadow bool) { // Index the resource into the builder idx := &indexedResource{ Resource: resource, - Shadow: shadow, PendingDependencies: map[Ref]struct{}{}, Dependents: map[Ref]*indexedResource{}, } @@ -149,9 +139,6 @@ func (t *tree) Get(key Ref) (res *Resource, visible bool, found bool) { if !ok { return nil, false, false } - if idx.Shadow { - return nil, false, false - } //TODO: debug logging on what we're blocked on might help future issues. return idx.Resource, (!idx.Backtracks() && len(idx.PendingDependencies) == 0), true } @@ -165,10 +152,8 @@ func (t *tree) UpdateState(ref ManifestRef, state *apiv1.ResourceState, enqueue // Requeue self when the state has changed lastKnown := idx.Resource.latestKnownState.Swap(state) - if !idx.Shadow { - if (!idx.Seen && lastKnown == nil) || !lastKnown.Equal(state) { - enqueue(idx.Resource.Ref) - } + if (!idx.Seen && lastKnown == nil) || !lastKnown.Equal(state) { + enqueue(idx.Resource.Ref) } idx.Seen = true @@ -204,7 +189,6 @@ func (t *tree) MarshalJSON() ([]byte, error) { valMap := map[string]any{ "ready": state != nil && state.Ready != nil, "reconciled": state != nil && state.Reconciled, - "shadow": value.Shadow, "dependencies": dependencies, "dependents": dependents, } diff --git a/internal/resource/tree_test.go b/internal/resource/tree_test.go index 8c8c2c41..c54bbad9 100644 --- a/internal/resource/tree_test.go +++ b/internal/resource/tree_test.go @@ -343,293 +343,6 @@ func TestTreeRefConflicts(t *testing.T) { assert.True(t, visible) assert.Equal(t, "b", string(res.manifestHash)) } -func TestShadowGetReturnsNotFound(t *testing.T) { - // Shadow resources should not be returned by Get - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-resource"), - ManifestRef: ManifestRef{Index: 0}, - }) - b.Add(&Resource{ - Ref: newTestRef("real-resource"), - ManifestRef: ManifestRef{Index: 1}, - }) - tree := b.Build() - - // Shadow resource is not found - res, visible, found := tree.Get(newTestRef("shadow-resource")) - assert.Nil(t, res) - assert.False(t, visible) - assert.False(t, found) - - // Real resource is found and visible - res, visible, found = tree.Get(newTestRef("real-resource")) - assert.NotNil(t, res) - assert.True(t, visible) - assert.True(t, found) -} - -func TestShadowUpdateStateDoesNotEnqueueSelf(t *testing.T) { - // Shadow resources should NOT enqueue themselves on state change, but should still track state - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-resource"), - ManifestRef: ManifestRef{Index: 0}, - }) - tree := b.Build() - - var enqueued []string - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Ready: &metav1.Time{}}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.Empty(t, enqueued, "shadow resource should not enqueue itself") -} - -func TestShadowDeletionGroupUnblocksDependents(t *testing.T) { - // Core cross-reconciler scenario: - // Shadow resource at deletion-group -1 (e.g. CRD managed by other reconciler) - // Real resource at deletion-group 0 (e.g. Deployment managed by this reconciler) - // When the shadow's Deleted status transitions, the real resource should be unblocked. - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-crd"), - deletionGroup: ptr.To(-1), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 0}, - }) - b.Add(&Resource{ - Ref: newTestRef("real-deployment"), - deletionGroup: ptr.To(0), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 1}, - }) - tree := b.Build() - - // Real resource should be blocked (depends on shadow) - res, visible, found := tree.Get(newTestRef("real-deployment")) - assert.NotNil(t, res) - assert.False(t, visible, "real resource should be blocked by shadow dependency") - assert.True(t, found) - - // Shadow resource should not be found via Get - res, visible, found = tree.Get(newTestRef("shadow-crd")) - assert.Nil(t, res) - assert.False(t, visible) - assert.False(t, found) - - // Simulate the other reconciler deleting the shadow resource and marking state as Deleted - var enqueued []string - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Deleted: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - // Shadow should NOT enqueue itself, but SHOULD enqueue the dependent real resource - assert.ElementsMatch(t, []string{"real-deployment"}, enqueued) - - // Now the real resource should be visible (unblocked) - res, visible, found = tree.Get(newTestRef("real-deployment")) - assert.NotNil(t, res) - assert.True(t, visible, "real resource should be unblocked after shadow deletion") - assert.True(t, found) -} - -func TestShadowReadinessGroupUnblocksDependents(t *testing.T) { - // Readiness scenario (non-deletion): - // Shadow resource at readiness-group 0 - // Real resource at readiness-group 1 - // When the shadow becomes ready, the real resource should be unblocked. - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-infra"), - readinessGroup: 0, - ManifestRef: ManifestRef{Index: 0}, - }) - b.Add(&Resource{ - Ref: newTestRef("real-app"), - readinessGroup: 1, - ManifestRef: ManifestRef{Index: 1}, - }) - tree := b.Build() - - // Real resource should be blocked - _, visible, found := tree.Get(newTestRef("real-app")) - assert.False(t, visible, "should be blocked by shadow dependency") - assert.True(t, found) - - // Shadow becomes ready - var enqueued []string - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Ready: &metav1.Time{}}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - // Shadow should NOT enqueue itself, but SHOULD unblock and enqueue the real resource - assert.ElementsMatch(t, []string{"real-app"}, enqueued) - - // Now the real resource should be visible - _, visible, found = tree.Get(newTestRef("real-app")) - assert.True(t, visible, "should be unblocked after shadow ready") - assert.True(t, found) -} - -func TestShadowMultipleDeletionGroups(t *testing.T) { - // Multiple shadows at different deletion groups should chain correctly: - // shadow-a (group -2) -> shadow-b (group -1) -> real (group 0) - // real should only become visible after both shadows report Deleted. - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-a"), - deletionGroup: ptr.To(-2), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 0}, - }) - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-b"), - deletionGroup: ptr.To(-1), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 1}, - }) - b.Add(&Resource{ - Ref: newTestRef("real-resource"), - deletionGroup: ptr.To(0), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 2}, - }) - tree := b.Build() - - // Real resource blocked - _, visible, _ := tree.Get(newTestRef("real-resource")) - assert.False(t, visible) - - // Delete shadow-a - var enqueued []string - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Deleted: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.ElementsMatch(t, []string{"shadow-b"}, enqueued, "shadow-a deletion should unblock shadow-b") - - // Real resource still blocked (shadow-b not yet deleted) - _, visible, _ = tree.Get(newTestRef("real-resource")) - assert.False(t, visible, "real should still be blocked by shadow-b") - - // Delete shadow-b - enqueued = nil - tree.UpdateState(ManifestRef{Index: 1}, &apiv1.ResourceState{Deleted: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.ElementsMatch(t, []string{"real-resource"}, enqueued, "shadow-b deletion should unblock real resource") - - // Real resource now visible - _, visible, _ = tree.Get(newTestRef("real-resource")) - assert.True(t, visible) -} - -func TestShadowAndRealMixedDeletionGroups(t *testing.T) { - // Mix of shadow and real resources in the same deletion group chain: - // real-a (group 0) -> shadow-b (group 1) -> real-c (group 2) - var b treeBuilder - b.Add(&Resource{ - Ref: newTestRef("real-a"), - deletionGroup: ptr.To(0), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 0}, - }) - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-b"), - deletionGroup: ptr.To(1), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 1}, - }) - b.Add(&Resource{ - Ref: newTestRef("real-c"), - deletionGroup: ptr.To(2), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 2}, - }) - tree := b.Build() - - // real-a is visible (no deps), shadow-b and real-c are blocked - _, visible, found := tree.Get(newTestRef("real-a")) - assert.True(t, visible) - assert.True(t, found) - - _, visible, _ = tree.Get(newTestRef("real-c")) - assert.False(t, visible, "real-c should be blocked by shadow-b") - - // Delete real-a -> unblocks shadow-b - var enqueued []string - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Deleted: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.Contains(t, enqueued, "shadow-b") - - // real-c still blocked (shadow-b not deleted yet) - _, visible, _ = tree.Get(newTestRef("real-c")) - assert.False(t, visible) - - // Delete shadow-b -> unblocks real-c - enqueued = nil - tree.UpdateState(ManifestRef{Index: 1}, &apiv1.ResourceState{Deleted: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.ElementsMatch(t, []string{"real-c"}, enqueued) - - _, visible, _ = tree.Get(newTestRef("real-c")) - assert.True(t, visible, "real-c should be unblocked after shadow-b deletion") -} - -func TestShadowNoDeletionGroupIsIndependent(t *testing.T) { - // A shadow without a deletion group should not block anything during deletion - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-no-group"), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 0}, - }) - b.Add(&Resource{ - Ref: newTestRef("real-with-group"), - deletionGroup: ptr.To(0), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 1}, - }) - tree := b.Build() - - // Real resource has no dependency on the shadow (shadow has no group) - _, visible, found := tree.Get(newTestRef("real-with-group")) - assert.True(t, visible, "real resource should not be blocked by groupless shadow") - assert.True(t, found) -} - -func TestShadowRepeatedUpdateStateNoSelfEnqueue(t *testing.T) { - // Repeated state updates to a shadow should never enqueue the shadow itself - var b treeBuilder - b.AddShadow(&Resource{ - Ref: newTestRef("shadow-resource"), - deletionGroup: ptr.To(0), - compositionDeleted: true, - ManifestRef: ManifestRef{Index: 0}, - }) - tree := b.Build() - - // First update - var enqueued []string - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.Empty(t, enqueued, "shadow should never self-enqueue") - - // Second update with change - enqueued = nil - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Reconciled: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.Empty(t, enqueued, "shadow should never self-enqueue even on state change") - - // Third update with Deleted - enqueued = nil - tree.UpdateState(ManifestRef{Index: 0}, &apiv1.ResourceState{Deleted: true}, func(r Ref) { - enqueued = append(enqueued, r.Name) - }) - assert.Empty(t, enqueued, "shadow with no dependents should enqueue nothing") -} - func TestIndexedResourceBacktracks(t *testing.T) { baseGVK := schema.GroupVersionKind{Group: "test.group", Version: "v1", Kind: "TestKind"}