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

Jump to bottom

Avoid pulling locally cached "infrastructure" (pause) image unless :latest tag is specified. Applies the same logic used for tasks to this image. #11380

Closed
wants to merge 3 commits into from
Closed

Avoid pulling locally cached "infrastructure" (pause) image unless :latest tag is specified. Applies the same logic used for tasks to this image. #11380

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b6f6a2
Avoid pulling locally cached "infrastructure" (pause) image unless :l…
phreakocious Oct 23, 2021
6199d79
update website documentation to reflect the new behavior introduced b…
phreakocious Oct 28, 2021
b765156
Update website/content/docs/drivers/docker.mdx
phreakocious Dec 8, 2021
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
26 changes: 15 additions & 11 deletions drivers/docker/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,21 @@ func (d *Driver) CreateNetwork(allocID string, createSpec *drivers.NetworkCreate
return nil, false, fmt.Errorf("failed to connect to docker daemon: %s", err)
}

repo, _ := parseDockerImage(d.config.InfraImage)
authOptions, err := firstValidAuth(repo, []authBackend{
authFromDockerConfig(d.config.Auth.Config),
authFromHelper(d.config.Auth.Helper),
})
if err != nil {
d.logger.Debug("auth failed for infra container image pull", "image", d.config.InfraImage, "error", err)
}
_, err = d.coordinator.PullImage(d.config.InfraImage, authOptions, allocID, noopLogEventFn, d.config.infraImagePullTimeoutDuration, d.config.pullActivityTimeoutDuration)
if err != nil {
return nil, false, err
repo, tag := parseDockerImage(d.config.InfraImage)

// Pull infra_image if it doesn't already exist locally or tag 'latest' is specified
if img, _ := client.InspectImage(d.config.InfraImage); img == nil || tag == "latest" {
Copy link
Member

Choose a reason for hiding this comment

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

Let's short-circuit the conditional by checking tag == "latest" first so that we don't make a Docker API call if we don't have to.

I'm not wild about throwing away the error here either but it looks like (*Driver).pullImage does it too... I guess if the inspect fails we always have a nil image and will try to pull, so any persistent errors talking to Docker will show up there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm relatively new to Go and not immediately seeing a way to reverse the order of the if without moving its body to a new function. There is no way to move the assignment statement to the right of the || in that syntax. Is a new function what you'd suggest?

Copy link
Member

Choose a reason for hiding this comment

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

I've sketched up https://play.golang.com/p/1IPLHG1NcwZ as an example. We reverse the sense of the conditionals and return early. So check tag != "latest", then check img != nil and bail out if both are met. Then the fallthrough becomes where we pull the image.

authOptions, err := firstValidAuth(repo, []authBackend{
authFromDockerConfig(d.config.Auth.Config),
authFromHelper(d.config.Auth.Helper),
})
if err != nil {
d.logger.Debug("auth failed for infra container image pull", "image", d.config.InfraImage, "error", err)
}
tgross marked this conversation as resolved.
Show resolved Hide resolved
_, err = d.coordinator.PullImage(d.config.InfraImage, authOptions, allocID, noopLogEventFn, d.config.infraImagePullTimeoutDuration, d.config.pullActivityTimeoutDuration)
if err != nil {
return nil, false, err
}
}

config, err := d.createSandboxContainerConfig(allocID, createSpec)
Expand Down
3 changes: 2 additions & 1 deletion website/content/docs/drivers/docker.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,8 @@ host system.

- `infra_image` - This is the Docker image to use when creating the parent
container necessary when sharing network namespaces between tasks. Defaults to
`gcr.io/google_containers/pause-<goarch>:3.1`.
`gcr.io/google_containers/pause-<goarch>:3.1`. If the image is already present,
phreakocious marked this conversation as resolved.
Show resolved Hide resolved
it won't be pulled again unless the `"latest"` tag is specified.

- `infra_image_pull_timeout` - A time duration that controls how long Nomad will
wait before cancelling an in-progress pull of the Docker image as specified in
Expand Down