Skip to content

Build/kvm ci runner#560

Open
artemnbo wants to merge 5 commits into
mainfrom
build/kvm-ci-runner
Open

Build/kvm ci runner#560
artemnbo wants to merge 5 commits into
mainfrom
build/kvm-ci-runner

Conversation

@artemnbo

Copy link
Copy Markdown

No description provided.

@artemnbo artemnbo requested review from neithanmo, ryan-s-roberts and suchapalaver and removed request for neithanmo May 29, 2026 18:43

@suchapalaver suchapalaver left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Summary

Migrates the Rust CI and KVM sandbox-E2E lanes from arc-runner-set / kvm-enabled to a self-hosted GCP KVM runner (kvm-gcp) that auto-stops after idle, plus a reusable start-kvm-runner.yml that wakes the GCE instance via Workload Identity Federation before dependent jobs run.

The architecture is sound — WIF over static keys, idempotent start (describes status first), a concurrency group so two runs don't race the gcloud start API, and clean transitive gating (start-runner → changes → {doctor, nextest, sandbox-*}). I verified all 6 kvm-gcp jobs transitively depend on start-runner and the migration is internally consistent. A couple of things need to change before this can land on main.


🔴 Blocking

1. rust-ci.ymlon.push.branches switched from [main] to [build/kvm-ci-runner].

on:
  push:
-    branches: [main]
+    branches: [build/kvm-ci-runner]

This is the scaffolding that makes CI run on the feature branch during testing. If merged as-is, push CI stops running on main entirely — post-merge nextest/doctor/sandbox lanes would never fire again (only pull_request would). Must revert to [main] before merge.


🟠 Should address

2. start-kvm-runner.ymlsleep ${{ inputs.wait_seconds }} breaks on the workflow_dispatch path.

wait_seconds is declared only under workflow_call, not workflow_dispatch. A manual dispatch renders the step as bare sleep sleep: missing operand → step fails. Add wait_seconds to the workflow_dispatch inputs, or default it in the shell (sleep "${{ inputs.wait_seconds || 60 }}").

3. Self-hosted runner + pull_request trigger — execution-surface risk given open-sourcing.

rust-ci.yml triggers on pull_request and runs build/test (arbitrary code: build scripts, proc-macros, tests) on the self-hosted kvm-gcp instance. Right after the Apache-2.0 license commit, fork PRs become a real vector. GitHub's "require approval for first-time contributors" mitigates but defaults still let returning contributors run code on the GCP box. Worth confirming the repo's fork-PR approval setting is locked down, and ideally that the WIF binding's subject condition can't be satisfied by fork refs. (sandbox-e2e-kvm.yml is fine — workflow_dispatch + weekly schedule only.)


🟢 Nits

  • start-kvm-runner.yml — the "Wait for register" sleep 60 runs unconditionally, even when describe already reported RUNNING. That's 60s burned on every CI run once the box is warm. Consider gating the sleep on whether start actually ran.
  • rust-ci.yml — the lightweight changes (paths-filter) job now consumes the kvm-gcp runner, so the instance must be awake just to compute the path filter. Acceptable since everything downstream needs it, just flagging.
  • Scope note (not a defect): arc-runner-set is still referenced by claude-md-sync.yml and k8s-pilot-validation.yml, so that runner-set must keep existing — intentional partial migration.

Recommendation: revert finding #1 (and ideally #2) first. Once on.push.branches is back to [main], the underlying runner migration is well-built and good to ship.

@suchapalaver

suchapalaver commented May 29, 2026

Copy link
Copy Markdown
Contributor

One follow-up on the on.push.branches change, re: the plan to flip it to main after merge.

Recommend doing that revert as the final commit of this PR, before merge — rather than as a post-merge change. Merging with [build/kvm-ci-runner] in place leaves a window where main's push-CI is dead until the follow-up lands, and leans on remembering the follow-up.

It also costs nothing here: once this PR is open, the pull_request trigger already runs the full CI on kvm-gcp for every push to the branch, so the runner stays fully validated by the PR checks even with on.push.branches set back to [main]. (The push.branches override is only doing extra work for branch pushes without a PR — e.g. note the cache save-if is gated on github.ref == 'refs/heads/main', so feature-branch pushes don't save cache anyway.)

Everything else in the hibernate/wake design looks good. Findings #2 (the workflow_dispatch sleep operand) and #3 (fork-PR exposure on the self-hosted runner) are non-blocking — worth a quick look but won't hold the merge.

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.

3 participants