mirror of
https://github.com/docker/compose.git
synced 2026-10-12 02:13:08 +00:00
fix(reconcile): retry network create on conflict, keep owned config-hash
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) <[email protected]> Signed-off-by: Nicolas De Loof <[email protected]>
This commit is contained in:
committed by
Guillaume Lours
co-authored by
Claude Opus 4.8
parent
f3647227b6
commit
d764fee527
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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() {}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user