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

Make docker build faster #1074

Merged
merged 7 commits into from
Mar 18, 2021
Merged

Make docker build faster #1074

merged 7 commits into from
Mar 18, 2021

Conversation

georgysavva
Copy link
Contributor

@georgysavva georgysavva commented Mar 16, 2021

Docker build now mounts a volume for Go build-cache during the image build. Another small optimization is: mount the codebase instead of COPYing it.
As a result, the docker re-build time is 4.5s instead of the 50s before on my laptop

Docker build now mounts a volume for Go build cache during the image build. Another small optimization is: mount source code base instead of COPYing it.
As a result the docker re-build time is 4.5s instead of 50s before.
Use special "syntax" word to refer to an external docker frontend in Dockerfile. Remove empty lines to not cause warning.
@georgysavva georgysavva requested review from Wollac, capossele, hmoog and jonastheis and removed request for Wollac March 16, 2021 12:03
@georgysavva
Copy link
Contributor Author

I still need to figure out why it doesn't work in the Github Actions.

Get back to coping the codebase instead of mounting it.
Set DOCKER_BUILDKIT in github actions
Copy link
Contributor

@jonastheis jonastheis left a comment

Choose a reason for hiding this comment

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

Maybe we should also enable BuildKit for the images we push to DockerHub on develop and for a release?

RUN --mount=target=. \
--mount=type=cache,target=/root/.cache/go-build \
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \
-ldflags='-w -s -extldflags "-static"' \
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you removed the -a (force rebuilding of packages that are already up-to-date.) flag. Was there any specific reason we had this in first place? @Wollac knows maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason that I see is to always rebuild everything including the std to be more sure that the resulting binary is statically linked. But build-cache doesn't work in that case, so I disabled it and added an additional check after the build that the resulting binary is indeed statically linked so we could see an error during the build if it's not true for some mysterious reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually add the -a just to be on the safe side. However, the Go compiler should be able to correctly detect necessary rebuilds for everything.

# 3. Build the binary
# 4. Verify that goshimmer binary is statically linked
RUN --mount=target=. \
--mount=type=cache,target=/root/.cache/go-build \
Copy link
Contributor

Choose a reason for hiding this comment

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

This should significantly speed up local builds and thus make development for integration tests much less painful. But we won't be able to leverage the cache on GH Actions since this is not persisted anywhere and we'd need to share it across builds as suggested as a "hack" here See this issue moby/buildkit#1512.
Another option would be using one of the techniques described in #890.

But in general I think this is already great and we should maybe first implement static peering to speed up overall execution of the integration tests before investing too much time into CI optimizations/caching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with you. Reusing RUN --mount=type=cache in GH actions seems to be hard right now. let's postpone it.
The docker layer caching is pretty doable: here are the official docker GH actions for that: https://github.com/docker/build-push-action/blob/master/docs/advanced/cache.md
and a third-party article https://evilmartians.com/chronicles/build-images-on-github-actions-with-docker-layer-caching

Copy link
Contributor

Choose a reason for hiding this comment

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

How is the mounting interacting with the docker build cache?
Before with a COPY . . it was assured that if none of the files (outside of .dockerignore) changed, all the successive build steps were skipped.
Is the mount . doing the same and is it also skipping files dockerignore? I think it could make sense to still keep the COPY for the sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It provides the same features as COPY . .: if none of the files changed: everything is cached in docker
Yes, it also respects the .dockerignore file.
This tips if from the official Docker article for Go applications https://www.docker.com/blog/containerize-your-go-developer-environment-part-3/:

Performing a COPY will create an extra layer in the container image which slows things down and uses extra disk space. This can be avoided by using RUN --mount and bind mounting from the build context, from a stage, or an image. Adopting this pattern, the resulting Dockerfile is as follows.
The default mount type is a read only bind mount from the context that you pass with the docker build command. This means that you can replace the COPY . . with a RUN --mount=target=. wherever you need the files from your context to run a command but do not need them to persist in the final image.

@georgysavva
Copy link
Contributor Author

Maybe we should also enable BuildKit for the images we push to DockerHub on develop and for a release?

Yes, I was going to do that shortly, I just wanted to test it indeed fixes the github actions

@georgysavva
Copy link
Contributor Author

Maybe we should also enable BuildKit for the images we push to DockerHub on develop and for a release?

BTW the official docker GH actions use BuildKit by default, so we don't need to change anything for develop and release.
https://github.com/docker/build-push-action/blob/92e71463491f2d026a477188b8ad3a0fdd9d672c/.github/workflows/main.yml#L27

Set DOCKER_BUILDKIT=1 and COMPOSE_DOCKER_CLI_BUILD=1 envs in scripts. Also use sha to refer to docker frontend image.
@georgysavva georgysavva requested a review from jonastheis March 17, 2021 13:18
For some reason sha referencing didn't work on GH action, I switched back to exact version number.
RUN --mount=target=. \
--mount=type=cache,target=/root/.cache/go-build \
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \
-ldflags='-w -s -extldflags "-static"' \
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually add the -a just to be on the safe side. However, the Go compiler should be able to correctly detect necessary rebuilds for everything.

CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \
-ldflags='-w -s -extldflags "-static"' \
-o /go/bin/goshimmer; \
./check_static.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the static check really needed? Shouldn't otherwise the build with -static fail anyways?

Copy link
Contributor Author

@georgysavva georgysavva Mar 18, 2021

Choose a reason for hiding this comment

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

We don't need it with 99.99% probability. The only case we would need it that I see is: if there is a flow in the Go build cache mechanism and it re-used some dynamically linked parts.

Since we don't rebuild everything from scratch as with -a I added that check to also be on the safe side :)

Add a few folders to .dockerignore
@georgysavva georgysavva requested a review from Wollac March 18, 2021 06:07
@georgysavva
Copy link
Contributor Author

For some reason, GH actions fail when I set docker frontend image via SHA (the preferred way) this commit: 36f8a67
So I used tag instead, it should be fine for now. I opened an issue for that error in docker BuildKit: moby/buildkit#2027

@capossele capossele merged commit d60992f into develop Mar 18, 2021
@capossele capossele deleted the feat/optimize-docker-build-time branch March 18, 2021 11:40
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 this pull request may close these issues.

4 participants