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

Add docker buildx build to the Makefile to build and push multi-arch docker images #1915

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

odidev
Copy link
Contributor

@odidev odidev commented Jun 13, 2022

Signed-off-by: odidev [email protected]

Ⅰ. Describe what this PR does

Add docker buildx build to the Makefile to build and push multi-arch docker images and update the documentation as well.

Ⅱ. Does this pull request fix one issue?

Helps to justify #1337

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

No tests.

Ⅳ. Describe how to verify it

make docker-buildx-all

Ⅴ. Special notes for reviews

N/A

@cheyang
Copy link
Collaborator

cheyang commented Jun 13, 2022

@odidev Could you take a look at failure of the build ? https://github.com/fluid-cloudnative/fluid/runs/6862549795?check_suite_focus=true

@cheyang
Copy link
Collaborator

cheyang commented Jun 13, 2022

@cheyang cheyang requested review from zwwhdls and cheyang June 14, 2022 02:25
@odidev odidev force-pushed the BUILDX_ARM64 branch 2 times, most recently from d20794e to 396f612 Compare June 14, 2022 06:13
docker buildx build --push --platform linux/amd64,linux/arm64 --no-cache . -f docker/Dockerfile.csi -t ${CSI_IMG}:${GIT_VERSION}

docker-buildx-init-users:
docker buildx build --push --platform linux/amd64,linux/arm64 --no-cache charts/alluxio/docker/init-users -t ${INIT_USERS_IMG}:${GIT_VERSION}

Copy link
Collaborator

@cheyang cheyang Jun 14, 2022

Choose a reason for hiding this comment

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

Seems that empty line 263 cause

make: *** No rule to make target 'v0.8.0-f665a4f', needed by 'build'.  Stop.
Error: Process completed with exit code 2.

@@ -16,14 +16,19 @@ RUN apk add --update curl tzdata iproute2 bash libc6-compat vim && \
cp /usr/share/zoneinfo/Asia/Shanghai /etc/localtime && \
echo "Asia/Shanghai" > /etc/timezone

RUN curl -o helm-v3.0.3-linux-amd64.tar.gz http://aliacs-k8s-cn-hongkong.oss-cn-hongkong.aliyuncs.com/public/pkg/helm/helm-v3.0.3-linux-amd64.tar.gz && \
RUN if [ `uname -m` == "aarch64" ] ; then curl -o helm-v3.0.3-linux-arm64.tar.gz https://get.helm.sh/helm-v3.7.2-linux-arm64.tar.gz && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using https://get.helm.sh/helm-v3.0.3-linux-arm64.tar.gz ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended the PR to use helm v3.0.3 for ARM64.

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #1915 (8f5251e) into master (c768afe) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 8f5251e differs from pull request most recent head e1ead04. Consider uploading reports for the commit e1ead04 to get more accurate results

@@            Coverage Diff             @@
##           master    #1915      +/-   ##
==========================================
+ Coverage   53.50%   53.53%   +0.02%     
==========================================
  Files         285      285              
  Lines       20387    20387              
==========================================
+ Hits        10908    10914       +6     
+ Misses       8307     8304       -3     
+ Partials     1172     1169       -3     
Impacted Files Coverage Δ
pkg/ddc/goosefs/metadata.go 64.28% <0.00%> (+3.29%) ⬆️

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 c768afe...e1ead04. Read the comment docs.

@@ -16,14 +16,19 @@ RUN apk add --update curl tzdata iproute2 bash libc6-compat vim && \
cp /usr/share/zoneinfo/Asia/Shanghai /etc/localtime && \
echo "Asia/Shanghai" > /etc/timezone

RUN curl -o helm-v3.0.3-linux-amd64.tar.gz http://aliacs-k8s-cn-hongkong.oss-cn-hongkong.aliyuncs.com/public/pkg/helm/helm-v3.0.3-linux-amd64.tar.gz && \
RUN if [ `uname -m` == "aarch64" ] ; then curl -o helm-v3.0.3-linux-arm64.tar.gz https://get.helm.sh/helm-v3.0.3-linux-arm64.tar.gz && \
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TARGETARCH works well while using docker buildx build. But normal docker build fails to detect TARGETARCH in the RUN clause and thus the helm tar file in the Dockerfiles isn’t getting curled. I have observed the same on both ARM64 and AMD64 platforms with docker build.

