diff --git a/pkg/compose/pull.go b/pkg/compose/pull.go index d80bfc421..1471e7121 100644 --- a/pkg/compose/pull.go +++ b/pkg/compose/pull.go @@ -77,24 +77,20 @@ func (s *composeService) pull(ctx context.Context, project *types.Project, opts continue } - switch service.PullPolicy { - case types.PullPolicyNever, types.PullPolicyBuild: + pullRequired, skipReason, err := shouldPullImage(service, images) + if err != nil { + // join already-scheduled pulls before returning: bailing out with + // goroutines still in flight would leak them past pull()'s return + return errors.Join(err, eg.Wait()) + } + if !pullRequired { s.events.On(api.Resource{ - ID: "Image " + service.Image, - Status: api.Done, - Text: "Skipped", + ID: "Image " + service.Image, + Status: api.Done, + Text: "Skipped", + Details: skipReason, }) continue - case types.PullPolicyMissing, types.PullPolicyIfNotPresent: - if imageAlreadyPresent(service.Image, images) { - s.events.On(api.Resource{ - ID: "Image " + service.Image, - Status: api.Done, - Text: "Skipped", - Details: "Image is already present locally", - }) - continue - } } if service.Build != nil && opts.IgnoreBuildable { @@ -137,25 +133,31 @@ func (s *composeService) pull(ctx context.Context, project *types.Project, opts // pre_start hook images run as ephemeral init containers with their own // registry image. They have no pull policy of their own, so we inherit the - // parent service's policy for skip decisions. Unlike the service image, a - // hook image can't be built, so `build` does not exempt it from pulling — - // only `never` does (consistent with the `up`/create path). + // parent service's policy for skip decisions — through the same + // shouldPullImage decision as the service image. Unlike the service + // image, a hook image can't be built, so `build` falls back to + // pull-if-missing instead of exempting it from pulling. for name, service := range project.Services { - if service.PullPolicy == types.PullPolicyNever { - continue + hookPolicy := service.PullPolicy + if hookPolicy == types.PullPolicyBuild { + hookPolicy = types.PullPolicyMissing } for _, img := range api.GetDependentImages(service, project.Name) { - switch service.PullPolicy { - case types.PullPolicyMissing, types.PullPolicyIfNotPresent, types.PullPolicyBuild: - if imageAlreadyPresent(img, images) { + pullRequired, skipReason, err := shouldPullImage(types.ServiceConfig{Name: name, Image: img, PullPolicy: hookPolicy}, images) + if err != nil { + // same as the service loop: never leave scheduled pulls unjoined + return errors.Join(err, eg.Wait()) + } + if !pullRequired { + if skipReason != "" { s.events.On(api.Resource{ ID: "Image " + img, Status: api.Done, Text: "Skipped", - Details: "Image is already present locally", + Details: skipReason, }) - continue } + continue } if _, ok := imagesBeingPulled[img]; ok { continue @@ -188,19 +190,49 @@ func (s *composeService) pull(ctx context.Context, project *types.Project, opts return errors.Join(pullErrors...) } -func imageAlreadyPresent(serviceImage string, localImages map[string]api.ImageSummary) bool { - normalizedImage, err := reference.ParseDockerRef(serviceImage) +// shouldPullImage decides whether `compose pull` refreshes a service's image, +// delegating to the exact pull_policy interpreter the up path uses (mustPull) +// so both commands honor never/build, skip-if-present and the +// daily/weekly/every_N refresh window identically. The command keeps two +// deliberate differences: +// - a service without an explicit pull_policy is always refreshed — an +// unset policy resolves to "missing" for `up`, but skipping it would turn +// an explicit `compose pull` into a no-op once images exist; +// - 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, no download, when the +// local image is already up to date. +func shouldPullImage(service types.ServiceConfig, images map[string]api.ImageSummary) (bool, string, error) { + if service.PullPolicy == "" { + return true, "", nil + } + pull, err := mustPull(service, images) + if err != nil || pull { + return pull, "", err + } + policy, _, _ := service.GetPullPolicy() + switch policy { + case types.PullPolicyRefresh: + return false, "Image is not due for refresh", nil + case types.PullPolicyMissing, types.PullPolicyIfNotPresent: + if isLatestTag(service.Image) { + return true, "", nil + } + return false, "Image is already present locally", nil + default: // never, build — and provider services short-circuited by mustPull + return false, "", nil + } +} + +// isLatestTag reports whether ref points at a `latest` tag, including bare +// references that normalize to it. +func isLatestTag(ref string) bool { + named, err := reference.ParseDockerRef(ref) if err != nil { return false } - switch refType := normalizedImage.(type) { - case reference.NamedTagged: - _, ok := localImages[serviceImage] - return ok && refType.Tag() != "latest" - default: - _, ok := localImages[serviceImage] - return ok - } + tagged, ok := named.(reference.Tagged) + return ok && tagged.Tag() == "latest" } func getUnwrappedErrorMessage(err error) string { @@ -286,7 +318,7 @@ func (s *composeService) pullServiceImage(ctx context.Context, service types.Ser } s.events.On(newEvent(resource, api.Done, api.StatusPulled)) - // Resolve the pulled image's identity exactly the way getImageSummaries + // Resolve the pulled image's identity exactly the way getLocalImagesDigests // does for already-local images: both values feed the // com.docker.compose.image label used to detect stale containers, so they // must be computed identically. Returning the raw inspect ID here (the diff --git a/pkg/compose/pull_test.go b/pkg/compose/pull_test.go index b6ace581d..7588ac46a 100644 --- a/pkg/compose/pull_test.go +++ b/pkg/compose/pull_test.go @@ -22,9 +22,9 @@ import ( "iter" "sort" "testing" + "time" "github.com/compose-spec/compose-go/v2/types" - "github.com/docker/cli/cli/config/configfile" "github.com/moby/moby/api/types/image" "github.com/moby/moby/api/types/jsonstream" "github.com/moby/moby/client" @@ -133,14 +133,7 @@ func (fakePullResponse) JSONMessages(context.Context) iter.Seq2[jsonstream.Messa func TestPullServiceImageUsesContentDigest(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - - mockAPI, cli := prepareMocks(mockCtrl) - cli.EXPECT().ConfigFile().Return(configfile.New("")).AnyTimes() - tested, err := NewComposeService(cli) - assert.NilError(t, err) - mockAPI.EXPECT().Ping(gomock.Any(), client.PingOptions{NegotiateAPIVersion: true}). - Return(client.PingResult{APIVersion: "1.48"}, nil).AnyTimes() - mockAPI.EXPECT().ClientVersion().Return("1.48").AnyTimes() + mockAPI, tested := newTestComposeService(t, mockCtrl, "1.48") ref := "foo:1@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" mockAPI.EXPECT(). @@ -157,8 +150,7 @@ func TestPullServiceImageUsesContentDigest(t *testing.T) { ImageInspect(anyCancellableContext(), ref, gomock.Any()). Return(client.ImageInspectResult{InspectResponse: inspect}, nil) - id, err := tested.(*composeService). - pullServiceImage(t.Context(), types.ServiceConfig{Name: "web", Image: ref}, true, "") + id, err := tested.pullServiceImage(t.Context(), types.ServiceConfig{Name: "web", Image: ref}, true, "") assert.NilError(t, err) assert.Equal(t, id, "sha256:image") } @@ -176,3 +168,70 @@ func TestAddPreStartHookPulls_DedupsSharedHookImage(t *testing.T) { assert.DeepEqual(t, scheduledHookImages(t, project, map[string]api.ImageSummary{}), []string{"init:latest"}) } + +func TestShouldPullImage(t *testing.T) { + present := map[string]api.ImageSummary{ + "web:1": {LastTagTime: time.Now()}, + "web:latest": {LastTagTime: time.Now()}, + "old:1": {LastTagTime: time.Now().Add(-48 * time.Hour)}, + } + svc := func(image, policy string) types.ServiceConfig { + return types.ServiceConfig{Name: "web", Image: image, PullPolicy: policy} + } + + t.Run("no explicit policy always refreshes", func(t *testing.T) { + pull, _, err := shouldPullImage(svc("web:1", ""), present) + assert.NilError(t, err) + assert.Assert(t, pull) + }) + + t.Run("never and build skip", func(t *testing.T) { + for _, policy := range []string{types.PullPolicyNever, types.PullPolicyBuild} { + pull, _, err := shouldPullImage(svc("web:1", policy), present) + assert.NilError(t, err) + assert.Assert(t, !pull) + } + }) + + t.Run("missing skips a present image", func(t *testing.T) { + pull, _, err := shouldPullImage(svc("web:1", types.PullPolicyMissing), present) + assert.NilError(t, err) + assert.Assert(t, !pull) + + pull, _, err = shouldPullImage(svc("absent:1", types.PullPolicyMissing), present) + assert.NilError(t, err) + assert.Assert(t, pull) + }) + + t.Run("missing still refreshes a present latest tag", func(t *testing.T) { + // deliberate exception: `latest` is expected to move, so the pull is + // triggered anyway and the daemon's registry negotiation decides + // (a no-op when the local image is already up to date) + for _, image := range []string{"web:latest", "web"} { + pull, _, err := shouldPullImage(svc(image, types.PullPolicyMissing), map[string]api.ImageSummary{ + image: {LastTagTime: time.Now()}, + }) + assert.NilError(t, err) + assert.Assert(t, pull, "present %s must still be refreshed", image) + } + }) + + t.Run("refresh policies honor the same window as up", func(t *testing.T) { + pull, _, err := shouldPullImage(svc("web:1", "daily"), present) + assert.NilError(t, err) + assert.Assert(t, !pull, "recently tagged image is not due for refresh") + + pull, _, err = shouldPullImage(svc("old:1", "daily"), present) + assert.NilError(t, err) + assert.Assert(t, pull, "image older than the window must be refreshed") + + pull, _, err = shouldPullImage(svc("absent:1", "weekly"), present) + assert.NilError(t, err) + assert.Assert(t, pull, "absent image must be pulled") + }) + + t.Run("invalid refresh spec errors", func(t *testing.T) { + _, _, err := shouldPullImage(svc("web:1", "every_bogus"), present) + assert.Assert(t, err != nil) + }) +}