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

Add support for RISC-V #346

Closed
wants to merge 2 commits into from
Closed

Conversation

chazapis
Copy link

@chazapis chazapis commented Jul 8, 2023

Adds the RISC-V architecture to the scripts involved in building and packaging Local Path Provisioner; no code changes.

The resulting container has been tested in QEMU with a RISC-V build of K3s and is working fine.

Notes

  • A riscv64 binary is generated by cross-compilation when $ARCH == riscv64.
  • Testing and validation steps that are part of the ci script are skipped (options set via SKIP_TEST and SKIP_VALIDATE environment variables).
  • The container image is built using Docker BuildKit. The base container image for Local Path Provisioner is now an argument, so it can be easily changed.

Signed-off-by: Antony Chazapis <[email protected]>
Signed-off-by: Antony Chazapis <[email protected]>
Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

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

LGTM
@Anarkis do we have RISC-V machines for building?

@chazapis
Copy link
Author

LGTM
@Anarkis do we have RISC-V machines for building?

There is no need to build natively on RISC-V. The patch sets GOARCH to cross-compile a binary. This seems to work ok in the Drone CI environment. However, packaging up the container image fails... Could it be that the underlying Docker instance does not support BuildKit?

@derekbit
Copy link
Member

LGTM
@Anarkis do we have RISC-V machines for building?

There is no need to build natively on RISC-V. The patch sets GOARCH to cross-compile a binary. This seems to work ok in the Drone CI environment. However, packaging up the container image fails... Could it be that the underlying Docker instance does not support BuildKit?

We still have CI test before releasing, so RISC-V is required...

However, packaging up the container image fails... Could it be that the underlying Docker instance does not support BuildKit?

Need to check it.

@Anarkis
Copy link

Anarkis commented Jul 13, 2023

LGTM @Anarkis do we have RISC-V machines for building and release?

@pgonin any info for this topic?

@chazapis
Copy link
Author

@derekbit, @Anarkis, @pgonin Sorry to bother you, but it has been over 3 months with no response. This is a really helpful feature that requires no code changes. Could it possibly be included in the next release?

@derekbit
Copy link
Member

@chazapis
Sorry for late reply. I am reviewing and preparing a new release. I am fine with the PR. Currently, we don't have machines for RISC-V test, so I'm thinking the workaround. Any feedback is appreciated.

@chazapis
Copy link
Author

While we have some hardware in our lab, we generally use QEMU for testing on RISC-V. However, I am not sure how this would tie up with running the CI jobs using Drone...

On the other hand, since the RISC-V community is still small, I think it would be perfectly acceptable to publish a build marked as experimental/untested.

@derekbit
Copy link
Member

@chazapis It sounds good.

@derekbit
Copy link
Member

@chazapis
We currently lack RISC-V machines for the building and release processes. To prevent triggering the drone task, could you please remove the .drone.yaml file.

I'm fine with the other changes. Could you please update the README.md to include instructions on building the image for the RISC-V platform? I think RISC-V users can build the image by themselves first.

WDYT?

@chazapis
Copy link
Author

@derekbit, I think it is worth trying to include a RISC-V image in each release. As local-path-provisioner is meant to run in containerized form within Kubernetes, it is not really helpful to tell users to recompile and package up everything themselves. The goal is to run K3s out of the box on RISC-V. In a recent discussion with @brandond in the respective K3s PR, he also mentions that the most important issue for RISC-V support right now is the lack of architecture-compatible container images.

So, if there is any way that local-path-provisioner can build and release RISC-V images, I suggest that we pursue that path. If it requires more work, I am eager to help. On the other hand, if you think that this is not possible at this point, I can remove the Drone.io part of this PR and add it in another.

@derekbit
Copy link
Member

Let's set the milestone to v0.0.28

@derekbit derekbit closed this May 27, 2024
@derekbit derekbit reopened this May 27, 2024
@derekbit derekbit added this to the 0.0.28 milestone May 27, 2024
@derekbit
Copy link
Member

derekbit commented Sep 3, 2024

@chazapis
Copy link
Author

@chazapis I don't have an env for the riscv64 test. Could you try https://hub.docker.com/layers/rancher/local-path-provisioner/v0.0.29/images/sha256-993e9467f39461ed7aacad2c6f1724abc249a1285bf8870de14ed68b99f869cb?context=explore? Thanks.

I can confirm that v0.0.29 works fine. I just tested it on a RISC-V VM with Ubuntu.

The logs:

# kubectl logs -n kube-system local-path-provisioner-6b6dfb894d-ldhpv
I0919 10:49:08.242741       1 controller.go:824] "Starting provisioner controller" component="rancher.io/local-path_local-path-provisioner-6b6dfb894d-ldhpv_a9c72231-99ef-458b-be99-4cef9160735b"
I0919 10:49:08.451079       1 controller.go:873] "Started provisioner controller" component="rancher.io/local-path_local-path-provisioner-6b6dfb894d-ldhpv_a9c72231-99ef-458b-be99-4cef9160735b"

I see that Alpine containers now support the riscv64 architecture, so the current solution is much better.

Thank you!

@derekbit
Copy link
Member

Let's close the PR. Thank you.

@derekbit derekbit closed this Sep 19, 2024
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