added multi arch images builds support for make file #2611

morlay · 2020-11-03T03:44:43Z

Which problem is this PR solving?

to support Multiarch docker images #2292, split from Setup docker buildx for all-in-one #2325

Short description of the changes

publishable multi-arch base images
added make build-binaries-linux-archs for all binaries for multi arch
updates makefile to switch docker build to docker buildx build for jaeger images
removes default value of Dockerfile arg TARGETARCH

local run（need buildx installed）

make build-binaries-linux-archs
DOCKER_BASE_NAMESPACE=querycapjaegertracing DOCKER_NAMESPACE=querycapjaegertracing make docker-push

# if not set  DOCKER_BASE_NAMESPACE
# need to 
# - run local registry to push base images 
# - create builder with --driver-opt=network=host (for let buildkit could access `localhost:5000` to pull base images)
docker run -d -p 5000:5000 --name registry registry:2
docker buildx create --use --driver-opt=network=host

images https://hub.docker.com/u/querycapjaegertracing

upstream cassandra not provide s390x image, so now just amd64 & arm64 first.

This PR should

no break changes of make docker and current ci flow. (switch to buildx but still build for amd64 only)
no Dockfile changes.

Next PRs will add

make docker-images-all-in-one
multi arch images pubishing workflow for ci

codecov · 2020-11-03T04:00:46Z

Codecov Report

Merging #2611 (e9de19f) into master (a6e29ad) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2611   +/-   ##
=======================================
  Coverage   95.73%   95.73%           
=======================================
  Files         216      216           
  Lines        9588     9588           
=======================================
  Hits         9179     9179           
  Misses        336      336           
  Partials       73       73

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	`94.78% <0.00%> (-1.74%)`	⬇️
cmd/query/app/server.go	`90.16% <0.00%> (+1.63%)`	⬆️

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 a6e29ad...e9de19f. Read the comment docs.

jpkrohling · 2020-11-03T09:48:01Z

@Ashmita152 would you like to take a look and review this PR?

Ashmita152 · 2020-11-03T11:23:43Z

Hi @jpkrohling sure I will try this PR today.

Ashmita152 · 2020-11-03T16:05:55Z

scripts/travis/setup-docker-buildx.sh

+# run a local registry for base images push
+# have to do this for multi-arch images
+# multi arch images could not --load
+docker run -d -p 5000:5000 --name registry registry:2


I am not sure but I'll prefer to push the base image to dockerhub under our organisation instead of starting our own docker-registry in every CI/CD run. That will remove all localhost:5000 too.

Agree. I'm actually wondering if we need docker buildx at all, given that buildah can also generate multi-arch images:

https://www.youtube.com/watch?v=SYJgkkjqd7s

From what I understand, we don't need qemu or a virtual machine to perform the build, given that we can produce our binaries for the target architectures anyway...

@Ashmita152 CI could be enhanced in next.

We need make create-baseimg-debugimg before other images build.

here is a important port. docker images in host can not store multi-arch image, but a registry could. (why we need to push base images to somewhere)

Now in ci, no access to docker.io/jaegertracing for PR. May publish base images in other ci job

@jpkrohling need to research for this.

Without qemu, RUN apk add ca may not be executable.
Means the base images will not be created.

it could be upgrade in future once this ready containers/buildah#1590.
Now I think we have to use buildx

Right, forgot about the RUN apk add ca command. Other than that, it would just work, right?

it may cause a secure issue in PR

Why would it? We should have those as secrets in Travis and they are suppressed from the output in the CI.

seems Docker username & password not provided for PR

If you set the appropriate env vars in the Travis job for your fork, it should work. You'd have to override the namespace, so that it won't attempt to push to docker.io/jaegertracing/all-in-one, but quay.io/youraccount/all-in-one.

Why would it? We should have those as secrets in Travis and they are suppressed from the output in the CI.

But i could create some PR and just added some docker push to overwrite published images of docker.io/jaegertracing. it not set, the PR may not pass bacause of base image pushing failed.

