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

Jump to bottom

Add support for multi-arch container images #3897

Merged
merged 3 commits into from
Dec 1, 2020
Merged

Add support for multi-arch container images #3897

merged 3 commits into from
Dec 1, 2020

Conversation

charith-elastic
Copy link
Contributor

@charith-elastic charith-elastic commented Nov 4, 2020 •
edited
Loading

Adds multi-arch build support. With these changes, the nightly job should build both the amd64 and arm64 versions of the operator.

E2E testing on EKS Graviton machines will be added in a separate PR.

Related to #3504

@botelastic botelastic bot added the triage label Nov 4, 2020
@charith-elastic charith-elastic added the >feature Adds or discusses adding a feature to the product label Nov 4, 2020
@botelastic botelastic bot removed the triage label Nov 4, 2020
@charith-elastic
Add multi-arch build to nightly
9cf04e4 
WIP

WIP

WIP

Force docker login

WIP

Install buildx

qemu

Script steps

WIP

Increase timeout

Multi-arch e2e image

Increase timeout

Avoid double build

Plain progress

Rebase

Fix Docker script

Add ARM build to nightly
@charith-elastic charith-elastic added :ci Things related to Continuous Integration, automation and releases v1.4.0 labels Nov 24, 2020
@charith-elastic charith-elastic changed the title Add Docker multi-arch support Add support for multi-arch container images Nov 24, 2020
@charith-elastic charith-elastic marked this pull request as ready for review November 24, 2020 09:50
pebrc
pebrc reviewed Nov 26, 2020
View reviewed changes
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM, maybe add a note for developers that they need to turn on experimental features in Docker Desktop for this to work locally. Not sure if it would make sense to include a check in check-requisites.sh?

Also all the people wanting to run the operator on their Raspberry Pi's will be out of luck with this as depending on the Pi's version they will need linux/arm/v7 or linux/arm/v8 I believe, which is not supported by our base image. Maybe worth a comment/

Makefile
docker-build: go-generate generate-config-file
docker build . \
docker-build: go-generate generate-config-file
DOCKER_BUILDKIT=1 docker build . \
Copy link
Collaborator

@pebrc pebrc Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Any particular reason we are introducing BuildKit with this PR? I am not opposed just curious.

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 had to introduce it because the normal docker build command could not parse the changes made to the Dockerfile (specifically, the --platform argument). The alternative would have been to use two Dockerfiles -- which didn't seem right.

hack/docker.sh Outdated Show resolved Hide resolved
@charith-elastic
Copy link
Contributor Author

I believe Raspberry Pi 4 supports arm64. As you say, because of the base image restrictions, we can't support the other architectures at this time. Where would you suggest we mention that?

@pebrc
Copy link
Collaborator

pebrc commented Nov 26, 2020

I believe Raspberry Pi 4 supports arm64. As you say, because of the base image restrictions, we can't support the other architectures at this time. Where would you suggest we mention that?

I am not sure, maybe we can turn this around and mention in the documentation which architectures we support (instead of saying what we don't support). I would think this could go near the "Supported versions" section in the Quickstart, maybe?

@charith-elastic
Copy link
Contributor Author

Sounds good. Since there's still some work remaining to run the E2E tests on ARM before we can officially support it, I will add the disclaimer to the documentation in that PR.

pebrc
pebrc approved these changes Nov 26, 2020
View reviewed changes
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jeffspahr
Copy link

I believe Raspberry Pi 4 supports arm64. As you say, because of the base image restrictions, we can't support the other architectures at this time. Where would you suggest we mention that?

FYI I tested an arm64 build of ECK on a Raspberry Pi 4, and I can confirm it works. See this issue comment for more details:
#3504 (comment)

@charith-elastic
Copy link
Contributor Author

run full pr build

@charith-elastic charith-elastic merged commit cab3dd1 into elastic:master Dec 1, 2020
@charith-elastic charith-elastic deleted the feature/arm-support branch December 1, 2020 09:12
@pebrc pebrc removed the :ci Things related to Continuous Integration, automation and releases label Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pebrc pebrc pebrc approved these changes

Assignees
No one assigned
Labels
>feature Adds or discusses adding a feature to the product v1.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@charith-elastic @pebrc @jeffspahr