Fix image digest reconciliation - #14011
Conversation
… digest With the containerd image store, ImageSummary.ID holds the digest of the platform-specific manifest so ServiceHash stays stable across attested rebuilds (see contentDigest). resolveImageVolumes reused that same value as the `type: image` mount Source, but the daemon only resolves a mount Source by name/tag or top-level image ID, not by manifest digest — so `compose up` failed with "No such image" whenever the volume's source image was already present locally (always for a built image; on a second run for a pulled one). Keep Source as the resolved image name, and track the digest separately via a new com.docker.compose.image-volume-digest label so mustRecreate can still detect a rebuilt/updated source image independently of Source. Fixes docker#14005 Signed-off-by: Ricardo Branco <rbranco@suse.de> Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
The e2e suite only ran on graphdriver daemons, where the different kinds of image digests coincide — the blind spot that let docker#13636, docker#13998 and docker#14005 through. Add one matrix entry enabling the containerd image store, plus TestUpIdempotentContainerdStore: two consecutive `up` runs with no change must not recreate any container. The test is red on this configuration (the com.docker.compose.image label is written from the index digest on the pulling run, then compared against the per-platform manifest digest on the next run) and skipped until the next commit resolves the pull-path digest. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
pullServiceImage returned the pulled image's raw inspect ID, while getImageSummaries resolves already-local images through contentDigest (the platform image-manifest digest). Both values feed the com.docker.compose.image label that mustRecreate compares to detect image changes, so the two paths disagreeing made the first 'up' after the pulling 'up' see a phantom image change and recreate every container once, with no change anywhere. Under the containerd image store a tag@digest reference triggers this: the raw inspect ID is the index digest, while contentDigest picks the platform manifest digest. Resolve the pulled image through the same manifests-aware inspect and contentDigest call getImageSummaries uses, so both sides of the staleness comparison speak the same scheme. Verified against a fresh docker:dind (29.7.0, containerd store) with a tag@digest service: unpatched v5.4.0 recreates the container on the second 'up'; with this fix the container survives repeated 'up' runs. Existing behavior is preserved for engines without manifest support (contentDigest falls back to the plain ID). (Squashed with the follow-up lint cleanup from the same PR.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Max Malm <benjick@dumfan.net> Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Image identities recorded for staleness detection were produced by several independent paths yielding different digest kinds for the same image: the platform check compared flat inspect fields while the digest picked a manifest with the host matcher (never the service's pinned platform), a wrong-platform summary just discarded still leaked its digest into the label, bake substituted digests host-side in batch, and the classic builder recorded the raw build-stream ID as-is. Any of those mismatches makes the next up see a phantom image change and recreate containers. Converge every producer on one selection (matchLocalManifest / localContentDigest): the shared parallel inspect feeds both the digest and the platform check, platform-pinned services resolve THEIR platform's manifest in-process (no extra API call), and both builders route through canonicalBuiltDigest. Registry-only builds (push-only, multi-platform without load) keep the builder-reported digest — volatile but honest, an actual rebuild is still detected, where a stable placeholder would hide real image changes. ensureImagesExists' final loop becomes the label's single writer so the pinned resolution can't be overwritten, superseded only by pull/build results already platform-resolved by their producers — and when a pull or build refreshed the shared entry mid-run (a digest resolved for whichever service triggered it), a service pinned on another platform re-resolves its own with one extra inspect, in that case only. With every producer converged, TestUpIdempotentContainerdStore is un-skipped here. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
scale and run were the only container-creating commands that never called applyPlatforms, yet both go through the regular create path and its config-hash comparison (run for the dependencies it starts). With DOCKER_DEFAULT_PLATFORM set, they hashed an empty service Platform where up had hashed the resolved one, so every invocation recreated the affected containers. run's project preparation is extracted to a helper to keep runCommand under the complexity threshold. No unit test: neither command has a test harness and the fix is the one missing call, aligned on create/watch; the config-hash equality is covered by the reconciler tests. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
compose pull switched on the raw pull_policy string: daily/weekly/every_N never matched a case and fell through to an unconditional re-pull, and the hook-image loop was a second interpreter that ignored the refresh window entirely. Delegate the decision to the exact interpreter the up path uses (mustPull), with hook images routed through the same decision (build mapped to missing — a hook image can't be built as a fallback). Two deliberate differences with up are kept and documented in shouldPullImage: a service without an explicit pull_policy is always refreshed (skipping it would turn an explicit compose pull into a no-op once images exist), and a present latest tag is still refreshed under missing/if_not_present — the tag is expected to move, and triggering the pull lets the daemon negotiate with the registry, a manifest check with no download when the local image is already current. User-visible change (changelog): compose pull now honors daily/weekly/every_N refresh windows instead of always re-pulling. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
|
Thanks for picking this up and generalizing it — the single-producer framing is much better than what I had, and the containerd CI job is the part I'm most glad to see. I went looking for whether the class is fully closed and I think one case remains: The pull side resolves an effective platform ( platform := service.Platform
if platform == "" {
platform = defaultPlatform
}
...
id, _, err := s.inspectLocalContent(ctx, service.Image, platform)The local side doesn't: I reproduced it against a fresh services:
app:
image: alpine:3.20
command: ["sleep", "infinity"]
Run 2 is stable because amd64 is the only available manifest at that point, so Worth noting the container in run 3 is still running x86_64 ( To be clear this isn't a regression from this PR — the same trace on the base commit recreates at run 2 and run 3 ( If you agree it's in scope, it looks like a small fix: Keeping up the tradition, my dog: |
ndeloof
left a comment
ndeloof
left a comment
There was a problem hiding this comment.
General concern — platform-specific digest as identity label
The core change moves the recorded identity (com.docker.compose.image) from the top-level image ID to a platform-specific manifest digest, but platform resolution is not a single well-defined function of (service, host) across the code paths that resolve it:
pullServiceImageresolves withservice.PlatformorDOCKER_DEFAULT_PLATFORM;imageSummary→localContentDigestresolves withservice.Platformor""(host default);applyPlatformsonly pinsservice.Platformwhen the service has abuildsection;pullRequiredImagescollapses per-service digests into a per-image map (images[service.Image]), losing the platform dimension.
Whenever two of these paths resolve the same tag to different manifests — multi-platform images in a containerd store, DOCKER_DEFAULT_PLATFORM set, services sharing an image with heterogeneous platform: — the label comparison in mustRecreate flips and we get exactly the phantom-recreate class this PR sets out to fix, except now nondeterministic and platform-dependent.
Suggestion: define one resolvePlatform(service) rule used by every digest-resolving path, and key the pulled-digest bookkeeping by (image, resolvedPlatform) instead of image name. The inline comments detail the concrete scenarios, plus a few adjacent regressions (dry-run, explicit pull semantics, image-volume pinning).
| // index digest, under the containerd store with a tag@digest ref) while | ||
| // later ups resolve the platform manifest digest via contentDigest made | ||
| // the first up after a pull recreate every container despite no change. | ||
| id, _, err := s.inspectLocalContent(ctx, service.Image, platform) |
There was a problem hiding this comment.
pullServiceImage now goes through inspectLocalContent, which breaks DryRunClient.ImageInspect's caller-name dispatch (getCallingFunction() uses runtime.Caller(2) and only matches pullServiceImage/buildContainerVolumes): in --dry-run up/pull the post-pull inspect falls into the default branch, queries the real daemon for an image that was never actually pulled, and fails with No such image. The dry-run switch needs to match the new caller — and this fragility is a good argument for revisiting the caller-name dispatch altogether.
| // index digest, under the containerd store with a tag@digest ref) while | ||
| // later ups resolve the platform manifest digest via contentDigest made | ||
| // the first up after a pull recreate every container despite no change. | ||
| id, _, err := s.inspectLocalContent(ctx, service.Image, platform) |
There was a problem hiding this comment.
Now that the resolved digest is platform-specific: two services sharing the same image but resolving different platforms (e.g. one with platform: linux/amd64, one unpinned on an arm64 host) are both scheduled by pullRequiredImages (needPull only dedups volume/hook images), and images[service.Image] = pulledImages[i] is last-writer-wins over map iteration order. The recorded digest for the shared image is nondeterministic, and the unpinned service can be labeled with the other service's platform digest → spurious recreate on the next up. Keying the bookkeeping by (image, resolvedPlatform) — or recording digests per service — avoids the collision. Pre-PR both pulls returned the same top-level inspect.ID, so no divergence was possible.
| // index digest, under the containerd store with a tag@digest ref) while | ||
| // later ups resolve the platform manifest digest via contentDigest made | ||
| // the first up after a pull recreate every container despite no change. | ||
| id, _, err := s.inspectLocalContent(ctx, service.Image, platform) |
There was a problem hiding this comment.
Platform asymmetry: this path resolves the digest with defaultPlatform (DOCKER_DEFAULT_PLATFORM), but for services without a build section applyPlatforms leaves service.Platform empty, so later runs compute the label via imageSummary → localContentDigest(inspect, "") against the host default platform. With DOCKER_DEFAULT_PLATFORM set to a non-native platform and both variants present locally (containerd store), the two paths disagree → recreate despite no change. Both paths should share the same platform-defaulting rule (see general comment).
| policy, _, _ := service.GetPullPolicy() | ||
| switch policy { | ||
| case types.PullPolicyRefresh: | ||
| return false, "Image is not due for refresh", nil |
There was a problem hiding this comment.
Behavior change for explicit docker compose pull: by delegating to mustPull, services whose refresh window (daily/weekly/every_N) isn't due are now skipped — previously pull always refreshed them. Users lose the only way to force-refresh these images short of editing the compose file. If aligning with up is intentional, explicit pull probably needs an escape hatch (treat it as "due now", or honor a --policy always override).
| return true, "", nil | ||
| } | ||
| return false, "Image is already present locally", nil | ||
| default: // never, build — and provider services short-circuited by mustPull |
There was a problem hiding this comment.
mustPull short-circuits on service.Provider != nil, so compose pull now silently skips services declaring both provider: and image: — including pull_policy: always — where the old pull loop fetched them. If skipping is intended, the skip event should say why; otherwise pull should still fetch the declared image.
| for name, service := range project.Services { | ||
| if service.PullPolicy == types.PullPolicyNever { | ||
| continue | ||
| hookPolicy := service.PullPolicy |
There was a problem hiding this comment.
Hook images still have two divergent pull-policy interpreters: this loop (build→missing + synthetic ServiceConfig through shouldPullImage) vs the inline logic in addPreStartHookPulls on the up path, which ignores refresh windows. So pull_policy: daily hook images behave differently between compose pull (window honored) and compose up (pull-if-missing). Same up-vs-pull divergence class this PR fixes for service images — one shared helper would remove the drift point.
| // localContentDigest). Keep Source as the resolved name so mounting always | ||
| // works, and track the digest separately so mustRecreate can still detect | ||
| // a changed source image. | ||
| service.Volumes[i].Source = imgName |
There was a problem hiding this comment.
Dropping vol.Source = img.ID changes what feeds ServiceHash (volumes are part of the hashed config): containers created by the previous release were hashed with Source=<sha256 ID>, the new code computes Source=<image name>, so every service using type: image volumes is recreated once on compose upgrade with zero config/image change. Needs either migration handling in mustRecreate or at least a release note, given the PR's goal is eliminating needless recreates.
| // localContentDigest). Keep Source as the resolved name so mounting always | ||
| // works, and track the digest separately so mustRecreate can still detect | ||
| // a changed source image. | ||
| service.Volumes[i].Source = imgName |
There was a problem hiding this comment.
Related (racy edge): the mount is now created from the mutable tag while the digest label comes from a separate inspect — a concurrent retag between the two leaves the container mounting the new content while labeled with the old digest (mismatch → recreate on next up; until then the running content doesn't match the recorded identity). The old ID-pinned Source made this impossible; pinning the mount to the digest just resolved would keep both properties.
| } | ||
| if len(available) == 1 { | ||
| return available[0].ID | ||
| return available[0].ID, false |
There was a problem hiding this comment.
When the matching manifest entry has no ImageData populated, this now reports the platform unsatisfied where the old flat-field Architecture/Os/Variant check accepted it; getLocalImagesDigests then drops the image from the summary and up attempts to pull a local-only image, which fails. Seen with locally-built (never pushed) images on containerd-store engines. Falling back to the flat inspect fields when ImageData is absent would preserve the old behavior.
| // resolve DOCKER_DEFAULT_PLATFORM into service.Platform exactly like | ||
| // `up`/`create` do: Platform feeds the service config-hash, so scale | ||
| // hashing a different value would recreate every container | ||
| if err := applyPlatforms(project, true); err != nil { |
There was a problem hiding this comment.
scale (and run via runProject) now evaluate applyPlatforms, so a DOCKER_DEFAULT_PLATFORM vs build.platforms conflict hard-fails commands that previously never checked it — e.g. compose scale web=3 aborts on a dependency's build.platforms even though nothing is being built. Consider validating only when a build will actually happen.
5 participants
What I did
Compose records image identities for staleness detection (the
com.docker.compose.imagelabel compared bymustRecreate) through several independent code paths — pull, bake, classic builder, already-local inspect — and they didn't all produce the same kind of digest for the same image: top-level index digest, per-platform manifest digest, or config digest, depending on the engine (graphdriver vs containerd image store), the API version, the builder, and how the image arrived locally. Whenever two runs resolved the same image through different paths, the comparison failed and containers were recreated with no actual change. This mismatch class is behind #13636, #14005, and the phantom-recreate-after-pull fixed by #13998 — plus several latent cases (platform-pinned services resolved with the host platform, wrong-platform digests leaking into labels, bake push-only builds recreating on everyup).This PR fixes the class, not just the instances:
matchLocalManifest/localContentDigest): every path — pull, bake, classic builder, local inspect — resolves the recorded identity the same way. Digest selection and the platform-mismatch check now share the same manifest resolution, so the digest recorded and the platform validated always refer to the same manifest.ensureImagesExistsis the only writer ofcom.docker.compose.image. Platform-pinned services get the digest of their platform's manifest, resolved in-process from the already-fetched inspects (zero extra API calls in steady state).type: imagevolumes mount by resolvable name and track their source-image digest in a dedicatedcom.docker.compose.image-volume-digestlabel (fixes the [BUG] type=image volumes fail with "No such image" when the source image is already present locally #14005 regression).TestUpIdempotentContainerdStorelocks the invariant: two consecutiveupruns with no change must not recreate anything.scaleandrunnow resolveDOCKER_DEFAULT_PLATFORMlikeupdoes (they recreated containers on every invocation otherwise), andcompose pullinterpretspull_policythrough the same interpreter asup(two deliberate, documented differences remain: an unset policy always refreshes, and a presentlatesttag is still refreshed undermissing).This PR supersedes and includes #13998 (@benjick) and #14006 (@ricardobranco777) — both commits are integrated with their original authorship preserved. Thanks to both for the investigations that narrowed this down.
User-visible changes
compose pullnow honorsdaily/weekly/every_Nrefresh windows instead of always re-pulling.upafter upgrading: label values change kind (and image-volume users transition from digest-as-mount-source to the new label). Subsequent runs are stable.Each commit is independently CI-green and reviewable on its own: image-volume mount fix → containerd-store CI job (test skipped) → pulled-image content digest (test enabled) → canonical producer/single writer → scale/run platform resolution → pull policy alignment.
Related issue
Fixes #14005
Supersedes #13998, #14006
(not mandatory) A picture of a cute animal, if possible in relation to what you did
