ci(bazel): source the bazel-ci container from a repo variable and run byoo first - #429
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBazel workflows now use the ChangesBazel CI configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant image-push-manual.yml
participant Bazel
Operator->>image-push-manual.yml: Provide service_path
image-push-manual.yml->>Bazel: Resolve module layout and working directory
image-push-manual.yml->>Bazel: Query scoped oci_image_index targets
Bazel-->>image-push-manual.yml: Return image targets
image-push-manual.yml-->>Operator: Build and push multi-arch images
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
balajinvda
force-pushed
the
ci/bazel-shallow-checkout
branch
from
July 24, 2026 20:41
0692055 to
e8c0c0a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BAZEL.md`:
- Around line 432-433: Update the Bazel toolchain documentation near the “Every
workflow” statement to limit the claim to containerized Bazel jobs, reflecting
that the bazel-docker job runs on ubuntu-latest and installs Bazelisk
independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b66742f-fd7a-4400-b982-a40a67a725c8
📒 Files selected for processing (4)
.github/workflows/bazel.yml.github/workflows/chart-push-manual.yml.github/workflows/image-push-manual.ymlBAZEL.md
balajinvda
force-pushed
the
ci/bazel-shallow-checkout
branch
from
July 25, 2026 02:58
e8c0c0a to
7f6df99
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/image-push-manual.yml:
- Line 149: Update the query handling around the indexes population so the bazel
query invoked by the workflow preserves and propagates failures instead of
hiding them behind mapfile process substitution. Run bazel query directly before
reading its output, while retaining the existing parsing and empty-result
behavior for successful queries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6445b402-fe05-4c65-b70b-58f305cda000
📒 Files selected for processing (4)
.github/workflows/bazel.yml.github/workflows/chart-push-manual.yml.github/workflows/image-push-manual.ymlBAZEL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/chart-push-manual.yml
- .github/workflows/bazel.yml
- BAZEL.md
Contributor
Author
|
Correction: reopening. Rescoping this PR to the Measurement showed the shallow checkout does not deliver a meaningful speedup. Per-step timings from a representative
Checkout is 7s of a 1972s job, so shallowing it saves a few seconds per row and nothing in wall-clock terms, since rows run in parallel. The cost is entirely in the build step. The |
The bazel-ci image was hardcoded in four places and had already drifted to three different versions: bazel.yml on 0.13.0, image-push-manual.yml and chart-push-manual.yml on 0.8.0, and BAZEL.md documenting 0.12.0. The release path building shipping images on a different toolchain than CI validates with is a correctness problem, not just untidiness. Source the container from the BAZEL_CI_IMAGE repository variable so the image is bumped in one place. Each workflow keeps the current pin as a || fallback so CI still runs when the variable is unavailable, for example on a fork. It must be vars, not env: GitHub Actions evaluates job-level container.image before the workflow-level env: context is reliably available, which is why the previous note told maintainers to keep the tag literal. The vars context does not have that ordering problem, so the value can now be centralized. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> (cherry picked from commit 881a51e)
…ding The manual image-push workflow carried a hardcoded choice list of 11 service paths, so every new service needed a workflow edit before its image could be pushed for pre-merge testing. cloud-tasks, for example, was not selectable. GitHub Actions cannot populate a choice input dynamically, so drop the list: service_path is a free-form path, and the workflow discovers what to build. It also assumed every subtree is its own Bazel module: it ran bazel from inside the subtree and queried //... . That is false for services in the repo-root module (the Java services), where the query finds nothing. The workflow now derives the layout from whether the subtree owns a MODULE.bazel, querying from the subtree with //... or from the root with a path-scoped pattern. Both were verified against a standalone module (grpc-proxy resolves //:image_index) and a root-module service (cloud-tasks resolves //src/control-plane-services/cloud-tasks/nvct-service:nvct-service-oss-image_index). An unknown path now fails with the list of subtrees owning a Bazel module, which replaces the discoverability the dropdown provided. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> (cherry picked from commit 7f6df99)
The repo-name rule only stripped an _image suffix, so a target using hyphens
fell through to the default branch and got both the service prefix and the
suffix left on. byoo-otel-collector-image became
byoo-otel-collector-byoo-otel-collector-image, and cloud-tasks would have
pushed nvct-service-oss as cloud-tasks-nvct-service-oss-image.
The tree uses the separator to mean two different things:
image the service's sole image; repo is the service
<component>_image a sub-component; repo is <service>-<component>
<image-name>-image the target already carries the full image name; use it
as-is without the service prefix
Add the hyphenated case. Verified against every image target in the tree: the
11 services currently selectable are unchanged, and byoo-otel-collector (2
targets) and cloud-tasks are corrected. byoo was latent because it is not in
the old hardcoded list; making the input free-form would have exposed it.
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
(cherry picked from commit 7d4fcc4)
The target discovery used mapfile with process substitution, so only stdout reached mapfile and the query's exit status was discarded. A failing query (a BUILD error, an unloadable package, a bad scope) produced an empty array and was then misreported as "no oci_image_index targets under <path>", pointing the operator at the wrong problem. Run the query directly, check its status, and build the array explicitly. The array is built in a loop rather than from a here-string because a here-string of empty output yields one empty element instead of an empty array, which would defeat the existing no-targets check. Verified all four cases: failing query surfaces the error, empty result gives 0 targets, single and multiple results parse correctly. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> (cherry picked from commit 054f2c1)
Matrix rows are dispatched top-down and capped by max-parallel: 8, so a long row placed late waits for a free slot before it starts. The byoo collector sat at row 10 and spent about four minutes queued before doing any work (run created 18:58:26, job started 19:02:22). It is the longest row in the matrix, so it now leads and starts in the first wave. This costs no additional runner concurrency; it only changes dispatch order. max-parallel stays at 8 because the remaining rows finish well inside the byoo row's runtime, so raising it would not move the run's wall clock. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
balajinvda
force-pushed
the
ci/bazel-shallow-checkout
branch
from
July 27, 2026 20:13
054f2c1 to
2771184
Compare
balajinvda
changed the title
kristinapathak
approved these changes
Jul 27, 2026
The byoo collector genrule produces a 271 MB binary that never reached
the remote cache. Bazel reported it per run:
WARNING: Remote Cache: Error while uploading artifact with digest
'71dfa2b5.../283998957'
so the genrule stayed a local action on every job. The build summary
showed the cost directly: "383 processes: 316 remote cache hit, 59
internal, 3 local" -- everything cached except the collector, rebuilt in
all three of its configurations each run.
The cause is a client/server mismatch, not a size ceiling. The cache
tolerates large slow uploads on purpose: nginx sets grpc_read_timeout and
grpc_send_timeout to 600s ("bb-storage itself tolerates 10 minute
streams, so do not be the bottleneck"), client_max_body_size to 1 GB, and
bb-storage sets maximumMessageSizeBytes to 1 GB against ~1.68 GB blocks.
271 MB clears all of those. The Bazel client, though, was left at
--remote_timeout=120, which requires sustaining ~2.4 MB/s from a
GitHub-hosted runner to us-west-2 for a single blob, three times per job.
Raise the client to 600s so it matches what the server already allows.
This only bounds how long a client waits, so it cannot break a healthy
build. It does mean a genuinely unreachable cache takes longer to give
up, but nginx keeps grpc_connect_timeout at 30s, so connection failures
still surface quickly; the longer budget applies to in-flight streams.
The cacheless-retry fallback remains the backstop.
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Three unrelated CI fixes that were sitting on one branch, rescoped after
measurement. The original shallow-checkout change was dropped: per-step
timings showed
actions/checkoutis 7s of a 1972sbazel (byoo-otel-collector)job, so shallowing it saves nothing meaningful and rows run in parallel anyway.
What remains are changes that do carry their weight.
What changed
BAZEL_CI_IMAGErepositoryvariable instead of a literal tag in each workflow, so the image is bumped
in one place. Uses
vars, notenv: job-levelcontainer.imageisevaluated before the workflow
envcontext is reliably available, whichpreviously made the matrix expand to zero jobs on push events. Keeps a
literal fallback so forks and unset-variable cases still work.
byoo-otel-collectorrow first. Matrix rows dispatch top-down undermax-parallel: 8, and byoo is the longest row, so sitting at position 10 itspent ~4 minutes queued before starting (run created 18:58:26, job started
19:02:22). This costs no extra runner concurrency, only dispatch order.
max-parallelstays at 8 because the remaining rows finish well inside thebyoo row's runtime.
derive the push repo name correctly for hyphenated image targets, and
surface
bazel queryfailures instead of masking them.Customer Release Notes
Not customer visible.
Plan Summary
Not applicable.
Usage
Set the repository variable
BAZEL_CI_IMAGEbefore merging, otherwise theliteral fallback is used.
Testing
CI on this PR. The reorder is observable as the byoo row starting in the
first dispatch wave rather than after a slot frees.
Notes
Requires the
BAZEL_CI_IMAGErepository variable to be set for theindirection to take effect.
References
None
Related Merge Requests/Pull Requests
Builds on #444 (byoo collector genrule remote-cacheable) and #471.
Dependencies
None