From c55a27b87ab1abb167ef87975e96c803ffcf46a8 Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Fri, 30 Sep 2022 08:56:38 -0700 Subject: [PATCH] Experimentally support remote persistent workers. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new `--experimental_remote_mark_tool_inputs` flag, which makes Bazel tag tool inputs when executing actions remotely, and also adds a tools input key to the platform proto sent as part of the remote execution request. This allows a remote execution system to implement persistent workers, i.e., to keep worker processes around and reuse them for subsequent actions. In a trivial example, this improves build performance by ~3x. We use "persistentWorkerKey" for the platform property, with the value being a hash of the tool inputs, and "bazel_tool_input" as the node property name, with an empty string as value—this is just a boolean tag. Fixes https://github.com/bazelbuild/bazel/issues/10091. Co-authored-by: Ulf Adams --- .../lib/analysis/platform/PlatformUtils.java | 49 +++++++---- .../lib/exec/local/LocalEnvProvider.java | 1 + .../google/devtools/build/lib/remote/BUILD | 5 ++ .../lib/remote/RemoteExecutionService.java | 65 ++++++++++++-- .../lib/remote/merkletree/DirectoryTree.java | 32 +++++-- .../merkletree/DirectoryTreeBuilder.java | 27 +++++- .../lib/remote/merkletree/MerkleTree.java | 42 +++++++-- .../lib/remote/options/RemoteOptions.java | 10 +++ .../build/lib/worker/WorkerParser.java | 2 +- .../build/lib/exec/util/SpawnBuilder.java | 8 ++ .../remote/RemoteExecutionServiceTest.java | 86 +++++++++++++++++++ .../ActionInputDirectoryTreeTest.java | 2 +- 12 files changed, 291 insertions(+), 38 deletions(-) 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 2773421656eee8..3fef4f8616a970 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 @@ -17,6 +17,7 @@ import build.bazel.remote.execution.v2.Platform; import build.bazel.remote.execution.v2.Platform.Property; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Ordering; import com.google.devtools.build.lib.actions.Spawn; @@ -64,6 +65,13 @@ public static Platform buildPlatformProto(Map executionPropertie @Nullable public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions remoteOptions) throws UserExecException { + return getPlatformProto(spawn, remoteOptions, ImmutableMap.of()); + } + + @Nullable + public static Platform getPlatformProto( + Spawn spawn, @Nullable RemoteOptions remoteOptions, Map additionalProperties) + throws UserExecException { SortedMap defaultExecProperties = remoteOptions != null ? remoteOptions.getRemoteDefaultExecProperties() @@ -71,34 +79,34 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem if (spawn.getExecutionPlatform() == null && spawn.getCombinedExecProperties().isEmpty() - && defaultExecProperties.isEmpty()) { + && defaultExecProperties.isEmpty() + && additionalProperties.isEmpty()) { return null; } - Platform.Builder platformBuilder = Platform.newBuilder(); - + Map properties; if (!spawn.getCombinedExecProperties().isEmpty()) { - Map combinedExecProperties; // Apply default exec properties if the execution platform does not already set // exec_properties if (spawn.getExecutionPlatform() == null || spawn.getExecutionPlatform().execProperties().isEmpty()) { - combinedExecProperties = new HashMap<>(); - combinedExecProperties.putAll(defaultExecProperties); - combinedExecProperties.putAll(spawn.getCombinedExecProperties()); + properties = new HashMap<>(); + properties.putAll(defaultExecProperties); + properties.putAll(spawn.getCombinedExecProperties()); } else { - combinedExecProperties = spawn.getCombinedExecProperties(); - } - - for (Map.Entry entry : combinedExecProperties.entrySet()) { - platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue()); + properties = spawn.getCombinedExecProperties(); } } else if (spawn.getExecutionPlatform() != null && !Strings.isNullOrEmpty(spawn.getExecutionPlatform().remoteExecutionProperties())) { + properties = new HashMap<>(); // Try and get the platform info from the execution properties. try { + Platform.Builder platformBuilder = Platform.newBuilder(); TextFormat.getParser() .merge(spawn.getExecutionPlatform().remoteExecutionProperties(), platformBuilder); + for (Property property : platformBuilder.getPropertiesList()) { + properties.put(property.getName(), property.getValue()); + } } catch (ParseException e) { String message = String.format( @@ -108,12 +116,23 @@ public static Platform getPlatformProto(Spawn spawn, @Nullable RemoteOptions rem e, createFailureDetail(message, Code.INVALID_REMOTE_EXECUTION_PROPERTIES)); } } else { - for (Map.Entry property : defaultExecProperties.entrySet()) { - platformBuilder.addProperties( - Property.newBuilder().setName(property.getKey()).setValue(property.getValue()).build()); + properties = defaultExecProperties; + } + + if (!additionalProperties.isEmpty()) { + if (properties.isEmpty()) { + properties = additionalProperties; + } else { + // Merge the two maps. + properties = new HashMap<>(properties); + properties.putAll(additionalProperties); } } + Platform.Builder platformBuilder = Platform.newBuilder(); + for (Map.Entry entry : properties.entrySet()) { + platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue()); + } sortPlatformProperties(platformBuilder); return platformBuilder.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java index c93008ca175830..614e652ec5faca 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java @@ -24,6 +24,7 @@ * probably should not exist, but is currently necessary for our local MacOS support. */ public interface LocalEnvProvider { + LocalEnvProvider NOOP = (env, binTools, fallbackTmpDir) -> ImmutableMap.copyOf(env); /** * Creates a local environment provider for the current OS. 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 d783b419a8c414..3ab8a1f4752c18 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -77,6 +77,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:spawn_input_expander", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry", + "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", @@ -93,6 +94,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers", "//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:exit_code", @@ -102,6 +104,9 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:output_service", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/lib/worker:worker_key", + "//src/main/java/com/google/devtools/build/lib/worker:worker_options", + "//src/main/java/com/google/devtools/build/lib/worker:worker_spawn_runner", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/common/options", "//src/main/protobuf:failure_details_java_proto", 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 ef33003709cd5c..97de87664a484f 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 @@ -77,13 +77,13 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.Spawns; -import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.analysis.platform.PlatformUtils; import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.SpawnInputExpander.InputWalker; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; +import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; @@ -110,12 +110,17 @@ import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; +import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.lib.worker.WorkerKey; +import com.google.devtools.build.lib.worker.WorkerOptions; +import com.google.devtools.build.lib.worker.WorkerParser; +import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; import com.google.protobuf.ExtensionRegistry; import com.google.protobuf.Message; @@ -382,13 +387,14 @@ private SortedMap buildOutputDirMap(Spawn spawn) { return outputDirMap; } - private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext context) + private MerkleTree buildInputMerkleTree( + Spawn spawn, SpawnExecutionContext context, ToolSignature toolSignature) throws IOException, ForbiddenActionInputException { // Add output directories to inputs so that they are created as empty directories by the // executor. The spec only requires the executor to create the parent directory of an output // directory, which differs from the behavior of both local and sandboxed execution. SortedMap outputDirMap = buildOutputDirMap(spawn); - if (remoteOptions.remoteMerkleTreeCache) { + if (remoteOptions.remoteMerkleTreeCache && toolSignature == null) { MetadataProvider metadataProvider = context.getMetadataProvider(); ConcurrentLinkedQueue subMerkleTrees = new ConcurrentLinkedQueue<>(); remotePathResolver.walkInputs( @@ -411,7 +417,12 @@ private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext conte newInputMap.putAll(outputDirMap); inputMap = newInputMap; } - return MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil); + return MerkleTree.build( + inputMap, + toolSignature == null ? ImmutableSet.of() : toolSignature.toolInputs, + context.getMetadataProvider(), + execRoot, + digestUtil); } } @@ -458,11 +469,24 @@ private static ByteString buildSalt(Spawn spawn) { /** Creates a new {@link RemoteAction} instance from spawn. */ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context) - throws IOException, UserExecException, ForbiddenActionInputException { - final MerkleTree merkleTree = buildInputMerkleTree(spawn, context); + throws IOException, ExecException, ForbiddenActionInputException, InterruptedException { + ToolSignature toolSignature = + remoteOptions.markToolInputs + && Spawns.supportsWorkers(spawn) + && !spawn.getToolFiles().isEmpty() + ? computePersistentWorkerSignature(spawn, context) + : null; + final MerkleTree merkleTree = buildInputMerkleTree(spawn, context, toolSignature); // Get the remote platform properties. Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); + if (toolSignature != null) { + platform = + PlatformUtils.getPlatformProto( + spawn, remoteOptions, ImmutableMap.of("persistentWorkerKey", toolSignature.key)); + } else { + platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); + } Command command = buildCommand( @@ -502,6 +526,21 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context actionKey); } + @Nullable + private ToolSignature computePersistentWorkerSignature(Spawn spawn, SpawnExecutionContext context) + throws IOException, ExecException, InterruptedException { + WorkerParser workerParser = + new WorkerParser( + execRoot, Options.getDefaults(WorkerOptions.class), LocalEnvProvider.NOOP, null); + WorkerKey workerKey = workerParser.compute(spawn, context).getWorkerKey(); + Fingerprint fingerprint = new Fingerprint(); + fingerprint.addBytes(workerKey.getWorkerFilesCombinedHash().asBytes()); + fingerprint.addIterableStrings(workerKey.getArgs()); + fingerprint.addStringMap(workerKey.getEnv()); + return new ToolSignature( + fingerprint.hexDigestAndReset(), workerKey.getWorkerFilesWithDigests().keySet()); + } + /** A value class representing the result of remotely executed {@link RemoteAction}. */ public static class RemoteActionResult { private final ActionResult actionResult; @@ -1498,4 +1537,18 @@ void report(Event evt) { reporter.handle(evt); } } + + /** + * A simple value class combining a hash of the tool inputs (and their digests) as well as a set + * of the relative paths of all tool inputs. + */ + private static final class ToolSignature { + private final String key; + private final Set toolInputs; + + private ToolSignature(String key, Set toolInputs) { + this.key = key; + this.toolInputs = toolInputs; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java index c8fe48fd2b5086..2a6e13b1bca2bc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTree.java @@ -77,6 +77,7 @@ static class FileNode extends Node { private final ByteString data; private final Digest digest; private final boolean isExecutable; + private final boolean toolInput; /** * Create a FileNode with its executable bit set. @@ -87,27 +88,41 @@ static class FileNode extends Node { * https://github.com/bazelbuild/bazel/issues/13262 for more details. */ static FileNode createExecutable(String pathSegment, Path path, Digest digest) { - return new FileNode(pathSegment, path, digest, /* isExecutable= */ true); + return new FileNode(pathSegment, path, digest, /* isExecutable= */ true, false); } - static FileNode createExecutable(String pathSegment, ByteString data, Digest digest) { - return new FileNode(pathSegment, data, digest, /* isExecutable= */ true); + static FileNode createExecutable( + String pathSegment, Path path, Digest digest, boolean toolInput) { + return new FileNode(pathSegment, path, digest, /* isExecutable= */ true, toolInput); } - private FileNode(String pathSegment, Path path, Digest digest, boolean isExecutable) { + static FileNode createExecutable( + String pathSegment, ByteString data, Digest digest, boolean toolInput) { + return new FileNode(pathSegment, data, digest, /* isExecutable= */ true, toolInput); + } + + private FileNode( + String pathSegment, Path path, Digest digest, boolean isExecutable, boolean toolInput) { super(pathSegment); this.path = Preconditions.checkNotNull(path, "path"); this.data = null; this.digest = Preconditions.checkNotNull(digest, "digest"); this.isExecutable = isExecutable; + this.toolInput = toolInput; } - private FileNode(String pathSegment, ByteString data, Digest digest, boolean isExecutable) { + private FileNode( + String pathSegment, + ByteString data, + Digest digest, + boolean isExecutable, + boolean toolInput) { super(pathSegment); this.path = null; this.data = Preconditions.checkNotNull(data, "data"); this.digest = Preconditions.checkNotNull(digest, "digest"); this.isExecutable = isExecutable; + this.toolInput = toolInput; } Digest getDigest() { @@ -126,9 +141,13 @@ public boolean isExecutable() { return isExecutable; } + boolean isToolInput() { + return toolInput; + } + @Override public int hashCode() { - return Objects.hash(super.hashCode(), path, data, digest, isExecutable); + return Objects.hash(super.hashCode(), path, data, digest, toolInput, isExecutable); } @Override @@ -139,6 +158,7 @@ public boolean equals(Object o) { && Objects.equals(path, other.path) && Objects.equals(data, other.data) && Objects.equals(digest, other.digest) + && toolInput == other.toolInput && isExecutable == other.isExecutable; } return false; diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index e38a60935b2ec5..3a731981345ede 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -15,6 +15,7 @@ import build.bazel.remote.execution.v2.Digest; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; @@ -34,6 +35,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; @@ -61,8 +63,19 @@ static DirectoryTree fromActionInputs( Path execRoot, DigestUtil digestUtil) throws IOException { + return fromActionInputs(inputs, ImmutableSet.of(), metadataProvider, execRoot, digestUtil); + } + + static DirectoryTree fromActionInputs( + SortedMap inputs, + Set toolInputs, + MetadataProvider metadataProvider, + Path execRoot, + DigestUtil digestUtil) + throws IOException { Map tree = new HashMap<>(); - int numFiles = buildFromActionInputs(inputs, metadataProvider, execRoot, digestUtil, tree); + int numFiles = + buildFromActionInputs(inputs, toolInputs, metadataProvider, execRoot, digestUtil, tree); return new DirectoryTree(tree, numFiles); } @@ -119,6 +132,7 @@ private static int buildFromPaths( */ private static int buildFromActionInputs( SortedMap inputs, + Set toolInputs, MetadataProvider metadataProvider, Path execRoot, DigestUtil digestUtil, @@ -134,7 +148,10 @@ private static int buildFromActionInputs( boolean childAdded = currDir.addChild( FileNode.createExecutable( - path.getBaseName(), virtualActionInput.getBytes(), d)); + path.getBaseName(), + virtualActionInput.getBytes(), + d, + toolInputs.contains(path))); return childAdded ? 1 : 0; } @@ -149,7 +166,9 @@ private static int buildFromActionInputs( Digest d = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); Path inputPath = ActionInputHelper.toInputPath(input, execRoot); boolean childAdded = - currDir.addChild(FileNode.createExecutable(path.getBaseName(), inputPath, d)); + currDir.addChild( + FileNode.createExecutable( + path.getBaseName(), inputPath, d, toolInputs.contains(path))); return childAdded ? 1 : 0; } @@ -157,7 +176,7 @@ private static int buildFromActionInputs( SortedMap directoryInputs = explodeDirectory(input.getExecPath(), execRoot); return buildFromActionInputs( - directoryInputs, metadataProvider, execRoot, digestUtil, tree); + directoryInputs, toolInputs, metadataProvider, execRoot, digestUtil, tree); case SYMLINK: { diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java index 6f3cd0a0c49047..5af8d982ba3a00 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java @@ -42,6 +42,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.SortedMap; import java.util.SortedSet; import java.util.TreeMap; @@ -50,6 +51,7 @@ /** A merkle tree representation as defined by the remote execution api. */ public class MerkleTree { + private static final String BAZEL_TOOL_INPUT_MARKER = "bazel_tool_input"; /** A path or contents */ public static class PathOrBytes { @@ -229,6 +231,32 @@ public static MerkleTree build( } } + /** + * Constructs a merkle tree from a lexicographically sorted map of inputs (files). + * + * @param inputs a map of path to input. The map is required to be sorted lexicographically by + * paths. Inputs of type tree artifacts are not supported and are expected to have been + * expanded before. + * @param metadataProvider provides metadata for all {@link ActionInput}s in {@code inputs}, as + * well as any {@link ActionInput}s being discovered via directory expansion. + * @param execRoot all paths in {@code inputs} need to be relative to this {@code execRoot}. + * @param digestUtil a hashing utility + */ + public static MerkleTree build( + SortedMap inputs, + Set toolInputs, + MetadataProvider metadataProvider, + Path execRoot, + DigestUtil digestUtil) + throws IOException { + try (SilentCloseable c = Profiler.instance().profile("MerkleTree.build(ActionInput)")) { + DirectoryTree tree = + DirectoryTreeBuilder.fromActionInputs( + inputs, toolInputs, metadataProvider, execRoot, digestUtil); + return build(tree, digestUtil); + } + } + /** * Constructs a merkle tree from a lexicographically sorted map of files. * @@ -354,11 +382,15 @@ private static MerkleTree buildMerkleTree( } private static FileNode buildProto(DirectoryTree.FileNode file) { - return FileNode.newBuilder() - .setName(decodeBytestringUtf8(file.getPathSegment())) - .setDigest(file.getDigest()) - .setIsExecutable(file.isExecutable()) - .build(); + var node = + FileNode.newBuilder() + .setName(decodeBytestringUtf8(file.getPathSegment())) + .setDigest(file.getDigest()) + .setIsExecutable(file.isExecutable()); + if (file.isToolInput()) { + node.getNodePropertiesBuilder().addPropertiesBuilder().setName(BAZEL_TOOL_INPUT_MARKER); + } + return node.build(); } private static DirectoryNode buildProto(String baseName, MerkleTree dir) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index ebda3d8080ded5..f7cbbeba30f575 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -631,6 +631,16 @@ public RemoteOutputsStrategyConverter() { + " just the first.") public boolean remoteDownloaderSendAllHeaders; + @Option( + name = "experimental_remote_mark_tool_inputs", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If set to true, Bazel will mark inputs as tool inputs for the remote executor. This " + + "can be used to implement remote persistent workers.") + public boolean markToolInputs; + // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java index ff09a7b5dfe64f..e5dbe815d7f199 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java @@ -41,7 +41,7 @@ * persistent worker process (actions with equal keys are allowed to use the same worker process), * and a separate list of flag files. The result is encapsulated as a {@link WorkerConfig}. */ -class WorkerParser { +public class WorkerParser { private static final String ERROR_MESSAGE_PREFIX = "Worker strategy cannot execute this %s action, "; private static final String REASON_NO_FLAGFILE = diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java index ebb56d4a7fe850..f8138908e96b7b 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java @@ -161,6 +161,14 @@ public SpawnBuilder withInput(ActionInput input) { return this; } + @CanIgnoreReturnValue + public SpawnBuilder withInputs(ActionInput... inputs) { + for (var input : inputs) { + this.inputs.add(input); + } + return this; + } + @CanIgnoreReturnValue public SpawnBuilder withInput(String name) { this.inputs.add(ActionInputHelper.fromPath(name)); 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 4fb988a67f3e58..d9efeb1a376834 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 @@ -14,6 +14,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.actions.ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS; import static com.google.devtools.build.lib.remote.util.DigestUtil.toBinaryDigest; @@ -39,6 +40,8 @@ import build.bazel.remote.execution.v2.Directory; import build.bazel.remote.execution.v2.DirectoryNode; import build.bazel.remote.execution.v2.FileNode; +import build.bazel.remote.execution.v2.NodeProperties; +import build.bazel.remote.execution.v2.NodeProperty; import build.bazel.remote.execution.v2.OutputDirectory; import build.bazel.remote.execution.v2.OutputFile; import build.bazel.remote.execution.v2.OutputSymlink; @@ -1738,6 +1741,89 @@ public void buildMerkleTree_withMemoization_works() throws Exception { verify(service, times(5 + 4)).uncachedBuildMerkleTreeVisitor(any(), any()); } + @Test + public void buildRemoteActionForRemotePersistentWorkers() throws Exception { + var input = ActionsTestUtil.createArtifact(artifactRoot, "input"); + fakeFileCache.createScratchInput(input, "value"); + var toolInput = ActionsTestUtil.createArtifact(artifactRoot, "worker_input"); + fakeFileCache.createScratchInput(toolInput, "worker value"); + Spawn spawn = + new SpawnBuilder("@flagfile") + .withExecutionInfo(ExecutionRequirements.SUPPORTS_WORKERS, "1") + .withInputs(input, toolInput) + .withTool(toolInput) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.markToolInputs = true; + RemoteExecutionService service = newRemoteExecutionService(remoteOptions); + + // Check that worker files are properly marked in the merkle tree. + var remoteAction = service.buildRemoteAction(spawn, context); + assertThat(remoteAction.getAction().getPlatform()) + .isEqualTo( + Platform.newBuilder() + .addProperties( + Platform.Property.newBuilder() + .setName("persistentWorkerKey") + .setValue( + "b22d48cd55755474eae27e63a79306a64146bd5947d5bd3423d78f001cf7b3de")) + .build()); + var merkleTree = remoteAction.getMerkleTree(); + var outputDirectory = + merkleTree.getDirectoryByDigest(merkleTree.getRootProto().getDirectories(0).getDigest()); + assertThat(outputDirectory) + .isEqualTo( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setName("input") + .setDigest( + Digest.newBuilder() + .setHash( + "cd42404d52ad55ccfa9aca4adc828aa5800ad9d385a0671fbcbf724118320619") + .setSizeBytes(5)) + .setIsExecutable(true)) + .addFiles( + FileNode.newBuilder() + .setName("worker_input") + .setDigest( + Digest.newBuilder() + .setHash( + "bbd21d9e9b2bbadb2bb67202833df0edc8d14baf38be49388ffc71831eb88ac4") + .setSizeBytes(12)) + .setIsExecutable(true) + .setNodeProperties( + NodeProperties.newBuilder() + .addProperties( + NodeProperty.newBuilder().setName("bazel_tool_input")))) + .build()); + + // Check that if an non-tool input changes, the persistent worker key does not change. + fakeFileCache.createScratchInput(input, "value2"); + assertThat(service.buildRemoteAction(spawn, context).getAction().getPlatform()) + .isEqualTo( + Platform.newBuilder() + .addProperties( + Platform.Property.newBuilder() + .setName("persistentWorkerKey") + .setValue( + "b22d48cd55755474eae27e63a79306a64146bd5947d5bd3423d78f001cf7b3de")) + .build()); + + // Check that if a tool input changes, the persistent worker key changes. + fakeFileCache.createScratchInput(toolInput, "worker value2"); + assertThat(service.buildRemoteAction(spawn, context).getAction().getPlatform()) + .isEqualTo( + Platform.newBuilder() + .addProperties( + Platform.Property.newBuilder() + .setName("persistentWorkerKey") + .setValue( + "997337de8dc20123cd7c8fcaed2c9c79cd8138831f9fbbf119f37d0859c9e83a")) + .build()); + } + private Spawn newSpawnFromResult(RemoteActionResult result) { return newSpawnFromResult(ImmutableMap.of(), result); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java index 393d610d715574..6af2ab698c80ff 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/merkletree/ActionInputDirectoryTreeTest.java @@ -72,7 +72,7 @@ public void virtualActionInputShouldWork() throws Exception { FileNode expectedFooNode = FileNode.createExecutable("foo.cc", foo.getPath(), digestUtil.computeAsUtf8("foo")); FileNode expectedBarNode = - FileNode.createExecutable("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar")); + FileNode.createExecutable("bar.cc", bar.getBytes(), digestUtil.computeAsUtf8("bar"), false); assertThat(fileNodesAtDepth(tree, 0)).isEmpty(); assertThat(fileNodesAtDepth(tree, 1)).containsExactly(expectedFooNode, expectedBarNode); }