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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7a18b8
Make docker build faster
georgysavva Mar 16, 2021
fe21590
Fix CI errors
georgysavva Mar 16, 2021
6a6f0af
Use COPY instead of mount
georgysavva Mar 17, 2021
fdbe5e1
Enable docker buildkit in CI
georgysavva Mar 17, 2021
36f8a67
Enable docker BuildKit in scripts
georgysavva Mar 17, 2021
5b22c73
Use exact version instead of sha for docker image
georgysavva Mar 17, 2021
9c287c2
Improve .dockerignore
georgysavva Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/integration-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ jobs:
name: autopeering
env:
TEST_NAME: autopeering
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -46,6 +47,7 @@ jobs:
name: common
env:
TEST_NAME: common
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -80,6 +82,7 @@ jobs:
name: consensus
env:
TEST_NAME: consensus
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -115,6 +118,7 @@ jobs:
name: drng
env:
TEST_NAME: drng
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -151,6 +155,7 @@ jobs:
name: message
env:
TEST_NAME: message
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -187,6 +192,7 @@ jobs:
name: value
env:
TEST_NAME: value
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -221,6 +227,7 @@ jobs:
name: faucet
env:
TEST_NAME: faucet
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -255,6 +262,7 @@ jobs:
name: syncbeacon
env:
TEST_NAME: syncbeacon
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down Expand Up @@ -289,6 +297,7 @@ jobs:
name: mana
env:
TEST_NAME: mana
DOCKER_BUILDKIT: 1
runs-on: ubuntu-latest
steps:

Expand Down
18 changes: 11 additions & 7 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# syntax = docker/dockerfile:1.2
############################
# Build
############################
Expand All @@ -19,13 +20,16 @@ ENV GO111MODULE=on
RUN go mod download
RUN go mod verify

# Copy everything from the current directory to the PWD(Present Working Directory) inside the container
COPY . .

# Build the binary
RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \
-ldflags='-w -s -extldflags "-static"' -a \
-o /go/bin/goshimmer
# 1. Mount everything from the current directory to the PWD(Present Working Directory) inside the container
# 2. Mount the build cache volume
# 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.

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.

-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 •
edited
Loading

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 :)


############################
# Image
Expand Down
10 changes: 10 additions & 0 deletions check_static.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/bin/bash
# This script uses ldd utility to verify that result goshimmer binary is statically linked

ldd /go/bin/goshimmer &> /dev/null
if [ $? -ne 0 ]; then
exit 0
else
echo "/go/bin/goshimmer must be a statically linked binary"
exit 1
fi