Setup docker buildx for all-in-one #2325

morlay · 2020-07-02T02:04:57Z

Which problem is this PR solving?

buildx in travis.yml to support Multiarch docker images #2292

Short description of the changes

# file ./scripts/travis/build-all-in-one-image.sh

bash ./scripts/travis/setup-docker-buildx.sh


# multi binary build
make build-all-in-one GOOS=linux GOARCH=amd64
make build-all-in-one GOOS=linux GOARCH=arm64

repo=jaegertracing/all-in-one

# load to local docker for integration_test
docker buildx build --load -f cmd/all-in-one/Dockerfile -t $repo:latest cmd/all-in-one
run_integration_test $repo
# push after testing
# could push multi tags by multi `-t $repo:latest -t $repo:1.18 -t $repo:1.18.1`
# it will load from buildkit cache
docker buildx build --push -f cmd/all-in-one/Dockerfile -t $repo:latest cmd/all-in-one

@pavolloffay
have to change upload-to-docker, need to discuss.

pavolloffay · 2020-07-07T09:15:55Z

Does this build images for all architectures docker buildx build --load -f cmd/all-in-one/Dockerfile -t $repo:latest cmd/all-in-one?

pavolloffay · 2020-07-07T09:17:53Z

Could you please enable the multi arch build on travis and if possible reuse the existing make targets? Ideally we should not require dependency on buildx when building a single arch locally.

morlay · 2020-07-07T11:13:29Z

docker buildx build --load -f --platform=linux/arm64,linux/amd64 cmd/all-in-one/Dockerfile -t $repo:latest cmd/all-in-one will build arm64 & amd64 both.

but now in travis, need to run tests after building. this why --load here.

after testings, we could use --push to push mutli arch images.

and now, in your scripts, will tag for mutil versions.
and buildx not reuse the ./scripts/travis/upload-to-docker.sh, i am no ideas should i refactor that file? or just refactor ./scripts/travis/build-all-in-one-image.sh

morlay · 2020-07-08T01:56:38Z

@pavolloffay i updated ./scripts/travis/build-all-in-one-image.sh with buildx.

need docker 19.03, so have to update to ubuntu:bionic too.

no idea why the crossdock failed.

codecov · 2020-07-08T02:17:41Z

Codecov Report

Merging #2325 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2325   +/-   ##
=======================================
  Coverage   95.07%   95.07%           
=======================================
  Files         209      209           
  Lines        9364     9364           
=======================================
  Hits         8903     8903           
  Misses        385      385           
  Partials       76       76

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a0d8cc...2376d75. Read the comment docs.

pavolloffay · 2020-07-08T07:25:08Z

@morlay could you please open a separate PR with change from trusty to bionic?

morlay · 2020-07-09T01:01:41Z

@pavolloffay sure

#2334

pavolloffay

Could you please rebase with the latest changes in master?

pavolloffay · 2020-07-09T07:34:10Z

scripts/travis/build-all-in-one-image.sh

+  REPO=${1}
+  CMD_ROOT=${1}
+
+  unset major minor patch


why has been this copied from upload-to-docker.sh? Could we just keep it there?

with docker buildx, we could not use docker tag directly.

docker tag just tag local image for local arch, when we push images with that tags, it will break the oci image with multi arch.

so here for creating multi flags of tag (-t all-in-one:latest -t all-in-one:1.18.1) for buildx

ok, but there's a lot being copied here, and would have to be repeated for other Docker images.
Can we not either parameterize upload-to-docker.sh or refactor to avoid repetition?

I am fine to go with this and see how it works and refactor other images in a second PR

pavolloffay · 2020-07-10T12:30:15Z

@jaegertracing/jaeger-maintainers any objections on merging this? We could the multiarch build for only the all-in-one initially.

yurishkuro · 2020-07-10T18:46:23Z

scripts/travis/build-all-in-one-image.sh

 }

