Skip to content

With Buildkit, don't pull docker images needlessly. #360

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

With Buildkit, don't pull docker images needlessly. #360

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dadaaca
With Buildkit, don't pull docker images needlessly.
neil-ach Apr 7, 2022
e25cde0
Extract some logic into a helper function
neil-ach Jun 10, 2022
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
17 changes: 14 additions & 3 deletions docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,20 @@ func (p Plugin) Exec() error {
cmds = append(cmds, commandVersion()) // docker version
cmds = append(cmds, commandInfo()) // docker info

// pre-pull cache images
for _, img := range p.Build.CacheFrom {
cmds = append(cmds, commandPull(img))
var inlineCache = false
for _, v := range p.Build.Args {
if v == "BUILDKIT_INLINE_CACHE=1" {
inlineCache = true
break
}
}
if os.Getenv("DOCKER_BUILDKIT") != "1" || !inlineCache {
Copy link
Member

@bradrydzewski bradrydzewski Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

could we remove the above for loop, and instead move this logic into a helper function? I'm having a bit of trouble understanding the if statement

if useCacheFrom(p) {
  ...
} else {
  ...
}
func useCacheFrom(p ...) bool {
	// if buildkit inline cache, return true
	for _, v := range p.Build.Args {
		if v == "BUILDKIT_INLINE_CACHE=1" {
			return true
		}
	}
	// else if buildkit, return false
	if os.Getenv("DOCKER_BUILDKIT") == "1" {
		return false
	}
	// else if not buildkit, return true
	return true
}

also might be good to comment the logic so folks unfamiliar with buildkit can follow the code. thanks!!

Copy link
Author

@neil-ach neil-ach Jun 10, 2022

Choose a reason for hiding this comment

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

Buildkit (when available and enabled) uses the inline cache information to pull only the relevant subset of cache, which is in general more efficient than unconditionally pulling the entire image in the hopes that some of it will trigger cache hits.

For the above to work, we need Buildkit enabled (DOCKER_BUILDKIT=1) AND Buildkit inline cache enabled (BUILDKIT_INLINE_CACHE=1) AND cache-from set to a valid image (with inline cache metadata present).

However, right now if this plugin sees the cache-from argument set, it will add the pre-pull of the image step, and that makes the more efficient Buildkit inline cache useless, because all of the cache is already fetched, useful or not. This is the part that we want to change. We never want to remove the cache-from directive if it was passed, we want to skip the image pre-pull when we know that it will be counterproductive (the above outlined scenario).

I don't mind adding some more documentation, although this PR combined with the commit message should serve as pretty in depth documentation at this point.

Thanks.

Copy link
Member

@bradrydzewski bradrydzewski Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the explanation. let's split the code into a helper function similar to the pseudo-code in my comment, with any changes that you feel are needed to make it accurate, and then we can get this merged

// pre-pull cache images for non-buildkit
for _, img := range p.Build.CacheFrom {
cmds = append(cmds, commandPull(img))
}
} else {
fmt.Println("Detected Buildkit with inline cache, will not pre-pull cache image.")
}

cmds = append(cmds, commandBuild(p.Build)) // docker build
Expand Down