fix(relay): reconnect to new networks when routes are unchanged - #14236
Conversation
relayIdentity only hashes image+routes, never network topology. When a dependent service is added after the relay is already up, on a network the relay isn't attached to, the relay's identity is unaffected -- so ensureServiceRelay's running-container fast path returned nil without ever looking at whether networkKeys had grown. The new consumer could never reach the provider through it, silently: no error, just an unreachable compose-native address. ensureRelayNetworks now runs on every reuse path (running, stopped, paused) before the state-specific branch decides what to do with the container, connecting whichever networks in networkKeys the relay isn't already on -- reusing NetworkSettings from the same ContainerList call findRelayContainer already made, no extra inspect. Networks the relay is already connected to are left untouched. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
| }); err != nil { | ||
| return fmt.Errorf("connect relay for service %s to network %s: %w", service.Name, netName, err) | ||
| } |
There was a problem hiding this comment.
[medium] NetworkConnect "endpoint already exists" error not tolerated — spurious failure under concurrent reconciliation
ensureRelayNetworks builds its connected map from the NetworkSettings returned by ContainerList in findRelayContainer. Because compose runs ensureServiceRelay concurrently per provider service, a second goroutine may have already connected the relay to the same network in the window between that snapshot and the NetworkConnect call. When that happens the daemon returns an "endpoint already exists in network" error, which is unconditionally wrapped and returned as a fatal error (line 227), even though the relay is actually in the desired state.
errdefs is already imported (line 31) and used for IsNotFound (line 323). The idiomatic fix is to treat an "already exists" / conflict response as a no-op, consistent with the rest of the relay reconciliation:
| }); err != nil { | |
| return fmt.Errorf("connect relay for service %s to network %s: %w", service.Name, netName, err) | |
| } | |
| }); err != nil && !errdefs.IsConflict(err) { | |
| return fmt.Errorf("connect relay for service %s to network %s: %w", service.Name, netName, err) | |
| } |
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ensureServiceRelay runs concurrently per provider service (the shared project mutex is released before this Docker API work). Two goroutines reconciling relays that happen to share a consumer network can both decide, from their own ContainerList snapshot, that the same relay still needs connecting to it. Whichever runs second got the daemon's "endpoint already exists" back from NetworkConnect, wrapped and returned as a fatal error even though the relay was already in the desired state. Tolerate errdefs.IsConflict the same way the rest of the relay/network reconciliation already does, instead of failing the run over a race that already resolved to the right outcome. (docker-agent review on docker#14236) Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This review covers only the commits pushed since
a5e0adc4476c8c05b963305b6662aeb6d3ecdb00.
The incremental change is a single targeted fix: now tolerates an "endpoint already exists" conflict from the Docker daemon when connecting a relay container to a network. This correctly handles the race between concurrent goroutines, where a second goroutine may connect the relay to the same network in the window between the snapshot and the call. Using from github.com/containerd/errdefs (already imported at line 31) is the correct predicate for the daemon's HTTP 409 response. The fix is well-scoped, the comment is accurate, and no issues are introduced.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This review covers only the commits pushed since
a5e0adc4476c8c05b963305b6662aeb6d3ecdb00.
The incremental change is a single targeted fix: ensureRelayNetworks now tolerates an "endpoint already exists" conflict from the Docker daemon when connecting a relay container to a network. This correctly handles the race between concurrent ensureServiceRelay goroutines, where a second goroutine may connect the relay to the same network in the window between the ContainerList snapshot and the NetworkConnect call. Using errdefs.IsConflict from github.com/containerd/errdefs (already imported at line 31) is the correct predicate for the daemon's HTTP 409 response. The fix is well-scoped, the comment is accurate, and no issues are introduced.
What this PR does, in one sentence
A relay whose identity is still up to date now also gets connected to any network a newly added dependent service needs, instead of silently staying unreachable from it.
Context
relayIdentity(pkg/compose/relay.go) hashes the relay's image and routes only — not the set of networks it's supposed to join.ensureServiceRelayuses that identity to decide whether an existing relay container can be reused as-is: when it matches and the container is already running, it returns immediately.That's correct for image/route changes, but network membership can change independently: a service added later, depending on the same provider, on a network the relay was never attached to. Since neither the image nor the routes changed, the identity still matches, and the running-container fast path returned without ever looking at whether the relay's current networks still covered every consumer. The new service could never reach the provider at its compose-native address — no error, just silent unreachability.
What this PR brings
ensureRelayNetworksnow runs on every reuse path (running, stopped, paused) before the state-specific branch decides what to do with the container: it connects whichever networks the relay isn't already on, using the network membershipfindRelayContainer'sContainerListcall already returned (no extra inspect round-trip). Networks the relay is already connected to are left untouched.Two tests lock this in:
ensureRelayNetworksin isolation (connects only the missing network), andensureServiceRelayend to end (a running relay with a matching identity gets connected to a new network without being recreated — noContainerCreate/ContainerStartexpected).🤖 Generated with Claude Code