Conversation
Wire the plan executor to the start-phase vocabulary the reconciler already produces (docker#14200): OpWaitCondition, OpRunPreStart, the enriched OpStartContainer, and OpRunPostStart all had no case in executeNode and hit "unknown operation type". Every operation reuses the exact imperative primitive it mirrors — waitDependency, runPreStart, runHook, injectSecrets/injectConfigs — so behavior and events cannot drift between the two engines: execWaitCondition resolves the dependency's live containers and hands off to the same polling/condition-check functions waitDependencies uses; the enriched OpStartContainer resolves its target either from the observed container or, for a replica the plan itself creates, from the CreateContainer node's result (the mechanism OpRenameContainer already relies on), then injects secrets/configs before ContainerStart; OpRunPreStart re-checks the daemon first so a replica that started in the observe-to-execute window skips a redundant hook run. A replica's pre_start/start/post_start chain shares one event group, reporting a single Starting→Started progression exactly like startServiceContainer does today — Starting fires specifically at StartContainer, not at the (silent) pre_start hooks preceding it. pkg/api/event.go documents this per-resource progression contract. Still inert in production: create() (executePlan's only caller never builds a plan with start-phase scope, so none of this runs outside tests until a later brick in epic docker#14081 wires a real caller. EOF ) 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.
Assessment: 🟢 APPROVE
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
A node's done-channel closes strictly before errgroup cancels its
derived context on that same error - the close happens synchronously
inside the failing node's goroutine, ctx cancellation only after it
returns. A dependent blocked in run()'s
select { case <-done[dep.ID]: ...; case <-ctx.Done(): ... } therefore
always sees its dependency's done-channel ready first, regardless of
whether that dependency failed for a genuine reason or was cancelled.
execCreateHookContainer already guarded against exactly this. The two
other new dispatch functions from the previous commit did not:
execStartContainer's enriched branch could call ContainerStart right
after its OpRunPreStart failed for real (e.g. "no hook runner
container found"), contradicting pre_start.go's documented guarantee
that a non-zero hook exit gates service start - and symmetrically for
execRunPostStart after a failed OpStartContainer.
Both now bail out on ctx.Err() first, mirroring the existing guard.
Pinned by a regression test reproducing the race deterministically: no
ContainerStart expectation is registered, so the unguarded code calls
it from inside the errgroup worker goroutine, where gomock's missing-
expectation failure hangs the test binary instead of failing it
cleanly - a stronger tell than a plain assertion failure would have
been.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Fixed in 3cecc1e: both |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers the commits pushed since ed531fc6b35d (the incremental diff for this cycle).
Assessment: 🟡 NEEDS ATTENTION
The fix correctly applies the ctx.Err() guard pattern (already used by execCreateHookContainer) to execStartContainer and execRunPostStart, and the regression test is well-structured. However, the guard does not fully close the race it targets: close(done[node.ID]) in run() fires inside the failing goroutine before errgroup calls cancel(), so there is a real window where a dependent goroutine sees the done-channel ready but ctx.Err() still returns nil.
docker-agent's review of the previous commit's fix correctly pointed out that the ctx.Err() guard added to execStartContainer/execRunPostStart narrows the race it targets but does not eliminate it: close(done[...]) still runs inside the failing node's own goroutine, strictly before errgroup's cancel() on that goroutine's returned error. A dependent unblocked from <-done[dep.ID] can observe ctx.Err() == nil for a brief window even though its dependency just failed for a genuine reason. This is exactly the structural gap the epic (docker#14081) already flagged in its "failed-dependency semantics" comment before this PR existed: the real fix carries each node's success/failure through what dependents wait on (e.g. a chan error) instead of inferring it from ctx.Err(), uniformly across every operation type - including the already-merged execCreateHookContainer, which has the same limit. That is a dedicated brick of the executor lot, not squeezed into this PR's diff. No behavior change: this documents the residual window explicitly at both guard sites and in the regression test, so the next reader (and the next PR) doesn't mistake "narrows" for "closes". Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Good catch — you're right that the |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 3cecc1e.
Assessment: 🟢 APPROVE
The incremental diff is purely comment/documentation changes — no code logic was modified. The two comment blocks in executor_ops.go now accurately describe the ctx.Err() checks as "interim narrowing" rather than "guards against" the race, explicitly acknowledge the window where the race still exists, and point to #14081 for the structural fix. The test doc comment in executor_start_test.go updates to matching language.
These comment updates are a direct and accurate response to the two prior review threads on this file (the ctx.Err() race at lines 160 and 287). The technical claims in the new comments are factually correct and consistent with the code.
Same interim narrowing as execStartContainer and execRunPostStart: the node hangs off the replica's create node, so when that create fails the pre_start hooks could still fire in the window before errgroup cancels the context. The structural fix stays tracked in docker#14081. 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.
This review covers only the commits pushed since 341a5ac.
Assessment: 🟢 APPROVE
The incremental diff adds a single ctx.Err() guard to execRunPreStart — the same interim-narrowing pattern already present at execStartContainer (line 160) and execRunPostStart (line 296), with an explicit comment acknowledging the residual race window. The guard is correct Go, introduces no new behavior relative to the existing two guards, and is consistent with the codebase convention. The comment accurately describes both the interim nature of the fix and the remaining race window. No bugs introduced by the added lines.
This branch has not been deployed
3 participants
Part of #14081 — Lot 1, item 3 ("executor runs start-phase operations").
What this PR does, in one sentence
It teaches the plan executor to actually run the start-phase operations the reconciler has already known how to describe since #14200 — waiting on a dependency's condition, running pre_start hooks, starting a container with the right injections, running post_start hooks — closing the gap between "the plan can say it" and "the plan can do it".
Context
#14200 taught the reconciler to plan the start phase:
OpWaitCondition,OpRunPreStart, an enrichedOpStartContainer,OpRunPostStart. Nobody consumed them — the executor's dispatch had no case for any of the four, so a plan carrying them would fail with "unknown operation type". The vocabulary existed; nothing could speak it.What the PR brings
The executor now runs every start-phase operation — by reusing the exact primitive the imperative engine (
start.go/service_containers.go) already uses for it, rather than reimplementing it: a wait delegates to the same polling/condition-check functionswaitDependenciescalls, pre_start/post_start hooks run through the samerunPreStart/runHook, and a start injects secrets/configs through the sameinjectSecrets/injectConfigsbeforeContainerStart. Behavior cannot drift between the two engines because there is only one implementation of each behavior.Two things follow from that shared-primitive design:
Starting → Startedprogression — exactly whatstartServiceContainerreports today — rather than three separate, confusing lines.pkg/api/event.gonow documents this per-resource progression contract, so the promise is written down, not just implied by the code.OpStartContainerresolves its target from theCreateContainernode's result when there is no prior observation — the same mechanismOpRenameContaineralready relies on.Guardrail unchanged from #14200: inert by construction.
create()— the only caller ofexecutePlan— never builds a plan with start-phase scope, so none of this runs outside tests yet. It lands now, proven correct in isolation, so the next bricks (the executor lot's remaining piece, then the actual migration lots) can build on a tested foundation instead of growing it and wiring a consumer in the same change.Why this is the right next brick
This is the second item of Lot 1 of epic #14081 (issue checklist): the vocabulary from #14200 gets an engine to run it before anything is asked to depend on it working.
🤖 Generated with Claude Code