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

Build linux/arm64 images #51

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

Conversation

motoki317
Copy link
Contributor

Hello,

I noticed that there is no multiplatform support of the driver image, and I thought it'd be nice to have the image built for both linux/amd64 and linux/arm64 platforms.
This change utilizes docker actions to build multiplatform images, which replaces old script in Makefile.

I tested it in my own arm64 environment on OCI A1 instance, it seems to be working.
ref: package, manifest (values.yaml)

Thank you in advance.

@motoki317
Copy link
Contributor Author

@vitalif Hi, it would be great if you could take a look at this patch and possibly merge it. Thanks!

@vitalif
Copy link
Collaborator

vitalif commented Apr 21, 2023

Hi, why did you remove local makefile? I want to be able to build it locally without github ci.

@vitalif
Copy link
Collaborator

vitalif commented Apr 21, 2023

I also think that arm64 csi-s3 can be built without qemu at all because Go allows for very easy cross compilation

@motoki317
Copy link
Contributor Author

Hi, why did you remove local makefile? I want to be able to build it locally without github ci.

I thought it would be verbose because everything can be built on GitHub Actions, but I guess it's helpful to keep it so you can test it (and publish it) locally.
I dropped the commit in which I removed the Makefile script.

I also think that arm64 csi-s3 can be built without qemu at all because Go allows for very easy cross compilation

You're right! This also drastically reduces build time, thanks for the suggestion.
I also removed QEMU setup which was not exactly needed in this case.

@motoki317
Copy link
Contributor Author

@vitalif Hi, I fixed the patch following your comments. Would you take a look at this again?

@motoki317 motoki317 force-pushed the build-linux-arm64 branch from e000550 to fc23e42 Compare June 4, 2023 06:52
@vitalif
Copy link
Collaborator

vitalif commented Jun 15, 2023

Sorry that you have to wait, I think your PR is fine, but I wasn't sure about the multi-architecture support in Yandex container registry. It seems it's not supported yet. So for now it won't work :) Container registry team says they are already working on a fix, so I'll merge it when it is ready.

@motoki317
Copy link
Contributor Author

Thanks for the update! I'm glad to hear that. I'm in no rush, looking forward to it.

@NikoGrano
Copy link

@vitalif Sorry for pushing, however, do we have an update on this yet? Is there anything we could assist with?

The lack of arm64 is currently blocking me to continue my research 😞.

@vitalif
Copy link
Collaborator

vitalif commented Jul 4, 2023

Hi. No, container registry support for "fat manifests" is not rolled out yet. I suppose you can use your own builds by now?)

@NikoGrano
Copy link

@vitalif Yes, I was able to do so. However following images need to be also upgraded (no arm support):

containers:
- name: driver-registrar
image: quay.io/k8scsi/csi-node-driver-registrar:v1.2.0
args:
- "--kubelet-registration-path=$(DRIVER_REG_SOCK_PATH)"

Here we need to use image:

registry.k8s.io/sig-storage/csi-node-driver-registrar:v2.8.0

containers:
- name: csi-provisioner
image: quay.io/k8scsi/csi-provisioner:v2.1.0
args:

And here also:

registry.k8s.io/sig-storage/csi-provisioner:v3.3.0

Anyways quay.io images are deprecated and we should be using registry.k8s.io instead.

@motoki317 motoki317 force-pushed the build-linux-arm64 branch from fc23e42 to 5ea1854 Compare July 20, 2023 08:28
@vitalif vitalif force-pushed the master branch 7 times, most recently from 6fbc8c9 to 227e1cf Compare August 28, 2023 15:41
@motoki317 motoki317 force-pushed the build-linux-arm64 branch 2 times, most recently from 5ea1854 to a419999 Compare September 8, 2023 00:36
@space192
Copy link

any update on this ?

@nidr0x
Copy link

nidr0x commented Dec 2, 2023

Any updates?

@igor-nikiforov igor-nikiforov mentioned this pull request Feb 12, 2024
@jhass
Copy link

jhass commented Feb 28, 2024

I'd also love to use this, how's the status? Could pushing the multi-platform version to for example GHCR be an interim solution?

@motoki317 motoki317 force-pushed the build-linux-arm64 branch from 0f5dca8 to 7ffbc6b Compare May 17, 2024 02:14
@Jean-Baptiste-Lasselle
Copy link

@motoki317 THANK YOU SOOOOO MUCH I JUST FINISHED TETING IT ALLLL AND IT WORKKSSSS on arm64 i run jupyterhub with that csi driver now with my own minio its just the heavennssss AWESOMEEE I really mean the THANK YOU

I had to trick around more things after that because i needed the official csi images like the registrar on ARM64 too, but i could easily find some from different companies on dockr hub with arm64 support, wow its really awesome ! without your work would have been muchhh harder!

@motoki317
Copy link
Contributor Author

@Jean-Baptiste-Lasselle I’m glad you found it useful :)

Yes, you do need to swap some more CSI default images from what is provides in this repository, see the PR description and their latest versions to see how I did it.

ref: package, manifest (values.yaml)

We should probably update those as well, maybe in another commit or PR.

@schlichtanders
Copy link

Is this still being worked on?
I also would appreciate arm support

@mertcangokgoz
Copy link

What about this PR, won't it support ARM?

@denysvitali
Copy link

I've partially added support for this in #145

@motoki317
Copy link
Contributor Author

Sorry that you have to wait, I think your PR is fine, but I wasn't sure about the multi-architecture support in Yandex container registry. It seems it's not supported yet. So for now it won't work :) Container registry team says they are already working on a fix, so I'll merge it when it is ready.

@vitalif Hi, it's been a while, any update on this? It looks like a few other people also want this PR merged.

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.

10 participants