Skip to content

Commit

Permalink
Remote: Use Action's salt field to differentiate cache across workspa…
Browse files Browse the repository at this point in the history
…ces.

Resolves bazelbuild#13339 (comment).

Closes bazelbuild#14184.

PiperOrigin-RevId: 410407819
  • Loading branch information
coeuvre authored and copybara-github committed Nov 17, 2021
1 parent a9d3cfb commit 8c2c78c
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}.
Expand Down Expand Up @@ -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 {
Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -446,6 +447,9 @@ public static Action buildAction(
if (platform != null) {
action.setPlatform(platform);
}
if (salt != null) {
action.setSalt(salt);
}
return action.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 8c2c78c

Please sign in to comment.