Cleanup Docker context and decrease build time #1498
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @drGrove! |
k8s-ci-robot
added
the
size/M
|
/check-cla |
|
/assign @Raffo |
|
@drGrove please verify you have signed the CLA with the correct email which you have used for your commit (danny@drgrovellc.com). |
|
/check-cla |
k8s-ci-robot
added
cncf-cla: yes
Done |
|
/assign njuettner |
|
@njuettner ping? |
|
/kind cleanup |
k8s-ci-robot
added
the
kind/cleanup
|
@drGrove please rebase your PR to pull the the latest changes from the master branch. This will trigger the new GitHub actions CI test to run. I'll review this PR after this is done. Thanks! /assign |
drGrove
force-pushed
the
docker-cleanup
branch
from
6411306
to
65a8f4e
Compare
|
@drGrove the below make targets are failing with the Dockerfile changes. I verified this all works on the master branch without an issue. |
|
/unassign @seanmalloy |
|
The code changes look good to me so far, however I also tested @drGrove could you please update the pull request to fix these two make targets and then I believe this PR is ready for final review by the maintainers. |
|
Will do! I was running into some weird issues and have been slammed at work so I'll take a look this weekend. |
|
Hey @drGrove did you have a chance to look into why the |
There was a problem hiding this comment.
@drGrove @vinny-sabatini To fix the Docker builds, make this change to both Dockerfiles:
-RUN go mod vendor && \
- go mod download
+RUN go mod downloadThere are two reasons that the current setup doesn't work;
- There appears to be an issue with running
go mod vendor(without any Go code to reference the dependencies) in go1.4.9. This appears to be fixed in go1.15.2 (thanks to https://go-review.googlesource.com/c/go/+/217135, I think?). - The bigger issue is that
go mod vendoronly copies pre-download dependencies into the vendor dir, and subsequent (e.g.go build) commands will use vendoring if that dir exists. So what we're doing here is:- Running
go mod vendorto copy no dependencies intovendor. - Running
go mod downloadto download dependencies into the/go/modsystem cache. - Running
go buildwhich then tries to use the emptyvendordirectory.
- Running
Testing with this change locally I get:
- A ~5 minute fresh Docker build (
make build.mini). - An instant rebuild when nothing changes.
- A ~3 minute rebuild when I change Go code (like adding a line to
main.go)
Normal builds still take 3 minutes, but at least you don't have to re-download the dependencies. Cheers!
drGrove
force-pushed
the
docker-cleanup
branch
from
65a8f4e
to
7bd716e
Compare
|
@vinny-sabatini ready for review |
| @@ -17,10 +17,12 @@ FROM golang:1.14 as builder | |||
|
|
|||
| WORKDIR /sigs.k8s.io/external-dns | |||
|
|
|||
| COPY go.mod . | |||
| COPY go.sum . | |||
| RUN go mod download | |||
There was a problem hiding this comment.
I think we don't explicitly need to run go mod download as make build will run go build which will download the needed dependencies with the version of Go that we use.
There was a problem hiding this comment.
It's an optimization in the docker caching mechanism.
If my go mod and go sum don't change why re-pull the deps when testing locally
|
|
||
| COPY go.mod . | ||
| COPY go.sum . | ||
| RUN go mod download |
| RUN apt-get update \ | ||
| && apt-get install \ | ||
| ca-certificates \ | ||
| && update-ca-certificates |
There was a problem hiding this comment.
what's the need of moving those lines?
There was a problem hiding this comment.
In docker, you want things that change less often (or can be slow) to happen first. If the packages don't change between builds, why have the cache dump by COPY . . affect my package install.
drGrove
force-pushed
the
docker-cleanup
branch
from
7bd716e
to
8026153
Compare
|
@Raffo ping |
|
@drGrove Sorry for the delay on this one. Changes are good, but we need to keep the |
This is based off the work found in kubernetes-sigs#1307 that was never merged. It moves around the install and copy of certain conponents to take better advantage of the Docker cache ad well as drops running tests during the build of the image. The reason for dropping tests is to improve build time and as running tests within the build while they're already being run in CI seems like an unnecessary added tax. Signed-off-by: Danny Grove <danny@drgrovellc.com>
drGrove
force-pushed
the
docker-cleanup
branch
from
8026153
to
2226e00
Compare
|
@Raffo done, sorry about the delay |
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: drGrove, Raffo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This is based off the work found in #1307 that was never merged. It
moves around the install and copy of certain conponents to take better
advantage of the Docker cache ad well as drops running tests during the
build of the image.
The reason for dropping tests is to improve build time and as running
tests within the build while they're already being run in CI seems like
an unnecessary added tax.
Signed-off-by: Danny Grove danny@drgrovellc.com