-make build-all-in-one GOOS=linux GOARCH=$GOARCH
+GOOS=linux GOARCH=amd64 make build-all-in-one
+GOOS=linux GOARCH=arm64 make build-all-in-one


nit: these vars are Make vars, not env vars, they can go on the right side.

yep. i reverted it

yurishkuro · 2020-07-10T18:49:13Z

scripts/travis/build-all-in-one-image.sh

-  bash ./scripts/travis/upload-to-docker.sh
+
+  REPO=${1}
+  CMD_ROOT=${1}


both of these are $1?

Sorry. CMD_ROOT should be $2. fixed it.

morlay · 2020-07-31T01:59:12Z

@pavolloffay

anything need i to do next?

pavolloffay

I will approve it and merge next week.

thanks @morlay!

davidsbond · 2020-10-07T14:32:52Z

@pavolloffay is there any chance to get this merged in? I'd like to be able to use the allinone image on my raspberry pi swarm

morlay · 2020-10-09T02:06:21Z

@davidsbond
you could use https://hub.docker.com/r/querycapistio/all-in-one/tags before official support.

source here. https://github.com/querycap/istio/tree/master/build/jaegertracing

jpkrohling

I'll leave it open for the day, to give other maintainers the chance to review.

jpkrohling · 2020-10-21T11:46:25Z

CONTRIBUTING.md

+
+### Developing guidelines
+
+#### Building OCI Images for multiple arch (linux/arm64, linux/amd64)


This section needs some better formatting and grammar check (@objectiser?), but I'm fine in having this done by someone else or in a future PR.

Ashmita152 · 2020-10-22T11:35:27Z

cmd/all-in-one/Dockerfile

+FROM alpine:3.12 AS cert
+RUN apk add --update --no-cache ca-certificates
+
+FROM scratch AS base-image-release


Are we changing back the base image back to scratch ?

Good catch! No, we shouldn't change it back to scratch.

i asked.

Why we need to change to alpine for the release image

See #2545 for context.

Sure.
got info from #2116.
I have put it back.

jpkrohling

Dismissing the approval, as the base image shouldn't be changed.

Dismissing the approval, as the base image shouldn't be changed

Ashmita152 · 2020-10-22T13:01:48Z

cmd/all-in-one/Dockerfile


-FROM $base_image AS release
-ARG TARGETARCH=amd64
+FROM golang:1.15-alpine AS base-image-debug


I see you're using golang:1.15-alpine as base image for debug, but it will still have delve missing. I feel like we reverted back changes to this Dockerfile from my PR.

This PR here should not revert that PR.

Sure. i added it.

We could setup multi arch build for base images next

Ashmita152 · 2020-10-22T13:11:37Z

cmd/all-in-one/Dockerfile

 ENTRYPOINT ["/go/bin/dlv", "exec", "/go/bin/all-in-one-linux", "--headless", "--listen=:12345", "--api-version=2", "--accept-multiclient", "--log", "--"]
 CMD ["--sampling.strategies-file=/etc/jaeger/sampling_strategies.json"]
+
+FROM ${TARGET}


I have zero knowledge of docker buildx, curious what is the meaning of having FROM at the end of Dockerfile ?

It is not new of buildx.

FROM could be a stage name.

if TARGET = release, that means FROM release
if TARGET = debug, that means FROM debug

i learn this from istio Dockerfiles, which is used to switch default and distroless

Signed-off-by: Morlay <[email protected]>

jpkrohling

@Ashmita152, could you please review and test this as well? We should have no regressions when compared to your previous work.

jpkrohling · 2020-10-26T09:35:58Z

CONTRIBUTING.md

