Resolve pre_start hook images alongside service images#13937
Conversation
pre_start hooks run as ephemeral init containers with their own image (ServiceHook.Image), but that image was ignored by image resolution: `config --images` didn't list it, `pull` didn't fetch it, and `up` failed at runtime with "No such image" when it wasn't already present locally. Add a GetDependentImages helper that returns a service's pre_start hook images, and use it wherever service images are collected/pulled: getLocalImagesDigests, pullRequiredImages (up path), the pull command, and `config --images`. Hook images inherit the parent service pull policy. post_start/pre_stop hooks run via ExecCreate inside the service container and never use hook.Image, so they are intentionally out of scope. Digest resolution/locking (--resolve-image-digests / --lock-image-digests) is not covered: compose-go's WithImagesResolved only resolves service.Image (needs an upstream change), and the --lock-image-digests override merges pre_start by concatenation, which would duplicate hooks. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly surfaces pre_start hook images for resolution, pulling, and listing. The core logic in GetDependentImages, the goroutine closures in pull(), and error propagation are all sound. No high or medium severity bugs were confirmed.
Summary of analysis:
GetDependentImagescorrectly filters empty images and only includespre_starthooks (notpost_start/pre_stop, which have no image to pull).pull()correctly inherits the parent service's pull policy for hook images, including skipping onPullPolicyNever/PullPolicyBuild, checking local presence forPullPolicyMissing/PullPolicyIfNotPresent, and always pulling forPullPolicyAlways.pullRequiredImages()follows the same "pull if missing" pattern as the existing volume-image block — consistent with the function's existing design.- Goroutine closures are safe:
hookServiceis a local copy per iteration. - Error propagation from hook-image pulls goes through
eg.Wait()and is correctly returned to callers.
There was a problem hiding this comment.
Pull request overview
This PR fixes image resolution/pulling for pre_start lifecycle hooks by treating hook images as dependent images of a service, ensuring they are listed by docker compose config --images and pulled by both docker compose pull and the up/create pull path.
Changes:
- Add
api.GetDependentImages(service)to exposepre_starthook images as additional service-dependent images. - Include hook images in local image digest inspection and image pulling logic.
- Extend
config --imagesoutput to includepre_starthook images.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/api/api.go | Adds GetDependentImages helper for pre_start hook images. |
| pkg/api/api_test.go | Adds unit tests for GetDependentImages. |
| pkg/compose/build.go | Includes hook images when collecting local image digests. |
| pkg/compose/build_test.go | Adds unit test validating hook image digest inspection. |
| pkg/compose/pull.go | Pulls missing pre_start hook images in both pull and up/create pull paths. |
| cmd/compose/config.go | Updates config --images to print hook images. |
|
|
||
| for i, img := range api.GetDependentImages(service) { | ||
| if _, ok := images[img]; !ok { | ||
| // Hack: create a fake ServiceConfig so we pull missing pre_start hook image |
There was a problem hiding this comment.
Note to myself: we should extract an ImageRef struct so pullServiceImage doesn't relies on ServiceConfig and we don't need such a hack
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Address review feedback on pre_start hook image resolution: 1. GetDependentImages now skips a hook image equal to the service image (resolved via GetImageNameOrDefault), so `config --images` no longer prints a duplicate line and pullRequiredImages no longer schedules a redundant pull for it. 2. pullRequiredImages (up/create path) now dedups dependent images by reference via a `scheduled` set, so several hooks/services sharing the same missing image don't schedule concurrent redundant pulls. The hook pass moved to a helper (addPreStartHookPulls) to keep complexity in check. 3. The `pull` command no longer skips hook images under `pull_policy: build`. A hook image is a registry image that can't be built, so only `never` justifies skipping it — making `pull` consistent with the `up` path. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly wires pre_start hook images into docker compose config --images, docker compose pull, and the up/create pull path. The overall approach is sound. One inconsistency was found in the up/create path: hook images are not force-pulled when the parent service has pull_policy: always, while service images are. See the inline comment for details.
| continue | ||
| } | ||
| for i, img := range api.GetDependentImages(service, project.Name) { | ||
| if _, ok := images[img]; ok { |
There was a problem hiding this comment.
[medium] addPreStartHookPulls silently skips hook images when service has pull_policy: always
addPreStartHookPulls guards against pulling already-present images with a plain existence check:
if _, ok := images[img]; ok {
continue // skips regardless of pull policy
}This means that when a service has pull_policy: always, its own image is force-pulled every time (because mustPull returns true for PullPolicyAlways), but its pre_start hook images are silently skipped if they are already present locally — they never get the same "always re-pull" treatment.
A user who sets pull_policy: always to ensure freshness on every docker compose up will see the service image updated while stale hook images are left behind, which can lead to subtle divergence between the service container and its init hooks.
The fix is to also respect PullPolicyAlways in addPreStartHookPulls, analogous to how mustPull handles it for service images:
| if _, ok := images[img]; ok { | |
| for name, service := range project.Services { | |
| if service.PullPolicy == types.PullPolicyNever { | |
| continue | |
| } | |
| for i, img := range api.GetDependentImages(service, project.Name) { | |
| if service.PullPolicy != types.PullPolicyAlways { | |
| if _, ok := images[img]; ok { | |
| continue | |
| } | |
| } | |
| if scheduled[img] { | |
| continue | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
What I did
Fixes #13924.
pre_starthooks run as ephemeral init containers with their own image (ServiceHook.Image), but that image was ignored by Compose image resolution:docker compose config --imagesdidn't list itdocker compose pulldidn't fetch itdocker compose upfailed at runtime withNo such imagewhen the hook image wasn't already present locally (the hook has no on-the-fly pull —createPreStartContainerjust callsContainerCreatewithhook.Image)This PR treats
pre_starthook images like service images. A newapi.GetDependentImages(service)helper returns a service'spre_starthook images, and it is used wherever service images are collected/pulled:pkg/api/api.goGetDependentImageshelperpkg/compose/build.go—getLocalImagesDigestspkg/compose/pull.go—pullRequiredImages(up/createpath)VolumeTypeImage)pkg/compose/pull.go—pull(thepullcommand)cmd/compose/config.go—runConfigImagesScope
Only
pre_startis in scope.post_start/pre_stophooks run viaExecCreateinside the service container (pkg/compose/hook.go) and never usehook.Image, so there is nothing to pull for them. An empty hook image falls back to the service image, which is already resolved.Known limitations / follow-ups
Digest resolution and locking are not covered by this PR, deliberately:
config --resolve-image-digests:compose-go'sProject.WithImagesResolvedonly resolvesservice.Image, never hook images. Covering hooks cleanly requires extendingWithImagesResolvedupstream — done in Resolve dependent images (pre_start hooks, type: image volumes) in WithImagesResolved compose-spec/compose-go#894, to be wired here once it is released and vendored.config --lock-image-digests: this produces an override file that is re-merged, butpre_starthas no special merge rule and its list is merged by concatenation (override/merge.go), so a hook override would duplicate the hook instead of pinning it. To be addressed together with / after the compose-go change.Tests
Unit tests only:
TestGetDependentImages(pkg/api) — with/without hook image, and confirmspost_start/pre_stopare not collected.TestGetLocalImagesDigests_PreStartHook(pkg/compose) — the hook image (alpine:3.19) is inspected alongside the service image (alpine:3.20).How to test manually