-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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.
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
Implement build cache based on history array #26839
Conversation
Is this not going to be plugin based like #24711 (comment) ? |
@graingert If you look at the contents of #24711 it is not about the plugin method. This was changed if favor of #26065 proposal a long time. |
ba1feda
to
d3222ea
Compare
I think design was already approved, so I'm tentatively moving to code review. Thanks for the carry @tonistiigi! |
var cacheFrom = []string{} | ||
cacheFromJSON := r.FormValue("cachefrom") | ||
if cacheFromJSON != "" { | ||
if err := json.NewDecoder(strings.NewReader(cacheFromJSON)).Decode(&cacheFrom); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json.Unmarshal(cacheFromJSON, &cacheFrom)
d3222ea
to
0a43b1d
Compare
LGTM after the small nit. |
return false | ||
} | ||
if parent == nil || len(parent.History) == 0 && len(parent.RootFS.DiffIDs) == 0 { | ||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil
is considered a valid parent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil
means FROM scratch
so we ignore the parent and only check that the configuration matches
OSVersion: target.OSVersion, | ||
}) | ||
if err != nil { | ||
return "", errors.Wrapf(err, "failed to marshal image config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Wrap
|
||
imgID, err := ic.daemon.imageStore.Create(config) | ||
if err != nil { | ||
return "", errors.Wrapf(err, "failed to create cache image") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Wrap
0a43b1d
to
fe741fc
Compare
@@ -447,11 +447,11 @@ func (b *Builder) processImageFrom(img builder.Image) error { | |||
// If no image is found, it returns `(false, nil)`. | |||
// If there is any error, it returns `(false, err)`. | |||
func (b *Builder) probeCache() (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment should be updated. Right now it says "probeCache checks if b.docker
implements builder.ImageCache and image-caching is enabled"
LGTM after nits |
Based on work by KJ Tsanaktsidis Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: KJ Tsanaktsidis <kjtsanaktsidis@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
fe741fc
to
4b8e680
Compare
@stevvooe @aaronlehmann updated |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds the cache-from build option (moby/moby#26839) and fixes #1382.
Hi guys, Is I also struggled to find an explanation of what happens if there are multiple Should the following work smoothly for all possible states of the "cache"? Or is anything here missing or redundant here? docker pull $CI_REGISTRY_IMAGE:$CI_BUILD_REF_NAME || docker pull $CI_REGISTRY_IMAGE:master || true
docker build --tag=$CI_REGISTRY_IMAGE:$CI_BUILD_REF_NAME --cache-from=$CI_REGISTRY_IMAGE:$CI_BUILD_REF_NAME --cache-from=$CI_REGISTRY_IMAGE:master .
docker push $CI_REGISTRY_IMAGE:$CI_BUILD_REF_NAME For docker pull registry.example.com/group/repo:my-branch || docker pull registry.example.com/group/repo:master || true
docker build --tag=registry.example.com/group/repo:my-branch --cache-from=registry.example.com/group/repo:my-branch --cache-from=registry.example.com/group/repo:master .
docker push registry.example.com/group/repo:my-branch And for docker pull registry.example.com/group/repo:master || docker pull registry.example.com/group/repo:master || true
docker build --tag=registry.example.com/group/repo:master --cache-from=registry.example.com/group/repo:my-branch --cache-from=registry.example.com/group/repo:master .
docker push registry.example.com/group/repo:master |
As far as my testing goes, However, I have written a blog post about using I am not sure whats the relevance of |
Yeah, that's what is unclear to me too. If I know that these questions can be answered after a few tests, but it'd be great if they were documented so that people could write robust CI scripts without too much iteration. |
I'd like to know so I can add --cache-from to shipwright. |
When using multiple |
With docker 1.13 a new build flag was added: --cache-from This flag deprecated the buildcache tool that we used with docker 1.12 Details: moby/moby#26839
With docker 1.13 a new build flag was added: --cache-from This flag deprecated the buildcache tool that we used with docker 1.12 Details: moby/moby#26839
Implement build cache based on history array
Implement build cache based on history array
With docker 1.13 a new build flag was added: --cache-from This flag deprecated the buildcache tool that we used with docker 1.12 Details: moby/moby#26839
I just ran into this issue and only by digging deep through history to find this PR did I find the answer to explain the behaviour of the flag - it is not documented this way. Suggestion: could the documentation be updated to include this detail? |
Was this behavior ever implemented? It looks like |
@Jumblemuddle Yes, if you use buildkit #34715 (comment) |
Documented usage of cache-from allows for users to specify multiple images to search through by passing the cache-from argument more than once. Current implementation of cache-from as a string does not enable this behavior. See: moby/moby#26839 (comment) Signed-off-by: Ryan Drew <ryan@thedrews.org>
It is unclear how docker handles multiple caches under the hood. I did find an older comment¹ that if multiple `--cache-from` sources are provided, it will use the first cache hit for the whole run. This is my attempt to prioritize the ordering of the sources to use the tagged images from the ghcr.io registry over the latest images from docker.io. ¹ moby/moby#26839 (comment)
carry #24711
fixes #26065
Adds capability to specify images used as a cache source on build. These images do not need to have local parent chain and can be pulled from other registries. User needs to make sure to only use trusted images as sources.
Usage: