fix: address #14011 image-identity review findings + corner-case e2e coverage - #14041
Merged
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 #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 #13636, #13998 and #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>
- pullRequiredImages resolves each distinct pulled image once, after all pulls completed, for the host default platform — the exact way getLocalImagesDigests resolves already-local images. Resolving from each pull's goroutine, for the pulled platform, let the recorded digest depend on pull completion order when several services pull the same tag for different platforms (last-writer-wins), and made the first up after a pull recreate containers. Platform-pinned services keep getting their own platform's digest from serviceImageDigest. In dry-run nothing was actually pulled, so the local inspect is skipped — it reached the real daemon through a DryRunClient dispatch that no longer matched and failed with 'No such image'. - explicit 'compose pull' treats daily/weekly/every_N windows as due: it is the only way to force a refresh ahead of the window. - 'compose pull' no longer silently skips services declaring both provider: and image:. - pre_start hook images go through the same mustPull interpreter on the up path as on the pull path, so refresh windows apply consistently. - matchLocalManifest falls back to the inspect's flat platform fields when the lone available manifest carries no ImageData (locally built images), instead of reporting the platform unsatisfied and pulling a possibly local-only image. - scale resolves DOCKER_DEFAULT_PLATFORM without validating build.platforms: a conflict on a service that isn't being built must not abort the command. run needs nothing: createOptions.Apply resolves and validates platforms on its path already. - resolveImageVolumes documents the accepted one-time-recreate and retag-race tradeoffs. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
- dry-run up with a missing image must not hit the real daemon - create twice with a non-native DOCKER_DEFAULT_PLATFORM is idempotent - two services sharing an image with mixed platform pinning get their own platform's digest label, idempotently, whatever pull ordering - explicit pull refreshes ahead of a daily refresh window Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Compose manipulates two digest kinds that must never be conflated: the platform-specific content digest (localContentDigest) is ONLY an identity to compare a running container with a fresh build/pull, while pinning image references in a reproducible compose model (publish / config --resolve-image-digests) must keep the registry descriptor digest — the multi-platform index — through ImageDigestResolver, or the published file would be bound to the platform of whoever resolved it. Document the distinction on both producers and lock it with a test that fails if the resolver ever goes through a local image inspect. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Regression test for #14007: with the containerd image store, a local multi-platform image holding the requested non-native variant must be used as-is by the default missing pull policy — compose used to inspect the image without a platform, compare the host variant's platform fields to the requested one, conclude the image is missing and try to pull the (unpublished) tag. The test fails on main and passes with #14011, whose platform check resolves the requested platform against the locally available manifests. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Regression test for #14005, pulled-image scenario: with the source image of a type=image volume already present in the local store, compose used to rewrite the mount source to a digest the daemon can't resolve as a mount source under the containerd image store (No such image). TestImageVolume only exercises this path when a previous test happens to have left the image locally; pre-pulling makes it deterministic. Also asserts a second unchanged up doesn't recreate the service. The test fails on main under the containerd image store and passes with the fix from #14011. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Several image-identity bugs only manifest on one image store backend: the graphdriver and containerd stores report different digest kinds and resolve them differently (#14005, #14007, #14014 all reproduce only under the containerd store, which no CI job covered). Cross the stable e2e jobs with a store axis (graphdriver/containerd) so both backends are exercised in plugin and standalone modes; oldstable jobs stay on the default graphdriver. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
The 'builder does not support multi-arch' subtest expects the docker driver to reject multi-platform builds, and skipped itself by checking that buildx lists both linux/amd64 and linux/arm64. That heuristic misses containerd-store hosts without binfmt emulation (plain CI runners): only native platforms are listed even though the driver happily cross-builds, so the build succeeds and the test fails with 'ExitCode was 0 expected 1'. Skip on the containerd image store directly, detected from the daemon's DriverStatus. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Replaces #14011: the branch carries #14011's commits as-is (authorship preserved, including the integrated #13998 by @benjick and #14006 by @ricardobranco777), plus the fixes for the review findings posted there, e2e coverage for the image-identity corner cases they exposed, and the standalone regression tests from #14025/#14026.
fix: address image-identity review findings:pullRequiredImagesnow resolves each distinct pulled image once, after all pulls completed, for the host default platform — the recorded digest no longer depends on which service pulled last or on the pulled platform (several services can pull the same tag for different platforms concurrently). Platform-pinned services keep getting their own platform's digest fromserviceImageDigest. The dry-run path skips the local inspect (nothing was pulled), fixing--dry-run upfailing withNo such imagesince theDryRunClientcaller-name dispatch no longer matched. Found along the way: the resolution must use the caller's context — the errgroup context is canceled as soon asWaitreturns.compose pulltreatsdaily/weekly/every_Nwindows as due (only way to force a refresh ahead of the window) and no longer silently skips services declaring bothprovider:andimage:.pre_starthook images go through the samemustPullinterpreter on theuppath as on thepullpath.matchLocalManifestfalls back to the flat platform fields when the lone available manifest carries noImageData(locally built images) instead of triggering a pull of a possibly local-only image.scaleresolvesDOCKER_DEFAULT_PLATFORMwithout validatingbuild.platforms;runneeds nothing (createOptions.Applyalready resolves and validates on its path).resolveImageVolumesdocuments the accepted one-time-recreate and retag-race tradeoffs.test: cover image-identity corner cases end to end: dry-run up with a missing image; idempotentcreate×2 under a non-nativeDOCKER_DEFAULT_PLATFORM; two services sharing an image with mixed platform pinning labeled with their own platform's digest, idempotently, whatever pull ordering; explicit pull refreshing ahead of a daily window — plus unit tests (hook refresh windows, provider+image,ImageData-less manifests, single post-pull resolution).Validated: unit + lint green, the whole image-identity e2e family green on a containerd-store daemon, counter-proof red on #14011's head for the dry-run and refresh-window tests, and two independent adversarial review passes (second one ran the rewritten unit test 20× under
-race). Known accepted nits: an inspect error right after a successful pull of a buildable service is now fatal (exotic: concurrentrmibetween pull and inspect), and the"pullServiceImage"case inpkg/dryrun/dryrunclient.gois now dead code (left untouched to keep the diff focused).docs+test: disambiguate content digests from distribution digests: Compose now manipulates two digest kinds that must never be conflated — the platform-specific content digest (localContentDigest) is only an identity for staleness comparison, while pinning references in a reproducible compose model (publish/config --resolve-image-digests) keeps the registry descriptor digest (the multi-platform index) throughImageDigestResolver. Both producers now document the distinction, and a test fails if the resolver is ever funneled through a local image inspect.Also cherry-picks the standalone e2e regression tests from #14025 (#14007: local multi-platform image must satisfy the
missingpull policy) and #14026 (#14005: image volume from an already-local image) — both ride along with the fix and pass — and the full e2e CI matrix from #14027 (plugin/standalone × graphdriver/containerd), which absorbs the single containerd job #14011's ci.yml change introduced, so every store×mode combination runs on this PR's CI.Related issue
Fixes #14005
Fixes #14007
Fixes #14016
Fixes #14036
Closes #14011
Closes #14025
Closes #14026
Closes #14027