-
Notifications
You must be signed in to change notification settings - Fork 549
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
Add arm support #144
Conversation
@@ -1,5 +1,4 @@ | |||
# Build based on redis:6.0 from 2020-05-05 | |||
FROM redis@sha256:f7ee67d8d9050357a6ea362e2a7e8b65a6823d9b612bc430d057416788ef6df9 | |||
FROM redis:6.0.16 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 . |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docker-compose.yml
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 . |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Thanks for your quick review! I've responded/addressed all of your comments, PTAL. @Grokzen |
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. |
@JanBerktold Thanks for the quick reply, |
@isaadabbasi This PR needs more work before i am passing it through the gates into the master branch |
Hello @Grokzen! Just to get back into this, would you be fine with updating the Travis CI workflow so the published builds support |
Hey @Grokzen is currently being actively developed? |
@ac27182 No not currently |
This PR adds basic
linux/arm64
support via docker's newbuildx
. For now, we're addinglinux/amd64
andlinux/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 runmake build
before runningmake up
. Unfortunately I'm not aware of a better way to solve this.Closes #133