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

Use buildx for multiarch docker builds #836

Conversation

LucasRoesler
Copy link
Member

Description

  • Refactor the Makefile and build scripts to consolidate the multiarch
    builds into a single docker file by using docker buildx. This mimics
    the changes from inlets/inlets#204.
  • The redist script has been removed and the makefile now contains the
    equivalent commands.
  • The build command now calls buildx twice, to get the root and rootless
    image.
  • Add a new constant shortPlatform so that we can map the TARGETPLATFORM
    value that is exposed by buildx so the required value used by the template store.

Motivation and Context

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

Wait and see how travis feels

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@LucasRoesler LucasRoesler force-pushed the simplify-multiarch-builds-with-buildx branch from ee48c66 to 09ba934 Compare October 25, 2020 20:08
.travis.yml Outdated
services:
- docker

before_script:
- curl -sSLf https://get.docker.com | sed s/sleep\ 20/sleep\ 0/g | sudo -E sh
- sudo rm -rf /var/lib/apt/lists/*
Copy link
Member

Choose a reason for hiding this comment

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

See how I extracted this into a shell script. I think it may be easier to maintain that way.

https://github.com/teamserverless/license-check/blob/master/.travis.yml#L14

.travis.yml Outdated
docker push quay.io/$DOCKER_NS/faas-cli:latest-root;
- make docker-login
- make docker-login-ghcr
- if [ ! -z "$TRAVIS_TAG" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need this, or it can move to the Makefile

Copy link
Member

Choose a reason for hiding this comment

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

See how I did it for inlets/inlets

Copy link
Member Author

Choose a reason for hiding this comment

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

i wasn't sure what you wanted to do with the quey stuff, so i left it alone

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Can we do it like the inlets/inlets repo?

Basically build again with another tag, it just uses the cache.

Let's switch to GHCR for this repo from Quay.io, I just checked that nobody has downloaded it from there all year.

Copy link
Member

Choose a reason for hiding this comment

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

(FYI I've added GHCR credentials and I've deleted the Quay.io repo for faas-cli)

Dockerfile Outdated
@@ -1,14 +1,23 @@
FROM teamserverless/license-check:0.3.6 as license-check
Copy link
Member

Choose a reason for hiding this comment

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

Can you bump to 0.3.9?

RUN VERSION=$(git describe --all --exact-match `git rev-parse HEAD` | grep tags | sed 's/tags\///') \
&& GIT_COMMIT=$(git rev-list -1 HEAD) \
&& CGO_ENABLED=0 GOOS=linux go build --ldflags "-s -w \
RUN GOOS=${TARGETOS} GOARCH=${TARGETARCH} CGO_ENABLED=0 \
Copy link
Member

Choose a reason for hiding this comment

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

Do we need -mod=vendor here?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is set via ENV GOFLAGS=-mod=vendor

@LucasRoesler LucasRoesler force-pushed the simplify-multiarch-builds-with-buildx branch 2 times, most recently from 0cc46e1 to cf15793 Compare October 25, 2020 20:17
alexellis added a commit that referenced this pull request Oct 25, 2020
The Quay image had no recent pulls, unlike the Docker Hub image
in #836 @LucasRoesler is also adding GHCR.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Sending comments over

.travis.yml Outdated
docker push quay.io/$DOCKER_NS/faas-cli:latest-root;
- make docker-login
- make docker-login-ghcr
- if [ ! -z "$TRAVIS_TAG" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

Sure. Can we do it like the inlets/inlets repo?

Basically build again with another tag, it just uses the cache.

Let's switch to GHCR for this repo from Quay.io, I just checked that nobody has downloaded it from there all year.

.travis.yml Outdated
docker push quay.io/$DOCKER_NS/faas-cli:latest-root;
- make docker-login
- make docker-login-ghcr
- if [ ! -z "$TRAVIS_TAG" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

(FYI I've added GHCR credentials and I've deleted the Quay.io repo for faas-cli)

@LucasRoesler LucasRoesler force-pushed the simplify-multiarch-builds-with-buildx branch 4 times, most recently from a20d10b to b7ccf42 Compare October 25, 2020 21:28
--output "$(.BUILDX_OUTPUT)" \
--target root \
--tag openfaas/faas-cli:latest-root \
--tag openfaas/faas-cli:$(.GIT_VERSION)-root .

.PHONY: build_redist
build_redist:
Copy link
Member

Choose a reason for hiding this comment

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

Previously we had a build that might have run 2x as fast because we built in Docker, then pulled out the binaries.

How that we are both building in Docker, and on the host, how does the performance compare?

Should we adapt the Docker builds to pull in these binaries peharps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied the flow from inlets, i figured you preferred that

Copy link
Member

Choose a reason for hiding this comment

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

What is the timing difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll just revert. I don't know the timing difference because i haven't gotten a successful build. My goal was consistency between projects to reduce the maintenance effort. But I don't have strong feelings here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hav reverted that change

Copy link
Member Author

Choose a reason for hiding this comment

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

Using buildx results in build time: 8 min 51 sec
The last master build was : 4 min 46 sec

@alexellis
Copy link
Member

alexellis commented Oct 30, 2020

Screenshot 2020-10-30 at 09 14 25

build golang.org/x/sys/internal/unsafeheader: cannot find module for path golang.org/x/sys/internal/unsafeheader
933

Can you take a look at the error please?

**What**
- Refactor the Makefile and build scripts to consolidate the multiarch
  builds into a single docker file by using `docker buildx`. This mimics
  the changes from inlets/inlets#204.
- The redist script has been removed and the makefile now contains the
  equivalent commands.
- The build command now calls buildx twice, to get the root and rootless
  image.

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
@LucasRoesler LucasRoesler force-pushed the simplify-multiarch-builds-with-buildx branch from b7ccf42 to b4bb0e8 Compare October 30, 2020 09:48
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 6, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 6, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 8, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 8, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 8, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 8, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 8, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 8, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 11, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 11, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 11, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 11, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 11, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 11, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 15, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 15, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 17, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 17, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 17, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
utsavanand2 added a commit to utsavanand2/faas-cli that referenced this pull request Nov 17, 2020
Fixes openfaas#838

Thanks to Lucas Roesler for his PR for buildx support openfaas#836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
alexellis pushed a commit that referenced this pull request Nov 17, 2020
Fixes #838

Thanks to Lucas Roesler for his PR for buildx support #836
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>
@alexellis
Copy link
Member

I've merged the newer PR from Utsav. Thank you for showing the way with this.

@alexellis alexellis closed this Nov 17, 2020
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.

None yet

2 participants