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 Arm runners and bring back alpine #1328

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Conversation

trws
Copy link
Member

@trws trws commented Jan 17, 2025

This does a couple of things to enable new arm runners:

  1. add the runner to the matrix generator so we can choose between x86 and arm
  2. remove the emulator support since we should no longer need it
  3. reintroduce alpine

At least on a temporary basis this changes the matrix to always run the arm builds for testing purposes.

@@ -200,14 +204,14 @@ def __str__(self):
)
# Disabled because the arm64 build is failing and preventing the
# generate-manifest step from running
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be removed as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I'll clean that out. A couple other things that need to happen for this to work. The testenv container doesn't exist for arm, and the core container is in some kind of strange state, so we need to wait a bit anyway.

Copy link
Member

Choose a reason for hiding this comment

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

comment is still here?

@trws trws force-pushed the arm-runners branch 3 times, most recently from fe30e42 to bc8dde6 Compare January 23, 2025 20:30
@trws trws requested review from jameshcorbett and grondo January 23, 2025 20:31
@trws
Copy link
Member Author

trws commented Jan 23, 2025

I think this is all set now. I'd like to re-enable the fedora40 arm setup, but we'd need to get that support back in core first. As a bit of a postmortem, the problems and solutions were as follows:

  • builds in sched failed because of broken fluxrm/flux-core:alpine arm tag because the arm build was busted upstream
  • the builds in core were busted because the fluxrm/testenv:alpine tag only had an amd64 container and not an arm64 container attached to them, probably a mistake in a buildx --push or a docker push or similar
  • pushed a new tag with both amd64 and arm64 manually for testenv:alpine
  • core produced a working tag on the next merge to master
  • updated runners here to use arm runners to make things faster, and actually run tests

I'm working on a small PR to update a readme in core, but it might also be really good to port over the use of the arm runners, they're working really well as far as I can tell.

Copy link
Member

@jameshcorbett jameshcorbett left a comment

Choose a reason for hiding this comment

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

LGTM just one little thing

@@ -200,14 +204,14 @@ def __str__(self):
)
# Disabled because the arm64 build is failing and preventing the
# generate-manifest step from running
Copy link
Member

Choose a reason for hiding this comment

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

comment is still here?

@trws
Copy link
Member Author

trws commented Jan 24, 2025

Dang, I had that gone and must have brought it back in a rebase conflict. Thanks for the catch @jameshcorbett!

@trws trws force-pushed the arm-runners branch 4 times, most recently from 003c3f6 to e65a003 Compare January 24, 2025 21:08
trws added 3 commits January 24, 2025 13:11
problem: the emulated arm64 environment is slow and causing problems

solution: use the new native arm64 runners
problem: arm builds currently only run on master and do not run tests

solution: run them always, and run the tests
problem: we disabled alpine, and it no longer needs to be

solution: add alpine back in
@trws
Copy link
Member Author

trws commented Jan 24, 2025

Ok, I think this is all cleaned up. Setting MWP.

@trws trws added the merge-when-passing mergify.io - merge PR automatically once CI passes label Jan 24, 2025
@mergify mergify bot merged commit 471da50 into flux-framework:master Jan 24, 2025
23 checks passed
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.3%. Comparing base (22c24d5) to head (f63537a).
Report is 4 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1328   +/-   ##
======================================
  Coverage    75.3%   75.3%           
======================================
  Files         111     111           
  Lines       16042   16042           
======================================
  Hits        12081   12081           
  Misses       3961    3961           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing mergify.io - merge PR automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants