Remote: Use Action's salt field to differentiate cache across workspa…

…ces. Resolves bazelbuild#13339 (comment). Closes bazelbuild#14184. PiperOrigin-RevId: 410407819
uber-common · Nov 17, 2021 · 8c2c78c · 8c2c78c
1 parent a9d3cfb
commit 8c2c78c
Show file tree

Hide file tree

Showing 8 changed files with 58 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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