introduce pull --policy flag to only pull images not present in cache #10981

ndeloof · 2023-09-07T14:38:48Z

What I did
introduce --missing flag on docker compose pull to only pull images not present in cache.
Note: "latest" is always pulled

Related issue
closes #10977

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

codecov · 2023-09-07T14:54:18Z

Codecov Report

Patch coverage: 76.19% and project coverage change: +0.09% 🎉

Comparison is base (e19232e) 57.88% compared to head (4e9ff18) 57.98%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10981      +/-   ##
==========================================
+ Coverage   57.88%   57.98%   +0.09%     
==========================================
  Files         129      129              
  Lines       11160    11121      -39     
==========================================
- Hits         6460     6448      -12     
+ Misses       4063     4041      -22     
+ Partials      637      632       -5

Files Changed	Coverage Δ
cmd/compose/pull.go	`77.35% <70.58%> (-0.15%)`	⬇️
cmd/compose/create.go	`60.00% <100.00%> (ø)`
cmd/compose/up.go	`67.40% <100.00%> (ø)`

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thaJeztah · 2023-09-07T15:02:48Z

Note: "latest" is always pulled

Is there a reason for that?

Overall, I'm still a bit curious about the general issue that led to opening #10977, and if it's an isolated event / one specific scenario that doesn't affect the wider group of users.

docker compose up should already be able to pull what's needed (based on pull policy defined), so for most (if not all) scenarios "it should just work".

To some extent, I would expect docker compose pull (without arguments?) to follow the pull-policy that's defined in the compose-file, and a --force option to force it to download / refresh all images (even if present locally)

ndeloof · 2023-09-07T15:54:49Z

@thaJeztah afaict this is inherited from docker compose v1
docker compose pull applies the pull policy, this flags just allows to globally override

thaJeztah · 2023-09-07T16:07:50Z

compose v1 of course didn't have pull-policies, so I'm a bit curious what the reporter's compose-file looks like;

