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

Image sizes increased dramatically since merging #289 #314

Closed
lubo opened this issue Aug 25, 2018 · 6 comments · Fixed by #319
Closed

Image sizes increased dramatically since merging #289 #314

lubo opened this issue Aug 25, 2018 · 6 comments · Fixed by #319
Assignees

Comments

@lubo
Copy link

lubo commented Aug 25, 2018

Since merging #289, Kaniko seems to create layers containing not only the files copied by COPY instruction, but also files from the previous layers.

@bobcatfish
Copy link
Contributor

That's not good! Thanks for reporting this @lubo - do you happen to have an example of how to reproduce this?

@lubo
Copy link
Author

lubo commented Aug 25, 2018

Sure. The Dockerfiles I've been testing Kaniko with look like this:

FROM image as installer
RUN <install files into /install>
COPY more files /install

FROM image
COPY --from=installer /install /

So, I use multi-stage builds. In the first stage, /install is created and then it's copied to the second stage. Now, I'd expect the layer created by COPY --from=installer /install / to only contain files from /install in the first stage. I haven't investigated this thoroughly, but the layer definitely contains files that it should not and it seems like they come from the previous layers of the second stage (or from the base image image in this particular case).

@bobcatfish
Copy link
Contributor

Thanks @lubo ! So it looks like this could be specifically related to multi-stage builds 🤔 That logic definitely may have changed in #289 (not intentionally tho!). I'll take a look!

@bobcatfish bobcatfish self-assigned this Aug 25, 2018
@priyawadhwa
Copy link
Collaborator

I used this test Dockerfile and was able to repro this @ HEAD:

FROM gcr.io/google-appengine/debian9 as installer
RUN mkdir /install
RUN echo "hey" > /install/hey
COPY Makefile hack /install/

FROM gcr.io/google-appengine/debian9
COPY --from=installer /install /

I used container-diff with the --type file --type layer arguments to diff images by layer, and kaniko built the image correctly.

I actually don't think this is a bug. #289 changed the logic here to better determine if we want to snapshot the entire filesystem or just certain files after each command.

Before #289 we defaulted to snapshotting the entire filesystem after the last command, so you wouldn't have seen a huge log message detailing the exact files to snapshot. Now that the logic is correct, since you're copying to root (/), we snapshot all of the files at root and you get a giant log message listing all of those files, which is expected behaviour.

We could change the code to not print out all the files we're snapshotting and instead just say something like (Snapshotting files...) though, so that the logs aren't super long and unhelpful!

@priyawadhwa
Copy link
Collaborator

priyawadhwa commented Aug 27, 2018

Whoops, please disregard my above comment, I just tried again with the same Dockerfile and it does seem like there's some sort of bug 😰

Update: The COPY command gets the files to snapshot by getting all files copied to the destination here, and in this case the destination is root (/) so we include all files to be snapshotted. Now that #289 forces snapshotting of specific files (Add instead of MaybeAdd) all of these files end up in the layer.

@bobcatfish , WDYT would make sense here?

priyawadhwa pushed a commit to priyawadhwa/kaniko that referenced this issue Aug 27, 2018
Before GoogleContainerTools#289 was merged, when copying over directories for COPY kaniko
would get a list of all files at the destination specified and add them
to the list of files to be snapshotted. If the destination was root it
would add all files. This worked because the snapshotter made sure the
file had been changed before adding it to the layer.

After GoogleContainerTools#289, we changed the logic to add all files snapshotted to a layer
without checking if the files had been changed. This created the bug in
got all the files at root and added them to the layer without checking
if they had been changed.

This change should fix this bug. Now, the CopyDir function returns a
list of files it copied over and only those files are added to the list
of files to be snapshotted.

Should fix GoogleContainerTools#314
@bobcatfish bobcatfish assigned priyawadhwa and unassigned bobcatfish Aug 27, 2018
@priyawadhwa
Copy link
Collaborator

Hey @lubo , this should have been fixed by #319, please open another issue if you encounter any more problems!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants