-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Adding architecture support for arm/arm64/amd64 docker images #1781
Conversation
@sagikazarmark @JoelSpeed Despite the CI failure it's because my proposal also will change the Github workflow. It would be great to get this reviewed, I'm currently building and maintaining an unofficial image here: https://github.com/raspbernetes/multi-arch-images/blob/master/build/cloudflared/Dockerfile However, my goal is to have this support in the upstream - Using Golang makes this somewhat trivial but happy to make any tweaks as you see necessary. |
I'll try to review this, but it's not on my high prio list. My problem with these kind of changes (similar to connector changes) is that we don't have an easy way to test them to prevent regressions. We can probably merge this PR, but we need to find a way to run tests on these architectures as well. |
@sagikazarmark we could run a different tag and put this in a unique workflow and run both images to avoid breaking backward compatibility. |
Changes look sane from my perspective, though I share similar concerns about testing. I wonder if there is some way we can test test these images automatically so that we don't introduce regressions in the future? |
I'm looking at self-hosted runners for GitHub Actions. In theory, they support ARM, so we could at least compile and run tests against ARM, though I'm not sure the entire pipeline can run on ARM architecture. CNCF could probably help with funding. |
@bonifaido @sagikazarmark @JoelSpeed Thanks for reviewing the PR - slight adjustments to now use the official docker github actions to execute multi-arch support using I've also used the native docker ARGS in the Dockerfile to simplify the logic. |
Working & Tested Image here: https://hub.docker.com/r/raspbernetes/dex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I'm happy with the changes, we just need to set up some secrets here to be able to try this out, and some minor requested changed on the tagging side of the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xunholy thanks a lot for the changes.
I'd prefer if you could minimize the changes and kept them strictly related to arm support. We shouldn't lose functionality. I wonder if upgrading the push action in a separate PR and adding arm support in another one would make sense.
Thanks, @sagikazarmark appreciate the comments.
my understanding is there is no "lost functionality" you're using the old docker push which doesn't support multi-arch so it's a required change. |
By lost functionality, I meant:
Also, can you please sign-off your commits? Thanks! |
Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> update Dockerfile Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> Update ci.yml Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> update to use new official docker buildx action Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> update Dockerfile Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> include version image for tagged releases Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> Update .github/workflows/ci.yml Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> add push based on event Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think we should add a comment/TODO to the images so that we remember why we are using master and eventually pin to a new release
Signed-off-by: Michael Fornaro <20387402+xUnholy@users.noreply.github.com>
@sagikazarmark @bonifaido all feedback has now been addressed, if you could both please rereview 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xunholy !
I think it's good enough for now. I have a bunch of changes incoming to the Dockerfile and the way we build images, so let's merge this ASAP.
Personally, I'd consider arm support experimental until we can find a way to make sure we don't break it.
Breaking test is okay, it's not supposed to work in PRs I think. (Note to self: fix that too) |
@sagikazarmark agreed. I've been testing the arm64 variant and using it for some time now without issues. Happy to review any Dockerfile changes you make in future to ensure its still going to work and has the most efficient and optimised approach. |
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
fix: typo in environment variables introduced in #1781
…#1781) add multi-arch image support for armv7/arm64/amd64 architectures
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Adding architecture support for arm/arm64/amd64 docker images using docker
buildx
functionality.Tested arm64 running on a local Kubernetes cluster to confirm it was working without issues.
Signed-off-by: Michael Fornaro 20387402+xUnholy@users.noreply.github.com