Skip to content

Commit 07d40ce

Browse files
wiwaWin Wang
wiwa
authored and
Win Wang
committed
Remote persistent workers off 5.3.0
1 parent c8c0169 commit 07d40ce

10 files changed

Lines changed: 262 additions & 36 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java‎

Lines changed: 55 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import build.bazel.remote.execution.v2.Platform;
1818
import build.bazel.remote.execution.v2.Platform.Property;
1919
import com.google.common.base.Strings;
20+
import com.google.common.collect.ImmutableMap;
2021
import com.google.common.collect.ImmutableSortedMap;
2122
import com.google.common.collect.Ordering;
2223
import com.google.devtools.build.lib.actions.Spawn;
@@ -63,6 +64,14 @@ public static Platform buildPlatformProto(Map<String, String> executionPropertie
6364

6465
@Nullable
6566
public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions remoteOptions)
67+
throws UserExecException {
68+
// ???! Allow additionalProperties
69+
return getPlatformProto(spawn, remoteOptions, ImmutableMap.of());
70+
}
71+
72+
@Nullable
73+
public static Platform getPlatformProto(
74+
Spawn spawn, @Nullable RemoteOptions remoteOptions, Map<String, String> additionalProperties)
6675
throws UserExecException {
6776
SortedMap<String, String> defaultExecProperties =
6877
remoteOptions != null
@@ -71,34 +80,44 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem
7180

7281
if (spawn.getExecutionPlatform() == null
7382
&& spawn.getCombinedExecProperties().isEmpty()
74-
&& defaultExecProperties.isEmpty()) {
83+
&& defaultExecProperties.isEmpty()
84+
&& additionalProperties.isEmpty()) {
7585
return null;
7686
}
7787

78-
Platform.Builder platformBuilder = Platform.newBuilder();
79-
88+
// ???? Move the the PlatformBuilder to... later?
89+
Map<String, String> properties;
8090
if (!spawn.getCombinedExecProperties().isEmpty()) {
81-
Map<String, String> combinedExecProperties;
8291
// Apply default exec properties if the execution platform does not already set
8392
// exec_properties
8493
if (spawn.getExecutionPlatform() == null
8594
|| spawn.getExecutionPlatform().execProperties().isEmpty()) {
86-
combinedExecProperties = new HashMap<>();
87-
combinedExecProperties.putAll(defaultExecProperties);
88-
combinedExecProperties.putAll(spawn.getCombinedExecProperties());
95+
properties = new HashMap<>();
96+
properties.putAll(defaultExecProperties);
97+
properties.putAll(spawn.getCombinedExecProperties());
8998
} else {
90-
combinedExecProperties = spawn.getCombinedExecProperties();
91-
}
92-
93-
for (Map.Entry<String, String> entry : combinedExecProperties.entrySet()) {
94-
platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue());
99+
properties = spawn.getCombinedExecProperties();
95100
}
101+
// Empty spawn.getCombinedExecProperties
102+
// Check for remoteExecutionProperties (and an executionPlatform)
96103
} else if (spawn.getExecutionPlatform() != null
97104
&& !Strings.isNullOrEmpty(spawn.getExecutionPlatform().remoteExecutionProperties())) {
105+
// This is pretty inefficient;
106+
// it would be better to store the parsed properties instead of the String text proto.
107+
properties = new HashMap<>();
98108
// Try and get the platform info from the execution properties.
99109
try {
100-
TextFormat.getParser()
101-
.merge(spawn.getExecutionPlatform().remoteExecutionProperties(), platformBuilder);
110+
// ???! platformBuilder is here in order to parse properties, presumably
111+
Platform.Builder platformBuilder = Platform.newBuilder();
112+
TextFormat
113+
.getParser()
114+
.merge(
115+
spawn.getExecutionPlatform().remoteExecutionProperties(),
116+
platformBuilder
117+
);
118+
for (Property property : platformBuilder.getPropertiesList()) {
119+
properties.put(property.getName(), property.getValue());
120+
}
102121
} catch (ParseException e) {
103122
String message =
104123
String.format(
@@ -107,12 +126,31 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem
107126
throw new UserExecException(
108127
e, createFailureDetail(message, Code.INVALID_REMOTE_EXECUTION_PROPERTIES));
109128
}
129+
110130
} else {
111-
for (Map.Entry<String, String> property : defaultExecProperties.entrySet()) {
112-
platformBuilder.addProperties(
113-
Property.newBuilder().setName(property.getKey()).setValue(property.getValue()).build());
131+
// for (Map.Entry<String, String> property : defaultExecProperties.entrySet()) {
132+
// platformBuilder.addProperties(
133+
// Property.newBuilder().setName(property.getKey()).setValue(property.getValue()).build());
134+
// }
135+
properties = defaultExecProperties;
136+
}
137+
138+
// ???! We always fill in properties with additionalProperties
139+
if (!additionalProperties.isEmpty()) {
140+
if (properties.isEmpty()) {
141+
properties = additionalProperties;
142+
} else {
143+
// Merge the two maps.
144+
properties = new HashMap<>(properties);
145+
properties.putAll(additionalProperties);
114146
}
115147
}
148+
// ???! Finally, the platform we build has all the properties we need.
149+
// This is more changes than needed to simply add additional properties, pretty much.
150+
Platform.Builder platformBuilder = Platform.newBuilder();
151+
for (Map.Entry<String, String> entry : properties.entrySet()) {
152+
platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue());
153+
}
116154

117155
sortPlatformProperties(platformBuilder);
118156
return platformBuilder.build();

‎src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
* probably should not exist, but is currently necessary for our local MacOS support.
2525
*/
2626
public interface LocalEnvProvider {
27+
28+
LocalEnvProvider NOOP = (env, binTools, fallbackTmpDir) -> ImmutableMap.copyOf(env);
2729

2830
/**
2931
* Creates a local environment provider for the current OS.

‎src/main/java/com/google/devtools/build/lib/remote/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ java_library(
7474
"//src/main/java/com/google/devtools/build/lib/exec:spawn_input_expander",
7575
"//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
7676
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
77+
"//src/main/java/com/google/devtools/build/lib/exec/local",
7778
"//src/main/java/com/google/devtools/build/lib/packages",
7879
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
7980
"//src/main/java/com/google/devtools/build/lib/profiler",
@@ -90,6 +91,7 @@ java_library(
9091
"//src/main/java/com/google/devtools/build/lib/sandbox",
9192
"//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier",
9293
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
94+
"//src/main/java/com/google/devtools/build/lib/util",
9395
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
9496
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
9597
"//src/main/java/com/google/devtools/build/lib/util:exit_code",
@@ -98,6 +100,7 @@ java_library(
98100
"//src/main/java/com/google/devtools/build/lib/vfs",
99101
"//src/main/java/com/google/devtools/build/lib/vfs:output_service",
100102
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
103+
"//src/main/java/com/google/devtools/build/lib/worker",
101104
"//src/main/java/com/google/devtools/common/options",
102105
"//src/main/protobuf:failure_details_java_proto",
103106
"//third_party:auth",

‎src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java‎

Lines changed: 95 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,14 @@
7575
import com.google.devtools.build.lib.actions.Spawn;
7676
import com.google.devtools.build.lib.actions.SpawnResult;
7777
import com.google.devtools.build.lib.actions.Spawns;
78-
import com.google.devtools.build.lib.actions.UserExecException;
7978
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
8079
import com.google.devtools.build.lib.analysis.platform.PlatformUtils;
8180
import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent;
8281
import com.google.devtools.build.lib.events.Event;
8382
import com.google.devtools.build.lib.events.Reporter;
8483
import com.google.devtools.build.lib.exec.SpawnInputExpander.InputWalker;
8584
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
85+
import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
8686
import com.google.devtools.build.lib.profiler.Profiler;
8787
import com.google.devtools.build.lib.profiler.ProfilerTask;
8888
import com.google.devtools.build.lib.profiler.SilentCloseable;
@@ -108,10 +108,15 @@
108108
import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput;
109109
import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution;
110110
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
111+
import com.google.devtools.build.lib.util.Fingerprint;
111112
import com.google.devtools.build.lib.util.io.FileOutErr;
112113
import com.google.devtools.build.lib.vfs.FileSystemUtils;
113114
import com.google.devtools.build.lib.vfs.Path;
114115
import com.google.devtools.build.lib.vfs.PathFragment;
116+
import com.google.devtools.build.lib.worker.WorkerKey;
117+
import com.google.devtools.build.lib.worker.WorkerOptions;
118+
import com.google.devtools.build.lib.worker.WorkerParser;
119+
import com.google.devtools.common.options.Options;
115120
import com.google.protobuf.ByteString;
116121
import com.google.protobuf.ExtensionRegistry;
117122
import com.google.protobuf.Message;
@@ -365,7 +370,12 @@ private SortedMap<PathFragment, ActionInput> buildOutputDirMap(Spawn spawn) {
365370
return outputDirMap;
366371
}
367372

368-
private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext context)
373+
// ???! Add ToolSignature to build input
374+
private MerkleTree buildInputMerkleTree(
375+
Spawn spawn,
376+
SpawnExecutionContext context,
377+
ToolSignature toolSignature
378+
)
369379
throws IOException, ForbiddenActionInputException {
370380
// Add output directories to inputs so that they are created as empty directories by the
371381
// executor. The spec only requires the executor to create the parent directory of an output
@@ -394,7 +404,13 @@ private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext conte
394404
newInputMap.putAll(outputDirMap);
395405
inputMap = newInputMap;
396406
}
397-
return MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil);
407+
return MerkleTree.build(
408+
inputMap,
409+
toolSignature == null ? ImmutableSet.of() : toolSignature.toolInputs,
410+
context.getMetadataProvider(),
411+
execRoot,
412+
digestUtil
413+
);
398414
}
399415
}
400416

@@ -441,12 +457,46 @@ private static ByteString buildSalt(Spawn spawn) {
441457

442458
/** Creates a new {@link RemoteAction} instance from spawn. */
443459
public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context)
444-
throws IOException, UserExecException, ForbiddenActionInputException {
445-
final MerkleTree merkleTree = buildInputMerkleTree(spawn, context);
460+
throws IOException, ExecException, ForbiddenActionInputException, InterruptedException {
461+
// ???! remove the call to the ToolSignature-less buildInputMerkleTree
462+
// final MerkleTree merkleTree = buildInputMerkleTree(spawn, context);
463+
ToolSignature toolSignature =
464+
remoteOptions.markToolInputs
465+
&& Spawns.supportsWorkers(spawn)
466+
&& !spawn.getToolFiles().isEmpty()
467+
? computePersistentWorkerSignature(spawn, context)
468+
: null;
469+
final MerkleTree merkleTree = buildInputMerkleTree(spawn, context, toolSignature);
470+
446471

447472
// Get the remote platform properties.
448473
Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
449474

475+
if (toolSignature != null) {
476+
477+
StringBuilder cmdStr = new StringBuilder();
478+
for (String c : toolSignature.command) {
479+
cmdStr.append(c);
480+
cmdStr.append(" ");
481+
}
482+
cmdStr.deleteCharAt(cmdStr.length()-1);
483+
484+
Map<String, String> platformAdditionalProperties =
485+
ImmutableMap.of(
486+
"persistentWorkerKey", toolSignature.toolHash,
487+
"persistentWorkerCommand", cmdStr.toString()
488+
);
489+
490+
platform =
491+
PlatformUtils.getPlatformProto(
492+
spawn,
493+
remoteOptions,
494+
platformAdditionalProperties
495+
);
496+
} else {
497+
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
498+
}
499+
450500
Command command =
451501
buildCommand(
452502
spawn.getOutputFiles(),
@@ -484,6 +534,16 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
484534
action,
485535
actionKey);
486536
}
537+
538+
@Nullable
539+
private ToolSignature computePersistentWorkerSignature(Spawn spawn, SpawnExecutionContext context)
540+
throws IOException, ExecException, InterruptedException {
541+
WorkerParser workerParser =
542+
new WorkerParser(
543+
execRoot, Options.getDefaults(WorkerOptions.class), LocalEnvProvider.NOOP, null);
544+
WorkerKey workerKey = workerParser.compute(spawn, context).getWorkerKey();
545+
return new ToolSignature(workerKey);
546+
}
487547

488548
/** A value class representing the result of remotely executed {@link RemoteAction}. */
489549
public static class RemoteActionResult {
@@ -1468,4 +1528,34 @@ void report(Event evt) {
14681528
reporter.handle(evt);
14691529
}
14701530
}
1531+
1532+
1533+
// ???! "key" field name is kinda... meh so I'll change it.
1534+
/**
1535+
* A simple value class combining a hash of the tool inputs (and their digests) as well as a set
1536+
* of the relative paths of all tool inputs.
1537+
*
1538+
* ???! We also need to spell out the actual command.
1539+
* This effectively means that this class is a subset of WorkerKey
1540+
*/
1541+
private static final class ToolSignature {
1542+
1543+
private final ImmutableList<String> command;
1544+
private final ImmutableMap<String, String> env;
1545+
private final Set<PathFragment> toolInputs;
1546+
private final String toolHash;
1547+
1548+
private ToolSignature(WorkerKey workerKey) {
1549+
this.command = workerKey.getArgs();
1550+
this.env = workerKey.getEnv();
1551+
this.toolInputs = workerKey.getWorkerFilesWithHashes().keySet();
1552+
1553+
Fingerprint fingerprint = new Fingerprint();
1554+
fingerprint.addBytes(workerKey.getWorkerFilesCombinedHash().asBytes());
1555+
fingerprint.addIterableStrings(workerKey.getArgs());
1556+
fingerprint.addStringMap(workerKey.getEnv());
1557+
1558+
this.toolHash = fingerprint.hexDigestAndReset();
1559+
}
1560+
}
14711561
}

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.Objects;
2828
import java.util.SortedSet;
2929

30+
// ???! We will be adding changes to allow for toolInput (FileNode)
3031
/**
3132
* Intermediate tree representation of a list of lexicographically sorted list of files. Each node
3233
* in the tree represents either a directory or file.
@@ -73,6 +74,7 @@ static class FileNode extends Node {
7374
private final ByteString data;
7475
private final Digest digest;
7576
private final boolean isExecutable;
77+
private final boolean toolInput;
7678

7779
/**
7880
* Create a FileNode with its executable bit set.
@@ -83,32 +85,50 @@ static class FileNode extends Node {
8385
* https://github.com/bazelbuild/bazel/issues/13262 for more details.
8486
*/
8587
static FileNode createExecutable(String pathSegment, Path path, Digest digest) {
86-
return new FileNode(pathSegment, path, digest, /* isExecutable= */ true);
88+
return new FileNode(pathSegment, path, digest, /* isExecutable= */ true, false);
8789
}
8890

8991
static FileNode createExecutable(String pathSegment, ByteString data, Digest digest) {
90-
return new FileNode(pathSegment, data, digest, /* isExecutable= */ true);
92+
return new FileNode(pathSegment, data, digest, /* isExecutable= */ true, false);
9193
}
9294

93-
private FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) {
95+
// ???! Add overloads for passing toolInput
96+
// Not sure why the diff seemed to remove the default ByteString createExecutable
97+
static FileNode createExecutable(
98+
String pathSegment, Path path, Digest digest, boolean toolInput) {
99+
return new FileNode(pathSegment, path, digest, /* isExecutable= */ true, toolInput);
100+
}
101+
102+
static FileNode createExecutable(
103+
String pathSegment, ByteString path, Digest digest, boolean toolInput) {
104+
return new FileNode(pathSegment, path, digest, /* isExecutable= */ true, toolInput);
105+
}
106+
107+
private FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable, boolean toolInput) {
94108
super(pathSegment);
95109
this.path = Preconditions.checkNotNull(path, "path");
96110
this.data = null;
97111
this.digest = Preconditions.checkNotNull(digest, "digest");
98112
this.isExecutable = isExecutable;
113+
this.toolInput = toolInput;
99114
}
100115

