fix(reconcile): hash resolved service refs to match executor

The reconciler hashed the raw service config while the executor
hashed the form with network_mode/ipc/pid/volumes_from references
resolved to container IDs. Persisted hash and recomputed hash
never matched, so dependents were recreated on every `up`.

Resolve references against observed containers before hashing,
and cascade recreation to namespace-sharing dependents when a
parent is replaced — otherwise the dependent would keep a stale
"container:<old_id>" reference. Also dedup stops in
planStopDependents via stoppedByPlan: with the cascade restored,
the dependent would otherwise receive two Stop nodes.

Fixes #13878.

Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
This commit is contained in:
Guillaume Lours 2026-06-26 19:20:01 +02:00 committed by Guillaume Lours
parent a2fb49dc68
commit 55c61ceef9
2 changed files with 324 additions and 47 deletions

View file

@ -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:<old_id>" 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:<id>" 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.

View file

@ -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:<id>" (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:<old_id>" 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
}