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 MacOS M1 runs to incremental builds #27156

Merged
merged 4 commits into from
Sep 20, 2022

Conversation

holly-cummins
Copy link
Contributor

I have set up a self-hosted runner on a Mac Mini which can now start running builds. Before this PR is merged, we will need to adjust the quarkus repository settings to wire in the runner.

As a first pass, I've added the runner to the JVM matrix in the incremental build.

  • We may also want to add it to the native matrix since that could be a place where we see lots of errors
  • We only have a single machine, so if the load is too high, we may want to move M1 to a less widely used workflow, or a lighter set of tests within the incremental CI workflow, or put a condition on its execution. However, I saw that the ubuntu builds Java 17 took 3 hours and the M1 build took 1 hour (and got further), so we may find that having the fast M1 machine serving build requests is a good way for contributors to get fast feedback.

I disabled some linux-specific steps which are not currently run on windows, so skipping them on mac should also be safe. (Although I wonder about the mongodb one, and whether we should find a platform-agnostic way to stop it if it leaks).

https://github.com/holly-cummins/quarkus/actions/runs/2798189377 has build output for my PR, on a runner I attached to my fork. The M1 build did not succeed, which isn't totally surprising because we know we have a range of issues on that platform (which is why we want an M1 build!). However, it got about as far as the ubuntu Java 17 build. Both seemed to fail in the first extensions test suite they ran.

The M1 failure was in mongodb, which makes me a bit nervous that stopping mongo will be needed in some way, especially since self-hosted-runners don't run on a fresh machine each time. I think it might be helpful to iterate on that while the M1 facility is available, to start building up some coverage of how often we see failures and where else they are. For example, I'm surprised I didn't see #25230 in the Actions build.

@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Aug 5, 2022
@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins marked this pull request as draft August 9, 2022 16:17
@holly-cummins
Copy link
Contributor Author

holly-cummins commented Aug 9, 2022

Merging of this is paused pending adding the M1 runner, which is paused pending quarkusio/quarkus-github-bot#256.

@holly-cummins
Copy link
Contributor Author

I've added an extra set of rules which leverage quarkusio/quarkus-github-bot#256 to control what PRs are allowed to run on the runners. This helps secure both the new M1 runner and the existing hosted runners.

This should be ready to merge; if we get the sequencing wrong and merge too early, the worst consequence is that we start to build up a backlog of requests on the M1 runner.

@holly-cummins holly-cummins marked this pull request as ready for review September 12, 2022 12:36
@holly-cummins holly-cummins force-pushed the holly-m1-action-runner branch 5 times, most recently from 8c0e51f to ad72802 Compare September 12, 2022 15:30
@gsmet
Copy link
Member

gsmet commented Sep 13, 2022

@holly-cummins have you seen the macOS build is pending?

@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins force-pushed the holly-m1-action-runner branch 2 times, most recently from a724ef3 to 0740842 Compare September 13, 2022 14:16
@holly-cummins
Copy link
Contributor Author

I wasn't planning to enable the M1 runner until the repo workflows were locked down and bot configuration was pushed. To allow the timing of the two things to be more separate, I'll pull the bot configuration changes out into another PR.

@holly-cummins holly-cummins marked this pull request as draft September 13, 2022 14:20
@quarkus-bot

This comment has been minimized.

@holly-cummins
Copy link
Contributor Author

See #27899 for the bot changes, which should be merged first.

@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins marked this pull request as ready for review September 15, 2022 11:07
@holly-cummins holly-cummins force-pushed the holly-m1-action-runner branch 2 times, most recently from 79338f1 to 170f1d2 Compare September 15, 2022 20:13
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@holly-cummins
Copy link
Contributor Author

Another nearly clear run on M1: just this failure, which seems to me to be a brittle test. The output is the same as the expected, except for the order of parameters:

[Check failure on line 19 in integration-tests/smallrye-config/src/test/java/io/quarkus/it/smallrye/config/AppConfigTest.java](https://github.com/quarkusio/quarkus/commit/b06790ddd8dd7357595cb5b9785790a1277d1d3d#annotation_4644614958)

@quarkus-bot quarkus-bot / Build summary for b06790ddd8dd7357595cb5b9785790a1277d1d3d

JVM Tests - JDK 17 MacOS M1

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.
Expected: is "AppConfig{name=app, info=Info{count=10, alias=alias}}"
  Actual: AppConfig{name=app, info=Info{alias=alias, count=10}}

	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)

@holly-cummins
Copy link
Contributor Author

(I'll re-run to confirm the smallrye-config failure is intermittent/unrelated-to-m1, and to incorporate the latest changes.)

@holly-cummins
Copy link
Contributor Author

Same smallrye config failure on the next run, so I have disabled and raised #28057.

@geoand geoand force-pushed the holly-m1-action-runner branch from a6ed521 to 14ee522 Compare September 19, 2022 12:41
Preprocess the matrix to avoid running M1 jobs on forks
…on Mac M1, in the CI.

 It may be all mac fails, but I have left the condition as is for ease of searching.
Also specify Mac OS

Reverse disabling test

Re-disable test
@holly-cummins
Copy link
Contributor Author

The main action run, https://github.com/quarkusio/quarkus/actions/runs/3083733398 is clean, and ran M1. In the run on my fork, https://github.com/holly-cummins/quarkus/actions/runs/3083733139, there's a test failure on windows, but the important thing is that M1 was not run. (We want to run M1 on PRs, but we do not want to run M1 on forks, since forks are very unlikely to have a self-hosted M1 runner attached to them.)

The changes include

  • Enabling the M1 workflow
  • Adjustments to preprocess the job matrix to exclude M1 on forks
  • Disabling some tests which failed on Mac(see comments above). Several of these tests have fixes merged, which we can validate once this changeset has merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/jackson Issues related to Jackson (JSON library) area/smallrye area/spring Issues relating to the Spring integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants