switch high-perf-docker to runs-on.com runners #13658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13658 +/- ##
===========================================
- Coverage 70.8% 58.9% -11.9%
===========================================
Files 2299 817 -1482
Lines 451793 195823 -255970
===========================================
- Hits 319875 115392 -204483
+ Misses 131918 80431 -51487 ☔ View full report in Codecov by Sentry. |
geekflyer
force-pushed
the
runs-on
branch
2 times, most recently
from
1ebb379 to
064b008
Compare
geekflyer
added
CICD:run-e2e-tests
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
geekflyer
force-pushed
the
runs-on
branch
3 times, most recently
from
5e2a4c1 to
3214c47
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
geekflyer
force-pushed
the
runs-on
branch
4 times, most recently
from
b46ffc9 to
22b89f2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
geekflyer
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
sionescu
approved these changes
Jun 18, 2024
| @@ -1,4 +1,4 @@ | |||
| self-hosted-runner: | |||
| # Labels of self-hosted runners in array of string | |||
| labels: | |||
| - high-perf-docker | |||
| - runs-on,cpu=64,family=c7,hdd=500,image=ubuntu22-full-x64,run-id=${{ github.run_id }} | |||
There was a problem hiding this comment.
Is this necessary ? It looks like a label not a runner spec.
There was a problem hiding this comment.
this is just to tell the actionlint linter not to complain about this in the action yamls.
ibalajiarun
approved these changes
Jun 18, 2024
geekflyer
added a commit
that referenced
this pull request
Jun 18, 2024
This reverts commit 916dba1.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This switches high-perf-docker (c2-standard-60) to roughly equivalent runs-on.com runners. Specifically AWS machines of the c7 family (runs-on has some heuristics to pick the cheapest spot instance in the c7 family that satisfies the CPU requirement of 64 CPUs).
Test Plan
Ran several tests a bunch of times. Compared performance to baseline PR (#13698 ). Performance is roughly similar, usually a bit faster (5-15%), few times a bit slower (less than 5%).
Re spot preemption: Haven't seen a single spot preemption so far (added a log-based chart here https://aptoslabs.grafana.net/d/cb066e70-b378-4de7-aadb-79b43386e664/gha-self-hosted-runners?orgId=1&from=now-30d&to=now ).
Note: Some of the checks below are failing but to the best of my ability they are just broken and also fail in the baseline PR that uses high-perf-docker.
Something to note
Runs-on in some cases actually picks machines with higher CPU count (96+ etc.). This due its heuristics of picking the cheapest instance, (sometimes the bigger machines are cheaper, who knew).
Probably not a problem for most jobs, but may lead to some surprised for perf/benchmarking jobs.
I think its worth the tradeoff for these types of jobs anyways.