So I prefer use a local registry
as the said https://github.com/docker/build-push-action#local-registry
for testing purposes, like PR testings.

@jpkrohling

Or could we create another namespace docker.io/jaegertracing-dev or quay.io/jaegertracing-dev

docker.io/jaegertracing only for master branch
docker.io/jaegertracing-dev for testing of PRs or other branches.

You'd still have to override the namespace to jaegertracing-dev... So, at least for now, you can override to your Dockerhub/Quay namespace.

@jpkrohling

in my fork i could override to my Dockerhub/Quay namespace.
but in PRs of jaegertracing, need namespace jaegertracing-dev to receive images from PRs.

Without a local registry. base images need to pushing to Dockerhub/Quay in PRs.

Makefile

Ashmita152 · 2020-11-03T16:39:17Z

One thing I noticed that if we run docker buildx install in the beginning, then we don't need to update all lines from docker build to docker buildx build

$ docker buildx install
$ docker build --help | head -4

Usage:	docker buildx build [OPTIONS] PATH | URL | -

Start a build

Reference: https://docs.docker.com/buildx/working-with-buildx/#set-buildx-as-the-default-builder

Thought it would be interesting.

morlay · 2020-11-03T17:10:28Z

@Ashmita152

This feature is for local switch for different projects.

Flags of docker buildx build not be compatible with docker build

--load & --push only for docker buildx build
--platform of docker build not support csv format linux/arm64,linux/amd64

# ensure buildx not as default build
docker buildx uninstall

docker buildx build --help
docker build --help

morlay · 2020-11-11T07:57:08Z

@jpkrohling @Ashmita152

is any plan about this?

if you don't want to mv to buildx by default. we could refactor Makefile like this https://github.com/kiali/kiali/blob/master/make/Makefile.container.mk#L54-L61

For local registry in CI, we could split the base images publishing to another CI workflow.

morlay · 2020-11-12T04:18:25Z

@jpkrohling @Ashmita152

I refactor again.
Now we could keep docker build as default. CI workflow will not change too much.
The base images no need to pre publish. so no need local registry any more.

only change is the final publishing

# build all binaries for multi-arch
make build-binaries-linux-archs

# enable BUILDX_PUSH=true to switch buildx and build && push for multi-arch
DOCKER_NAMESPACE="jaegertracing" make dockerx-push

