Introduce Docker layer caching#1559
Conversation
|
Codecov Report
@@ Coverage Diff @@
## develop #1559 +/- ##
========================================
Coverage 76.71% 76.71%
========================================
Files 82 82
Lines 3032 3032
Branches 463 463
========================================
Hits 2326 2326
Misses 706 706
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
snario
left a comment
There was a problem hiding this comment.
Makes sense to me and really easy to understand. Had a couple questions and a code style comment.
| @@ -1,4 +1,5 @@ | |||
| FROM ethereumoptimism/builder AS builder | |||
| ARG LOCAL_REGISTRY=docker.io | |||
| FROM ${LOCAL_REGISTRY}/ethereumoptimism/builder AS builder | |||
There was a problem hiding this comment.
This is more of a question, and not a comment on this PR, but do you know if it would be possible to also pull a tagged version of this builder image with an ARG too? We recently had some confusion on the team due to this being pushed as latest on every CI run.
There was a problem hiding this comment.
Yep, we can pull a tagged version of the builder image using an ARG. Doing so introduces some funky behavior around using the ARG elsewhere in the build but it's totally doable. Here's the Docker documentation on doing this:
https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact
| id: buildx | ||
| with: | ||
| version: latest | ||
| driver-opts: image=moby/buildkit:master,network=host |
There was a problem hiding this comment.
Does image=moby/buildkit:master basically mean "use the docker-container backend"? I was just curious how we toggle between the native binary and this option.
There was a problem hiding this comment.
This directive specifies configures the docker-container backend. To toggle between backends, you'd specify the driver option.
I'm using the master version of buildkit here because of this bug, which requires changes both in buildkit and containerd. Unfortunately, GH Action's version of containerd is out of date and does not contain the fix. By specifying the version here the bug will hopefully fix itself as soon as GitHub updates containerd since we're already at the correct bulidkit version.
Co-authored-by: Liam Horne <liam@lihorne.com>
|
Now that the cache is warm, I rebuilt the integration tests and got the following results:
Turns out the |
gakonst
left a comment
There was a problem hiding this comment.
LGTM - Matt and I synced on this offline. The warm/cold cache caveat seems fine to me.
Overview
This PR improves the performance of our integration tests by adding Docker layer caching. After running a couple of test builds on my fork, I was able to reduce the time taken by the "build services" step from 15 minutes to under 5.
Implementation
To make layer caching work I did the following things:
build-ci.shscript to use BuildKit/BuildX, then configured the builder to store layers in the cache directory.Caveats
yarn buildin thebuild-ci.shscript was superfluous so I removed it. Let me know if this is incorrect and I'll fix it. The tests all passed after removing it.