CI benchmarks: DX tweaks and next steps

[aochagavia]

As mentioned in #1487, we now have a dedicated server to run the CI benchmarks. Before we start using it, we need to think about two points that have an impact on DX (developer experience).

1. The benchmarks will run in an environment that is no longer ephemeral, which means we need to be extra careful when allowing benchmarks to run (because we are running arbitrary code from the internet);
2. We can use custom software running on the server to coordinate the benchmark runs, giving us more freedom in how we trigger them and how we report results.

Here are some ideas on how this affects DX:

1. Automatically triggering icount benchmarks: the automatic trigger has already been useful to identify regressions (e.g. here) and it'd be nice to keep it. We can still use a GitHub Actions job to trigger the icount bench run, as long as we make the job require maintainer approval for users who are not collaborators of the repository. Implementation wise, the benchmarks would no longer run in the GitHub runner itself, but the runner would instead send a signed POST request to our server, telling it to enqueue a bench run. That could happen in two flavors:
   1. The server keeps the HTTP connection open until the benchmarks are done and sends the results in the response. This could take ~2 minutes in the best case, or longer if we have to wait for previously enqueued runs to complete (wasting GitHub Actions time and potentially causing contention if multiple runners are blocked waiting for the bench server). The main benefit here is that we could keep reporting significant results in the same way we are doing now (i.e. as a CI failure + a summary). Having a CI failure is nice, because it is practically impossible to miss. Waiting for a response by the server would also surface any unexpected server errors.
   2. The server immediately returns an empty response (i.e. the GitHub Actions job is always successful, unless there are connectivity issues). Later, when the results become available, they are posted as a comment to the PR (in a way similar to the test coverage comment, which gets updated automatically after each push). This is more friendly towards the GitHub Actions runners, but we lose the ability to let the CI fail. There is also a chance that internal errors in the bench runner go unnoticed (e.g. a crash that kills the current bench run and prevents the results comment from ever being posted to GitHub).
2. Manually triggering time-based benchmarks: these benchmarks are not necessary for most PRs, but they could be manually triggered by posting a comment to the PR addressed at a bot (something like @rustls-bot bench). The results would be posted back by the bot as a comment, similar to how this is done for the development of the Rust compiler.
3. Automatically triggering icount + walltime benchmark runs for each push to main: this allows us to keep track of results over time, both for visualization purposes and for calculating the appropriate noise threshold per benchmark. We should definitely keep the results around locally, and it might make sense to push them to something like bencher.dev to get fancy graphs.

After this wall of text, here are two questions:

1. For the icount benchmarks, my gut feeling is that we should let the GitHub Actions job finish early. The lack of CI failures is IMO compensated by the PR comment with the bench results. Do you agree? Regarding the possibility of server failures going unnoticed, I think we can make the server robust enough that the only point of failure is GitHub being unavailable when we try to post the results (in case of an error, we would post that as a comment too).
2. Do you have any other remarks or suggestions?

[djc]

All of this sounds good to me, I agree with your arguments regarding the icount benchmarks.

[ctz]

For the icount benchmarks, my gut feeling is that we should let the GitHub Actions job finish early.

Agree.

Ideally this asynchronous process would (as well as posting a comment when done) also hit the github commit status API as it goes, so we can see when such a run is pending, and which commit is being tested. This need not be more complex than calling curl once when a new run is triggered, and once when it completes or fails.

[cpu]

The server keeps the HTTP connection open until the benchmarks are done and sends the results in the response. This could take ~2 minutes in the best case, or longer if we have to wait for previously enqueued runs to complete (wasting GitHub Actions time and potentially causing contention if multiple runners are blocked waiting for the bench server).

I'm not sure this option is viable. I thought that GitHub required a response to an outbound webhook within ~10 seconds or it's considered a delivery failure.

The server immediately returns an empty response (i.e. the GitHub Actions job is always successful, unless there are connectivity issues). Later, when the results become available, they are posted as a comment to the PR (in a way similar to the test coverage comment, which gets updated automatically after each push). This is more friendly towards the GitHub Actions runners, but we lose the ability to let the CI fail. There is also a chance that internal errors in the bench runner go unnoticed (e.g. a crash that kills the current bench run and prevents the results comment from ever being posted to GitHub).

I think this workflow matches up well with the checks API. When we receive a webhook we can create a queued check before initiating benchmarking. The branch protection rules can be configured to require the benchmarking check pass before a branch could be merged. Internal errors in the runner would leave the check in a queued/pending/running status and not a completed status. Once benchmarking is completed the check could be updated to the completed status required for merge.

