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 arm support #144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

JanBerktold
Copy link

@JanBerktold JanBerktold commented Aug 8, 2022

This PR adds basic linux/arm64 support via docker's new buildx. For now, we're adding linux/amd64 and linux/arm64 only but it is fairly trivial to add more platforms.

This does change the development experience: Since docker-compose can't natively build an multi-arch image, we are moving that phase into the make build command. In order to spin up a local dev environment, you now need to run make build before running make up. Unfortunately I'm not aware of a better way to solve this.

Closes #133

@JanBerktold JanBerktold marked this pull request as ready for review August 9, 2022 00:09
@@ -1,5 +1,4 @@
# Build based on redis:6.0 from 2020-05-05
FROM redis@sha256:f7ee67d8d9050357a6ea362e2a7e8b65a6823d9b612bc430d057416788ef6df9
FROM redis:6.0.16
Copy link
Owner

Choose a reason for hiding this comment

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

This wont fly. If you are updating the image you need to update to the hash like it used to be before. I am more then happy to update to a newer version, but it needs to be in the hash format.

Copy link
Author

Choose a reason for hiding this comment

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

The problem is that we need docker to be able to use the ARM base image when we're building the ARM package. By using the tag -- which points to a different container for every supported CPU architecture --, we allow docker to do this whereas the static hash is only able to reference a single container for a single CPU architecture and hence we can't easily switch around. Could you elaborate on your concerns w/ using a tag here?

image

Copy link
Owner

Choose a reason for hiding this comment

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

In the past there was always issues with people having different and wrong base images when building the image. We had bugs where things got changed around in upstream and caused bugs for us downstream and it was always a new build all the time for the tag we used. Even having a specific tag was not always guaranteed correct as with docket tags they are not totally permanent and i can push over a new build at any point, it is more a label then a git style tag. A specific hash is the only solution that worked out and has worked out flawlessly so far. This part is basically to enable and to force reproducible builds where i know you and i are using the same image.

My suggestion tho is that i might accept a solution where you use a build variable to make the hash reconfigurable from the outside with a default of a known one. I am not going to open up to :latest and most likely not to a tag. But having it variable could work because then the issue is pushed to the runner of the image to sort it out kinda.

Makefile Outdated
@@ -8,9 +8,10 @@ help:
@echo " cli run redis-cli inside the container on the server with port 7000"

build:
docker-compose build
docker buildx build --build-arg redis_version=6.2.1 --platform linux/amd64,linux/arm64 --tag grokzen/redis-cluster:latest .
Copy link
Owner

Choose a reason for hiding this comment

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

So can i build both amd64 and arm64 now with buildx on any other platform? or do i need to have a specific base OS installed in order to run this version of make build now?

Copy link
Author

Choose a reason for hiding this comment

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

No! You just need buildx which ships with any recently modern docker engine. I've built this both on my Macbook and a Windows desktop to confirm.

Copy link
Owner

Choose a reason for hiding this comment

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

I will say no to this call it "default" build step then. I do not want to limit the docker version that heavily by default. They need to be split into two commands one for the old way and one for ARM specific way then.

args:
redis_version: '6.2.1'
# We are building the image via `make build` which uses buildx since we need it build an multi-arch image.
image: grokzen/redis-cluster:latest
Copy link
Owner

Choose a reason for hiding this comment

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

The downside of this move is that in the case you just download this compose version and you just run it w/o running make build before it is that you will download the latest version from docker-hub automatically. The 2 issues with downloading from there these days is the number of downloads permitted by users from docker-hub itself. And the second one is that if you do not specify a version but :latest then you might get redis 7 or 8 in the future, and not a reliable redis 6.2.1 like before this.

Copy link
Author

Choose a reason for hiding this comment

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

Good points! I've switched this to the custom tag local instead to differentiate it from the commonly used latest. This will cause the build to fail (after seeing the "please run make build" message) when the image doesn't exist locally.

Copy link
Owner

Choose a reason for hiding this comment

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

I could live with that yes

Makefile Outdated
@@ -8,9 +8,10 @@ help:
@echo " cli run redis-cli inside the container on the server with port 7000"

build:
docker-compose build
docker buildx build --build-arg redis_version=6.2.1 --platform linux/amd64,linux/arm64 --tag grokzen/redis-cluster:latest .
Copy link
Owner

Choose a reason for hiding this comment

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

Another major issue with forcing building both here is that most people don't care about having both, they only need one or the other. It might be more logical to make two different make targets instead of forcing everyone to build two different images. keep amd64 for make build and maybe make make build-arm or something that does the other version.

Copy link
Author

Choose a reason for hiding this comment

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

Good point! buildx works by producing a single docker manifest with entries for each supported platform type. When consuming the image from a remote registry (e.g. dockerhub), the client can easily figure out which layers it needs for it's native platform and only download those via the manifest. So there's no additional cost for users who consume the image via a registry.

Unfortunately, buildx only supports building all platform types at once when you want to combine them into one manifest. To help streamline the experience of users who consume this image by building it locally, I could add a make build-native that builds a manifest that only supports the local CPU architecture. This way you would have

  • make build => This is the release build that needs to be published to dockerhub. Supports AMD64 and ARM64.
  • make build-native => This is a build that will only work on the native CPU architecture and shouldn't be published.

See https://www.docker.com/blog/multi-arch-build-and-images-the-simple-way/

Let me know what you think! I'm happy to just add the make build-native command.

Copy link
Owner

Choose a reason for hiding this comment

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

No i am more inclined to go the other way around. Keep build as it was to keep backwards compatibility and then make a special make buildx or something that will work with these new features. It is much more pleasant to let the users opt-in during a trtransition phase and meek the old way it was until docker continues to build and evolve these features. I do not want to force a user into using a minimum docker version that is way to new and that might not even have install package in some distros. Many people still use super old dockers and the old way still works and should work for a long time to come imo.

@JanBerktold
Copy link
Author

Thanks for your quick review! I've responded/addressed all of your comments, PTAL. @Grokzen

@isaadabbasi
Copy link

When are we expecting this to be merged?

@JanBerktold
Copy link
Author

When are we expecting this to be merged?

Unclear! I'll have time to do another design revision today/tomorrow. We'll see after that.

@isaadabbasi
Copy link

@JanBerktold Thanks for the quick reply,
I am using a manually created container image for now. I'd be watching this thread, I hope you get some time for it.
Have a nice day.

@Grokzen
Copy link
Owner

Grokzen commented Aug 19, 2022

@isaadabbasi This PR needs more work before i am passing it through the gates into the master branch

@rsre
Copy link

rsre commented Dec 22, 2023

Hello @Grokzen! Just to get back into this, would you be fine with updating the Travis CI workflow so the published builds support linux/arm64 using docker buildx?

@ac27182
Copy link

ac27182 commented Apr 2, 2024

Hey @Grokzen is currently being actively developed?

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.

Image for Apple M1 docker
5 participants