feat(reconcile): model network lifecycle in the plan#13966
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR cleanly migrates network lifecycle into the reconciliation plan. The plan ordering (stop → disconnect → remove → create → reconnect) and the anti-race dependency edges (connectNodes) look correct and are well-tested. Two findings worth addressing:
createNetworkdrops the conflict-retry thatensureNetworkhad for concurrent invocations — concurrentdocker compose upordocker compose runprocesses can now fail with a hard error instead of retrying.- Networks with an incomplete label set (project label present, network label absent) are stored with an empty
ConfigHashbydiscoverUnmanagedNetworks, silently bypassing divergence detection for a narrow class of legacy deployments.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR cleanly ports network lifecycle management into the declarative reconciliation plan, mirroring the prior volumes change. The dependency-ordering machinery (stoppedByPlan, connectNodes, infrastructureDeps) is well-structured and the test coverage is solid.
One moderate-confidence finding: in the entangled scenario where a network diverges and an attached container is independently scheduled for recreation, planRecreateNetworks emits an OpConnectNetwork targeting the old container's ID. The ordering guarantee prevents the reconnect from racing the removal, but StopContainer is not ordered relative to OpConnectNetwork, so the reconnect may be called against a stopped container — Docker's behavior in that case is implementation-defined. The new container already attaches to the new network via infrastructureDeps, so the reconnect is also redundant in this scenario.
| Type: OpConnectNetwork, | ||
| ResourceID: resID, | ||
| Cause: fmt.Sprintf("network %s recreate", key), | ||
| Container: &oc.Summary, |
There was a problem hiding this comment.
[medium] OpConnectNetwork targets the old container ID when the container is also being independently recreated
In planRecreateNetworks, the reconnect node is built with Container: &oc.Summary, which holds the observed (old) container's ID. When reconcileContainers later decides to recreate that same container for an unrelated reason (e.g. its config hash changed), the plan's dependency ordering is:
StopContainer(old)
↓ (parallel with)
OpConnectNetwork(old_id → new_network) ← this targets the old container
↓
RemoveContainer(old) ← waits for OpConnectNetwork via connectNodes
↓
RenameContainer(new)
The connectNodes guard correctly prevents RemoveContainer from racing OpConnectNetwork, so the removal ordering is sound. However, StopContainer is not ordered relative to OpConnectNetwork: the old container may already be stopped (or stopping) when NetworkConnect(old_id) is called. Whether Docker accepts NetworkConnect on a stopped container is implementation-defined; if it rejects it, the reconnect step errors and the entire reconcile operation fails even though the new container is already being created with the new network via infrastructureDeps.
The reconnect of the old container is also redundant when the container is being recreated: the new container is gated on networkNodes[net] (the CreateNetwork node) through infrastructureDeps, so it is created already attached to the new network.
Suggested fix: In planRecreateNetworks, skip emitting OpConnectNetwork for any container that is already scheduled for recreation (r.stoppedByPlan is set from an earlier volume-recreate stage, or the container will be detected diverged by reconcileContainers). Since the check cannot be done at plan-build time for reconcileContainers-driven recreations (they run after planRecreateNetworks), a simpler guard is: if the container's service has a config-hash mismatch, skip the reconnect — the new container will come up on the correct network. Alternatively, record the fact that the container's reconnect node was already provided by a network recreate, and let planRecreateContainer skip the connectNodes for it while still honoring the createNetwork ordering via infrastructureDeps.
| Confidence | Score |
|---|---|
| 🟡 moderate | 57/100 |
glours
left a comment
There was a problem hiding this comment.
Rename doesn't converge when the best-effort removal is skipped
| r.stoppedByPlan[oc.ID] = stopNode | ||
| } | ||
| disconnectNodes = append(disconnectNodes, r.plan.addNode(Operation{ | ||
| Type: OpDisconnectNetwork, |
There was a problem hiding this comment.
After a rename where the old network survives (the exact case BestEffort was added for), it keeps its compose labels: two live networks now share the same project+network labels. The discovery loop then picks a daemon-order-dependent winner via
map overwrite (observed_state.go:126-137).
On the next up, if the old network wins, the rename path re-fires and emits OpDisconnectNetwork for containers that are only attached to the new network (reconcile.go:229-236). Verified against a live daemon: NetworkDisconnect on a non-attached container errors even with Force: true → the errgroup cancels the whole plan after the stop node ran → up hard-fails with containers left stopped. And nothing ever removes the old network, so every subsequent up is a coin flip between a no-op and this failure.
Suggested fix (first half alone prevents the failure):
- In
planRecreateNetworks, only emit the disconnect for containers actually attached to the observed network —oc.ConnectedNetworksalready has the data. - On label-key collision in the discovery loop, prefer the network whose
Namematchesproject.Networks[key].Name(and/or warn), so runs become deterministic.
Mirror the volume reconciliation work (docker#13962) for networks: move network divergence detection and recreation out of the imperative pre-reconcile path (ensureNetwork/resolveOrCreateNetwork/removeDivergedNetwork) into the reconciliation plan. - reconcileNetworks now owns creation of missing networks and, for a network whose config-hash diverged, an explicit recreation sequence (no user confirmation: recreating a network is not destructive): stop containers -> disconnect -> remove network -> create network -> reconnect containers. Attached containers keep their identity (they are reconnected, not recreated), matching the previous behavior. If a container is independently recreated by reconcileContainers, its removal is ordered after the reconnect so they don't race. - Renaming a network creates the new one additively and leaves the old one untouched. - collectObservedState discovers legacy/unlabeled networks by name and records them as unmanaged matches (empty config-hash) so the reconciler reuses them untouched; ownership warnings move to warnUnmanagedNetworks. checkExternalNetworks keeps external-network validation/resolution. - execCreateNetwork now issues a plain createNetwork; the imperative ensureNetwork/resolveOrCreateNetwork/removeDivergedNetwork and the connect/disconnect helpers are removed. Adds reconcile, observed-state and executor tests covering network create/diverge/rename, the entangled diverge+recreate case, legacy by-name discovery and the ownership warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Treat a network rename (observed.Name != desired.Name) as a recreation rather than an additive create: the old network is removed, the new one created, and attached containers are migrated onto it (reconnected), so they no longer stay on the previous network until recreated for another reason. Networks carry no data, so removing the previous network — instead of leaving it dangling alongside the new one under the same compose label — is safe and keeps subsequent runs deterministic. This is a marginal behavior change from previous Compose releases (which created the new network and left the old attachments in place) in exchange for the more logical outcome. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Address review findings on the network reconcile migration: - createNetwork now treats a NetworkCreate conflict as success. A concurrent `docker compose up|run` can create the same network in the TOCTOU window between the observed-state snapshot and the create call; the previous ensureNetwork retried on conflict, the plain create must not fail hard. - discoverUnmanagedNetworks/Volumes preserve the config-hash when the live resource is owned by this project (project label present, key label absent — e.g. written by an older Compose) so genuine divergence is still detected. For resources we don't own the hash stays empty and they are reused untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
A network rename does not require removing the old network — the new one has a different name and is created independently. Yet the old removal could block the whole operation: NetworkRemove fails when non-Compose containers are still attached, and CreateNetwork depended on it. Split the rename path from the same-name divergence path: - Rename: CreateNetwork no longer depends on RemoveNetwork; the container migration proceeds regardless. RemoveNetwork is marked best-effort and, if the network is still in use (reported as a conflict), is skipped with a warning instead of failing. Any other error (transport, Moby API) is still propagated. - Same-name divergence keeps the mandatory remove-before-create ordering. Adds an Operation.BestEffort flag, honored by execRemoveNetwork, plus reconcile and executor tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…in reconcile collectObservedState indexed networks/volumes by compose label into a single-valued map, so two live resources sharing a label (e.g. a leftover after a rename) collided and the "winner" depended on the daemon's list order — a nondeterministic `up` (spurious create/recreate events, possible churn) on subsequent runs. Make collection lossless and move the selection into the reconciler: - ObservedState.Networks/Volumes become map[string][]Observed*: collection records every label-sharing resource and makes no premature choice. - selectNetwork/selectVolume deterministically pick the resource matching the desired name (else the lexicographically smallest), returning the others as orphans. - reconcile resolves the observed state once (resolveObserved) into single-valued resolvedNetworks/resolvedVolumes used everywhere, and warns about orphans instead of acting on them — they are left untouched because removing them could drop data or break unrelated workloads. Adds selection unit tests, a collector aggregation test and a reconcile conflict test (deterministic no-op + orphan warning across list orders). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
01380dd to
700a83d
Compare
Follow-up to #13962 (volumes), applying the same treatment to networks.
What I did
Move network divergence detection and recreation out of the imperative
pre-reconcile path (
ensureNetwork/resolveOrCreateNetwork/removeDivergedNetwork)into the reconciliation plan.
reconcileNetworksowns creation of missing networks and, for a divergednetwork, an explicit recreation sequence — no user confirmation, since
recreating a network is not destructive:
stop containers → disconnect → remove network → create network → reconnect.Attached containers keep their identity (reconnected, not recreated), matching
the previous behavior. If a container is independently recreated by
reconcileContainers, its removal is ordered after the reconnect so they don't race.collectObservedStatediscovers legacy/unlabeled networks by name and recordsthem as unmanaged matches (empty config-hash) so the reconciler reuses them
untouched; ownership warnings move to
warnUnmanagedNetworks.checkExternalNetworkskeeps external-network validation/resolution.execCreateNetworknow issues a plaincreateNetwork; the imperativeensureNetwork/resolveOrCreateNetwork/removeDivergedNetworkand theconnect/disconnect helpers are removed.
Reconcile, observed-state and executor tests cover network create/diverge/rename,
the entangled diverge+recreate case, legacy by-name discovery and the ownership warnings.
Network rename behavior (intentional change)
A network rename is handled as a recreation: the old network is removed, the new
one created, and attached containers are migrated onto it. This differs
slightly from previous Compose releases, which created the new network and left
existing containers attached to the old one (they'd only move once recreated for
another reason). We chose migration for the more logical outcome — after the next
up, the service runs on the network it declares.Because networks carry no data (unlike volumes, where the old volume is kept),
removing the previous network is safe and, importantly, keeps subsequent runs
deterministic: it avoids leaving two networks sharing the same compose label.
The volume/network asymmetry (volume rename keeps the old volume, network rename
removes it) is intentional and reflects that only volumes hold data worth preserving.
🤖 Generated with Claude Code