From b587be37b3b817879d700d7ee55c44cd884b0905 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 24 Nov 2021 16:04:22 +0100 Subject: [PATCH] Remote: Use Action's salt field to differentiate cache across workspaces. (#14320) Resolves https://github.com/bazelbuild/bazel/pull/13339#issuecomment-950686805. Closes #14184. PiperOrigin-RevId: 410407819 --- .../build/lib/analysis/platform/BUILD | 1 - .../lib/analysis/platform/PlatformUtils.java | 11 -------- .../google/devtools/build/lib/remote/BUILD | 1 + .../lib/remote/RemoteExecutionService.java | 25 ++++++++++++++++++- .../RemoteRepositoryRemoteExecutor.java | 8 +++++- .../devtools/build/lib/remote/util/Utils.java | 6 ++++- .../analysis/platform/PlatformUtilsTest.java | 21 ---------------- .../remote/RemoteExecutionServiceTest.java | 21 ++++++++++++++++ 8 files changed, 58 insertions(+), 36 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD index f9797e6453ff52..c7c42a7a2908c4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/BUILD @@ -44,7 +44,6 @@ java_library( srcs = ["PlatformUtils.java"], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/protobuf:failure_details_java_proto", "//third_party:guava", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index 503b649886b819..2773421656eee8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -19,7 +19,6 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Ordering; -import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -115,16 +114,6 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem } } - String workspace = - spawn.getExecutionInfo().get(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE); - if (workspace != null) { - platformBuilder.addProperties( - Property.newBuilder() - .setName("bazel-differentiate-workspace-cache") - .setValue(workspace) - .build()); - } - sortPlatformProperties(platformBuilder); return platformBuilder.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 562053eec62ba9..1eaa3fdf618efe 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -48,6 +48,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index aa48ba91656eb7..8cb9a9e8762716 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -73,6 +73,7 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.MetadataProvider; @@ -307,6 +308,11 @@ public String getActionId() { return actionKey.getDigest().getHash(); } + /** Returns underlying {@link Action} of this remote action. */ + public Action getAction() { + return action; + } + /** * Returns a {@link SortedMap} which maps from input paths for remote action to {@link * ActionInput}. @@ -420,6 +426,22 @@ public MerkleTree uncachedBuildMerkleTreeVisitor( return MerkleTree.merge(subMerkleTrees, digestUtil); } + @Nullable + private static ByteString buildSalt(Spawn spawn) { + String workspace = + spawn.getExecutionInfo().get(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE); + if (workspace != null) { + Platform platform = + Platform.newBuilder() + .addProperties( + Platform.Property.newBuilder().setName("workspace").setValue(workspace).build()) + .build(); + return platform.toByteString(); + } + + return null; + } + /** Creates a new {@link RemoteAction} instance from spawn. */ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context) throws IOException, UserExecException, ForbiddenActionInputException { @@ -442,7 +464,8 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context merkleTree.getRootDigest(), platform, context.getTimeout(), - Spawns.mayBeCachedRemotely(spawn)); + Spawns.mayBeCachedRemotely(spawn), + buildSalt(spawn)); ActionKey actionKey = digestUtil.computeActionKey(action); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java index 646a4dc9ebf060..69f521e84da95f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRepositoryRemoteExecutor.java @@ -132,7 +132,13 @@ public ExecutionResult execute( Digest commandHash = digestUtil.compute(command); MerkleTree merkleTree = MerkleTree.build(inputFiles, digestUtil); Action action = - buildAction(commandHash, merkleTree.getRootDigest(), platform, timeout, acceptCached); + buildAction( + commandHash, + merkleTree.getRootDigest(), + platform, + timeout, + acceptCached, + /*salt=*/ null); Digest actionDigest = digestUtil.compute(action); ActionKey actionKey = new ActionKey(actionDigest); CachedActionResult cachedActionResult; diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index ca7ca0c8d0acb0..a13faab04af198 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -433,7 +433,8 @@ public static Action buildAction( Digest inputRoot, @Nullable Platform platform, java.time.Duration timeout, - boolean cacheable) { + boolean cacheable, + @Nullable ByteString salt) { Action.Builder action = Action.newBuilder(); action.setCommandDigest(command); action.setInputRootDigest(inputRoot); @@ -446,6 +447,9 @@ public static Action buildAction( if (platform != null) { action.setPlatform(platform); } + if (salt != null) { + action.setSalt(salt); + } return action.build(); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java index fc15ae5d79f493..9af22a72f7a0ce 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java @@ -19,7 +19,6 @@ import build.bazel.remote.execution.v2.Platform; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -99,26 +98,6 @@ public void testParsePlatformSortsProperties_execProperties() throws Exception { assertThat(PlatformUtils.getPlatformProto(s, null)).isEqualTo(expected); } - @Test - public void testGetPlatformProto_differentiateWorkspace() throws Exception { - Spawn s = - new SpawnBuilder("dummy") - .withExecutionInfo(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE, "aa") - .build(); - - Platform expected = - Platform.newBuilder() - .addProperties(Platform.Property.newBuilder().setName("a").setValue("1")) - .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) - .addProperties( - Platform.Property.newBuilder() - .setName("bazel-differentiate-workspace-cache") - .setValue("aa")) - .build(); - // execProperties are sorted by key - assertThat(PlatformUtils.getPlatformProto(s, remoteOptions())).isEqualTo(expected); - } - @Test public void getPlatformProto_mergeTargetExecPropertiesWithPlatform() throws Exception { Spawn spawn = new SpawnBuilder("dummy").withExecProperties(ImmutableMap.of("c", "3")).build(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 30f13bdf32bd00..b90cc763060c69 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -40,6 +40,7 @@ import build.bazel.remote.execution.v2.OutputDirectory; import build.bazel.remote.execution.v2.OutputFile; import build.bazel.remote.execution.v2.OutputSymlink; +import build.bazel.remote.execution.v2.Platform; import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.SymlinkNode; import build.bazel.remote.execution.v2.Tree; @@ -59,6 +60,7 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; +import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.SimpleSpawn; @@ -76,6 +78,7 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.exec.util.FakeOwner; +import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteAction; import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult; import com.google.devtools.build.lib.remote.common.BulkTransferException; @@ -164,6 +167,24 @@ public final void setUp() throws Exception { remoteActionExecutionContext = RemoteActionExecutionContext.create(metadata); } + @Test + public void buildRemoteAction_differentiateWorkspace_generateActionSalt() throws Exception { + Spawn spawn = + new SpawnBuilder("dummy") + .withExecutionInfo(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE, "aa") + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + + RemoteAction remoteAction = service.buildRemoteAction(spawn, context); + + Platform expected = + Platform.newBuilder() + .addProperties(Platform.Property.newBuilder().setName("workspace").setValue("aa")) + .build(); + assertThat(remoteAction.getAction().getSalt()).isEqualTo(expected.toByteString()); + } + @Test public void downloadOutputs_outputFiles_executableBitIgnored() throws Exception { // Test that executable bit of downloaded output files are ignored since it will be chmod 555