[htoyoda18]

What this PR does

Since the reconciliation plan (v5.2.0), the containers that up and scale stop on their own (scale down, the container a recreate replaces, a restart: true dependent, a recreated network or volume) were stopped without running their pre_stop hooks. Only stop and down still ran them. The plan's stop operation now runs the service's pre_stop hooks first, as stop and down do.

Related issue

Fixes #14476

Changes made

• pkg/compose/executor_ops.go: execStopContainer calls a new runPreStop before ContainerStop. It runs the hooks of the project's service on a running container with runHook, the same helper execRunPostStart uses.
  • A failing hook fails the plan, and the container is not stopped.
  • Orphans, containers that are not running, and provider relay containers have no hooks to run. This is the same condition stopContainer uses for stop and down.
• pkg/compose/executor_test.go: TestExecutePlanStopContainerRunsPreStop covers four cases: the hook runs before the stop; a failing hook fails the plan without stopping; no hook for a container that is not running; no hook for a container of no service of the project.
• pkg/e2e/hooks_test.go, testdata/TestPreStopHookWhenUpStopsAContainer/compose.yaml: a scenario checking that the hook runs for a scaled-down replica, a restart: true dependent, and a recreated container. Each service writes to its own file in a volume.

Testing done

• Unit tests pass: make test
• E2E tests pass: make e2e-compose or make e2e-compose-standalone
• Linting passes: make lint
• Code formatted: make fmt
• Manually tested (describe):

AI tool used (if applicable)

AI Tool:

Additional context

Two design decisions I was unsure about:

1. The container a recreate replaces now runs pre_stop too (it never did before).
2. A failing hook fails the plan, as with stop and down. On a recreate, this leaves the old container running.

Also, the output of these hooks is not streamed, because they run before runStart sets the hook-log listener.

(not mandatory) A picture of a cute animal, if possible in relation to what you did

[ndeloof]

Thanks for the fix, and for the clear reproduction in #14476. I checked it against the code: execStopContainer calls ContainerStop directly, and none of the OpStopContainer nodes carry the service, so the plan never reaches the pre_stop hooks that v5.1.4 ran through stopContainer. Putting the hook in execStopContainer covers every stop of the plan (scale down, restart: true dependents, network and volume recreation, recreate) without touching the seven sites in reconcile.go, which is the right place.

I read the diff only. I did not run the tests, and CI has not run yet (only DCO shows), so none of this is verified by a run.

On your two design questions: running pre_stop on the container a recreate replaces is a legitimate change, even if v5.1.4 did not do it, and I would keep it. A failing hook failing the plan is consistent with stop and down.

Changes I would ask for

1. A container that is gone or stopped by the time of its stop now fails up. stopContainer in down.go ignores NotFound and Conflict coming from the hook, which is what #12986 added for exactly this case. runPreStop returns the error as is. The ctr.State != container.StateRunning check works on the summary observed when the plan was built, which can be stale: in a recreate, the old container is only stopped after the new one has been created. If it exits in between, ExecCreate fails with a conflict and the whole plan fails, where ContainerStop on a stopped container used to succeed. I would ignore NotFound and Conflict from the hook as down does, and go on to ContainerStop.

2. The hook loop now exists twice. The condition (service known, not a relay container) and the loop over PreStop are in stopContainer and in runPreStop. A shared helper would keep the error policy of point 1 in one place.

To check

3. The temporary container of a recreate whose hook fails. The recreate plan is create (temporary name) → stop old → remove old → rename. If the hook fails, the old container stays running, as the description says, but the new one stays too, created and not started, with the labels of the service. I could not tell what the next up does with it, for instance whether it counts as a second replica. Since a failing hook makes this easy to reach, could you try a recreate with a failing pre_stop, then a second up, and look at docker ps -a? A test for it would help.

Smaller points

4. The description says that skipping containers that are not running is "the same condition stopContainer uses". It is not: stopContainer does not look at the state and relies on the Conflict handling of point 1.
5. The isRelayContainer branch has no test, and the recreate case is only covered by the e2e scenario. A unit case on a plan with a recreate would be quick to add.
6. The "Testing done" checkboxes are all unchecked and the "AI tool" section is empty. Please fill them in.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!