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) <noreply@anthropic.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This commit is contained in:
Nicolas De Loof 2026-04-20 10:44:12 +02:00
parent ef364939f0
commit ff0c5cdcaf
No known key found for this signature in database
GPG key ID: 9858809D6F8F6E7E
3 changed files with 31 additions and 9 deletions

View file

@ -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 == "" {

View file

@ -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)

View file

@ -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