Conversation
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 (docker#14200). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
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 (<project>-<service>-pre_start-<i>) 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 <service>) — 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 <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
docker-agent to retry.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Nice one, turning hook runners into first-class plan resources removes a whole class of ad-hoc bookkeeping. Two non-blocking should-fixes found while going through the diff (AI-assisted review, verified against the code before posting), nothing here blocks merge.
| 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} |
There was a problem hiding this comment.
The loop reassigns deps to just the previous hook's node, chaining every OpCreateHookContainer for a service into a serial dependency line — even though each targets the same already-live replica independently and has no reason to wait on a sibling hook's create. Keeping deps fixed at the shared set (containerNodes+infraDeps+purges) lets the executor create them in parallel instead.
| 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} | |
| for i := range service.PreStart { | |
| 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...) | |
| } |
| 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 |
There was a problem hiding this comment.
Side effect of the fix suggested in reconcile.go: with deps no longer reassigned per iteration, hook[1]'s create would depend on [1] (the replica create) instead of [2] (hook[0]'s own node).
| [2] -> #3 hook:app:pre_start:1, CreateHookContainer, pre_start hook | |
| [1] -> #3 hook:app:pre_start:1, CreateHookContainer, pre_start hook |
| 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) |
There was a problem hiding this comment.
Narrow but real race: keptRunning is decided from the pre-plan ObservedState snapshot in reconcile.go, while startService re-checks container state independently via a fresh ContainerList after the whole create-phase plan has executed. If the sole surviving replica is stopped externally in that window, plan-time skips runner creation (replica looked like it would survive) but start-time now sees nothing running and hits this hard error — previously self-healing via the inline create, now a dead end pointing at up for a situation up didn't cause.
Not asking for the full fix here (that'd mean re-validating state at start time instead of trusting two independent daemon reads), but the message could at least hint this isn't necessarily a planning bug:
| 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) | |
| return fmt.Errorf("service %q pre_start[%d]: no hook runner container found — either its runner was already consumed by a previous start, or the service's container state changed after reconciliation planned this run; run %q to prepare fresh runners", | |
| service.Name, i, "docker compose up "+service.Name) |
3 participants
What this PR does, in one sentence
pre_starthook containers become first-class resources of the reconciliation plan: the plan creates them, the start phase only executes them.Context
pre_starthook containers have their own identity — a name, labels, and possibly an image different from the service's — yet they live entirely outside the reconciliation plan: todayrunPreStartcreates, runs and removes them imperatively inside the start flow. The engine cannot see them, so it cannot order them, purge them deterministically, or account for their failures the way it does for every other resource. Leftover runners from a failed run are swept by ad-hoc label scans, and nothing ties a runner to the hook it was created for.What the PR brings
The reconciler now plans one
CreateHookContaineroperation per declared hook, whenever the start phase is going 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 is properly ordered in the DAG: after every replica operation of its service (so the executor resolves theVolumesFromtarget against a final live view), after the infrastructure, and after the purge of previously observed runners. Runners get a deterministic name (<project>-<service>-pre_start-<i>) and a newcom.docker.compose.hook-indexlabel, so repeated plans converge on the same container instead of accumulating anonymous ones, and the start phase can match each declared hook with the runner prepared for it.runPreStartbecomes pure execution: it looks up the created-state runner for each hook and never creates one. A declared hook without a prepared runner is an actionable error naming the reconciliation command to run (docker compose up <service>). The accepted consequence — locked by an e2e scenario — is thatstopthenstartof a hooked service errors, since runners are consumed on success;upcovers the overwhelming majority of usage and always prepares fresh runners.Guardrails: success removal, failure retention (container kept for post-mortem) and cancellation cleanup are byte-for-byte the previous behavior; runners stay invisible to start/ps listings (they carry no config-hash label, which the default filters require);
downkeeps removing them by hook label; runners from previous compose versions (no index label) are recognized and purged.Why this is the right next brick
The plan engine currently covers the create phase only. Moving runner creation into that phase needs no start-phase operations, so this lands independently of the start-phase plan (#14200) while setting up the follow-up: with runners as plan resources, hook execution and removal can become start-phase operations, making failure retention a DAG property instead of imperative code.