diff --git a/api/v1/composition.go b/api/v1/composition.go index c0f87d19..492d8ce3 100644 --- a/api/v1/composition.go +++ b/api/v1/composition.go @@ -9,13 +9,15 @@ import ( ) const ( - enoAzureOperationIDKey = "eno.azure.io/operationID" - enoAzureOperationOrigin = "eno.azure.io/operationOrigin" - OperationIdKey string = "operationID" - OperationOrigionKey string = "operationOrigin" - CircularDependencyReason string = "CircularDependency" - WaitingOnDependentsReason string = "WaitingOnDependents" - WaitingOnDependenciesReason string = "WaitingOnDependencies" + enoAzureOperationIDKey = "eno.azure.io/operationID" + enoAzureOperationOrigin = "eno.azure.io/operationOrigin" + OperationIdKey string = "operationID" + OperationOrigionKey string = "operationOrigin" + CircularDependencyReason string = "CircularDependency" + WaitingOnDependentsReason string = "WaitingOnDependents" + WaitingOnDependenciesReason string = "WaitingOnDependencies" + WaitingOnDependencyNotFoundReason string = "DependencyNotFound" + WaitingOnDependencyNotReadyReason string = "DependencyNotReady" ) // +kubebuilder:object:root=true diff --git a/internal/controllers/composition/controller.go b/internal/controllers/composition/controller.go index 52e83b58..2d39d865 100644 --- a/internal/controllers/composition/controller.go +++ b/internal/controllers/composition/controller.go @@ -323,6 +323,11 @@ func buildSimplifiedStatus(synth *apiv1.Synthesizer, comp *apiv1.Composition) *a } if comp.Status.CurrentSynthesis == nil && comp.Status.InFlightSynthesis == nil { + // This means that dependencies exists and no synthesis started showing WaitingOnDependencies + if len(comp.Spec.DependsOn) > 0 { + status.Status = apiv1.WaitingOnDependenciesReason + return status + } status.Status = "PendingSynthesis" return status } diff --git a/internal/controllers/composition/controller_test.go b/internal/controllers/composition/controller_test.go index 19c3f886..a4ee576b 100644 --- a/internal/controllers/composition/controller_test.go +++ b/internal/controllers/composition/controller_test.go @@ -214,6 +214,10 @@ func TestSimplifiedStatus(t *testing.T) { state.Comp.Status.Simplified = &apiv1.SimplifiedStatus{Error: "Previous reconciliation error"} return state }). + WithMutation("with dependencies", func(state *simplifiedStatusState) *simplifiedStatusState { + state.Comp.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "dep-a", Namespace: "default"}} + return state + }). WithInvariant("missing synth", func(state *simplifiedStatusState, result *apiv1.SimplifiedStatus) bool { return state.Comp.DeletionTimestamp != nil || state.Synth != nil || result.Status == "MissingSynthesizer" }). @@ -261,6 +265,13 @@ func TestSimplifiedStatus(t *testing.T) { state.Comp.Status.Simplified.Error == "" || result.Error == state.Comp.Status.Simplified.Error }). + WithInvariant("waiting on dependencies", func(state *simplifiedStatusState, result *apiv1.SimplifiedStatus) bool { + hasDeps := len(state.Comp.Spec.DependsOn) > 0 + noSynthesis := state.Comp.Status.CurrentSynthesis == nil && state.Comp.Status.InFlightSynthesis == nil + notDeleting := state.Comp.DeletionTimestamp == nil + hasSynth := state.Synth != nil + return !(hasDeps && noSynthesis && notDeleting && hasSynth) || result.Status == apiv1.WaitingOnDependenciesReason + }). Evaluate(t) } @@ -300,7 +311,6 @@ func TestIsAddonComposition(t *testing.T) { labels: map[string]string{AKSComponentLabel: addOnLabelValue}, expected: true, }, - } for _, tt := range tests { diff --git a/internal/controllers/scheduling/controller.go b/internal/controllers/scheduling/controller.go index c58dc61c..61c1175e 100644 --- a/internal/controllers/scheduling/controller.go +++ b/internal/controllers/scheduling/controller.go @@ -6,10 +6,12 @@ import ( "encoding/json" "fmt" "hash/fnv" + "path" "sort" "time" "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -114,9 +116,42 @@ func (c *controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // Reset the compositionHealth metric before iterating through compositions compositionHealth.Reset() + // Build readiness index and composition map for dependency checking + readySet := buildReadySet(comps) + existSet := buildExistsSet(comps) + sortedComps, cyclicSet := topoSortCompositions(comps.Items) + + // Pre-loop: set/clear circular dependency status on all compositions. + // Cyclic compositions are excluded from sortedComps by Kahn's algorithm, + // so we must handle them here before the main loop. + for _, comp := range comps.Items { + comp := comp + if comp.DeletionTimestamp != nil || len(comp.Spec.DependsOn) == 0 { + continue + } + compKey := path.Join(comp.GetNamespace(), comp.GetName()) + if cyclicSet[compKey] { + logger.Error(fmt.Errorf("circular dependency detected"), + "skipping composition synthesis", + "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace()) + if _, err := c.setDependencyStatus(ctx, &comp, &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.CircularDependencyReason, + }); err != nil { + logger.Error(err, "failed to update circular dependency status") + return ctrl.Result{}, err + } + } else if cleared, err := c.clearDependencyStatus(ctx, &comp, apiv1.CircularDependencyReason); err != nil { + logger.Error(err, "failed to clear circular dependency status") + return ctrl.Result{}, err + } else if cleared { + logger.Info("cleared stale circular dependency status", "compositionName", comp.GetName()) + } + } + var inFlight int var op *op - for _, comp := range comps.Items { + for _, comp := range sortedComps { comp := comp if comp.Synthesizing() { inFlight++ @@ -131,6 +166,46 @@ func (c *controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu compositionHealth.WithLabelValues(comp.Name, comp.Namespace, comp.Spec.Synthesizer.Name).Set(0) } + if comp.DeletionTimestamp == nil && len(comp.Spec.DependsOn) > 0 { + if !areDependenciesReady(&comp, readySet) { + // Build BlockedBy list + var blockedBy []apiv1.BlockedByRef + for _, dep := range comp.Spec.DependsOn { + key := path.Join(dep.Namespace, dep.Name) + if !readySet[key] { + reason := apiv1.WaitingOnDependencyNotReadyReason + if !existSet[key] { + reason = apiv1.WaitingOnDependencyNotFoundReason + } + logger.Info("dependency not satisfied for composition", "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace(), + "dependencyName", dep.Name, "dependencyNamespace", dep.Namespace, "reason", reason) + blockedBy = append(blockedBy, apiv1.BlockedByRef{ + Name: dep.Name, + Namespace: dep.Namespace, + Reason: reason, + }) + } + } + if _, err := c.setDependencyStatus(ctx, &comp, &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.WaitingOnDependenciesReason, + BlockedBy: blockedBy, + }); err != nil { + return ctrl.Result{}, err + } + logger.Info("not all dependent compositions are ready, skipping composition synthesis", "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace()) + continue + } + + // clear the waitingonDependencies status + if cleared, err := c.clearDependencyStatus(ctx, &comp, apiv1.WaitingOnDependenciesReason); err != nil { + logger.Error(err, "failed to clear dependency status") + return ctrl.Result{}, err + } else if cleared { + logger.Info("cleared stale waiting on dependency status", "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace()) + } + } + synth, ok := synthsByName[comp.Spec.Synthesizer.Name] if !ok { continue @@ -242,3 +317,39 @@ func getSynthOwner(synth *apiv1.Synthesizer) string { } return synth.GetAnnotations()["eno.azure.io/owner"] } + +// setDependencyStatus patches the composition's DependencyStatus if it doesn't already +// match the given reason. Returns true if a patch was applied +func (c *controller) setDependencyStatus(ctx context.Context, comp *apiv1.Composition, newStatus *apiv1.DependencyStatus) (bool, error) { + logger := logr.FromContextOrDiscard(ctx) + // same status, we can ignore + if comp.Status.DependencyStatus != nil && equality.Semantic.DeepEqual(*comp.Status.DependencyStatus, *newStatus) { + return false, nil // modified=false, err = nil + } + + copy := comp.DeepCopy() + copy.Status.DependencyStatus = newStatus + logger.Info("Setting DependencyStatus for composition", "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace(), + "DependencyStatusReason", newStatus.Reason, "BlockedBy", newStatus.BlockedBy) + if err := c.client.Status().Patch(ctx, copy, client.MergeFrom(comp)); err != nil { + return false, err + } + return true, nil +} + +// clearDependencyStatus clears the composition's DependencyStatus if it currently has the given reason. +// Returns true if a patch was applied +func (c *controller) clearDependencyStatus(ctx context.Context, comp *apiv1.Composition, reason string) (bool, error) { + logger := logr.FromContextOrDiscard(ctx) + if comp.Status.DependencyStatus == nil || comp.Status.DependencyStatus.Reason != reason { + return false, nil + } + copy := comp.DeepCopy() + copy.Status.DependencyStatus = nil + logger.Info("clearing dependency status", "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace(), + "prevDependencyReason", comp.Status.DependencyStatus.Reason) + if err := c.client.Status().Patch(ctx, copy, client.MergeFrom(comp)); err != nil { + return false, err + } + return true, nil +} diff --git a/internal/controllers/scheduling/controller_test.go b/internal/controllers/scheduling/controller_test.go index 8eae71b7..f41bec62 100644 --- a/internal/controllers/scheduling/controller_test.go +++ b/internal/controllers/scheduling/controller_test.go @@ -767,3 +767,622 @@ func TestCompositionHealthMetrics(t *testing.T) { stuckValue := prometheustestutil.ToFloat64(compositionHealth.WithLabelValues("stuck-comp", "default", "test-synth")) assert.Equal(t, float64(1), stuckValue, "stuck composition should have health value 1") } + +// TestDependencyBlocksSynthesis proves that a composition with an unmet required dependency +// is not dispatched for synthesis. The composition is the only one eligible, so if it's +// blocked, no op is created and Reconcile returns nil. +func TestDependencyBlocksSynthesis(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // comp-b depends on "comp-a" which doesn't exist — dependency not in readySet + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + // Reconcile returns nil — comp-b was blocked, no dispatch attempted + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) +} + +// TestDependencyAllowsSynthesisWhenReady proves that a composition with all required +// dependencies ready passes the dependency check and is selected for dispatch. +func TestDependencyAllowsSynthesisWhenReady(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // comp-a is fully synthesized and ready — in readySet but produces no new op + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + require.NoError(t, cli.Create(ctx, compA)) + + compA.Status.CurrentSynthesis = &apiv1.Synthesis{ + UUID: "uuid-a", + ObservedCompositionGeneration: compA.Generation, + ObservedSynthesizerGeneration: synth.Generation, + Synthesized: ptr.To(metav1.Now()), + Reconciled: ptr.To(metav1.Now()), + Ready: ptr.To(metav1.Now()), + } + require.NoError(t, cli.Status().Update(ctx, compA)) + + // comp-b depends on comp-a (ready), needs initial synthesis + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + // Reconcile returns an error from dispatchOp (fake client JSON patch limitation). + // This proves comp-b PASSED the dependency check and was selected for dispatch. + _, err := c.Reconcile(ctx, ctrl.Request{}) + assert.Error(t, err, "expected dispatch error — proves comp-b passed dep check") +} + +// TestCyclicDependencyBlocksSynthesis proves that compositions involved in a dependency +// cycle are skipped and not dispatched for synthesis. +func TestCyclicDependencyBlocksSynthesis(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // A depends on B, B depends on A — mutual cycle + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + compA.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-b", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compA)) + + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + // Both compositions are blocked by cycle detection — no op, nil error + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) +} + +// TestDependencyCheckSkippedForDeletingComposition proves that dependency checks are +// bypassed for compositions that are being deleted (DeletionTimestamp is set). +func TestDependencyCheckSkippedForDeletingComposition(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // comp-a has deps on a non-existent composition but is being deleted + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + compA.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "nonexistent", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compA)) + + // Mark as deleting + require.NoError(t, cli.Delete(ctx, compA)) + + // Reconcile completes without error — the deleted composition bypasses the + // dependency guard (DeletionTimestamp != nil check) and is also excluded from + // synthesis by classifyOp, so no dispatch is attempted. + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) +} + +// TestNoDependenciesCompositionUnaffected proves that compositions without any dependsOn +// field are completely unaffected by the dependency checking logic and proceed to dispatch. +func TestNoDependenciesCompositionUnaffected(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + comp := &apiv1.Composition{} + comp.Name = "comp-no-deps" + comp.Namespace = "default" + comp.Finalizers = []string{"eno.azure.io/cleanup"} + comp.Spec.Synthesizer.Name = synth.Name + require.NoError(t, cli.Create(ctx, comp)) + + // Reconcile returns an error from dispatchOp (fake client JSON patch limitation). + // This proves the composition was not blocked by any dependency check. + _, err := c.Reconcile(ctx, ctrl.Request{}) + assert.Error(t, err, "expected dispatch error — proves comp without deps is eligible") +} + +// TestChainedDependencyBlocking proves that in a chain A → B → C, C is blocked +// when B is not ready, even though A is ready. +func TestChainedDependencyBlocking(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // A is ready and fully synthesized (no new op) + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + require.NoError(t, cli.Create(ctx, compA)) + + compA.Status.CurrentSynthesis = &apiv1.Synthesis{ + UUID: "uuid-a", + ObservedCompositionGeneration: compA.Generation, + ObservedSynthesizerGeneration: synth.Generation, + Synthesized: ptr.To(metav1.Now()), + Reconciled: ptr.To(metav1.Now()), + Ready: ptr.To(metav1.Now()), + } + require.NoError(t, cli.Status().Update(ctx, compA)) + + // B depends on A (ready) — passes dep check but NOT ready itself + // B is fully synthesized and has matching generation, so no new op is produced. + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + compB.Status.CurrentSynthesis = &apiv1.Synthesis{ + UUID: "uuid-b", + ObservedCompositionGeneration: compB.Generation, + ObservedSynthesizerGeneration: synth.Generation, + Synthesized: ptr.To(metav1.Now()), + Reconciled: ptr.To(metav1.Now()), + // Ready is NOT set — B is synthesized but not ready + } + require.NoError(t, cli.Status().Update(ctx, compB)) + + // C depends on B (not ready) — should be blocked + compC := &apiv1.Composition{} + compC.Name = "comp-c" + compC.Namespace = "default" + compC.Finalizers = []string{"eno.azure.io/cleanup"} + compC.Spec.Synthesizer.Name = synth.Name + compC.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-b", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compC)) + + // No eligible ops: A and B produce no ops (fully synthesized), C is blocked. + // Reconcile returns nil. + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) +} + +// TestCircularDependencyStatusSet proves that when a circular dependency is detected, +// the DependencyStatus is set with CircularDependencyReason on the involved compositions. +func TestCircularDependencyStatusSet(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // A depends on B, B depends on A — mutual cycle + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + compA.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-b", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compA)) + + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) + + // Verify both compositions have circular dependency status set + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compA), compA)) + require.NotNil(t, compA.Status.DependencyStatus) + assert.True(t, compA.Status.DependencyStatus.Blocked) + assert.Equal(t, apiv1.CircularDependencyReason, compA.Status.DependencyStatus.Reason) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compB), compB)) + require.NotNil(t, compB.Status.DependencyStatus) + assert.True(t, compB.Status.DependencyStatus.Blocked) + assert.Equal(t, apiv1.CircularDependencyReason, compB.Status.DependencyStatus.Reason) +} + +// TestWaitingOnDependenciesStatusSet proves that when a composition has unmet dependencies, +// the DependencyStatus is set with WaitingOnDependenciesReason and proper BlockedBy entries. +func TestWaitingOnDependenciesStatusSet(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // comp-a exists but is NOT ready (no Ready timestamp) + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + require.NoError(t, cli.Create(ctx, compA)) + + compA.Status.CurrentSynthesis = &apiv1.Synthesis{ + UUID: "uuid-a", + ObservedCompositionGeneration: compA.Generation, + ObservedSynthesizerGeneration: synth.Generation, + Synthesized: ptr.To(metav1.Now()), + Reconciled: ptr.To(metav1.Now()), + // Ready is NOT set + } + require.NoError(t, cli.Status().Update(ctx, compA)) + + // comp-b depends on comp-a (not ready) + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) + + // Verify comp-b has WaitingOnDependencies status with BlockedBy + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compB), compB)) + require.NotNil(t, compB.Status.DependencyStatus) + assert.True(t, compB.Status.DependencyStatus.Blocked) + assert.Equal(t, apiv1.WaitingOnDependenciesReason, compB.Status.DependencyStatus.Reason) + require.Len(t, compB.Status.DependencyStatus.BlockedBy, 1) + assert.Equal(t, "comp-a", compB.Status.DependencyStatus.BlockedBy[0].Name) + assert.Equal(t, "default", compB.Status.DependencyStatus.BlockedBy[0].Namespace) + assert.Equal(t, apiv1.WaitingOnDependencyNotReadyReason, compB.Status.DependencyStatus.BlockedBy[0].Reason) +} + +// TestWaitingOnDependenciesStatusCleared proves that when dependencies become ready, +// the WaitingOnDependencies status is cleared from the composition. +func TestWaitingOnDependenciesStatusCleared(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // comp-a exists but is NOT ready initially + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + compA.Finalizers = []string{"eno.azure.io/cleanup"} + compA.Spec.Synthesizer.Name = synth.Name + require.NoError(t, cli.Create(ctx, compA)) + + compA.Status.CurrentSynthesis = &apiv1.Synthesis{ + UUID: "uuid-a", + ObservedCompositionGeneration: compA.Generation, + ObservedSynthesizerGeneration: synth.Generation, + Synthesized: ptr.To(metav1.Now()), + Reconciled: ptr.To(metav1.Now()), + // Ready is NOT set + } + require.NoError(t, cli.Status().Update(ctx, compA)) + + // comp-b depends on comp-a + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + // First reconcile: comp-b gets WaitingOnDependencies status + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compB), compB)) + require.NotNil(t, compB.Status.DependencyStatus) + assert.Equal(t, apiv1.WaitingOnDependenciesReason, compB.Status.DependencyStatus.Reason) + + // Now make comp-a ready + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compA), compA)) + compA.Status.CurrentSynthesis.Ready = ptr.To(metav1.Now()) + require.NoError(t, cli.Status().Update(ctx, compA)) + + // Reconcile again — comp-b's dependency is now satisfied, status should be cleared + _, err = c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compB), compB)) + assert.Nil(t, compB.Status.DependencyStatus, "waiting on dependencies status should be cleared when deps become ready") +} + +func TestSetDependencyStatus(t *testing.T) { + ctx := testutil.NewContext(t) + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + + comp := &apiv1.Composition{} + comp.Name = "comp-a" + comp.Namespace = "default" + comp.Finalizers = []string{"eno.azure.io/cleanup"} + comp.Spec.Synthesizer.Name = synth.Name + + t.Run("sets status when none exists", func(t *testing.T) { + cli := testutil.NewClient(t, comp.DeepCopy(), synth.DeepCopy()) + c := &controller{client: cli} + + fresh := comp.DeepCopy() + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(fresh), fresh)) + + patched, err := c.setDependencyStatus(ctx, fresh, &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.CircularDependencyReason, + }) + require.NoError(t, err) + assert.True(t, patched) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(fresh), fresh)) + require.NotNil(t, fresh.Status.DependencyStatus) + assert.Equal(t, apiv1.CircularDependencyReason, fresh.Status.DependencyStatus.Reason) + assert.True(t, fresh.Status.DependencyStatus.Blocked) + }) + + t.Run("skips patch when status already matches", func(t *testing.T) { + existing := comp.DeepCopy() + cli := testutil.NewClient(t, existing, synth.DeepCopy()) + c := &controller{client: cli} + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + existing.Status.DependencyStatus = &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.CircularDependencyReason, + } + require.NoError(t, cli.Status().Update(ctx, existing)) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + patched, err := c.setDependencyStatus(ctx, existing, &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.CircularDependencyReason, + }) + require.NoError(t, err) + assert.False(t, patched) + }) + + t.Run("updates when reason differs", func(t *testing.T) { + existing := comp.DeepCopy() + cli := testutil.NewClient(t, existing, synth.DeepCopy()) + c := &controller{client: cli} + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + existing.Status.DependencyStatus = &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.CircularDependencyReason, + } + require.NoError(t, cli.Status().Update(ctx, existing)) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + patched, err := c.setDependencyStatus(ctx, existing, &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.WaitingOnDependenciesReason, + }) + require.NoError(t, err) + assert.True(t, patched) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + assert.Equal(t, apiv1.WaitingOnDependenciesReason, existing.Status.DependencyStatus.Reason) + }) + + t.Run("updates when blockedBy changes", func(t *testing.T) { + existing := comp.DeepCopy() + cli := testutil.NewClient(t, existing, synth.DeepCopy()) + c := &controller{client: cli} + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + existing.Status.DependencyStatus = &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.WaitingOnDependenciesReason, + BlockedBy: []apiv1.BlockedByRef{ + {Name: "dep-a", Namespace: "default", Reason: apiv1.WaitingOnDependencyNotReadyReason}, + }, + } + require.NoError(t, cli.Status().Update(ctx, existing)) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + patched, err := c.setDependencyStatus(ctx, existing, &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.WaitingOnDependenciesReason, + BlockedBy: []apiv1.BlockedByRef{ + {Name: "dep-b", Namespace: "default", Reason: apiv1.WaitingOnDependencyNotFoundReason}, + }, + }) + require.NoError(t, err) + assert.True(t, patched) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + require.Len(t, existing.Status.DependencyStatus.BlockedBy, 1) + assert.Equal(t, "dep-b", existing.Status.DependencyStatus.BlockedBy[0].Name) + assert.Equal(t, apiv1.WaitingOnDependencyNotFoundReason, existing.Status.DependencyStatus.BlockedBy[0].Reason) + }) +} + +func TestClearDependencyStatus(t *testing.T) { + ctx := testutil.NewContext(t) + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + + comp := &apiv1.Composition{} + comp.Name = "comp-a" + comp.Namespace = "default" + comp.Finalizers = []string{"eno.azure.io/cleanup"} + comp.Spec.Synthesizer.Name = synth.Name + + t.Run("clears matching reason", func(t *testing.T) { + existing := comp.DeepCopy() + cli := testutil.NewClient(t, existing, synth.DeepCopy()) + c := &controller{client: cli} + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + existing.Status.DependencyStatus = &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.WaitingOnDependenciesReason, + } + require.NoError(t, cli.Status().Update(ctx, existing)) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + cleared, err := c.clearDependencyStatus(ctx, existing, apiv1.WaitingOnDependenciesReason) + require.NoError(t, err) + assert.True(t, cleared) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + assert.Nil(t, existing.Status.DependencyStatus) + }) + + t.Run("skips when reason does not match", func(t *testing.T) { + existing := comp.DeepCopy() + cli := testutil.NewClient(t, existing, synth.DeepCopy()) + c := &controller{client: cli} + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + existing.Status.DependencyStatus = &apiv1.DependencyStatus{ + Blocked: true, + Reason: apiv1.CircularDependencyReason, + } + require.NoError(t, cli.Status().Update(ctx, existing)) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + cleared, err := c.clearDependencyStatus(ctx, existing, apiv1.WaitingOnDependenciesReason) + require.NoError(t, err) + assert.False(t, cleared) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + require.NotNil(t, existing.Status.DependencyStatus) + assert.Equal(t, apiv1.CircularDependencyReason, existing.Status.DependencyStatus.Reason) + }) + + t.Run("skips when status is nil", func(t *testing.T) { + existing := comp.DeepCopy() + cli := testutil.NewClient(t, existing, synth.DeepCopy()) + c := &controller{client: cli} + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(existing), existing)) + cleared, err := c.clearDependencyStatus(ctx, existing, apiv1.WaitingOnDependenciesReason) + require.NoError(t, err) + assert.False(t, cleared) + }) +} + +// TestDependencyNotFoundReasonSet proves that when a composition depends on a non-existent +// composition, the BlockedBy entry uses DependencyNotFound rather than DependencyNotReady. +func TestDependencyNotFoundReasonSet(t *testing.T) { + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t) + + c := &controller{client: cli, concurrencyLimit: 10, watchdogThreshold: time.Hour} + + synth := &apiv1.Synthesizer{} + synth.Name = "test-synth" + require.NoError(t, cli.Create(ctx, synth)) + + // comp-b depends on comp-a, but comp-a does NOT exist + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "default" + compB.Finalizers = []string{"eno.azure.io/cleanup"} + compB.Spec.Synthesizer.Name = synth.Name + compB.Spec.DependsOn = []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + } + require.NoError(t, cli.Create(ctx, compB)) + + _, err := c.Reconcile(ctx, ctrl.Request{}) + require.NoError(t, err) + + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(compB), compB)) + require.NotNil(t, compB.Status.DependencyStatus) + assert.True(t, compB.Status.DependencyStatus.Blocked) + assert.Equal(t, apiv1.WaitingOnDependenciesReason, compB.Status.DependencyStatus.Reason) + require.Len(t, compB.Status.DependencyStatus.BlockedBy, 1) + assert.Equal(t, "comp-a", compB.Status.DependencyStatus.BlockedBy[0].Name) + assert.Equal(t, "default", compB.Status.DependencyStatus.BlockedBy[0].Namespace) + assert.Equal(t, apiv1.WaitingOnDependencyNotFoundReason, compB.Status.DependencyStatus.BlockedBy[0].Reason) +} diff --git a/internal/controllers/scheduling/dependency.go b/internal/controllers/scheduling/dependency.go new file mode 100644 index 00000000..07d0cf4b --- /dev/null +++ b/internal/controllers/scheduling/dependency.go @@ -0,0 +1,56 @@ +package scheduling + +import ( + "path" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/Azure/eno/internal/toposort" +) + +// buildReadySet creates a set of namespace/name keys for compositions that +// have CurrentSynthesis.Ready != nil +func buildReadySet(comps *apiv1.CompositionList) map[string]bool { + readyMap := make(map[string]bool, len(comps.Items)) + for _, comp := range comps.Items { + if comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Ready != nil { + readyMap[path.Join(comp.GetNamespace(), comp.GetName())] = true + } + } + return readyMap +} + +// buildExistsSet creates a set of namespace/name keys for all compositions +func buildExistsSet(comps *apiv1.CompositionList) map[string]bool { + existMap := make(map[string]bool, len(comps.Items)) + for _, comp := range comps.Items { + existMap[path.Join(comp.GetNamespace(), comp.GetName())] = true + } + return existMap +} + +// areDependenciesReady checks if all required dependencies are ready. +func areDependenciesReady(comp *apiv1.Composition, readySet map[string]bool) bool { + for _, dep := range comp.Spec.DependsOn { + key := path.Join(dep.Namespace, dep.Name) + if !readySet[key] { // dependency is not ready + return false + } + } + return true +} + +// topoSortCompositions returns compositions in topological order (dependency first) +// and a set of synthesizer names that are part of dependency cycle +// Uses the generic Kahn's algorithm O(V+E) +func topoSortCompositions(compositions []apiv1.Composition) (sortedComposition []apiv1.Composition, cyclicSet map[string]bool) { + return toposort.TopologySort(compositions, + func(comp *apiv1.Composition) string { return path.Join(comp.GetNamespace(), comp.GetName()) }, + func(comp *apiv1.Composition) []string { + var deps []string + for _, dep := range comp.Spec.DependsOn { + deps = append(deps, path.Join(dep.Namespace, dep.Name)) + } + return deps + }, + ) +} diff --git a/internal/controllers/scheduling/dependency_test.go b/internal/controllers/scheduling/dependency_test.go new file mode 100644 index 00000000..aea2aa51 --- /dev/null +++ b/internal/controllers/scheduling/dependency_test.go @@ -0,0 +1,333 @@ +package scheduling + +import ( + "testing" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" +) + +func TestBuildReadySet(t *testing.T) { + comps := &apiv1.CompositionList{ + Items: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "ready-comp", Namespace: "ns1"}, + Status: apiv1.CompositionStatus{ + CurrentSynthesis: &apiv1.Synthesis{ + Ready: ptr.To(metav1.Now()), + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "not-ready-comp", Namespace: "ns1"}, + Status: apiv1.CompositionStatus{ + CurrentSynthesis: &apiv1.Synthesis{}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "no-synthesis", Namespace: "ns2"}, + }, + }, + } + + readySet := buildReadySet(comps) + + assert.True(t, readySet["ns1/ready-comp"]) + assert.False(t, readySet["ns1/not-ready-comp"]) + assert.False(t, readySet["ns2/no-synthesis"]) + assert.Len(t, readySet, 1) +} + +func TestBuildExistsSet(t *testing.T) { + comps := &apiv1.CompositionList{ + Items: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "comp-a", Namespace: "ns1"}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "comp-b", Namespace: "ns1"}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "comp-c", Namespace: "ns2"}, + }, + }, + } + + existsSet := buildExistsSet(comps) + + assert.True(t, existsSet["ns1/comp-a"]) + assert.True(t, existsSet["ns1/comp-b"]) + assert.True(t, existsSet["ns2/comp-c"]) + assert.False(t, existsSet["ns1/nonexistent"]) + assert.Len(t, existsSet, 3) +} + +func TestAreDependenciesReady(t *testing.T) { + readySet := map[string]bool{ + "ns1/dep-a": true, + "ns1/dep-b": true, + } + + tests := []struct { + name string + comp *apiv1.Composition + expected bool + }{ + { + name: "no dependencies", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "ns1"}, + }, + expected: true, + }, + { + name: "all deps ready", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "ns1"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "dep-a", Namespace: "ns1"}, + {Name: "dep-b", Namespace: "ns1"}, + }, + }, + }, + expected: true, + }, + { + name: "required dep not ready", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "ns1"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "dep-a", Namespace: "ns1"}, + {Name: "dep-missing", Namespace: "ns1"}, + }, + }, + }, + expected: false, + }, + { + name: "all deps required - one missing blocks", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "ns1"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "dep-a", Namespace: "ns1"}, + {Name: "dep-missing", Namespace: "ns1"}, + }, + }, + }, + expected: false, + }, + { + name: "cross-namespace dep ready", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "other-ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "dep-a", Namespace: "ns1"}, + }, + }, + }, + expected: true, + }, + { + name: "cross-namespace dep not ready", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "other-ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "dep-a", Namespace: "other-ns"}, // explicit other-ns, which isn't in readySet + }, + }, + }, + expected: false, + }, + { + name: "all deps missing blocks", + comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{Name: "comp", Namespace: "ns1"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "nonexistent-1", Namespace: "ns1"}, + {Name: "nonexistent-2", Namespace: "ns1"}, + }, + }, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := areDependenciesReady(tt.comp, readySet) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestTopoSortCompositions(t *testing.T) { + tests := []struct { + name string + compositions []apiv1.Composition + expectedOrder []string // namespace/name keys in expected order + expectedCyclic []string + }{ + { + name: "empty", + compositions: nil, + expectedOrder: []string{}, + expectedCyclic: nil, + }, + { + name: "single no deps", + compositions: []apiv1.Composition{ + {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "ns"}}, + }, + expectedOrder: []string{"ns/a"}, + expectedCyclic: nil, + }, + { + name: "linear chain A->B->C", + compositions: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "b", Namespace: "ns"}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "a", Namespace: "ns"}}, + }, + }, + {ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "ns"}}, + }, + expectedOrder: []string{"ns/a", "ns/b", "ns/c"}, + expectedCyclic: nil, + }, + { + name: "diamond A->B,A->C,B->D,C->D", + compositions: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "d", Namespace: "ns"}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Name: "b", Namespace: "ns"}, + {Name: "c", Namespace: "ns"}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "d", Namespace: "ns"}}, + }, + }, + {ObjectMeta: metav1.ObjectMeta{Name: "d", Namespace: "ns"}}, + }, + expectedOrder: []string{"ns/d", "ns/b", "ns/c", "ns/a"}, + expectedCyclic: nil, + }, + { + name: "simple cycle A<->B", + compositions: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "b", Namespace: "ns"}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "a", Namespace: "ns"}}, + }, + }, + }, + expectedOrder: []string{}, + expectedCyclic: []string{"ns/a", "ns/b"}, + }, + { + name: "cycle with independent node", + compositions: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "b", Namespace: "ns"}}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "b", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "a", Namespace: "ns"}}, + }, + }, + {ObjectMeta: metav1.ObjectMeta{Name: "c", Namespace: "ns"}}, + }, + expectedOrder: []string{"ns/c"}, + expectedCyclic: []string{"ns/a", "ns/b"}, + }, + { + name: "cross-namespace dependencies", + compositions: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "app", Namespace: "prod"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "db", Namespace: "infra"}}, + }, + }, + {ObjectMeta: metav1.ObjectMeta{Name: "db", Namespace: "infra"}}, + }, + expectedOrder: []string{"infra/db", "prod/app"}, + expectedCyclic: nil, + }, + { + name: "self loop", + compositions: []apiv1.Composition{ + { + ObjectMeta: metav1.ObjectMeta{Name: "a", Namespace: "ns"}, + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{{Name: "a", Namespace: "ns"}}, + }, + }, + }, + expectedOrder: []string{}, + expectedCyclic: []string{"ns/a"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sorted, cyclicSet := topoSortCompositions(tt.compositions) + + var sortedKeys []string + for _, comp := range sorted { + sortedKeys = append(sortedKeys, comp.Namespace+"/"+comp.Name) + } + if len(tt.expectedOrder) == 0 { + assert.Empty(t, sortedKeys) + } else { + assert.Equal(t, tt.expectedOrder, sortedKeys) + } + + if tt.expectedCyclic == nil { + assert.Empty(t, cyclicSet) + } else { + assert.Len(t, cyclicSet, len(tt.expectedCyclic)) + for _, key := range tt.expectedCyclic { + assert.True(t, cyclicSet[key], "expected %s in cyclic set", key) + } + } + }) + } +} diff --git a/internal/controllers/symphony/controller.go b/internal/controllers/symphony/controller.go index b054ef92..84860994 100644 --- a/internal/controllers/symphony/controller.go +++ b/internal/controllers/symphony/controller.go @@ -225,8 +225,23 @@ func (c *symphonyController) reconcileForward(ctx context.Context, symph *apiv1. } } - // Sync forward (create/update) - for _, variation := range symph.Spec.Variations { + // NEW: Build map of synthesizer name -> existing composition for dependency resolution + compBySynth := make(map[string]*apiv1.Composition, len(comps.Items)) + for i := range comps.Items { + compBySynth[comps.Items[i].Spec.Synthesizer.Name] = &comps.Items[i] + } + + // Sort variations so dependencies come first; detect cycles as a byproduct + sortedVariations, cyclicSynths := topoSortVariations(symph.Spec.Variations) + for synthName := range cyclicSynths { + logger.Error(fmt.Errorf("circular dependency detected in variation"), + "skipping variation", + "synthesizerName", synthName) + } + + // Sync forward (create/update) — process ALL variations in dependency order + for _, variation := range sortedVariations { + comp := &apiv1.Composition{} comp.Namespace = symph.Namespace comp.GenerateName = variation.Synthesizer.Name + "-" @@ -235,6 +250,42 @@ func (c *symphonyController) reconcileForward(ctx context.Context, symph *apiv1. comp.Spec.SynthesisEnv = getSynthesisEnv(symph, &variation) comp.Labels = variation.Labels comp.Annotations = variation.Annotations + + // Resolve variation dependencies to composition dependencies. If the dependent composition does not exist yet + // DO NOT CREATE the current composition as it might lead to race condition and ordering not being respected + var validDeps []apiv1.VariationDependency + for _, dep := range variation.DependsOn { + if dep.Synthesizer == "" { + logger.Error(fmt.Errorf("No Variation Dependency Synthesizer"), + "Error: variation dependency has no synthesizer set, dependency will be ignored", + "synthesizerName", variation.Synthesizer.Name) + continue + } + validDeps = append(validDeps, dep) + } + deps, allresolved := resolveVariationDeps(validDeps, compBySynth) + + comp.Spec.DependsOn = deps + idx := slices.IndexFunc(comps.Items, func(existing apiv1.Composition) bool { + return existing.Spec.Synthesizer.Name == variation.Synthesizer.Name + }) + + // Safety fallback: with topological sort this should not trigger since + // dependencies are processed first, but guards against edge cases like + // a failed Create earlier in the loop. + if !allresolved { + if idx == -1 { + logger.Error(fmt.Errorf("not all synthesizer-based dependencies resolved yet"), + "skipping composition creation", + "synthesizerName", variation.Synthesizer.Name) + } else { + logger.Error(fmt.Errorf("not all synthesizer-based dependencies resolved yet"), + "skipping composition update", + "synthesizerName", variation.Synthesizer.Name, "compositionName", comps.Items[idx].Name) + } + continue + } + err := controllerutil.SetControllerReference(symph, comp, c.client.Scheme()) if err != nil { logger.Error(err, "failed to set controller reference", "synthesizerName", variation.Synthesizer.Name) @@ -243,11 +294,13 @@ func (c *symphonyController) reconcileForward(ctx context.Context, symph *apiv1. logger := logger.WithValues("compositionName", comp.Name, "compositionNamespace", comp.Namespace, "operationID", comp.GetAzureOperationID(), "operationOrigin", comp.GetAzureOperationOrigin()) - // Compose missing variations - idx := slices.IndexFunc(comps.Items, func(existing apiv1.Composition) bool { - return existing.Spec.Synthesizer.Name == variation.Synthesizer.Name - }) if idx == -1 { + // Assumption to ensure idx == -1 works correctly. + // noCacheClient.List returns all compositions in the namespace, which under the below assumptions is correct. + // Need to revisit if any of the below assumptions change. + // 1. One symphony per namespace + // 2. No standalone compositions in the namespace + // 3. No cross-symphony dependency references err := c.noCacheClient.List(ctx, comps, client.InNamespace(symph.Namespace)) if err != nil { logger.Error(err, "failed to list compositions without cache", "synthesizerName", variation.Synthesizer.Name) @@ -272,7 +325,10 @@ func (c *symphonyController) reconcileForward(ctx context.Context, symph *apiv1. return false, fmt.Errorf("creating composition: %w", err) } logger.Info("created composition for symphony", "compositionName", comp.Name, "synthesizerName", variation.Synthesizer.Name) - return true, nil + // Add to compBySynth so later iterations can resolve deps on this composition + compBySynth[variation.Synthesizer.Name] = comp + modified = true + continue } // Diff and update if needed @@ -288,11 +344,11 @@ func (c *symphonyController) reconcileForward(ctx context.Context, symph *apiv1. return false, fmt.Errorf("updating existing composition: %w", err) } logger.Info("updated composition because its variation changed") - return true, nil + modified = true } logger.Info("reconcile forward complete") - return false, nil + return modified, nil } func (c *symphonyController) syncStatus(ctx context.Context, symph *apiv1.Symphony, comps *apiv1.CompositionList) error { @@ -462,3 +518,25 @@ func pruneAnnotations(variation *apiv1.Variation, existing *apiv1.Composition) b } return changed } + +func resolveVariationDeps(varDeps []apiv1.VariationDependency, compBySynth map[string]*apiv1.Composition) ([]apiv1.CompositionDependency, bool) { + if len(varDeps) == 0 { + return nil, true + } + + allResolved := true + var resolved []apiv1.CompositionDependency + for _, dep := range varDeps { + target, ok := compBySynth[dep.Synthesizer] + if !ok { + allResolved = false + continue // composition does not exist yet - will resolve on next reconcile + } + resolved = append(resolved, apiv1.CompositionDependency{ + Name: target.Name, + Namespace: target.Namespace, + }) + + } + return resolved, allResolved +} diff --git a/internal/controllers/symphony/controller_test.go b/internal/controllers/symphony/controller_test.go index de021f64..34de81bb 100644 --- a/internal/controllers/symphony/controller_test.go +++ b/internal/controllers/symphony/controller_test.go @@ -6,6 +6,8 @@ import ( "time" apiv1 "github.com/Azure/eno/api/v1" + "github.com/Azure/eno/internal/controllers/composition" + "github.com/Azure/eno/internal/controllers/scheduling" "github.com/Azure/eno/internal/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -834,3 +836,252 @@ func TestPruneAnnotationsOrdering(t *testing.T) { return setOn1 }) } + +func TestResolveVariationDeps(t *testing.T) { + compA := &apiv1.Composition{} + compA.Name = "comp-a" + compA.Namespace = "default" + + compB := &apiv1.Composition{} + compB.Name = "comp-b" + compB.Namespace = "ns-other" + + compBySynth := map[string]*apiv1.Composition{ + "synth-a": compA, + "synth-b": compB, + } + + tests := []struct { + name string + varDeps []apiv1.VariationDependency + compBySynth map[string]*apiv1.Composition + expectedDeps []apiv1.CompositionDependency + expectedAllResv bool + }{ + { + name: "empty deps", + varDeps: nil, + compBySynth: compBySynth, + expectedDeps: nil, + expectedAllResv: true, + }, + { + name: "synthesizer resolved", + varDeps: []apiv1.VariationDependency{ + {Synthesizer: "synth-a"}, + }, + compBySynth: compBySynth, + expectedDeps: []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + }, + expectedAllResv: true, + }, + { + name: "synthesizer unresolved", + varDeps: []apiv1.VariationDependency{ + {Synthesizer: "synth-missing"}, + }, + compBySynth: compBySynth, + expectedDeps: nil, + expectedAllResv: false, + }, + { + name: "unresolved synth blocks", + varDeps: []apiv1.VariationDependency{ + {Synthesizer: "synth-missing"}, + }, + compBySynth: compBySynth, + expectedDeps: nil, + expectedAllResv: false, + }, + { + name: "dep with empty synthesizer is unresolved", + varDeps: []apiv1.VariationDependency{ + {}, + }, + compBySynth: compBySynth, + expectedDeps: nil, + expectedAllResv: false, + }, + { + name: "partial synth resolution", + varDeps: []apiv1.VariationDependency{ + {Synthesizer: "synth-a"}, + {Synthesizer: "synth-missing"}, + {Synthesizer: "synth-b"}, + }, + compBySynth: compBySynth, + expectedDeps: []apiv1.CompositionDependency{ + {Name: "comp-a", Namespace: "default"}, + {Name: "comp-b", Namespace: "ns-other"}, + }, + expectedAllResv: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deps, allResolved := resolveVariationDeps(tc.varDeps, tc.compBySynth) + assert.Equal(t, tc.expectedAllResv, allResolved) + assert.Equal(t, tc.expectedDeps, deps) + }) + } +} + +// TestSymphonyNoDependencyVariationsReconcileNormally proves that a symphony +// whose variations have no dependsOn creates compositions that get synthesized +// without any ordering constraints. +func TestSymphonyNoDependencyVariationsReconcileNormally(t *testing.T) { + ctx := testutil.NewContext(t) + mgr := testutil.NewManager(t) + cli := mgr.GetClient() + + require.NoError(t, NewController(mgr.Manager)) + require.NoError(t, scheduling.NewController(mgr.Manager, 100, 2*time.Second, 0)) + require.NoError(t, composition.NewController(mgr.Manager, time.Minute)) + mgr.Start(t) + + // Create synthesizers + synthA := &apiv1.Synthesizer{} + synthA.Name = "synth-a" + synthA.Namespace = "default" + require.NoError(t, cli.Create(ctx, synthA)) + + synthB := &apiv1.Synthesizer{} + synthB.Name = "synth-b" + synthB.Namespace = "default" + require.NoError(t, cli.Create(ctx, synthB)) + + // Create symphony with NO dependsOn + sym := &apiv1.Symphony{} + sym.Name = "test-no-deps" + sym.Namespace = "default" + sym.Spec.Variations = []apiv1.Variation{ + {Synthesizer: apiv1.SynthesizerRef{Name: "synth-a"}}, + {Synthesizer: apiv1.SynthesizerRef{Name: "synth-b"}}, + } + require.NoError(t, cli.Create(ctx, sym)) + + // Both compositions should be created + testutil.Eventually(t, func() bool { + comps := &apiv1.CompositionList{} + cli.List(ctx, comps) + return len(comps.Items) == 2 + }) + + // Neither composition should have DependsOn + comps := &apiv1.CompositionList{} + require.NoError(t, cli.List(ctx, comps)) + for _, comp := range comps.Items { + assert.Empty(t, comp.Spec.DependsOn, "composition %s should have no dependencies", comp.Name) + } + + // Both compositions should eventually get an InFlightSynthesis (no blocking) + testutil.Eventually(t, func() bool { + comps := &apiv1.CompositionList{} + cli.List(ctx, comps) + for _, comp := range comps.Items { + if comp.Status.InFlightSynthesis == nil { + return false + } + } + return len(comps.Items) == 2 + }) +} + +// TestSymphonyDependencyVariationsReconcileInOrder proves that when variation B +// depends on variation A (via synthesizer reference), comp-B is not synthesized +// until comp-A is marked as Ready. +func TestSymphonyDependencyVariationsReconcileInOrder(t *testing.T) { + ctx := testutil.NewContext(t) + mgr := testutil.NewManager(t) + cli := mgr.GetClient() + + require.NoError(t, NewController(mgr.Manager)) + require.NoError(t, scheduling.NewController(mgr.Manager, 100, 2*time.Second, 0)) + require.NoError(t, composition.NewController(mgr.Manager, time.Minute)) + mgr.Start(t) + + // Create synthesizers + synthA := &apiv1.Synthesizer{} + synthA.Name = "synth-a" + synthA.Namespace = "default" + require.NoError(t, cli.Create(ctx, synthA)) + + synthB := &apiv1.Synthesizer{} + synthB.Name = "synth-b" + synthB.Namespace = "default" + require.NoError(t, cli.Create(ctx, synthB)) + + // Create symphony: variation B depends on variation A + sym := &apiv1.Symphony{} + sym.Name = "test-dep-order" + sym.Namespace = "default" + sym.Spec.Variations = []apiv1.Variation{ + {Synthesizer: apiv1.SynthesizerRef{Name: "synth-a"}}, + { + Synthesizer: apiv1.SynthesizerRef{Name: "synth-b"}, + DependsOn: []apiv1.VariationDependency{ + {Synthesizer: "synth-a"}, + }, + }, + } + require.NoError(t, cli.Create(ctx, sym)) + + // Wait for both compositions to be created + testutil.Eventually(t, func() bool { + comps := &apiv1.CompositionList{} + cli.List(ctx, comps) + return len(comps.Items) == 2 + }) + + // Verify comp-b has DependsOn pointing to comp-a + var compA, compB *apiv1.Composition + comps := &apiv1.CompositionList{} + require.NoError(t, cli.List(ctx, comps)) + for i := range comps.Items { + switch comps.Items[i].Spec.Synthesizer.Name { + case "synth-a": + compA = &comps.Items[i] + case "synth-b": + compB = &comps.Items[i] + } + } + require.NotNil(t, compA) + require.NotNil(t, compB) + require.Len(t, compB.Spec.DependsOn, 1) + assert.Equal(t, compA.Name, compB.Spec.DependsOn[0].Name) + assert.Equal(t, compA.Namespace, compB.Spec.DependsOn[0].Namespace) + + // comp-a should get an InFlightSynthesis (no deps) + testutil.Eventually(t, func() bool { + cli.Get(ctx, client.ObjectKeyFromObject(compA), compA) + return compA.Status.InFlightSynthesis != nil + }) + + // comp-b should NOT have InFlightSynthesis yet (dependency not ready) + cli.Get(ctx, client.ObjectKeyFromObject(compB), compB) + assert.Nil(t, compB.Status.InFlightSynthesis, "comp-b should be blocked while comp-a is not ready") + + // Mark comp-a synthesis as complete and Ready + err := retry.RetryOnConflict(testutil.Backoff, func() error { + cli.Get(ctx, client.ObjectKeyFromObject(compA), compA) + compA.Status.CurrentSynthesis = &apiv1.Synthesis{ + UUID: compA.Status.InFlightSynthesis.UUID, + ObservedCompositionGeneration: compA.Generation, + ObservedSynthesizerGeneration: synthA.Generation, + Synthesized: ptr.To(metav1.Now()), + Reconciled: ptr.To(metav1.Now()), + Ready: ptr.To(metav1.Now()), + } + compA.Status.InFlightSynthesis = nil + return cli.Status().Update(ctx, compA) + }) + require.NoError(t, err) + + // Now comp-b should eventually get an InFlightSynthesis (dependency is ready) + testutil.Eventually(t, func() bool { + cli.Get(ctx, client.ObjectKeyFromObject(compB), compB) + return compB.Status.InFlightSynthesis != nil + }) +} diff --git a/internal/controllers/symphony/topologySort.go b/internal/controllers/symphony/topologySort.go new file mode 100644 index 00000000..fb8f81dc --- /dev/null +++ b/internal/controllers/symphony/topologySort.go @@ -0,0 +1,24 @@ +package symphony + +import ( + apiv1 "github.com/Azure/eno/api/v1" + "github.com/Azure/eno/internal/toposort" +) + +// topoSortVariations returns variations in topological order (dependency first) +// and a set of synthesizer names that are part of dependency cycle +// Uses the generic Kahn's algorithm O(V+E) +func topoSortVariations(variations []apiv1.Variation) (sorted []apiv1.Variation, cyclicSynths map[string]bool) { + return toposort.TopologySort(variations, + func(variation *apiv1.Variation) string { return variation.Synthesizer.Name }, + func(variation *apiv1.Variation) []string { + var deps []string + for _, dep := range variation.DependsOn { + if dep.Synthesizer != "" { // a safety guard to ensure no empty synthesizers will be added + deps = append(deps, dep.Synthesizer) + } + } + return deps + }, + ) +} diff --git a/internal/controllers/symphony/topologySort_test.go b/internal/controllers/symphony/topologySort_test.go new file mode 100644 index 00000000..5a404bd6 --- /dev/null +++ b/internal/controllers/symphony/topologySort_test.go @@ -0,0 +1,112 @@ +package symphony + +import ( + "testing" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func variation(synth string, deps ...string) apiv1.Variation { + v := apiv1.Variation{Synthesizer: apiv1.SynthesizerRef{Name: synth}} + for _, d := range deps { + v.DependsOn = append(v.DependsOn, apiv1.VariationDependency{Synthesizer: d}) + } + return v +} + +func synthNames(variations []apiv1.Variation) []string { + names := make([]string, len(variations)) + for i, v := range variations { + names[i] = v.Synthesizer.Name + } + return names +} + +func TestTopoSortVariations(t *testing.T) { + tests := []struct { + name string + variations []apiv1.Variation + wantOrder []string + wantCyclicLen int + }{ + { + name: "empty", + variations: nil, + wantOrder: []string{}, + wantCyclicLen: 0, + }, + { + name: "single no deps", + variations: []apiv1.Variation{variation("a")}, + wantOrder: []string{"a"}, + wantCyclicLen: 0, + }, + { + name: "two independent", + variations: []apiv1.Variation{variation("b"), variation("a")}, + wantOrder: []string{"a", "b"}, // deterministic alphabetical + wantCyclicLen: 0, + }, + { + name: "linear chain A <- B <- C", + variations: []apiv1.Variation{variation("c", "b"), variation("a"), variation("b", "a")}, + wantOrder: []string{"a", "b", "c"}, + wantCyclicLen: 0, + }, + { + name: "diamond A <- B, A <- C, B <- D, C <- D", + variations: []apiv1.Variation{ + variation("d", "b", "c"), + variation("b", "a"), + variation("c", "a"), + variation("a"), + }, + wantOrder: []string{"a", "b", "c", "d"}, + wantCyclicLen: 0, + }, + { + name: "simple cycle A <-> B", + variations: []apiv1.Variation{variation("a", "b"), variation("b", "a")}, + wantOrder: []string{}, + wantCyclicLen: 2, + }, + { + name: "cycle with independent node", + variations: []apiv1.Variation{ + variation("a", "b"), + variation("b", "a"), + variation("c"), + }, + wantOrder: []string{"c"}, + wantCyclicLen: 2, + }, + { + name: "self loop", + variations: []apiv1.Variation{variation("a", "a")}, + wantOrder: []string{}, + wantCyclicLen: 1, + }, + { + name: "empty synthesizer in dep is ignored", + variations: []apiv1.Variation{ + variation("a"), + { + Synthesizer: apiv1.SynthesizerRef{Name: "b"}, + DependsOn: []apiv1.VariationDependency{{Synthesizer: ""}, {Synthesizer: "a"}}, + }, + }, + wantOrder: []string{"a", "b"}, + wantCyclicLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sorted, cyclic := topoSortVariations(tt.variations) + assert.Equal(t, tt.wantOrder, synthNames(sorted)) + require.Len(t, cyclic, tt.wantCyclicLen) + }) + } +} diff --git a/internal/toposort/toposort.go b/internal/toposort/toposort.go new file mode 100644 index 00000000..42788ff1 --- /dev/null +++ b/internal/toposort/toposort.go @@ -0,0 +1,67 @@ +// internal/toposort/toposort.go +package toposort + +import "sort" + +// TopoSort performs Kahn's algorithm on a slice of items. +// keyFn extracts a unique string key for each item. +// depsFn extracts the dependency keys for each item. +// Returns items in topological order (dependencies first) and a set of keys in cycles. +func TopologySort[T any](items []T, keyFn func(*T) string, depsFn func(*T) []string) (sorted []T, cyclic map[string]bool) { + byKey := make(map[string]*T, len(items)) + inDegree := make(map[string]int, len(items)) + dependents := make(map[string][]string) + + // First pass: index all items by key + for i := range items { + key := keyFn(&items[i]) + byKey[key] = &items[i] + inDegree[key] = 0 + } + + // Second pass: compute in-degrees only for deps that exist in the item set. + // Dependencies referencing non-existent keys are skipped so they don't + // artificially inflate in-degree and get misclassified as cycles. + for i := range items { + key := keyFn(&items[i]) + for _, dep := range depsFn(&items[i]) { + if _, exists := byKey[dep]; !exists { + continue + } + dependents[dep] = append(dependents[dep], key) + inDegree[key]++ + } + } + + var queue []string + for key, deg := range inDegree { + if deg == 0 { + queue = append(queue, key) + } + } + sort.Strings(queue) + + sorted = make([]T, 0, len(items)) + for len(queue) > 0 { + key := queue[0] + queue = queue[1:] + if node, ok := byKey[key]; ok { + sorted = append(sorted, *node) + } + for _, dep := range dependents[key] { + inDegree[dep]-- + if inDegree[dep] == 0 { + queue = append(queue, dep) + sort.Strings(queue) + } + } + } + + cyclic = make(map[string]bool) + for key, deg := range inDegree { + if deg > 0 { + cyclic[key] = true + } + } + return sorted, cyclic +} diff --git a/internal/toposort/toposort_test.go b/internal/toposort/toposort_test.go new file mode 100644 index 00000000..cfca2baa --- /dev/null +++ b/internal/toposort/toposort_test.go @@ -0,0 +1,149 @@ +package toposort + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +type node struct { + name string + deps []string +} + +func TestTopologySort(t *testing.T) { + tests := []struct { + name string + items []node + expectedOrder []string + expectedCyclic []string + }{ + { + name: "empty input", + items: nil, + expectedOrder: []string{}, + expectedCyclic: nil, + }, + { + name: "single node no deps", + items: []node{{name: "a"}}, + expectedOrder: []string{"a"}, + expectedCyclic: nil, + }, + { + name: "two independent nodes", + items: []node{ + {name: "b"}, + {name: "a"}, + }, + expectedOrder: []string{"a", "b"}, // sorted alphabetically by Kahn's queue + expectedCyclic: nil, + }, + { + name: "linear chain a->b->c", + items: []node{ + {name: "c", deps: []string{"b"}}, + {name: "b", deps: []string{"a"}}, + {name: "a"}, + }, + expectedOrder: []string{"a", "b", "c"}, + expectedCyclic: nil, + }, + { + name: "diamond", + items: []node{ + {name: "a", deps: []string{"b", "c"}}, + {name: "b", deps: []string{"d"}}, + {name: "c", deps: []string{"d"}}, + {name: "d"}, + }, + expectedOrder: []string{"d", "b", "c", "a"}, + expectedCyclic: nil, + }, + { + name: "simple cycle", + items: []node{ + {name: "a", deps: []string{"b"}}, + {name: "b", deps: []string{"a"}}, + }, + expectedOrder: []string{}, + expectedCyclic: []string{"a", "b"}, + }, + { + name: "self loop", + items: []node{ + {name: "a", deps: []string{"a"}}, + }, + expectedOrder: []string{}, + expectedCyclic: []string{"a"}, + }, + { + name: "cycle with independent nodes", + items: []node{ + {name: "x", deps: []string{"y"}}, + {name: "y", deps: []string{"x"}}, + {name: "z"}, + }, + expectedOrder: []string{"z"}, + expectedCyclic: []string{"x", "y"}, + }, + { + name: "three node cycle", + items: []node{ + {name: "a", deps: []string{"b"}}, + {name: "b", deps: []string{"c"}}, + {name: "c", deps: []string{"a"}}, + }, + expectedOrder: []string{}, + expectedCyclic: []string{"a", "b", "c"}, + }, + { + name: "dep references non-existent node", + items: []node{ + {name: "a", deps: []string{"phantom"}}, + }, + expectedOrder: []string{"a"}, + expectedCyclic: nil, + }, + { + name: "complex mixed graph", + items: []node{ + {name: "e"}, + {name: "d", deps: []string{"e"}}, + {name: "a", deps: []string{"b"}}, + {name: "b", deps: []string{"c"}}, + {name: "c", deps: []string{"a"}}, // a,b,c form a cycle + }, + expectedOrder: []string{"e", "d"}, + expectedCyclic: []string{"a", "b", "c"}, + }, + } + + keyFn := func(n *node) string { return n.name } + depsFn := func(n *node) []string { return n.deps } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sorted, cyclicSet := TopologySort(tt.items, keyFn, depsFn) + + var sortedKeys []string + for _, n := range sorted { + sortedKeys = append(sortedKeys, n.name) + } + if len(tt.expectedOrder) == 0 { + assert.Empty(t, sortedKeys) + } else { + assert.Equal(t, tt.expectedOrder, sortedKeys) + } + + if tt.expectedCyclic == nil { + assert.Empty(t, cyclicSet) + } else { + assert.Len(t, cyclicSet, len(tt.expectedCyclic)) + for _, key := range tt.expectedCyclic { + assert.True(t, cyclicSet[key], "expected %s in cyclic set", key) + } + } + }) + } +}