Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 15, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmd/compose/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ func createCommand(p *ProjectOptions, dockerCli command.Cli, backend api.Service
}
flags := cmd.Flags()
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")`)
Comment on lines 74 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

flags.BoolVar(&opts.forceRecreate, "force-recreate", false, "Recreate containers even if their configuration and image haven't changed.")
flags.BoolVar(&opts.noRecreate, "no-recreate", false, "If containers already exist, don't recreate them. Incompatible with --force-recreate.")
flags.BoolVar(&opts.removeOrphans, "remove-orphans", false, "Remove containers for services not defined in the Compose file.")
Expand Down Expand Up @@ -145,7 +145,7 @@ func (opts createOptions) Apply(project *types.Project) error {
}
// N.B. opts.Build means "force build all", but images can still be built
// when this is false
// e.g. if a service has pull_policy: build or its local image is missing
// e.g. if a service has pull_policy: build or its local image is policy
if opts.Build {
for i, service := range project.Services {
if service.Build == nil {
Expand Down
30 changes: 25 additions & 5 deletions cmd/compose/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
includeDeps bool
ignorePullFailures bool
noBuildable bool
policy string
}

func pullCommand(p *ProjectOptions, dockerCli command.Cli, backend api.Service) *cobra.Command {
Expand Down Expand Up @@ -67,20 +68,39 @@
flags.MarkHidden("no-parallel") //nolint:errcheck
cmd.Flags().BoolVar(&opts.ignorePullFailures, "ignore-pull-failures", false, "Pull what it can and ignores images with pull failures.")
cmd.Flags().BoolVar(&opts.noBuildable, "ignore-buildable", false, "Ignore images that can be built.")
cmd.Flags().StringVar(&opts.policy, "policy", "", `Apply pull policy ("missing"|"always").`)
return cmd
}

func (opts pullOptions) apply(project *types.Project, services []string) error {
if !opts.includeDeps {
err := project.ForServices(services, types.IgnoreDependencies)
if err != nil {
return err
}

Check warning on line 80 in cmd/compose/pull.go

View check run for this annotation

Codecov / codecov/patch

cmd/compose/pull.go#L79-L80 

Added lines #L79 - L80 were not covered by tests
}

if opts.policy != "" {
for i, service := range project.Services {
if service.Image == "" {
continue
}
service.PullPolicy = opts.policy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

project.Services[i] = service
}
}
return nil
}

func runPull(ctx context.Context, dockerCli command.Cli, backend api.Service, opts pullOptions, services []string) error {
project, err := opts.ToProject(dockerCli, services)
if err != nil {
return err
}

if !opts.includeDeps {
err := project.ForServices(services, types.IgnoreDependencies)
if err != nil {
return err
}
err = opts.apply(project, services)
if err != nil {
return err

Check warning on line 103 in cmd/compose/pull.go

View check run for this annotation

Codecov / codecov/patch

cmd/compose/pull.go#L103 

Added line #L103 was not covered by tests
}

return backend.Pull(ctx, project, api.PullOptions{
Expand Down
57 changes: 57 additions & 0 deletions cmd/compose/pullOptions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
Copyright 2023 Docker Compose CLI authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package compose

import (
"testing"

"github.com/compose-spec/compose-go/types"
"gotest.tools/v3/assert"
)

func TestApplyPullOptions(t *testing.T) {
project := &types.Project{
Services: []types.ServiceConfig{
{
Name: "must-build",
// No image, local build only
Build: &types.BuildConfig{
Context: ".",
},
},
{
Name: "has-build",
Image: "registry.example.com/myservice",
Build: &types.BuildConfig{
Context: ".",
},
},
{
Name: "must-pull",
Image: "registry.example.com/another-service",
},
},
}
err := pullOptions{
policy: types.PullPolicyMissing,
}.apply(project, nil)
assert.NilError(t, err)

assert.Equal(t, project.Services[0].PullPolicy, "") // still default
assert.Equal(t, project.Services[1].PullPolicy, types.PullPolicyMissing)
assert.Equal(t, project.Services[2].PullPolicy, types.PullPolicyMissing)
}
4 changes: 2 additions & 2 deletions cmd/compose/up.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ func upCommand(p *ProjectOptions, dockerCli command.Cli, backend api.Service) *c
flags := upCmd.Flags()
flags.BoolVarP(&up.Detach, "detach", "d", false, "Detached mode: Run containers in the background")
flags.BoolVar(&create.Build, "build", false, "Build images before starting containers.")
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")`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@ndeloof ndeloof Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(&create.removeOrphans, "remove-orphans", false, "Remove containers for services not defined in the Compose file.")
flags.StringArrayVar(&create.scale, "scale", []string{}, "Scale SERVICE to NUM instances. Overrides the `scale` setting in the Compose file if present.")
flags.BoolVar(&up.noColor, "no-color", false, "Produce monochrome output.")
Expand Down
20 changes: 10 additions & 10 deletions docs/reference/compose_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@ Creates containers for a service.

### Options

| Name | Type | Default | Description |
|:-------------------|:--------------|:----------|:----------------------------------------------------------------------------------------------|
| `--build` | | | Build images before starting containers. |
| `--dry-run` | | | Execute command in dry run mode |
| `--force-recreate` | | | Recreate containers even if their configuration and image haven't changed. |
| `--no-build` | | | Don't build an image, even if it's missing. |
| `--no-recreate` | | | If containers already exist, don't recreate them. Incompatible with --force-recreate. |
| `--pull` | `string` | `missing` | Pull image before running ("always"\|"missing"\|"never") |
| `--remove-orphans` | | | Remove containers for services not defined in the Compose file. |
| `--scale` | `stringArray` | | Scale SERVICE to NUM instances. Overrides the `scale` setting in the Compose file if present. |
| Name | Type | Default | Description |
|:-------------------|:--------------|:---------|:----------------------------------------------------------------------------------------------|
| `--build` | | | Build images before starting containers. |
| `--dry-run` | | | Execute command in dry run mode |
| `--force-recreate` | | | Recreate containers even if their configuration and image haven't changed. |
| `--no-build` | | | Don't build an image, even if it's policy. |
| `--no-recreate` | | | If containers already exist, don't recreate them. Incompatible with --force-recreate. |
| `--pull` | `string` | `policy` | Pull image before running ("always"\|"policy"\|"never") |
| `--remove-orphans` | | | Remove containers for services not defined in the Compose file. |
| `--scale` | `stringArray` | | Scale SERVICE to NUM instances. Overrides the `scale` setting in the Compose file if present. |


<!---MARKER_GEN_END-->
Expand Down
15 changes: 8 additions & 7 deletions docs/reference/compose_pull.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ Pull service images

### Options

| Name | Type | Default | Description |
|:-------------------------|:-----|:--------|:--------------------------------------------------------|
| `--dry-run` | | | Execute command in dry run mode |
| `--ignore-buildable` | | | Ignore images that can be built. |
| `--ignore-pull-failures` | | | Pull what it can and ignores images with pull failures. |
| `--include-deps` | | | Also pull services declared as dependencies. |
| `-q`, `--quiet` | | | Pull without printing progress information. |
| Name | Type | Default | Description |
|:-------------------------|:---------|:--------|:--------------------------------------------------------|
| `--dry-run` | | | Execute command in dry run mode |
| `--ignore-buildable` | | | Ignore images that can be built. |
| `--ignore-pull-failures` | | | Pull what it can and ignores images with pull failures. |
| `--include-deps` | | | Also pull services declared as dependencies. |
| `--policy` | `string` | | Apply pull policy ("missing"\|"always"). |
| `-q`, `--quiet` | | | Pull without printing progress information. |


<!---MARKER_GEN_END-->
Expand Down
54 changes: 27 additions & 27 deletions docs/reference/compose_up.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,33 @@ Create and start containers

### Options

| Name | Type | Default | Description |
|:-----------------------------|:--------------|:----------|:---------------------------------------------------------------------------------------------------------|
| `--abort-on-container-exit` | | | Stops all containers if any container was stopped. Incompatible with -d |
| `--always-recreate-deps` | | | Recreate dependent containers. Incompatible with --no-recreate. |
| `--attach` | `stringArray` | | Restrict attaching to the specified services. Incompatible with --attach-dependencies. |
| `--attach-dependencies` | | | Automatically attach to log output of dependent services. |
| `--build` | | | Build images before starting containers. |
| `-d`, `--detach` | | | Detached mode: Run containers in the background |
| `--dry-run` | | | Execute command in dry run mode |
| `--exit-code-from` | `string` | | Return the exit code of the selected service container. Implies --abort-on-container-exit |
| `--force-recreate` | | | Recreate containers even if their configuration and image haven't changed. |
| `--no-attach` | `stringArray` | | Do not attach (stream logs) to the specified services. |
| `--no-build` | | | Don't build an image, even if it's missing. |
| `--no-color` | | | Produce monochrome output. |
| `--no-deps` | | | Don't start linked services. |
| `--no-log-prefix` | | | Don't print prefix in logs. |
| `--no-recreate` | | | If containers already exist, don't recreate them. Incompatible with --force-recreate. |
| `--no-start` | | | Don't start the services after creating them. |
| `--pull` | `string` | `missing` | Pull image before running ("always"\|"missing"\|"never") |
| `--quiet-pull` | | | Pull without printing progress information. |
| `--remove-orphans` | | | Remove containers for services not defined in the Compose file. |
| `-V`, `--renew-anon-volumes` | | | Recreate anonymous volumes instead of retrieving data from the previous containers. |
| `--scale` | `stringArray` | | Scale SERVICE to NUM instances. Overrides the `scale` setting in the Compose file if present. |
| `-t`, `--timeout` | `int` | `0` | Use this timeout in seconds for container shutdown when attached or when containers are already running. |
| `--timestamps` | | | Show timestamps. |
| `--wait` | | | Wait for services to be running\|healthy. Implies detached mode. |
| `--wait-timeout` | `int` | `0` | Maximum duration to wait for the project to be running\|healthy. |
| Name | Type | Default | Description |
|:-----------------------------|:--------------|:---------|:---------------------------------------------------------------------------------------------------------|
| `--abort-on-container-exit` | | | Stops all containers if any container was stopped. Incompatible with -d |
| `--always-recreate-deps` | | | Recreate dependent containers. Incompatible with --no-recreate. |
| `--attach` | `stringArray` | | Restrict attaching to the specified services. Incompatible with --attach-dependencies. |
| `--attach-dependencies` | | | Automatically attach to log output of dependent services. |
| `--build` | | | Build images before starting containers. |
| `-d`, `--detach` | | | Detached mode: Run containers in the background |
| `--dry-run` | | | Execute command in dry run mode |
| `--exit-code-from` | `string` | | Return the exit code of the selected service container. Implies --abort-on-container-exit |
| `--force-recreate` | | | Recreate containers even if their configuration and image haven't changed. |
| `--no-attach` | `stringArray` | | Do not attach (stream logs) to the specified services. |
| `--no-build` | | | Don't build an image, even if it's policy. |
| `--no-color` | | | Produce monochrome output. |
| `--no-deps` | | | Don't start linked services. |
| `--no-log-prefix` | | | Don't print prefix in logs. |
| `--no-recreate` | | | If containers already exist, don't recreate them. Incompatible with --force-recreate. |
| `--no-start` | | | Don't start the services after creating them. |
| `--pull` | `string` | `policy` | Pull image before running ("always"\|"policy"\|"never") |
| `--quiet-pull` | | | Pull without printing progress information. |
| `--remove-orphans` | | | Remove containers for services not defined in the Compose file. |
| `-V`, `--renew-anon-volumes` | | | Recreate anonymous volumes instead of retrieving data from the previous containers. |
| `--scale` | `stringArray` | | Scale SERVICE to NUM instances. Overrides the `scale` setting in the Compose file if present. |
| `-t`, `--timeout` | `int` | `0` | Use this timeout in seconds for container shutdown when attached or when containers are already running. |
| `--timestamps` | | | Show timestamps. |
| `--wait` | | | Wait for services to be running\|healthy. Implies detached mode. |
| `--wait-timeout` | `int` | `0` | Maximum duration to wait for the project to be running\|healthy. |


<!---MARKER_GEN_END-->
Expand Down
6 changes: 3 additions & 3 deletions docs/reference/docker_compose_create.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ options:
- option: no-build
value_type: bool
default_value: "false"
description: Don't build an image, even if it's missing.
description: Don't build an image, even if it's policy.
deprecated: false
hidden: false
experimental: false
Expand All @@ -49,8 +49,8 @@ options:
swarm: false
- option: pull
value_type: string
default_value: missing
description: Pull image before running ("always"|"missing"|"never")
default_value: policy
description: Pull image before running ("always"|"policy"|"never")
deprecated: false
hidden: false
experimental: false
Expand Down
Loading