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

Docker images with arm64 support #86

Merged
merged 10 commits into from Jul 22, 2022
Merged

Docker images with arm64 support #86

merged 10 commits into from Jul 22, 2022

Conversation

mcbex
Copy link
Contributor

@mcbex mcbex commented May 2, 2022

I am still working on the image for the dev container but the testing process is slow so I will make a separate PR once that is ready.

@mcbex
Copy link
Contributor Author

mcbex commented May 2, 2022

The build error may be related to this issue: golang/go#51253

@mcbex
Copy link
Contributor Author

mcbex commented May 2, 2022

Also need to look more closely at these issues which were brought to my attention by Aron:

actions/checkout#760
actions/checkout#766

@@ -51,8 +51,11 @@ RUN chmod +x /usr/local/bin/docker-compose

# Install Go. Keep in sync with other Dockerfiles.
ENV GOLANG_VERSION 1.18
ENV GOLANG_DOWNLOAD_SHA256 e85278e98f57cdb150fe8409e6e5df5343ecb13cebf03a5d5ff12bd55a80264f
RUN curl -fsSL "https://dl.google.com/go/go${GOLANG_VERSION}.linux-amd64.tar.gz" -o golang.tar.gz \
RUN ARCH=$(if [ $(uname -m) = "aarch64" ]; then echo "arm64"; else echo "amd64"; fi) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, and above, I wonder if we should consider simply using an ARG ARCH=amd64 and optionally passing the architecture to the build. For example, see the Launcher dockerfile: https://github.com/rstudio/launcher/blob/main/jenkins/docker/Dockerfile.bionic#L1.

Using an ARG would keep the Dockerfile a bit cleaner and let us handle the shell logic elsewhere.

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 think the issue would be that when we install awscli and Go the URLs contain the architecture but it's not consistent: aarch64 and arm64 vs x86_64 and amd64. So passing in a single variable would not remove the need to resolve those URLs dynamically.... unless you're suggesting the part where we build the URLs should move out of the Dockerfile?

A default architecture hard-coded in the image would also mean that anyone not on an amd64 machine has to remember to pass the parameter every time. That really only affects me at the moment but it feels kind of clunky to require from users.

Maybe another solution is to have two Dockerfiles and separate build targets for each architecture?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking more along the lines of delegating the responsibility of determining architecture to the caller. In other words, instead of determining architecture in the docker file, you'd determine it in the justfile or in a script called by the justfile, and then you'd pass the ARCH arg to the docker build command.

Copy link
Member

Choose a reason for hiding this comment

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

I comment this above before I saw this comment. Docker BuildKit will set TARGETARCH to the architecture you're building for, so you won't have to pass any extra parameters. The info is pretty far down the page:

https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/

BUILDPLATFORM — matches the current machine. (e.g. linux/amd64)

BUILDOS — os component of BUILDPLATFORM, e.g. linux

BUILDARCH — e.g. amd64, arm64, riscv64

BUILDVARIANT — used to set ARM variant, e.g. v7

TARGETPLATFORM — The value set with --platform flag on build

TARGETOS - OS component from --platform, e.g. linux

TARGETARCH - Architecture from --platform, e.g. arm64

TARGETVARIANT

Whoever is building this Dockerfile would then use the BuildKit --platform argument which would take care of all of the architecture-related stuff, e.g. setting the environment variable and choosing the right multi-platform base image.

You would want to remove the --platform option in your Dockerfile and instead pass that in the command line, e.g. docker build --platform linux/amd64. When a platform argument isn't specified Docker will use the host architecture.

This is a really nice approach because consumers can use the same Docker image without worrying about architecture, and you only have to worry about one Dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks for the feedback @jonyoder @shepherdjerred

I briefly read some of the documentation on BuildKit features but I wasn't sure if using BuildKit was considered portable or not (since it looked pretty new). So good to know others are also looking at this and thinking about it!

Copy link
Member

Choose a reason for hiding this comment

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

BuildKit is now about 3 years old: https://docs.docker.com/engine/release-notes/18.09/

