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

Jump to bottom

Speed up buildx process #1267

Merged
merged 1 commit into from
Oct 21, 2020
Merged

Speed up buildx process #1267

merged 1 commit into from
Oct 21, 2020

Conversation

morlay
Copy link
Contributor

@morlay morlay commented Oct 20, 2020 •
edited
Loading

avoid to run gobuild with qemu to speed up multi arch build.

added make dockerx for local run.

@codecov
Copy link

codecov bot commented Oct 20, 2020 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1267   +/-   ##
=======================================
  Coverage   87.38%   87.38%           
=======================================
  Files          90       90           
  Lines        4913     4913           
=======================================
  Hits         4293     4293           
  Misses        457      457           
  Partials      163      163

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 d7f244f...f8b74fc. Read the comment docs.

jpkrohling
jpkrohling reviewed Oct 20, 2020
View reviewed changes
build/Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -77,6 +78,10 @@ gobuild:
docker:
@[ ! -z "$(PIPELINE)" ] || docker build --build-arg=GOPROXY=${GOPROXY} --file build/Dockerfile -t "$(BUILD_IMAGE)" .

.PHONY: dockerx
dockerx:
@[ ! -z "$(PIPELINE)" ] || docker buildx build --progress=plain --build-arg=GOPROXY=${GOPROXY} --platform=linux/arm64,linux/amd64 --file build/Dockerfile -t "$(BUILD_IMAGE)" .
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't ./.ci/publish-images.sh be changed to use this new target?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but how hard would it be to add s390? Is it just a matter of adding a new platform, or would it typically imply other changes as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

golang support s390x.
so just added platform linux/s390x.
without this pr

old way will build in qemu, which will be very slow. this pr is for fixing this

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 should, i missing the changes, update it later

@morlay
chore: enhance buildx process
f8b74fc 
Signed-off-by: Morlay <morlay.null@gmail.com>
@morlay
Copy link
Contributor Author

morlay commented Oct 20, 2020 •
edited
Loading

make dockerx (with linux/s930x too) results: https://hub.docker.com/r/morlay/jaeger-operator/tags

@jgehrcke i change a little for make docker to make e2e testing happy. maybe switch to buildx in future.

@morlay
Copy link
Contributor Author

morlay commented Oct 20, 2020

previous way means

We build both linux/amd64 host andlinux/arm64 host (simulated by qemu, which will be very slow for compiling.)

current changes means.

We build all on linux/amd64 host, and compiling to arm64 and arm64.

qemu is cool for executing, but is a disater for compiling. the costs show the difference.

image

jpkrohling
jpkrohling reviewed Oct 20, 2020
View reviewed changes
Makefile
@@ -75,7 +78,11 @@ gobuild:

.PHONY: docker
docker:
@[ ! -z "$(PIPELINE)" ] || docker build --build-arg=GOPROXY=${GOPROXY} --file build/Dockerfile -t "$(BUILD_IMAGE)" .
@[ ! -z "$(PIPELINE)" ] || docker build --build-arg=GOPROXY=${GOPROXY} --build-arg=TARGETARCH=$(GOARCH) --file build/Dockerfile -t "$(BUILD_IMAGE)" .
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything still using this target? Should they still be using this target? If all users should move to dockerx, then we can just replace this target with the new one (ie, the contents of dockerx goes into this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e2e testing still use this，the Dockerfile could be built by docker directly.

moving to dockerx could be next stage.
for testing, --push need to be --load

@jpkrohling
Copy link
Contributor

@rubenvp8510 if your upgrade e2e test gets merged after this one, you might want to align the building of the container with the changes from this PR.

@jpkrohling jpkrohling changed the title chore: enhance buildx process Speed up buildx process Oct 21, 2020
@mergify mergify bot merged commit 3d15e00 into jaegertracing:master Oct 21, 2020
@jpkrohling
Copy link
Contributor

Looks like this has caused the publish workflow to fail. Should I revert this last change, or are you able to take a look?

https://github.com/jaegertracing/jaeger-operator/runs/1285761358

@morlay
Copy link
Contributor Author

morlay commented Oct 21, 2020 •
edited
Loading

@jpkrohling could you check the username & password for push to docker.io/jaegertracing and quay.io/jaegertracing?

@jpkrohling
Copy link
Contributor

The credentials in the env vars were fine in the previous commit: https://github.com/jaegertracing/jaeger-operator/runs/1281540702

@morlay
Copy link
Contributor Author

morlay commented Oct 21, 2020

@jpkrohling

could you re run this https://github.com/jaegertracing/jaeger-operator/runs/1285761358?
sometimes may network issues.

@jpkrohling
Copy link
Contributor

The message is quite clear:

failed to solve: rpc error: code = Unknown desc = server message: insufficient_scope: authorization failed

But I'll trigger it again.

@morlay
Copy link
Contributor Author

morlay commented Oct 21, 2020

@jpkrohling

quay.io robot account need to set permissions for repo. it that changed?

image
image

I create a quay.io account, got authorization failed too without setting permissions.

now pushed https://quay.io/repository/morlay/jaeger-operator?tag=latest&tab=tags

@jpkrohling
Copy link
Contributor

It hasn't changed since the last successful commit.

@morlay
Copy link
Contributor Author

morlay commented Oct 21, 2020

Could you check the setting on quay.io ?

or re run the last success workflow https://github.com/jaegertracing/jaeger-operator/runs/1281540702.

this pr doesn't change the pushing flow.

@morlay
Copy link
Contributor Author

morlay commented Oct 21, 2020

Final pushing should like this.
image

But now failed on first pushing.
maybe the logon user to docker hub have no access to jaegertracing/jaeger-operator
image

@morlay morlay mentioned this pull request Oct 21, 2020
Merged
@morlay
Copy link
Contributor Author

morlay commented Oct 21, 2020 •
edited
Loading

here is a additional tag $(USER)/jaeger-operator:latest, which USER is unknown.

mergify bot pushed a commit that referenced this pull request Oct 21, 2020
@morlay
Publish container - dockerx should not use tag BUILD_IMAGE (#1270)
9091a47 
fix #1267
@morlay morlay mentioned this pull request Nov 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@morlay @jpkrohling