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