chore: evaluate Remote Build Execution (RBE) @ Namespace - #10579
chore: evaluate Remote Build Execution (RBE) @ Namespace#10579basvandijk wants to merge 245 commits into
Conversation
basvandijk
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces an experimental GitHub Actions path to run bazel test using Namespace Bazel Remote Execution (BRE), including automation to build/mirror/optimize a worker image and keep the workflow pinned to an immutable digest.
Changes:
- Adds a new experimental workflow to run
bazel teston Namespace runners with remote execution enabled. - Extends the container autobuild workflow with jobs to mirror
ic-buildintonscr.io, optimize it for BRE, and automatically update the pinned worker-image digest used by the new workflow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/container-autobuild.yml | Adds jobs to create an optimized Namespace BRE worker image and auto-update the pinned digest reference in workflows. |
| .github/workflows/bre-namespace-test.yml | New opt-in workflow to run bazel test using Namespace remote execution with a pinned worker image. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add a new 'bre-namespace-test.yml' workflow that runs 'bazel test' on Namespace runners using Bazel Remote Execution (BRE). Actions execute on Namespace workers booted from a custom worker image (a mirror of ic-build). Extend 'container-autobuild.yml' to mirror the freshly built ic-build image into nscr.io, optimize it for BRE, and pin the resulting digest. These jobs are decoupled from the production image-reference update so an early-access BRE failure can never block it.
Addresses Copilot review: 'nsc bazel execution setup' writes short-lived credentials into the bazelrc, so 'cat'-ing it could leak auth material into the Actions logs.
…update job - Use the same fully-qualified nscr.io ref for the upload destination and the digest lookup, so the inspected tag is guaranteed to exist. - Guard bre-worker-image and bre-namespace-test against fork PRs via head.repo.full_name == github.repository (matching ci-kickoff.yml), since both run on privileged Namespace runners with pre-authenticated nsc. - Drop update-image-references from update-worker-reference's needs for true decoupling; the existing 'git pull --rebase' absorbs any concurrent push.
27f3f10 to
95ee442
Compare
…ac895bdd550cac7bacb9dad553bae ic-build: sha256:f4c6c7e0e16da470cba7ebceb0145f588d5fd4859c04acfa607bee475ecfa914 ic-dev: sha256:2f98d344d708a1ae70938d5e777a1f141f7f2a9545687653f407a405eb1a27ea
|
Run URL: https://github.com/dfinity/ic/actions/runs/30281453965 New container images with tag: |
update-image-references now depends on bre-worker-image (default needs semantics) and pins the worker digest unconditionally in the same commit, dropping the cancelled()-guard and the separate update-worker-reference job. Waiting on bre-worker-image before committing/pushing avoids the concurrency cancellation seen earlier.
nsc base-image upload prepends $NSC_CONTAINER_REGISTRY (nscr.io/<tenant>) to a relative name. Passing the fully-qualified nscr.io/dfinity ref caused a double-prefixed push (nscr.io/<tenant>/nscr.io/dfinity/...) and a 401 on the digest lookup. Derive the registry from $NSC_CONTAINER_REGISTRY, upload a relative name, pin the resulting full ref, and match any tenant in the pin sed. Also fixes a stale comment referencing the removed update-worker-reference job.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to cbabdf1. Security Overview
Detected Code Changes
|
|
✅ No security or compliance issues detected. Reviewed everything up to cbabdf1. Security Overview
Detected Code Changes
|
daniel-wong-dfinity-org-twin
left a comment
daniel-wong-dfinity-org-twin
left a comment
There was a problem hiding this comment.
approving for Governance-owned files.
…e-bazel-remote-execution
Bazel declares test.xml as an output of every test action. When a test doesn't write $XML_OUTPUT_FILE itself, StandaloneTestStrategy runs a *second* spawn (@bazel_tools//tools/test:test_xml_generator) to derive it from test.log. Under RBE that spawn has to queue for a free worker to do well under a second of work: in run 30158073133 every one of the 152 executed system tests paid a median 435s of queueing for it (p90 1726s). Convert the SystemGroupSummary into a JUnit XML document and write it to $XML_OUTPUT_FILE at the end of the parent process. Bazel only runs the generator when the test didn't produce test.xml itself, so writing it ourselves skips that spawn entirely. Also stop the test-driver e2e scenarios from inheriting XML_OUTPUT_FILE: they spawn the driver as a child process, and those children would each overwrite the test.xml that Bazel expects //rs/tests/idx:test_e2e_scenarios itself to produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it XML Every <testcase> was emitted with status="run", including the skipped ones, which contradicts the <skipped/> child element they carry and can confuse consumers that aggregate on the attribute. Derive it from the outcome instead, following GoogleTest's convention that Bazel's own test_xml_generator also emits: "run" for a case that executed, "notrun" for one that didn't. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review comments on #10908: - Validate that `cpus_oversubscription_factor` and `cpus` are ints >= 1 and `fail()` with a clear message otherwise. Previously a 0 factor produced an obscure division-by-zero at analysis time, a negative one silently produced a nonsense reservation, and a float produced a non-integral `cpu` exec_property like "16.0". - Compute the ceiling with `(cpus + factor - 1) // factor` instead of `-(-cpus // factor)`; now that both are known-positive ints, this is the clearer idiom. Verified the two formulas agree, and that all 369 `_local` targets keep byte-identical `cpu` exec_properties. - Reword the `cpus_oversubscription_factor` docstring, which was grammatically unclear ("factor to divide the requested cpus with which"). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e-bazel-remote-execution
nmattia
left a comment
nmattia
left a comment
There was a problem hiding this comment.
Really exciting! couple questions related to the GHA side of things to start with
| # This job runs a bazel build & test on the Remote Build Execution cluster @ Namespace. | ||
| # It's currently in the evaluation phase where we compare its speed and stability with the `bazel-test-all` job. | ||
| # We expect it to fail and that's OK and it shouldn't block PRs & releases. | ||
| ci-rbe-evaluation: |
There was a problem hiding this comment.
for the time being (while it's being evaluated) how about we keep this as a separate workflow (./.github/workflows/ci-rbe-evaluation.yml) with regular (eg pull_request) trigger and only wire it in here when we decide to make it blocking?
this way we keep things simple and agile for the RBE side of things and avoid modifying the infra we currently rely on for gatekeeping
There was a problem hiding this comment.
As discussed privately: we need to make sure this workflow doesn't run on PRs from forks. ci-kickoff.yml has the protection in place for that. If we would introduce a new workflow with its own pull_request trigger we would have to distribute the right if condition to every job in the workflow.
The current approach is simpler and more secure.
| id: worker | ||
| shell: bash | ||
| run: | | ||
| set -xeuo pipefail |
There was a problem hiding this comment.
| set -xeuo pipefail | |
| set -euo pipefail |
|
|
||
| ic_build_repo="ghcr.io/dfinity/ic-build" | ||
| src="${ic_build_repo}@${{ needs.ic-build-image.outputs.ic-build-imageid }}" | ||
| upload_log="$(nsc base-image upload "$src" "ic-build-worker:${{ needs.build-image-prep.outputs.image_tag }}" 2>&1)" |
| src="${ic_build_repo}@${{ needs.ic-build-image.outputs.ic-build-imageid }}" | ||
| upload_log="$(nsc base-image upload "$src" "ic-build-worker:${{ needs.build-image-prep.outputs.image_tag }}" 2>&1)" | ||
| echo "$upload_log" | ||
| worker_digest="$(printf '%s\n' "$upload_log" | grep -i 'Uploaded base image' | grep -oE 'sha256:[0-9a-f]{64}' | tail -n1 || true)" |
There was a problem hiding this comment.
probably simpler to read the digest from the build-push-action: https://github.com/docker/build-push-action#outputs
There was a problem hiding this comment.
I suggest we do a revamp of the container image builds and how we refer to them in a separate PR because I don't want to make any imageid and digest changes which don't strictly have to do with RBE.
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| ref: ${{ github.head_ref }} | ||
| ref: ${{ github.head_ref || github.ref_name }} |
There was a problem hiding this comment.
IDK anymore why I had that there. Removed.
| -m 'ic-build: ${{ needs.ic-build-image.outputs.ic-build-imageid }}' \ | ||
| -m 'ic-dev: ${{ needs.ic-build-image.outputs.ic-dev-imageid }}' | ||
| -m 'ic-dev: ${{ needs.ic-build-image.outputs.ic-dev-imageid }}' \ | ||
| -m 'ic-build-worker: ${{ needs.rbe-worker-image.outputs.worker-image }}' |
There was a problem hiding this comment.
I'm a bit surprised, why is the worker image different? isn't it just ic-build?
There was a problem hiding this comment.
It's the RBE optimised variant of ic-build
…e-bazel-remote-execution
…d04bfc8e46f6e9a1ebd61765d3472 ic-build: sha256:7d8e6aa9eae77e13e91ed30835c0fa72e05e86d5067c6d0dd14905e8c53efc36 ic-dev: sha256:06d7680338792de19f456577f37614d344e3feb119b843f535311ec5bdd66eb6 ic-build-worker: nscr.io/c9ptjuknd7oc6/ic-build-worker@sha256:28668b9bb053f840fb12354545ef4a2434cb41b7a7dfcb594afc6cb6686e4c17
…d04bfc8e46f6e9a1ebd61765d3472 ic-build: sha256:3e3ee32a5de1fde724d4ab931cd5868dad52fef823c41331494a5b36742a6907 ic-dev: sha256:ad31e83146e452c3b6c31ecbb25c1ccfda79ecd10ec16e8dedb1f6952f1dfd6c ic-build-worker: nscr.io/c9ptjuknd7oc6/ic-build-worker@sha256:145163a97a493d19798422b6b87cabb5b64d8eac50318a14395d6a25d3796439
Why
To reduce reliance on DFINITY's own Data Centers and to make developing, building and testing the IC more portable we're planning to switch the majority of CI to run on Namespace, a CI provider who DFINITY already uses for ARM64 builds on Linux and MacOS.
In particular we're planning to move the
bazel-test-alljob (AMD64 bazel build and test) to Namespace. This includes system-tests. Since Namespace does not have access to Farm the system-tests on Namespace will use theLocalsystem-test backend, i.e. each test will spawn its QEMU VMs locally on the worker running the bazel action.Considering that local system-tests are resource heavy, i.e. they can easily consume 10s of CPUs, they shouldn't be executed on a single worker because that would effectively sequentialise all system-tests which would consume almost 4 hours.
We therefor configure bazel to run its actions on a Remote Build Execution (RBE) cluster @ Namespace. This way builds and tests distribute over many workers and run in parallel. 15 workers for now of shape 32x64 that each can accept 16 action slots.
Before switching to RBE we first need to evaluate its performance and stability. That's this PR.
What
To compare our existing
bazel-test-allwith RBE we need to have a copy of that job that is as similar as possible with the only difference that it runs bazel on RBE @ Namespace. To this end:ci-kickoff.ymlis extended with aci-rbe-evaluationjob (likeci-main) which calls the newci-rbe-evaluation.ymlworkflow.ci-rbe-evaluation.ymlis a slimmed down copy ofci-main.ymlwith similar jobs likeconfig,infer-bazel-targetsandbazel-test-all-rbe. Theinfer-bazel-targetsjob will behave the same as inci-mainexcept that it excludes Farm-based system-tests; the reverse is true forinfer-bazel-targetsinci-mainwhich excludes local system-tests.bazel-test-all-rbeis similar tobazel-test-allexcept that it runs on anamespace-profile-rbe-driver-amd64-linux-16x32runner and configures bazel to use Namespace's RBE cluster.Note that this job doesn't use the same container image as
bazel-test-allbut instead uses a special optimised variant stored at Namespace's Container Registry (nscr.io).container-autobuild.ymlhas been extended with arbe-worker-imagejob which generates this special variant. It takes theic-buildcontainer image, mirrors it tonscr.ioand optimises it for RBE. The existingupdate-image-referencesjob will then make sure references to this image are updated in the code .Evaluation
Since now both
bazel-test-allandbazel-test-all-rbewill run on each update to each PR, for each push to master and for each Release Testing run we can compare the difference in performance, stability and debugability.Note that
bazel-test-all-rbeis configured withcontinue-on-error: trueto ensure it doesn't block PRs or releases in case it fails.Regarding debugability,
bazel-test-all-rbedoesn't upload its BEP data to our internal BuildBuddy. Instead it uploads logs of failed runs to Namespace's Artifact Storage from which users can download them.We'll run with this for a short while until we're confident we can switch to RBE @ Namespace completely.
Unresolved Questions
There are a currently 94 system-tests that are currently configured as Farm-only (
backend = "farm") we need to decide how to handle these when we switch to Namespace completely.