+it could be executed for local publish `all-in-one` images via:
+
+```shell
+TRAVIS_SECURE_ENV_VARS=true NAMESPACE=$(whoami) BRANCH=master ./scripts/travis/build-all-in-one-image.sh


Why is this TRAVIS_SECURE_ENV_VARS=true needed? Isn't there a make target that could be used instead of the Travis script?

I don't want to change too much in this pr.

We could refactor this in future

Ashmita152 · 2020-10-26T10:20:10Z

Sure I will give it a try today.

Ashmita152 · 2020-10-27T04:26:53Z

Hi @morlay looks great overall. I've left some minor comments. Would it be possible for you to share the travis run link where you tested your branch.

Ashmita152 · 2020-10-27T04:07:27Z

scripts/travis/build-all-in-one-image.sh

-        --tag $repo:latest cmd/all-in-one \
-        --build-arg base_image=localhost/baseimg:1.0.0-alpine-3.12 \
-        --build-arg debug_image=localhost/debugimg:1.0.0-golang-1.15-alpine \
-        --build-arg TARGETARCH=$GOARCH


@morlay I noticed all occurrences of TARGETARCH is removed from this script but I don't see the place it is passed as argument to Dockerfile from this script. TARGETARCH is being used in all-in-one's Dockerfile to decide which binary to copy inside container. Is this some arg which buildx populates internally by default ?

TARGETARCH and other platform args will be passed by buildx

https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope

Ashmita152 · 2020-10-27T04:12:10Z

scripts/travis/build-all-in-one-image.sh

@@ -7,6 +7,9 @@ BRANCH=${BRANCH:?'missing BRANCH env var'}
 # be overrided by passing architecture value to the script:
 # `GOARCH=<target arch> ./scripts/travis/build-all-in-one-image.sh`.
 GOARCH=${GOARCH:-$(go env GOARCH)}
+# local run with `TRAVIS_SECURE_ENV_VARS=true NAMESPACE=$(whoami) BRANCH=master ./scripts/travis/build-all-in-one-image.sh`
+NAMESPACE=${NAMESPACE:-jaegertracing}
+ARCHS="amd64 arm64 s390x"


Does travis support building images for s390x architecture ? Doesn't that means it will require base image of this architecture for alpine too ?

Yes. We could build base images by buildx in next step. now i have to put base images stages to all-in-one Dockerfile.

image golang and image alpine both support s390x too.
so the base images of jaeger could support s390x easier.

the magic is tonistiigi/binfmt who installs qemu

Ashmita152 · 2020-10-27T04:20:58Z

scripts/travis/setup-docker-buildx.sh

+
+# enabled qemu if needed
+if [[ ! $(docker buildx inspect default | grep Platforms) == *arm64* ]]; then
+  docker run --privileged --rm tonistiigi/binfmt --install all


I noticed you're using tonistiigi/binfmt image. I guess this is because it has images for all architecture which we need. Can we use append the tag too tonistiigi/binfmt:<tag> Right now it is pulling latest tag and the owner updating the latest image may break Jaeger travis CI.

tonistiigi/binfmt is not provide the images we need for architecture.

it just install qemu as simulator for target arch, which let us could run arm64 binary on amd64 host. (but it is a very slow way, and we don't use this for compiling)

the images alpine:3.12, you could see the arch list in docker hub.

This means docker hub store each image for each arch.

we could pull the target arch image to local using --platform

docker pull --platform=linux/arm64 alpine:3.12 docker pull --platform=linux/amd64 alpine:3.12 docker pull --platform=linux/s390x alpine:3.12

With buildx, the FROM alpine:3.12 in Dockerfile, will pull all images of target archs.
And in RUN apt add git, apt (arm64 binary) need to execute on matched arch (arm64) host.
This why we need qemu. with qemu we could run apt (arm64 binary) on amd64 host.

And we don't need the tag. it is a official tool in buildx doc. when this tool is stable, i think it will merge back to docker's namespace.

I mean you are using tonistiigi/binfmt instead of official image linuxkit/binfmt Is there any particular reason for that ?

no special reason, just it is on README of buildx Which is widely used by github actions https://github.com/docker/setup-qemu-action.

https://github.com/docker/buildx#building-multi-platform-images

May be the version of qemu.

tonistiigi/binfmt with [email protected]
linuxkit/binfmt with [email protected]

And linuxkit/binfmt not provide OCI images for multi arch. it could not be used on a non-amd64 host.

Ahh I see, thank you @morlay that answers my question.

morlay · 2020-10-27T05:37:59Z

@Ashmita152 i tested on local by (need to setup buildx)

TRAVIS_SECURE_ENV_VARS=true NAMESPACE=$(whoami) BRANCH=v1.20.0 ./scripts/travis/build-all-in-one-image.sh

Ashmita152 · 2020-10-27T07:01:59Z

I tested the all-in-one's Dockerfile changes locally, it worked for me like charm. Thank you @morlay for answering all my questions. I am not sure what will be the right way to test travis part.

morlay · 2020-10-27T07:49:56Z

./scripts/travis/build-all-in-one-image.sh called in pr checks, only difference is not push to hub.

jpkrohling · 2020-10-27T09:20:35Z

@yurishkuro, would you like to take a look? The only point that I had was about the usage of travis-specific env vars for local building/publishing, but if @morlay promises to work on that on a follow-up PR, I'm OK with that.

jpkrohling · 2020-11-02T09:00:02Z

ping @yurishkuro

yurishkuro · 2020-11-02T17:00:29Z

It seems to be that there are a lot of changes to the Dockerfiles that revert the previous changes for debug images.

morlay · 2020-11-02T17:17:13Z

@yurishkuro it is not revert.

Just added stages of base images to support multi arch for all-in-one images, and enhance the build stages to avoid redundant configuration.

We could to do next for support multi arch for all component images.

build & push base & debug muti images
refactor Dockerfile of all-in-one and other components.

This pr is since July. before debug images created.
I don't want change the scope for all-in-one.

If you couldn't like those changes. We could close this pr.
And re-open a new pr to setup from base images.

yurishkuro · 2020-11-02T18:15:06Z

It most definitely reverts a bunch of stuff. I realize it is unfortunate that the two PRs overlapped in time, but the debug images PR did a lot of refactoring on the images to DRY them, and those changes are being reverted here (for example, all the copying of cert files, which is currently done only once in the base images).

I think we need to rethink how buildx should work with the new refactored setup, simply merging this old PR on top is probably not going to work.

Ashmita152 · 2020-11-03T00:37:38Z

I was thinking to pick work on multi-arch images for base images after this PR gets merged. I can pick it before if you think that will help.

morlay · 2020-11-03T00:54:47Z

@Ashmita152 @yurishkuro

if we want to setup from base images, and for all components images

Stages could be

Stage 0 - switch docker build to docker buildx build

updates all docker cli
--platform
Dockerfile，remove default value of platform args like TARGETARCH
Makefile enhancement and updates CI scripts

Base images not need to publish docker.io.
Using local registery works well too.
But will be a extra setup step for local buld.

Stage 1 - Dockerfile enhancement

multi stage for base image switching

We could close this pr, and re open 2 or more prs to got final goals

morlay requested a review from a team as a code owner July 2, 2020 02:04

morlay requested a review from vprithvi July 2, 2020 02:04

morlay force-pushed the master branch from 8a49e15 to 0df8ce6 Compare July 2, 2020 02:11

morlay force-pushed the master branch from 0df8ce6 to fcf7055 Compare July 8, 2020 01:47

morlay force-pushed the master branch from fcf7055 to 4dad837 Compare July 8, 2020 02:10

morlay force-pushed the master branch 4 times, most recently from f001655 to 148b831 Compare July 8, 2020 04:03

pavolloffay reviewed Jul 9, 2020

View reviewed changes

morlay force-pushed the master branch from 148b831 to 7584d5b Compare July 9, 2020 09:53

morlay requested a review from pavolloffay July 10, 2020 01:28

pavolloffay changed the title ~~Setup docker buildx~~ Setup docker buildx for all-in-one Jul 10, 2020

yurishkuro reviewed Jul 10, 2020

View reviewed changes

morlay force-pushed the master branch from 7584d5b to 27b0f28 Compare July 11, 2020 01:03

pavolloffay previously approved these changes Jul 31, 2020

View reviewed changes

morlay mentioned this pull request Sep 29, 2020

Multi arch docker build jaegertracing/jaeger-operator#1203

Merged

morlay force-pushed the master branch from cc6dbad to 63ead09 Compare October 12, 2020 16:04

morlay force-pushed the master branch from 1eb51cc to 094b32f Compare October 21, 2020 09:41

jpkrohling mentioned this pull request Oct 21, 2020

Create debug docker images for jaeger backends #2545

Merged

jpkrohling previously approved these changes Oct 21, 2020

View reviewed changes

Ashmita152 reviewed Oct 22, 2020

View reviewed changes

jpkrohling reviewed Oct 22, 2020

View reviewed changes

morlay force-pushed the master branch from 094b32f to bc11486 Compare October 22, 2020 12:46

Ashmita152 reviewed Oct 22, 2020

View reviewed changes

morlay force-pushed the master branch from bc11486 to 24aa9de Compare October 22, 2020 13:49

albertteoh mentioned this pull request Oct 25, 2020

push official images to quay.io in addition to docker.io #2588

Closed

build: setup buildx for all-in-ones

260bb4a

Signed-off-by: Morlay <[email protected]>

morlay force-pushed the master branch from 4cd381b to 260bb4a Compare October 26, 2020 00:37

jpkrohling reviewed Oct 26, 2020

View reviewed changes

Ashmita152 suggested changes Oct 27, 2020

View reviewed changes

Merge branch 'master' into master

2376d75

morlay closed this Nov 3, 2020

morlay mentioned this pull request Nov 3, 2020

added multi arch images builds support for make file #2611

Closed

2 tasks


		### Developing guidelines

		#### Building OCI Images for multiple arch (linux/arm64, linux/amd64)

Setup docker buildx for all-in-one #2325

Setup docker buildx for all-in-one #2325

Conversation

morlay commented Jul 2, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

pavolloffay commented Jul 7, 2020

pavolloffay commented Jul 7, 2020

morlay commented Jul 7, 2020

morlay commented Jul 8, 2020 • edited Loading

codecov bot commented Jul 8, 2020 • edited Loading

Codecov Report

pavolloffay commented Jul 8, 2020

morlay commented Jul 9, 2020 • edited Loading

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay commented Jul 31, 2020

pavolloffay left a comment

Choose a reason for hiding this comment

davidsbond commented Oct 7, 2020

morlay commented Oct 9, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ashmita152 Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

morlay Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ashmita152 commented Oct 26, 2020

Ashmita152 commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

morlay Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay commented Oct 27, 2020

Ashmita152 commented Oct 27, 2020

morlay commented Oct 27, 2020

jpkrohling commented Oct 27, 2020

jpkrohling commented Nov 2, 2020

yurishkuro commented Nov 2, 2020

morlay commented Nov 2, 2020

yurishkuro commented Nov 2, 2020

Ashmita152 commented Nov 3, 2020

morlay commented Nov 3, 2020 • edited Loading

morlay commented Jul 2, 2020 •

edited

Loading

morlay commented Jul 8, 2020 •

edited

Loading

codecov bot commented Jul 8, 2020 •

edited

Loading

morlay commented Jul 9, 2020 •

edited

Loading

morlay Jul 9, 2020 •

edited

Loading

Ashmita152 Oct 22, 2020 •

edited

Loading

morlay Oct 22, 2020 •

edited

Loading

morlay Oct 27, 2020 •

edited

Loading

morlay Oct 27, 2020 •

edited

Loading

morlay Oct 27, 2020 •

edited

Loading

morlay Oct 27, 2020 •

edited

Loading

morlay commented Nov 3, 2020 •

edited

Loading