Skip to content

[Eno] Add inter composition ordering#581

Closed
ruinan-liu wants to merge 24 commits intomainfrom
users/ruinanliu/InterCompositionOrdering
Closed

[Eno] Add inter composition ordering#581
ruinan-liu wants to merge 24 commits intomainfrom
users/ruinanliu/InterCompositionOrdering

Conversation

@ruinan-liu
Copy link
Copy Markdown
Collaborator

uses dependsOn to add inter composition ordering

@ruinan-liu ruinan-liu marked this pull request as ready for review March 24, 2026 18:42
Copy link
Copy Markdown
Collaborator Author

@ruinan-liu ruinan-liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reply to comments

@ruinan-liu ruinan-liu requested a review from xiazhan March 30, 2026 22:52
Copy link
Copy Markdown
Collaborator Author

@ruinan-liu ruinan-liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied to comments

@ruinan-liu ruinan-liu requested a review from xiazhan April 1, 2026 03:31
Copy link
Copy Markdown
Collaborator Author

@ruinan-liu ruinan-liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Respond to comment

@ruinan-liu ruinan-liu requested a review from xiazhan April 1, 2026 17:09
@ruinan-liu ruinan-liu enabled auto-merge (squash) April 3, 2026 20:02
@xiazhan
Copy link
Copy Markdown
Collaborator

xiazhan commented Apr 3, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@ruinan-liu
Copy link
Copy Markdown
Collaborator Author

  1. DependencyStatus with CircularDependency reason is never cleared after cycle resolution
    Fixed, updated in code
  2. No DependencyStatus update when dependencies aren't ready (scheduling controller logs + continues silently)
    Currently it is doing in compositionController
    // This means that dependencies exists and no synthesis started showing WaitingOnDependencies
    if len(comp.Spec.DependsOn) > 0 {
    status.Status = apiv1.WaitingOnDependenciesReason
    return status
    }
  3. path.Join with empty namespace produces wrong key for standalone Compositions
    Not Applicable, there will always be a namespace
  4. Circular dependency status patch errors are logged but swallowed (no requeue)
    Currently are not applicable, the scheduling controller runs on every composition/synthesizer change via SingleEventHandler, so the next reconcile will naturally retry the patch. The status will eventually converge.
  5. newDependencyEventHandler predicate allows updates through but handler only enqueues on deletion
    Changed to only allow update when a composition is in deletionTimestamp given it's only used to unblock dependency deletion
  6. CompositionDependency.Namespace comment says "dependent" but means "dependency" (wrong API doc)
    Done
  7. Doc comment says buildComsByKey but function is named buildCompsByKey
    done. Fixed

@ruinan-liu ruinan-liu requested a review from xiazhan April 3, 2026 23:36
logger.Info("reconciling reverse - checking for compositions to delete", "variationCount", len(symph.Spec.Variations), "existingCompositionCount", len(comps.Items), "symphonyDeleting", symph.DeletionTimestamp != nil)
// Delete compositions when their synth has been removed from the symphony
existingBySynthName := map[string][]*apiv1.Composition{}
for _, comp := range comps.Items {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should sort the comps.Items with dependency graph before traversing. So we can complete the creation/deletion in O(n). During the sorting, we can detect the circular dependency as well.

}

// Sync forward (create/update)
for _, variation := range symph.Spec.Variations {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same comment. Should sort the comps.Items with dependency graph before traversing. So we can complete the creation/deletion in O(n). During the sorting, we can detect the circular dependency as well.

The pr is too big for an effective review. Please split into multiple prs with each targeting one specific controller.


var inFlight int
var op *op
for _, comp := range comps.Items {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort it with dependency graph before traversing.

ruinan-liu added a commit that referenced this pull request Apr 6, 2026
Part1 of breaking down of the larger PR:
#581
Adding API changes in `Composition` and `Symphony` Specs.
@ruinan-liu ruinan-liu closed this Apr 7, 2026
auto-merge was automatically disabled April 7, 2026 00:04

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants