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

Speed up local layer builds by skipping unnecessary compression of docker context #862

Merged
merged 4 commits into from
Jan 18, 2019

Conversation

StefanSmith
Copy link
Contributor

@StefanSmith StefanSmith commented Dec 15, 2018

Issue #, if available:

#863

Description of changes:

Stopped using compression when creating tarball of docker context for building samcli/lambda docker image containing local Lambda layers. With compression, the stream-and-build procedure takes roughly 27 seconds for a sample 215 MB file tree of Python dependencies (compressing to 51 MB). Without compression, the same procedure takes less than 4 seconds.

Checklist:

  • Write Design Document
  • Write unit tests
  • Write/update functional tests
  • Write/update integration tests
  • make pr passes
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@StefanSmith
Copy link
Contributor Author

Failed the Python 3.6 CI build but passed 2.7 and 3.7. Cause is a test that is already failing on the develop branch according to recent build history (TestWithDifferentLambdaRuntimeZips testMethod=test_runtime_zip_0_Go1xFunction).

@jfuss
Copy link
Contributor

jfuss commented Dec 20, 2018

@StefanSmith This has happened from time to time. At the moment, we aren't entirely sure why sometimes that tests fails within Travis. I just restarted the build.

Remove the compression could be right here but not entirely sure of the full impact this could have. Do you have an understanding of how this this could impact very large tarballs? Will this make creating the images slower now?

@StefanSmith
Copy link
Contributor Author

StefanSmith commented Dec 20, 2018

Remove the compression could be right here but not entirely sure of the full impact this could have. Do you have an understanding of how this this could impact very large tarballs? Will this make creating the images slower now?

@jfuss I'm unaware of any factor that would cause docker images to build slower with compression off. My understanding is that it's a convenience feature provided by Docker to allow you to build directly from a gzipped tarball: https://docs.docker.com/engine/reference/commandline/build/#tarball-contexts. My expectation is that in all cases for SAM CLI the compression is redundant and creates a large overhead (compress and decompress). See also https://docker-py.readthedocs.io/en/stable/images.html for the library used by SAM CLI.

As mentioned in the issue (#863), I believe there may be an even faster way to invoke functions that reference layers but I have not got the bandwidth at present to pursue it.

@jfuss
Copy link
Contributor

jfuss commented Dec 21, 2018

@StefanSmith My initial thought was if we don't compress then we need to send bigger tarballs to docker causing builds to be slow (hence the gzip). But this is done locally to the docker daemon so that concern might not be valid.

With respect to the issue, keeping track of individual Dockerfiles is a lot of overhead. Ideally, we can place everything into the layers-cache and build from there. This wasn't done initially because we don't have a good mechanism to tell if something changed, copy to the cache, and then build. This is doable but the commands end up needing to manage state, which we didn't want to introduce.

One thing you can do, is formally create a layer (especially if you don't expect it to change). SAM CLI will download the layer and build the image once, as long as the layer order in the template does not change, add another layer, or change the version. We can confidently do this because layers are immutable.

Can you fix the tests as well? Looks like your recent commit broke unit tests.

@StefanSmith
Copy link
Contributor Author

@jfuss Tests now passing. Sorry for the delay on this. Thanks!

@jfuss
Copy link
Contributor

jfuss commented Jan 18, 2019

@StefanSmith Not a problem. Thanks for the patch and the improvement to using layers locally.

@jfuss jfuss added area/layers stage/accepted Accepted and will be fixed labels Jan 18, 2019
@jfuss jfuss merged commit 4259a52 into aws:develop Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/layers stage/accepted Accepted and will be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants