From dbf2692d72f93213b00077d136880e8b2f46148b Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 23 Jul 2026 22:05:51 +0200 Subject: [PATCH 1/5] feat(reconcile): model network lifecycle in the plan Mirror the volume reconciliation work (#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) Signed-off-by: Nicolas De Loof --- pkg/compose/create.go | 238 +++++++---------------------- pkg/compose/executor_ops.go | 3 +- pkg/compose/executor_test.go | 79 +++++++++- pkg/compose/observed_state.go | 42 +++++ pkg/compose/observed_state_test.go | 110 ++++++++++++- pkg/compose/reconcile.go | 175 +++++++++++++-------- pkg/compose/reconcile_test.go | 179 ++++++++++++++++------ 7 files changed, 515 insertions(+), 311 deletions(-) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 434ae7b0e8c..04b8dd9e599 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -86,8 +86,7 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt } prepareNetworks(project) - - networks, err := s.ensureNetworks(ctx, project) + externalNetworks, err := s.checkExternalNetworks(ctx, project) if err != nil { return err } @@ -108,8 +107,9 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt if err != nil { return err } - observed.setResolvedNetworks(networks, project) + observed.setResolvedNetworks(externalNetworks, project) observed.setResolvedVolumes(externalVolumes) + warnUnmanagedNetworks(project, observed) warnUnmanagedVolumes(project, observed) if len(observed.Orphans) > 0 && !options.IgnoreOrphans && !options.RemoveOrphans { @@ -141,17 +141,51 @@ func prepareNetworks(project *types.Project) { } } -func (s *composeService) ensureNetworks(ctx context.Context, project *types.Project) (map[string]string, error) { - networks := map[string]string{} - for name, nw := range project.Networks { - id, err := s.ensureNetwork(ctx, project, name, &nw) +// checkExternalNetworks validates that every external network exists and returns +// their resolved IDs. External networks carry no compose label and are therefore +// absent from the label-scoped observed state, so the reconciler needs them +// injected via setResolvedNetworks. +// +// Managed and legacy (unlabeled, name-matched) networks are discovered by +// collectObservedState; their lifecycle is owned by the reconciliation plan, so +// this function performs no mutation on them. +func (s *composeService) checkExternalNetworks(ctx context.Context, project *types.Project) (map[string]string, error) { + external := map[string]string{} + for k, nw := range project.Networks { + if !nw.External { + continue + } + id, err := s.resolveExternalNetwork(ctx, &nw) if err != nil { return nil, err } - networks[name] = id - project.Networks[name] = nw + external[k] = id + } + return external, nil +} + +// warnUnmanagedNetworks warns about declared networks backed by a live network +// this project does not own — either created outside Compose (no project label) +// or by another project. Such networks are matched by name and reused untouched +// (see discoverUnmanagedNetworks); the warning tells the user to set +// `external: true` to make the intent explicit. +func warnUnmanagedNetworks(project *types.Project, observed *ObservedState) { + for k, nw := range project.Networks { + if nw.External { + continue + } + obs, ok := observed.Networks[k] + if !ok || obs.ProjectName == project.Name { + continue + } + if obs.ProjectName == "" { + logrus.Warnf("a network with name %s exists but was not created by compose.\n"+ + "Set `external: true` to use an existing network", nw.Name) + } else { + logrus.Warnf("a network with name %s exists but was not created for project %q.\n"+ + "Set `external: true` to use an existing network", nw.Name, project.Name) + } } - return networks, nil } // prepareVolumes injects the compose-managed labels onto every project volume so @@ -1363,102 +1397,18 @@ func buildImageOptions(image *types.ServiceVolumeImage) *mount.ImageOptions { } } -func (s *composeService) ensureNetwork(ctx context.Context, project *types.Project, name string, n *types.NetworkConfig) (string, error) { - if n.External { - return s.resolveExternalNetwork(ctx, n) - } - - id, err := s.resolveOrCreateNetwork(ctx, project, name, n) - if errdefs.IsConflict(err) { - // Maybe another execution of `docker compose up|run` created same network - // let's retry once - return s.resolveOrCreateNetwork(ctx, project, name, n) - } - return id, err -} - -func (s *composeService) resolveOrCreateNetwork(ctx context.Context, project *types.Project, name string, n *types.NetworkConfig) (string, error) { //nolint:gocyclo - // This is containers that could be left after a diverged network was removed - var dangledContainers Containers - - // First, try to find a unique network matching by name or ID - res, err := s.apiClient().NetworkInspect(ctx, n.Name, client.NetworkInspectOptions{}) - if err == nil { - inspect := res.Network - // NetworkInspect will match on ID prefix, so double check we get the expected one - // as looking for network named `db` we could erroneously match network ID `db9086999caf` - if inspect.Name == n.Name || inspect.ID == n.Name { - p, ok := inspect.Labels[api.ProjectLabel] - if !ok { - logrus.Warnf("a network with name %s exists but was not created by compose.\n"+ - "Set `external: true` to use an existing network", n.Name) - } else if p != project.Name { - logrus.Warnf("a network with name %s exists but was not created for project %q.\n"+ - "Set `external: true` to use an existing network", n.Name, project.Name) - } - if inspect.Labels[api.NetworkLabel] != name { - return "", fmt.Errorf( - "network %s was found but has incorrect label %s set to %q (expected: %q)", - n.Name, - api.NetworkLabel, - inspect.Labels[api.NetworkLabel], - name, - ) - } - - hash := inspect.Labels[api.ConfigHashLabel] - expected, err := NetworkHash(n) - if err != nil { - return "", err - } - if hash == "" || hash == expected { - return inspect.ID, nil - } - - dangledContainers, err = s.removeDivergedNetwork(ctx, project, name, n) - if err != nil { - return "", err - } - } - } - // ignore other errors. Typically, an ambiguous request by name results in some generic `invalidParameter` error - - // Either not found, or name is ambiguous - use NetworkList to list by name - nwList, err := s.apiClient().NetworkList(ctx, client.NetworkListOptions{ - Filters: make(client.Filters).Add("name", n.Name), - }) - if err != nil { - return "", err - } - - // NetworkList Matches all or part of a network name, so we have to filter for a strict match - networks := slices.DeleteFunc(nwList.Items, func(net network.Summary) bool { - return net.Name != n.Name - }) - - for _, nw := range networks { - if nw.Labels[api.ProjectLabel] == project.Name && - nw.Labels[api.NetworkLabel] == name { - return nw.ID, nil - } - } - - // we could have set NetworkList with a projectFilter and networkFilter but not doing so allows to catch this - // scenario were a network with same name exists but doesn't have label, and use of `CheckDuplicate: true` - // prevents to create another one. - if len(networks) > 0 { - logrus.Warnf("a network with name %s exists but was not created by compose.\n"+ - "Set `external: true` to use an existing network", n.Name) - return networks[0].ID, nil - } - +// createNetwork creates the given (managed) network with its compose labels and +// config-hash. It is executed as a plan operation (OpCreateNetwork); resolution +// of external networks lives in resolveExternalNetwork, and reuse of legacy +// name-matched networks is decided by the reconciler from the observed state. +func (s *composeService) createNetwork(ctx context.Context, n *types.NetworkConfig) error { var ipam *network.IPAM if n.Ipam.Config != nil { var config []network.IPAMConfig for _, pool := range n.Ipam.Config { c, err := parseIPAMPool(pool) if err != nil { - return "", err + return err } config = append(config, c) } @@ -1469,7 +1419,7 @@ func (s *composeService) resolveOrCreateNetwork(ctx context.Context, project *ty } hash, err := NetworkHash(n) if err != nil { - return "", err + return err } n.CustomLabels = n.CustomLabels.Add(api.ConfigHashLabel, hash) createOpts := client.NetworkCreateOptions{ @@ -1494,7 +1444,7 @@ func (s *composeService) resolveOrCreateNetwork(ctx context.Context, project *ty for _, ipamConfig := range n.Ipam.Config { c, err := parseIPAMPool(ipamConfig) if err != nil { - return "", err + return err } createOpts.IPAM.Config = append(createOpts.IPAM.Config, c) } @@ -1502,91 +1452,11 @@ func (s *composeService) resolveOrCreateNetwork(ctx context.Context, project *ty networkEventName := fmt.Sprintf("Network %s", n.Name) s.events.On(creatingEvent(networkEventName)) - resp, err := s.apiClient().NetworkCreate(ctx, n.Name, createOpts) - if err != nil { + if _, err := s.apiClient().NetworkCreate(ctx, n.Name, createOpts); err != nil { s.events.On(errorEvent(networkEventName, err.Error())) - return "", fmt.Errorf("failed to create network %s: %w", n.Name, err) + return fmt.Errorf("failed to create network %s: %w", n.Name, err) } s.events.On(createdEvent(networkEventName)) - - err = s.connectNetwork(ctx, n.Name, dangledContainers, nil) - if err != nil { - return "", err - } - - return resp.ID, nil -} - -func (s *composeService) removeDivergedNetwork(ctx context.Context, project *types.Project, name string, n *types.NetworkConfig) (Containers, error) { - // Remove services attached to this network to force recreation - var services []string - for _, service := range project.Services.Filter(func(config types.ServiceConfig) bool { - _, ok := config.Networks[name] - return ok - }) { - services = append(services, service.Name) - } - - // Stop containers so we can remove network - // They will be restarted (actually: recreated) with the updated network - err := s.stop(ctx, project.Name, api.StopOptions{ - Services: services, - Project: project, - }, nil) - if err != nil { - return nil, err - } - - containers, err := s.getContainers(ctx, project.Name, oneOffExclude, true, services...) - if err != nil { - return nil, err - } - - err = s.disconnectNetwork(ctx, n.Name, containers) - if err != nil { - return nil, err - } - - _, err = s.apiClient().NetworkRemove(ctx, n.Name, client.NetworkRemoveOptions{}) - eventName := fmt.Sprintf("Network %s", n.Name) - s.events.On(removedEvent(eventName)) - return containers, err -} - -func (s *composeService) disconnectNetwork( - ctx context.Context, - nwName string, - containers Containers, -) error { - for _, c := range containers { - _, err := s.apiClient().NetworkDisconnect(ctx, nwName, client.NetworkDisconnectOptions{ - Container: c.ID, - Force: true, - }) - if err != nil { - return err - } - } - - return nil -} - -func (s *composeService) connectNetwork( - ctx context.Context, - nwName string, - containers Containers, - config *network.EndpointSettings, -) error { - for _, c := range containers { - _, err := s.apiClient().NetworkConnect(ctx, nwName, client.NetworkConnectOptions{ - Container: c.ID, - EndpointConfig: config, - }) - if err != nil { - return err - } - } - return nil } diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index affe717f6e8..8affbe2b715 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -31,8 +31,7 @@ import ( // --- Network operations --- func (exec *planExecutor) execCreateNetwork(ctx context.Context, op Operation) error { - _, err := exec.compose.ensureNetwork(ctx, exec.project, op.Name, op.Network) - return err + return exec.compose.createNetwork(ctx, op.Network) } func (exec *planExecutor) execRemoveNetwork(ctx context.Context, op Operation) error { diff --git a/pkg/compose/executor_test.go b/pkg/compose/executor_test.go index 2228361c438..019deb47133 100644 --- a/pkg/compose/executor_test.go +++ b/pkg/compose/executor_test.go @@ -65,11 +65,8 @@ func TestExecutePlanCreateNetwork(t *testing.T) { Networks: types.Networks{"default": nw}, } - // ensureNetwork: inspect → not found, list → empty, create - apiClient.EXPECT().NetworkInspect(gomock.Any(), "test_default", gomock.Any()). - Return(client.NetworkInspectResult{}, notFoundError{}) - apiClient.EXPECT().NetworkList(gomock.Any(), gomock.Any()). - Return(client.NetworkListResult{}, nil) + // createNetwork issues a plain NetworkCreate (divergence/reuse decisions are + // made by the reconciler from the observed state, not here). apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). Return(client.NetworkCreateResult{ID: "net1"}, nil) @@ -316,6 +313,78 @@ func TestExecutePlanRecreateVolume(t *testing.T) { assert.NilError(t, err) } +// TestExecutePlanRecreateNetwork drives a network recreation — stop container → +// disconnect → remove network → create network → connect — end to end through +// the executor, asserting each Docker API call fires. The container keeps its +// identity (it is reconnected, not recreated). +func TestExecutePlanRecreateNetwork(t *testing.T) { + svc, apiClient := newTestService(t) + + ctr := container.Summary{ + ID: "c1", + Names: []string{"/recreate-web-1"}, + Labels: map[string]string{ + api.ServiceLabel: "web", + api.ContainerNumberLabel: "1", + }, + } + + apiClient.EXPECT().ContainerStop(gomock.Any(), "c1", gomock.Any()). + Return(client.ContainerStopResult{}, nil) + apiClient.EXPECT().NetworkDisconnect(gomock.Any(), "recreate_frontend", gomock.Any()). + Return(client.NetworkDisconnectResult{}, nil) + apiClient.EXPECT().NetworkRemove(gomock.Any(), "recreate_frontend", gomock.Any()). + Return(client.NetworkRemoveResult{}, nil) + apiClient.EXPECT().NetworkCreate(gomock.Any(), "recreate_frontend", gomock.Any()). + Return(client.NetworkCreateResult{ID: "net2"}, nil) + apiClient.EXPECT().NetworkConnect(gomock.Any(), "recreate_frontend", gomock.Any()). + Return(client.NetworkConnectResult{}, nil) + + nw := types.NetworkConfig{Name: "recreate_frontend", Driver: "overlay"} + project := &types.Project{ + Name: "recreate", + Networks: types.Networks{"frontend": nw}, + } + + plan := &Plan{} + stopNode := plan.addNode(Operation{ + Type: OpStopContainer, + ResourceID: "service:web:1", + Cause: "network frontend config changed", + Container: &ctr, + }, "") + disconnectNode := plan.addNode(Operation{ + Type: OpDisconnectNetwork, + ResourceID: "service:web:1", + Cause: "network frontend recreate", + Container: &ctr, + Name: nw.Name, + }, "", stopNode) + removeNode := plan.addNode(Operation{ + Type: OpRemoveNetwork, + ResourceID: "network:frontend", + Cause: "config hash diverged", + Name: nw.Name, + }, "", disconnectNode) + createNode := plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: "network:frontend", + Cause: "recreate after config change", + Name: nw.Name, + Network: &nw, + }, "", removeNode) + plan.addNode(Operation{ + Type: OpConnectNetwork, + ResourceID: "service:web:1", + Cause: "network frontend recreate", + Container: &ctr, + Name: nw.Name, + }, "", createNode) + + err := svc.executePlan(t.Context(), project, emptyObservedState("recreate"), plan) + assert.NilError(t, err) +} + // notFoundError implements the errdefs.ErrNotFound interface for test mocks. type notFoundError struct{} diff --git a/pkg/compose/observed_state.go b/pkg/compose/observed_state.go index 8379803f0b6..80f4b6b94cc 100644 --- a/pkg/compose/observed_state.go +++ b/pkg/compose/observed_state.go @@ -156,6 +156,10 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type } } + if err := s.discoverUnmanagedNetworks(ctx, project, state); err != nil { + return nil, err + } + if err := s.discoverUnmanagedVolumes(ctx, project, state); err != nil { return nil, err } @@ -163,6 +167,44 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type return state, nil } +// discoverUnmanagedNetworks augments the observed state with networks that match +// a declared network by name but carry no compose label — pre-label Compose or +// manually created networks, missed by the label-filtered NetworkList. Each is +// recorded as an unmanaged match with an empty ConfigHash: the reconciler then +// reuses it untouched instead of scheduling a CreateNetwork. See +// warnUnmanagedNetworks for the accompanying user warning. +func (s *composeService) discoverUnmanagedNetworks(ctx context.Context, project *types.Project, state *ObservedState) error { + for _, key := range project.NetworkNames() { + nw := project.Networks[key] + if nw.External { + continue + } + if _, ok := state.Networks[key]; ok { + continue + } + inspected, err := s.apiClient().NetworkInspect(ctx, nw.Name, client.NetworkInspectOptions{}) + if err != nil { + if errdefs.IsNotFound(err) { + continue // absent: it will be created by the reconciliation plan + } + return err + } + // NetworkInspect matches on ID prefix, so guard against a partial match + // (e.g. a network whose ID starts with the requested name). + if inspected.Network.Name != nw.Name && inspected.Network.ID != nw.Name { + continue + } + state.Networks[key] = ObservedNetwork{ + ID: inspected.Network.ID, + Name: inspected.Network.Name, + ProjectName: inspected.Network.Labels[api.ProjectLabel], + // ConfigHash intentionally left empty: the network is not owned by + // this project, so we must not treat it as diverged and recreate it. + } + } + return nil +} + // discoverUnmanagedVolumes augments the observed state with volumes that match a // declared volume by name but carry no compose label — pre-label Compose or // manually created volumes, missed by the label-filtered VolumeList. Each is diff --git a/pkg/compose/observed_state_test.go b/pkg/compose/observed_state_test.go index f6a119c0eb1..94ca256cb72 100644 --- a/pkg/compose/observed_state_test.go +++ b/pkg/compose/observed_state_test.go @@ -205,9 +205,9 @@ func TestCollectObservedState(t *testing.T) { assert.Equal(t, vol.ConfigHash, "volhash1") } -// collectVolumesOnly mocks empty container/network/volume lists so that only the -// legacy by-name volume discovery is exercised. -func collectVolumesOnly(t *testing.T, project *types.Project, inspect func(apiClient *mocks.MockAPIClient)) (*ObservedState, error) { +// collectByNameDiscovery mocks empty container/network/volume lists so that only +// the legacy by-name network/volume discovery is exercised. +func collectByNameDiscovery(t *testing.T, project *types.Project, inspect func(apiClient *mocks.MockAPIClient)) (*ObservedState, error) { t.Helper() svc, apiClient := newTestService(t) apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{}, nil) @@ -217,13 +217,109 @@ func collectVolumesOnly(t *testing.T, project *types.Project, inspect func(apiCl return svc.collectObservedState(t.Context(), project) } +// TestCollectObservedState_LegacyNetworkMatchedByName verifies that a network +// matching a declared network by name but carrying no compose label is recorded +// as an unmanaged match with an empty ConfigHash, so the reconciler reuses it. +func TestCollectObservedState_LegacyNetworkMatchedByName(t *testing.T) { + project := &types.Project{Name: "myproject", Networks: types.Networks{"frontend": {Name: "myproject_frontend"}}} + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { + apiClient.EXPECT().NetworkInspect(gomock.Any(), "myproject_frontend", gomock.Any()).Return(client.NetworkInspectResult{ + Network: network.Inspect{Network: network.Network{ID: "net1", Name: "myproject_frontend"}}, + }, nil) + }) + assert.NilError(t, err) + obs, ok := state.Networks["frontend"] + assert.Assert(t, ok, "legacy network must be discovered by name") + assert.Equal(t, obs.ID, "net1") + assert.Equal(t, obs.Name, "myproject_frontend") + assert.Equal(t, obs.ProjectName, "") + assert.Equal(t, obs.ConfigHash, "", "unmanaged match must have an empty config hash") +} + +// TestCollectObservedState_ForeignProjectNetworkMatchedByName verifies that a +// network owned by another project but matching the declared name is recorded +// with an empty ConfigHash (reused untouched) and keeps the foreign project name. +func TestCollectObservedState_ForeignProjectNetworkMatchedByName(t *testing.T) { + project := &types.Project{Name: "myproject", Networks: types.Networks{"frontend": {Name: "shared_net"}}} + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { + apiClient.EXPECT().NetworkInspect(gomock.Any(), "shared_net", gomock.Any()).Return(client.NetworkInspectResult{ + Network: network.Inspect{Network: network.Network{ID: "net9", Name: "shared_net", Labels: map[string]string{ + api.ProjectLabel: "otherproject", + api.ConfigHashLabel: "foreignhash", + }}}, + }, nil) + }) + assert.NilError(t, err) + obs := state.Networks["frontend"] + assert.Equal(t, obs.ProjectName, "otherproject") + assert.Equal(t, obs.ConfigHash, "", "foreign network must not be treated as diverged") +} + +// TestCollectObservedState_NetworkNotFoundByName verifies a declared network with +// no live counterpart is left absent so the reconciler schedules a create. +func TestCollectObservedState_NetworkNotFoundByName(t *testing.T) { + project := &types.Project{Name: "myproject", Networks: types.Networks{"frontend": {Name: "myproject_frontend"}}} + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { + apiClient.EXPECT().NetworkInspect(gomock.Any(), "myproject_frontend", gomock.Any()).Return(client.NetworkInspectResult{}, notFoundError{}) + }) + assert.NilError(t, err) + _, ok := state.Networks["frontend"] + assert.Assert(t, !ok, "absent network must not be in observed state") +} + +// TestCollectObservedState_ExternalNetworkNotInspectedByName verifies external +// networks are excluded from the legacy by-name discovery (no NetworkInspect). +func TestCollectObservedState_ExternalNetworkNotInspectedByName(t *testing.T) { + project := &types.Project{Name: "myproject", Networks: types.Networks{"frontend": {Name: "ext_net", External: true}}} + state, err := collectByNameDiscovery(t, project, func(_ *mocks.MockAPIClient) {}) + assert.NilError(t, err) + _, ok := state.Networks["frontend"] + assert.Assert(t, !ok) +} + +// TestWarnUnmanagedNetworks verifies the legacy ownership warnings for networks +// reused by name, and silence for managed/external/absent networks. +func TestWarnUnmanagedNetworks(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Networks: types.Networks{ + "managed": {Name: "myproject_managed"}, + "unlabel": {Name: "unlabel_net"}, + "foreign": {Name: "foreign_net"}, + "external": {Name: "ext_net", External: true}, + "tocreate": {Name: "myproject_tocreate"}, + }, + } + observed := &ObservedState{ + Networks: map[string]ObservedNetwork{ + "managed": {Name: "myproject_managed", ProjectName: "myproject", ConfigHash: "h"}, + "unlabel": {Name: "unlabel_net", ProjectName: ""}, + "foreign": {Name: "foreign_net", ProjectName: "otherproject"}, + "external": {Name: "ext_net"}, + }, + } + + hook := logrustest.NewGlobal() + warnUnmanagedNetworks(project, observed) + + var msgs []string + for _, e := range hook.AllEntries() { + assert.Equal(t, e.Level, logrus.WarnLevel) + msgs = append(msgs, e.Message) + } + assert.Equal(t, len(msgs), 2, "expected exactly two warnings, got: %v", msgs) + joined := strings.Join(msgs, "\n") + assert.Assert(t, strings.Contains(joined, "a network with name unlabel_net exists but was not created by compose"), joined) + assert.Assert(t, strings.Contains(joined, `a network with name foreign_net exists but was not created for project "myproject"`), joined) +} + // TestCollectObservedState_LegacyVolumeMatchedByName verifies that a volume that // matches a declared volume by name but carries no compose label (pre-label // Compose or manually created) is recorded as an unmanaged match with an empty // ConfigHash, so the reconciler reuses it untouched. func TestCollectObservedState_LegacyVolumeMatchedByName(t *testing.T) { project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": {Name: "myproject_data"}}} - state, err := collectVolumesOnly(t, project, func(apiClient *mocks.MockAPIClient) { + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { apiClient.EXPECT().VolumeInspect(gomock.Any(), "myproject_data", gomock.Any()).Return(client.VolumeInspectResult{ Volume: volume.Volume{Name: "myproject_data", Driver: "local"}, }, nil) @@ -242,7 +338,7 @@ func TestCollectObservedState_LegacyVolumeMatchedByName(t *testing.T) { // foreign project name for the warning. func TestCollectObservedState_ForeignProjectVolumeMatchedByName(t *testing.T) { project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": {Name: "shared_data"}}} - state, err := collectVolumesOnly(t, project, func(apiClient *mocks.MockAPIClient) { + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { apiClient.EXPECT().VolumeInspect(gomock.Any(), "shared_data", gomock.Any()).Return(client.VolumeInspectResult{ Volume: volume.Volume{Name: "shared_data", Driver: "local", Labels: map[string]string{ api.ProjectLabel: "otherproject", @@ -260,7 +356,7 @@ func TestCollectObservedState_ForeignProjectVolumeMatchedByName(t *testing.T) { // with no live counterpart is left absent so the reconciler schedules a create. func TestCollectObservedState_VolumeNotFoundByName(t *testing.T) { project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": {Name: "myproject_data"}}} - state, err := collectVolumesOnly(t, project, func(apiClient *mocks.MockAPIClient) { + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { apiClient.EXPECT().VolumeInspect(gomock.Any(), "myproject_data", gomock.Any()).Return(client.VolumeInspectResult{}, notFoundError{}) }) assert.NilError(t, err) @@ -273,7 +369,7 @@ func TestCollectObservedState_VolumeNotFoundByName(t *testing.T) { // gomock would fail on an unexpected call). func TestCollectObservedState_ExternalVolumeNotInspectedByName(t *testing.T) { project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": {Name: "ext_data", External: true}}} - state, err := collectVolumesOnly(t, project, func(_ *mocks.MockAPIClient) {}) + state, err := collectByNameDiscovery(t, project, func(_ *mocks.MockAPIClient) {}) assert.NilError(t, err) _, ok := state.Volumes["data"] assert.Assert(t, !ok) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index 64aee5bc5ad..d22ba814c65 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -82,6 +82,12 @@ type reconciler struct { // one against an already-stopped container. stoppedByPlan map[string]*PlanNode // container ID → existing Stop node + // connectNodes records the OpConnectNetwork nodes emitted for a container by + // planRecreateNetworks (reconnecting it to a freshly recreated network). If + // reconcileContainers later recreates the same container, its RemoveContainer + // must wait for these reconnects so they don't race the removal. + connectNodes map[string][]*PlanNode // container ID → reconnect nodes + // recreatedServices is the set of services with at least one container // scheduled for recreation in the current plan. Services iterate in // dependency order, so by the time a dependent is evaluated, all its @@ -110,6 +116,7 @@ func reconcile(_ context.Context, project *types.Project, observed *ObservedStat volumeNodes: map[string]*PlanNode{}, serviceNodes: map[string]*PlanNode{}, stoppedByPlan: map[string]*PlanNode{}, + connectNodes: map[string][]*PlanNode{}, recreatedServices: map[string]bool{}, observedContainersByService: observed.containersByService(), } @@ -133,8 +140,17 @@ func reconcile(_ context.Context, project *types.Project, observed *ObservedStat return r.plan, nil } -// reconcileNetworks adds plan nodes for network creation or recreation. +// reconcileNetworks plans the network lifecycle: creation of missing networks +// and, for networks whose configuration has diverged from the live resource, +// recreation. Unlike volumes, recreating a network is not destructive, so no +// user confirmation is required. +// +// Divergence is detected by comparing NetworkHash(desired) with the config-hash +// persisted on the live network (observed.ConfigHash). A network with no +// recorded hash (e.g. created by an older Compose or manually) is left +// untouched, matching the previous ensureNetwork behavior. func (r *reconciler) reconcileNetworks() error { + var diverged []string for _, key := range sortedKeys(r.project.Networks) { desired := r.project.Networks[key] if desired.External { @@ -142,92 +158,115 @@ func (r *reconciler) reconcileNetworks() error { } observed, exists := r.observed.Networks[key] if !exists { - r.planCreateNetwork(key, &desired) + r.planCreateNetwork(key, &desired, "not found") continue } - - expectedHash, err := NetworkHash(&desired) + expected, err := NetworkHash(&desired) if err != nil { return err } - if observed.ConfigHash != "" && observed.ConfigHash != expectedHash { - if err := r.planRecreateNetwork(key, &desired); err != nil { - return err - } + if observed.ConfigHash == "" || observed.ConfigHash == expected { + continue } - // else: network exists and config matches, nothing to do + if observed.Name != desired.Name { + // The network was renamed: the live network matched by label carries + // a different name, i.e. a distinct Docker resource. Create the new + // network and leave the old one untouched, matching the historical + // additive ensureNetwork behavior. + r.planCreateNetwork(key, &desired, "renamed") + continue + } + diverged = append(diverged, key) } + r.planRecreateNetworks(diverged) return nil } // planCreateNetwork adds a single CreateNetwork node and records it for dependency tracking. -func (r *reconciler) planCreateNetwork(key string, nw *types.NetworkConfig) *PlanNode { - node := r.plan.addNode(Operation{ +func (r *reconciler) planCreateNetwork(key string, nw *types.NetworkConfig, cause string) { + r.networkNodes[key] = r.plan.addNode(Operation{ Type: OpCreateNetwork, ResourceID: fmt.Sprintf("network:%s", key), - Cause: "not found", + Cause: cause, Name: nw.Name, Network: nw, }, "") - r.networkNodes[key] = node - return node } -// planRecreateNetwork adds the full sequence for a diverged network: -// stop affected containers → disconnect → remove network → create network. -func (r *reconciler) planRecreateNetwork(key string, nw *types.NetworkConfig) error { - observed := r.observed.Networks[key] - affectedServices := r.servicesUsingNetwork(key) - affectedContainers := r.containersForServices(affectedServices) - - // Stop all affected containers, recording each Stop node so that a later - // recreate of the same container does not emit a second Stop against a - // container that is already stopped. - var stopNodes []*PlanNode - for i := range affectedContainers { - oc := &affectedContainers[i] - node := r.plan.addNode(Operation{ - Type: OpStopContainer, - ResourceID: fmt.Sprintf("service:%s:%d", oc.Summary.Labels[api.ServiceLabel], oc.Number), - Cause: fmt.Sprintf("network %s config changed", key), - Container: &oc.Summary, - }, "") - stopNodes = append(stopNodes, node) - r.stoppedByPlan[oc.ID] = node - } +// planRecreateNetworks adds, for each diverged network, the sequence: +// +// stop containers → disconnect containers → remove network → create network → connect containers +// +// Attached containers must be disconnected before the network can be removed +// (Docker refuses to remove a network with active endpoints) and are reconnected +// to the fresh network afterwards — they keep their identity and are not +// recreated, matching the previous ensureNetwork/removeDivergedNetwork behavior. +// +// Stops are deduplicated through stoppedByPlan so a container attached to several +// diverged networks (or later recreated by reconcileContainers) is stopped once. +// Each reconnect is recorded in connectNodes so that, should reconcileContainers +// recreate the container for an unrelated reason, its removal is ordered after +// the reconnect instead of racing it. +func (r *reconciler) planRecreateNetworks(keys []string) { + for _, key := range keys { + observed := r.observed.Networks[key] + desired := r.project.Networks[key] + containers := r.containersForServices(r.servicesUsingNetwork(key)) + + // Stop then disconnect every attached container. + var disconnectNodes []*PlanNode + for i := range containers { + oc := &containers[i] + resID := fmt.Sprintf("service:%s:%d", oc.Summary.Labels[api.ServiceLabel], oc.Number) + stopNode, alreadyStopped := r.stoppedByPlan[oc.ID] + if !alreadyStopped { + stopNode = r.plan.addNode(Operation{ + Type: OpStopContainer, + ResourceID: resID, + Cause: fmt.Sprintf("network %s config changed", key), + Container: &oc.Summary, + Timeout: r.options.Timeout, + }, "") + r.stoppedByPlan[oc.ID] = stopNode + } + disconnectNodes = append(disconnectNodes, r.plan.addNode(Operation{ + Type: OpDisconnectNetwork, + ResourceID: resID, + Cause: fmt.Sprintf("network %s recreate", key), + Container: &oc.Summary, + Name: observed.Name, + }, "", stopNode)) + } - // Disconnect all affected containers from the *observed* network (each depends on its own stop) - var disconnectNodes []*PlanNode - for i, oc := range affectedContainers { - node := r.plan.addNode(Operation{ - Type: OpDisconnectNetwork, - ResourceID: fmt.Sprintf("service:%s:%d", oc.Summary.Labels[api.ServiceLabel], oc.Number), - Cause: fmt.Sprintf("network %s recreate", key), - Container: &affectedContainers[i].Summary, + removeNode := r.plan.addNode(Operation{ + Type: OpRemoveNetwork, + ResourceID: fmt.Sprintf("network:%s", key), + Cause: "config hash diverged", Name: observed.Name, - }, "", stopNodes[i]) - disconnectNodes = append(disconnectNodes, node) + }, "", disconnectNodes...) + createNode := r.plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: fmt.Sprintf("network:%s", key), + Cause: "recreate after config change", + Name: desired.Name, + Network: &desired, + }, "", removeNode) + r.networkNodes[key] = createNode + + // Reconnect every attached container to the fresh network. + for i := range containers { + oc := &containers[i] + resID := fmt.Sprintf("service:%s:%d", oc.Summary.Labels[api.ServiceLabel], oc.Number) + connectNode := r.plan.addNode(Operation{ + Type: OpConnectNetwork, + ResourceID: resID, + Cause: fmt.Sprintf("network %s recreate", key), + Container: &oc.Summary, + Name: desired.Name, + }, "", createNode) + r.connectNodes[oc.ID] = append(r.connectNodes[oc.ID], connectNode) + } } - - // Remove the *observed* network (depends on all disconnects) - removeNode := r.plan.addNode(Operation{ - Type: OpRemoveNetwork, - ResourceID: fmt.Sprintf("network:%s", key), - Cause: "config hash diverged", - Name: observed.Name, - }, "", disconnectNodes...) - - // Create network (depends on remove) - createNode := r.plan.addNode(Operation{ - Type: OpCreateNetwork, - ResourceID: fmt.Sprintf("network:%s", key), - Cause: "recreate after config change", - Name: nw.Name, - Network: nw, - }, "", removeNode) - r.networkNodes[key] = createNode - - return nil } // reconcileVolumes plans the volume lifecycle: creation of missing volumes and, @@ -818,6 +857,10 @@ func (r *reconciler) planRecreateContainer(service types.ServiceConfig, oc *Obse if alreadyStopped { removeDeps = append(removeDeps, createNode) } + // If planRecreateNetworks scheduled reconnects for this container (network + // recreation), let them complete before the old container is removed so the + // reconnect does not race the removal. + removeDeps = append(removeDeps, r.connectNodes[oc.ID]...) removeNode := r.plan.addNode(Operation{ Type: OpRemoveContainer, ResourceID: resID, diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index cb83956b440..2a5643216b9 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -132,34 +132,72 @@ func TestReconcileNetworks_ExternalSkipped(t *testing.T) { assert.Assert(t, plan.IsEmpty()) } +// networkAttachedContainer builds an observed container of `service` connected +// to `netID` and up to date (its config-hash matches), so a network divergence +// reconnects it rather than recreating it. +func networkAttachedContainer(t *testing.T, svc types.ServiceConfig, id string) ObservedContainer { + t.Helper() + hash := mustServiceHash(t, svc) + return ObservedContainer{ + ID: id, Number: 1, State: container.StateRunning, ConfigHash: hash, + ConnectedNetworks: map[string]string{"frontend": "net1"}, + Summary: container.Summary{ + ID: id, State: container.StateRunning, + Labels: map[string]string{api.ServiceLabel: svc.Name, api.ContainerNumberLabel: "1", api.ConfigHashLabel: hash}, + }, + } +} + +// TestReconcileNetworks_Diverged asserts the Option-A recreation sequence for a +// diverged network: attached containers are stopped and disconnected, the +// network is removed then recreated, and the same containers are reconnected — +// not recreated. func TestReconcileNetworks_Diverged(t *testing.T) { + web := types.ServiceConfig{Name: "web", Scale: intPtr(1), Networks: map[string]*types.ServiceNetworkConfig{"frontend": {}}} project := &types.Project{ - Name: "myproject", - Networks: types.Networks{ - "frontend": {Name: "myproject_frontend", Driver: "overlay"}, - }, - Services: types.Services{ - "web": { - Name: "web", - Scale: intPtr(1), - Networks: map[string]*types.ServiceNetworkConfig{"frontend": {}}, - }, - }, + Name: "myproject", + Networks: types.Networks{"frontend": {Name: "myproject_frontend", Driver: "overlay"}}, + Services: types.Services{"web": web}, } observed := &ObservedState{ ProjectName: "myproject", - Containers: map[string][]ObservedContainer{ - "web": {{ - ID: "c1aabbccddee", Number: 1, State: container.StateRunning, - Summary: container.Summary{ - ID: "c1aabbccddee", - Labels: map[string]string{ - api.ServiceLabel: "web", - api.ContainerNumberLabel: "1", - }, - }, - }}, + Containers: map[string][]ObservedContainer{"web": {networkAttachedContainer(t, web, "c1aabbccddee")}}, + Networks: map[string]ObservedNetwork{ + "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}, }, + Volumes: map[string]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + assert.Equal(t, plan.String(), strings.TrimSpace(` +[] -> #1 service:web:1, StopContainer, network frontend config changed +[1] -> #2 service:web:1, DisconnectNetwork, network frontend recreate +[2] -> #3 network:frontend, RemoveNetwork, config hash diverged +[3] -> #4 network:frontend, CreateNetwork, recreate after config change +[4] -> #5 service:web:1, ConnectNetwork, network frontend recreate +`)+"\n") +} + +// TestReconcileNetworks_DivergedAlsoRecreatesChangedContainer verifies the +// entangled case: when a container attached to a diverged network also has its +// own config changed, it is both reconnected (by the network recreate) and +// recreated (by reconcileContainers), with the reconnect ordered before the old +// container's removal so they don't race. +func TestReconcileNetworks_DivergedAlsoRecreatesChangedContainer(t *testing.T) { + web := types.ServiceConfig{Name: "web", Scale: intPtr(1), Networks: map[string]*types.ServiceNetworkConfig{"frontend": {}}} + project := &types.Project{ + Name: "myproject", + Networks: types.Networks{"frontend": {Name: "myproject_frontend", Driver: "overlay"}}, + Services: types.Services{"web": web}, + } + oc := networkAttachedContainer(t, web, "c1aabbccddee") + oc.ConfigHash = "stale" // service config changed too -> reconcileContainers recreates + oc.Summary.Labels[api.ConfigHashLabel] = "stale" + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{"web": {oc}}, Networks: map[string]ObservedNetwork{ "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}, }, @@ -169,16 +207,17 @@ func TestReconcileNetworks_Diverged(t *testing.T) { plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) assert.NilError(t, err) - // The recreate phase reuses the Stop from the network-recreate phase - // instead of emitting a second one against an already-stopped container. + // #5 is the reconnect of the old container; the recreate's RemoveContainer + // (#7) depends on it (in addition to Stop #1 and Create #6). assert.Equal(t, plan.String(), strings.TrimSpace(` [] -> #1 service:web:1, StopContainer, network frontend config changed [1] -> #2 service:web:1, DisconnectNetwork, network frontend recreate [2] -> #3 network:frontend, RemoveNetwork, config hash diverged [3] -> #4 network:frontend, CreateNetwork, recreate after config change -[4] -> #5 service:web:1, CreateContainer, config changed (tmpName) [recreate:web:1] -[1,5] -> #6 service:web:1, RemoveContainer, replaced by #5 [recreate:web:1] -[6] -> #7 service:web:1, RenameContainer, finalize recreate [recreate:web:1] +[4] -> #5 service:web:1, ConnectNetwork, network frontend recreate +[4] -> #6 service:web:1, CreateContainer, config changed (tmpName) [recreate:web:1] +[1,5,6] -> #7 service:web:1, RemoveContainer, replaced by #6 [recreate:web:1] +[7] -> #8 service:web:1, RenameContainer, finalize recreate [recreate:web:1] `)+"\n") } @@ -201,17 +240,13 @@ func TestReconcileNetworks_DivergedMultipleServices(t *testing.T) { }, }, } + apiSvc := project.Services["api"] + web := project.Services["web"] observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{ - "web": {{ - ID: "c1aabbccddee", Number: 1, State: container.StateRunning, - Summary: container.Summary{ID: "c1aabbccddee", Labels: map[string]string{api.ServiceLabel: "web", api.ContainerNumberLabel: "1"}}, - }}, - "api": {{ - ID: "c2aabbccddee", Number: 1, State: container.StateRunning, - Summary: container.Summary{ID: "c2aabbccddee", Labels: map[string]string{api.ServiceLabel: "api", api.ContainerNumberLabel: "1"}}, - }}, + "web": {networkAttachedContainer(t, web, "c1aabbccddee")}, + "api": {networkAttachedContainer(t, apiSvc, "c2aabbccddee")}, }, Networks: map[string]ObservedNetwork{ "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}, @@ -222,24 +257,67 @@ func TestReconcileNetworks_DivergedMultipleServices(t *testing.T) { plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) assert.NilError(t, err) - // Services sorted alphabetically: api before web. Each service's recreate - // reuses the Stop from the network-recreate phase (no second Stop). + // Services sorted alphabetically: api before web. Each attached container is + // stopped, disconnected and reconnected — not recreated (config unchanged). assert.Equal(t, plan.String(), strings.TrimSpace(` [] -> #1 service:api:1, StopContainer, network frontend config changed -[] -> #2 service:web:1, StopContainer, network frontend config changed -[1] -> #3 service:api:1, DisconnectNetwork, network frontend recreate -[2] -> #4 service:web:1, DisconnectNetwork, network frontend recreate -[3,4] -> #5 network:frontend, RemoveNetwork, config hash diverged +[1] -> #2 service:api:1, DisconnectNetwork, network frontend recreate +[] -> #3 service:web:1, StopContainer, network frontend config changed +[3] -> #4 service:web:1, DisconnectNetwork, network frontend recreate +[2,4] -> #5 network:frontend, RemoveNetwork, config hash diverged [5] -> #6 network:frontend, CreateNetwork, recreate after config change -[6] -> #7 service:api:1, CreateContainer, config changed (tmpName) [recreate:api:1] -[1,7] -> #8 service:api:1, RemoveContainer, replaced by #7 [recreate:api:1] -[8] -> #9 service:api:1, RenameContainer, finalize recreate [recreate:api:1] -[6] -> #10 service:web:1, CreateContainer, config changed (tmpName) [recreate:web:1] -[2,10] -> #11 service:web:1, RemoveContainer, replaced by #10 [recreate:web:1] -[11] -> #12 service:web:1, RenameContainer, finalize recreate [recreate:web:1] +[6] -> #7 service:api:1, ConnectNetwork, network frontend recreate +[6] -> #8 service:web:1, ConnectNetwork, network frontend recreate +`)+"\n") +} + +// TestReconcileNetworks_Renamed verifies that renaming a network creates the new +// one additively and leaves the old (label-matched) network untouched, without +// prompting. +func TestReconcileNetworks_Renamed(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Networks: types.Networks{"frontend": {Name: "myproject_frontend_v2", Driver: "overlay"}}, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + Networks: map[string]ObservedNetwork{ + "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: mustNetworkHash(t, types.NetworkConfig{Name: "myproject_frontend", Driver: "overlay"})}, + }, + Volumes: map[string]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + assert.Equal(t, plan.String(), strings.TrimSpace(` +[] -> #1 network:frontend, CreateNetwork, renamed `)+"\n") } +// TestReconcileNetworks_UnmanagedMatchReused verifies that a network discovered +// by name but not owned by the project (empty ConfigHash — see +// collectObservedState) is reused untouched: no create, no recreation. +func TestReconcileNetworks_UnmanagedMatchReused(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Networks: types.Networks{"frontend": {Name: "myproject_frontend", Driver: "overlay"}}, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + Networks: map[string]ObservedNetwork{ + "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: ""}, + }, + Volumes: map[string]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + assert.Assert(t, plan.IsEmpty(), "unmanaged network must be reused untouched:\n%s", plan.String()) +} + // --- Volume tests --- func TestReconcileVolumes_CreateMissing(t *testing.T) { @@ -1441,6 +1519,13 @@ func mustVolumeHash(t *testing.T, vol types.VolumeConfig) string { return h } +func mustNetworkHash(t *testing.T, nw types.NetworkConfig) string { + t.Helper() + h, err := NetworkHash(&nw) + assert.NilError(t, err) + return h +} + // mustResolvedServiceHash mirrors what the executor persists at create time: // the service references are resolved before hashing. Use it to seed // ObservedContainer.ConfigHash in tests involving network_mode/ipc/pid: From d7f72412ff775331f3e02487655776b4877b5fe8 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Fri, 24 Jul 2026 09:40:24 +0200 Subject: [PATCH 2/5] feat(reconcile): migrate containers on network rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Nicolas De Loof --- pkg/compose/reconcile.go | 14 ++++++-------- pkg/compose/reconcile_test.go | 32 +++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index d22ba814c65..65d39d52782 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -149,6 +149,12 @@ func reconcile(_ context.Context, project *types.Project, observed *ObservedStat // persisted on the live network (observed.ConfigHash). A network with no // recorded hash (e.g. created by an older Compose or manually) is left // untouched, matching the previous ensureNetwork behavior. +// +// A rename (observed.Name != desired.Name) also diverges the hash — NetworkHash +// includes the name — and is handled by the same recreation path: the old +// network is removed and the new one created, migrating attached containers onto +// it. Networks carry no data, so removing the previous network (rather than +// leaving it dangling) is safe and keeps subsequent runs deterministic. func (r *reconciler) reconcileNetworks() error { var diverged []string for _, key := range sortedKeys(r.project.Networks) { @@ -168,14 +174,6 @@ func (r *reconciler) reconcileNetworks() error { if observed.ConfigHash == "" || observed.ConfigHash == expected { continue } - if observed.Name != desired.Name { - // The network was renamed: the live network matched by label carries - // a different name, i.e. a distinct Docker resource. Create the new - // network and leave the old one untouched, matching the historical - // additive ensureNetwork behavior. - r.planCreateNetwork(key, &desired, "renamed") - continue - } diverged = append(diverged, key) } r.planRecreateNetworks(diverged) diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index 2a5643216b9..2fcf4039070 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -271,17 +271,21 @@ func TestReconcileNetworks_DivergedMultipleServices(t *testing.T) { `)+"\n") } -// TestReconcileNetworks_Renamed verifies that renaming a network creates the new -// one additively and leaves the old (label-matched) network untouched, without -// prompting. +// TestReconcileNetworks_Renamed verifies that renaming a network (the label +// matched live network carries a different name) is handled as a recreation: the +// old network is removed, the new one is created, and the attached container is +// migrated onto it (reconnected). Networks carry no data, so removing the old one +// is safe and keeps subsequent runs deterministic. func TestReconcileNetworks_Renamed(t *testing.T) { + web := types.ServiceConfig{Name: "web", Scale: intPtr(1), Networks: map[string]*types.ServiceNetworkConfig{"frontend": {}}} project := &types.Project{ Name: "myproject", Networks: types.Networks{"frontend": {Name: "myproject_frontend_v2", Driver: "overlay"}}, + Services: types.Services{"web": web}, } observed := &ObservedState{ ProjectName: "myproject", - Containers: map[string][]ObservedContainer{}, + Containers: map[string][]ObservedContainer{"web": {networkAttachedContainer(t, web, "c1aabbccddee")}}, Networks: map[string]ObservedNetwork{ "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: mustNetworkHash(t, types.NetworkConfig{Name: "myproject_frontend", Driver: "overlay"})}, }, @@ -292,8 +296,26 @@ func TestReconcileNetworks_Renamed(t *testing.T) { assert.NilError(t, err) assert.Equal(t, plan.String(), strings.TrimSpace(` -[] -> #1 network:frontend, CreateNetwork, renamed +[] -> #1 service:web:1, StopContainer, network frontend config changed +[1] -> #2 service:web:1, DisconnectNetwork, network frontend recreate +[2] -> #3 network:frontend, RemoveNetwork, config hash diverged +[3] -> #4 network:frontend, CreateNetwork, recreate after config change +[4] -> #5 service:web:1, ConnectNetwork, network frontend recreate `)+"\n") + + // The old network is removed by name and the new name is created — proving + // the rename migrates rather than leaving the old network dangling. + var removed, created string + for _, n := range plan.Nodes { + switch n.Operation.Type { + case OpRemoveNetwork: + removed = n.Operation.Name + case OpCreateNetwork: + created = n.Operation.Name + } + } + assert.Equal(t, removed, "myproject_frontend") + assert.Equal(t, created, "myproject_frontend_v2") } // TestReconcileNetworks_UnmanagedMatchReused verifies that a network discovered From 369d273864a7f3d6436eff5b88f3031678842330 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Fri, 24 Jul 2026 11:28:18 +0200 Subject: [PATCH 3/5] fix(reconcile): retry network create on conflict, keep owned config-hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Nicolas De Loof --- pkg/compose/create.go | 8 +++++++ pkg/compose/executor_test.go | 30 ++++++++++++++++++++++++++ pkg/compose/observed_state.go | 26 +++++++++++++++++++---- pkg/compose/observed_state_test.go | 34 ++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 4 deletions(-) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 04b8dd9e599..19503f36822 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -1453,6 +1453,14 @@ func (s *composeService) createNetwork(ctx context.Context, n *types.NetworkConf s.events.On(creatingEvent(networkEventName)) if _, err := s.apiClient().NetworkCreate(ctx, n.Name, createOpts); err != nil { + // A concurrent `docker compose up|run` may have created the same network + // between the observed-state snapshot and now. Treat the resulting + // conflict as success rather than failing hard, mirroring the retry the + // previous ensureNetwork performed. + if errdefs.IsConflict(err) { + s.events.On(createdEvent(networkEventName)) + return nil + } s.events.On(errorEvent(networkEventName, err.Error())) return fmt.Errorf("failed to create network %s: %w", n.Name, err) } diff --git a/pkg/compose/executor_test.go b/pkg/compose/executor_test.go index 019deb47133..ea0cafbde7a 100644 --- a/pkg/compose/executor_test.go +++ b/pkg/compose/executor_test.go @@ -385,8 +385,38 @@ func TestExecutePlanRecreateNetwork(t *testing.T) { assert.NilError(t, err) } +// TestExecutePlanCreateNetworkConflictIsSuccess verifies that a NetworkCreate +// conflict (a concurrent up/run created the same network) is treated as success +// rather than surfacing as a hard failure. +func TestExecutePlanCreateNetworkConflictIsSuccess(t *testing.T) { + svc, apiClient := newTestService(t) + + nw := types.NetworkConfig{Name: "test_default"} + project := &types.Project{Name: "test", Networks: types.Networks{"default": nw}} + + apiClient.EXPECT().NetworkCreate(gomock.Any(), "test_default", gomock.Any()). + Return(client.NetworkCreateResult{}, conflictError{}) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateNetwork, + ResourceID: "network:default", + Cause: "not found", + Name: nw.Name, + Network: &nw, + }, "") + + assert.NilError(t, svc.executePlan(t.Context(), project, emptyObservedState("test"), plan)) +} + // notFoundError implements the errdefs.ErrNotFound interface for test mocks. type notFoundError struct{} func (notFoundError) Error() string { return "not found" } func (notFoundError) NotFound() {} + +// conflictError implements the errdefs.ErrConflict interface for test mocks. +type conflictError struct{} + +func (conflictError) Error() string { return "conflict" } +func (conflictError) Conflict() {} diff --git a/pkg/compose/observed_state.go b/pkg/compose/observed_state.go index 80f4b6b94cc..10ec46cab49 100644 --- a/pkg/compose/observed_state.go +++ b/pkg/compose/observed_state.go @@ -198,13 +198,27 @@ func (s *composeService) discoverUnmanagedNetworks(ctx context.Context, project ID: inspected.Network.ID, Name: inspected.Network.Name, ProjectName: inspected.Network.Labels[api.ProjectLabel], - // ConfigHash intentionally left empty: the network is not owned by - // this project, so we must not treat it as diverged and recreate it. + // Preserve the config-hash only when the network belongs to this + // project (e.g. an older Compose wrote the project label but not the + // network-key label): the reconciler then still detects genuine + // divergence. For a network we don't own the hash is left empty so we + // reuse it untouched rather than recreate it. + ConfigHash: ownedConfigHash(inspected.Network.Labels, project.Name), } } return nil } +// ownedConfigHash returns the config-hash label only when the resource belongs +// to the given project; otherwise it returns "" so the reconciler treats the +// resource as an unmanaged match to be reused untouched. +func ownedConfigHash(labels map[string]string, projectName string) string { + if labels[api.ProjectLabel] != projectName { + return "" + } + return labels[api.ConfigHashLabel] +} + // discoverUnmanagedVolumes augments the observed state with volumes that match a // declared volume by name but carry no compose label — pre-label Compose or // manually created volumes, missed by the label-filtered VolumeList. Each is @@ -231,8 +245,12 @@ func (s *composeService) discoverUnmanagedVolumes(ctx context.Context, project * Name: inspected.Volume.Name, ProjectName: inspected.Volume.Labels[api.ProjectLabel], Driver: inspected.Volume.Driver, - // ConfigHash intentionally left empty: the volume is not owned by - // this project, so we must not treat it as diverged and recreate it. + // Preserve the config-hash only when the volume belongs to this + // project (older Compose wrote the project label but not the + // volume-key label): the reconciler then still detects divergence. + // For a volume we don't own the hash is left empty so we reuse it + // untouched rather than recreate it. + ConfigHash: ownedConfigHash(inspected.Volume.Labels, project.Name), } } return nil diff --git a/pkg/compose/observed_state_test.go b/pkg/compose/observed_state_test.go index 94ca256cb72..919408c6902 100644 --- a/pkg/compose/observed_state_test.go +++ b/pkg/compose/observed_state_test.go @@ -236,6 +236,40 @@ func TestCollectObservedState_LegacyNetworkMatchedByName(t *testing.T) { assert.Equal(t, obs.ConfigHash, "", "unmanaged match must have an empty config hash") } +// TestCollectObservedState_OwnedNetworkMissingKeyLabelKeepsHash verifies that a +// network owned by this project but missing the network-key label (e.g. written +// by an older Compose) keeps its config-hash, so genuine divergence is still +// detected rather than silently skipped. +func TestCollectObservedState_OwnedNetworkMissingKeyLabelKeepsHash(t *testing.T) { + project := &types.Project{Name: "myproject", Networks: types.Networks{"frontend": {Name: "myproject_frontend"}}} + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { + apiClient.EXPECT().NetworkInspect(gomock.Any(), "myproject_frontend", gomock.Any()).Return(client.NetworkInspectResult{ + Network: network.Inspect{Network: network.Network{ID: "net1", Name: "myproject_frontend", Labels: map[string]string{ + api.ProjectLabel: "myproject", // owned, but no NetworkLabel + api.ConfigHashLabel: "realhash", + }}}, + }, nil) + }) + assert.NilError(t, err) + assert.Equal(t, state.Networks["frontend"].ConfigHash, "realhash") +} + +// TestCollectObservedState_OwnedVolumeMissingKeyLabelKeepsHash is the volume +// counterpart of the network case above. +func TestCollectObservedState_OwnedVolumeMissingKeyLabelKeepsHash(t *testing.T) { + project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": {Name: "myproject_data"}}} + state, err := collectByNameDiscovery(t, project, func(apiClient *mocks.MockAPIClient) { + apiClient.EXPECT().VolumeInspect(gomock.Any(), "myproject_data", gomock.Any()).Return(client.VolumeInspectResult{ + Volume: volume.Volume{Name: "myproject_data", Labels: map[string]string{ + api.ProjectLabel: "myproject", // owned, but no VolumeLabel + api.ConfigHashLabel: "realhash", + }}, + }, nil) + }) + assert.NilError(t, err) + assert.Equal(t, state.Volumes["data"].ConfigHash, "realhash") +} + // TestCollectObservedState_ForeignProjectNetworkMatchedByName verifies that a // network owned by another project but matching the declared name is recorded // with an empty ConfigHash (reused untouched) and keeps the foreign project name. From cebb21bc79ff3834d65407f8a8b6a56e1e51cc9e Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Fri, 24 Jul 2026 14:03:46 +0200 Subject: [PATCH 4/5] feat(reconcile): best-effort old-network removal on rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Nicolas De Loof --- pkg/compose/executor_ops.go | 9 +++++++++ pkg/compose/executor_test.go | 30 +++++++++++++++++++++++++++ pkg/compose/plan.go | 6 ++++++ pkg/compose/reconcile.go | 38 ++++++++++++++++++++++++++++++----- pkg/compose/reconcile_test.go | 23 ++++++++++++--------- 5 files changed, 92 insertions(+), 14 deletions(-) diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index 8affbe2b715..f5236e3f47d 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -21,8 +21,10 @@ import ( "fmt" "slices" + "github.com/containerd/errdefs" "github.com/moby/moby/api/types/container" "github.com/moby/moby/client" + "github.com/sirupsen/logrus" "github.com/docker/compose/v5/pkg/api" "github.com/docker/compose/v5/pkg/utils" @@ -36,6 +38,13 @@ func (exec *planExecutor) execCreateNetwork(ctx context.Context, op Operation) e func (exec *planExecutor) execRemoveNetwork(ctx context.Context, op Operation) error { _, err := exec.compose.apiClient().NetworkRemove(ctx, op.Name, client.NetworkRemoveOptions{}) + // A best-effort removal (old network on a rename) tolerates the network + // still being in use — Docker reports that as a conflict. Any other error + // (transport failure, Moby API error, ...) is still propagated. + if err != nil && op.BestEffort && errdefs.IsConflict(err) { + logrus.Warnf("network %s is still in use and was left in place; remove it manually once no container is attached", op.Name) + return nil + } return err } diff --git a/pkg/compose/executor_test.go b/pkg/compose/executor_test.go index ea0cafbde7a..631dcb2e9ed 100644 --- a/pkg/compose/executor_test.go +++ b/pkg/compose/executor_test.go @@ -18,6 +18,7 @@ package compose import ( "context" + "errors" "fmt" "testing" @@ -409,6 +410,35 @@ func TestExecutePlanCreateNetworkConflictIsSuccess(t *testing.T) { assert.NilError(t, svc.executePlan(t.Context(), project, emptyObservedState("test"), plan)) } +// TestExecRemoveNetworkBestEffort verifies that a best-effort network removal +// (old network on a rename) tolerates a conflict (still in use) but propagates +// any other error, while a mandatory removal propagates conflicts too. +func TestExecRemoveNetworkBestEffort(t *testing.T) { + newExec := func(t *testing.T) (*planExecutor, *mocks.MockAPIClient) { + svc, apiClient := newTestService(t) + return svc.newPlanExecutor(&types.Project{Name: "test"}, emptyObservedState("test")), apiClient + } + + t.Run("best-effort ignores conflict", func(t *testing.T) { + exec, apiClient := newExec(t) + apiClient.EXPECT().NetworkRemove(gomock.Any(), "old", gomock.Any()).Return(client.NetworkRemoveResult{}, conflictError{}) + err := exec.execRemoveNetwork(t.Context(), Operation{Type: OpRemoveNetwork, Name: "old", BestEffort: true}) + assert.NilError(t, err) + }) + t.Run("best-effort propagates non-conflict", func(t *testing.T) { + exec, apiClient := newExec(t) + apiClient.EXPECT().NetworkRemove(gomock.Any(), "old", gomock.Any()).Return(client.NetworkRemoveResult{}, errors.New("transport failure")) + err := exec.execRemoveNetwork(t.Context(), Operation{Type: OpRemoveNetwork, Name: "old", BestEffort: true}) + assert.ErrorContains(t, err, "transport failure") + }) + t.Run("mandatory removal propagates conflict", func(t *testing.T) { + exec, apiClient := newExec(t) + apiClient.EXPECT().NetworkRemove(gomock.Any(), "old", gomock.Any()).Return(client.NetworkRemoveResult{}, conflictError{}) + err := exec.execRemoveNetwork(t.Context(), Operation{Type: OpRemoveNetwork, Name: "old", BestEffort: false}) + assert.Assert(t, err != nil, "mandatory removal must propagate conflict") + }) +} + // notFoundError implements the errdefs.ErrNotFound interface for test mocks. type notFoundError struct{} diff --git a/pkg/compose/plan.go b/pkg/compose/plan.go index 6f64f830e18..6a118f12e02 100644 --- a/pkg/compose/plan.go +++ b/pkg/compose/plan.go @@ -103,6 +103,12 @@ type Operation struct { Volume *types.VolumeConfig // for volume operations Timeout *time.Duration // for stop operations CreateNodeID int // for OpRenameContainer: ID of the CreateContainer node whose result to rename + // BestEffort marks an operation whose failure must not abort the plan. It is + // used for the optional removal of the old network on a rename: if the + // network is still in use (by non-Compose containers) the removal is skipped + // with a warning instead of failing — the new network already carries a + // different name, so the migration does not depend on the old one going away. + BestEffort bool } // PlanNode is a single node in the reconciliation DAG. It represents one diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index 65d39d52782..36e8eda0e4f 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -236,32 +236,60 @@ func (r *reconciler) planRecreateNetworks(keys []string) { }, "", stopNode)) } + // A rename (the live network has a different name than desired) does not + // require removing the old network: the new one has a distinct name, so + // it is created independently and the old removal becomes best-effort + // cleanup — skipped with a warning if the network is still in use by + // non-Compose containers, instead of blocking the whole operation. A + // same-name divergence, on the other hand, must remove the old network + // before the new one can be created. + rename := observed.Name != desired.Name + removeCause := "config hash diverged" + createCause := "recreate after config change" + if rename { + removeCause = "renamed (best-effort cleanup)" + createCause = "renamed" + } + removeNode := r.plan.addNode(Operation{ Type: OpRemoveNetwork, ResourceID: fmt.Sprintf("network:%s", key), - Cause: "config hash diverged", + Cause: removeCause, Name: observed.Name, + BestEffort: rename, }, "", disconnectNodes...) + + var createDeps []*PlanNode + if !rename { + createDeps = []*PlanNode{removeNode} + } createNode := r.plan.addNode(Operation{ Type: OpCreateNetwork, ResourceID: fmt.Sprintf("network:%s", key), - Cause: "recreate after config change", + Cause: createCause, Name: desired.Name, Network: &desired, - }, "", removeNode) + }, "", createDeps...) r.networkNodes[key] = createNode - // Reconnect every attached container to the fresh network. + // Reconnect every attached container to the fresh network. On a rename the + // reconnect also waits for the container to be disconnected from the old + // network first (on a same-name recreate that ordering already holds + // transitively through remove → create). for i := range containers { oc := &containers[i] resID := fmt.Sprintf("service:%s:%d", oc.Summary.Labels[api.ServiceLabel], oc.Number) + deps := []*PlanNode{createNode} + if rename { + deps = append(deps, disconnectNodes[i]) + } connectNode := r.plan.addNode(Operation{ Type: OpConnectNetwork, ResourceID: resID, Cause: fmt.Sprintf("network %s recreate", key), Container: &oc.Summary, Name: desired.Name, - }, "", createNode) + }, "", deps...) r.connectNodes[oc.ID] = append(r.connectNodes[oc.ID], connectNode) } } diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index 2fcf4039070..48a7bd7e975 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -272,10 +272,11 @@ func TestReconcileNetworks_DivergedMultipleServices(t *testing.T) { } // TestReconcileNetworks_Renamed verifies that renaming a network (the label -// matched live network carries a different name) is handled as a recreation: the -// old network is removed, the new one is created, and the attached container is -// migrated onto it (reconnected). Networks carry no data, so removing the old one -// is safe and keeps subsequent runs deterministic. +// matched live network carries a different name) migrates the attached container +// onto the new network. The new network is created independently of the old one, +// and the old network's removal is best-effort cleanup (it does not gate the +// migration), so a network still in use by non-Compose containers cannot block a +// rename. func TestReconcileNetworks_Renamed(t *testing.T) { web := types.ServiceConfig{Name: "web", Scale: intPtr(1), Networks: map[string]*types.ServiceNetworkConfig{"frontend": {}}} project := &types.Project{ @@ -295,21 +296,25 @@ func TestReconcileNetworks_Renamed(t *testing.T) { plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) assert.NilError(t, err) + // CreateNetwork (#4) does not depend on RemoveNetwork (#3): the new network + // has a different name, so it is created independently and the old removal is + // best-effort. The reconnect (#5) waits for the new network and for the + // container to leave the old one. assert.Equal(t, plan.String(), strings.TrimSpace(` [] -> #1 service:web:1, StopContainer, network frontend config changed [1] -> #2 service:web:1, DisconnectNetwork, network frontend recreate -[2] -> #3 network:frontend, RemoveNetwork, config hash diverged -[3] -> #4 network:frontend, CreateNetwork, recreate after config change -[4] -> #5 service:web:1, ConnectNetwork, network frontend recreate +[2] -> #3 network:frontend, RemoveNetwork, renamed (best-effort cleanup) +[] -> #4 network:frontend, CreateNetwork, renamed +[2,4] -> #5 service:web:1, ConnectNetwork, network frontend recreate `)+"\n") - // The old network is removed by name and the new name is created — proving - // the rename migrates rather than leaving the old network dangling. + // The old-network removal is best-effort; the new name is created. var removed, created string for _, n := range plan.Nodes { switch n.Operation.Type { case OpRemoveNetwork: removed = n.Operation.Name + assert.Assert(t, n.Operation.BestEffort, "rename removal must be best-effort") case OpCreateNetwork: created = n.Operation.Name } From 700a83dbc4bc93487f32000e8e25b51335a0d1a2 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Fri, 24 Jul 2026 10:16:37 +0200 Subject: [PATCH 5/5] refactor(reconcile): aggregate observed resources, resolve conflicts in reconcile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Nicolas De Loof --- pkg/compose/create.go | 4 +- pkg/compose/executor_test.go | 12 +- pkg/compose/observed_state.go | 107 +++++++++++---- pkg/compose/observed_state_test.go | 113 +++++++++++++--- pkg/compose/reconcile.go | 58 ++++++++- pkg/compose/reconcile_test.go | 200 +++++++++++++++++------------ 6 files changed, 351 insertions(+), 143 deletions(-) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 19503f36822..1c08e1501ea 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -174,7 +174,7 @@ func warnUnmanagedNetworks(project *types.Project, observed *ObservedState) { if nw.External { continue } - obs, ok := observed.Networks[k] + obs, _, ok := observed.selectNetwork(k, nw.Name) if !ok || obs.ProjectName == project.Name { continue } @@ -237,7 +237,7 @@ func warnUnmanagedVolumes(project *types.Project, observed *ObservedState) { if volume.External { continue } - obs, ok := observed.Volumes[k] + obs, _, ok := observed.selectVolume(k, volume.Name) if !ok || obs.ProjectName == project.Name { continue } diff --git a/pkg/compose/executor_test.go b/pkg/compose/executor_test.go index 631dcb2e9ed..5480b006e9d 100644 --- a/pkg/compose/executor_test.go +++ b/pkg/compose/executor_test.go @@ -125,8 +125,8 @@ func emptyObservedState(project string) *ObservedState { return &ObservedState{ ProjectName: project, Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } } @@ -160,8 +160,8 @@ func TestExecutePlanRemoveContainerDropsFromCache(t *testing.T) { Containers: map[string][]ObservedContainer{ "web": {{ID: "old-id", Summary: oldCtr}}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan := &Plan{} @@ -222,8 +222,8 @@ func TestExecutePlanConcurrentRemovesCacheCoherence(t *testing.T) { observed := &ObservedState{ ProjectName: "test", Containers: map[string][]ObservedContainer{"web": webContainers}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } // Build N independent Stop→Remove chains. The errgroup will fan them out diff --git a/pkg/compose/observed_state.go b/pkg/compose/observed_state.go index 10ec46cab49..d2e187e0b67 100644 --- a/pkg/compose/observed_state.go +++ b/pkg/compose/observed_state.go @@ -18,6 +18,8 @@ package compose import ( "context" + "slices" + "sort" "strconv" "strings" @@ -37,8 +39,65 @@ type ObservedState struct { ProjectName string Containers map[string][]ObservedContainer // service name → containers Orphans []ObservedContainer // containers with no matching service - Networks map[string]ObservedNetwork // compose network key → observed - Volumes map[string]ObservedVolume // compose volume key → observed + // Networks/Volumes map a compose key to *all* live resources bearing that + // compose label. Collection makes no premature choice: when several + // resources share a key (e.g. a leftover after a rename), they are all + // recorded here and the reconciler selects the right one and reports the + // others as orphans (see selectNetwork/selectVolume). + Networks map[string][]ObservedNetwork // compose network key → observed + Volumes map[string][]ObservedVolume // compose volume key → observed +} + +// selectNetwork picks, among the live networks recorded for a compose key, the +// one that best matches the desired name, and returns the remaining ones as +// orphans. Selection is deterministic: an exact name match wins; otherwise the +// lexicographically smallest name is chosen so repeated runs are stable +// regardless of the daemon's list order. +func (s *ObservedState) selectNetwork(key, desiredName string) (ObservedNetwork, []ObservedNetwork, bool) { + matches := s.Networks[key] + if len(matches) == 0 { + return ObservedNetwork{}, nil, false + } + sorted := slices.Clone(matches) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].Name < sorted[j].Name }) + selected := sorted[0] + for _, m := range sorted { + if m.Name == desiredName { + selected = m + break + } + } + var orphans []ObservedNetwork + for _, m := range sorted { + if m.Name != selected.Name { + orphans = append(orphans, m) + } + } + return selected, orphans, true +} + +// selectVolume is the volume counterpart of selectNetwork. +func (s *ObservedState) selectVolume(key, desiredName string) (ObservedVolume, []ObservedVolume, bool) { + matches := s.Volumes[key] + if len(matches) == 0 { + return ObservedVolume{}, nil, false + } + sorted := slices.Clone(matches) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].Name < sorted[j].Name }) + selected := sorted[0] + for _, m := range sorted { + if m.Name == desiredName { + selected = m + break + } + } + var orphans []ObservedVolume + for _, m := range sorted { + if m.Name != selected.Name { + orphans = append(orphans, m) + } + } + return selected, orphans, true } // ObservedContainer holds the relevant state extracted from a running or stopped @@ -86,8 +145,8 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type state := &ObservedState{ ProjectName: project.Name, Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } // --- Containers --- @@ -128,12 +187,12 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type if key == "" { continue } - state.Networks[key] = ObservedNetwork{ + state.Networks[key] = append(state.Networks[key], ObservedNetwork{ ID: nw.ID, Name: nw.Name, ConfigHash: nw.Labels[api.ConfigHashLabel], ProjectName: nw.Labels[api.ProjectLabel], - } + }) } // --- Volumes --- @@ -148,12 +207,12 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type if key == "" { continue } - state.Volumes[key] = ObservedVolume{ + state.Volumes[key] = append(state.Volumes[key], ObservedVolume{ Name: vol.Name, ConfigHash: vol.Labels[api.ConfigHashLabel], ProjectName: vol.Labels[api.ProjectLabel], Driver: vol.Driver, - } + }) } if err := s.discoverUnmanagedNetworks(ctx, project, state); err != nil { @@ -179,7 +238,7 @@ func (s *composeService) discoverUnmanagedNetworks(ctx context.Context, project if nw.External { continue } - if _, ok := state.Networks[key]; ok { + if len(state.Networks[key]) > 0 { continue } inspected, err := s.apiClient().NetworkInspect(ctx, nw.Name, client.NetworkInspectOptions{}) @@ -194,7 +253,7 @@ func (s *composeService) discoverUnmanagedNetworks(ctx context.Context, project if inspected.Network.Name != nw.Name && inspected.Network.ID != nw.Name { continue } - state.Networks[key] = ObservedNetwork{ + state.Networks[key] = append(state.Networks[key], ObservedNetwork{ ID: inspected.Network.ID, Name: inspected.Network.Name, ProjectName: inspected.Network.Labels[api.ProjectLabel], @@ -204,7 +263,7 @@ func (s *composeService) discoverUnmanagedNetworks(ctx context.Context, project // divergence. For a network we don't own the hash is left empty so we // reuse it untouched rather than recreate it. ConfigHash: ownedConfigHash(inspected.Network.Labels, project.Name), - } + }) } return nil } @@ -231,7 +290,7 @@ func (s *composeService) discoverUnmanagedVolumes(ctx context.Context, project * if vol.External { continue } - if _, ok := state.Volumes[key]; ok { + if len(state.Volumes[key]) > 0 { continue } inspected, err := s.apiClient().VolumeInspect(ctx, vol.Name, client.VolumeInspectOptions{}) @@ -241,7 +300,7 @@ func (s *composeService) discoverUnmanagedVolumes(ctx context.Context, project * } return err } - state.Volumes[key] = ObservedVolume{ + state.Volumes[key] = append(state.Volumes[key], ObservedVolume{ Name: inspected.Volume.Name, ProjectName: inspected.Volume.Labels[api.ProjectLabel], Driver: inspected.Volume.Driver, @@ -251,7 +310,7 @@ func (s *composeService) discoverUnmanagedVolumes(ctx context.Context, project * // For a volume we don't own the hash is left empty so we reuse it // untouched rather than recreate it. ConfigHash: ownedConfigHash(inspected.Volume.Labels, project.Name), - } + }) } return nil } @@ -284,14 +343,11 @@ func toObservedContainer(c container.Summary) ObservedContainer { // into the observed state, so the reconciler can compare container connections // against actual network IDs. func (s *ObservedState) setResolvedNetworks(networks map[string]string, project *types.Project) { + // Only external networks are passed here; they carry no compose label and so + // are absent from the collected state, hence a plain append. for key, id := range networks { - if obs, exists := s.Networks[key]; exists { - obs.ID = id - s.Networks[key] = obs - } else { - nw := project.Networks[key] - s.Networks[key] = ObservedNetwork{ID: id, Name: nw.Name} - } + nw := project.Networks[key] + s.Networks[key] = append(s.Networks[key], ObservedNetwork{ID: id, Name: nw.Name}) } } @@ -299,13 +355,10 @@ func (s *ObservedState) setResolvedNetworks(networks map[string]string, project // (external volumes) into the observed state. Managed volumes are discovered // directly by collectObservedState, so only external ones need injecting. func (s *ObservedState) setResolvedVolumes(volumes map[string]string) { + // Only external volumes are passed here; they carry no compose label and so + // are absent from the collected state, hence a plain append. for key, id := range volumes { - if obs, exists := s.Volumes[key]; exists { - obs.Name = id - s.Volumes[key] = obs - } else { - s.Volumes[key] = ObservedVolume{Name: id} - } + s.Volumes[key] = append(s.Volumes[key], ObservedVolume{Name: id}) } } diff --git a/pkg/compose/observed_state_test.go b/pkg/compose/observed_state_test.go index 919408c6902..5e965ba7b99 100644 --- a/pkg/compose/observed_state_test.go +++ b/pkg/compose/observed_state_test.go @@ -192,19 +192,90 @@ func TestCollectObservedState(t *testing.T) { // Networks assert.Equal(t, len(state.Networks), 1) - nw := state.Networks["frontend"] + nw := state.Networks["frontend"][0] assert.Equal(t, nw.ID, "net1") assert.Equal(t, nw.Name, "myproject_frontend") assert.Equal(t, nw.ConfigHash, "nethash1") // Volumes assert.Equal(t, len(state.Volumes), 1) - vol := state.Volumes["data"] + vol := state.Volumes["data"][0] assert.Equal(t, vol.Name, "myproject_data") assert.Equal(t, vol.Driver, "local") assert.Equal(t, vol.ConfigHash, "volhash1") } +// TestCollectObservedState_AggregatesDuplicateLabels verifies that two live +// volumes (or networks) sharing the same compose label are both recorded, with +// no premature choice — the reconciler resolves the conflict later. +func TestCollectObservedState_AggregatesDuplicateLabels(t *testing.T) { + svc, apiClient := newTestService(t) + project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": {Name: "pgdata_v2"}}} + + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()).Return(client.ContainerListResult{}, nil) + apiClient.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{}, nil) + apiClient.EXPECT().VolumeList(gomock.Any(), gomock.Any()).Return(client.VolumeListResult{ + Items: []volume.Volume{ + {Name: "pgdata_v1", Labels: map[string]string{api.VolumeLabel: "data", api.ProjectLabel: "myproject", api.ConfigHashLabel: "old"}}, + {Name: "pgdata_v2", Labels: map[string]string{api.VolumeLabel: "data", api.ProjectLabel: "myproject", api.ConfigHashLabel: "new"}}, + }, + }, nil) + + state, err := svc.collectObservedState(t.Context(), project) + assert.NilError(t, err) + assert.Equal(t, len(state.Volumes["data"]), 2, "both label-sharing volumes must be recorded") +} + +func TestSelectVolume(t *testing.T) { + v1 := ObservedVolume{Name: "pgdata_v1", ConfigHash: "old"} + v2 := ObservedVolume{Name: "pgdata_v2", ConfigHash: "new"} + + t.Run("empty", func(t *testing.T) { + s := &ObservedState{Volumes: map[string][]ObservedVolume{}} + _, _, ok := s.selectVolume("data", "pgdata_v2") + assert.Assert(t, !ok) + }) + t.Run("single", func(t *testing.T) { + s := &ObservedState{Volumes: map[string][]ObservedVolume{"data": {v1}}} + sel, orphans, ok := s.selectVolume("data", "pgdata_v2") + assert.Assert(t, ok) + assert.Equal(t, sel.Name, "pgdata_v1") + assert.Equal(t, len(orphans), 0) + }) + t.Run("prefers exact name match regardless of order", func(t *testing.T) { + for _, order := range [][]ObservedVolume{{v1, v2}, {v2, v1}} { + s := &ObservedState{Volumes: map[string][]ObservedVolume{"data": order}} + sel, orphans, ok := s.selectVolume("data", "pgdata_v2") + assert.Assert(t, ok) + assert.Equal(t, sel.Name, "pgdata_v2") + assert.Equal(t, len(orphans), 1) + assert.Equal(t, orphans[0].Name, "pgdata_v1") + } + }) + t.Run("no name match picks lexicographically smallest, deterministically", func(t *testing.T) { + for _, order := range [][]ObservedVolume{{v1, v2}, {v2, v1}} { + s := &ObservedState{Volumes: map[string][]ObservedVolume{"data": order}} + sel, orphans, ok := s.selectVolume("data", "pgdata_v3") + assert.Assert(t, ok) + assert.Equal(t, sel.Name, "pgdata_v1", "smallest name wins") + assert.Equal(t, len(orphans), 1) + assert.Equal(t, orphans[0].Name, "pgdata_v2") + } + }) +} + +func TestSelectNetwork(t *testing.T) { + n1 := ObservedNetwork{ID: "net1", Name: "front_v1"} + n2 := ObservedNetwork{ID: "net2", Name: "front_v2"} + + s := &ObservedState{Networks: map[string][]ObservedNetwork{"frontend": {n2, n1}}} + sel, orphans, ok := s.selectNetwork("frontend", "front_v2") + assert.Assert(t, ok) + assert.Equal(t, sel.Name, "front_v2") + assert.Equal(t, len(orphans), 1) + assert.Equal(t, orphans[0].Name, "front_v1") +} + // collectByNameDiscovery mocks empty container/network/volume lists so that only // the legacy by-name network/volume discovery is exercised. func collectByNameDiscovery(t *testing.T, project *types.Project, inspect func(apiClient *mocks.MockAPIClient)) (*ObservedState, error) { @@ -228,8 +299,9 @@ func TestCollectObservedState_LegacyNetworkMatchedByName(t *testing.T) { }, nil) }) assert.NilError(t, err) - obs, ok := state.Networks["frontend"] - assert.Assert(t, ok, "legacy network must be discovered by name") + matches, ok := state.Networks["frontend"] + assert.Assert(t, ok && len(matches) == 1, "legacy network must be discovered by name") + obs := matches[0] assert.Equal(t, obs.ID, "net1") assert.Equal(t, obs.Name, "myproject_frontend") assert.Equal(t, obs.ProjectName, "") @@ -251,7 +323,7 @@ func TestCollectObservedState_OwnedNetworkMissingKeyLabelKeepsHash(t *testing.T) }, nil) }) assert.NilError(t, err) - assert.Equal(t, state.Networks["frontend"].ConfigHash, "realhash") + assert.Equal(t, state.Networks["frontend"][0].ConfigHash, "realhash") } // TestCollectObservedState_OwnedVolumeMissingKeyLabelKeepsHash is the volume @@ -267,7 +339,7 @@ func TestCollectObservedState_OwnedVolumeMissingKeyLabelKeepsHash(t *testing.T) }, nil) }) assert.NilError(t, err) - assert.Equal(t, state.Volumes["data"].ConfigHash, "realhash") + assert.Equal(t, state.Volumes["data"][0].ConfigHash, "realhash") } // TestCollectObservedState_ForeignProjectNetworkMatchedByName verifies that a @@ -284,7 +356,7 @@ func TestCollectObservedState_ForeignProjectNetworkMatchedByName(t *testing.T) { }, nil) }) assert.NilError(t, err) - obs := state.Networks["frontend"] + obs := state.Networks["frontend"][0] assert.Equal(t, obs.ProjectName, "otherproject") assert.Equal(t, obs.ConfigHash, "", "foreign network must not be treated as diverged") } @@ -325,11 +397,11 @@ func TestWarnUnmanagedNetworks(t *testing.T) { }, } observed := &ObservedState{ - Networks: map[string]ObservedNetwork{ - "managed": {Name: "myproject_managed", ProjectName: "myproject", ConfigHash: "h"}, - "unlabel": {Name: "unlabel_net", ProjectName: ""}, - "foreign": {Name: "foreign_net", ProjectName: "otherproject"}, - "external": {Name: "ext_net"}, + Networks: map[string][]ObservedNetwork{ + "managed": {{Name: "myproject_managed", ProjectName: "myproject", ConfigHash: "h"}}, + "unlabel": {{Name: "unlabel_net", ProjectName: ""}}, + "foreign": {{Name: "foreign_net", ProjectName: "otherproject"}}, + "external": {{Name: "ext_net"}}, }, } @@ -359,8 +431,9 @@ func TestCollectObservedState_LegacyVolumeMatchedByName(t *testing.T) { }, nil) }) assert.NilError(t, err) - obs, ok := state.Volumes["data"] - assert.Assert(t, ok, "legacy volume must be discovered by name") + matches, ok := state.Volumes["data"] + assert.Assert(t, ok && len(matches) == 1, "legacy volume must be discovered by name") + obs := matches[0] assert.Equal(t, obs.Name, "myproject_data") assert.Equal(t, obs.ProjectName, "") assert.Equal(t, obs.ConfigHash, "", "unmanaged match must have an empty config hash") @@ -381,7 +454,7 @@ func TestCollectObservedState_ForeignProjectVolumeMatchedByName(t *testing.T) { }, nil) }) assert.NilError(t, err) - obs := state.Volumes["data"] + obs := state.Volumes["data"][0] assert.Equal(t, obs.ProjectName, "otherproject") assert.Equal(t, obs.ConfigHash, "", "foreign volume must not be treated as diverged") } @@ -423,11 +496,11 @@ func TestWarnUnmanagedVolumes(t *testing.T) { }, } observed := &ObservedState{ - Volumes: map[string]ObservedVolume{ - "managed": {Name: "myproject_managed", ProjectName: "myproject", ConfigHash: "h"}, - "unlabel": {Name: "unlabel_data", ProjectName: ""}, - "foreign": {Name: "foreign_data", ProjectName: "otherproject"}, - "external": {Name: "ext_data"}, + Volumes: map[string][]ObservedVolume{ + "managed": {{Name: "myproject_managed", ProjectName: "myproject", ConfigHash: "h"}}, + "unlabel": {{Name: "unlabel_data", ProjectName: ""}}, + "foreign": {{Name: "foreign_data", ProjectName: "otherproject"}}, + "external": {{Name: "ext_data"}}, // "tocreate" absent: will be created, no warning. }, } diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index 36e8eda0e4f..56eaced0f55 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -27,6 +27,7 @@ import ( "github.com/compose-spec/compose-go/v2/types" "github.com/moby/moby/api/types/container" mmount "github.com/moby/moby/api/types/mount" + "github.com/sirupsen/logrus" "github.com/docker/compose/v5/pkg/api" ) @@ -100,6 +101,13 @@ type reconciler struct { // (an O(services * containers) build) for expectedConfigHash, which is // called once per service. observedContainersByService map[string]Containers + + // resolvedNetworks/resolvedVolumes hold the single live resource selected per + // compose key from the (possibly multi-valued) observed state — see + // resolveObserved. All reconcile logic reads these, never observed.Networks/ + // observed.Volumes directly, so selection happens exactly once. + resolvedNetworks map[string]ObservedNetwork + resolvedVolumes map[string]ObservedVolume } // reconcile is the main entry point: it builds a Plan from desired vs observed state. @@ -121,6 +129,8 @@ func reconcile(_ context.Context, project *types.Project, observed *ObservedStat observedContainersByService: observed.containersByService(), } + r.resolveObserved() + if err := r.reconcileNetworks(); err != nil { return nil, err } @@ -140,6 +150,40 @@ func reconcile(_ context.Context, project *types.Project, observed *ObservedStat return r.plan, nil } +// resolveObserved selects, for every declared network and volume, the single +// live resource that matches it best (see selectNetwork/selectVolume) and stores +// it in resolvedNetworks/resolvedVolumes — the only observed views the rest of +// the reconciler reads. Extra live resources sharing a compose key (typically a +// leftover after a rename) are reported as orphans: they are left untouched — +// removing them could drop data or break unrelated workloads — but the user is +// warned so they can clean up, and selection stays deterministic across runs. +func (r *reconciler) resolveObserved() { + r.resolvedNetworks = make(map[string]ObservedNetwork, len(r.project.Networks)) + for _, key := range sortedKeys(r.project.Networks) { + selected, orphans, ok := r.observed.selectNetwork(key, r.project.Networks[key].Name) + if !ok { + continue + } + r.resolvedNetworks[key] = selected + for _, o := range orphans { + logrus.Warnf("network %q (id %s) carries the compose label %q but does not match the compose file (using %q); "+ + "it is left untouched — remove it manually if it is no longer needed", o.Name, o.ID, key, selected.Name) + } + } + r.resolvedVolumes = make(map[string]ObservedVolume, len(r.project.Volumes)) + for _, key := range sortedKeys(r.project.Volumes) { + selected, orphans, ok := r.observed.selectVolume(key, r.project.Volumes[key].Name) + if !ok { + continue + } + r.resolvedVolumes[key] = selected + for _, o := range orphans { + logrus.Warnf("volume %q carries the compose label %q but does not match the compose file (using %q); "+ + "it is left untouched — remove it manually if it is no longer needed", o.Name, key, selected.Name) + } + } +} + // reconcileNetworks plans the network lifecycle: creation of missing networks // and, for networks whose configuration has diverged from the live resource, // recreation. Unlike volumes, recreating a network is not destructive, so no @@ -162,7 +206,7 @@ func (r *reconciler) reconcileNetworks() error { if desired.External { continue } - observed, exists := r.observed.Networks[key] + observed, exists := r.resolvedNetworks[key] if !exists { r.planCreateNetwork(key, &desired, "not found") continue @@ -207,7 +251,7 @@ func (r *reconciler) planCreateNetwork(key string, nw *types.NetworkConfig, caus // the reconnect instead of racing it. func (r *reconciler) planRecreateNetworks(keys []string) { for _, key := range keys { - observed := r.observed.Networks[key] + observed := r.resolvedNetworks[key] desired := r.project.Networks[key] containers := r.containersForServices(r.servicesUsingNetwork(key)) @@ -310,7 +354,7 @@ func (r *reconciler) reconcileVolumes() error { if desired.External { continue } - observed, exists := r.observed.Volumes[key] + observed, exists := r.resolvedVolumes[key] if !exists { r.planCreateVolume(key, &desired, "not found") continue @@ -335,7 +379,7 @@ func (r *reconciler) reconcileVolumes() error { // path did), and so later runs match deterministically on the new // name rather than split-braining between the two. observed.Name = desired.Name - r.observed.Volumes[key] = observed + r.resolvedVolumes[key] = observed continue } confirmed, err := r.prompt( @@ -432,7 +476,7 @@ func (r *reconciler) planRecreateVolumes(keys []string) { Type: OpRemoveVolume, ResourceID: fmt.Sprintf("volume:%s", key), Cause: "config hash diverged", - Name: r.observed.Volumes[key].Name, + Name: r.resolvedVolumes[key].Name, }, "", removeNodes...) createVolNode := r.plan.addNode(Operation{ Type: OpCreateVolume, @@ -775,7 +819,7 @@ func serviceHashWithResolvedRefs(svc types.ServiceConfig, containers map[string] func (r *reconciler) hasNetworkMismatch(expected types.ServiceConfig, oc ObservedContainer) bool { for _, net := range sortedKeys(expected.Networks) { expectedID := "" - if obs, ok := r.observed.Networks[net]; ok { + if obs, ok := r.resolvedNetworks[net]; ok { expectedID = obs.ID } if expectedID == "" || expectedID == "swarm" { @@ -802,7 +846,7 @@ func (r *reconciler) hasVolumeMismatch(expected types.ServiceConfig, oc Observed continue } expectedName := "" - if obs, ok := r.observed.Volumes[vol.Source]; ok { + if obs, ok := r.resolvedVolumes[vol.Source]; ok { expectedName = obs.Name } if expectedName == "" { diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index 48a7bd7e975..8f58122669e 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -24,6 +24,8 @@ import ( "github.com/compose-spec/compose-go/v2/types" "github.com/moby/moby/api/types/container" + "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" "gotest.tools/v3/assert" "github.com/docker/compose/v5/pkg/api" @@ -76,8 +78,8 @@ func TestReconcileNetworks_CreateMissing(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -102,10 +104,10 @@ func TestReconcileNetworks_ExistingMatch(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{ - "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: hash}, + Networks: map[string][]ObservedNetwork{ + "frontend": {{ID: "net1", Name: "myproject_frontend", ConfigHash: hash}}, }, - Volumes: map[string]ObservedVolume{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -123,8 +125,8 @@ func TestReconcileNetworks_ExternalSkipped(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -162,10 +164,10 @@ func TestReconcileNetworks_Diverged(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{"web": {networkAttachedContainer(t, web, "c1aabbccddee")}}, - Networks: map[string]ObservedNetwork{ - "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}, + Networks: map[string][]ObservedNetwork{ + "frontend": {{ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}}, }, - Volumes: map[string]ObservedVolume{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -198,10 +200,10 @@ func TestReconcileNetworks_DivergedAlsoRecreatesChangedContainer(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{"web": {oc}}, - Networks: map[string]ObservedNetwork{ - "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}, + Networks: map[string][]ObservedNetwork{ + "frontend": {{ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}}, }, - Volumes: map[string]ObservedVolume{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -248,10 +250,10 @@ func TestReconcileNetworks_DivergedMultipleServices(t *testing.T) { "web": {networkAttachedContainer(t, web, "c1aabbccddee")}, "api": {networkAttachedContainer(t, apiSvc, "c2aabbccddee")}, }, - Networks: map[string]ObservedNetwork{ - "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}, + Networks: map[string][]ObservedNetwork{ + "frontend": {{ID: "net1", Name: "myproject_frontend", ConfigHash: "oldhash"}}, }, - Volumes: map[string]ObservedVolume{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -287,10 +289,10 @@ func TestReconcileNetworks_Renamed(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{"web": {networkAttachedContainer(t, web, "c1aabbccddee")}}, - Networks: map[string]ObservedNetwork{ - "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: mustNetworkHash(t, types.NetworkConfig{Name: "myproject_frontend", Driver: "overlay"})}, + Networks: map[string][]ObservedNetwork{ + "frontend": {{ID: "net1", Name: "myproject_frontend", ConfigHash: mustNetworkHash(t, types.NetworkConfig{Name: "myproject_frontend", Driver: "overlay"})}}, }, - Volumes: map[string]ObservedVolume{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -334,10 +336,10 @@ func TestReconcileNetworks_UnmanagedMatchReused(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{ - "frontend": {ID: "net1", Name: "myproject_frontend", ConfigHash: ""}, + Networks: map[string][]ObservedNetwork{ + "frontend": {{ID: "net1", Name: "myproject_frontend", ConfigHash: ""}}, }, - Volumes: map[string]ObservedVolume{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -355,8 +357,8 @@ func TestReconcileVolumes_CreateMissing(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -379,9 +381,9 @@ func TestReconcileVolumes_ExistingMatch(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{ - "data": {Name: "myproject_data", ConfigHash: hash}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{ + "data": {{Name: "myproject_data", ConfigHash: hash}}, }, } @@ -398,8 +400,8 @@ func TestReconcileVolumes_ExternalSkipped(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -422,8 +424,8 @@ func divergedVolumeProject(t *testing.T, count, scale int) (*types.Project, *Obs observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{"data": {Name: vol.Name, ConfigHash: "oldhash"}}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{"data": {{Name: vol.Name, ConfigHash: "oldhash"}}}, } for s := 0; s < count; s++ { name := fmt.Sprintf("db%d", s) @@ -483,9 +485,9 @@ func TestReconcileVolumes_DivergedDeclined(t *testing.T) { // never prompts — matching the previous ensureVolume behavior. func TestReconcileVolumes_DivergedNoRecordedHash(t *testing.T) { project, observed := divergedVolumeProject(t, 1, 1) - obs := observed.Volumes["data"] + obs := observed.Volumes["data"][0] obs.ConfigHash = "" - observed.Volumes["data"] = obs + observed.Volumes["data"] = []ObservedVolume{obs} // noPrompt panics if consulted, proving the empty-hash guard short-circuits. plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -590,10 +592,10 @@ func TestReconcileVolumes_DivergedConfirmedSharedContainer(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{ - "data1": {Name: vol1.Name, ConfigHash: "oldhash"}, - "data2": {Name: vol2.Name, ConfigHash: "oldhash"}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{ + "data1": {{Name: vol1.Name, ConfigHash: "oldhash"}}, + "data2": {{Name: vol2.Name, ConfigHash: "oldhash"}}, }, } @@ -641,10 +643,10 @@ func TestReconcileVolumes_DivergedPartialConfirm(t *testing.T) { "db1": {mountedContainer("c1", "db1", h1, vol1.Name)}, "db2": {mountedContainer("c2", "db2", h2, vol2.Name)}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{ - "data1": {Name: vol1.Name, ConfigHash: "oldhash"}, - "data2": {Name: vol2.Name, ConfigHash: "oldhash"}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{ + "data1": {{Name: vol1.Name, ConfigHash: "oldhash"}}, + "data2": {{Name: vol2.Name, ConfigHash: "oldhash"}}, }, } @@ -714,8 +716,8 @@ func TestReconcileVolumes_DivergedCascadesToDependent(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{"data": {Name: vol.Name, ConfigHash: "oldhash"}}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{"data": {{Name: vol.Name, ConfigHash: "oldhash"}}}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), yesPrompt) @@ -785,8 +787,8 @@ func TestReconcileVolumes_DivergedVolumesFromRemovedBeforeVolume(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{"data": {Name: vol.Name, ConfigHash: "oldhash"}}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{"data": {{Name: vol.Name, ConfigHash: "oldhash"}}}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), yesPrompt) @@ -844,9 +846,9 @@ func TestReconcileVolumes_UnmanagedMatchReused(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, + Networks: map[string][]ObservedNetwork{}, // Unmanaged match: name resolved, but no config hash recorded. - Volumes: map[string]ObservedVolume{"data": {Name: "myproject_data", ConfigHash: ""}}, + Volumes: map[string][]ObservedVolume{"data": {{Name: "myproject_data", ConfigHash: ""}}}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -865,10 +867,10 @@ func TestReconcileVolumes_RenamedIsAdditive(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, + Networks: map[string][]ObservedNetwork{}, // Same compose key "data", but the live volume still has the old name. - Volumes: map[string]ObservedVolume{ - "data": {Name: "myproject_data", ConfigHash: mustVolumeHash(t, types.VolumeConfig{Name: "myproject_data", Driver: "local"})}, + Volumes: map[string][]ObservedVolume{ + "data": {{Name: "myproject_data", ConfigHash: mustVolumeHash(t, types.VolumeConfig{Name: "myproject_data", Driver: "local"})}}, }, } @@ -908,9 +910,9 @@ func TestReconcileVolumes_RenamedMigratesContainers(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{ - "data": {Name: "myproject_data", ConfigHash: mustVolumeHash(t, types.VolumeConfig{Name: "myproject_data", Driver: "local"})}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{ + "data": {{Name: "myproject_data", ConfigHash: mustVolumeHash(t, types.VolumeConfig{Name: "myproject_data", Driver: "local"})}}, }, } @@ -941,8 +943,8 @@ func TestReconcileVolumes_DivergedUnmountedVolume(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{"data": {Name: vol.Name, ConfigHash: "oldhash"}}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{"data": {{Name: vol.Name, ConfigHash: "oldhash"}}}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), yesPrompt) @@ -954,6 +956,42 @@ func TestReconcileVolumes_DivergedUnmountedVolume(t *testing.T) { `)+"\n") } +// TestReconcileVolumes_DuplicateLabelSelectsDesired verifies that when two live +// volumes share the compose label (e.g. a leftover after a rename), the +// reconciler deterministically selects the one matching the desired name — so a +// no-op `up` stays a no-op regardless of the daemon's list order — and warns +// about the orphan rather than acting on it. +func TestReconcileVolumes_DuplicateLabelSelectsDesired(t *testing.T) { + desired := types.VolumeConfig{Name: "pgdata_v2", Driver: "local"} + project := &types.Project{Name: "myproject", Volumes: types.Volumes{"data": desired}} + + // Both orders must yield the same outcome (deterministic selection). + current := ObservedVolume{Name: "pgdata_v2", ConfigHash: mustVolumeHash(t, desired)} + orphan := ObservedVolume{Name: "pgdata_v1", ConfigHash: "old"} + + for _, order := range [][]ObservedVolume{{orphan, current}, {current, orphan}} { + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{"data": order}, + } + + hook := logrustest.NewGlobal() + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + assert.Assert(t, plan.IsEmpty(), "matching volume selected -> no-op, got:\n%s", plan.String()) + + var warned bool + for _, e := range hook.AllEntries() { + if e.Level == logrus.WarnLevel && strings.Contains(e.Message, "pgdata_v1") { + warned = true + } + } + assert.Assert(t, warned, "orphan pgdata_v1 must be reported") + } +} + // --- Container tests --- func TestReconcileContainers_NewProject(t *testing.T) { @@ -966,8 +1004,8 @@ func TestReconcileContainers_NewProject(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{"web": {}}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -997,8 +1035,8 @@ func TestReconcileContainers_AlreadyRunning(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1024,8 +1062,8 @@ func TestReconcileContainers_ConfigChanged(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1058,8 +1096,8 @@ func TestReconcileContainers_ScaleUp(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1099,8 +1137,8 @@ func TestReconcileContainers_ScaleDown(t *testing.T) { }, }, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1131,8 +1169,8 @@ func TestReconcileContainers_ForceRecreate(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } opts := defaultReconcileOptions() @@ -1167,8 +1205,8 @@ func TestReconcileContainers_NeverRecreate(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } opts := defaultReconcileOptions() @@ -1198,8 +1236,8 @@ func TestReconcileContainers_ExitedIsNoop(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1229,8 +1267,8 @@ func TestReconcileContainers_DependsOnChain(t *testing.T) { observed := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1275,8 +1313,8 @@ func TestReconcileContainers_DependsOnScaleDown(t *testing.T) { }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) @@ -1304,8 +1342,8 @@ func TestReconcileOrphans(t *testing.T) { ID: "orphan1", Number: 1, Name: "myproject-old-1", Summary: container.Summary{ID: "orphan1"}, }}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } opts := defaultReconcileOptions() @@ -1366,8 +1404,8 @@ func parentDependentObserved(t *testing.T, parent, dependent types.ServiceConfig }, }}, }, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } } @@ -1470,8 +1508,8 @@ func TestReconcileContainers_MultipleParents_EitherTriggersCascade(t *testing.T) obs := &ObservedState{ ProjectName: "myproject", Containers: map[string][]ObservedContainer{}, - Networks: map[string]ObservedNetwork{}, - Volumes: map[string]ObservedVolume{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, } for name, svc := range map[string]types.ServiceConfig{"netparent": netParent, "volparent": volParent} { hash := mustServiceHash(t, svc)