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

Add arm support #144

Closed
wants to merge 3 commits into from
Closed

Add arm support #144

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7c67ed
Add arm support
JanBerktold Aug 8, 2022
8a9f516
remove arm/v7
JanBerktold Aug 8, 2022
9e0935a
Change latest -> local
JanBerktold Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.


LABEL maintainer="Johan Andersson <[email protected]>"

Expand Down
3 changes: 2 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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.


up:
@echo "Ensure that you have run `make build` to use the latest image"
docker-compose up

down:
Expand Down
6 changes: 2 additions & 4 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ services:
IP: ${REDIS_CLUSTER_IP}
SENTINEL: ${REDIS_USE_SENTINEL}
STANDALONE: ${REDIS_USE_STANDALONE}
build:
context: .
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

@JanBerktold JanBerktold Aug 9, 2022 •
edited
Loading

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

hostname: server
ports:
- '7000-7050:7000-7050'
Expand Down