From cdf118a607ab179807cd39d0cafdb551859d1cd1 Mon Sep 17 00:00:00 2001 From: ulfjack Date: Mon, 28 May 2018 03:02:06 -0700 Subject: [PATCH] Enable --experimental_remote_spawn_cache by default - It is now an error to specify the gRPC remote execution backend in combination with a local disk or HTTP-based cache. - It is now an error to specify both local disk and HTTP-based caches. Note that before this CL, enabling the local disk cache silently disabled remote execution - we now give an error in that case. With these combination no longer being accepted, remote execution being enabled now means that we only create a RemoteSpawnRunner, and don't provide a SpawnCache. This is not a semantic change - we never created both. In principle, it should be possible for users to combine local execution with remote caching for actions that are marked local or no-remote, and still use remote execution otherwise. However, Bazel cannot currently express this combination of execution strategies. RELNOTES: The --experimental_remote_spawn_cache flag is now enabled by default, and remote caching no longer needs --*_strategy=remote flags (it will fail if they are specified). PiperOrigin-RevId: 198280398 --- .../remote/RemoteActionContextProvider.java | 6 ++-- .../build/lib/remote/RemoteModule.java | 32 +++++++++++++------ .../build/lib/remote/RemoteOptions.java | 3 +- .../lib/remote/SimpleBlobStoreFactory.java | 2 +- .../remote/remote_execution_http_test.sh | 26 --------------- .../bazel/remote/remote_execution_test.sh | 12 ------- 6 files changed, 28 insertions(+), 53 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index e6222b4ebe20f8..4bcbb74ff1a3c0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -38,8 +38,8 @@ */ final class RemoteActionContextProvider extends ActionContextProvider { private final CommandEnvironment env; - private final AbstractRemoteActionCache cache; - private final GrpcRemoteExecutor executor; + @Nullable private final AbstractRemoteActionCache cache; + @Nullable private final GrpcRemoteExecutor executor; private final DigestUtil digestUtil; private final Path logDir; @@ -64,7 +64,7 @@ public Iterable getActionContexts() { String buildRequestId = env.getBuildRequestId().toString(); String commandId = env.getCommandId().toString(); - if (remoteOptions.experimentalRemoteSpawnCache || remoteOptions.diskCache != null) { + if (executor == null && cache != null) { RemoteSpawnCache spawnCache = new RemoteSpawnCache( env.getExecRoot(), diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 36521f987bab91..11a6119bae473c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -97,7 +97,7 @@ public void serverInit(OptionsProvider startupOptions, ServerBuilder builder) } @Override - public void beforeCommand(CommandEnvironment env) { + public void beforeCommand(CommandEnvironment env) throws AbruptExitException { env.getEventBus().register(this); String buildRequestId = env.getBuildRequestId().toString(); String commandId = env.getCommandId().toString(); @@ -113,6 +113,7 @@ public void beforeCommand(CommandEnvironment env) { } catch (IOException e) { env.getReporter() .handle(Event.error("Could not create base directory for remote logs: " + logDir)); + throw new AbruptExitException(ExitCode.LOCAL_ENVIRONMENTAL_ERROR, e); } RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); @@ -126,10 +127,22 @@ public void beforeCommand(CommandEnvironment env) { return; } - try { - boolean remoteOrLocalCache = SimpleBlobStoreFactory.isRemoteCacheOptions(remoteOptions); - boolean grpcCache = GrpcRemoteCache.isRemoteCacheOptions(remoteOptions); + boolean enableRestCache = SimpleBlobStoreFactory.isRestUrlOptions(remoteOptions); + boolean enableDiskCache = SimpleBlobStoreFactory.isDiskCache(remoteOptions); + if (enableRestCache && enableDiskCache) { + throw new AbruptExitException( + "Cannot enable HTTP-based and local disk cache simultaneously", + ExitCode.COMMAND_LINE_ERROR); + } + boolean enableBlobStoreCache = enableRestCache || enableDiskCache; + boolean enableGrpcCache = GrpcRemoteCache.isRemoteCacheOptions(remoteOptions); + if (enableBlobStoreCache && remoteOptions.remoteExecutor != null) { + throw new AbruptExitException( + "Cannot combine gRPC based remote execution with local disk or HTTP-based caching", + ExitCode.COMMAND_LINE_ERROR); + } + try { LoggingInterceptor logger = null; if (!remoteOptions.experimentalRemoteGrpcLog.isEmpty()) { rpcLogFile = new AsynchronousFileOutputStream(remoteOptions.experimentalRemoteGrpcLog); @@ -139,10 +152,8 @@ public void beforeCommand(CommandEnvironment env) { RemoteRetrier retrier = new RemoteRetrier( remoteOptions, RemoteRetrier.RETRIABLE_GRPC_ERRORS, Retrier.ALLOW_ALL_CALLS); - // TODO(davido): The naming is wrong here. "Remote"-prefix in RemoteActionCache class has no - // meaning. final AbstractRemoteActionCache cache; - if (remoteOrLocalCache) { + if (enableBlobStoreCache) { cache = new SimpleBlobStoreActionCache( remoteOptions, @@ -151,9 +162,9 @@ public void beforeCommand(CommandEnvironment env) { GoogleAuthUtils.newCredentials(authAndTlsOptions), env.getWorkingDirectory()), digestUtil); - } else if (grpcCache || remoteOptions.remoteExecutor != null) { + } else if (enableGrpcCache || remoteOptions.remoteExecutor != null) { // If a remote executor but no remote cache is specified, assume both at the same target. - String target = grpcCache ? remoteOptions.remoteCache : remoteOptions.remoteExecutor; + String target = enableGrpcCache ? remoteOptions.remoteCache : remoteOptions.remoteExecutor; Channel ch = GoogleAuthUtils.newChannel(target, authAndTlsOptions); if (logger != null) { ch = ClientInterceptors.intercept(ch, logger); @@ -169,6 +180,8 @@ public void beforeCommand(CommandEnvironment env) { cache = null; } + // TODO(davido): The naming is wrong here. "Remote"-prefix in RemoteActionCache class has no + // meaning. final GrpcRemoteExecutor executor; if (remoteOptions.remoteExecutor != null) { Channel ch = GoogleAuthUtils.newChannel(remoteOptions.remoteExecutor, authAndTlsOptions); @@ -184,7 +197,6 @@ public void beforeCommand(CommandEnvironment env) { } else { executor = null; } - actionContextProvider = new RemoteActionContextProvider(env, cache, executor, digestUtil, logDir); } catch (IOException e) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java index 3431e95dac036b..ebef95e8b20e27 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java @@ -164,11 +164,12 @@ public final class RemoteOptions extends OptionsBase { ) public double experimentalRemoteRetryJitter; + @Deprecated @Option( name = "experimental_remote_spawn_cache", defaultValue = "false", documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.UNKNOWN}, + effectTags = {OptionEffectTag.NO_OP}, help = "Whether to use the experimental spawn cache infrastructure for remote caching. " + "Enabling this flag makes Bazel ignore any setting for remote_executor." diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java index 424a6886f41797..47367560e9cab9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java @@ -77,7 +77,7 @@ public static boolean isDiskCache(RemoteOptions options) { return options.diskCache != null; } - private static boolean isRestUrlOptions(RemoteOptions options) { + static boolean isRestUrlOptions(RemoteOptions options) { return options.remoteHttpCache != null; } } diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh index 215ea742194803..8de00d2c4f0e3a 100755 --- a/src/test/shell/bazel/remote/remote_execution_http_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh @@ -78,7 +78,6 @@ EOF bazel clean --expunge >& $TEST_log bazel build \ - --experimental_remote_spawn_cache=true \ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote REST cache service" @@ -112,7 +111,6 @@ EOF bazel clean --expunge >& $TEST_log bazel build \ - --experimental_remote_spawn_cache=true \ --remote_http_cache=http://bad.hostname/bad/cache \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote REST cache service" @@ -136,15 +134,6 @@ genrule( ) EOF bazel build \ - --genrule_strategy=remote \ - --noremote_allow_symlink_upload \ - --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ - //:make-link &> $TEST_log \ - && fail "should have failed" || true - expect_log "/l is a symbolic link" - - bazel build \ - --experimental_remote_spawn_cache \ --noremote_allow_symlink_upload \ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ //:make-link &> $TEST_log \ @@ -161,15 +150,6 @@ genrule( ) EOF bazel build \ - --genrule_strategy=remote \ - --noremote_allow_symlink_upload \ - --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ - //:make-link &> $TEST_log \ - && fail "should have failed" || true - expect_log "dir/l is a symbolic link" - - bazel build \ - --experimental_remote_spawn_cache \ --noremote_allow_symlink_upload \ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ //:make-link &> $TEST_log \ @@ -224,7 +204,6 @@ function test_directory_artifact() { bazel build \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote execution" diff bazel-genfiles/a/qux/out.txt a/test_expected \ @@ -235,7 +214,6 @@ function test_directory_artifact_grpc_cache() { set_directory_artifact_testfixtures bazel build \ - --spawn_strategy=remote \ --remote_cache=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote gRPC cache" @@ -247,7 +225,6 @@ function test_directory_artifact_rest_cache() { set_directory_artifact_testfixtures bazel build \ - --spawn_strategy=remote \ --remote_rest_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote REST cache" @@ -320,7 +297,6 @@ function test_directory_artifact_skylark() { bazel build \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote execution" diff bazel-genfiles/a/qux/out.txt a/test_expected \ @@ -331,7 +307,6 @@ function test_directory_artifact_skylark_grpc_cache() { set_directory_artifact_skylark_testfixtures bazel build \ - --spawn_strategy=remote \ --remote_cache=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote gRPC cache" @@ -343,7 +318,6 @@ function test_directory_artifact_skylark_rest_cache() { set_directory_artifact_skylark_testfixtures bazel build \ - --spawn_strategy=remote \ --remote_rest_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote REST cache" diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index ecbddb65326022..f56b5fb36d6a3f 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -89,7 +89,6 @@ EOF bazel build \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote execution" diff bazel-bin/a/test ${TEST_TMPDIR}/test_expected \ @@ -119,7 +118,6 @@ EOF bazel test \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ || fail "Failed to run //a:test with remote execution" @@ -144,7 +142,6 @@ EOF bazel clean --expunge >& $TEST_log bazel build \ - --spawn_strategy=remote \ --remote_cache=localhost:${worker_port} \ //a:test >& $TEST_log \ || fail "Failed to build //a:test with remote gRPC cache service" @@ -168,7 +165,6 @@ EOF bazel test \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ && fail "Expected test failure" || true @@ -194,7 +190,6 @@ EOF int main() { std::cout << "Fail me!" << std::endl; return 1; } EOF bazel test \ - --spawn_strategy=remote \ --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ @@ -219,7 +214,6 @@ EOF int main() { std::cout << "Fail me!" << std::endl; return 1; } EOF bazel test \ - --experimental_remote_spawn_cache \ --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ @@ -236,7 +230,6 @@ EOF int main() { std::cout << "Fail me again!" << std::endl; return 1; } EOF bazel test \ - --experimental_remote_spawn_cache \ --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ @@ -272,7 +265,6 @@ EOF bazel build \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ //a:large_output >& $TEST_log \ || fail "Failed to build //a:large_output with remote execution" diff bazel-genfiles/a/large_blob.txt ${TEST_TMPDIR}/large_blob_expected.txt \ @@ -296,7 +288,6 @@ EOF bazel test \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ || fail "Failed to run //a:test with remote execution" @@ -331,7 +322,6 @@ EOF bazel test \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ || fail "Failed to run //a:test with remote execution" @@ -370,7 +360,6 @@ EOF bazel test \ --spawn_strategy=remote \ --remote_executor=localhost:${worker_port} \ - --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \ && fail "Expected test failure" || true @@ -400,7 +389,6 @@ package(default_visibility = ["//visibility:public"]) empty(name = 'test') EOF bazel build \ - --spawn_strategy=remote \ --remote_cache=localhost:${worker_port} \ --test_output=errors \ //a:test >& $TEST_log \