From 1404651cafe5c26c5dae469e9126de53c2f4f024 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 7 Jul 2022 16:52:17 +0200 Subject: [PATCH] Create output directories for remote execution (#15818) By explicitly declaring output directories as inputs to a remote action, this commit ensures that these directories are created by the remote executor prior to the execution of the action. This brings the behavior of remote execution regarding tree artifacts in line with that of local or sandboxed execution. Getting the tests to pass requires modifying a check in Bazel's own remote worker implementation: Previously, the worker explicitly verified that output directories don't exist after the inputs have been staged. This behavior is not backed by the spec and has thus been modified: Now, it is only checked that the output directories either don't exist or are directories. Fixes #6393 Closes #15366. PiperOrigin-RevId: 447451303 Co-authored-by: Chenchu K --- .../lib/remote/RemoteExecutionService.java | 35 +++++++- .../build/lib/remote/merkletree/BUILD | 1 + .../merkletree/DirectoryTreeBuilder.java | 30 ++----- .../bazel/remote/remote_execution_test.sh | 79 +++++++++++++++++++ .../build/remote/worker/ExecutionServer.java | 8 +- 5 files changed, 126 insertions(+), 27 deletions(-) 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 62fecc7a92cee5..cc1727fe586be9 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 @@ -139,6 +139,7 @@ import java.util.Objects; import java.util.Set; import java.util.SortedMap; +import java.util.TreeMap; import java.util.TreeSet; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.Executor; @@ -305,8 +306,25 @@ public boolean mayBeExecutedRemotely(Spawn spawn) { && Spawns.mayBeExecutedRemotely(spawn); } + private SortedMap buildOutputDirMap(Spawn spawn) { + TreeMap outputDirMap = new TreeMap<>(); + for (ActionInput output : spawn.getOutputFiles()) { + if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { + outputDirMap.put( + PathFragment.create(remotePathResolver.getWorkingDirectory()) + .getRelative(remotePathResolver.localPathToOutputPath(output.getExecPath())), + output); + } + } + return outputDirMap; + } + private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext context) 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) { MetadataProvider metadataProvider = context.getMetadataProvider(); ConcurrentLinkedQueue subMerkleTrees = new ConcurrentLinkedQueue<>(); @@ -316,9 +334,20 @@ private MerkleTree buildInputMerkleTree(Spawn spawn, SpawnExecutionContext conte (Object nodeKey, InputWalker walker) -> { subMerkleTrees.add(buildMerkleTreeVisitor(nodeKey, walker, metadataProvider)); }); + if (!outputDirMap.isEmpty()) { + subMerkleTrees.add(MerkleTree.build(outputDirMap, metadataProvider, execRoot, digestUtil)); + } return MerkleTree.merge(subMerkleTrees, digestUtil); } else { SortedMap inputMap = remotePathResolver.getInputMapping(context); + if (!outputDirMap.isEmpty()) { + // The map returned by getInputMapping is mutable, but must not be mutated here as it is + // shared with all other strategies. + SortedMap newInputMap = new TreeMap<>(); + newInputMap.putAll(inputMap); + newInputMap.putAll(outputDirMap); + inputMap = newInputMap; + } return MerkleTree.build(inputMap, context.getMetadataProvider(), execRoot, digestUtil); } } @@ -937,9 +966,9 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // Check that all mandatory outputs are created. for (ActionInput output : action.getSpawn().getOutputFiles()) { if (action.getSpawn().isMandatoryOutput(output)) { - // Don't check output that is tree artifact since spawn could generate nothing under that - // directory. Remote server typically doesn't create directory ahead of time resulting in - // empty tree artifact missing from action cache entry. + // In the past, remote execution did not create output directories if the action didn't do + // this explicitly. This check only remains so that old remote cache entries that do not + // include empty output directories remain valid. if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) { continue; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD index b7831a8136a7f6..bca6b54363c084 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD @@ -21,6 +21,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/util:string", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:guava", 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 c641f80accfe0c..f188b234ef18d6 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 @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode; import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode; import com.google.devtools.build.lib.remote.util.DigestUtil; -import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -35,7 +34,6 @@ import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; -import javax.annotation.Nullable; /** Builder for directory trees. */ class DirectoryTreeBuilder { @@ -97,7 +95,6 @@ private static int buildFromPaths( Map tree) throws IOException { return build( - null, inputs, tree, (input, path, currDir) -> { @@ -126,7 +123,6 @@ private static int buildFromActionInputs( Map tree) throws IOException { return build( - metadataProvider, inputs, tree, (input, path, currDir) -> { @@ -182,7 +178,6 @@ private static int buildFromActionInputs( } private static int build( - @Nullable MetadataProvider metadataProvider, SortedMap inputs, Map tree, FileNodeVisitor fileNodeVisitor) @@ -200,24 +195,13 @@ private static int build( T input = e.getValue(); if (input instanceof DerivedArtifact && ((DerivedArtifact) input).isTreeArtifact()) { - DerivedArtifact artifact = (DerivedArtifact) input; - // MetadataProvider is provided by all callers for which T is a superclass of - // DerivedArtifact. - Preconditions.checkNotNull(metadataProvider); - FileArtifactValue metadata = - Preconditions.checkNotNull( - metadataProvider.getMetadata(artifact), - "missing metadata for '%s'", - artifact.getExecPathString()); - Preconditions.checkState( - metadata.equals(TreeArtifactValue.empty().getMetadata()), - "Encountered non-empty TreeArtifact '%s' with metadata '%s', which should have" - + " been expanded by SpawnInputExpander. This is a bug.", - path, - metadata); - // Create an empty directory and its parent directories but don't visit the TreeArtifact - // input itself: A TreeArtifact's metadata has type REGULAR_FILE, not DIRECTORY, and would - // thus lead to an empty file being created in the buildFromActionInputs visitor. + // SpawnInputExpander has already expanded non-empty tree artifacts into a collection of + // TreeFileArtifacts. Thus, at this point, tree artifacts represent empty directories, which + // we create together with their parents. + // Note: This also handles output directories of actions, which are explicitly included as + // inputs so that they are created by the executor before the action executes. Since such a + // directory must remain writeable, MetadataProvider#getMetadata must not be called on the + // tree artifact here as it would have the side effect of making it read only. DirectoryNode emptyDir = new DirectoryNode(path.getBaseName()); tree.put(path, emptyDir); createParentDirectoriesIfNotExist(path, emptyDir, tree); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 037a386469dda9..c215615c12255a 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3052,6 +3052,85 @@ function test_empty_tree_artifact_as_inputs_remote_cache() { expect_log "remote cache hit" } +function generate_tree_artifact_output() { + touch WORKSPACE + mkdir -p pkg + + cat > pkg/def.bzl <<'EOF' +def _r(ctx): + empty_dir = ctx.actions.declare_directory("%s/empty_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [empty_dir], + command = "cd %s && pwd" % empty_dir.path, + ) + non_empty_dir = ctx.actions.declare_directory("%s/non_empty_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [non_empty_dir], + command = "cd %s && pwd && touch out" % non_empty_dir.path, + ) + return [DefaultInfo(files = depset([empty_dir, non_empty_dir]))] + +r = rule(implementation = _r) +EOF + +cat > pkg/BUILD <<'EOF' +load(":def.bzl", "r") + +r(name = "a") +EOF +} + +function test_create_tree_artifact_outputs() { + # Test that if a tree artifact is declared as an input, then the corresponding + # empty directory is created before the action executes remotely. + generate_tree_artifact_output + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:a &>$TEST_log || fail "expected build to succeed" + [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file" + [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist" + + bazel clean --expunge + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_remote_merkle_tree_cache \ + //pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree cache" + [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file" + [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist" + + bazel clean --expunge + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_sibling_repository_layout \ + //pkg:a &>$TEST_log || fail "expected build to succeed with sibling repository layout" + [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file" + [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist" +} + +function test_create_tree_artifact_outputs_remote_cache() { + # Test that implicitly created empty directories corresponding to empty tree + # artifacts outputs are correctly cached in the remote cache. + generate_tree_artifact_output + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + //pkg:a &>$TEST_log || fail "expected build to succeed" + + bazel clean + + bazel build \ + --remote_cache=grpc://localhost:${worker_port} \ + //pkg:a &>$TEST_log || fail "expected build to succeed" + + expect_log "2 remote cache hit" + [[ -f bazel-bin/pkg/a/non_empty_dir/out ]] || fail "expected tree artifact to contain a file" + [[ -d bazel-bin/pkg/a/empty_dir ]] || fail "expected directory to exist" +} + # Runs coverage with `cc_test` and RE then checks the coverage file is returned. # Older versions of gcov are not supported with bazel coverage and so will be skipped. # See the above `test_java_rbe_coverage_produces_report` for more information. diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index c1e23f417af4b0..ffb7a61d750685 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -283,7 +283,13 @@ private ActionResult execute( for (String output : command.getOutputDirectoriesList()) { Path file = workingDirectory.getRelative(output); if (file.exists()) { - throw new FileAlreadyExistsException("Output directory/file already exists: " + file); + if (!file.isDirectory()) { + throw new FileAlreadyExistsException( + "Non-directory exists at output directory path: " + file); + } else if (!file.getDirectoryEntries().isEmpty()) { + throw new FileAlreadyExistsException( + "Non-empty directory exists at output directory path: " + file); + } } FileSystemUtils.createDirectoryAndParents(file.getParentDirectory()); outputs.add(file);