[9.3.2] Remove remote endpoint and instance flags from action execution salt. (#32254) Including `--remote_cache`, `--remote_executor`, `--remote_downloader`, `--remote_instance_name`, `--remote_bytestream_uri_prefix`, and `--remote_proxy` in the action execution salt invalidates the entire action cache whenever any of these flags change across builds. With `--rewind_lost_inputs` enabled by default, Bazel already rewinds and regenerates only the cached actions whose remote outputs are missing from the new cache instance without invalidating unrelated cached actions. Fixes https://github.com/bazelbuild/bazel/issues/32048. Closes #32253. PiperOrigin-RevId: 1009722875 Change-Id: I4e6eb97b00a0c5e13dc457f11d5197a5c4a7287c (cherry picked from commit 0ebd37a61ebb4285742bbd7068f888f1581a6147) Co-authored-by: chiwang <chiwang@google.com>
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 633cae1..ad89468 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
@@ -1328,18 +1328,6 @@ fp.addStringMap( remoteOptions != null ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableMap.of()); - // When switching cache endpoints/instances across invocations, cached remote actions reference - // blobs stored in the previous cache instance which cannot be downloaded from the new cache - // instance. See https://github.com/bazelbuild/bazel/issues/23780. - if (remoteOptions != null) { - fp.addNullableString(remoteOptions.remoteCache); - fp.addNullableString(remoteOptions.remoteExecutor); - fp.addNullableString(remoteOptions.remoteDownloader); - fp.addNullableString(remoteOptions.remoteInstanceName); - fp.addNullableString(remoteOptions.remoteBytestreamUriPrefix); - fp.addNullableString(remoteOptions.remoteProxy); - } - return fp.hexDigestAndReset(); }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 07b16b7..08a99a8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
@@ -1757,4 +1757,188 @@ // Assert: the generating action of the symlink's target was rewound assertValidOutputFile("a/link", "foo\n"); } + + @Test + public void actionRewinding_changeRemoteExecutor_topLevelOutputsRewoundWithoutInvalidatingCached() + throws Exception { + assumeFalse(useDiskCache); + writeOutputDirRule(); + write("BUILD"); + write( + "a/BUILD", + """ + load("//:output_dir.bzl", "output_dir") + + genrule( + name = "cached_file", + srcs = [], + outs = ["cached.out"], + cmd = "echo -n cached > $@", + ) + + output_dir( + name = "cached_tree.out", + content_map = {"file-inside": "cached-tree"}, + ) + + output_dir( + name = "remote_tree.out", + content_map = {"file-inside": "remote-tree"}, + ) + + genrule( + name = "foo", + srcs = [], + outs = ["foo.out"], + cmd = "echo -n foo > $@", + ) + + genrule( + name = "bar", + srcs = [ + ":foo.out", + "bar.in", + ], + outs = ["bar.out"], + cmd = "cat $(location :foo.out) $(location bar.in) > $@", + ) + """); + write("a/bar.in", "one"); + addOptions("--remote_instance_name=instance1", "--remote_download_regex=.*cached.*"); + + // Build on the initial worker: cached.out and cached_tree.out are downloaded locally, while + // foo.out, bar.out, and remote_tree.out remain remote-only. + buildTarget("//a:cached_file", "//a:cached_tree.out", "//a:remote_tree.out", "//a:bar"); + + // Switch to a fresh remote worker and instance name across invocations. Cached actions whose + // outputs are already available locally (//a:cached_file, //a:cached_tree.out) must not be + // invalidated, while remote-only outputs needed as top-level outputs in the new build + // (//a:remote_tree.out, //a:bar, and its transitive input //a:foo) must be rewound. + var secondWorker = IntegrationTestUtils.createWorker(); + try (var ignored = secondWorker.start()) { + // Add the new options before waitDownloads, which internally creates the next command and + // parses the options. + addOptions( + "--remote_executor=grpc://localhost:" + secondWorker.getPort(), + "--remote_instance_name=instance2", + "--remote_bytestream_uri_prefix=custom-prefix"); + enableActionRewinding(); + setDownloadToplevel(); + waitDownloads(); + assertValidOutputFile("a/cached.out", "cached"); + assertValidOutputFile("a/cached_tree.out/file-inside", "cached-tree"); + assertOutputDoesNotExist("a/remote_tree.out/file-inside"); + assertOutputDoesNotExist("a/foo.out"); + assertOutputDoesNotExist("a/bar.out"); + + var rewoundKeys = rewindingTestsHelper.collectOrderedRewoundKeys(); + + buildTarget("//a:cached_file", "//a:cached_tree.out", "//a:remote_tree.out", "//a:bar"); + waitDownloads(); + + assertRewoundActions(rewoundKeys, "//a:remote_tree.out", "//a:bar", "//a:foo"); + assertValidOutputFile("a/cached.out", "cached"); + assertValidOutputFile("a/cached_tree.out/file-inside", "cached-tree"); + assertValidOutputFile("a/remote_tree.out/file-inside", "remote-tree"); + assertValidOutputFile("a/bar.out", "fooone\n"); + assertThat(secondWorker.hasCasBlob("cached".getBytes(UTF_8))).isFalse(); + assertThat(secondWorker.hasCasBlob("cached-tree".getBytes(UTF_8))).isFalse(); + assertThat(secondWorker.hasCasBlob("remote-tree".getBytes(UTF_8))).isTrue(); + assertThat(secondWorker.hasCasBlob("foo".getBytes(UTF_8))).isTrue(); + } + } + + @Test + public void actionRewinding_changeRemoteInstance_incrementalBuildRewindsOnlyMissingInputs( + @TestParameter boolean localExecution) throws Exception { + assumeFalse(useDiskCache); + writeOutputDirRule(); + write("BUILD"); + write( + "a/BUILD", + """ + load("//:output_dir.bzl", "output_dir") + + genrule( + name = "cached_file", + srcs = [], + outs = ["cached.out"], + cmd = "echo -n cached > $@", + ) + + output_dir( + name = "cached_tree.out", + content_map = {"file-inside": "cached-tree"}, + ) + + genrule( + name = "remote_file", + srcs = [], + outs = ["remote.out"], + cmd = "echo -n remote > $@", + ) + + output_dir( + name = "remote_tree.out", + content_map = {"file-inside": "remote-tree"}, + ) + + genrule( + name = "consumer", + srcs = [ + ":cached.out", + ":cached_tree.out", + ":remote.out", + ":remote_tree.out", + "consumer.in", + ], + outs = ["consumer.out"], + cmd = "cat $(location :cached.out) $(location :cached_tree.out)/file-inside $(location :remote.out) $(location :remote_tree.out)/file-inside $(location consumer.in) > $@", + ) + """); + write("a/consumer.in", "one"); + addOptions("--remote_instance_name=instance1", "--remote_download_regex=.*cached.*"); + + // Build //a:consumer on the initial worker so that both cached_tree.out and remote_tree.out are + // recorded as uploaded in MerkleTreeComputer's persistent subtree cache. + buildTarget("//a:consumer"); + + var secondWorker = IntegrationTestUtils.createWorker(); + try (var ignored = secondWorker.start()) { + write("a/consumer.in", "two"); + if (localExecution) { + addOptions( + "--remote_executor=", + "--remote_cache=grpc://localhost:" + secondWorker.getPort(), + "--strategy_regexp=.*=local"); + } else { + addOptions("--remote_executor=grpc://localhost:" + secondWorker.getPort()); + } + addOptions("--remote_instance_name=instance2"); + enableActionRewinding(); + setDownloadToplevel(); + waitDownloads(); + assertValidOutputFile("a/cached.out", "cached"); + assertValidOutputFile("a/cached_tree.out/file-inside", "cached-tree"); + assertOutputDoesNotExist("a/remote.out"); + assertOutputDoesNotExist("a/remote_tree.out/file-inside"); + + var rewoundKeys = rewindingTestsHelper.collectOrderedRewoundKeys(); + + buildTarget("//a:consumer"); + waitDownloads(); + + // Locally present inputs (cached.out and cached_tree.out) are reused/reuploaded without + // rewinding their generating actions, whereas remote-only inputs missing from the new worker + // (remote.out and remote_tree.out) are rewound and regenerated. + assertRewoundActions(rewoundKeys, "//a:remote_file", "//a:remote_tree.out"); + assertValidOutputFile("a/consumer.out", "cachedcached-treeremoteremote-treetwo\n"); + if (!localExecution) { + assertThat(secondWorker.hasCasBlob("cached".getBytes(UTF_8))).isTrue(); + assertThat(secondWorker.hasCasBlob("cached-tree".getBytes(UTF_8))).isTrue(); + } + assertThat(secondWorker.hasCasBlob("remote".getBytes(UTF_8))).isTrue(); + assertThat(secondWorker.hasCasBlob("remote-tree".getBytes(UTF_8))).isTrue(); + } + } }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java index 33e5767..a467e98 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java
@@ -14,7 +14,6 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static com.google.devtools.build.lib.util.io.CommandExtensionReporter.NO_OP_COMMAND_EXTENSION_REPORTER; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -875,37 +874,44 @@ } @Test - public void testComputeActionExecutionSalt_changesWhenCacheInstanceChanges() throws Exception { + public void computeActionExecutionSalt_remoteEndpointOrInstanceChanges_doesNotChange() + throws Exception { RemoteOptions baseOptions = parseRemoteOptions( "--remote_cache=grpc://cache1", "--remote_executor=grpc://exec1", "--remote_instance_name=instance1"); String baseSalt = RemoteModule.computeActionExecutionSalt(baseOptions); - assertThat(RemoteModule.computeActionExecutionSalt(null)).isNotNull(); - assertThat(RemoteModule.computeActionExecutionSalt(baseOptions)).isEqualTo(baseSalt); + assertThat(RemoteModule.computeActionExecutionSalt(null)).isNotEqualTo(baseSalt); + assertThat(RemoteModule.computeActionExecutionSalt(parseRemoteOptions())) + .isNotEqualTo(baseSalt); + assertThat( + RemoteModule.computeActionExecutionSalt( + parseRemoteOptions( + "--remote_cache=grpc://cache1", + "--remote_default_exec_properties=OSFamily=linux"))) + .isNotEqualTo(baseSalt); RemoteOptions changedInstanceName = parseRemoteOptions( "--remote_cache=grpc://cache1", "--remote_executor=grpc://exec1", "--remote_instance_name=instance2"); - assertThat(RemoteModule.computeActionExecutionSalt(changedInstanceName)).isNotEqualTo(baseSalt); + assertThat(RemoteModule.computeActionExecutionSalt(changedInstanceName)).isEqualTo(baseSalt); RemoteOptions changedRemoteCache = parseRemoteOptions( "--remote_cache=grpc://cache2", "--remote_executor=grpc://exec1", "--remote_instance_name=instance1"); - assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteCache)).isNotEqualTo(baseSalt); + assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteCache)).isEqualTo(baseSalt); RemoteOptions changedRemoteExecutor = parseRemoteOptions( "--remote_cache=grpc://cache1", "--remote_executor=grpc://exec2", "--remote_instance_name=instance1"); - assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteExecutor)) - .isNotEqualTo(baseSalt); + assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteExecutor)).isEqualTo(baseSalt); RemoteOptions changedRemoteDownloader = parseRemoteOptions( @@ -914,7 +920,7 @@ "--remote_instance_name=instance1", "--experimental_remote_downloader=grpc://downloader1"); assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteDownloader)) - .isNotEqualTo(baseSalt); + .isEqualTo(baseSalt); RemoteOptions changedBytestreamPrefix = parseRemoteOptions( @@ -923,7 +929,7 @@ "--remote_instance_name=instance1", "--remote_bytestream_uri_prefix=host/instance2"); assertThat(RemoteModule.computeActionExecutionSalt(changedBytestreamPrefix)) - .isNotEqualTo(baseSalt); + .isEqualTo(baseSalt); RemoteOptions changedRemoteProxy = parseRemoteOptions( @@ -931,6 +937,6 @@ "--remote_executor=grpc://exec1", "--remote_instance_name=instance1", "--remote_proxy=unix:/socket2"); - assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteProxy)).isNotEqualTo(baseSalt); + assertThat(RemoteModule.computeActionExecutionSalt(changedRemoteProxy)).isEqualTo(baseSalt); } }
diff --git a/src/test/shell/bazel/remote/remote_utils.sh b/src/test/shell/bazel/remote/remote_utils.sh index 1234d44..ed7a6b0 100644 --- a/src/test/shell/bazel/remote/remote_utils.sh +++ b/src/test/shell/bazel/remote/remote_utils.sh
@@ -24,10 +24,7 @@ pid_file="${TEST_TMPDIR}/remote.pid_file" mkdir -p "${work_path}" mkdir -p "${cas_path}" - # Preserve port across restarts so tests simulating cache eviction don't invalidate action execution salt. - if [[ -z "${worker_port:-}" ]]; then - worker_port=$(pick_random_unused_tcp_port) || fail "no port found" - fi + worker_port=$(pick_random_unused_tcp_port) || fail "no port found" native_lib="${BAZEL_RUNFILES}/src/main/native/" "${REMOTE_WORKER}" \ --singlejar \