From 32bddfc4c693dd1b9ed8633ff2adb2d511a07ee1 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Fri, 25 Sep 2026 15:25:14 +0200 Subject: [PATCH] fix: provider migration condemns the service's stale replicas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A service migrated from regular replicas to a provider left its old containers standing: not converged (a provider service has no replicas to converge), not orphaned (the service is still in the model). The first up after the migration then failed creating the relay, because the leftover replica still held the service's canonical container name the relay takes over. And even without the name conflict (replicas 2..n of a scaled service), stale replicas kept the service's network alias, competing with the relay on DNS and serving outdated traffic. The reconciler now plans a stop+remove for every observed non-relay container of a provider service, and the RunProvider node depends on those removals: the canonical name is free by the time ensureServiceRelay creates the relay. The relay itself (RelayLabel) is the provider service's legitimate container and is left alone — converged by ensureServiceRelay, not by the plan. The e2e scenario migrates a deployed service to a provider and locks the replacement (Recreated, relay reachable at the compose-native address); the Scenario DSL gains FromFile — a local mirror of FromRemote — to switch the model between steps. Signed-off-by: Nicolas De Loof --- pkg/compose/reconcile.go | 34 ++++++++ pkg/compose/reconcile_test.go | 77 +++++++++++++++++++ pkg/e2e/providers_test.go | 20 +++++ pkg/e2e/scenario.go | 19 +++++ .../TestProviderMigration/compose.yaml | 5 ++ .../TestProviderMigration/migrated.yaml | 13 ++++ 6 files changed, 168 insertions(+) create mode 100644 pkg/e2e/testdata/TestProviderMigration/compose.yaml create mode 100644 pkg/e2e/testdata/TestProviderMigration/migrated.yaml diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index a7127d78d..c3d166770 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -720,6 +720,40 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { if service.Provider != nil { serviceCopy := service deps := r.infrastructureDeps(service) + // A service migrated from replicas to a provider leaves its old + // containers behind: not converged (a provider service has no + // replicas to converge) and not orphaned (the service is still in + // the model). They must go before the provider's relay stands in: + // the relay takes over the service's canonical container name and + // its network alias, so a leftover replica is both a name conflict + // at relay creation and a competing DNS entry serving stale + // traffic. The relay itself (RelayLabel) is the provider service's + // legitimate container and is left alone — converged by + // ensureServiceRelay, not by the plan. + for i := range r.observed.Containers[service.Name] { + oc := &r.observed.Containers[service.Name][i] + if isRelayContainer(oc.Summary) { + continue + } + resID := serviceReplicaID(service.Name, oc.Number) + stopNode := r.plan.addNode(Operation{ + Type: OpStopContainer, + ResourceID: resID, + Cause: "service is now provider-backed", + Container: &oc.Summary, + Timeout: r.options.Timeout, + }, "") + removeNode := r.plan.addNode(Operation{ + Type: OpRemoveContainer, + ResourceID: resID, + Cause: "service is now provider-backed", + Container: &oc.Summary, + }, "", stopNode) + r.removedByPlan[resID] = true + // the provider run deploys the relay: the canonical name must + // be free by then + deps = append(deps, removeNode) + } node := r.plan.addNode(Operation{ Type: OpRunProvider, ResourceID: "provider:" + service.Name, diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index 9d89275de..981ab2f55 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -183,6 +183,83 @@ func TestReconcileNetworks_Diverged(t *testing.T) { `)+"\n") } +// A service migrated from regular replicas to a provider must have its old +// containers condemned by the plan: they are neither converged (a provider +// service has no replicas) nor orphaned (the service is still in the model), +// yet they hold the canonical container name the relay takes over and the +// service's network alias. The relay itself is the provider service's +// legitimate container and must be left alone. +func TestReconcileService_ProviderMigrationRemovesStaleReplicas(t *testing.T) { + db := types.ServiceConfig{Name: "db", Provider: &types.ServiceProviderConfig{Type: "test"}} + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db}, + } + stale := ObservedContainer{ + ID: "old-db", Number: 1, State: container.StateRunning, ConfigHash: "prehash", + Summary: container.Summary{ + ID: "old-db", State: container.StateRunning, + Labels: map[string]string{ + api.ServiceLabel: "db", + api.ContainerNumberLabel: "1", + api.ConfigHashLabel: "prehash", + }, + }, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{"db": {stale}}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + // the provider run deploys the relay under the canonical name: it must + // depend on the removal that frees it + assert.Equal(t, plan.String(), strings.TrimSpace(` +[] -> #1 service:db:1, StopContainer, service is now provider-backed +[1] -> #2 service:db:1, RemoveContainer, service is now provider-backed +[2] -> #3 provider:db, RunProvider, provider service +`)+"\n") +} + +// An up-to-date relay container observed for a provider service is NOT the +// plan's to touch: ensureServiceRelay converges it during the provider run. +func TestReconcileService_ProviderRelayLeftAlone(t *testing.T) { + db := types.ServiceConfig{Name: "db", Provider: &types.ServiceProviderConfig{Type: "test"}} + project := &types.Project{ + Name: "myproject", + Services: types.Services{"db": db}, + } + relay := ObservedContainer{ + ID: "relay-db", Number: 1, State: container.StateRunning, ConfigHash: "relay-identity", + Summary: container.Summary{ + ID: "relay-db", State: container.StateRunning, + Labels: map[string]string{ + api.ServiceLabel: "db", + api.ContainerNumberLabel: "1", + api.ConfigHashLabel: "relay-identity", + api.RelayLabel: "relay-identity", + }, + }, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{"db": {relay}}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + assert.Equal(t, plan.String(), strings.TrimSpace(` +[] -> #1 provider:db, RunProvider, provider service +`)+"\n") +} + // TestReconcileNetworks_DivergedAlsoRecreatesChangedContainer verifies the // entangled case: when a container attached to a diverged network also has its // own config changed, it is both reconnected (by the network recreate) and diff --git a/pkg/e2e/providers_test.go b/pkg/e2e/providers_test.go index 999c9df28..fe9fdeccd 100644 --- a/pkg/e2e/providers_test.go +++ b/pkg/e2e/providers_test.go @@ -107,6 +107,26 @@ func TestProviderRawSetEnvOverridesInheritedEnvMapForm(t *testing.T) { OutputContains("overrides environment variable")) } +func TestProviderMigration(t *testing.T) { + // Regression: migrating an already-deployed service to a provider left + // its replica standing — neither converged (provider services have no + // replicas) nor orphaned (the service is still in the model) — and the + // relay creation then failed with a name conflict on the canonical + // container name the replica still held. + relayImage := "compose-relay-e2e" + s := providerScenario(t, "migrating a deployed service to a provider must replace its replica with the relay") + s.CLI().RunCmd(t, "docker", "build", "-t", relayImage, "../../relay") + s.Env("PROVIDER_DEMO_ENDPOINT=1", "COMPOSE_RELAY_IMAGE="+relayImage) + s.Step("the service first runs as a regular container", + ComposeCmd("up", "-d"), + ServiceState("db", "running")) + s.FromFile("migrated.yaml") + s.Step("up after the migration replaces the replica with the relay, reachable at the compose-native address", + ComposeCmd("up"), + Recreated("db"), + OutputContains("test-1 | hello from provider")) +} + func TestProviderPublishEndpoint(t *testing.T) { // The example provider stands up a real endpoint on the host and // publishes it; compose deploys a relay under the service's name, so the diff --git a/pkg/e2e/scenario.go b/pkg/e2e/scenario.go index 3d43752c7..cd2c3a145 100644 --- a/pkg/e2e/scenario.go +++ b/pkg/e2e/scenario.go @@ -268,6 +268,25 @@ func (s *Scenario) FromRemote(source string, rootFlags ...string) *Scenario { return s } +// FromFile switches the scenario's compose file to another file of the +// anchored project directory — for scenarios whose intent is a model +// migration: steps before the switch ran the initial model, steps after it +// run the migrated one, against the same project. +func (s *Scenario) FromFile(name string) *Scenario { + s.t.Helper() + if s.file == "" { + s.t.Fatal("FromFile requires an anchored testdata directory") + } + file := filepath.Join(filepath.Dir(s.file), name) + if _, err := os.Stat(file); err != nil { + s.t.Fatalf("FromFile(%q): %v", name, err) + } + s.remote = "" + s.rootArgs = nil + s.file = file + return s +} + // Requires skips the scenario unless every requirement is met by the target // environment. func (s *Scenario) Requires(reqs ...Requirement) *Scenario { diff --git a/pkg/e2e/testdata/TestProviderMigration/compose.yaml b/pkg/e2e/testdata/TestProviderMigration/compose.yaml new file mode 100644 index 000000000..b190baf9f --- /dev/null +++ b/pkg/e2e/testdata/TestProviderMigration/compose.yaml @@ -0,0 +1,5 @@ +services: + db: + image: alpine + init: true + command: sleep infinity diff --git a/pkg/e2e/testdata/TestProviderMigration/migrated.yaml b/pkg/e2e/testdata/TestProviderMigration/migrated.yaml new file mode 100644 index 000000000..885287740 --- /dev/null +++ b/pkg/e2e/testdata/TestProviderMigration/migrated.yaml @@ -0,0 +1,13 @@ +services: + test: + image: alpine + command: wget -qO- -T 10 http://db + depends_on: + - db + db: + provider: + type: example-provider + options: + name: db + type: test1 + size: 1