fix(reconcile): remove volumes_from consumers before volume recreation

servicesUsingVolume only matched services mounting the volume directly, so a service reaching it through volumes_from was not stopped/removed before RemoveVolume. Docker materializes the inherited mount on the consumer's container, so its removal would fail with "volume in use". Compute the transitive volumes_from closure so every container referencing the volume is removed first. (network_mode/ipc/pid: service:x share namespaces, not mounts, and are intentionally excluded.) Also reassign the result of Labels.Add in createVolume: it mutates in place only when the map is non-nil, so discarding the return would drop the config-hash label for a volume with no CustomLabels. Addresses review feedback: documents why observed.Containers is cleared without touching the observedContainersByService hashing snapshot, and strengthens the cascade tests to assert the full plan ordering. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Nicolas De Loof <[email protected]>
2026-10-12 02:13:08 +00:00 · 2026-07-23 16:17:43 +02:00
parent 44ac2c94e9
commit b45991bf47
3 changed files with 149 additions and 14 deletions
@@ -1638,7 +1638,7 @@ func (s *composeService) createVolume(ctx context.Context, volume types.VolumeCo
 	if err != nil {
 		return err
 	}
-	volume.CustomLabels.Add(api.ConfigHashLabel, hash)
+	volume.CustomLabels = volume.CustomLabels.Add(api.ConfigHashLabel, hash)
 	_, err = s.apiClient().VolumeCreate(ctx, client.VolumeCreateOptions{
 		Labels:     mergeLabels(volume.Labels, volume.CustomLabels),
 		Name:       volume.Name,
@@ -365,7 +365,17 @@ func (r *reconciler) planRecreateVolumes(keys []string) {
 		r.volumeNodes[key] = createVolNode
 	}

-	// Hand container re-creation to reconcileContainers.
+	// Hand container re-creation to reconcileContainers: cleared services are
+	// seen as absent and scheduled fresh (gated on their CreateVolume node via
+	// infrastructureDeps), and marking them recreated cascades to
+	// namespace/volume-sharing dependents.
+	//
+	// Only observed.Containers is cleared, not the observedContainersByService
+	// snapshot memoized at reconciler init: that snapshot backs config-hash
+	// resolution (serviceHashWithResolvedRefs), which must mirror the state the
+	// executor hashed against at create time, whereas clearing here is purely a
+	// scheduling concern carried by the plan's dependency edges. The two
+	// intentionally diverge; do not "fix" one to match the other.
 	for _, svc := range services {
 		r.recreatedServices[svc] = true
 		r.observed.Containers[svc] = nil
@@ -385,20 +395,56 @@ func (r *reconciler) servicesUsingNetwork(networkKey string) []string {
 	return names
 }

-// servicesUsingVolume returns the names of services that mount the given
-// compose volume key, sorted for deterministic plan output.
+// servicesUsingVolume returns the names of services whose containers reference
+// the given compose volume — either by mounting it directly (service.Volumes) or
+// by inheriting the mount transitively through volumes_from. Every such service's
+// containers must be removed before the volume can be removed: Docker refuses to
+// remove a volume still referenced by any container, and volumes_from
+// materializes the source's mounts on the target container. Sorted for
+// deterministic plan output.
+//
+// Only volumes_from propagates a *mount* (and therefore a volume reference);
+// network_mode/ipc/pid: service:x share namespaces, not mounts, so they do not
+// keep a volume in use and are intentionally excluded here.
 func (r *reconciler) servicesUsingVolume(volumeKey string) []string {
-	var names []string
+	inSet := map[string]bool{}
+	// Seed with services that mount the volume directly.
 	for _, key := range sortedKeys(r.project.Services) {
 		svc := r.project.Services[key]
 		for _, v := range svc.Volumes {
 			if v.Source == volumeKey {
-				names = append(names, svc.Name)
+				inSet[svc.Name] = true
 				break
 			}
 		}
 	}
-	return names
+	// Grow the set by transitive volumes_from closure until it stabilizes: a
+	// service inherits the mount when it draws volumes from a service already in
+	// the set (references to external containers carry no compose dependency).
+	for {
+		added := false
+		for _, key := range sortedKeys(r.project.Services) {
+			svc := r.project.Services[key]
+			if inSet[svc.Name] {
+				continue
+			}
+			for _, vf := range svc.VolumesFrom {
+				if strings.HasPrefix(vf, types.ContainerPrefix) {
+					continue
+				}
+				name, _, _ := strings.Cut(vf, ":")
+				if inSet[name] {
+					inSet[svc.Name] = true
+					added = true
+					break
+				}
+			}
+		}
+		if !added {
+			break
+		}
+	}
+	return sortedKeys(inSet)
 }

 // containersForServices returns all observed containers belonging to the given
@@ -616,14 +616,103 @@ func TestReconcileVolumes_DivergedCascadesToDependent(t *testing.T) {
 	plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), yesPrompt)
 	assert.NilError(t, err)

+	// The dependent inherits the volume mount via volumes_from, so its container
+	// must be stopped and removed before RemoveVolume (#5 depends on both #2 and
+	// #4) — otherwise the removal would fail with "volume in use". Both fresh
+	// containers are then gated on the new volume: owner (#7) depends on
+	// CreateVolume (#6), and the dependent (#8) depends on owner (#7).
+	assert.Equal(t, plan.String(), strings.TrimSpace(`
+[] -> #1 service:dependent:1, StopContainer, mounted volume config changed
+[1] -> #2 service:dependent:1, RemoveContainer, mounted volume config changed
+[] -> #3 service:owner:1, StopContainer, mounted volume config changed
+[3] -> #4 service:owner:1, RemoveContainer, mounted volume config changed
+[2,4] -> #5 volume:data, RemoveVolume, config hash diverged
+[5] -> #6 volume:data, CreateVolume, recreate after config change
+[6] -> #7 service:owner:1, CreateContainer, no existing container
+[7] -> #8 service:dependent:1, CreateContainer, no existing container
+`)+"\n")
+}
+
+// TestReconcileVolumes_DivergedVolumesFromRemovedBeforeVolume specifically guards
+// against a "volume in use" failure: a service reaching the diverged volume only
+// through volumes_from (never mounting it directly) must still have its container
+// removed before the volume is removed.
+func TestReconcileVolumes_DivergedVolumesFromRemovedBeforeVolume(t *testing.T) {
+	vol := types.VolumeConfig{Name: "myproject_data", Driver: "local"}
+	owner := types.ServiceConfig{
+		Name:    "owner",
+		Image:   "alpine",
+		Scale:   intPtr(1),
+		Volumes: []types.ServiceVolumeConfig{{Source: "data", Type: "volume"}},
+	}
+	// consumer inherits owner's mounts (including data) but never declares the
+	// volume itself.
+	consumer := types.ServiceConfig{
+		Name:        "consumer",
+		Image:       "alpine",
+		Scale:       intPtr(1),
+		VolumesFrom: []string{"owner"},
+		DependsOn:   types.DependsOnConfig{"owner": {Condition: types.ServiceConditionStarted, Required: true}},
+	}
+	project := &types.Project{
+		Name:     "myproject",
+		Volumes:  types.Volumes{"data": vol},
+		Services: types.Services{"owner": owner, "consumer": consumer},
+	}
+	ownerHash := mustServiceHash(t, owner)
+	ownerSummary := container.Summary{
+		ID: "owner-1", State: container.StateRunning,
+		Labels: map[string]string{api.ServiceLabel: "owner", api.ContainerNumberLabel: "1", api.ConfigHashLabel: ownerHash},
+		Mounts: []container.MountPoint{{Type: "volume", Name: vol.Name}},
+	}
+	consumerHash := mustResolvedServiceHash(t, consumer, map[string]Containers{"owner": {ownerSummary}})
+	observed := &ObservedState{
+		ProjectName: "myproject",
+		Containers: map[string][]ObservedContainer{
+			"owner": {{ID: "owner-1", Number: 1, State: container.StateRunning, ConfigHash: ownerHash, Summary: ownerSummary}},
+			"consumer": {{
+				ID: "consumer-1", Number: 1, State: container.StateRunning, ConfigHash: consumerHash,
+				Summary: container.Summary{
+					ID: "consumer-1", State: container.StateRunning,
+					Labels: map[string]string{api.ServiceLabel: "consumer", api.ContainerNumberLabel: "1", api.ConfigHashLabel: consumerHash},
+					// Docker materializes the inherited mount on the consumer.
+					Mounts: []container.MountPoint{{Type: "volume", Name: vol.Name}},
+				},
+			}},
+		},
+		Networks: map[string]ObservedNetwork{},
+		Volumes:  map[string]ObservedVolume{"data": {Name: vol.Name, ConfigHash: "oldhash"}},
+	}
+
+	plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), yesPrompt)
+	assert.NilError(t, err)
+
 	planStr := plan.String()
-	// Volume recreate sequence for the owner.
-	assert.Assert(t, strings.Contains(planStr, "service:owner:1, RemoveContainer, mounted volume config changed"), planStr)
-	assert.Assert(t, strings.Contains(planStr, "volume:data, RemoveVolume, config hash diverged"), planStr)
-	assert.Assert(t, strings.Contains(planStr, "volume:data, CreateVolume, recreate after config change"), planStr)
-	assert.Assert(t, strings.Contains(planStr, "service:owner:1, CreateContainer"), planStr)
-	// Cascade: the dependent must be recreated too.
-	assert.Assert(t, strings.Contains(planStr, "service:dependent:1, CreateContainer"), "dependent must cascade-recreate:\n%s", planStr)
+	// The volumes_from consumer is stopped and removed as part of the volume
+	// recreation batch, even though it never declares the volume.
+	assert.Assert(t, strings.Contains(planStr, "service:consumer:1, RemoveContainer, mounted volume config changed"),
+		"volumes_from consumer must be removed before RemoveVolume:\n%s", planStr)
+
+	// RemoveVolume must depend on the consumer's RemoveContainer node.
+	var removeConsumer, removeVolume *PlanNode
+	for _, n := range plan.Nodes {
+		if n.Operation.Type == OpRemoveContainer && n.Operation.ResourceID == "service:consumer:1" {
+			removeConsumer = n
+		}
+		if n.Operation.Type == OpRemoveVolume {
+			removeVolume = n
+		}
+	}
+	assert.Assert(t, removeConsumer != nil, "no RemoveContainer for consumer:\n%s", planStr)
+	assert.Assert(t, removeVolume != nil, "no RemoveVolume:\n%s", planStr)
+	found := false
+	for _, dep := range removeVolume.DependsOn {
+		if dep == removeConsumer {
+			found = true
+			break
+		}
+	}
+	assert.Assert(t, found, "RemoveVolume must depend on the consumer's RemoveContainer:\n%s", planStr)
 }

 // TestReconcileVolumes_DivergedUnmountedVolume verifies that a diverged volume