From 6578c79b72b46ef53250c6ecf253566ed1d4cf14 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Wed, 9 Sep 2026 10:14:33 +0200 Subject: [PATCH 1/2] feat: reconciler plans the purge of stale pre_start hook runners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pre_start hooks run in ephemeral containers. When a run fails, the runner is deliberately retained for post-mortem inspection, and the next run purges it — but that purge lived entirely inside the imperative primitive, invisible to the reconciliation plan. It is now a plan operation: when pre_start is going to run again (hooks declared, no replica running at observation — the imperative gating), the plan emits one best-effort RemoveContainer per stale runner, dropping its anonymous volumes, exactly the warn-only semantics of the imperative purge that remains in place as backstop. Observed state learns to tell hook containers apart: they carry no container-number label and previously classified as a service replica numbered 0. They now land in a dedicated HookContainers bucket the reconciler plans purges from. The imperative primitive is also split into its lifecycle-free execution piece (execPreStartHook: start, wait, log streaming, retain-on-failure) and the create/remove pieces around it, recomposed identically in runPreStart — locked by the existing characterization tests. This prepares moving hook-container creation and post-success removal into the plan once the start phase lands (#14200). Signed-off-by: Nicolas De Loof --- pkg/compose/executor_ops.go | 9 +- pkg/compose/executor_test.go | 32 ++++++++ pkg/compose/observed_state.go | 19 +++++ pkg/compose/observed_state_test.go | 44 +++++++++- pkg/compose/plan.go | 14 ++-- pkg/compose/pre_start.go | 52 ++++++------ pkg/compose/reconcile.go | 37 +++++++++ pkg/compose/reconcile_test.go | 128 +++++++++++++++++++++++++++++ 8 files changed, 302 insertions(+), 33 deletions(-) diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index 13738d045b..7dc936c7cd 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -142,8 +142,15 @@ func (exec *planExecutor) execStopContainer(ctx context.Context, op Operation) e } func (exec *planExecutor) execRemoveContainer(ctx context.Context, op Operation) error { - _, err := exec.compose.apiClient().ContainerRemove(ctx, op.Container.ID, client.ContainerRemoveOptions{Force: true}) + _, err := exec.compose.apiClient().ContainerRemove(ctx, op.Container.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: op.RemoveVolumes}) if err != nil { + if op.BestEffort { + // warn-only removal (stale pre_start hook runner): the container + // stays visible to the operator, the plan carries on — and the + // live view below keeps it, since it was not removed + logrus.Warnf("failed to remove %s: %v", op.ResourceID, err) + return nil + } return err } // Why: a dependent service's create may resolve `network_mode: service:X` diff --git a/pkg/compose/executor_test.go b/pkg/compose/executor_test.go index 9e711c22f3..bbfb8b4896 100644 --- a/pkg/compose/executor_test.go +++ b/pkg/compose/executor_test.go @@ -174,6 +174,38 @@ func emptyObservedState(project string) *ObservedState { // Goes through newPlanExecutor + run (i.e. the same code path executePlan // uses in production) so the test exercises the errgroup, done-channel // wiring and group tracker — not a hand-rolled loop over executeNode. +// A best-effort removal failure (stale pre_start hook runner purge) is +// warn-only: the plan carries on, and the removal passes RemoveVolumes so the +// runner's anonymous volumes go with it — the imperative purge semantics. +func TestExecutePlanBestEffortRemoveContainerFailureTolerated(t *testing.T) { + svc, apiClient := newTestService(t) + + ctr := container.Summary{ + ID: "hook1", + Names: []string{"/some-hook-runner"}, + Labels: map[string]string{api.ServiceLabel: "web"}, + } + + apiClient.EXPECT().ContainerRemove(gomock.Any(), "hook1", gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, opts client.ContainerRemoveOptions) (client.ContainerRemoveResult, error) { + assert.Assert(t, opts.RemoveVolumes, "hook-runner purge must drop anonymous volumes") + return client.ContainerRemoveResult{}, errors.New("device or resource busy") + }) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpRemoveContainer, + ResourceID: "hook:web:stale:hook1", + Cause: "stale pre_start hook container", + Container: &ctr, + RemoveVolumes: true, + BestEffort: true, + }, "") + + err := svc.executePlan(t.Context(), &types.Project{Name: "test"}, emptyObservedState("test"), plan) + assert.NilError(t, err) +} + func TestExecutePlanRemoveContainerDropsFromCache(t *testing.T) { svc, apiClient := newTestService(t) diff --git a/pkg/compose/observed_state.go b/pkg/compose/observed_state.go index 25d0e3da6c..393435438d 100644 --- a/pkg/compose/observed_state.go +++ b/pkg/compose/observed_state.go @@ -46,6 +46,11 @@ type ObservedState struct { // others as orphans (see selectNetwork/selectVolume). Networks map[string][]ObservedNetwork // compose network key → observed Volumes map[string][]ObservedVolume // compose volume key → observed + // HookContainers are ephemeral lifecycle-hook runners (HookLabel set), + // per service. Any observed at collection time is stale by definition — + // a previous run failed before removing it — and the reconciler plans + // its purge before re-running the hooks. + HookContainers map[string][]ObservedContainer // service name → hook containers } // selectNetwork picks, among the live networks recorded for a compose key, the @@ -148,6 +153,8 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type Containers: map[string][]ObservedContainer{}, Networks: map[string][]ObservedNetwork{}, Volumes: map[string][]ObservedVolume{}, + + HookContainers: map[string][]ObservedContainer{}, } // --- Containers --- @@ -170,6 +177,18 @@ func (s *composeService) collectObservedState(ctx context.Context, project *type for _, ctr := range raw { svcName := ctr.Labels[api.ServiceLabel] + if ctr.Labels[api.HookLabel] != "" && knownServices[svcName] { + // lifecycle-hook containers (ephemeral pre_start runners) are + // neither service replicas nor one-offs: classified apart, so + // they never masquerade as a replica (they carry no + // container-number label and would otherwise read as number 0) + // and the reconciler can plan purging stale ones. A hook + // container whose service left the model falls through to the + // orphan check below instead — nothing plans purges for an + // unknown service, and --remove-orphans must keep cleaning it. + state.HookContainers[svcName] = append(state.HookContainers[svcName], toObservedContainer(ctr)) + continue + } if isNotOneOff(ctr) && knownServices[svcName] { state.Containers[svcName] = append(state.Containers[svcName], toObservedContainer(ctr)) } else if isOrphaned(project)(ctr) { diff --git a/pkg/compose/observed_state_test.go b/pkg/compose/observed_state_test.go index cc24868de4..a09a092018 100644 --- a/pkg/compose/observed_state_test.go +++ b/pkg/compose/observed_state_test.go @@ -160,6 +160,33 @@ func TestCollectObservedState(t *testing.T) { api.OneoffLabel: "True", }, }, + { + // Stale lifecycle-hook runner (a previous run failed before + // removing it): neither a replica (it has no container-number + // label and must not read as number 0) nor a one-off — + // classified apart so the reconciler can plan its purge. + ID: "c5", + Names: []string{"/hook-runner"}, + State: container.StateExited, + Labels: map[string]string{ + api.ServiceLabel: "web", + api.ProjectLabel: "myproject", + api.HookLabel: "pre_start", + }, + }, + { + // Hook runner whose service left the model: nothing plans + // purges for an unknown service, so it must keep flowing to + // the orphan path --remove-orphans cleans. + ID: "c6", + Names: []string{"/old-hook-runner"}, + State: container.StateExited, + Labels: map[string]string{ + api.ServiceLabel: "old", + api.ProjectLabel: "myproject", + api.HookLabel: "pre_start", + }, + }, }, }, nil) @@ -202,11 +229,20 @@ func TestCollectObservedState(t *testing.T) { assert.Equal(t, len(state.Containers["db"]), 1) assert.Equal(t, state.Containers["db"][0].ID, "c2") - // Orphans: only the model-absent service "old". The running one-off c4 is - // absent everywhere — not in the "web" bucket (asserted above: 1 replica), - // not an orphan: up leaves live `compose run` sessions alone. - assert.Equal(t, len(state.Orphans), 1) + // The hook runner is classified apart — not a "web" replica (asserted + // above: 1 replica), not an orphan + assert.Equal(t, len(state.HookContainers["web"]), 1) + assert.Equal(t, state.HookContainers["web"][0].ID, "c5") + + // Orphans: the model-absent service "old" — its replica AND its hook + // runner (c6), which must not hide in HookContainers where nothing would + // ever purge it. The running one-off c4 is absent everywhere — not in the + // "web" bucket (asserted above: 1 replica), not an orphan: up leaves live + // `compose run` sessions alone. + assert.Equal(t, len(state.Orphans), 2) assert.Equal(t, state.Orphans[0].ID, "c3") + assert.Equal(t, state.Orphans[1].ID, "c6") + assert.Equal(t, len(state.HookContainers["old"]), 0) // Networks assert.Equal(t, len(state.Networks), 1) diff --git a/pkg/compose/plan.go b/pkg/compose/plan.go index 6a118f12e0..6c8d48eaf4 100644 --- a/pkg/compose/plan.go +++ b/pkg/compose/plan.go @@ -103,11 +103,15 @@ type Operation struct { Volume *types.VolumeConfig // for volume operations Timeout *time.Duration // for stop operations CreateNodeID int // for OpRenameContainer: ID of the CreateContainer node whose result to rename - // BestEffort marks an operation whose failure must not abort the plan. It is - // used for the optional removal of the old network on a rename: if the - // network is still in use (by non-Compose containers) the removal is skipped - // with a warning instead of failing — the new network already carries a - // different name, so the migration does not depend on the old one going away. + // RemoveVolumes asks OpRemoveContainer to also remove the container's + // anonymous volumes — the imperative semantics for hook-runner containers. + RemoveVolumes bool + // BestEffort marks an operation whose failure must not abort the plan. + // Used for the optional removal of the old network on a rename (if the + // network is still in use by non-Compose containers the removal is skipped + // with a warning — the new network already carries a different name), and + // for purging stale pre_start hook runners (the imperative purge is + // warn-only: a failed removal leaves the container visible, never blocks). BestEffort bool } diff --git a/pkg/compose/pre_start.go b/pkg/compose/pre_start.go index 4d70c54bbd..de056f4198 100644 --- a/pkg/compose/pre_start.go +++ b/pkg/compose/pre_start.go @@ -79,26 +79,38 @@ func (s *composeService) runPreStart(ctx context.Context, project *types.Project logrus.Warnf("service %q: failed to remove stale pre_start hook containers: %v", service.Name, err) } for i, hook := range service.PreStart { - if err := s.runPreStartHook(ctx, project, service, ctr, i, hook, listener); err != nil { + created, err := s.createPreStartContainer(ctx, project, service, ctr, hook) + if err != nil { + return err + } + if err := s.execPreStartHook(ctx, service, i, created.ID, listener); err != nil { return err } + // Success: remove the hook container, mirroring the old AutoRemove behaviour + // (including its anonymous volumes). A removal failure is logged but does not + // gate service start — the hook already succeeded. + if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil { + logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s: %v", service.Name, i, created.ID, removeErr) + } } return nil } -func (s *composeService) runPreStartHook( - ctx context.Context, project *types.Project, service types.ServiceConfig, - ctr container.Summary, index int, hook types.ServiceHook, listener api.ContainerEventListener, +// execPreStartHook starts an already-created hook container, streams its logs +// and waits for its exit. It owns only execution-failure handling: a container +// that never started or a run cancelled by the user is removed, a genuinely +// failed hook is retained for post-mortem inspection. Removing the container +// after a successful run is the caller's job — the container's lifecycle +// belongs to whoever created it (the imperative runPreStart loop today, the +// reconciliation plan once the executor runs hook nodes). +func (s *composeService) execPreStartHook( + ctx context.Context, service types.ServiceConfig, + index int, containerID string, listener api.ContainerEventListener, ) error { - created, err := s.createPreStartContainer(ctx, project, service, ctr, hook) - if err != nil { - return err - } - // Subscribe to wait before start to avoid missing the exit event for short-lived hooks. // WaitConditionNotRunning would match immediately because the container is still in // "created" state, so use WaitConditionNextExit to block until the run actually finishes. - waitRes := s.apiClient().ContainerWait(ctx, created.ID, client.ContainerWaitOptions{ + waitRes := s.apiClient().ContainerWait(ctx, containerID, client.ContainerWaitOptions{ Condition: container.WaitConditionNextExit, }) @@ -108,13 +120,13 @@ func (s *composeService) runPreStartHook( // open cannot deadlock `<-logsDone`. logCtx, cancelLogs := context.WithCancel(ctx) defer cancelLogs() - logsDone, getTail := s.streamPreStartLogs(logCtx, created.ID, service, index, listener) + logsDone, getTail := s.streamPreStartLogs(logCtx, containerID, service, index, listener) - if _, err := s.apiClient().ContainerStart(ctx, created.ID, client.ContainerStartOptions{}); err != nil { + if _, err := s.apiClient().ContainerStart(ctx, containerID, client.ContainerStartOptions{}); err != nil { // AutoRemove is false, so we must remove the never-started container // explicitly. A failed removal is logged so the orphan is visible. - if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { - logrus.Warnf("service %q pre_start[%d]: failed to remove orphan hook container %s: %v", service.Name, index, created.ID, removeErr) + if _, removeErr := s.apiClient().ContainerRemove(ctx, containerID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { + logrus.Warnf("service %q pre_start[%d]: failed to remove orphan hook container %s: %v", service.Name, index, containerID, removeErr) } // Drain waitRes so the client's wait goroutine exits without having to // wait for the parent context to be canceled. @@ -136,15 +148,15 @@ func (s *composeService) runPreStartHook( // and return the raw context error without decorating it with the tail or // retaining the container for post-mortem inspection. if ctx.Err() != nil { - if _, removeErr := s.apiClient().ContainerRemove(context.Background(), created.ID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { - logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s after cancellation: %v", service.Name, index, created.ID, removeErr) + if _, removeErr := s.apiClient().ContainerRemove(context.Background(), containerID, client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); removeErr != nil { + logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s after cancellation: %v", service.Name, index, containerID, removeErr) } return waitErr } // Genuine hook failure: retain the container so the operator can run // `docker logs ` and `docker inspect ` to diagnose the failure. // Include the short container ID in the error to make it actionable. - shortID := created.ID + shortID := containerID if len(shortID) > 12 { shortID = shortID[:12] } @@ -153,12 +165,6 @@ func (s *composeService) runPreStartHook( } return fmt.Errorf("%w (hook container %s retained for inspection)", waitErr, shortID) } - // Success: remove the hook container, mirroring the old AutoRemove behaviour - // (including its anonymous volumes). A removal failure is logged but does not - // gate service start — the hook already succeeded. - if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil { - logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s: %v", service.Name, index, created.ID, removeErr) - } return nil } diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index d22f035617..5d04d815c4 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -662,6 +662,8 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { return err } + r.planPurgeStaleHookRunners(service, expected) + containers := r.observed.Containers[service.Name] actual := len(containers) @@ -760,6 +762,41 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { // mustRecreate decides whether oc must be recreated to match expected. The // expectedHash and parentRecreated inputs are precomputed once per service by +// planPurgeStaleHookRunners plans the removal of hook-runner containers left +// behind by a previous run that failed before removing them. It mirrors the +// imperative purge living inside the gated runPreStart call: emitted only when +// pre_start is going to run again — hooks declared, a replica to start +// (scale > 0: the imperative start path returns before the hooks for a +// scale-0 service) and no replica running at observation — so a genuinely +// failed hook container stays retained for inspection as long as its service +// is otherwise up. Removals are best-effort (the imperative purge is +// warn-only) and independent of every other node. +func (r *reconciler) planPurgeStaleHookRunners(service types.ServiceConfig, expectedScale int) { + stale := r.observed.HookContainers[service.Name] + if len(stale) == 0 || len(service.PreStart) == 0 || expectedScale == 0 { + return + } + for _, oc := range r.observed.Containers[service.Name] { + if oc.State == container.StateRunning { + return + } + } + serviceCopy := service + stale = slices.Clone(stale) + slices.SortFunc(stale, func(a, b ObservedContainer) int { return strings.Compare(a.ID, b.ID) }) + for i := range stale { + r.plan.addNode(Operation{ + Type: OpRemoveContainer, + ResourceID: fmt.Sprintf("hook:%s:stale:%s", service.Name, stale[i].ID[:min(12, len(stale[i].ID))]), + Cause: "stale pre_start hook container", + Service: &serviceCopy, + Container: &stale[i].Summary, + RemoveVolumes: true, + BestEffort: true, + }, "") + } +} + // 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 { diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index c0336ce162..25df7fbdb8 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -1251,6 +1251,134 @@ func TestReconcileContainers_ExitedIsNoop(t *testing.T) { // container creation depends on the last plan node of the service it depends // on (via reconciler.serviceNodes). Without this, services declared in // depends_on could start before their dependencies' operations complete. +// Stale pre_start hook runners (left by a previous run that failed before +// removing them) are purged by the plan when pre_start is going to run again: +// hooks declared and no replica running at observation — the imperative +// gating. Removals are best-effort and drop the runner's anonymous volumes, +// like the warn-only imperative purge they mirror. +func TestReconcileContainers_StaleHookRunnersPurged(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Services: types.Services{ + "app": {Name: "app", Scale: intPtr(1), PreStart: []types.ServiceHook{{}}}, + }, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + HookContainers: map[string][]ObservedContainer{ + "app": { + // deliberately out of ID order: the plan sorts for determinism + {ID: "stale-b-id", Summary: container.Summary{ID: "stale-b-id"}}, + {ID: "stale-a-id", Summary: container.Summary{ID: "stale-a-id"}}, + }, + }, + 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 hook:app:stale:stale-a-id, RemoveContainer, stale pre_start hook container +[] -> #2 hook:app:stale:stale-b-id, RemoveContainer, stale pre_start hook container +[] -> #3 service:app:1, CreateContainer, no existing container +`)+"\n") + + for _, n := range plan.Nodes { + if n.Operation.Type != OpRemoveContainer { + continue + } + assert.Assert(t, n.Operation.BestEffort, "purge #%d must not abort the plan on failure", n.ID) + assert.Assert(t, n.Operation.RemoveVolumes, "purge #%d must drop the runner's anonymous volumes", n.ID) + } +} + +// A scale-0 service never reaches its pre_start hooks (the imperative start +// path returns before them), so its stale runners are not purged either. +func TestReconcileContainers_StaleHookRunnersKeptWhenScaleZero(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Services: types.Services{ + "app": {Name: "app", Scale: intPtr(0), PreStart: []types.ServiceHook{{}}}, + }, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + HookContainers: map[string][]ObservedContainer{ + "app": {{ID: "stale-a-id", Summary: container.Summary{ID: "stale-a-id"}}}, + }, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + assert.Assert(t, plan.IsEmpty(), "unexpected plan:\n%s", plan) +} + +// A running replica gates pre_start off, so the stale runner stays: the +// imperative purge lives inside the gated runPreStart call and would not run +// either — a genuinely failed hook container stays retained for inspection as +// long as its service is otherwise up. +func TestReconcileContainers_StaleHookRunnersKeptWhenReplicaRunning(t *testing.T) { + svc := types.ServiceConfig{Name: "app", Scale: intPtr(1), PreStart: []types.ServiceHook{{}}} + hash := mustServiceHash(t, svc) + project := &types.Project{ + Name: "myproject", + Services: types.Services{"app": svc}, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{ + "app": {{ + ID: "c1", Number: 1, State: container.StateRunning, ConfigHash: hash, + Summary: container.Summary{ + ID: "c1", State: container.StateRunning, + Labels: map[string]string{api.ServiceLabel: "app", api.ContainerNumberLabel: "1", api.ConfigHashLabel: hash}, + }, + }}, + }, + HookContainers: map[string][]ObservedContainer{ + "app": {{ID: "stale-a-id", Summary: container.Summary{ID: "stale-a-id"}}}, + }, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + assert.Assert(t, plan.IsEmpty()) +} + +// Without pre_start hooks in the model, stale runners are not the plan's to +// purge: runPreStart never runs, so the imperative engine leaves them alone +// too. +func TestReconcileContainers_StaleHookRunnersKeptWithoutHooks(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Services: types.Services{"app": {Name: "app", Scale: intPtr(1)}}, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + HookContainers: map[string][]ObservedContainer{ + "app": {{ID: "stale-a-id", Summary: container.Summary{ID: "stale-a-id"}}}, + }, + 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 service:app:1, CreateContainer, no existing container +`)+"\n") +} + func TestReconcileContainers_DependsOnChain(t *testing.T) { project := &types.Project{ Name: "myproject", From 39ea63cf25ac8808b7f5a7f178fe6c6723bcce13 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Tue, 15 Sep 2026 11:00:01 +0200 Subject: [PATCH 2/2] feat: reconciliation plan creates pre_start hook runners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pre_start hook containers become first-class resources of the reconciliation plan. The reconciler plans one OpCreateHookContainer per declared hook — gated on the same predicate the start phase uses to run the hooks: a replica to start and no running replica SURVIVING the plan (a recreated replica is not running at start time, so its runners are planned too). Runner creation depends on every replica node (the executor resolves the VolumesFrom target against a final live view), on the infrastructure, and on the purge of previously observed runners: names are deterministic (--pre_start-) so repeated plans converge on the same container instead of accumulating anonymous ones, and a new HookIndexLabel ties each runner to its hook. runPreStart becomes pure execution: it looks up the created-state runner for each hook by label and never creates one. A declared hook without a prepared runner is an actionable error naming the reconciliation command (docker compose up ) — the accepted consequence is that stop-then-start of a hooked service errors, since runners are consumed on success (locked by e2e). Success removal, failure retention and cancellation cleanup are unchanged. Runners stay invisible to the start/ps container listings — they carry no ConfigHashLabel, which getDefaultFilters requires — and down keeps removing them by hook label. Signed-off-by: Nicolas De Loof --- pkg/api/labels.go | 5 + pkg/compose/executor.go | 2 + pkg/compose/executor_ops.go | 26 + pkg/compose/executor_test.go | 80 +++ pkg/compose/observed_state.go | 8 +- pkg/compose/plan.go | 7 + pkg/compose/pre_start.go | 126 +++-- pkg/compose/pre_start_test.go | 527 ++++++++---------- pkg/compose/reconcile.go | 78 ++- pkg/compose/reconcile_test.go | 93 +++- pkg/compose/service_containers.go | 7 +- pkg/compose/start_test.go | 28 +- pkg/e2e/hooks_test.go | 33 ++ .../compose.yaml | 11 + .../compose.yaml | 11 + 15 files changed, 651 insertions(+), 391 deletions(-) create mode 100644 pkg/e2e/testdata/TestPreStartHookCreateThenStart/compose.yaml create mode 100644 pkg/e2e/testdata/TestPreStartHookStopThenStartFails/compose.yaml diff --git a/pkg/api/labels.go b/pkg/api/labels.go index 9122fb5a83..53e863e31d 100644 --- a/pkg/api/labels.go +++ b/pkg/api/labels.go @@ -67,6 +67,11 @@ const ( // runPreStartHook so orphan hook containers from a previous failed run can // be found and removed by project+service+hook label filters. HookLabel = "com.docker.compose.hook" + // HookIndexLabel stores the position of the hook in its service's hook + // list (e.g. pre_start[2] → "2"), so the start phase can match each + // declared hook with the runner container the reconciliation plan + // prepared for it. + HookIndexLabel = "com.docker.compose.hook-index" ) // ComposeVersion is the compose tool version as declared by label VersionLabel diff --git a/pkg/compose/executor.go b/pkg/compose/executor.go index c23b3f7b21..5be4beab1a 100644 --- a/pkg/compose/executor.go +++ b/pkg/compose/executor.go @@ -160,6 +160,8 @@ func (exec *planExecutor) executeNode(ctx context.Context, node *PlanNode) error return exec.execRemoveContainer(ctx, op) case OpRenameContainer: return exec.execRenameContainer(ctx, node) + case OpCreateHookContainer: + return exec.execCreateHookContainer(ctx, node) case OpRunProvider: return exec.compose.runPlugin(ctx, exec.project, *op.Service, "up") default: diff --git a/pkg/compose/executor_ops.go b/pkg/compose/executor_ops.go index 7dc936c7cd..2b515a0a34 100644 --- a/pkg/compose/executor_ops.go +++ b/pkg/compose/executor_ops.go @@ -168,6 +168,32 @@ func (exec *planExecutor) execRemoveContainer(ctx context.Context, op Operation) return nil } +// execCreateHookContainer creates the runner container for one pre_start hook. +// The target replica — whose volumes the hook shares via VolumesFrom — is +// resolved from the live view at execution time: the node depends on every +// container operation of its service, so the view is final here, and the +// lowest-numbered replica matches the one the start phase hands the hooks. +func (exec *planExecutor) execCreateHookContainer(ctx context.Context, node *PlanNode) error { + op := node.Operation + service := *op.Service + exec.containersMu.Lock() + replicas := slices.Clone(exec.containersByService[service.Name]) + exec.containersMu.Unlock() + if len(replicas) == 0 { + return fmt.Errorf("internal: no %q container to attach pre_start hook %d to", service.Name, op.HookIndex) + } + target := lowestNumberedContainer(replicas) + created, err := exec.compose.createPreStartContainer(ctx, exec.project, service, target, op.HookIndex, op.Name) + if err != nil { + return err + } + exec.pctx.set(node.ID, operationResult{ + ContainerID: created.ID, + ContainerName: op.Name, + }) + return nil +} + func (exec *planExecutor) execRenameContainer(ctx context.Context, node *PlanNode) error { op := node.Operation if op.CreateNodeID == 0 { diff --git a/pkg/compose/executor_test.go b/pkg/compose/executor_test.go index bbfb8b4896..ea60259503 100644 --- a/pkg/compose/executor_test.go +++ b/pkg/compose/executor_test.go @@ -518,3 +518,83 @@ type conflictError struct{} func (conflictError) Error() string { return "conflict" } func (conflictError) Conflict() {} + +// TestExecutePlanCreateHookContainer verifies the hook-runner create op: the +// target replica is resolved from the executor's live view (lowest +// container-number, mirroring what the start phase hands the hooks) and the +// runner is created under the deterministic name carried by the operation. +func TestExecutePlanCreateHookContainer(t *testing.T) { + svc, apiClient := newTestService(t) + apiClient.EXPECT().Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}). + Return(client.PingResult{APIVersion: "1.44"}, nil).AnyTimes() + apiClient.EXPECT().ClientVersion().Return("1.44").AnyTimes() + + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Command: types.ShellCommand{"init"}}, + }, + } + observed := &ObservedState{ + ProjectName: "test", + Containers: map[string][]ObservedContainer{ + "web": { + // Deliberately listed out of order: replica 2 first. + {ID: "c2", Summary: container.Summary{ID: "c2", Labels: map[string]string{api.ContainerNumberLabel: "2"}}}, + {ID: "c1", Summary: container.Summary{ID: "c1", Labels: map[string]string{api.ContainerNumberLabel: "1"}}}, + }, + }, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, + } + + var gotOpts client.ContainerCreateOptions + apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { + gotOpts = opts + return client.ContainerCreateResult{ID: "hook-1"}, nil + }) + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateHookContainer, + ResourceID: "hook:web:pre_start:0", + Cause: "pre_start hook", + Service: &service, + HookIndex: 0, + Name: getHookContainerName("test", "web", 0), + }, "") + + err := svc.executePlan(t.Context(), &types.Project{Name: "test"}, observed, plan) + assert.NilError(t, err) + assert.Equal(t, gotOpts.Name, "test-web-pre_start-0") + assert.DeepEqual(t, gotOpts.HostConfig.VolumesFrom, []string{"c1"}) + assert.Equal(t, gotOpts.Config.Labels[api.HookIndexLabel], "0") +} + +// TestExecutePlanCreateHookContainerNoReplica: a hook-create node scheduled +// with no replica in the live view is an internal planning error — the +// reconciler guarantees the node depends on the replica's create. +func TestExecutePlanCreateHookContainerNoReplica(t *testing.T) { + svc, _ := newTestService(t) + + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{{Command: types.ShellCommand{"init"}}}, + } + + plan := &Plan{} + plan.addNode(Operation{ + Type: OpCreateHookContainer, + ResourceID: "hook:web:pre_start:0", + Cause: "pre_start hook", + Service: &service, + HookIndex: 0, + Name: getHookContainerName("test", "web", 0), + }, "") + + err := svc.executePlan(t.Context(), &types.Project{Name: "test"}, emptyObservedState("test"), plan) + assert.ErrorContains(t, err, `no "web" container`) +} diff --git a/pkg/compose/observed_state.go b/pkg/compose/observed_state.go index 393435438d..32a272ff3a 100644 --- a/pkg/compose/observed_state.go +++ b/pkg/compose/observed_state.go @@ -47,9 +47,11 @@ type ObservedState struct { Networks map[string][]ObservedNetwork // compose network key → observed Volumes map[string][]ObservedVolume // compose volume key → observed // HookContainers are ephemeral lifecycle-hook runners (HookLabel set), - // per service. Any observed at collection time is stale by definition — - // a previous run failed before removing it — and the reconciler plans - // its purge before re-running the hooks. + // per service: fresh runners prepared by a previous plan and not yet + // consumed, or leftovers of a run that failed before removing them. + // Whenever the hooks are going to run again, the reconciler purges every + // observed runner and plans fresh ones (see planPreStartHookRunners), so + // the set always converges to the current service definition. HookContainers map[string][]ObservedContainer // service name → hook containers } diff --git a/pkg/compose/plan.go b/pkg/compose/plan.go index 6c8d48eaf4..fa6bbc1f6c 100644 --- a/pkg/compose/plan.go +++ b/pkg/compose/plan.go @@ -53,6 +53,10 @@ const ( // Provider operations OpRunProvider OperationType = 30 + + // Hook operations. 40-42 are reserved for the start-phase operations + // (wait condition, pre_start run, post_start run). + OpCreateHookContainer OperationType = 43 ) // String returns the human-readable name of an OperationType. @@ -82,6 +86,8 @@ func (o OperationType) String() string { return "RenameContainer" case OpRunProvider: return "RunProvider" + case OpCreateHookContainer: + return "CreateHookContainer" default: return fmt.Sprintf("Unknown(%d)", int(o)) } @@ -103,6 +109,7 @@ type Operation struct { Volume *types.VolumeConfig // for volume operations Timeout *time.Duration // for stop operations CreateNodeID int // for OpRenameContainer: ID of the CreateContainer node whose result to rename + HookIndex int // for OpCreateHookContainer: position of the hook in the service's hook list // RemoveVolumes asks OpRemoveContainer to also remove the container's // anonymous volumes — the imperative semantics for hook-runner containers. RemoveVolumes bool diff --git a/pkg/compose/pre_start.go b/pkg/compose/pre_start.go index de056f4198..97b04e651a 100644 --- a/pkg/compose/pre_start.go +++ b/pkg/compose/pre_start.go @@ -21,6 +21,7 @@ import ( "fmt" "io" "strconv" + "strings" "github.com/compose-spec/compose-go/v2/types" "github.com/moby/moby/api/pkg/stdcopy" @@ -38,6 +39,14 @@ import ( // by a project+service+hook label filter. const preStartHookType = "pre_start" +// getHookContainerName builds the deterministic name of a pre_start runner +// container, e.g. "myproject-db-pre_start-0". A stable name makes the runner +// an addressable resource of the reconciliation plan: repeated plans converge +// on the same container instead of accumulating anonymous ones. +func getHookContainerName(projectName, serviceName string, index int) string { + return strings.Join([]string{projectName, serviceName, preStartHookType, strconv.Itoa(index)}, api.Separator) +} + // lowestNumberedContainer returns the container with the lowest // com.docker.compose.container-number label, so pre_start always targets the // same replica regardless of the order the daemon returned them in. @@ -55,54 +64,85 @@ func lowestNumberedContainer(containers Containers) container.Summary { } // runPreStart executes the service's pre_start hooks sequentially, in declared -// order. Each hook runs as an ephemeral container that shares the service -// container's volumes via VolumesFrom and is attached to the same networks. -// A non-zero exit gates service start. +// order. Each hook runs in a runner container prepared by the reconciliation +// plan (see planPreStartHookRunners) that shares the service container's +// volumes via VolumesFrom and is attached to the same networks. A non-zero +// exit gates service start. +// +// runPreStart never creates a runner itself: a declared hook without a fresh +// runner is an error telling the user to reconcile — the runners were either +// consumed by a previous start (e.g. `stop` then `start`) or never prepared. // // With per_replica: false (the only currently supported mode), the hook sees // the volumes of the first non-running replica only — anonymous volumes and // tmpfs mounts are per-replica and not shared. Use named volumes or bind // mounts for data the hook produces. -func (s *composeService) runPreStart(ctx context.Context, project *types.Project, service types.ServiceConfig, ctr container.Summary, listener api.ContainerEventListener) error { +func (s *composeService) runPreStart(ctx context.Context, project *types.Project, service types.ServiceConfig, listener api.ContainerEventListener) error { // Validate every hook up front so an unsupported entry never triggers any I/O. for i, hook := range service.PreStart { if hook.PerReplica { return fmt.Errorf("service %q pre_start[%d]: per_replica is not yet supported; remove per_replica or set it to false", service.Name, i) } } - // Remove any hook containers left behind by a previous failed run so they do - // not accumulate. Only one orphan can exist per service (failure gates the - // remaining hooks), but we clean the whole set in case the service definition - // changed between runs. Removal failures are non-fatal: they are logged so - // the operator can identify the stale container. - if err := s.removeOrphanPreStartContainers(ctx, project.Name, service.Name); err != nil { - logrus.Warnf("service %q: failed to remove stale pre_start hook containers: %v", service.Name, err) + runners, err := s.listPreStartRunners(ctx, project.Name, service.Name) + if err != nil { + return err } - for i, hook := range service.PreStart { - created, err := s.createPreStartContainer(ctx, project, service, ctr, hook) - if err != nil { - return err + for i := range service.PreStart { + runner, ok := runners[i] + if !ok { + return fmt.Errorf("service %q pre_start[%d]: no hook runner container found — runners are prepared when the service is created and consumed when its hooks run; run %q to prepare them again", + service.Name, i, "docker compose up "+service.Name) } - if err := s.execPreStartHook(ctx, service, i, created.ID, listener); err != nil { + if err := s.execPreStartHook(ctx, service, i, runner.ID, listener); err != nil { return err } // Success: remove the hook container, mirroring the old AutoRemove behaviour // (including its anonymous volumes). A removal failure is logged but does not // gate service start — the hook already succeeded. - if _, removeErr := s.apiClient().ContainerRemove(ctx, created.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil { - logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s: %v", service.Name, i, created.ID, removeErr) + if _, removeErr := s.apiClient().ContainerRemove(ctx, runner.ID, client.ContainerRemoveOptions{RemoveVolumes: true}); removeErr != nil { + logrus.Warnf("service %q pre_start[%d]: failed to remove hook container %s: %v", service.Name, i, runner.ID, removeErr) } } return nil } +// listPreStartRunners returns the service's created-state pre_start hook +// runner containers, indexed by their HookIndexLabel. Runners in any other +// state are ignored — a failed hook retained for inspection or an +// old-generation container without an index label is not executable; only a +// fresh runner prepared by the reconciliation plan is. +func (s *composeService) listPreStartRunners(ctx context.Context, projectName, serviceName string) (map[int]container.Summary, error) { + f := projectFilter(projectName) + f.Add("label", serviceFilter(serviceName)) + f.Add("label", hookFilter(preStartHookType)) + res, err := s.apiClient().ContainerList(ctx, client.ContainerListOptions{ + All: true, + Filters: f, + }) + if err != nil { + return nil, err + } + runners := map[int]container.Summary{} + for _, ctr := range res.Items { + if ctr.State != container.StateCreated { + continue + } + index, err := strconv.Atoi(ctr.Labels[api.HookIndexLabel]) + if err != nil { + continue + } + runners[index] = ctr + } + return runners, nil +} + // execPreStartHook starts an already-created hook container, streams its logs // and waits for its exit. It owns only execution-failure handling: a container // that never started or a run cancelled by the user is removed, a genuinely // failed hook is retained for post-mortem inspection. Removing the container -// after a successful run is the caller's job — the container's lifecycle -// belongs to whoever created it (the imperative runPreStart loop today, the -// reconciliation plan once the executor runs hook nodes). +// after a successful run is the caller's job — the runner was prepared by the +// reconciliation plan and is consumed (removed) by runPreStart on success. func (s *composeService) execPreStartHook( ctx context.Context, service types.ServiceConfig, index int, containerID string, listener api.ContainerEventListener, @@ -168,10 +208,16 @@ func (s *composeService) execPreStartHook( return nil } +// createPreStartContainer creates the runner container for the index-th +// pre_start hook of service, named after getHookContainerName and stamped +// with the hook labels so both the start phase (by index) and the purge (by +// hook type) can find it. It only creates: the runner is left in created +// state for the start phase to execute. func (s *composeService) createPreStartContainer( ctx context.Context, project *types.Project, service types.ServiceConfig, - ctr container.Summary, hook types.ServiceHook, + ctr container.Summary, index int, name string, ) (client.ContainerCreateResult, error) { + hook := service.PreStart[index] image := hook.Image if image == "" { image = api.GetImageNameOrDefault(service, project.Name) @@ -188,10 +234,11 @@ func (s *composeService) createPreStartContainer( // HookLabel also distinguishes hook containers from the real service // container (which shares ProjectLabel and ServiceLabel). Labels: map[string]string{ - api.ProjectLabel: project.Name, - api.ServiceLabel: service.Name, - api.VersionLabel: api.ComposeVersion, - api.HookLabel: preStartHookType, + api.ProjectLabel: project.Name, + api.ServiceLabel: service.Name, + api.VersionLabel: api.ComposeVersion, + api.HookLabel: preStartHookType, + api.HookIndexLabel: strconv.Itoa(index), }, } hostCfg := &container.HostConfig{ @@ -215,6 +262,7 @@ func (s *composeService) createPreStartContainer( hostCfg.NetworkMode = networkMode created, err := s.apiClient().ContainerCreate(ctx, client.ContainerCreateOptions{ + Name: name, Config: cfg, HostConfig: hostCfg, NetworkingConfig: networkingConfig, @@ -259,32 +307,6 @@ func (s *composeService) connectPreStartExtraNetworks(ctx context.Context, proje return nil } -// removeOrphanPreStartContainers finds and force-removes any hook containers -// left behind by a previous failed run of this service's pre_start hooks. -// Containers are identified by project + service + HookLabel=pre_start. -// Removal failures are logged at warn level and do not abort the run. -func (s *composeService) removeOrphanPreStartContainers(ctx context.Context, projectName, serviceName string) error { - f := projectFilter(projectName) - f.Add("label", serviceFilter(serviceName)) - f.Add("label", hookFilter(preStartHookType)) - res, err := s.apiClient().ContainerList(ctx, client.ContainerListOptions{ - All: true, - Filters: f, - }) - if err != nil { - return err - } - for _, ctr := range res.Items { - if _, removeErr := s.apiClient().ContainerRemove(ctx, ctr.ID, client.ContainerRemoveOptions{ - Force: true, - RemoveVolumes: true, - }); removeErr != nil { - logrus.Warnf("failed to remove stale pre_start hook container %s: %v", ctr.ID, removeErr) - } - } - return nil -} - func waitPreStart(ctx context.Context, serviceName string, index int, waitRes client.ContainerWaitResult) error { // ContainerWait can deliver on Result and Error at the same instant. Two // races have to be closed deterministically here: diff --git a/pkg/compose/pre_start_test.go b/pkg/compose/pre_start_test.go index 635eef6739..6da00e7335 100644 --- a/pkg/compose/pre_start_test.go +++ b/pkg/compose/pre_start_test.go @@ -21,6 +21,7 @@ import ( "context" "errors" "io" + "strconv" "strings" "testing" @@ -69,16 +70,29 @@ func emptyLogs() client.ContainerLogsResult { return io.NopCloser(bytes.NewReader(nil)) } -// expectEmptyOrphanScan sets up the ContainerList expectation for the orphan -// pre_start cleanup that happens once at the start of every runPreStart call -// (after the per_replica validation loop). It returns the empty list. -func expectEmptyOrphanScan(apiClient *mocks.MockAPIClient) *gomock.Call { +// runnerSummary builds the container.Summary of a hook runner as prepared by +// the reconciliation plan: created state, hook labels carrying the index. +func runnerSummary(id string, index int) container.Summary { + return container.Summary{ + ID: id, + State: container.StateCreated, + Labels: map[string]string{ + api.HookLabel: preStartHookType, + api.HookIndexLabel: strconv.Itoa(index), + }, + } +} + +// expectRunnerScan sets up the ContainerList expectation for the runner lookup +// that happens once at the start of every runPreStart call (after the +// per_replica validation loop). It returns the given runners. +func expectRunnerScan(apiClient *mocks.MockAPIClient, runners ...container.Summary) *gomock.Call { return apiClient.EXPECT(). ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{}, nil) + Return(client.ContainerListResult{Items: runners}, nil) } -// expectSuccessRemove sets up the ContainerRemove call that runPreStartHook +// expectSuccessRemove sets up the ContainerRemove call that runPreStart // makes after a successful hook run (mirrors old AutoRemove behaviour). func expectSuccessRemove(apiClient *mocks.MockAPIClient, hookID string) *gomock.Call { return apiClient.EXPECT(). @@ -98,34 +112,29 @@ func TestPreStart_SuccessTwoHooksInOrder(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"echo", "second"}}, }, } - ctr := container.Summary{ID: "service-ctr-id"} - // Orphan cleanup runs once before the first hook. - scan := expectEmptyOrphanScan(apiClient) + // Both runners were prepared by the plan; the scan runs once up front. + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0), runnerSummary("hook-2", 1)) - // Hook 1: create → wait (subscribe) → logs (subscribe) → start → remove. - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + // Hook 1: wait (subscribe) → logs (subscribe) → start → remove. wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)).After(create1) + Return(waitResultExit(0)).After(scan) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil).After(wait1) start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) remove1 := expectSuccessRemove(apiClient, "hook-1").After(start1) - // Hook 2 is only created after hook 1 has been removed. - create2 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-2"}, nil).After(remove1) + // Hook 2 only runs after hook 1 has been removed. wait2 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-2", gomock.Any()). - Return(waitResultExit(0)).After(create2) + Return(waitResultExit(0)).After(remove1) logs2 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-2", gomock.Any()). Return(emptyLogs(), nil).After(wait2) start2 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-2", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs2) expectSuccessRemove(apiClient, "hook-2").After(start2) - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + err := tested.runPreStart(t.Context(), project, service, func(api.ContainerEvent) {}) assert.NilError(t, err) } @@ -141,20 +150,18 @@ func TestPreStart_FirstHookFailsStopsExecution(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"echo", "never"}}, }, } - ctr := container.Summary{ID: "service-ctr-id"} - scan := expectEmptyOrphanScan(apiClient) - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0), runnerSummary("hook-2", 1)) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(42)).After(create1) + Return(waitResultExit(42)).After(scan) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil).After(wait1) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) - // Hook container is retained on failure — no ContainerRemove expected. + // Hook container is retained on failure — no ContainerRemove expected, and + // hook-2's runner is never touched. - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + err := tested.runPreStart(t.Context(), project, service, func(api.ContainerEvent) {}) assert.ErrorContains(t, err, `service "web" pre_start[0]`) assert.ErrorContains(t, err, "42") } @@ -170,13 +177,147 @@ func TestPreStart_PerReplicaRejected(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}, PerReplica: true}, }, } - ctr := container.Summary{ID: "service-ctr-id"} - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + err := tested.runPreStart(t.Context(), project, service, func(api.ContainerEvent) {}) assert.ErrorContains(t, err, `service "web" pre_start[0]`) assert.ErrorContains(t, err, "per_replica is not yet supported") } +// --------------------------------------------------------------------------- +// Pure-execution contract: runPreStart never creates a runner +// --------------------------------------------------------------------------- + +// TestPreStart_MissingRunnerFails pins the no-fallback contract: a declared +// hook without a prepared runner is an error pointing the user at the +// reconciliation command, not an implicit creation. This is what a user sees +// on `stop` then `start`: the runners were consumed by the first start. +func TestPreStart_MissingRunnerFails(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "demo"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + + expectRunnerScan(apiClient) + + err := tested.runPreStart(t.Context(), project, service, nil) + assert.ErrorContains(t, err, `service "web" pre_start[0]`) + assert.ErrorContains(t, err, "docker compose up web") +} + +// TestPreStart_ConsumedRunnerNotReused verifies that a runner in any state but +// created (here: exited, e.g. retained after a failure) is not re-executed — +// the hook reports the runner as missing instead of re-running a stale one. +func TestPreStart_ConsumedRunnerNotReused(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "demo"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + + consumed := runnerSummary("hook-old", 0) + consumed.State = container.StateExited + expectRunnerScan(apiClient, consumed) + + err := tested.runPreStart(t.Context(), project, service, nil) + assert.ErrorContains(t, err, `service "web" pre_start[0]`) + assert.ErrorContains(t, err, "docker compose up web") +} + +// TestPreStart_OldGenerationRunnerIgnored verifies that a hook container +// without a HookIndexLabel (created by an older compose version) is never +// matched to a declared hook: the purge planned by the reconciler is the only +// consumer of those containers. +func TestPreStart_OldGenerationRunnerIgnored(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "demo"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + + legacy := container.Summary{ + ID: "legacy-hook", + State: container.StateCreated, + Labels: map[string]string{api.HookLabel: preStartHookType}, + } + expectRunnerScan(apiClient, legacy) + + err := tested.runPreStart(t.Context(), project, service, nil) + assert.ErrorContains(t, err, `service "web" pre_start[0]`) + assert.ErrorContains(t, err, "docker compose up web") +} + +// TestPreStart_SecondRunnerMissing verifies that a missing runner is only +// reported when its hook is reached: the first hook runs (and is consumed) +// before pre_start[1] fails. +func TestPreStart_SecondRunnerMissing(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "demo"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) + wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). + Return(waitResultExit(0)).After(scan) + logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). + Return(emptyLogs(), nil).After(wait1) + start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). + Return(client.ContainerStartResult{}, nil).After(logs1) + expectSuccessRemove(apiClient, "hook-1").After(start1) + + err := tested.runPreStart(t.Context(), project, service, nil) + assert.ErrorContains(t, err, `service "web" pre_start[1]`) + assert.ErrorContains(t, err, "docker compose up web") +} + +// TestPreStart_RunnerScanFails verifies that a ContainerList failure during +// the runner lookup is fatal: without the runner set runPreStart cannot tell +// prepared hooks from missing ones. +func TestPreStart_RunnerScanFails(t *testing.T) { + tested, apiClient := newPreStartTestService(t) + + project := &types.Project{Name: "proj"} + service := types.ServiceConfig{ + Name: "web", + Image: "alpine", + PreStart: []types.ServiceHook{ + {Image: "alpine", Command: types.ShellCommand{"true"}}, + }, + } + + apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). + Return(client.ContainerListResult{}, errors.New("daemon unavailable")) + + err := tested.runPreStart(t.Context(), project, service, nil) + assert.ErrorContains(t, err, "daemon unavailable") +} + +// --------------------------------------------------------------------------- +// Runner creation (createPreStartContainer, called by the plan executor) +// --------------------------------------------------------------------------- + func TestPreStart_ImageFallsBackToBuiltImage(t *testing.T) { tested, apiClient := newPreStartTestService(t) @@ -191,22 +332,15 @@ func TestPreStart_ImageFallsBackToBuiltImage(t *testing.T) { ctr := container.Summary{ID: "service-ctr-id"} var gotImage string - scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { gotImage = opts.Config.Image return client.ContainerCreateResult{ID: "hook-1"}, nil - }).After(scan) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) - expectSuccessRemove(apiClient, "hook-1") + }) - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + created, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 0, "demo-web-pre_start-0") assert.NilError(t, err) + assert.Equal(t, created.ID, "hook-1") assert.Equal(t, gotImage, api.GetImageNameOrDefault(service, project.Name)) } @@ -224,26 +358,22 @@ func TestPreStart_ExplicitHookImageUsed(t *testing.T) { ctr := container.Summary{ID: "service-ctr-id"} var gotImage string - scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { gotImage = opts.Config.Image return client.ContainerCreateResult{ID: "hook-1"}, nil - }).After(scan) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) - expectSuccessRemove(apiClient, "hook-1") + }) - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + _, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 0, "demo-web-pre_start-0") assert.NilError(t, err) assert.Equal(t, gotImage, "custom-hook-image:1.2.3") } -func TestPreStart_VolumesFromServiceContainer(t *testing.T) { +// TestPreStart_CreateNameAndLabels pins the runner's plan-visible identity: +// deterministic container name, VolumesFrom on the target replica, AutoRemove +// off (retention is managed explicitly) and the hook labels — type for the +// purge, index for the start-phase lookup. +func TestPreStart_CreateNameAndLabels(t *testing.T) { tested, apiClient := newPreStartTestService(t) project := &types.Project{Name: "demo"} @@ -252,37 +382,26 @@ func TestPreStart_VolumesFromServiceContainer(t *testing.T) { Image: "alpine", PreStart: []types.ServiceHook{ {Image: "alpine", Command: types.ShellCommand{"true"}}, + {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } ctr := container.Summary{ID: "service-ctr-id"} - var gotVolumesFrom []string - var gotAutoRemove bool - var gotLabels map[string]string - scan := expectEmptyOrphanScan(apiClient) + var gotOpts client.ContainerCreateOptions apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { - gotVolumesFrom = opts.HostConfig.VolumesFrom - gotAutoRemove = opts.HostConfig.AutoRemove - gotLabels = opts.Config.Labels + gotOpts = opts return client.ContainerCreateResult{ID: "hook-1"}, nil - }).After(scan) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) - expectSuccessRemove(apiClient, "hook-1") + }) - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + name := getHookContainerName(project.Name, service.Name, 1) + _, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 1, name) assert.NilError(t, err) - assert.DeepEqual(t, gotVolumesFrom, []string{"service-ctr-id"}) - // AutoRemove must be false: the hook container is retained on failure for - // post-mortem and explicitly removed on success by runPreStartHook. - assert.Assert(t, !gotAutoRemove, "AutoRemove must be false") - // HookLabel must be set so orphan cleanup can identify the container. - assert.Equal(t, gotLabels[api.HookLabel], preStartHookType) + assert.Equal(t, gotOpts.Name, "demo-web-pre_start-1") + assert.DeepEqual(t, gotOpts.HostConfig.VolumesFrom, []string{"service-ctr-id"}) + assert.Assert(t, !gotOpts.HostConfig.AutoRemove, "AutoRemove must be false") + assert.Equal(t, gotOpts.Config.Labels[api.HookLabel], preStartHookType) + assert.Equal(t, gotOpts.Config.Labels[api.HookIndexLabel], "1") } func TestPreStart_ContainerCreateFailurePropagates(t *testing.T) { @@ -294,16 +413,14 @@ func TestPreStart_ContainerCreateFailurePropagates(t *testing.T) { Image: "alpine", PreStart: []types.ServiceHook{ {Image: "missing:latest", Command: types.ShellCommand{"true"}}, - {Image: "alpine", Command: types.ShellCommand{"never"}}, }, } ctr := container.Summary{ID: "service-ctr-id"} - scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{}, errors.New("no such image: missing:latest")).After(scan) + Return(client.ContainerCreateResult{}, errors.New("no such image: missing:latest")) - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + _, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 0, "demo-web-pre_start-0") assert.ErrorContains(t, err, "no such image") } @@ -318,13 +435,10 @@ func TestPreStart_ContainerStartFailurePropagates(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "service-ctr-id"} - scan := expectEmptyOrphanScan(apiClient) - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)).After(create1) + Return(waitResultExit(0)).After(scan) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil).After(wait1) start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). @@ -335,7 +449,7 @@ func TestPreStart_ContainerStartFailurePropagates(t *testing.T) { apiClient.EXPECT().ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). Return(client.ContainerRemoveResult{}, nil).After(start1) - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + err := tested.runPreStart(t.Context(), project, service, func(api.ContainerEvent) {}) assert.ErrorContains(t, err, "container start failed") } @@ -356,7 +470,6 @@ func TestPreStart_WaitResultPreferredOverNilError(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "service-ctr-id"} // Both channels are buffered and pre-populated so the outer select in // waitPreStart sees them ready at the same instant. @@ -365,18 +478,16 @@ func TestPreStart_WaitResultPreferredOverNilError(t *testing.T) { resultC <- container.WaitResponse{StatusCode: 0} errC <- nil - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerWaitResult{Result: resultC, Error: errC}) + Return(client.ContainerWaitResult{Result: resultC, Error: errC}).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) expectSuccessRemove(apiClient, "hook-1") - err := tested.runPreStart(t.Context(), project, service, ctr, func(api.ContainerEvent) {}) + err := tested.runPreStart(t.Context(), project, service, func(api.ContainerEvent) {}) assert.NilError(t, err) } @@ -440,13 +551,10 @@ func TestPreStart_DetachedModeAttachesLogs(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "service-ctr-id"} - scan := expectEmptyOrphanScan(apiClient) - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)).After(create1) + Return(waitResultExit(0)).After(scan) // ContainerLogs MUST be called even with a nil listener. logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil).After(wait1) @@ -454,7 +562,7 @@ func TestPreStart_DetachedModeAttachesLogs(t *testing.T) { Return(client.ContainerStartResult{}, nil).After(logs1) expectSuccessRemove(apiClient, "hook-1") - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.NilError(t, err) } @@ -472,7 +580,6 @@ func TestPreStart_FailureIncludesTail(t *testing.T) { {Image: "postgres", Command: types.ShellCommand{"migrate"}}, }, } - ctr := container.Summary{ID: "service-ctr-id"} // Build a stdcopy-multiplexed log stream with a stderr error line. logContent := append( @@ -480,18 +587,16 @@ func TestPreStart_FailureIncludesTail(t *testing.T) { stdcopyFrame(2, "table 'sites' doesn't exist\n")..., // stderr error ) - scan := expectEmptyOrphanScan(apiClient) - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) wait1 := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(1)).After(create1) + Return(waitResultExit(1)).After(scan) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(io.NopCloser(bytes.NewReader(logContent)), nil).After(wait1) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil).After(logs1) // Hook container is retained on failure — no ContainerRemove expected. - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.Assert(t, err != nil) assert.ErrorContains(t, err, "pre_start[0]") // Stderr content must be in the error (stderr bias). @@ -504,7 +609,7 @@ func TestPreStart_FailureIncludesTail(t *testing.T) { // TestPreStart_SuccessRemovesContainer verifies that a successful pre_start hook // triggers an explicit ContainerRemove (with RemoveVolumes: true to mirror the -// old AutoRemove behaviour) and that AutoRemove is false at create time. +// old AutoRemove behaviour). func TestPreStart_SuccessRemovesContainer(t *testing.T) { tested, apiClient := newPreStartTestService(t) @@ -516,17 +621,10 @@ func TestPreStart_SuccessRemovesContainer(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} - var gotAutoRemove bool - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - DoAndReturn(func(_ any, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { - gotAutoRemove = opts.HostConfig.AutoRemove - return client.ContainerCreateResult{ID: "hook-1"}, nil - }).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) + Return(waitResultExit(0)).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). @@ -536,9 +634,8 @@ func TestPreStart_SuccessRemovesContainer(t *testing.T) { ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{RemoveVolumes: true}). Return(client.ContainerRemoveResult{}, nil) - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.NilError(t, err) - assert.Assert(t, !gotAutoRemove, "AutoRemove must be false; retention is managed explicitly") } // TestPreStart_FailureRetainsContainer verifies that a pre_start hook that exits @@ -555,22 +652,19 @@ func TestPreStart_FailureRetainsContainer(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"migrate"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} logContent := stdcopyFrame(2, "migration failed: table missing\n") - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(1)) + Return(waitResultExit(1)).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(io.NopCloser(bytes.NewReader(logContent)), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) // No ContainerRemove expectation: gomock fails on unexpected calls. - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.ErrorContains(t, err, "pre_start[0]") assert.ErrorContains(t, err, "table missing") // Short container ID must appear in the error so the operator can run @@ -594,17 +688,14 @@ func TestPreStart_CancellationRemovesContainer(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"long-running-op"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-cancel-123"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-cancel-123", 0)) // ContainerWait channel: cancel the context before delivering any result so // waitPreStart returns ctx.Err(). resultC := make(chan container.WaitResponse) errC := make(chan error) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-cancel-123", gomock.Any()). - Return(client.ContainerWaitResult{Result: resultC, Error: errC}) + Return(client.ContainerWaitResult{Result: resultC, Error: errC}).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-cancel-123", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-cancel-123", gomock.Any()). @@ -616,7 +707,7 @@ func TestPreStart_CancellationRemovesContainer(t *testing.T) { errCh := make(chan error, 1) go func() { - errCh <- tested.runPreStart(ctx, project, service, ctr, nil) + errCh <- tested.runPreStart(ctx, project, service, nil) }() // Cancel after the hook container has started. cancel() @@ -627,43 +718,6 @@ func TestPreStart_CancellationRemovesContainer(t *testing.T) { assert.Assert(t, !strings.Contains(err.Error(), "retained"), "cancelled hook must not be retained; got: %s", err) } -// TestPreStart_RemovesOrphanBeforeRun verifies that a hook container left behind -// by a previous failed run is force-removed before the new container is created. -func TestPreStart_RemovesOrphanBeforeRun(t *testing.T) { - tested, apiClient := newPreStartTestService(t) - - project := &types.Project{Name: "proj"} - service := types.ServiceConfig{ - Name: "web", - Image: "alpine", - PreStart: []types.ServiceHook{ - {Image: "alpine", Command: types.ShellCommand{"migrate"}}, - }, - } - ctr := container.Summary{ID: "svc-ctr"} - - // ContainerList returns the stale orphan from a previous run. - orphan := container.Summary{ID: "orphan-hook-123", State: "exited"} - scan := apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{Items: []container.Summary{orphan}}, nil) - // Orphan must be removed before the new hook container is created. - removeOrphan := apiClient.EXPECT(). - ContainerRemove(gomock.Any(), "orphan-hook-123", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). - Return(client.ContainerRemoveResult{}, nil).After(scan) - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(removeOrphan) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)).After(create1) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) - expectSuccessRemove(apiClient, "hook-1") - - err := tested.runPreStart(t.Context(), project, service, ctr, nil) - assert.NilError(t, err) -} - // TestPreStart_SuccessRemoveFailureIsNonFatal verifies that a ContainerRemove // failure on the success path is logged but does not fail runPreStart. func TestPreStart_SuccessRemoveFailureIsNonFatal(t *testing.T) { @@ -677,13 +731,10 @@ func TestPreStart_SuccessRemoveFailureIsNonFatal(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) + Return(waitResultExit(0)).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). @@ -694,7 +745,7 @@ func TestPreStart_SuccessRemoveFailureIsNonFatal(t *testing.T) { Return(client.ContainerRemoveResult{}, errors.New("already removed")) // The hook succeeded; the service must start even if removal failed. - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.NilError(t, err) } @@ -769,13 +820,10 @@ func TestPreStart_StreamLogsError_NilListener(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) + Return(waitResultExit(0)).After(scan) // ContainerLogs fails; nil listener → no warning event, done closed immediately. apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(nil, errors.New("logs: connection refused")) @@ -783,7 +831,7 @@ func TestPreStart_StreamLogsError_NilListener(t *testing.T) { Return(client.ContainerStartResult{}, nil) expectSuccessRemove(apiClient, "hook-1") - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.NilError(t, err) } @@ -801,13 +849,10 @@ func TestPreStart_StreamLogsError_WithListener(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) + Return(waitResultExit(0)).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(nil, errors.New("logs: daemon unavailable")) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). @@ -818,7 +863,7 @@ func TestPreStart_StreamLogsError_WithListener(t *testing.T) { listener := func(ev api.ContainerEvent) { gotWarning = ev.Line } - err := tested.runPreStart(t.Context(), project, service, ctr, listener) + err := tested.runPreStart(t.Context(), project, service, listener) assert.NilError(t, err) assert.Assert(t, gotWarning != "", "listener must receive a warning when ContainerLogs fails") assert.Assert(t, bytes.Contains([]byte(gotWarning), []byte("warning")), "expected 'warning' in: %q", gotWarning) @@ -827,7 +872,7 @@ func TestPreStart_StreamLogsError_WithListener(t *testing.T) { // TestPreStart_OldAPIVersion covers the versions.LessThan(apiVersion, "1.44") // branch in createPreStartContainer: on a pre-1.44 daemon the extra-networks // path runs via connectPreStartExtraNetworks. With only one (primary) network -// no NetworkConnect call is issued and the hook succeeds normally. +// no NetworkConnect call is issued and the create succeeds normally. func TestPreStart_OldAPIVersion(t *testing.T) { tested, apiClient := newPreStartTestServiceWithVersion(t, "1.43") @@ -849,20 +894,13 @@ func TestPreStart_OldAPIVersion(t *testing.T) { } ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil) // Single network = primary only; no NetworkConnect expected. - expectSuccessRemove(apiClient, "hook-1") - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + created, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 0, "proj-web-pre_start-0") assert.NilError(t, err) + assert.Equal(t, created.ID, "hook-1") } // TestPreStart_ConnectExtraNetworksSuccess covers connectPreStartExtraNetworks @@ -924,7 +962,7 @@ func TestPreStart_ConnectExtraNetworksFails(t *testing.T) { } // TestPreStart_ContainerStartFailureAndRemoveFails covers the Warnf path in -// runPreStartHook when ContainerStart fails AND the subsequent ContainerRemove +// execPreStartHook when ContainerStart fails AND the subsequent ContainerRemove // also fails (the orphan is unremovable but the caller still gets the start error). func TestPreStart_ContainerStartFailureAndRemoveFails(t *testing.T) { tested, apiClient := newPreStartTestService(t) @@ -937,13 +975,10 @@ func TestPreStart_ContainerStartFailureAndRemoveFails(t *testing.T) { {Image: "alpine", Command: types.ShellCommand{"true"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) - create1 := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)).After(create1) + Return(waitResultExit(0)).After(scan) logs1 := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(emptyLogs(), nil) start1 := apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). @@ -952,82 +987,11 @@ func TestPreStart_ContainerStartFailureAndRemoveFails(t *testing.T) { apiClient.EXPECT().ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). Return(client.ContainerRemoveResult{}, errors.New("removal failed")).After(start1) - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) // The original start error must be returned, not the removal error. assert.ErrorContains(t, err, "start failed") } -// TestPreStart_OrphanScanFails verifies that when ContainerList fails during -// orphan cleanup the Warnf path in runPreStart is hit, but execution continues -// and the hook runs successfully. -func TestPreStart_OrphanScanFails(t *testing.T) { - tested, apiClient := newPreStartTestService(t) - - project := &types.Project{Name: "proj"} - service := types.ServiceConfig{ - Name: "web", - Image: "alpine", - PreStart: []types.ServiceHook{ - {Image: "alpine", Command: types.ShellCommand{"true"}}, - }, - } - ctr := container.Summary{ID: "svc-ctr"} - - // ContainerList fails → Warnf in runPreStart; hook still proceeds. - apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{}, errors.New("daemon unavailable")) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) - expectSuccessRemove(apiClient, "hook-1") - - err := tested.runPreStart(t.Context(), project, service, ctr, nil) - assert.NilError(t, err) -} - -// TestPreStart_OrphanRemovalFails verifies the Warnf path in -// removeOrphanPreStartContainers when an individual stale container cannot be -// removed. The hook must still proceed normally. -func TestPreStart_OrphanRemovalFails(t *testing.T) { - tested, apiClient := newPreStartTestService(t) - - project := &types.Project{Name: "proj"} - service := types.ServiceConfig{ - Name: "web", - Image: "alpine", - PreStart: []types.ServiceHook{ - {Image: "alpine", Command: types.ShellCommand{"true"}}, - }, - } - ctr := container.Summary{ID: "svc-ctr"} - - orphan := container.Summary{ID: "orphan-123"} - // ContainerList finds the orphan; its removal fails → Warnf then continue. - apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{Items: []container.Summary{orphan}}, nil) - apiClient.EXPECT(). - ContainerRemove(gomock.Any(), "orphan-123", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). - Return(client.ContainerRemoveResult{}, errors.New("permission denied")) - // Hook still runs and succeeds. - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil) - apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)) - apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). - Return(emptyLogs(), nil) - apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). - Return(client.ContainerStartResult{}, nil) - expectSuccessRemove(apiClient, "hook-1") - - err := tested.runPreStart(t.Context(), project, service, ctr, nil) - assert.NilError(t, err) -} - // TestPreStart_OldAPINetworkConnectFails covers the createPreStartContainer path // for API < 1.44 with a secondary network: when NetworkConnect fails the function // must clean up the created-but-never-started container and return the error. @@ -1054,9 +1018,8 @@ func TestPreStart_OldAPINetworkConnectFails(t *testing.T) { } ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil) // NetworkConnect for the secondary network fails. apiClient.EXPECT().NetworkConnect(gomock.Any(), "proj_extra", gomock.Any()). Return(client.NetworkConnectResult{}, errors.New("network not found")) @@ -1065,7 +1028,7 @@ func TestPreStart_OldAPINetworkConnectFails(t *testing.T) { ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). Return(client.ContainerRemoveResult{}, nil) - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + _, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 0, "proj-web-pre_start-0") assert.ErrorContains(t, err, "network not found") } @@ -1094,9 +1057,8 @@ func TestPreStart_OldAPINetworkConnectAndRemoveFails(t *testing.T) { } ctr := container.Summary{ID: "svc-ctr"} - scan := expectEmptyOrphanScan(apiClient) apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + Return(client.ContainerCreateResult{ID: "hook-1"}, nil) apiClient.EXPECT().NetworkConnect(gomock.Any(), "proj_extra", gomock.Any()). Return(client.NetworkConnectResult{}, errors.New("network not found")) // Cleanup removal also fails → Warnf; original error is still returned. @@ -1104,7 +1066,7 @@ func TestPreStart_OldAPINetworkConnectAndRemoveFails(t *testing.T) { ContainerRemove(gomock.Any(), "hook-1", client.ContainerRemoveOptions{Force: true, RemoveVolumes: true}). Return(client.ContainerRemoveResult{}, errors.New("removal also failed")) - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + _, err := tested.createPreStartContainer(t.Context(), project, service, ctr, 0, "proj-web-pre_start-0") assert.ErrorContains(t, err, "network not found") } @@ -1133,15 +1095,12 @@ func TestPreStart_RuntimeAPIVersionError(t *testing.T) { } ctr := container.Summary{ID: "svc-ctr"} - // Orphan scan succeeds (ContainerList does not need Ping). - apiClient.EXPECT().ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{}, nil) // createPreStartContainer calls RuntimeAPIVersion → Ping fails. apiClient.EXPECT(). Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}). Return(client.PingResult{}, errors.New("daemon unreachable")) - err = s.runPreStart(t.Context(), project, service, ctr, nil) + _, err = s.createPreStartContainer(t.Context(), project, service, ctr, 0, "proj-web-pre_start-0") assert.ErrorContains(t, err, "daemon unreachable") } @@ -1159,23 +1118,27 @@ func TestPreStart_FailureStdoutOnlyTail(t *testing.T) { {Image: "postgres", Command: types.ShellCommand{"migrate"}}, }, } - ctr := container.Summary{ID: "svc-ctr"} // stdout-only content: stderr frame absent so getTail falls back to stdout. logContent := stdcopyFrame(1, "migration failed: schema mismatch\n") - scan := expectEmptyOrphanScan(apiClient) - apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - Return(client.ContainerCreateResult{ID: "hook-1"}, nil).After(scan) + scan := expectRunnerScan(apiClient, runnerSummary("hook-1", 0)) apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(1)) + Return(waitResultExit(1)).After(scan) apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). Return(io.NopCloser(bytes.NewReader(logContent)), nil) apiClient.EXPECT().ContainerStart(gomock.Any(), "hook-1", gomock.Any()). Return(client.ContainerStartResult{}, nil) - err := tested.runPreStart(t.Context(), project, service, ctr, nil) + err := tested.runPreStart(t.Context(), project, service, nil) assert.ErrorContains(t, err, "pre_start[0]") // Stdout fallback: no stderr → stdout content appears in the error. assert.ErrorContains(t, err, "schema mismatch") } + +// TestGetHookContainerName pins the deterministic runner naming scheme the +// reconciliation plan relies on for convergence. +func TestGetHookContainerName(t *testing.T) { + assert.Equal(t, getHookContainerName("demo", "web", 0), "demo-web-pre_start-0") + assert.Equal(t, getHookContainerName("demo", "web", 12), "demo-web-pre_start-12") +} diff --git a/pkg/compose/reconcile.go b/pkg/compose/reconcile.go index 5d04d815c4..6f7e45c8ef 100644 --- a/pkg/compose/reconcile.go +++ b/pkg/compose/reconcile.go @@ -662,8 +662,6 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { return err } - r.planPurgeStaleHookRunners(service, expected) - containers := r.observed.Containers[service.Name] actual := len(containers) @@ -690,6 +688,14 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { infraDeps := r.infrastructureDeps(service) var lastNode *PlanNode + // containerNodes collects every node planned for this service's replicas, + // so the pre_start hook-runner creation below can wait for the executor's + // live container view to be final before resolving its target replica. + var containerNodes []*PlanNode + // keptRunning reports whether a running replica survives the plan + // untouched — the start phase will then skip pre_start, so no hook + // runner is needed. + keptRunning := false // Process existing containers for i, oc := range containers { @@ -710,11 +716,13 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { Cause: "scale down", Container: &containers[i].Summary, }, "", stopNode) + containerNodes = append(containerNodes, lastNode) continue } if r.mustRecreate(service, expectedHash, parentRecreated, oc, strategy) { lastNode = r.planRecreateContainer(service, &containers[i], infraDeps) + containerNodes = append(containerNodes, lastNode) r.recreatedServices[service.Name] = true continue } @@ -727,6 +735,9 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { // (start.go), which lists containers again and starts them in // dependency order. Exited containers are deliberately left as-is // here so that phase (or the user) decides. + if oc.State == container.StateRunning { + keptRunning = true + } default: // Any other state (paused, dead, ...): attempt to (re)start lastNode = r.plan.addNode(Operation{ @@ -735,6 +746,10 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { Cause: "not running", Container: &containers[i].Summary, }, "", infraDeps...) + containerNodes = append(containerNodes, lastNode) + // The replica is running once this op executes, so the start + // phase will skip pre_start for the service. + keptRunning = true } } @@ -752,40 +767,43 @@ func (r *reconciler) reconcileService(service types.ServiceConfig) error { Number: number, Name: name, }, "", infraDeps...) + containerNodes = append(containerNodes, lastNode) } + r.planPreStartHookRunners(service, expected, keptRunning, containerNodes, infraDeps) + if lastNode != nil { r.serviceNodes[service.Name] = lastNode } return nil } -// mustRecreate decides whether oc must be recreated to match expected. The -// expectedHash and parentRecreated inputs are precomputed once per service by -// planPurgeStaleHookRunners plans the removal of hook-runner containers left -// behind by a previous run that failed before removing them. It mirrors the -// imperative purge living inside the gated runPreStart call: emitted only when -// pre_start is going to run again — hooks declared, a replica to start -// (scale > 0: the imperative start path returns before the hooks for a -// scale-0 service) and no replica running at observation — so a genuinely -// failed hook container stays retained for inspection as long as its service -// is otherwise up. Removals are best-effort (the imperative purge is -// warn-only) and independent of every other node. -func (r *reconciler) planPurgeStaleHookRunners(service types.ServiceConfig, expectedScale int) { - stale := r.observed.HookContainers[service.Name] - if len(stale) == 0 || len(service.PreStart) == 0 || expectedScale == 0 { +// planPreStartHookRunners plans the create-phase side of the pre_start hook +// lifecycle. When the start phase is going to run the hooks — hooks declared, +// a replica to start (scale > 0) and no running replica surviving the plan — +// it purges every observed hook-runner container (leftovers of a previous run +// that failed, or was never started) and creates one fresh runner per declared +// hook. The start phase only executes runners prepared here; it never creates +// one itself. +// +// Purges are best-effort (mirroring the historical imperative purge, which was +// warn-only) but the creates depend on them: runner names are deterministic, +// so a genuinely stuck old runner surfaces as a name conflict on the create. +// The creates also depend on every replica node and on the infrastructure, so +// the executor resolves the target replica (VolumesFrom) against a final live +// view with networks in place. When a running replica survives the plan, the +// start phase skips pre_start entirely: nothing is created, and a hook +// container retained after a failure stays available for inspection. +func (r *reconciler) planPreStartHookRunners(service types.ServiceConfig, expectedScale int, keptRunning bool, containerNodes, infraDeps []*PlanNode) { + if len(service.PreStart) == 0 || expectedScale == 0 || keptRunning { return } - for _, oc := range r.observed.Containers[service.Name] { - if oc.State == container.StateRunning { - return - } - } serviceCopy := service - stale = slices.Clone(stale) + deps := slices.Concat(containerNodes, infraDeps) + stale := slices.Clone(r.observed.HookContainers[service.Name]) slices.SortFunc(stale, func(a, b ObservedContainer) int { return strings.Compare(a.ID, b.ID) }) for i := range stale { - r.plan.addNode(Operation{ + node := r.plan.addNode(Operation{ Type: OpRemoveContainer, ResourceID: fmt.Sprintf("hook:%s:stale:%s", service.Name, stale[i].ID[:min(12, len(stale[i].ID))]), Cause: "stale pre_start hook container", @@ -794,9 +812,23 @@ func (r *reconciler) planPurgeStaleHookRunners(service types.ServiceConfig, expe RemoveVolumes: true, BestEffort: true, }, "") + deps = append(deps, node) + } + for i := range service.PreStart { + node := r.plan.addNode(Operation{ + Type: OpCreateHookContainer, + ResourceID: fmt.Sprintf("hook:%s:%s:%d", service.Name, preStartHookType, i), + Cause: "pre_start hook", + Service: &serviceCopy, + HookIndex: i, + Name: getHookContainerName(r.project.Name, service.Name, i), + }, "", deps...) + deps = []*PlanNode{node} } } +// 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 { diff --git a/pkg/compose/reconcile_test.go b/pkg/compose/reconcile_test.go index 25df7fbdb8..674d5f6f7e 100644 --- a/pkg/compose/reconcile_test.go +++ b/pkg/compose/reconcile_test.go @@ -1280,10 +1280,13 @@ func TestReconcileContainers_StaleHookRunnersPurged(t *testing.T) { plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) assert.NilError(t, err) + // The fresh runner (#4) waits for the replica create (live view final) and + // for both purges (its deterministic name must be free). assert.Equal(t, plan.String(), strings.TrimSpace(` -[] -> #1 hook:app:stale:stale-a-id, RemoveContainer, stale pre_start hook container -[] -> #2 hook:app:stale:stale-b-id, RemoveContainer, stale pre_start hook container -[] -> #3 service:app:1, CreateContainer, no existing container +[] -> #1 service:app:1, CreateContainer, no existing container +[] -> #2 hook:app:stale:stale-a-id, RemoveContainer, stale pre_start hook container +[] -> #3 hook:app:stale:stale-b-id, RemoveContainer, stale pre_start hook container +[1,2,3] -> #4 hook:app:pre_start:0, CreateHookContainer, pre_start hook `)+"\n") for _, n := range plan.Nodes { @@ -1295,8 +1298,80 @@ func TestReconcileContainers_StaleHookRunnersPurged(t *testing.T) { } } -// A scale-0 service never reaches its pre_start hooks (the imperative start -// path returns before them), so its stale runners are not purged either. +// A cold start of a hooked service plans one runner per declared hook, chained +// in declaration order after the replica create. +func TestReconcileContainers_HookRunnersPlannedOnColdStart(t *testing.T) { + project := &types.Project{ + Name: "myproject", + Services: types.Services{ + "app": {Name: "app", Scale: intPtr(1), PreStart: []types.ServiceHook{{}, {}}}, + }, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{}, + HookContainers: map[string][]ObservedContainer{}, + 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 service:app:1, CreateContainer, no existing container +[1] -> #2 hook:app:pre_start:0, CreateHookContainer, pre_start hook +[2] -> #3 hook:app:pre_start:1, CreateHookContainer, pre_start hook +`)+"\n") + + for _, n := range plan.Nodes { + if n.Operation.Type != OpCreateHookContainer { + continue + } + assert.Equal(t, n.Operation.Name, getHookContainerName("myproject", "app", n.Operation.HookIndex)) + } +} + +// A running replica whose config diverged is recreated, so it will not be +// running when the start phase evaluates pre_start: the runner must be +// planned. The observed-running gate applies to replicas that SURVIVE the +// plan, not to the observation itself. +func TestReconcileContainers_HookRunnersPlannedOnRecreate(t *testing.T) { + svc := types.ServiceConfig{Name: "app", Scale: intPtr(1), PreStart: []types.ServiceHook{{}}} + project := &types.Project{ + Name: "myproject", + Services: types.Services{"app": svc}, + } + observed := &ObservedState{ + ProjectName: "myproject", + Containers: map[string][]ObservedContainer{ + "app": {{ + ID: "c1", Number: 1, State: container.StateRunning, ConfigHash: "outdated", + Summary: container.Summary{ + ID: "c1", State: container.StateRunning, + Labels: map[string]string{api.ServiceLabel: "app", api.ContainerNumberLabel: "1", api.ConfigHashLabel: "outdated"}, + }, + }}, + }, + HookContainers: map[string][]ObservedContainer{}, + Networks: map[string][]ObservedNetwork{}, + Volumes: map[string][]ObservedVolume{}, + } + + plan, err := reconcile(t.Context(), project, observed, defaultReconcileOptions(), noPrompt) + assert.NilError(t, err) + + var hookCreates []*PlanNode + for _, n := range plan.Nodes { + if n.Operation.Type == OpCreateHookContainer { + hookCreates = append(hookCreates, n) + } + } + assert.Equal(t, len(hookCreates), 1, "recreated service must get its hook runner:\n%s", plan) +} + +// A scale-0 service never reaches its pre_start hooks (the start phase +// returns before them), so nothing is planned: no runner, no purge. func TestReconcileContainers_StaleHookRunnersKeptWhenScaleZero(t *testing.T) { project := &types.Project{ Name: "myproject", @@ -1319,10 +1394,10 @@ func TestReconcileContainers_StaleHookRunnersKeptWhenScaleZero(t *testing.T) { assert.Assert(t, plan.IsEmpty(), "unexpected plan:\n%s", plan) } -// A running replica gates pre_start off, so the stale runner stays: the -// imperative purge lives inside the gated runPreStart call and would not run -// either — a genuinely failed hook container stays retained for inspection as -// long as its service is otherwise up. +// A running replica surviving the plan gates pre_start off, so nothing is +// planned for the hooks: no fresh runner, and the stale runner stays — a +// genuinely failed hook container is retained for inspection as long as its +// service is otherwise up. func TestReconcileContainers_StaleHookRunnersKeptWhenReplicaRunning(t *testing.T) { svc := types.ServiceConfig{Name: "app", Scale: intPtr(1), PreStart: []types.ServiceHook{{}}} hash := mustServiceHash(t, svc) diff --git a/pkg/compose/service_containers.go b/pkg/compose/service_containers.go index 716acd0a11..add93f346f 100644 --- a/pkg/compose/service_containers.go +++ b/pkg/compose/service_containers.go @@ -599,11 +599,10 @@ func (s *composeService) startService(ctx context.Context, // pre_start runs once per service, only when no replica is already running // (e.g. initial up, force-recreate, or spec change). per_replica: false is - // the only currently supported mode. Pick the replica with the lowest - // container-number so the choice is deterministic regardless of the order - // the daemon returns containers in. + // the only currently supported mode. The hooks execute in runner containers + // prepared by the reconciliation plan. if len(service.PreStart) > 0 && len(serviceContainers) == len(toStart) { - if err := s.runPreStart(ctx, project, service, lowestNumberedContainer(toStart), listener); err != nil { + if err := s.runPreStart(ctx, project, service, listener); err != nil { return err } } diff --git a/pkg/compose/start_test.go b/pkg/compose/start_test.go index 859aa274cc..0b67e96f2e 100644 --- a/pkg/compose/start_test.go +++ b/pkg/compose/start_test.go @@ -185,11 +185,11 @@ func TestStartService_StartsOnlyStoppedReplicas(t *testing.T) { }) } -// TestStartService_PreStartOnLowestReplica locks the pre_start gating: with no -// replica running, the hooks run exactly once, against the replica with the -// lowest container-number — regardless of the order the daemon listed them in -// — and before any service container is started. -func TestStartService_PreStartOnLowestReplica(t *testing.T) { +// TestStartService_PreStartRunsBeforeReplicas locks the pre_start gating: with +// no replica running, the hooks run exactly once — executing the runner +// container the reconciliation plan prepared — and before any service +// container is started. +func TestStartService_PreStartRunsBeforeReplicas(t *testing.T) { svc, apiClient, _ := newStartTestService(t) project := &types.Project{Name: "prj"} @@ -203,21 +203,13 @@ func TestStartService_PreStartOnLowestReplica(t *testing.T) { replica1 := serviceContainer("web", 1, container.StateExited) containers := Containers{replica2, replica1} - // runPreStart sweeps orphan hook containers from any previous failed run - // before creating the new one. - orphanScan := apiClient.EXPECT(). + // runPreStart looks up the runner containers prepared by the plan. + runnerScan := apiClient.EXPECT(). ContainerList(gomock.Any(), gomock.Any()). - Return(client.ContainerListResult{}, nil) - - // The hook container shares the volumes of the lowest-numbered replica. - hookCreate := apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any()). - After(orphanScan). - DoAndReturn(func(_ context.Context, opts client.ContainerCreateOptions) (client.ContainerCreateResult, error) { - assert.DeepEqual(t, opts.HostConfig.VolumesFrom, []string{replica1.ID}) - return client.ContainerCreateResult{ID: "hook-1"}, nil - }) + Return(client.ContainerListResult{Items: []container.Summary{runnerSummary("hook-1", 0)}}, nil) + hookWait := apiClient.EXPECT().ContainerWait(gomock.Any(), "hook-1", gomock.Any()). - Return(waitResultExit(0)).After(hookCreate) + Return(waitResultExit(0)).After(runnerScan) // streamPreStartLogs always opens ContainerLogs (even with nil listener) so // the tail is available for failure error messages. hookLogs := apiClient.EXPECT().ContainerLogs(gomock.Any(), "hook-1", gomock.Any()). diff --git a/pkg/e2e/hooks_test.go b/pkg/e2e/hooks_test.go index e7db829f33..96bfff69b6 100644 --- a/pkg/e2e/hooks_test.go +++ b/pkg/e2e/hooks_test.go @@ -200,6 +200,39 @@ func TestPreStartHookNotReRunOnScaleUp(t *testing.T) { OutputContains("1 /mnt/tokens.txt")) } +func TestPreStartHookCreateThenStart(t *testing.T) { + s := NewScenario(t, "create must prepare the pre_start runners so a later start executes them") + s.Step("create leaves the service created, hooks not yet run", + ComposeCmd("create"), + ServiceState("sample", "created")). + Step("nothing in the volume before start", + probeVolume(s, "sh", "-c", "wc -l < /mnt/tokens.txt || echo missing"), + OutputContains("missing")). + Step("start runs the hook then the service", + ComposeCmd("start"), + ServiceState("sample", "running")). + Step("the hook ran exactly once", + probeVolume(s, "wc", "-l", "/mnt/tokens.txt"), + OutputContains("1 /mnt/tokens.txt")) +} + +func TestPreStartHookStopThenStartFails(t *testing.T) { + NewScenario(t, "start after stop must fail on a hooked service: the runners were consumed, only a reconciliation (up) prepares new ones"). + Step("up runs the hook and starts the service", + ComposeCmd("up", "-d", "--wait").Within(60*time.Second)). + Step("stop leaves the service exited", + ComposeCmd("stop"), + ServiceState("sample", "exited")). + Step("start fails, pointing at the reconciliation command", + ComposeCmd("start").MayFail(), + ExitCode(1), + OutputContains("pre_start[0]"), + OutputContains("docker compose up")). + Step("up prepares fresh runners and recovers", + ComposeCmd("up", "-d", "--wait").Within(60*time.Second), + ServiceState("sample", "running")) +} + func TestPreStartHookRunsOnceForScaledService(t *testing.T) { s := NewScenario(t, "with the default per_replica: false, a pre_start hook must run once for the whole service") s.Step("up starts both replicas", diff --git a/pkg/e2e/testdata/TestPreStartHookCreateThenStart/compose.yaml b/pkg/e2e/testdata/TestPreStartHookCreateThenStart/compose.yaml new file mode 100644 index 0000000000..e00ab31f58 --- /dev/null +++ b/pkg/e2e/testdata/TestPreStartHookCreateThenStart/compose.yaml @@ -0,0 +1,11 @@ +services: + sample: + image: alpine + command: sh -c 'sleep 30' + volumes: + - data:/shared + pre_start: + - image: alpine + command: sh -c 'echo $(cat /proc/sys/kernel/random/uuid) >> /shared/tokens.txt' +volumes: + data: diff --git a/pkg/e2e/testdata/TestPreStartHookStopThenStartFails/compose.yaml b/pkg/e2e/testdata/TestPreStartHookStopThenStartFails/compose.yaml new file mode 100644 index 0000000000..e00ab31f58 --- /dev/null +++ b/pkg/e2e/testdata/TestPreStartHookStopThenStartFails/compose.yaml @@ -0,0 +1,11 @@ +services: + sample: + image: alpine + command: sh -c 'sleep 30' + volumes: + - data:/shared + pre_start: + - image: alpine + command: sh -c 'echo $(cat /proc/sys/kernel/random/uuid) >> /shared/tokens.txt' +volumes: + data: