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.

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

Fix caching for multi-step builds. #441

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

dlorenc
Copy link
Collaborator

@dlorenc dlorenc commented Nov 8, 2018

#397 broke caching for Dockerfiles that have more than a single command.

This change fixes that by properly "replaying" the Dockerfile and mutating the config when
calculating cache keys. Previously we were looking at the wrong cache key for each command
when there was more than one.

I think this should fix #410

img, err := layerCache.RetrieveLayer(ck)
if err != nil {
logrus.Infof("No cached layer found for cmd %s", command.String())
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if I'm missing something, but won't the same problem from the issue happen here?

For this Dockerfile:

WORKDIR /dir
RUN something

we won't find the cached layer for WORKDIR so it'll break and never check the cache for RUN?

Copy link
Collaborator

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

Left a question. Also, I think WORKDIR also needs to return false for MetadataOnly() since a directory could be created.

@dlorenc dlorenc force-pushed the fixcache branch 2 times, most recently from 5093ade to 6a91162 Compare November 8, 2018 22:28
@priyawadhwa
Copy link
Collaborator

Looks like the cache test failed in kokoro 😞

@@ -18,3 +18,5 @@

FROM gcr.io/google-appengine/debian9@sha256:1d6a9a6d106bd795098f60f4abb7083626354fa6735e81743c7f8cfca11259f0
RUN apt-get update && apt-get install -y make
COPY context/bar /context
Copy link

Choose a reason for hiding this comment

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

Kind of lurking here so apologies if it's a stupid suggestion but it may help to add a WORKDIR command to this test Dockerfile as it's something many Dockerfile out in the wild will have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Good idea.

@xoen
Copy link

xoen commented Nov 9, 2018

Hello @dlorenc - first of all, thanks so much for the quick fix ❤️

I've checked out this branch and built the executor docker image locally and tried again with our template Dockerfile (as commented on Issue #410).

I tried both with the original Dockerfile version and with the version tweaked to remove the WORKDIR and the good news is that in both cases it now seems to use the cache as I see a bunch of Found cached layer, extracting to filesystem, so that's great 🎊 .

I still see that it pushes more stuff to the /cache repo on subsequent builds despite nothing changing - is this the intended/expected behaviour?

To be clear, these are very small images (AWS ECR says "< 0.01 MiB") but over time it can hit the default AWS ECR limit of 1,000 images per repository, especially if using one single cache repository per class of applications - unless of kaniko has some kind of built-in cache cleanup mechanism which I'm not aware of.

@dlorenc
Copy link
Collaborator Author

dlorenc commented Nov 9, 2018

I still see that it pushes more stuff to the /cache repo on subsequent builds despite nothing changing - is this the intended/expected behaviour?

Thanks - I do think this is a further optimization we can make, and I need to dig into where it's coming from still.

@dlorenc
Copy link
Collaborator Author

dlorenc commented Nov 9, 2018

Looks like the cache test failed in kokoro 😞

It looks like that was a bug with container-diff. I changed the Dockerfile to not triggger it.

xoen added a commit to ministryofjustice/analytics-platform-common-concourse-tasks that referenced this pull request Nov 9, 2018
> kaniko is a tool to build container images from a Dockerfile [...]
> [...]
> kaniko doesn't depend on a Docker daemon and executes each
> command within a Dockerfile completely in userspace.

Kaniko can cache docker layers by pushing these to a special
docker repository and reusing them later on which is ideal
in the context of Concourse where each task runs in an independent
container.

There is a bug in the kaniko caching mechanism at the moment but there is
a PR which seems to solve it (I tested it locally and it's now using the cached layers).

This task is using a resource which was already there, we'll see if this
works (resource code looks OK to me)

Resourse: https://github.com/lxfontes/kaniko-resource
Kaniko: https://github.com/GoogleContainerTools/kaniko
Kanino cache bugfix: GoogleContainerTools/kaniko#441
xoen added a commit to ministryofjustice/analytics-platform-common-concourse-tasks that referenced this pull request Nov 9, 2018
> kaniko is a tool to build container images from a Dockerfile [...]
> [...]
> kaniko doesn't depend on a Docker daemon and executes each
> command within a Dockerfile completely in userspace.

Kaniko can cache docker layers by pushing these to a special
docker repository and reusing them later on which is ideal
in the context of Concourse where each task runs in an independent
container.

There is a bug in the kaniko caching mechanism at the moment but there is
a PR which seems to solve it (I tested it locally and it's now using the cached layers).

This task is using a resource which was already there, we'll see if this
works (resource code looks OK to me)

Resourse: https://github.com/lxfontes/kaniko-resource
Kaniko: https://github.com/GoogleContainerTools/kaniko
Kanino cache bugfix: GoogleContainerTools/kaniko#441
xoen added a commit to ministryofjustice/analytics-platform-common-concourse-tasks that referenced this pull request Nov 9, 2018
> kaniko is a tool to build container images from a Dockerfile [...]
> [...]
> kaniko doesn't depend on a Docker daemon and executes each
> command within a Dockerfile completely in userspace.

Kaniko can cache docker layers by pushing these to a special
docker repository and reusing them later on which is ideal
in the context of Concourse where each task runs in an independent
container.

There is a bug in the kaniko caching mechanism at the moment but there is
a PR which seems to solve it (I tested it locally and it's now using the cached layers).

This task is using a resource which was already there, we'll see if this
works (resource code looks OK to me)

Resourse: https://github.com/lxfontes/kaniko-resource
Kaniko: https://github.com/GoogleContainerTools/kaniko
Kanino cache bugfix: GoogleContainerTools/kaniko#441
xoen added a commit to ministryofjustice/analytics-platform-common-concourse-tasks that referenced this pull request Nov 9, 2018
> kaniko is a tool to build container images from a Dockerfile [...]
> [...]
> kaniko doesn't depend on a Docker daemon and executes each
> command within a Dockerfile completely in userspace.

Kaniko can cache docker layers by pushing these to a special
docker repository and reusing them later on which is ideal
in the context of Concourse where each task runs in an independent
container.

There is a bug in the kaniko caching mechanism at the moment but there is
a PR which seems to solve it (I tested it locally and it's now using the cached layers).

This task is using a resource which was already there, we'll see if this
works (resource code looks OK to me)

Resourse: https://github.com/lxfontes/kaniko-resource
Kaniko: https://github.com/GoogleContainerTools/kaniko
Kanino cache bugfix: GoogleContainerTools/kaniko#441
xoen added a commit to ministryofjustice/analytics-platform-common-concourse-tasks that referenced this pull request Nov 9, 2018
> kaniko is a tool to build container images from a Dockerfile [...]
> [...]
> kaniko doesn't depend on a Docker daemon and executes each
> command within a Dockerfile completely in userspace.

Kaniko can cache docker layers by pushing these to a special
docker repository and reusing them later on which is ideal
in the context of Concourse where each task runs in an independent
container.

There is a bug in the kaniko caching mechanism at the moment but there is
a PR which seems to solve it (I tested it locally and it's now using the cached layers).

This task is using a resource which was already there, we'll see if this
works (resource code looks OK to me)

Resourse: https://github.com/lxfontes/kaniko-resource
Kaniko: https://github.com/GoogleContainerTools/kaniko
Kanino cache bugfix: GoogleContainerTools/kaniko#441
xoen added a commit to ministryofjustice/analytics-platform-common-concourse-tasks that referenced this pull request Nov 9, 2018
> kaniko is a tool to build container images from a Dockerfile [...]
> [...]
> kaniko doesn't depend on a Docker daemon and executes each
> command within a Dockerfile completely in userspace.

Kaniko can cache docker layers by pushing these to a special
docker repository and reusing them later on which is ideal
in the context of Concourse where each task runs in an independent
container.

There is a bug in the kaniko caching mechanism at the moment but there is
a PR which seems to solve it (I tested it locally and it's now using the cached layers).

This task is using a resource which was already there, we'll see if this
works (resource code looks OK to me)

Resourse: https://github.com/lxfontes/kaniko-resource
Kaniko: https://github.com/GoogleContainerTools/kaniko
Kanino cache bugfix: GoogleContainerTools/kaniko#441
Copy link
Collaborator

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

LGTM, just need to remove the -run from the tests and rerun kokoro :)

integration-test.sh Outdated Show resolved Hide resolved
This change fixes that by properly "replaying" the Dockerfile and mutating the config when
calculating cache keys. Previously we were looking at the wrong cache key for each command
when there was more than one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trouble getting caching to work
4 participants