re-implement compose logic#13638
Closed
ndeloof wants to merge 1 commit into
Closed
Conversation
ndeloof
force-pushed
the
reconciliation
branch
8 times, most recently
from
f0460b4 to
8017d4e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR re-implements the Compose “up/create” convergence logic as a pure reconciliation step (observed vs desired state) that outputs a dependency-ordered plan, plus an executor that applies that plan via Docker API calls. This aims to decouple decision-making from Docker API interactions and enable more targeted unit testing.
Changes:
- Introduces
ObservedState, a pureReconcile()planner, and a DAG-basedExecutePlan()executor to replace most of the old convergence logic. - Refactors
createto: inspect current state → reconcile → execute plan, and updatesrunto use the new execution state for service reference resolution. - Adds extensive unit tests for reconcile behavior and annotates some e2e tests with TODOs pointing to the new unit coverage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/compose/create.go | Switches create flow to InspectState → Reconcile → ExecutePlan and adds external network validation + “untouched” event emission. |
| pkg/compose/observed_state.go | Adds observed state builder (containers/networks/volumes/orphans) used as Reconcile input. |
| pkg/compose/reconcile.go | Adds the pure reconciliation/planning logic, operation model, and dependency wiring. |
| pkg/compose/plan_executor.go | Adds plan execution engine (concurrent DAG traversal) and display helpers; includes new executionState. |
| pkg/compose/reconcile_test.go | Adds a large suite of unit tests validating plan generation across scenarios. |
| pkg/compose/observed_state_test.go | Adds basic tests for observed state and plan helper behaviors. |
| pkg/compose/run.go | Replaces convergence-based service reference resolution with executionState. |
| pkg/compose/convergence.go | Removes most of the old convergence implementation (now replaced by reconcile+executor). |
| pkg/e2e/*.go | Adds TODO comments noting equivalent coverage by new Reconcile unit tests. |
| pkg/compose/publish.go | Minor refactor to use fmt.Fprintf with builders. |
| pkg/compose/progress.go | Updates comment wording for runningEvent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+1098
to
+1104
| ctrName := getCanonicalContainerName(ctr) | ||
| // Skip if we already have operations for this container | ||
| if _, exists := plan.Operations["recreate-container:"+ctrName]; exists { | ||
| continue | ||
| } | ||
| if _, exists := plan.Operations["stop-container:"+ctrName]; exists { | ||
| continue |
| case op.ContainerOp != nil: | ||
| serviceOps[op.ServiceName] = append(serviceOps[op.ServiceName], op) | ||
| case op.PluginOp != nil: | ||
| serviceOps[op.ServiceName] = append(serviceOps[op.ServiceName], op) |
Comment on lines
+720
to
+733
| removeID := fmt.Sprintf("remove-container:%s", ctrName) | ||
| plan.Operations[removeID] = &Operation{ | ||
| ID: removeID, | ||
| Type: OpRemoveContainer, | ||
| ServiceName: service.Name, | ||
| Resource: ctrName, | ||
| ContainerOp: &ContainerOperation{ | ||
| Service: service, | ||
| ContainerName: ctrName, | ||
| Existing: &ctr, | ||
| }, | ||
| DependsOn: []string{stopID}, | ||
| Reason: "scale down", | ||
| } |
Comment on lines
+71
to
+115
| func (es *executionState) getContainers(serviceName string) Containers { | ||
| es.mu.Lock() | ||
| defer es.mu.Unlock() | ||
| return es.containers[serviceName] | ||
| } | ||
|
|
||
| func (es *executionState) setNetworkID(key, id string) { | ||
| es.mu.Lock() | ||
| defer es.mu.Unlock() | ||
| es.networks[key] = id | ||
| } | ||
|
|
||
| func (es *executionState) setVolumeID(key, id string) { | ||
| es.mu.Lock() | ||
| defer es.mu.Unlock() | ||
| es.volumes[key] = id | ||
| } | ||
|
|
||
| // resolveServiceReferences replaces service references in a ServiceConfig with | ||
| // actual container IDs from the execution state. This mirrors the logic in | ||
| // convergence.resolveServiceReferences but uses executionState instead. | ||
| func (es *executionState) resolveServiceReferences(service *types.ServiceConfig) error { | ||
| if err := es.resolveVolumeFrom(service); err != nil { | ||
| return err | ||
| } | ||
| return es.resolveSharedNamespaces(service) | ||
| } | ||
|
|
||
| func (es *executionState) resolveVolumeFrom(service *types.ServiceConfig) error { | ||
| for i, vol := range service.VolumesFrom { | ||
| spec := strings.Split(vol, ":") | ||
| if len(spec) == 0 { | ||
| continue | ||
| } | ||
| if spec[0] == "container" { | ||
| service.VolumesFrom[i] = spec[1] | ||
| continue | ||
| } | ||
| name := spec[0] | ||
| dependencies := es.getContainers(name) | ||
| if len(dependencies) == 0 { | ||
| return fmt.Errorf("cannot share volume with service %s: container missing", name) | ||
| } | ||
| service.VolumesFrom[i] = dependencies.sorted()[0].ID | ||
| } |
Comment on lines
+101
to
+109
| // Partition containers by service | ||
| containersByService := map[string]Containers{} | ||
| for _, c := range allContainers { | ||
| service := c.Labels[api.ServiceLabel] | ||
| containersByService[service] = append(containersByService[service], c) | ||
| } | ||
|
|
||
| // Identify orphan containers | ||
| orphans := allContainers.filter(isOrphaned(project)) |
Comment on lines
+271
to
+279
| // Build network ID map and volume name map for needsRecreate checks | ||
| networkIDs := map[string]string{} | ||
| for key, n := range observed.Networks { | ||
| networkIDs[key] = n.ID | ||
| } | ||
| volumeNames := map[string]string{} | ||
| for key, v := range observed.Volumes { | ||
| volumeNames[key] = v.Name | ||
| } |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- build observed state - compute reconciliation plan by comparing observed vs desired state - execute plan this allow to decorelate reconciliation (aka "convergence") logic from docker API, and write simpler and efficient tests to cover various scenarios Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
re-implement compose logic as a reconciliation between observed and desired state, producing a plan with pending operations.
This allow to decorelate reconciliation (aka "convergence") logic from docker API, and write simpler and efficient tests to cover various scenarios
This PR was created with AI assistance
What I did
told Claude about the architecture I had in mind, and expectations regarding tests structure
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did
