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

Change cache key calculation to be more reproducible. #525

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

dlorenc
Copy link
Collaborator

@dlorenc dlorenc commented Jan 11, 2019

Before we were using the full image digest, but that contains a timestamp. Now
we only use the layers themselves and the image config (env vars, etc.).

Also fix a bug in unpacking the layers themselves. mtimes can change during unpacking,
so set them all once at the end.

I think this will fix #521

@dinvlad
Copy link

dinvlad commented Jan 11, 2019

Thanks! I've tested this image with a real-world example (which I previously reduced to the example from #521), and for some reason caching still doesn't work. I've shared the full example at https://github.com/broadinstitute/kaniko-stage-caching-bug/blob/master/Dockerfile.full

Could you take a look? I'm also still trying to understand how to enable more verbose logs.

@dlorenc
Copy link
Collaborator Author

dlorenc commented Jan 22, 2019

Could you take a look? I'm also still trying to understand how to enable more verbose logs.

Definitely! I added some better logging in this PR so once it goes in it should be easier to debug cache misses like this.

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.

Just one question, ow LGTM!

RUN date > /date
COPY context/foo /foo
RUN echo hey
FROM alpine as base_stage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this Dockerfile fail integration tests if caching didn't work? As in, if kaniko built it a second time without the cache during the tests, I'm not sure if container-diff would catch the difference (that's why I had RUN date > /date in the old version, since container-diff would fail if the date layer wasn't pulled from the cache).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point! I changed this to reproduce the caching issue I was hitting, but dropped the date thing. I'll double check.

Before we were using the full image digest, but that contains a timestamp. Now
we only use the layers themselves and the image config (env vars, etc.).

Also fix a bug in unpacking the layers themselves. mtimes can change during unpacking,
so set them all once at the end.
@dlorenc dlorenc merged commit 1ffae47 into GoogleContainerTools:master Jan 23, 2019
@dlorenc dlorenc deleted the cachebetter branch January 23, 2019 19:46
discordianfish added a commit to discordianfish/kaniko that referenced this pull request Apr 3, 2019
@discordianfish discordianfish mentioned this pull request Apr 3, 2019
dlorenc pushed a commit that referenced this pull request May 10, 2019
* Revert "Change cache key calculation to be more reproducible. (#525)"

This reverts commit 1ffae47.

* Add logging of composition key back

* Do not include build args in cache key

This should be save, given that the commands will have the args included
when the cache key gets built.
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.

Stages with WORKDIR/only mkdir break caching
4 participants