-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
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. |
Definitely! I added some better logging in this PR so once it goes in it should be easier to debug cache misses like this. |
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.
Just one question, ow LGTM!
RUN date > /date | ||
COPY context/foo /foo | ||
RUN echo hey | ||
FROM alpine as base_stage |
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.
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).
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.
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.
…ContainerTools#525)" This reverts commit 1ffae47.
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