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 arm64 support #380

Merged
merged 8 commits into from
Apr 1, 2024
Merged

Add arm64 support #380

merged 8 commits into from
Apr 1, 2024

Conversation

audacioustux
Copy link
Contributor

solves #187

Pls make any changes if required.

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @audacioustux!

I've left a couple of comments but there are quite a few of CI bumps all at once - that's a lot to double-check 🙂 So it's best to split this up as separate PRs. Ideally in 3 parts: ARM itself (i.e. only what's needed for ARM), Golang CI updates and other version CI updates.

Specifically regarding ARM change: did you test this change and how?

.github/workflows/e2e-test.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-test.yaml Outdated Show resolved Hide resolved
@audacioustux
Copy link
Contributor Author

audacioustux commented Mar 4, 2024

@yorugac thanks for the feedback, I'll send individual PRs as you mentioned.
Yes, have checked the ARM Images, and it works https://github.com/audacioustux/k6-operator/pkgs/container/k6-operator .. tested by changing the operator image value in helm deployment, on Ampere/Arm64 processor.

@yorugac
Copy link
Collaborator

yorugac commented Mar 27, 2024

Hi @audacioustux, it's been awhile; hope you're well 🙂
How is it going? Would you be able to continue with this PR?

@audacioustux
Copy link
Contributor Author

Hi @yorugac, sorry got busy with other stuffs. https://github.com/audacioustux/k6-operator I could make the ARM64 support work here, but that was a bit too involved than I had anticipated. + The fact that there's hard-coded image uri in the code made it a bit difficult to make a well-tested PR.
Also, I couldn't make the browser tests work grafana/xk6-browser#1239 (I think it's not related to Arm though) and couldn't manage the time to look into it either.

I think the first thing that needs to be done, is to fix that hardcoded container image URI issue (for example:

image := "ghcr.io/grafana/k6-operator:latest-starter"
)

For the ARM64 support, I can hopefully send the PR in next weekend (30/31 March), sorry for keep you waiting.

@yorugac
Copy link
Collaborator

yorugac commented Mar 28, 2024

Hi @audacioustux, thanks for quick response!

Yes, the browser tests are a "known" issue: #289 AFAIR, some cases have worked and some didn't. Investigating this wasn't prioritized yet but good thing you've opened the issue in xk6-browser repo as well 👍

I think the first thing that needs to be done, is to fix that hardcoded container image URI issue

🤔 you meant for testing? To test custom images, you can pass them as part of TestRun CR definition, i.e. starter.image in this example.

For the ARM64 support, I can hopefully send the PR in next weekend (30/31 March)

That would be amazing; thank you.

Btw, re-reading this PR: I've asked to have CI updates as separate PRs -- would you like me to take those, to reduce the "change request"?

@audacioustux
Copy link
Contributor Author

you meant for testing? To test custom images, you can pass them as part of TestRun CR definition, i.e. starter.image in this example.

Umm, as far I can remember, even though starter and runner image was declared in TestRun manifest, it was spawning grafana/k6-operator. May have to re-check that actually...

Btw, re-reading this PR: I've asked to have CI updates as separate PRs -- would you like me to take those, to reduce the "change request"?

Yes please.

@audacioustux
Copy link
Contributor Author

audacioustux commented Mar 30, 2024

@yorugac Hi, pls let me know if the image build and push works correctly and if anything needs to be changed.

@audacioustux audacioustux changed the title upgrade workflow dependencies, add arm64 support add arm64 support Mar 30, 2024
Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Hi @audacioustux, Thanks for the update!

On CI changes in push.yaml: I've gone through these ones by now and they seem to be mostly OK.
buildx question remains: it seems like the issue we encountered was around ~ v0.10 so maybe v0.12 will be fine. If this version change is not necessary for ARM, I'd prefer to see it in another PR.

Secondly, on Dockerfiles: there is a change request there.

let me know if the image build and push works correctly

To test this workflow (without further changes), I'll need to merge this PR first 😂 In general, that's why I request as minimal version of changes as possible: it can be pretty time-consuming to debug CI issues so it's best when they're small and atomic.

Dockerfile.starter Show resolved Hide resolved
@audacioustux
Copy link
Contributor Author

audacioustux commented Mar 31, 2024

On CI changes in push.yaml: I've gone through these ones by now and they seem to be mostly OK. buildx question remains: it seems like the issue we encountered was around ~ v0.10 so maybe v0.12 will be fine. If this version change is not necessary for ARM, I'd prefer to see it in another PR.

umm which issue exactly? I updated the version just to be on safe side and hoped it's most likely backward compatible. I wasn't sure if the "platform" value is supported in the previous version, so tried to save one trial and error.

edit: ah I see, you're referring to #187? but how to test if that issue is now fixed or not with latest version? as the sole purpose of buildx is to simply build the images, it worked fine for me, also in cluster...

To test this workflow (without further changes), I'll need to merge this PR first 😂 In general, that's why I request as minimal version of changes as possible: it can be pretty time-consuming to debug CI issues so it's best when they're small and atomic.

I see. that's actually the issue I was referring to earlier. I think in the current situation of the repo it's a bit hard to rollout new changes and non-ideal for community contributions. I actually had plan for adding Devcontainers support, that I think should significantly make things easier and streamlined - we may discuss about that later at some point.

@yorugac
Copy link
Collaborator

yorugac commented Mar 31, 2024

On buildx: yes, I meant that issue. Basically Docker manifests got broken with newer version. I've been checking the GH changelogs: it seems like it's still an issue even now... There might be a way to make a fix without pinning a version but for now let's not change that line with buildx version. I think ARM build should work anyway, right?

@audacioustux
Copy link
Contributor Author

https://github.com/audacioustux/k6-operator/actions/runs/8500731873/job/23282902694 the push.yml worked with the buildx version pinned to 0.9.1, kept the USER 65532:65532 in starter dockerfile... tested in k8s and working

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Great, thanks @audacioustux! 🙌

@yorugac yorugac changed the title add arm64 support Add arm64 support Apr 1, 2024
@yorugac yorugac merged commit 1441c17 into grafana:main Apr 1, 2024
6 checks passed
@audacioustux audacioustux deleted the grafana-pr branch April 1, 2024 09:16
@yorugac yorugac added the enhancement New feature or request label Apr 1, 2024
@Israphel
Copy link

Israphel commented Jun 7, 2024

which version has ARM support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants