chore(bazel): remove references to the decommissioned remote cache#435
chore(bazel): remove references to the decommissioned remote cache#435balajinvda wants to merge 2 commits into
Conversation
The internal Buildbarn host was retired, but it was still referenced in nine files. The per-service .bazel-remote-probe scripts defaulted NVCF_BAZEL_REMOTE_HOST to it, so an unset variable pointed the probe at a dead host rather than cleanly disabling the cache. - Probe scripts (6): no default host. An unset NVCF_BAZEL_REMOTE_HOST now disables the cache and builds local-only, which is the safe fallback the scripts already document. No new endpoint is hardcoded in its place; the caller supplies it, matching the root .bazelrc. - nvsnap/.bazelrc: drop the dead endpoint from the :remote profile; the profile keeps only policy and tuning flags. - byoo README, grpc-proxy geo BUILD.bazel: comment references. While in the geo BUILD.bazel comment, corrected a factual error: it claimed does NOT bypass result caching. It does. is the union of no-sandbox, no-remote-exec and no-cache. The same misconception is what kept byoo-otel-collector recompiling on every CI run. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
📝 WalkthroughWalkthroughChangesRemote cache configuration
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe`:
- Around line 20-31: Add a reusable source-level behavioral test with a stubbed
grpcurl that exercises the shared remote-cache gate for an unset host, disabled
remote caching, and a configured host, then run it against all six
.bazel-remote-probe scripts:
src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe (20-31),
src/compute-plane-services/nvca/scripts/.bazel-remote-probe (19-29),
src/control-plane-services/function-autoscaler/scripts/.bazel-remote-probe
(20-31), src/control-plane-services/helm-reval/scripts/.bazel-remote-probe
(18-29),
src/invocation-plane-services/http-invocation/scripts/.bazel-remote-probe
(20-31), and
src/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe
(20-31). Verify each probe’s host and remote-enabled gate produces the expected
behavior, and run the repository-native test runner.</code>
In `@src/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazel`:
- Around line 77-80: Correct the comment describing local = True in the relevant
Bazel test configuration: state that it forces local unsandboxed execution and
disables remote execution, but does not disable all caching. Keep the
explanation that the external tag prevents Bazel from reusing cached test
results, and retain the requires-docker note.
🪄 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: 5a518dbf-60c3-427f-9e52-5eac44c08098
📒 Files selected for processing (9)
src/compute-plane-services/byoo-otel-collector/README.mdsrc/compute-plane-services/ess-agent/scripts/.bazel-remote-probesrc/compute-plane-services/nvca/scripts/.bazel-remote-probesrc/compute-plane-services/nvsnap/.bazelrcsrc/control-plane-services/function-autoscaler/scripts/.bazel-remote-probesrc/control-plane-services/helm-reval/scripts/.bazel-remote-probesrc/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazelsrc/invocation-plane-services/http-invocation/scripts/.bazel-remote-probesrc/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe
| BAZEL_REMOTE_FLAGS="" | ||
| if [ "${NVCF_BAZEL_REMOTE:-1}" != "1" ]; then | ||
| echo "[bazel-remote] NVCF_BAZEL_REMOTE=${NVCF_BAZEL_REMOTE:-unset}; remote cache disabled" | ||
| if [ "${NVCF_BAZEL_REMOTE:-1}" != "1" ] || [ -z "${NVCF_BAZEL_REMOTE_HOST:-}" ]; then | ||
| echo "[bazel-remote] remote cache disabled (NVCF_BAZEL_REMOTE=${NVCF_BAZEL_REMOTE:-unset}, NVCF_BAZEL_REMOTE_HOST=${NVCF_BAZEL_REMOTE_HOST:-unset}); building local-only" | ||
| else | ||
| host="${NVCF_BAZEL_REMOTE_HOST:-nvcfbarn.nvidia.com}" | ||
| host="${NVCF_BAZEL_REMOTE_HOST}" | ||
| port="${NVCF_BAZEL_REMOTE_PORT:-443}" | ||
| tls="${NVCF_BAZEL_REMOTE_TLS:-1}" | ||
| if [ "${tls}" = "1" ] && [ -z "${BAZEL_REMOTE_CA_PEM:-}" ]; then | ||
| # TLS requires a CA pem. grpcurl can probe with `-insecure`, but | ||
| # Bazel's own gRPC client cannot: with `grpcs://` and no | ||
| # `--tls_certificate` it tries the system trust store, which does | ||
| # not contain the nvcfbarn self-signed cert, and fails mid-build | ||
| # not contain the cache's self-signed cert, and fails mid-build |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add behavioral tests for the shared remote-cache gate.
bash -n validates syntax only. Add a reusable source-level test with a stubbed grpcurl covering host unset, remote disabled, and a configured host, then run it for all six probes.
src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe#L20-L31: cover the new host check.src/compute-plane-services/nvca/scripts/.bazel-remote-probe#L19-L29: cover the new host check.src/control-plane-services/function-autoscaler/scripts/.bazel-remote-probe#L20-L31: cover the new host check.src/control-plane-services/helm-reval/scripts/.bazel-remote-probe#L18-L29: cover the new host check.src/invocation-plane-services/http-invocation/scripts/.bazel-remote-probe#L20-L31: cover the new host check.src/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe#L20-L31: cover the new host check.
As per coding guidelines, code changes must include tests, or the Pull Request must explain why tests are not applicable; run the repository-native test runner before committing.
📍 Affects 6 files
src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe#L20-L31(this comment)src/compute-plane-services/nvca/scripts/.bazel-remote-probe#L19-L29src/control-plane-services/function-autoscaler/scripts/.bazel-remote-probe#L20-L31src/control-plane-services/helm-reval/scripts/.bazel-remote-probe#L18-L29src/invocation-plane-services/http-invocation/scripts/.bazel-remote-probe#L20-L31src/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe#L20-L31
🤖 Prompt for 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.
In `@src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe` around
lines 20 - 31, Add a reusable source-level behavioral test with a stubbed
grpcurl that exercises the shared remote-cache gate for an unset host, disabled
remote caching, and a configured host, then run it against all six
.bazel-remote-probe scripts:
src/compute-plane-services/ess-agent/scripts/.bazel-remote-probe (20-31),
src/compute-plane-services/nvca/scripts/.bazel-remote-probe (19-29),
src/control-plane-services/function-autoscaler/scripts/.bazel-remote-probe
(20-31), src/control-plane-services/helm-reval/scripts/.bazel-remote-probe
(18-29),
src/invocation-plane-services/http-invocation/scripts/.bazel-remote-probe
(20-31), and
src/invocation-plane-services/llm-api-gateway/scripts/.bazel-remote-probe
(20-31). Verify each probe’s host and remote-enabled gate produces the expected
behavior, and run the repository-native test runner.</code>
Source: Coding guidelines
| # tag) live outside Bazel's hermetic action hash. Note `local = True` | ||
| # is the union of no-sandbox, no-remote-exec AND no-cache, so it does | ||
| # bypass result caching; the `external` tag additionally stops a cached | ||
| # pass from pinning a green forever. requires-docker is kept so callers |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd --hidden --type f '^\.bazelversion$' -x sh -c '
printf "%s: " "$1"
cat "$1"
' shRepository: NVIDIA/nvcf
Length of output: 1260
🌐 Web query:
Bazel test tags local external no-cache semantics local test result caching
💡 Result:
In Bazel, the local and no-cache tags interact with test execution and result caching in distinct ways. The "local" tag (or setting the local = True attribute on a test or genrule) forces the action to run locally on the host machine [1][2]. This specifically precludes the action from being executed remotely, run inside a sandbox, or remotely cached [1]. Because remote caching relies on the reproducibility of sandbox-isolated actions, designating an action as local implicitly disables remote caching [1][3]. The "no-cache" tag is intended to signal that an action or test should never be cached [1][4][5]. However, its behavior is limited in practice: 1. Scope: The no-cache tag primarily affects the disk and remote caches [6][5]. It does not disable the in-memory Skyframe cache (which affects incremental builds) or, in many cases, the persistent local action cache maintained within a Bazel workspace [6][5]. 2. Limitations: There is a known distinction in Bazel's caching layers [6][5]. While no-cache prevents results from being stored in or retrieved from external/disk caches, users often observe that Bazel still "caches" results via the persistent action cache or Skyframe, leading to perceived inconsistencies [6][5]. 3. Workarounds: Users requiring a complete bypass of caching often use a combination of tags or command-line flags. For example, disabling the persistent action cache requires using the --nouse_action_cache flag [6][5]. Because the Skyframe cache is internal and transient to the Bazel process, it cannot be fully disabled, though it is cleared by a bazel clean or process restart [6][5]. In summary, while local and no-cache are used to control execution and caching, they are not absolute "disable all caching" switches [6][3][5]. local is primarily an execution constraint that forces local, non-sandboxed processing, whereas no-cache is a directive intended to prevent storage and retrieval from external/disk caches, but with known limitations regarding local persistent caches [1][6][5].
Citations:
- 1: https://bazel.build/reference/be/common-definitions
- 2: https://bazel-docs-staging.netlify.app/versions/1.2.0/be/general
- 3: Local actions in a remote execution build should be cachable bazelbuild/bazel#7932
- 4: cc_test with tags=["no-cache"] gets cached, crashes Bazel if run with --nocache_test_results bazelbuild/bazel#4680
- 5: no-cache tag does not disable caching of genrule bazelbuild/bazel#19909
- 6:
no-cachetag does not apply to the persistent action cache bazelbuild/bazel#15516
Do not equate local = True with no-cache.
local = True forces local, unsandboxed execution and disables remote execution/caching, but it does not turn off all caching. The external tag is what prevents Bazel from reusing a cached test result.
🤖 Prompt for 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.
In `@src/invocation-plane-services/grpc-proxy/proxy/geo/BUILD.bazel` around lines
77 - 80, Correct the comment describing local = True in the relevant Bazel test
configuration: state that it forces local unsandboxed execution and disables
remote execution, but does not disable all caching. Keep the explanation that
the external tag prevents Bazel from reusing cached test results, and retain the
requires-docker note.
Source: MCP tools
The byoo-otel-collector subtree has a version gate that requires bumping its VERSION file whenever ANY file under the subtree changes, with no exemption for documentation. Bumping the collector's version to fix a one-word comment would cut a spurious release, so leave that reference for the next byoo change that bumps VERSION for a real reason. All functional references are still removed; the remaining one is a single prose mention in a README. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
1 participant
Why
The internal Buildbarn host was retired, but nine files still referenced it. The important one is not cosmetic: the per-service
.bazel-remote-probescripts defaultedNVCF_BAZEL_REMOTE_HOSTto that host, so an unset variable pointed the probe at a dead endpoint instead of cleanly disabling the cache.What changed
NVCF_BAZEL_REMOTE_HOSTnow disables the cache and builds local-only, which is the safe fallback these scripts already document. No replacement endpoint is hardcoded; the caller supplies it, matching the root.bazelrc.nvsnap/.bazelrc: drop the dead endpoint from the:remoteprofile, leaving only policy and tuning flags.byoo-otel-collector/README.mdandgrpc-proxy/proxy/geo/BUILD.bazel: comment references.There are now zero references to the retired host in the repository.
While editing the geo
BUILD.bazelcomment, corrected a factual error. It claimedlocal = True"does NOT bypass result caching". It does:localis the union ofno-sandbox,no-remote-exec, andno-cache. That same misconception is what kept byoo-otel-collector recompiling on every CI run instead of hitting the remote cache. Only the comment changed here; the target's tags are untouched.Customer Release Notes
Not customer visible.
Testing
All six probe scripts pass
bash -n. The behavior change is fail-safe by construction: previously an unset host produced a probe against a dead endpoint, now it produces a local-only build.Notes
Not included, deliberately. This branch originally also synced every subtree
.bazelversionto the root's 9.1.1. Local validation showed that is a migration, not a file edit, so it was dropped:nats-auth-callout(Go) on 9.1.1:name 'sh_test' is not defined. Bazel 9 removedsh_testfrom the built-in globals; it needs an explicitload()plus arules_shelldependency.ratelimiter(Rust) on 9.1.1:protoc version does not match protobuf Bazel module.Two of two sampled subtrees fail, for two different reasons, so converging the 15 subtrees currently on 8.6.0 up to the root's 9.1.1 needs per-subtree work and belongs in its own tracked effort. The separate float bug (four worker subtrees carrying no
.bazelversionat all, so their release builds resolve whatever bazelisk considers latest) is fixed in #433.References
Follows the root-level cleanup in #399.
Related Merge Requests/Pull Requests
#433 (pins the four worker subtrees that ship no
.bazelversion).Dependencies
None.
Summary by CodeRabbit
Bug Fixes
Documentation