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

M1 mac compatibility #5511

Merged
merged 6 commits into from
Sep 22, 2024
Merged

M1 mac compatibility #5511

merged 6 commits into from
Sep 22, 2024

Conversation

style95
Copy link
Member

@style95 style95 commented Sep 17, 2024

This is a very first step to make OW compatible with M1 Mac.

Description

There could be many ways but I chose to minimize changes. I decided to have another Dockerfile for the M1 Mac. It will be selectively used according to the OS and its architecture.

I could build and deploy OW on M1 Mac and run an action.
(Though I had to install rosetta because openwhisk-cli is also not being built for M1 mac.)

$ wsk action invoke noop -i -r
{}

Related issue and scope

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Scheduler
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

version: 3.4
image:
amd64: zookeeper:3.4
arm64: arm64v8/zookeeper:3.4
Copy link
Member Author

Choose a reason for hiding this comment

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

I added another zookeeper image for arm64

@@ -464,7 +466,7 @@ zeroDowntimeDeployment:
enabled: "{{ zerodowntime_deployment_switch | default(false) }}"

etcd:
version: "{{ etcd_version | default('v3.4.33') }}"
version: "{{ etcd_version | default('3.5') }}"
Copy link
Member Author

Choose a reason for hiding this comment

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

etcd supports multiarch since 3.5 version.
https://hub.docker.com/r/bitnami/etcd/

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is stale now, but does etcd still require one to enable experimental features to run on arm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore that, I found the answer. Looks like arm64 has first-class support now as of 3.5. Previously it required one to use the ETCD_UNSUPPORTED_ARCH environment variable.

@@ -80,7 +80,7 @@
when: environmentInformation.type == "docker-machine"

- name: "determine docker root dir"
shell: echo -e "GET http:/v1.24/info HTTP/1.0\r\n" | nc -U /var/run/docker.sock | grep "{"
Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't work with the recent version of docker for mac.

- name: set zookeeper image
set_fact:
zookeeper_image: "{{ zookeeper.image.arm64 }}"
when: ansible_facts['os_family'] == "Darwin" and ansible_facts['architecture'] == "arm64"
Copy link
Member Author

Choose a reason for hiding this comment

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

Selectively use the arm64 zookeeper image according to the os and architecture.

# limitations under the License.
#
# if you change version of openjsk, also update tools/github/setup.sh to download the corresponding jdk
FROM arm64v8/eclipse-temurin:21.0.4_7-jdk-alpine
Copy link
Member Author

Choose a reason for hiding this comment

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

Only the base image is changed.

@@ -24,6 +24,9 @@ plugins {
}

ext.dockerImageName = 'scala'
if(System.getProperty("os.arch").toLowerCase(Locale.ENGLISH).startsWith("aarch")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

If the architecture of OS is m1 Mac, it uses the Dockerfile with the .arm suffix.

protobuf {
protoc {
if (osdetector.os == "osx") {
artifact = 'com.google.protobuf:protoc:3.4.0:osx-x86_64'
Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, we cant' upgrade the protoc version for now because some libraries are dependent on this version.
And protoc-3.4.0 does not support arm architecture yet, so I chose this way.
This also requires rosetta installed.

grpc/grpc-java#7690 (comment)

# limitations under the License.
#

FROM arm64v8/eclipse-temurin:8u422-b05-jdk-noble
Copy link
Member Author

Choose a reason for hiding this comment

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

I only changed the base image from adoptopenjdk/openjdk8:jdk8u262-b10 to arm64v8/eclipse-temurin:8u422-b05-jdk-noble.

I used it as the current image is deprecated.

image

https://hub.docker.com/_/adoptopenjdk

@style95 style95 requested a review from rabbah September 17, 2024 07:36
@style95
Copy link
Member Author

style95 commented Sep 17, 2024

One concern is we don't have any CI pipelines for the m1 Mac.
It seems the M1 runner is available for all plans. It would be great if we set up one pipeline.

actions/runner-images#9254

@dgrove-oss
Copy link
Member

One concern is we don't have any CI pipelines for the m1 Mac. It seems the M1 runner is available for all plans. It would be great if we set up one pipeline.

actions/runner-images#9254

My suggestion would be that we go after enabling one of the CI pipelines for both x86_64 and arm in a separate PR.

Copy link
Member

@dgrove-oss dgrove-oss left a comment

Choose a reason for hiding this comment

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

This all makes sense to me. Thank you for getting started on this @style95!

I have not had a chance to try it on my Mac yet, but hope to carve out some time this afternoon.

@dgrove-oss
Copy link
Member

dgrove-oss commented Sep 17, 2024

Definitely an improvement; I was able to do distDocker and install successfully on my main laptop for the first time in several years.

Copy link
Member Author

@style95 style95 left a comment

Choose a reason for hiding this comment

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

Let me try deploying this branch on a linux machine before merging to do a basic sanity check.

state: started
recreate: true
restart_policy: "{{ docker.restart.policy }}"
volumes: "{{volume_dir | default([])}}"
command: "/usr/local/bin/etcd \
Copy link
Member Author

Choose a reason for hiding this comment

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

Before merging, I will check how to enable these options with the Bitnami/etcd image.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this can be addressed in a subsequent PR.

@style95
Copy link
Member Author

style95 commented Sep 19, 2024

I could build and deploy OW with this branch and successfully invoke an action with a Linux machine.

@style95
Copy link
Member Author

style95 commented Sep 22, 2024

I am merging this as it has been open for a while.

@style95 style95 merged commit daac765 into apache:master Sep 22, 2024
6 checks passed
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.

3 participants