Skip to content
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

[7.1.0] Let scrubbed actions fall back to local execution when remote execution is enabled. #21384

Merged
merged 1 commit into from Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -341,7 +341,8 @@ public CachePolicy getWriteCachePolicy(Spawn spawn) {
public boolean mayBeExecutedRemotely(Spawn spawn) {
return remoteCache instanceof RemoteExecutionCache
&& remoteExecutor != null
&& Spawns.mayBeExecutedRemotely(spawn);
&& Spawns.mayBeExecutedRemotely(spawn)
&& !isScrubbedSpawn(spawn, scrubber);
}

@VisibleForTesting
Expand Down Expand Up @@ -1596,6 +1597,10 @@ void report(Event evt) {
}
}

private static boolean isScrubbedSpawn(Spawn spawn, @Nullable Scrubber scrubber) {
return scrubber != null && scrubber.forSpawn(spawn) != null;
}

/**
* A simple value class combining a hash of the tool inputs (and their digests) as well as a set
* of the relative paths of all tool inputs.
Expand Down
Expand Up @@ -314,10 +314,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {

boolean enableScrubbing = remoteOptions.scrubber != null;
if (enableScrubbing && enableRemoteExecution) {

throw createOptionsExitException(
"Cannot combine remote cache key scrubbing with remote execution",
FailureDetails.RemoteOptions.Code.EXECUTION_WITH_SCRUBBING);
env.getReporter()
.handle(
Event.warn(
"Cache key scrubbing is incompatible with remote execution. Actions that are"
+ " scrubbed per the --experimental_remote_scrubbing_config configuration"
+ " file will be executed locally instead."));
}

// TODO(bazel-team): Consider adding a warning or more validation if the remoteDownloadRegex is
Expand Down
Expand Up @@ -728,9 +728,11 @@ public RemoteOutputsStrategyConverter() {
+ " cache entries and result in incorrect builds.\n\n"
+ "Scrubbing does not affect how an action is executed, only how its remote/disk"
+ " cache key is computed for the purpose of retrieving or storing an action result."
+ " It cannot be used in conjunction with remote execution. Modifying the scrubbing"
+ " configuration does not invalidate outputs present in the local filesystem or"
+ " internal caches; a clean build is required to reexecute affected actions.\n\n"
+ " Scrubbed actions are incompatible with remote execution, and will always be"
+ " executed locally instead.\n\n"
+ "Modifying the scrubbing configuration does not invalidate outputs present in the"
+ " local filesystem or internal caches; a clean build is required to reexecute"
+ " affected actions.\n\n"
+ "In order to successfully use this feature, you likely want to set a custom"
+ " --host_platform together with --experimental_platform_in_output_dir (to normalize"
+ " output prefixes) and --incompatible_strict_action_env (to normalize environment"
Expand Down
3 changes: 2 additions & 1 deletion src/main/protobuf/failure_details.proto
Expand Up @@ -295,7 +295,8 @@ message RemoteOptions {
CREDENTIALS_WRITE_FAILURE = 3 [(metadata) = { exit_code: 36 }];
DOWNLOADER_WITHOUT_GRPC_CACHE = 4 [(metadata) = { exit_code: 2 }];
EXECUTION_WITH_INVALID_CACHE = 5 [(metadata) = { exit_code: 2 }];
EXECUTION_WITH_SCRUBBING = 6 [(metadata) = { exit_code: 2 }];

reserved 6;
}

Code code = 1;
Expand Down
30 changes: 24 additions & 6 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Expand Up @@ -3384,28 +3384,46 @@ EOF
bazel build --experimental_remote_scrubbing_config=scrubbing.cfg \
--//:src=//:foo :gen &> $TEST_log \
|| fail "failed to build with input foo and no cache"
expect_log "2 processes: 1 internal, 1 .*-sandbox"
expect_log "2 processes: 1 internal, 1 .*-sandbox"

bazel build --experimental_remote_scrubbing_config=scrubbing.cfg \
--//:src=//:bar :gen &> $TEST_log \
|| fail "failed to build with input bar and no cache"
expect_log "2 processes: 1 internal, 1 .*-sandbox"

# Now build with a cache. Even though Bazel considers the actions distinct,
# they will be looked up in the remote cache using the scrubbed key, so one
# can serve as a cache hit for the other.
# Now build with a disk cache. Even though Bazel considers the actions to be
# distinct, they will be looked up in the remote cache using the scrubbed key,
# so one can serve as a cache hit for the other.

CACHEDIR=$(mktemp -d)

bazel build --experimental_remote_scrubbing_config=scrubbing.cfg \
--disk_cache=$CACHEDIR --//:src=//:foo :gen &> $TEST_log \
|| fail "failed to build with input foo and a cache"
|| fail "failed to build with input foo and a disk cache miss"
expect_log "2 processes: 1 internal, 1 .*-sandbox"

bazel build --experimental_remote_scrubbing_config=scrubbing.cfg \
--disk_cache=$CACHEDIR --//:src=//:bar :gen &> $TEST_log \
|| fail "failed to build with input bar and a cache"
|| fail "failed to build with input bar and a disk cache hit"
expect_log "2 processes: 1 disk cache hit, 1 internal"

# Now build with remote execution enabled. The first action should fall back
# to local execution. The second action should be able to hit the remote cache
# as it was cached by its scrubbed key.

bazel build --experimental_remote_scrubbing_config=scrubbing.cfg \
--remote_executor=grpc://localhost:${worker_port} --//:src=//:foo \
:gen &> $TEST_log \
|| fail "failed to build with input foo and remote execution"
expect_log "will be executed locally instead"
expect_log "2 processes: 1 internal, 1 .*-sandbox"

bazel build --experimental_remote_scrubbing_config=scrubbing.cfg \
--remote_executor=grpc://localhost:${worker_port} --//:src=//:bar \
:gen &> $TEST_log \
|| fail "failed to build with input bar and a remote cache hit"
expect_log "will be executed locally instead"
expect_log "2 processes: 1 remote cache hit, 1 internal"
}

run_suite "Remote execution and remote cache tests"