From ff0c5cdcaf2de5391ef602d664e2858ea971af02 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Mon, 20 Apr 2026 10:44:12 +0200 Subject: [PATCH] Fix code review issues: replace label, observed names for disconnect/remove - Fix ContainerReplaceLabel detection: use op.Inherited != nil (not op.Container) as signal for recreate in execCreateContainer - Use observed network name (not desired) for DisconnectNetwork and RemoveNetwork operations, in case the name changed - Use observed volume name (not desired) for RemoveVolume operations - Update reconciliation.md with 3 new lessons learned (7.8, 7.9, 7.10) Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Nicolas De Loof --- pkg/compose/executor.go | 2 +- pkg/compose/reconcile.go | 16 ++++++++-------- reconciliation.md | 22 ++++++++++++++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index 6746b5a17..8fe3bc46c 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -207,7 +207,7 @@ func (exec *planExecutor) execCreateContainer(ctx context.Context, node *PlanNod } labels := mergeLabels(service.Labels, service.CustomLabels) - if op.Container != nil { + if op.Inherited != nil { // This is a recreate: add the replace label replacedName := op.Service.ContainerName if replacedName == "" { diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index e3392fda1..aab5c2b09 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -148,6 +148,7 @@ func (r *reconciler) planCreateNetwork(key string, nw *types.NetworkConfig) *Pla // 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) @@ -164,7 +165,7 @@ func (r *reconciler) planRecreateNetwork(key string, nw *types.NetworkConfig) er stopNodes = append(stopNodes, node) } - // Disconnect all affected containers (each depends on its own stop) + // 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{ @@ -172,18 +173,17 @@ func (r *reconciler) planRecreateNetwork(key string, nw *types.NetworkConfig) er ResourceID: fmt.Sprintf("service:%s:%d", oc.Summary.Labels[serviceLabel], oc.Number), Cause: fmt.Sprintf("network %s recreate", key), Container: &affectedContainers[i].Summary, - Name: nw.Name, + Name: observed.Name, }, "", stopNodes[i]) disconnectNodes = append(disconnectNodes, node) } - // Remove network (depends on all disconnects) + // 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: nw.Name, - Network: nw, + Name: observed.Name, }, "", disconnectNodes...) // Create network (depends on remove) @@ -251,6 +251,7 @@ func (r *reconciler) planCreateVolume(key string, vol *types.VolumeConfig) *Plan // Containers must be removed (not just stopped) because Docker does not allow // removing a volume that is referenced by any container, even a stopped one. func (r *reconciler) planRecreateVolume(key string, vol *types.VolumeConfig) { + observed := r.observed.Volumes[key] affectedServices := r.servicesUsingVolume(key) affectedContainers := r.containersForServices(affectedServices) @@ -279,13 +280,12 @@ func (r *reconciler) planRecreateVolume(key string, vol *types.VolumeConfig) { removeNodes = append(removeNodes, node) } - // Remove volume (depends on all container removals) + // Remove the *observed* volume (depends on all container removals) removeVolNode := r.plan.addNode(Operation{ Type: OpRemoveVolume, ResourceID: fmt.Sprintf("volume:%s", key), Cause: "config hash diverged", - Name: vol.Name, - Volume: vol, + Name: observed.Name, }, "", removeNodes...) // Create volume (depends on remove) diff --git a/reconciliation.md b/reconciliation.md index bfc282cf6..a4b54fa1e 100644 --- a/reconciliation.md +++ b/reconciliation.md @@ -235,6 +235,28 @@ dans les tests, il faut trier les cles de maps partout : `reconcileNetworks`, `sortedKeys[V any](map[string]V)` centralise ce pattern. Le `String()` du plan trie aussi les IDs de dependances. +### 7.8 Disconnect/Remove utilisent les noms observes, pas desires + +Quand un reseau ou volume a diverge, les operations de disconnect et remove doivent +cibler la ressource **observee** (l'ancienne), pas la ressource **desiree** (la nouvelle +config). Si le nom a change entre les deux, utiliser le nom desire pour le remove +echouerait ou supprimerait la mauvaise ressource. Seul le `CreateNetwork`/`CreateVolume` +utilise le nom desire. + +### 7.9 Le label ContainerReplaceLabel necessite un signal fiable pour le recreate + +Dans `execCreateContainer`, le label `api.ContainerReplaceLabel` doit etre ajoute pour +les conteneurs crees en remplacement d'un autre (recreate). Le signal fiable est +`op.Inherited != nil` (conteneur dont on herite les volumes anonymes), pas +`op.Container != nil` (qui n'est pas set sur le noeud CreateContainer du recreate). + +### 7.10 La boucle `for range` en Go 1.22+ + +Depuis Go 1.22, les variables de boucle `for _, x := range` sont copiees a chaque +iteration. Le pattern `x := x` dans les goroutines n'est plus necessaire. Docker Compose +utilise Go 1.25+. Si le projet devait un jour revenir a une version anterieure a 1.22, +les closures dans `executePlan` devraient capturer explicitement la variable de boucle. + --- ## 8. Strategie d'implementation recommandee