wasmtime / PR #3740 Add workflow to test perf using sight... · git-wasmtime

When a push to main happens I think this'll double-checkout main and build it twice?

Ideally though it'd be awesome if we could do just one build as part of this PR and use stored data for previous runs perhaps for comparison. We should be guaranteed that all commits on main have performance information so comparing a new merge to main or a PR is ideally just about picking the revision to compare against.

Wasmtime GitHub notifications bot (Jan 31 2022 at 15:52):

alexcrichton created PR review comment:

I think we'll want to confirm the security story here before merging this. Unfortunately hooking up hosted runners to public repos is explicitly recommended against in the github actions documentation because it becomes pretty easy to run arbitrary code on these hosted runners from external contributors via PRs. I think there's various measures we could put in place to mitigate that but we'll want to make sure that's all in order before having this all hooked up.

Wasmtime GitHub notifications bot (Jan 31 2022 at 15:52):

alexcrichton created PR review comment:

Is it possible to use the main branch of sightglass? (unsure if this is largely for testing or whether it's intended that we periodically update this)

Wasmtime GitHub notifications bot (Jan 31 2022 at 15:52):

alexcrichton created PR review comment:

I think we might be able to detect this with various env vars set on github actions for prs?

Wasmtime GitHub notifications bot (Jan 31 2022 at 15:52):

alexcrichton created PR review comment:

Personally I'd prefer to keep the usage of external actions to a minimum for the wasmtime repo, so here if we can arrange for rustup to be installed on the hosted runner this I think could be rustup update stable && rustup default stable

Wasmtime GitHub notifications bot (Jan 31 2022 at 15:52):

alexcrichton created PR review comment:

Although if not my main concern about using third-party actions is the security implications. AFAIK we're largely giving all these actions read/write access to the wasmtime repo so a push to this jwalton/gh-find-current-pr repo could end up compromising us.

One way to mitigate this perhaps would be to run this workflow in a separate repository that is triggered from this repository (or something like that). That way if our other repository gets messed up it's not the end of the world.

Wasmtime GitHub notifications bot (Jan 31 2022 at 18:18):

jlb6740 created PR review comment:

I think there is some mechanism for generating/confirming with tokens that may be what we want. I don't believe I use that here as I was just following the github doc for a basic setup but it is something I'll look into. Also, since you highlighted the on: push/pull_request, note I want to change this to be on comment .. where the trigger occurs on a comment with the string /bench_x64 or something like that.

Wasmtime GitHub notifications bot (Jan 31 2022 at 18:18):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 18:20):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 18:20):

jlb6740 created PR review comment:

This is a ref from the main branch of sightglas .. it's actually HEAD as of today. However I wanted to talk about versioning with sightglass so as to be more transparent about what we are using to run tests.

Wasmtime GitHub notifications bot (Jan 31 2022 at 18:34):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 18:34):

jlb6740 created PR review comment:

To be honest, I think there is some caching that is taking place via this github code that lives on the server. I do though the blast the sightglass directory and rebuild that. I don't blast the wasmtime directories but at some point during developing this it seemed those directories were removed but then very quickly rebuilt by the environment, implying to me that something was cached.

This iteration doesn't actually test against much of anything (that will be critiqued for improvement), but as a result this entire process from start to finish only takes about 4-1/2 minutes, so even if everything is rebuilt it currently really doesn't take that long. Also like I mentioned above (and suggested offline), I want this to only run on demand not after every PR is submitted or updated. For this patch, I wanted to have something that was for sure going to work every time and then get fancy with the caching and other efficiency improvements on a future iteration. If there is something obvious to change now though, lets do it.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:24):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:24):

jlb6740 created PR review comment:

I think we can get away with not using this findPr or other third-party actions which I'll try to do. That said, when you say "AFAIK we're largely giving all these actions read/write access to the wasmtime repo so a push to this jwalton/gh-find-current-pr repo could end up compromising us." do you mean read/write access to just the downloaded wasmtime repository that is housed on the remote server or are you thinking access to the wasmtime repository as it lives in github?

Wasmtime GitHub notifications bot (Feb 01 2022 at 20:47):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 20:47):

alexcrichton created PR review comment:

One thing I'd be worried about is that once we have a custom runner hooked up any contributor can make a PR with a new workflow file to run on all PRs which uses the custom runner which I think means we basically don't have any ability to limit what runs on the runner unless we can hard configure it to ignore PRs entirely (or something like that)

Wasmtime GitHub notifications bot (Feb 01 2022 at 20:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 20:48):

alexcrichton created PR review comment:

Ah ok if it's fast enough then seems reasonable to at least start with this and we can alter later if it ever becomes necessary

Wasmtime GitHub notifications bot (Feb 01 2022 at 20:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 20:49):

alexcrichton created PR review comment:

I believe that workflows generally get read/write tokens to the bytecodealliance/wasmtime repository that lives on github, so I'm under the impression that a malicious workflow could compromise big chunks of the source code and repo

Wasmtime GitHub notifications bot (Feb 07 2022 at 21:38):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2022 at 21:38):

jlb6740 created PR review comment:

This was discussed today and I think the solution is to just only allow to have the privileges to run. Anyone else can run what this is running offline before committing. I will look into a way to have this be triggered via a comment (which I already have but need to work out a wrinkle) and then look to see if we can discover the author of that comment to allow running of the code based on that comment author.

Wasmtime GitHub notifications bot (Feb 08 2022 at 16:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2022 at 16:01):

alexcrichton created PR review comment:

FWIW I'm mostly parroting this which says:

We recommend that you only use self-hosted runners with private repositories

and this

As a result, self-hosted runners should almost never be used for public repositories on GitHub

I don't know why the "almost" is there, though.

Wasmtime GitHub notifications bot (Jul 11 2022 at 16:56):

fitzgen closed without merge PR #3740.

Last updated: Nov 22 2024 at 16:03 UTC