As far as I know, it's the best way to make cross-platform images. You are right that it's not completely mature, this issue shows the gaps: moby/moby#40379

I believe that BuildKit is the default builder now, so that at least shows that Docker considers it to be the way forward: docker/cli#3314

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL!

@mcbex
Copy link
Contributor Author

mcbex commented May 3, 2022

I'm not too familiar with VS code or developing in containers but I had to update how the dev container was installing Go modules to be the same as the other Dockerfile to get the image to build. I installed VS code and was able to connect to the container afterwards.

Happy to discuss the overall approach more as well.

@bdeitte
Copy link

bdeitte commented May 3, 2022

I'm not sure if this will help here, but I saw it come up in a ticket from @shepherdjerred for RSPM. https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/

@shepherdjerred
Copy link
Member

I'm not sure if this will help here, but I saw it come up in a ticket from @shepherdjerred for RSPM. https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/

Thanks for the mention Brian! The solution here looks similar to what I'm doing for RSPM.

@mcbex
Copy link
Contributor Author

mcbex commented May 11, 2022

I was able to remove the logic for downloading the Go tarball by using the TARGETARCH env var which simplified the Dockerfile quite a bit but I couldn't think of a better way to set the correct checksum. Other suggestions welcome.

@mcbex mcbex requested a review from jonyoder May 11, 2022 18:21
Copy link
Collaborator

@jonyoder jonyoder left a comment

Choose a reason for hiding this comment

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

One remaining question! Looks great!

.devcontainer/Dockerfile Show resolved Hide resolved
Comment on lines +57 to +58
then echo "7ac7b396a691e588c5fb57687759e6c4db84a2a3bbebb0765f4b38e5b1c5b00e"; \
else echo "e85278e98f57cdb150fe8409e6e5df5343ecb13cebf03a5d5ff12bd55a80264f"; fi) \
Copy link
Member

Choose a reason for hiding this comment

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

From what I've seen this is (unfortunately) the idiomatic way to do platform-related logic in Dockerfiles. One note though; explicitly check for amd64, don't assume that arm64 and amd64 are the only possible architectures.

RUN PATH="$PATH:/usr/local/go/bin" go install github.com/go-delve/delve/cmd/dlv@latest
RUN GO111MODULE=on PATH="$PATH:/usr/local/go/bin" GOPATH=/usr/local/gotools go get -v \
RUN GO111MODULE=on PATH="$PATH:/usr/local/go/bin" GOPATH=/usr/local/gotools go install \
Copy link
Member

Choose a reason for hiding this comment

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

Should GO111MODULE still be here? I thought it was removed in 1.18.

Copy link
Collaborator

@jonyoder jonyoder left a comment

Choose a reason for hiding this comment

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

Marking this as approved so I don't hold things up. If you're able to look into the GO111MODULE question and refactor the unames, this looks great to me!

@jonyoder
Copy link
Collaborator

@mcbex should we merge this?

@mcbex
Copy link
Contributor Author

mcbex commented Jul 20, 2022

Ugh my apologies I never saw these comments. Going to go over it again and make those changes!


RUN if [ "${TARGETARCH}" != "amd64" ] && [ "${TARGETARCH}" != "arm64" ]; then \
echo "${TARGETARCH} is not a supported architecture. Please use linux/amd64 or linux/arm64"; \
exit 1; fi
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 don't love this but realized while testing that waiting until the later build stages to discover a particular platform is unsupported is annoying. I couldn't find a built in mechanism to allowlist architectures but happy to hear better ideas.

Copy link
Member

Choose a reason for hiding this comment

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

Hm I've never thought about restricting architectures like this, but it does make sense.

@mcbex
Copy link
Contributor Author

mcbex commented Jul 22, 2022

@jonyoder Let me know if you see any other changes I should make before we merge

@mcbex mcbex merged commit 0635ea2 into main Jul 22, 2022
@jonyoder jonyoder deleted the docker-platform-images branch November 16, 2022 03:42
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

4 participants