For multi namespace or multi tags, we could (in another PR) updates all -t $(DOCKER_NAMESPACE)/*:${DOCKER_TAG} to

$(foreach namespace,$(DOCKER_NAMESPACE),$(foreach tag,$(DOCKER_TAG),--tag=${namespace}/*:${tag}))

Then we could just run

DOCKER_NAMESPACE="jaegertracing quay.io/jaegertracing" DOCKER_TAG="latest 1.20.0 1.20 1" make dockerx-push

Makefile

docker/Makefile

yurishkuro · 2020-12-01T02:59:13Z

Makefile

@@ -390,6 +405,10 @@ docker-push:
 		docker push $(DOCKER_NAMESPACE)/jaeger-$$component ; \
 	done

+.PHONY: dockerx-push
+dockerx-push: setup-buildx-builder
+	BUILDX_PUSH=true $(MAKE) docker-images-only


Var before command becomes env var, after command it becomes make var. Do you want $(MAKE) docker-images-only BUILDX_PUSH=true?

env var could overwrites make var BUILDX_PUSH when define with ?=.
will same as $(MAKE) docker-images-only BUILDX_PUSH=true.

i will update this for style.

yurishkuro · 2020-12-01T03:05:11Z

Makefile

+TARGET_ARCHS ?= amd64 arm64
+
+BUILDX_PUSH ?= false
+DOCKER_BUILD ?= docker build --build-arg=TARGETARCH=$(GOARCH)


TARGETARCH is defined here but not in L76. How does it work, especially if the default value is removed from all Dockerfiles?

Also, wouldn't this line be immediately evaluated with the current value of $(GOARCH) ?

TARGETARCH is a platform args, which will be auto pass to Dockerfile when we use buildx.
https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope

nope. here not use :=, so $(GOARCH) as a ref value, will be evaluated when $(DOCKER_BUILD) used.

yurishkuro · 2020-12-01T03:05:35Z

Makefile

+	DOCKER_BUILD = DOCKER_CLI_EXPERIMENTAL="enabled" docker buildx build --builder=jaeger-builder --push $(foreach arch,${TARGET_ARCHS},--platform=linux/${arch})
+endif
+
+# make sure variables could pass to includes


what is the meaning of this comment?

make sure sub makefiles could use the make variables defined in root makefile.
how about this?

yurishkuro · 2020-12-02T01:09:07Z

are these warnings expected? https://github.com/jaegertracing/jaeger/pull/2611/checks?check_run_id=1483365080#step:11:375

yurishkuro · 2020-12-02T01:10:21Z

This doesn't look good either, pulling all-in-one while trying to build it (cc @Ashmita152 )

yurishkuro · 2020-12-02T01:13:29Z

Makefile

@@ -390,6 +405,10 @@ docker-push:
 		docker push $(DOCKER_NAMESPACE)/jaeger-$$component ; \
 	done

+.PHONY: dockerx-push
+dockerx-push: setup-buildx-builder


Is this a WIP? The all-in-one build is only doing amd64, I assume because BUILDX_PUSH=false by default. Wouldn't we want x-platform build to be the default, rather than having a separate target?

all-in-one is not in make docker, so i haven't add it in make dockerx-push

now all-in-one split into another job.
we could do all-in-one next to support multi-arch

morlay · 2020-12-02T01:40:12Z

@yurishkuro

are these warnings expected? https://github.com/jaegertracing/jaeger/pull/2611/checks?check_run_id=1483365080#step:11:375

just because there is no ARG TARGETARCH in docker/base/Dockerfile && docker/debug/Dockerfile.
this will not break anything

morlay · 2020-12-02T02:01:56Z

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

@@ -63,8 +63,8 @@ repo=jaegertracing/all-in-one-debug
 docker build -f cmd/all-in-one/Dockerfile \
        --target debug \
        --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 \


@yurishkuro

fixed #2611 (comment)

we may move this into makefile too.
this part is not easy to maintain when base image updates (cc @Ashmita152)

morlay · 2020-12-02T02:19:03Z

no idea why es testing failed.

jpkrohling · 2020-12-07T10:24:20Z

Sharing something I've read this weekend: https://carlosbecker.com/posts/multi-platform-docker-images-goreleaser-gh-actions/
Note how they use docker manifest instead of buildx: the binaries are still build outside of Docker, via regular Go builds.

morlay · 2020-12-08T00:50:35Z

@jpkrohling

with DOCKER_CLI_EXPERIMENTAL=enabled docker manifest should publish arch suffix images -arm64 -amd64 first.
docker manifest is EXPERIMENTAL feature too.

it will be more complex for publishing workflow, especially publishing quay.io and docker.io both.

after migrated github actions, setup buildx will be very easy

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

morlay · 2021-01-08T05:17:44Z

waiting for a little long. I have to setup workflow to release most versioned jaeger images

with some hacks

https://github.com/querycap/jaeger

GaruGaru · 2021-06-29T20:29:50Z

Any update on this ?
The changes looks similar to what is proposed on #2948

yurishkuro · 2021-06-29T20:43:29Z

Yes, I think we should close this in favor of #2948, which is about to be merged.

morlay requested a review from a team as a code owner November 3, 2020 03:44

morlay requested a review from pavolloffay November 3, 2020 03:44

mergify bot assigned jpkrohling Nov 3, 2020

mergify bot requested a review from jpkrohling November 3, 2020 03:45

morlay force-pushed the multi-arch branch 6 times, most recently from 59b3b49 to 58c6a9b Compare November 3, 2020 05:14

jpkrohling mentioned this pull request Nov 3, 2020

Setup docker buildx for all-in-one #2325

Closed

Ashmita152 reviewed Nov 3, 2020

View reviewed changes

morlay force-pushed the multi-arch branch from 58c6a9b to 502f13e Compare November 11, 2020 07:50

morlay force-pushed the multi-arch branch from 502f13e to d97cc0f Compare November 12, 2020 03:56

morlay force-pushed the multi-arch branch 6 times, most recently from 92a485d to ee921d2 Compare November 12, 2020 06:38

jpkrohling reviewed Nov 12, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jpkrohling reviewed Nov 12, 2020

View reviewed changes

docker/Makefile Outdated Show resolved Hide resolved

morlay force-pushed the multi-arch branch from ee921d2 to 8d8308e Compare November 12, 2020 14:03

morlay force-pushed the multi-arch branch 2 times, most recently from a341799 to 66a4599 Compare December 1, 2020 02:40

yurishkuro reviewed Dec 1, 2020

View reviewed changes

morlay force-pushed the multi-arch branch 3 times, most recently from 91f3ec0 to f3e27fe Compare December 2, 2020 00:05

yurishkuro reviewed Dec 2, 2020

View reviewed changes

morlay commented Dec 2, 2020

View reviewed changes

morlay force-pushed the multi-arch branch from 96d3f21 to 3451818 Compare December 2, 2020 02:04

morlay force-pushed the multi-arch branch 2 times, most recently from 739170c to 76d9a7c Compare December 3, 2020 05:49

morlay force-pushed the multi-arch branch 2 times, most recently from a2f6c04 to 6fedb81 Compare December 10, 2020 02:12

morlay force-pushed the multi-arch branch from 6fedb81 to f690fbf Compare January 7, 2021 06:12

morlay added 2 commits January 7, 2021 15:33

build: setup buildx

a8666ba

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

update base debug images in scripts/travis/build-all-in-one-image.sh

e9de19f

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

morlay force-pushed the multi-arch branch from f690fbf to e9de19f Compare January 7, 2021 07:33

yurishkuro closed this Jun 29, 2021

Ashmita152 mentioned this pull request Aug 16, 2021

Refactor CI jobs to use goreleaser #3209

Closed

added multi arch images builds support for make file #2611

added multi arch images builds support for make file #2611

Conversation

morlay commented Nov 3, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Nov 3, 2020 • edited Loading

Codecov Report

jpkrohling commented Nov 3, 2020

Ashmita152 commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

morlay Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Nov 11, 2020 • edited Loading

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 commented Nov 3, 2020

morlay commented Nov 3, 2020 • edited Loading

morlay commented Nov 11, 2020 • edited Loading

morlay commented Nov 12, 2020 • edited Loading

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 Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

yurishkuro commented Dec 2, 2020

yurishkuro commented Dec 2, 2020

Choose a reason for hiding this comment

morlay Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

morlay commented Dec 2, 2020

morlay Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

morlay commented Dec 2, 2020

jpkrohling commented Dec 7, 2020

morlay commented Dec 8, 2020 • edited Loading

morlay commented Jan 8, 2021 • edited Loading

GaruGaru commented Jun 29, 2021

yurishkuro commented Jun 29, 2021

morlay commented Nov 3, 2020 •

edited

Loading

codecov bot commented Nov 3, 2020 •

edited

Loading

morlay Nov 3, 2020 •

edited

Loading

morlay Nov 3, 2020 •

edited

Loading

morlay Nov 11, 2020 •

edited

Loading

morlay commented Nov 3, 2020 •

edited

Loading

morlay commented Nov 11, 2020 •

edited

Loading

morlay commented Nov 12, 2020 •

edited

Loading

morlay Dec 1, 2020 •

edited

Loading

morlay Dec 1, 2020 •

edited

Loading

morlay Dec 2, 2020 •

edited

Loading

morlay Dec 2, 2020 •

edited

Loading

morlay commented Dec 8, 2020 •

edited

Loading

morlay commented Jan 8, 2021 •

edited

Loading