diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index c3bab7cc2..2169d9a5c 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -21,6 +21,7 @@ import ( "fmt" "slices" "sort" + "strings" "time" "github.com/compose-spec/compose-go/v2/types" @@ -90,6 +91,19 @@ type reconciler struct { // can chain on the existing OpStopContainer instead of emitting a second // one against an already-stopped container. stoppedByPlan map[string]*PlanNode // container ID → existing Stop node + + // 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 + // parents are final — read by parentNamespaceRecreated to cascade + // recreates to dependents that would otherwise hold stale + // "container:" references. + recreatedServices map[string]bool + + // observedContainersByService memoizes ObservedState.containersByService() + // (an O(services * containers) build) for expectedConfigHash, which is + // called once per service. + observedContainersByService map[string]Containers } // reconcile is the main entry point: it builds a Plan from desired vs observed state. @@ -97,15 +111,17 @@ type reconciler struct { // reconciler.prompt field). func reconcile(_ context.Context, project *types.Project, observed *ObservedState, options ReconcileOptions, prompt Prompt) (*Plan, error) { r := &reconciler{ - project: project, - observed: observed, - options: options, - prompt: prompt, - plan: &Plan{}, - networkNodes: map[string]*PlanNode{}, - volumeNodes: map[string]*PlanNode{}, - serviceNodes: map[string]*PlanNode{}, - stoppedByPlan: map[string]*PlanNode{}, + project: project, + observed: observed, + options: options, + prompt: prompt, + plan: &Plan{}, + networkNodes: map[string]*PlanNode{}, + volumeNodes: map[string]*PlanNode{}, + serviceNodes: map[string]*PlanNode{}, + stoppedByPlan: map[string]*PlanNode{}, + recreatedServices: map[string]bool{}, + observedContainersByService: observed.containersByService(), } if err := r.reconcileNetworks(); err != nil { @@ -434,11 +450,18 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { strategy = r.options.Recreate } - // Sort containers: obsolete first, then by number descending, then reverse - // to get the same ordering as the existing convergence code. - if err := r.sortContainers(containers, service, strategy); err != nil { + // Precompute once per service: mustRecreate is called twice per container + // (sortContainers + main loop) and the hash/cascade inputs depend on the + // service, not the container. + expectedHash, err := serviceHashWithResolvedRefs(service, r.observedContainersByService) + if err != nil { return err } + parentRecreated := r.parentNamespaceRecreated(service) + + // Sort containers: obsolete first, then by number descending, then reverse + // to get the same ordering as the existing convergence code. + r.sortContainers(containers, service, expectedHash, parentRecreated, strategy) // Collect dependency nodes that container creation should depend on infraDeps := r.infrastructureDeps(service) @@ -467,12 +490,9 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { continue } - recreate, err := r.mustRecreate(service, oc, strategy) - if err != nil { - return err - } - if recreate { + if r.mustRecreate(service, expectedHash, parentRecreated, oc, strategy) { lastNode = r.planRecreateContainer(service, &containers[i], infraDeps) + r.recreatedServices[service.Name] = true continue } @@ -513,33 +533,69 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { return nil } -// mustRecreate mirrors the existing convergence.mustRecreate logic. -func (r *reconciler) mustRecreate(expected types.ServiceConfig, oc ObservedContainer, policy string) (bool, error) { - if policy == api.RecreateNever { - return false, nil +// mustRecreate decides whether oc must be recreated to match expected. The +// expectedHash and parentRecreated inputs are precomputed once per service by +// reconcileService — see expectedConfigHash and parentNamespaceRecreated for +// the rationale (issue #13878). +func (r *reconciler) mustRecreate(expected types.ServiceConfig, expectedHash string, parentRecreated bool, oc ObservedContainer, policy string) bool { + switch policy { + case api.RecreateNever: + return false + case api.RecreateForce: + return true } - if policy == api.RecreateForce { - return true, nil + if parentRecreated { + return true } - configHash, err := ServiceHash(expected) - if err != nil { - return false, err - } - if oc.ConfigHash != configHash { - return true, nil + if oc.ConfigHash != expectedHash { + return true } if oc.ImageDigest != expected.CustomLabels[api.ImageDigestLabel] { - return true, nil + return true } - if oc.State == container.StateRunning && r.hasNetworkMismatch(expected, oc) { - return true, nil - } - if r.hasVolumeMismatch(expected, oc) { - return true, nil + return true } + return r.hasVolumeMismatch(expected, oc) +} - return false, nil +// parentNamespaceRecreated reports whether any namespace- or volume-sharing +// parent of svc has at least one container scheduled for recreation. The +// parent set is derived from svc itself (network_mode/ipc/pid and volumes_from) +// rather than depends_on, so the cascade fires only when a stale +// "container:" reference would otherwise be left behind. +func (r *reconciler) parentNamespaceRecreated(svc types.ServiceConfig) bool { + for _, mode := range []string{svc.NetworkMode, svc.Ipc, svc.Pid} { + if name := getDependentServiceFromMode(mode); name != "" && r.recreatedServices[name] { + return true + } + } + for _, vol := range svc.VolumesFrom { + if strings.HasPrefix(vol, types.ContainerPrefix) { + continue + } + name, _, _ := strings.Cut(vol, ":") + if r.recreatedServices[name] { + return true + } + } + return false +} + +// serviceHashWithResolvedRefs mirrors what the executor persists at create +// time: service references (network_mode/ipc/pid: service:X, volumes_from) are +// resolved against the observed containers snapshot before hashing. On +// resolution failure (e.g. referenced parent absent) the raw form is hashed — +// it cannot match the persisted hash either way, so recreation is forced. +// +// Only fields mutated by resolveServiceReferences need defensive copying. +// svc.Networks (a map) is left shared because resolveServiceReferences does +// not touch it; revisit if that changes. +func serviceHashWithResolvedRefs(svc types.ServiceConfig, containers map[string]Containers) (string, error) { + resolved := svc + resolved.VolumesFrom = slices.Clone(svc.VolumesFrom) + _ = resolveServiceReferences(&resolved, containers) + return ServiceHash(resolved) } // hasNetworkMismatch checks if the container is not connected to all expected networks. @@ -676,7 +732,9 @@ func (r *reconciler) planRecreateContainer(service types.ServiceConfig, oc *Obse } // planStopDependents plans stop operations for containers of services that -// depend on the given service with restart: true. +// depend on the given service with restart: true. Each emitted Stop is +// recorded in stoppedByPlan so a later planRecreateContainer for the same +// dependent reuses it instead of emitting a duplicate Stop. func (r *reconciler) planStopDependents(service types.ServiceConfig) []*PlanNode { dependents := r.project.GetDependentsForService(service, func(dep types.ServiceDependency) bool { return dep.Restart @@ -684,6 +742,9 @@ func (r *reconciler) planStopDependents(service types.ServiceConfig) []*PlanNode var nodes []*PlanNode for _, depName := range dependents { for i, oc := range r.observed.Containers[depName] { + if _, already := r.stoppedByPlan[oc.ID]; already { + continue + } node := r.plan.addNode(Operation{ Type: OpStopContainer, ResourceID: fmt.Sprintf("service:%s:%d", depName, oc.Number), @@ -691,6 +752,7 @@ func (r *reconciler) planStopDependents(service types.ServiceConfig) []*PlanNode Container: &r.observed.Containers[depName][i].Summary, Timeout: r.options.Timeout, }, "") + r.stoppedByPlan[oc.ID] = node nodes = append(nodes, node) } } @@ -726,17 +788,12 @@ func (r *reconciler) infrastructureDeps(service types.ServiceConfig) []*PlanNode // sortContainers sorts containers the same way as convergence.go:138-160: // obsolete first, then by container number descending, then reversed. // -// mustRecreate is evaluated once per container before sorting, both to avoid -// quadratic re-evaluation in the comparator and to surface any hashing error -// instead of silently treating the container as non-obsolete. -func (r *reconciler) sortContainers(containers []ObservedContainer, service types.ServiceConfig, policy string) error { +// mustRecreate is evaluated once per container before sorting to avoid +// quadratic re-evaluation in the comparator. +func (r *reconciler) sortContainers(containers []ObservedContainer, service types.ServiceConfig, expectedHash string, parentRecreated bool, policy string) { obsolete := make(map[string]bool, len(containers)) for _, oc := range containers { - o, err := r.mustRecreate(service, oc, policy) - if err != nil { - return err - } - obsolete[oc.ID] = o + obsolete[oc.ID] = r.mustRecreate(service, expectedHash, parentRecreated, oc, policy) } sort.Slice(containers, func(i, j int) bool { obsi, obsj := obsolete[containers[i].ID], obsolete[containers[j].ID] @@ -750,7 +807,6 @@ func (r *reconciler) sortContainers(containers []ObservedContainer, service type return containers[i].Summary.Created < containers[j].Summary.Created }) slices.Reverse(containers) - return nil } // reconcileOrphans plans stop + remove for orphaned containers. diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index 918181d38..991b7a4f8 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -694,6 +694,216 @@ func TestReconcileOrphans(t *testing.T) { `)+"\n") } +// --- Service-reference resolution tests (issue #13878) --- +// +// When a service uses network_mode/ipc/pid: service:X or volumes_from: serviceName, +// the executor mutates the resolved field to "container:" (or bare ID for +// volumes_from) before computing and persisting the config-hash. The reconciler +// must hash the same resolved form so that an unchanged config does not appear +// as a divergence on the next `up`. + +// parentDependentObserved builds an ObservedState containing a "parent" container +// (running, with its own raw hash) and a "dependent" container whose hash was +// computed on the resolved form of its service. +func parentDependentObserved(t *testing.T, parent, dependent types.ServiceConfig) *ObservedState { + t.Helper() + const parentID = "parent_container_abc123" + parentSummary := container.Summary{ + ID: parentID, State: container.StateRunning, + Labels: map[string]string{ + api.ServiceLabel: "parent", + api.ContainerNumberLabel: "1", + api.ConfigHashLabel: mustServiceHash(t, parent), + }, + } + parentObserved := []ObservedContainer{{ + ID: parentID, Number: 1, State: container.StateRunning, + ConfigHash: mustServiceHash(t, parent), + Summary: parentSummary, + }} + containersByService := map[string]Containers{"parent": {parentSummary}} + dependentHash := mustResolvedServiceHash(t, dependent, containersByService) + return &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{ + "parent": parentObserved, + "dependent": {{ + ID: "dependent_container_xyz", Number: 1, State: container.StateRunning, + ConfigHash: dependentHash, + Summary: container.Summary{ + ID: "dependent_container_xyz", State: container.StateRunning, + Labels: map[string]string{ + api.ServiceLabel: "dependent", + api.ContainerNumberLabel: "1", + api.ConfigHashLabel: dependentHash, + }, + }, + }}, + }, + Networks: map[string]ObservedNetwork{}, + Volumes: map[string]ObservedVolume{}, + } +} + +// TestReconcileContainers_ServiceReference_NoRecreate covers every reference +// form mutated by resolveServiceReferences. The volumes_from "container:X" +// form is included because resolveVolumeFrom strips the prefix — the persisted +// hash differs from the raw user form even though no service reference is +// involved. +func TestReconcileContainers_ServiceReference_NoRecreate(t *testing.T) { + cases := []struct { + name string + mutate func(*types.ServiceConfig) + }{ + {"network_mode_service", func(s *types.ServiceConfig) { s.NetworkMode = "service:parent" }}, + {"ipc_service", func(s *types.ServiceConfig) { s.Ipc = "service:parent" }}, + {"pid_service", func(s *types.ServiceConfig) { s.Pid = "service:parent" }}, + {"volumes_from_service", func(s *types.ServiceConfig) { s.VolumesFrom = []string{"parent"} }}, + {"volumes_from_container", func(s *types.ServiceConfig) { s.VolumesFrom = []string{"container:some_external"} }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + parent := types.ServiceConfig{Name: "parent", Image: "alpine", Scale: intPtr(1)} + dependent := types.ServiceConfig{Name: "dependent", Image: "alpine", Scale: intPtr(1)} + tc.mutate(&dependent) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"parent": parent, "dependent": dependent}, + } + plan, err := reconcile(t.Context(), project, parentDependentObserved(t, parent, dependent), defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + assert.Assert(t, plan.IsEmpty(), "unexpected plan:\n%s", plan.String()) + }) + } +} + +// TestReconcileContainers_NamespaceParentRecreated_CascadesToDependent verifies +// that when a parent service is scheduled for recreation, dependents sharing +// its namespace (network_mode: service:X) are also recreated. Otherwise the +// dependent would keep a stale "container:" reference at runtime. +// The implicit depends_on {restart: true} matches what compose-go's normalizer +// produces for namespace-sharing services, so planStopDependents fires too — +// the test also asserts the resulting Stop is not duplicated. +func TestReconcileContainers_NamespaceParentRecreated_CascadesToDependent(t *testing.T) { + parent := types.ServiceConfig{Name: "parent", Image: "alpine", Scale: intPtr(1)} + dependent := types.ServiceConfig{ + Name: "dependent", Image: "alpine", Scale: intPtr(1), NetworkMode: "service:parent", + DependsOn: types.DependsOnConfig{"parent": {Condition: types.ServiceConditionStarted, Restart: true, Required: true}}, + } + project := &types.Project{ + Name: "myproject", + Services: types.Services{"parent": parent, "dependent": dependent}, + } + observed := parentDependentObserved(t, parent, dependent) + observed.Containers["parent"][0].ConfigHash = "stale_parent_hash" + observed.Containers["parent"][0].Summary.Labels[api.ConfigHashLabel] = "stale_parent_hash" + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + planStr := plan.String() + assert.Assert(t, strings.Contains(planStr, "service:parent:1, CreateContainer"), "parent must be recreated:\n%s", planStr) + assert.Assert(t, strings.Contains(planStr, "service:dependent:1, CreateContainer"), "dependent must cascade-recreate:\n%s", planStr) + // One Stop per container — planStopDependents must not duplicate the Stop + // emitted by planRecreateContainer for the dependent. + assert.Equal(t, strings.Count(planStr, "service:dependent:1, StopContainer"), 1, "duplicate Stop for dependent:\n%s", planStr) +} + +// TestReconcileContainers_MultipleParents_EitherTriggersCascade guards +// parentNamespaceRecreated against a future refactor that early-returns on a +// non-matching parent: a dependent sharing namespace with two parents must +// cascade-recreate when either parent is scheduled for recreation. +func TestReconcileContainers_MultipleParents_EitherTriggersCascade(t *testing.T) { + netParent := types.ServiceConfig{Name: "netparent", Image: "alpine", Scale: intPtr(1)} + volParent := types.ServiceConfig{Name: "volparent", Image: "alpine", Scale: intPtr(1)} + dependent := types.ServiceConfig{ + Name: "dependent", Image: "alpine", Scale: intPtr(1), + NetworkMode: "service:netparent", + VolumesFrom: []string{"volparent"}, + // Mirrors what compose-go's normalizer injects for namespace-sharing + // references, so the dependency graph orders parents before dependent. + DependsOn: types.DependsOnConfig{ + "netparent": {Condition: types.ServiceConditionStarted, Restart: true, Required: true}, + "volparent": {Condition: types.ServiceConditionStarted, Restart: true, Required: true}, + }, + } + project := &types.Project{ + Name: "myproject", + Services: types.Services{"netparent": netParent, "volparent": volParent, "dependent": dependent}, + } + + // Build a containersByService map containing both parents so the + // dependent's resolved hash can be computed. + parents := map[string]Containers{ + "netparent": {container.Summary{ID: "netparent_id", Labels: map[string]string{api.ServiceLabel: "netparent"}}}, + "volparent": {container.Summary{ID: "volparent_id", Labels: map[string]string{api.ServiceLabel: "volparent"}}}, + } + dependentHash := mustResolvedServiceHash(t, dependent, parents) + + makeObserved := func(staleService string) *ObservedState { + obs := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + Networks: map[string]ObservedNetwork{}, + Volumes: map[string]ObservedVolume{}, + } + for name, svc := range map[string]types.ServiceConfig{"netparent": netParent, "volparent": volParent} { + hash := mustServiceHash(t, svc) + if name == staleService { + hash = "stale" + } + obs.Containers[name] = []ObservedContainer{{ + ID: name + "_id", Number: 1, State: container.StateRunning, ConfigHash: hash, + Summary: container.Summary{ + ID: name + "_id", State: container.StateRunning, + Labels: map[string]string{api.ServiceLabel: name, api.ContainerNumberLabel: "1", api.ConfigHashLabel: hash}, + }, + }} + } + obs.Containers["dependent"] = []ObservedContainer{{ + ID: "dependent_id", Number: 1, State: container.StateRunning, ConfigHash: dependentHash, + Summary: container.Summary{ + ID: "dependent_id", State: container.StateRunning, + Labels: map[string]string{api.ServiceLabel: "dependent", api.ContainerNumberLabel: "1", api.ConfigHashLabel: dependentHash}, + }, + }} + return obs + } + + for _, staleParent := range []string{"netparent", "volparent"} { + t.Run("stale_"+staleParent, func(t *testing.T) { + plan, err := reconcile(t.Context(), project, makeObserved(staleParent), defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + planStr := plan.String() + assert.Assert(t, strings.Contains(planStr, "service:dependent:1, CreateContainer"), "dependent must cascade-recreate when %s is recreated:\n%s", staleParent, planStr) + }) + } +} + +// TestReconcileContainers_RegularDependsOn_NoCascade ensures the cascade fires +// only for namespace/volume-sharing dependencies, not for plain depends_on. +func TestReconcileContainers_RegularDependsOn_NoCascade(t *testing.T) { + parent := types.ServiceConfig{Name: "parent", Image: "alpine", Scale: intPtr(1)} + dependent := types.ServiceConfig{ + Name: "dependent", Image: "alpine", Scale: intPtr(1), + DependsOn: types.DependsOnConfig{"parent": {Condition: types.ServiceConditionStarted, Restart: true}}, + } + project := &types.Project{ + Name: "myproject", + Services: types.Services{"parent": parent, "dependent": dependent}, + } + observed := parentDependentObserved(t, parent, dependent) + observed.Containers["parent"][0].ConfigHash = "stale_parent_hash" + observed.Containers["parent"][0].Summary.Labels[api.ConfigHashLabel] = "stale_parent_hash" + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + planStr := plan.String() + assert.Assert(t, strings.Contains(planStr, "service:parent:1, CreateContainer"), "parent must be recreated:\n%s", planStr) + assert.Assert(t, !strings.Contains(planStr, "service:dependent:1, CreateContainer"), "dependent must NOT recreate without namespace sharing:\n%s", planStr) +} + // --- Helpers --- func mustServiceHash(t *testing.T, svc types.ServiceConfig) string { @@ -702,3 +912,14 @@ func mustServiceHash(t *testing.T, svc types.ServiceConfig) string { 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: +// service:X or volumes_from: serviceName. +func mustResolvedServiceHash(t *testing.T, svc types.ServiceConfig, containers map[string]Containers) string { + t.Helper() + h, err := serviceHashWithResolvedRefs(svc, containers) + assert.NilError(t, err) + return h +}