[eno][part2] Adding composition creation order for creation/update

[ruinan-liu]

Adding controller logic for using DependsOn on creation flow.

[ruinan-liu]

replied to comments

[xiazhan]

Code review

Found 3 issues:

1. Compositions referencing non-existent dependencies are silently treated as "not ready" rather than flagged as "not found". The WaitingOnDependencyNotFoundReason constant is defined but never used. In scheduling/controller.go, when a dependency key is not in readySet, the code always sets DependencyNotReady — but the dependency composition might not exist at all. There is no distinction between "exists but not ready" and "does not exist". This makes debugging harder and the DependencyNotFound reason dead code.

eno/internal/controllers/scheduling/controller.go

Lines 174 to 191 in 4a465d4

	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] {
	logger.Info("Current Dependency not ready for composition", "compositionName", comp.GetName(), "compositionNamespace", comp.GetNamespace(),
	"dependencyName", dep.Name, "dependencyNamespace", dep.Namespace)
	blockedBy = append(blockedBy, apiv1.BlockedByRef{
	Name: dep.Name,
	Namespace: dep.Namespace,
	Reason: apiv1.WaitingOnDependencyNotReadyReason,
	})
	}
	}
	newStatus := &apiv1.DependencyStatus{
	Blocked: true,

2. The WaitingOnDependencies status update comparison only checks Reason, not BlockedBy contents. If the set of blocking dependencies changes (e.g., one dependency becomes ready but another is still blocked), the BlockedBy list in the status will be stale because the condition comp.Status.DependencyStatus.Reason != newStatus.Reason is already false (both are WaitingOnDependencies), so the patch is skipped. The status will only update once all deps are ready or the reason changes entirely.

eno/internal/controllers/scheduling/controller.go

Lines 195 to 202 in 4a465d4

	if comp.Status.DependencyStatus == nil || comp.Status.DependencyStatus.Reason != newStatus.Reason {
	copy := comp.DeepCopy()
	copy.Status.DependencyStatus = newStatus
	if err := c.client.Status().Patch(ctx, copy, client.MergeFrom(&comp)); err != nil {
	logger.Error(err, "failed to update dependency status")
	return ctrl.Result{}, err
	}
	}

3. resolveVariationDeps treats an empty Synthesizer field as "unresolved" (returns allResolved=false), but the caller already filters these out. This creates a subtle inconsistency: the caller in symphony/controller.go strips empty-synthesizer deps into validDeps before calling resolveVariationDeps, but resolveVariationDeps itself also handles empty synthesizer by failing lookup (empty string won't be in compBySynth). The double handling is benign but the function's contract is unclear — if it's supposed to receive pre-validated deps, the empty-string guard in topoSortVariations and the caller's filter are redundant. More importantly, the test "dep with empty synthesizer is unresolved" tests resolveVariationDeps directly with an empty synthesizer, which the real call path would never hit.

eno/internal/controllers/symphony/controller.go

Lines 257 to 270 in 4a465d4

	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
	})

[xiazhan]

Addendum to code review — one more issue:

4. Non-existent dependencies are misclassified as circular dependencies. In TopologySort, if a composition references a dependency that does not exist in the item set (e.g., typo in DependsOn, deleted composition, or not-yet-created), the referencing item's in-degree is incremented but the missing key is never enqueued (no entry in byKey). The item's in-degree never reaches 0, so it lands in the cyclic set — even though there is no actual cycle. The scheduling controller then labels it with CircularDependencyReason, which is incorrect and misleading. The user sees "circular dependency" when the real problem is "dependency not found."

eno/internal/toposort/toposort.go

Lines 20 to 25 in 4a465d4

	}
	for _, dep := range depsFn(&items[i]) {
	dependents[dep] = append(dependents[dep], key)
	inDegree[key]++
	}
	}

Fix: Before incrementing in-degree for a dep key, check if it exists in the item set. If not, track it separately as "unresolved" rather than letting it fall into the cyclic bucket.

[ruinan-liu]

Respond to comments:

1. Compositions referencing non-existent dependencies are silently treated as "not ready" rather than flagged as "not found".
   This is fixed in the code

2. The WaitingOnDependencies status update comparison only checks Reason, not BlockedBy contents.
   This is fixed by using

if comp.Status.DependencyStatus != nil && equality.Semantic.DeepEqual(*comp.Status.DependencyStatus, *newStatus) {
		return false, nil // modified=false, err = nil
	}

4. resolveVariationDeps treats an empty Synthesizer field as "unresolved" (returns allResolved=false), but the caller already filters these out.
   resolveVariationDeps implicitly rejects empty-synthesier dependencies( they won't match any key in compBySynth), but the caller in reconcileForward already explicitly filters them out into validDeps before calling it. This double handling is benign - both layers produce the correct result, and the redundancy provides defense in depth if a future call site skips pre-filtering. The tests dep with empty synthesizer is unresolved exercise an unreachable path forn the current caller, but it still documents the function's standalone contract. Removing either guard would tighten coupling for zero behavior benefit, so this is intentionally left as-is.

5. Non-existent dependencies are misclassified as circular dependencies.
   Fixed in the test.

[ruinan-liu]

Responded to comments

[ruinan-liu]

Thanks, replied to comments

[ruinan-liu]

REplied to comments