Step 4/4 : RUN curl -o helm-v3.0.3-linux-${TARGETARCH}.tar.gz https://get.helm.sh/helm-v3.0.3-linux-${TARGETARCH}.tar.gz &&     tar -xvf helm-v3.0.3-linux-${TARGETARCH}.tar.gz &&     mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm &&     chmod u+x /usr/local/bin/ddc-helm &&     rm -f helm-v3.0.3-linux-${TARGETARCH}.tar.gz
 ---> Running in 4aeeac5ded68
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   215  100   215    0     0    817      0 --:--:-- --:--:-- --:--:--   817
tar: invalid tar magic
The command '/bin/sh -c curl -o helm-v3.0.3-linux-${TARGETARCH}.tar.gz https://get.helm.sh/helm-v3.0.3-linux-${TARGETARCH}.tar.gz &&     tar -xvf helm-v3.0.3-linux-${TARGETARCH}.tar.gz &&     mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm &&     chmod u+x /usr/local/bin/ddc-helm &&     rm -f helm-v3.0.3-linux-${TARGETARCH}.tar.gz' returned a non-zero code: 1

Copy link
Member

Choose a reason for hiding this comment

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

add ARG TARGETARCH in dockerfile and have a try? It works well in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already used ARG TARGETARCH in my Dockerfiles but was still facing the same issue.

Actually, in docker build, we need to specify flag --build-arg TARGETARCH=amd64 along with docker build command to use TARGETARCH in Dockerfile.

$ sudo docker build --build-arg TARGETARCH=amd64 -f docker/Dockerfile.application -t <NAME> .

I will amend the PR to use --build-arg flag with docker build commands.

Copy link
Member

Choose a reason for hiding this comment

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

Misunderstood. I think we can explicitly add --build-arg in docker build commands and make dockerfile more clear. What do you think @cheyang ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. I also prefer --build-arg to make the dockerfile easy to understand.

@odidev
Copy link
Contributor Author

odidev commented Jun 14, 2022

I have amended the PR. Kindly have a look and suggest.

@@ -165,22 +176,22 @@ update-api-doc:

# Build the docker image
docker-build-dataset-controller: generate gen-openapi fmt vet
docker build --no-cache . -f docker/Dockerfile.dataset -t ${DATASET_CONTROLLER_IMG}:${GIT_VERSION}
docker build --no-cache --build-arg TARGETARCH=${ARCH} . -f docker/Dockerfile.dataset -t ${DATASET_CONTROLLER_IMG}:${GIT_VERSION}
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 set default value amd64 for ARCH in front ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added.

Makefile Outdated

docker-build-all: ${DOCKER_BUILD}
docker-push-all: ${DOCKER_PUSH}
docker-buildx-all: ${DOCKER_BUILDX}
Copy link
Member

Choose a reason for hiding this comment

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

When building multi-platform images using buildx, we need to create a builder for it, refer to https://docs.docker.com/buildx/working-with-buildx/#build-multi-platform-images

I think docker-buildx-all better to add the whole process.

Furthermore, maybe name docker-buildx-all-push is more suitable for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docker buildx uses qemu emulation to build images for other platforms than the host.

I already have docker installed on my system. When I built multi-arch images, I simply had to run qemu emulation before using docker buildx, as below:

$ docker run --rm --privileged multiarch/qemu-user-static --reset -p yes 
$ docker buildx build --platform linux/amd64,linux/arm64 ….. 

You want me to add qemu installation along with the docker buildx commands...am I correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can manually run qemu emulation in short time. And Let the users know through documentation.
In long time I agree that we can run make docker-buildx-all-push from scratch. I see the similar implementation in https://github.com/openyurtio/openyurt/blob/master/Makefile#L129-L136 .

WDYT? @odidev and @zwwhdls

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. We can add a point in how_to_develop.md to run qemu emulation before using docker buildx build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind to rename docker-buildx-all to docker-buildx-all-push ? I think it's more clear for the developer. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cheyang Sure, added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks very much!

@cheyang cheyang merged commit 36d9956 into fluid-cloudnative:master Jun 16, 2022
@cheyang
Copy link
Collaborator

cheyang commented Jun 16, 2022

@odidev Thanks very much for your contribution!

@odidev
Copy link
Contributor Author

odidev commented Jun 16, 2022

@cheyang , Thank you for the PR merge. May I know when should we expect the official multi-arch docker images release at dockerhub?

@cheyang
Copy link
Collaborator

cheyang commented Jun 16, 2022

@cheyang , Thank you for the PR merge. May I know when should we expect the official multi-arch docker images release at dockerhub?

Hopefully late in June. But we have a blocker that our major runtime Alluxio which only still support amd64 version officially. But seems that it can work Alluxio/alluxio#12704 (comment)

If you are interested, could you help on how to support Alluxio (https://github.com/Alluxio/alluxio/blob/master/integration/docker/Dockerfile) on ARM platform. The build scripts are in https://github.com/fluid-cloudnative/fluid/tree/master/tools/alluxio

@odidev
Copy link
Contributor Author

odidev commented Jun 16, 2022

Okay. I had raised an issue earlier in Fluid, targeting that the unit tests are failing for Linux/ARM64, and I had attached the failing logs in this issue.
May I know, are the test failures reflected due to alluxio dependency in Fluid?

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.

3 participants