Skip to content

Enable self-hosted runners #3950

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

Merged
merged 5 commits into from
Jul 23, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

No description provided.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jul 23, 2025

I've assigned @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@TheBlueMatt TheBlueMatt changed the title 2025 07 self hosted Enable self-hosted runners Jul 23, 2025
@TheBlueMatt TheBlueMatt force-pushed the 2025-07-self-hosted branch 3 times, most recently from 1d7aefc to 1e975e2 Compare July 23, 2025 14:36
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Nice, can't wait to see how much faster it will be. Mind sharing the runner config somewhere, I'd be interested to see how it's set up.

Any CI job which runs on Linux and doesn't use any GitHub Actions
tasks aside from `checkout` should move to self-hosted, which we do
here.
@TheBlueMatt TheBlueMatt force-pushed the 2025-07-self-hosted branch from 1e975e2 to f5b259d Compare July 23, 2025 14:41
There's no reason to set the default toolchain when we just
installed the toolchain. Also, for some reason this now fails since
the `$PATH` isn't updated by the installer in the same shell so
`rustup` isn't found.
These are now available by default
@TheBlueMatt TheBlueMatt force-pushed the 2025-07-self-hosted branch from f5b259d to 5238928 Compare July 23, 2025 15:07
@TheBlueMatt
Copy link
Collaborator Author

Okay, this should work now 🎉. The runners are all ephemeral so after one job they'll self-destruct and get rebuilt from an image but that all looks like its working.

Copy link

codecov bot commented Jul 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.94%. Comparing base (e36abb7) to head (2e35242).
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3950      +/-   ##
==========================================
- Coverage   89.02%   88.94%   -0.08%     
==========================================
  Files         168      168              
  Lines      121831   121962     +131     
  Branches   121831   121962     +131     
==========================================
+ Hits       108457   108485      +28     
- Misses      10970    11078     +108     
+ Partials     2404     2399       -5     
Flag Coverage Δ
fuzzing 22.70% <ø> (+0.01%) ⬆️
tests 88.77% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheBlueMatt
Copy link
Collaborator Author

Its not materially faster, sadly, but that's while running a number of tasks in parallel (currently set as 10 runners in parallel). It should net us a lot less waiting on CI, though, I hope.

Mind sharing the runner config somewhere, I'd be interested to see how it's set up.

Its split across a few places, so not really sure how to put it anywhere. Its just a container image that has the required things pre-installed, gets started by a script, configured as a fresh runner (they make it real easy, its just a config script), then run, then torn down once a task finishes, and we go again.

@TheBlueMatt
Copy link
Collaborator Author

Anyway, once CI passes this should be ready to land.

@joostjager
Copy link
Contributor

Its not materially faster, sadly, but that's while running a number of tasks in parallel (currently set as 10 runners in parallel). It should net us a lot less waiting on CI, though, I hope.

How is it faster then? Because our CI jobs always have a lot of tasks in parallel.

And is it possible to do 50 runners, or doesn't that work with resources available?

@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Jul 23, 2025

How is it faster then? Because our CI jobs always have a lot of tasks in parallel.

Today when most contributors are awake we'll often end up with like 3 or 4 hours of backlog before the free runners get around to picking up all the jobs we have pending, so even if its just providing us more parallel tasks that will still be a big win.

Also, I anticipate the fuzz job will be a good bit faster with 8 cores rather than just 2.

And is it possible to do 50 runners, or doesn't that work with resources available?

I don't think we'll get a lot of value running too many more. There's obviously a memory limit, we could probably do something like 12 or 16 runners, but we'll see how things perform and its easy to tweak later.

This target is actually quite slow, but was added after we tuned
the fuzzing runtime, so here we also reduce the iterations on
lsps_message_target as well.
@TheBlueMatt
Copy link
Collaborator Author

CI-only.

@TheBlueMatt TheBlueMatt merged commit c4f94e0 into lightningdevkit:main Jul 23, 2025
28 checks passed
@joostjager
Copy link
Contributor

joostjager commented Jul 24, 2025

Perhaps another quick win is to only build macos, windows and beta on main? 10 fewer checks for PRs

The probability of one of those specifically failing seems low, and if a quick fix isn't available, we can revert the merge.

@tnull
Copy link
Contributor

tnull commented Jul 24, 2025

Perhaps another quick win is to only build macos, windows and beta on main? 10 fewer checks for PRs

I disagree, we should really run tests on all platforms. If we necessarily need all of them is a bit separate discussion, but yes, there can be (and have been) platform-specific edge cases, especially when it comes to stuff like threading, file system behavior, etc. (and as recently mentioned elsewhere, IMO we need more threaded test coverage going forward)

@joostjager
Copy link
Contributor

My proposal is to only run those after merge on main. Trading off the risk of rare instances of breaking main with reduced CI time.

@tnull
Copy link
Contributor

tnull commented Jul 24, 2025

My proposal is to only run those after merge on main. Trading off the risk of rare instances of breaking main with reduced CI time.

Ah, then I misunderstood what you wrote above. Sorry!

@TheBlueMatt
Copy link
Collaborator Author

Given no one really ever looks at the main CI status, I'm a bit heasitant to do that. But I did go ahead and remove the beta jobs at #3958

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

Successfully merging this pull request may close these issues.

5 participants