if the compose file has missing as pull policy, then docker compose --parallel=1 pull would pull duplicate images once (first service using the image would pull it, second one would (should) see it's there, so skip pulling)
^^ without --parallel=1, ideally, compose would still de-duplicate images (2 services using the same image would not be doing two separate pulls)

I'd like to understand the cause of the issue; perhaps we're adding a new feature, whereas there's s bug somewhere, and adding a --missing, would lead to adding an --always and a --never and --unless-built (and there's already a --ignore-pull-failures and .... an explosion of new options

ndeloof · 2023-09-07T19:08:11Z

we also could use --policy=missing so that we don't need additional flags

FFdhorkin · 2023-09-07T19:51:39Z

What I did introduce --missing flag on docker compose pull to only pull images not present in cache. Note: "latest" is always pulled

Wow! That was fast

@thaJeztah afaict this is inherited from docker compose v1 docker compose pull applies the pull policy, this flags just allows to globally override

sounds like there must be a bug, then.

compose v1 of course didn't have pull-policies, so I'm a bit curious what the reporter's compose-file looks like;

* if the compose file has `missing` as pull policy, then `docker compose --parallel=1 pull` would pull duplicate images once (first service using the image would pull it, second one would (should) see it's there, so skip pulling)

* ^^ without `--parallel=1`, ideally, compose would still de-duplicate images (2 services using the same image would not be doing two separate pulls)

I replied on the original issue, but the docs say pull_policy defaults to missing. I don't have any explicit pull_policy set.

I'd like to understand the cause of the issue; perhaps we're adding a new feature, whereas there's s bug somewhere, and adding a --missing, would lead to adding an --always and a --never and --unless-built (and there's already a --ignore-pull-failures and .... an explosion of new options

Yeah, if the intended behavior is to respect pull_policy, I don't really see much reason to add a --missing option, as opposed to figuring out why it either isn't respecting the pull_policy or the defaults aren't being applied, etc.

we also could use --policy=missing so that we don't need additional flags

That seems a lot more useful, IMHO, than --missing, since it'd be much more flexible and not need the feature creep that adding comprehensive flags would

milas

No real concerns with the code, but I think there's more to think through here around pull_policy: build.

Also...I'd love some form of tests here as a result, even if it's for the createOptions::apply so that it can be a cheap unit test to cover some of the different possibilities

milas · 2023-09-11T13:44:12Z

cmd/compose/pull.go

@@ -83,6 +85,13 @@ func runPull(ctx context.Context, dockerCli command.Cli, backend api.Service, op
 		}
 	}

+	if opts.policy != "" {
+		for i, service := range project.Services {
+			service.PullPolicy = opts.policy


I think this should ignore services that either have pull_policy: build already set in YAML or have a <svc>.build section and no <svc>.image section.

e.g. imagine this project

services: explicit-build: image: registry.example.com/myservice pull_policy: build build: . no-image: build: . nginx: image: nginx

Without args:

❯ docker compose pull [+] Pulling 3/3 ✔ explicit-build Skipped 0.0s ✔ no-image Skipped - No image to be pulled 0.0s ✔ nginx Pulled

With --policy=missing (or --policy=always):

❯ ~/dev/compose/bin/build/docker-compose pull --policy=missing [+] Pulling 3/3 ✔ no-image Skipped - No image to be pulled 0.0s ✔ nginx Pulled 0.5s ! explicit-build Warning 0.2s WARNING: Some service image(s) must be built from source by running: docker compose build explicit-build 1 error occurred: * Error response from daemon: Get "https://registry.example.com/v2/": dialing registry.example.com:443 with direct connection: resolving host registry.example.com: lookup registry.example.com: no such host

So it is properly skipping the one without <svc>.image, but is unusable if you have any pull_policy: build services that do specify an image name.

Agree about service without an image MUST be excluded
disagree on pull_policy: build: we should only require a local build if image is not available on registry under configured image

milas · 2023-09-11T13:50:54Z

cmd/compose/up.go

-	flags.BoolVar(&create.noBuild, "no-build", false, "Don't build an image, even if it's missing.")
-	flags.StringVar(&create.Pull, "pull", "missing", `Pull image before running ("always"|"missing"|"never")`)
+	flags.BoolVar(&create.noBuild, "no-build", false, "Don't build an image, even if it's policy.")
+	flags.StringVar(&create.Pull, "pull", "policy", `Pull image before running ("always"|"policy"|"never")`)


Hmm...this (also) interferes with pull_policy: build...which apparently was already the case, but I think if we're making changes here we should fix it in the process.

e.g. for my same example from before, up --pull=always will prevent services with a pull_policy: build from being built. In practice, I would expect that would behave more like a docker build --pull ., i.e. ensure that it fetches the latest base images before doing the build.

that's not the expected behavior. When compose file has both image and build attributes set, first it tries to pull the image (so you benefit from already built image) then it will build from local resources. Si this isn't the same as docker build --pull (while confusing)

We should look at that by the way, because if build uses a custom builder (which may be remote), we would now potentially be;

pulling an image locally

run a build (which won't use the image we pulled)

load the image from the builder (replacing the image we just pulled)

docker compose build --pull just pass the pull flag to the builder, same as docker build --pull. Using a remote builder has no impact here

milas · 2023-09-11T13:57:00Z

cmd/compose/create.go

 	flags.BoolVar(&opts.Build, "build", false, "Build images before starting containers.")
-	flags.BoolVar(&opts.noBuild, "no-build", false, "Don't build an image, even if it's missing.")
-	flags.StringVar(&opts.Pull, "pull", "missing", `Pull image before running ("always"|"missing"|"never")`)
+	flags.BoolVar(&opts.noBuild, "no-build", false, "Don't build an image, even if it's policy.")
+	flags.StringVar(&opts.Pull, "pull", "policy", `Pull image before running ("always"|"policy"|"never")`)


We have these args/descriptions across several commands, and I think we need to improve the copy here, especially since we're making changes, as they're pretty vague and unclear as-is.

cc @aevesdocker

My understanding of desired behaviors:

--build: Re-build images for all services being launched that have a build section. Images with pull_policy: build are always re-built unless --no-build is specified.

--no-build: Don't build ANY images, which might mean the project can't start if local versions are not already available in the engine.

--policy

always

for services with build section, fetch the latest base image (FROM), similar to docker build --pull .

for services without a build section, fetch the image even if it's already present locally (e.g. useful for :latest)

never

for services with a build section, no-op (AFAIK there's no builder option to say "don't automatically pull missing base images" nor would it make much sense)

for services without a build section, don't pull the image even if it doesn't exist in the engine, which might mean the project can't launch.

always for services with build section, fetch the latest base image (FROM),

Nope, fetch the configured tagged image, which is the result of the build. We don't parse the Dockerfile to know about the base image. No relation with docker build --pull

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

thaJeztah · 2023-09-15T10:30:56Z

How was this merged with pending questions, and force-pushes after the last review? (Note that the commit message and PR title also don't match the implementation.

ndeloof · 2023-09-15T10:51:04Z

@thaJeztah AFAICT all comments have been addressed, did I missed something
force push is used as we prefer rebase over merge commits

thaJeztah · 2023-09-19T11:41:17Z

force push is used as we prefer rebase over merge commits

I'm familiar with rebase over merge commits. (although not sure if a rebase is always needed (of course in case there's a conflict)); if the concern is that merging an outdated branch could in some cases make the main branch fail, then perhaps the merge-queue feature would work.

Rebases are ok (if they're needed), but at least the last force push mad actual code changes, and those changes were not reviewed, and were not called out;
https://github.com/docker/compose/compare/0c2a6c8ce74644a746e46299b4748fe4b41923f6..4e9ff18d62fbf6fe538a8ea61c7db283894fbfce

Those changes look fine, but I'm not a fan of doing so if not needed (force-push and self-merge), especially not if it's not a critical PR to get merged. If such a fix gets included in a force-push-and-merge, what's keeping me from including other changes? (without calling them out or having them reviewed).

AFAICT all comments have been addressed, did I missed something

There were still questions pending, and your answers were not acknowledged by the persons asking them.

My other issue is that we picked "low-hanging-fruit" here, without fully triaging the request. This PR was created for a single report for a single user, and after further questioning may have been a valid bug report, written as a "feature request", as acknowledged by the original reporter; #10981 (comment)

the original issue reported something what looks like a potential bug
but did so in the form of an "xy problem" (https://xyproblem.info)
- something didn't work (the bug)
- I tried X to work around the problem (the bug)
- But X didn't work, so can you add Y to make my workaround X for a bug work?

And what we did was "implement Y to make the workaround for a bug work".

It was not clear yet if there was an actual need for the new flag, and we didn't fix the bug, only more flags to workaround a bug.

and remove deprecated options See docker/compose#10981

ndeloof force-pushed the pull-missing branch from 016399f to 8398ca0 Compare September 7, 2023 15:57

thaJeztah mentioned this pull request Sep 7, 2023

docker compose pull has no options for pull policy #10977

Closed

ndeloof force-pushed the pull-missing branch from 8398ca0 to 2067a17 Compare September 8, 2023 09:52

glours approved these changes Sep 8, 2023

View reviewed changes

ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, milas and laurazard and removed request for a team September 11, 2023 06:51

milas reviewed Sep 11, 2023

View reviewed changes

ndeloof force-pushed the pull-missing branch 2 times, most recently from f95281a to 0c2a6c8 Compare September 14, 2023 14:04

introduce pull --missing flag to only pull images not present in cache

4e9ff18

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof force-pushed the pull-missing branch from 0c2a6c8 to 4e9ff18 Compare September 14, 2023 14:05

ndeloof merged commit a697a06 into docker:main Sep 15, 2023
25 checks passed

ndeloof changed the title ~~introduce pull --missing flag to only pull images not present in cache~~ introduce pull --policy flag to only pull images not present in cache Sep 15, 2023

ndeloof deleted the pull-missing branch September 15, 2023 10:50

dnhn mentioned this pull request Sep 21, 2023

docker-compose 2.22.0 Homebrew/homebrew-core#144591

Merged

chenrui333 mentioned this pull request Sep 22, 2023

docker-compose 2.22.0 Homebrew/homebrew-core#144705

Closed

liamjones mentioned this pull request Oct 18, 2023

[BUG] --pull policy flag does not complain about invalid values and silently swallows subsequent options #11104

Closed

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Apr 8, 2024

Compose 2.26.1: new pull --policy

7888a04

and remove deprecated options See docker/compose#10981

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce pull --policy flag to only pull images not present in cache #10981

introduce pull --policy flag to only pull images not present in cache #10981

ndeloof commented Sep 7, 2023

codecov bot commented Sep 7, 2023 •

edited

Loading

thaJeztah commented Sep 7, 2023 •

edited

Loading

ndeloof commented Sep 7, 2023

thaJeztah commented Sep 7, 2023

ndeloof commented Sep 7, 2023

FFdhorkin commented Sep 7, 2023 •

edited

Loading

milas left a comment

milas Sep 11, 2023

ndeloof Sep 14, 2023

milas Sep 11, 2023

ndeloof Sep 14, 2023

thaJeztah Sep 19, 2023

ndeloof Sep 19, 2023 •

edited

Loading

milas Sep 11, 2023

ndeloof Sep 14, 2023

thaJeztah commented Sep 15, 2023

ndeloof commented Sep 15, 2023

thaJeztah commented Sep 19, 2023

introduce pull --policy flag to only pull images not present in cache #10981

introduce pull --policy flag to only pull images not present in cache #10981

Conversation

ndeloof commented Sep 7, 2023

codecov bot commented Sep 7, 2023 • edited Loading

Codecov Report

thaJeztah commented Sep 7, 2023 • edited Loading

ndeloof commented Sep 7, 2023

thaJeztah commented Sep 7, 2023

ndeloof commented Sep 7, 2023

FFdhorkin commented Sep 7, 2023 • edited Loading

milas left a comment

Choose a reason for hiding this comment

milas Sep 11, 2023

Choose a reason for hiding this comment

ndeloof Sep 14, 2023

Choose a reason for hiding this comment

milas Sep 11, 2023

Choose a reason for hiding this comment

ndeloof Sep 14, 2023

Choose a reason for hiding this comment

thaJeztah Sep 19, 2023

Choose a reason for hiding this comment

ndeloof Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

milas Sep 11, 2023

Choose a reason for hiding this comment

ndeloof Sep 14, 2023

Choose a reason for hiding this comment

thaJeztah commented Sep 15, 2023

ndeloof commented Sep 15, 2023

thaJeztah commented Sep 19, 2023

codecov bot commented Sep 7, 2023 •

edited

Loading

thaJeztah commented Sep 7, 2023 •

edited

Loading

FFdhorkin commented Sep 7, 2023 •

edited

Loading

ndeloof Sep 19, 2023 •

edited

Loading