101-
private FileNode(String pathSegment, ByteString data, Digest digest, boolean isExecutable) {
116+
private FileNode(String pathSegment, ByteString data, Digest digest, boolean isExecutable, boolean toolInput) {
102117
super(pathSegment);
103118
this.path = null;
104119
this.data = Preconditions.checkNotNull(data, "data");
105120
this.digest = Preconditions.checkNotNull(digest, "digest");
106121
this.isExecutable = isExecutable;
122+
this.toolInput = toolInput;
107123
}
108124

109125
Digest getDigest() {
110126
return digest;
111127
}
128+
129+
boolean isToolInput() {
130+
return toolInput;
131+
}
112132

113133
Path getPath() {
114134
return path;
@@ -124,7 +144,7 @@ public boolean isExecutable() {
124144

125145
@Override
126146
public int hashCode() {
127-
return Objects.hash(super.hashCode(), path, data, digest, isExecutable);
147+
return Objects.hash(super.hashCode(), path, data, digest, toolInput, isExecutable);
128148
}
129149

130150
@Override
@@ -135,6 +155,7 @@ public boolean equals(Object o) {
135155
&& Objects.equals(path, other.path)
136156
&& Objects.equals(data, other.data)
137157
&& Objects.equals(digest, other.digest)
158+
&& toolInput == other.toolInput
138159
&& isExecutable == other.isExecutable;
139160
}
140161
return false;

0 commit comments

Comments
 (0)