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

Do not recompute layers retrieved from cache #882

Merged

Conversation

cvgw
Copy link
Contributor

@cvgw cvgw commented Nov 25, 2019

Description
Updates kaniko to reuse cached layers rather than recreating the layer.

Previously kaniko would retrieve a layer from the cache, unpack the tar, snapshot the file system, and then repack the tar, recreate the layer. This caused unneeded work repacking the ta. It also resulted in layer and image digests that would change every build due to the tar being repacked.

This change updates kaniko so that the new behavior is

  • Retrieve layer from cache
  • Unpack tar
  • Snapshot filesystem
  • Reuse layer retrieved from cache

Note that for the moment it is only implemented for RUN and COPY commands.

This eliminates the extra steps of repacking the tar. It also results in layer and image digest which do not change for cached layers.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

- Improve performance when caching is enabled
- Improve caching behavior for FROM commands which reference a previous stage

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

lgtm!

pkg/executor/build.go Outdated Show resolved Hide resolved
@cvgw cvgw force-pushed the u/cvgw/reuse-cached-layer branch 5 times, most recently from 47d772c to 73635b7 Compare November 25, 2019 23:00
@cvgw cvgw changed the title Work in progress Do not recompute layers retrieved from cache Nov 26, 2019
@cvgw cvgw force-pushed the u/cvgw/reuse-cached-layer branch 2 times, most recently from 3bd9702 to 9f175a2 Compare November 26, 2019 01:43
@cvgw
Copy link
Contributor Author

cvgw commented Nov 27, 2019

This actually will fix the specific case in #682 but not the underlying issue. PR #891 should be a more complete fix.

This change may still be worth doing to remove the extra work of recreating the tar.

pkg/commands/caching_support.go Outdated Show resolved Hide resolved
pkg/commands/caching_support.go Outdated Show resolved Hide resolved
@tejal29
Copy link
Member

tejal29 commented Jan 17, 2020

@cvgw Would be able to pick this up for release tomorrow? if not i will rebase and merge.

@cvgw
Copy link
Contributor Author

cvgw commented Jan 17, 2020

@tejal29 do we still want to make this change? I'm not sure what the motivation is now that we've made all the caching fixes. I suppose there could be a performance win here.

WDYT?

@tejal29
Copy link
Member

tejal29 commented Jan 17, 2020

Since you have spent cycles on it, we can get this in. Upto you!

@cvgw
Copy link
Contributor Author

cvgw commented Jan 18, 2020

This has a LOT of conflicts with master and some substantial overlap with #973

I'm going to wait until #973 is merged and then cherry-pick the changes over

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Jan 18, 2020
@raijinsetsu
Copy link

I believe this PR and/or #715 are contributing to significant time in our builds. Is there an ETA when these changes may be available?
Note: #973 has been merged as of 2 weeks ago.

@cvgw cvgw mentioned this pull request Feb 7, 2020
@tejal29 tejal29 merged commit a17ad8e into GoogleContainerTools:master Feb 7, 2020
@tejal29
Copy link
Member

tejal29 commented Feb 7, 2020

We should look into how we can measure performance increase due to this PR.
One think we can do is,

  1. Monitor build time decrease for GCB (internally)

@raijinsetsu
Copy link

@tejal29
Based on the time I am seeing in my build spent re-archiving and sending the archive to Cloud Storage, my build time would be reduced by about 10 minutes in the "cache hit" scenario, or 33%. If you think it would help, I could try to build and use master to give you actual numbers.

Note: early in my Dockerfile is a "RUN npx lerna bootstrap --ci" which installs all the dependencies for the mono-repository. Installing the dependencies takes about 10 minutes, which includes building 15+ local modules. The subsequent archiving takes about 10 minutes. As the dependencies do not change often, having the cache work correctly would trim about 20 minutes from most builds.

@cvgw cvgw deleted the u/cvgw/reuse-cached-layer branch February 7, 2020 22:41
@raijinsetsu
Copy link

Just for giggles, I used the "gcr.io/kaniko-project/executor:debug-8d9e6b8ea54274f73517f11c113c13cd03d26349" image which appears to have this change.
My build went from 20 minutes to 2 minutes. 👍

abayer added a commit to abayer/jenkins-x-builders that referenced this pull request Feb 11, 2020
Specifically GoogleContainerTools/kaniko#882

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer added a commit to abayer/jenkins-x-builders that referenced this pull request Feb 11, 2020
Specifically GoogleContainerTools/kaniko#882

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer added a commit to abayer/jenkins-x-builders that referenced this pull request Feb 11, 2020
Specifically GoogleContainerTools/kaniko#882

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate-release cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants