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#10981ndeloof merged 1 commit intodocker:mainfrom
Conversation
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
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.
To some extent, I would expect |
|
@thaJeztah afaict this is inherited from docker compose v1 |
ndeloof
force-pushed
the
pull-missing
branch
from
016399f to
8398ca0
Compare
|
compose v1 of course didn't have pull-policies, so I'm a bit curious what the reporter's compose-file looks like;
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 |
|
we also could use |
Wow! That was fast
sounds like there must be a bug, then.
I replied on the original issue, but the docs say
Yeah, if the intended behavior is to respect
That seems a lot more useful, IMHO, than |
ndeloof
force-pushed
the
pull-missing
branch
from
8398ca0 to
2067a17
Compare
ndeloof
requested review from
a team,
StefanScherer,
laurazard,
milas,
nicksieger and
ulyssessouza
and removed request for
a team
milas
left a comment
milas
left a comment
There was a problem hiding this comment.
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
|
|
||
| if opts.policy != "" { | ||
| for i, service := range project.Services { | ||
| service.PullPolicy = opts.policy |
There was a problem hiding this comment.
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: nginxWithout 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.
There was a problem hiding this comment.
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
| 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")`) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
| 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")`) |
There was a problem hiding this comment.
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 abuildsection. Images withpull_policy: buildare always re-built unless--no-buildis 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.--policyalways- for services with
buildsection, fetch the latest base image (FROM), similar todocker build --pull . - for services without a
buildsection, fetch theimageeven if it's already present locally (e.g. useful for:latest)
- for services with
never- for services with a
buildsection, 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
buildsection, don't pull theimageeven if it doesn't exist in the engine, which might mean the project can't launch.
- for services with a
There was a problem hiding this comment.
alwaysfor 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
ndeloof
force-pushed
the
pull-missing
branch
2 times, most recently
from
f95281a to
0c2a6c8
Compare
Signed-off-by: Nicolas De Loof <[email protected]>
ndeloof
force-pushed
the
pull-missing
branch
from
0c2a6c8 to
4e9ff18
Compare
|
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
changed the title
|
@thaJeztah AFAICT all comments have been addressed, did I missed something |
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; 
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).
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)
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
5 participants
What I did
introduce
--missingflag ondocker compose pullto 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