Ideally this asynchronous process would (as well as posting a comment when done) also hit the github commit status API as it goes, so we can see when such a run is pending, and which commit is being tested.

I think the check API would let us accomplish these same goals (seeing when a run is pending, etc). Admittedly it is a more complicated API surface to work with. However, I used it fairly extensively at a previous job for programmatic CI tasks and would be happy to help work through the details if it's interesting to folks (but I'm also happy to see us use a simpler approach to start).

[aochagavia]

Thanks for the comments! I'll look into the APIs you mentioned.

Regarding server setup and deployments, I was thinking of automating stuff with Ansible so it all requires minimal effort to maintain. The CI Bench HTTP server would run natively as a systemd service, behind a reverse proxy (probably nginx + certbot). I'm also open to suggestions on this front ;)

[cpu]

I was thinking of automating stuff with Ansible so it all requires minimal effort to maintain.

That sounds OK to me. I have Ansible experience if needed. It's not the lightest-weight solution but I agree it's a very worthwhile goal to have a reproducible setup instead of a snowflake machine. Something Docker based is probably the second best option but I'm not a huge fan generally (and I'll save you enduring a pitch for NixOS since I suspect the unfamiliarity of the tooling is too high effort for a group endeavour 😆).

behind a reverse proxy (probably nginx + certbot).

SGTM!

[djc]

I'd probably use a minimal hyper server and instant-acme for certificates rather than nginx, but that might be because I'm not very familiar with nginx.

[aochagavia]

I'd probably use a minimal hyper server and instant-acme

My current prototype uses axum on top of hyper, so it should be doable. However, I expect maintainance would be easier with nginx + certbot (i.e. configure once and forget, automatically receiving updates whenever relevant). Or do you have a strong preference for the instant-acme approach?

[djc]

Whatever will be easiest to maintain -- not very strong.

[aochagavia]

I've been fighting the GitHub API and GitHub Actions to create a suitable replacement for the current system. Here's a brain dump, partly to get my own thoughts straight, partly to get your feedback (@cpu I'm curious to hear any insights you might have, based on your experience with programmatic CI tasks).

The goal: we want to run the icount benchmarks on our bare-metal server, while keeping the simplicity of the current setup where icount benchmarks are automatically run after each push to a PR.

The challenge: we need to prevent random people on the internet from enqueuing benchmark runs on our server, because that equates to giving them remote code execution capabilities.

Attempted solution:

• Keep using a GitHub Actions job, but require approval for PRs from forks to ensure the code is safe to run.
• Instead of running the benchmarks in the GH Actions runner, use the runner to POST the necessary data to our server (e.g. using curl) and return immediately. That causes a job to be enqueued in our custom server, with progress (pending / success / failure) being reported through the commit status API.
• The POST from GH Actions to our server includes a pre-shared secret in the payload for authentication. Requests that fail to authenticate are ignored.

Problem: the GitHub API won't allow us to access repository secrets when the PR originates from a fork, even for GitHub Actions that require approval. This means that, for fork-originated PRs, we can't include the secret in the POST payload.

Alternatives:

• Keep the attempted solution, but turn the GH Actions run into a no-op for PRs from forks. When a bench run for a fork-originated PR is desired, a maintainer must post a comment to the PR, which will be automatically picked up by the server (e.g. @rustls-bot bench icount). Not ideal, but maybe the most pragmatic option.
• Ditch GitHub Actions entirely for icount benchmarking purposes. Come up with something custom to track PRs and trigger / approve jobs. (I think this would result in too much scope creep, but I'm mentioning it for completeness.)
• Use a self-hosted GH Actions runner that communicates with the bench server in a way that cannot be spoofed from the internet (e.g. creating a file in a watched directory). (This is too much of a hack for my taste and has a few other minor drawbacks, but it's technically possible.)
• Something else?

[djc]

Why do we need the shared secret?

(Note that @cpu is unavailable today, he'll be back tomorrow.)

[aochagavia]

Otherwise anyone can POST to our server, telling it to compare rustls main against an arbitrary fork. This involves running the benchmarking code from both versions of rustls, and it is possible that the fork has been modified in such a way that the code for "running benchmarks" has been changed to mine bitcoin, DOS a website, etc.

[djc]

But the PR and any approvals on it are public data, right? We could write a little code that looks for that context, without any shared secrets involved?

[aochagavia]

That sounds like a viable solution! I'll investigate.

[djc]

(I recommend the octocrab crate.)