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

Use portable builds by default #5489

Closed
wants to merge 14 commits into from

Conversation

antondlr
Copy link
Member

@antondlr antondlr commented Mar 27, 2024

Issue Addressed

Heavily reduces strain on CI infra during docker builds and releases
(docker builds: 9 -> 4* ; release builds 8 -> 4)
See #5457

Proposed Changes

Make blst-portable the default across the board and stop offering force-adx builds

Additional Info

In this PR, all current docker tags and direct release links** are preserved but lead to a portable build instead, which should work for everyone.

The idea is to release with these placeholders to try and assess how many downloads we still get on the to-be-deprecated release links.

Before we consider this, we should make absolutely sure that the performance hit, if present, is negligible on modern hardware.

(* 8 lighthouse and 1 lcli build --> 2 lighthouse and 2 lcli builds)
(** excluding the link for a non-portable windows build)

Summary of changes

  • all lighthouse builds (both binaries and docker) use the portable blst library
  • lcli is built through cross, and published as multiplatform image
  • regular docker images now include spec-minimal

@antondlr
Copy link
Member Author

antondlr commented Mar 27, 2024

Succesful CI runs (on subpar runners so timings aren't accurate):

release ci run release page
docker ci run lighthouse lcli

@antondlr
Copy link
Member Author

Michael has already run some benchmarks

next step would be for us to dogfood this and deploy portable builds across the fleet.
it's now the default choice on our builder

@michaelsproul
Copy link
Member

I'm ok with rolling it out. I was hesitant previously because the benchmarks showed a slight detriment, but we should gather some data on what impact this small slowdown has on the real workload

@paulhauner
Copy link
Member

I'm onboard to try it out 👍

@antondlr antondlr added ready-for-review The code is ready for review and removed do-not-merge labels Apr 2, 2024
@antondlr
Copy link
Member Author

antondlr commented Apr 2, 2024

I'm ok with rolling it out. I was hesitant previously because the benchmarks showed a slight detriment, but we should gather some data on what impact this small slowdown has on the real workload

We have portable builds running on our own fleet now, up to and including canaries, while mainnet runs the canonical non-portable release.
If we need more datapoints for the VC, I could update one or two mainnet VCs as they are quite homogenous, resource-wise. That should provide us with enough data.

@AgeManning
Copy link
Member

I support this change also (under the believe that Michael's performance metrics show no real/significant performance degradation)

@antondlr
Copy link
Member Author

closing in favour of #5614 (docker) and #5615 (binaries), which are cleaner, split-out versions of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra-ci ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants