| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
ci: Tokenless self-hosted runner select #39900
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
Conversation
Signed-off-by: Delan Azabani <dazabani@igalia.com>
9094e6b to
95a1fee
Compare
|
another interesting idea while reading this PR: can we just trigger some kind of github webhook (label or smth with less noise) and subscribe monitor to that webhook. This way we do not need to have any "public" API for this (all data would be encoded in this webhook event). |
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Signed-off-by: Delan Azabani <dazabani@igalia.com>
potentially yea! it might be tricky for reserve-or-fallback scenarios like we do now, but for self-hosted-only workflows it looks like a good way to communicate demand and autoscale the cluster. |
|
this should be ready for review now :) |
jschwe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments, but otherwise looks good to me. I believe sagudev also wanted to review this, so we should probably wait for that review too.
sagudev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certainly interesting to review, given that there is also code in https://github.com/servo/ci-runners/blob/61dd2d53d1c5af41e2f83d2aaa65961e5e6d797c/monitor/src/main.rs#L237. I think we should probably remove some complexity and UI spamming (with artifacts).
monitor code is fair game! to be honest, i should probably start getting my monitor changes reviewed by someone else, but no one has expressed interest prior to now.
thanks, i’ll see what i can do. i like the idea of using the artifact creation time, because determining the job run start time is annoyingly difficult (it requires us to put a uuid in the name so we can uniquely identify the job in the job tree). |
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Signed-off-by: Delan Azabani <dazabani@igalia.com>
sagudev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise LGTM
Signed-off-by: Delan Azabani <dazabani@igalia.com>
Pull Request is not mergeable
this patch extends servo#39900, making lint jobs run on self-hosted runners if available. Testing: - before (15min) <https://github.com/servo/servo/actions/runs/18532931828/job/52820332815> - after (<3min) <https://github.com/servo/servo/actions/runs/18839433347/job/53748113347> Fixes: part of servo#38141 Signed-off-by: Delan Azabani <dazabani@igalia.com>
currently our self-hosted runner system falls back to github-hosted runners if there’s no available capacity at the exact moment of the select runner request. this is suboptimal, because if the job would take 5x as long on github-hosted runners, then you could wait up to 80% of that time for a self-hosted runner and still win. this patch implements a new global queue service that allows self-hosted runner jobs to wait for available capacity. the service will run on [one server](https://ci0.servo.org) for now, as a single queue that dispatches to all available servers, like any efficient supermarket. queueing a job works like this: 1. **POST /profile/<profile_key>/enqueue?<unique_id>&<qualified_repo>&<run_id>** (tokenful) or **POST /enqueue?<unique_id>&<qualified_repo>&<run_id>** (tokenless) to enqueue a job. - both endpoints return a random token that is used to authenticate the client in the next step. - the tokenless endpoint validates that the request came from an authorised job, [using an artifact](servo/servo#39900). - the request is rejected if no servers are configured to target non-zero runners for the requested profile, because we may never be able to satisfy it. - there are no limits to queue depth (at least not yet), but clients probably have better knowledge of the nature of their job anyway, and in theory, they could use that knowledge to decide how long to wait (see below). 2. **POST /take/<unique_id>?<token>** to try to take the runner for the enqueued job. once capacity is available, this endpoint is effectively proxied to POST /profile/<profile_key>/take on one of the underlying servers. - if the client failed to provide the correct token from the previous step, the response is HTTP 403. - if the unique id is unknown, because it expired or the queue service restarted, the response is HTTP 404. - if there’s no capacity yet, the response is HTTP 503. repeat after waiting for ‘Retry-After’ seconds. - if taking the runner was successful, the response is HTTP 200, with the runner details as JSON. - if taking the runner was somehow unsuccessful (bug), the response is HTTP 200, with `null` as JSON. this sucks, to be honest, but it was also true for the underlying monitor API. - when we fix this, we should be careful about curl --retry. - clients are free to abandon a queued job without actually taking it, by doing nothing for 30 seconds. for now, the runner-select action client abandons a queued job if it has been waiting for one hour. i’ve added a “self-test” workflow that can be manually dispatched to test the new flow (e.g. [ok 1](https://github.com/delan/servo-ci-runners/actions/runs/19192407233/job/54868629052#step:3:138), [ok 2](https://github.com/delan/servo-ci-runners/actions/runs/19192417407/job/54868653437#step:3:138), [ok 3](https://github.com/delan/servo-ci-runners/actions/runs/19192424667/job/54868671844#step:3:138), [unsatisfiable](https://github.com/delan/servo-ci-runners/actions/runs/19192441772/job/54868712126#step:3:138), [unauthorised](https://github.com/delan/servo-ci-runners/actions/runs/19192458274/job/54868749313#step:3:138)). you can also play around with this locally by spinning up a monitor and a queue on your own machine, then sending the requests by hand (so three separate terminals): - `$ cargo build && sudo IMAGE_DEPS_DIR=$(nix eval --raw .\#image-deps) LIB_MONITOR_DIR=. $CARGO_TARGET_DIR/debug/monitor` - `$ cargo build && sudo IMAGE_DEPS_DIR=$(nix eval --raw .\#image-deps) LIB_MONITOR_DIR=. $CARGO_TARGET_DIR/debug/queue` - `$ unique_id=$RANDOM; curl --fail-with-body -sSX POST --retry-max-time 3600 --retry 3600 --retry-delay 1 'https://192.168.100.1:8002/take/'"$unique_id"'?token='"$(curl --fail-with-body -sSX POST --oauth2-bearer "$SERVO_CI_MONITOR_API_TOKEN" 'https://192.168.100.1:8002/profile/servo-windows10/enqueue?unique_id='"$unique_id"'&qualified_repo=delan/servo&run_id=123')"`
one of the biggest gaps in our self-hosted runner coverage is that pull request checks can’t run on self-hosted runners, because reserving a runner currently requires a MONITOR_API_TOKEN secret that is unavailable to pull requests.
this patch replaces the authenticated requests to POST /profile/<image>/take?unique_id&qualified_repo&run_id with unauthenticated requests to POST /select-runner?unique_id&qualified_repo&run_id, in the runner select jobs. to guard the CI monitors against abuse, the runner select job must prove that its request was genuine by uploading a small artifact containing the parameters of that request, which the monitors can then verify. once the job is marked as done, the monitors will refuse further reservation requests.
note that the old authenticated endpoint still works, so this patch can be reverted if anything breaks, or we can turn it off with the usual methods (force-github-hosted-runner or NO_SELF_HOSTED_RUNNERS).
Testing:
Fixes: part of #38141