diff --git a/src/main/java/build/buildfarm/worker/Executor.java b/src/main/java/build/buildfarm/worker/Executor.java index 78517cdae6..9d89fc4286 100644 --- a/src/main/java/build/buildfarm/worker/Executor.java +++ b/src/main/java/build/buildfarm/worker/Executor.java @@ -452,12 +452,7 @@ private Code executeCommand( Tree execTree = workerContext.getQueuedOperation(operationContext.queueEntry).getTree(); WorkFilesContext filesContext = - new WorkFilesContext( - execDir, - execTree, - ImmutableList.copyOf(operationContext.command.getOutputPathsList()), - ImmutableList.copyOf(operationContext.command.getOutputFilesList()), - ImmutableList.copyOf(operationContext.command.getOutputDirectoriesList())); + WorkFilesContext.fromContext(execDir, execTree, operationContext.command); return PersistentExecutor.runOnPersistentWorker( limits.persistentWorkerCommand, @@ -467,6 +462,7 @@ private Code executeCommand( ImmutableMap.copyOf(environment), limits, timeout, + PersistentExecutor.defaultWorkRootsDir, resultBuilder); } diff --git a/src/main/java/build/buildfarm/worker/OperationContext.java b/src/main/java/build/buildfarm/worker/OperationContext.java index 71b1975783..ef73e1bbf9 100644 --- a/src/main/java/build/buildfarm/worker/OperationContext.java +++ b/src/main/java/build/buildfarm/worker/OperationContext.java @@ -22,7 +22,7 @@ import com.google.longrunning.Operation; import java.nio.file.Path; -final class OperationContext { +public final class OperationContext { final ExecuteResponse.Builder executeResponse; final Operation operation; final Poller poller; diff --git a/src/main/java/build/buildfarm/worker/persistent/FileAccessUtils.java b/src/main/java/build/buildfarm/worker/persistent/FileAccessUtils.java index 36baaa84e3..8410c75dd1 100644 --- a/src/main/java/build/buildfarm/worker/persistent/FileAccessUtils.java +++ b/src/main/java/build/buildfarm/worker/persistent/FileAccessUtils.java @@ -3,23 +3,41 @@ import static java.nio.file.StandardCopyOption.COPY_ATTRIBUTES; import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; +import com.google.common.collect.ImmutableSet; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Supplier; import java.util.logging.Logger; -// Utility for concurrent move/copy/link of files +/** + * Utility for concurrent move/copy of files Can be extended in the future to (sym)linking if we + * need performance + */ public final class FileAccessUtils { // singleton class with only static methods private FileAccessUtils() {} private static final Logger logger = Logger.getLogger(FileAccessUtils.class.getName()); + public static Path addPosixOwnerWrite(Path absPath) throws IOException { + Set perms = Files.getPosixFilePermissions(absPath); + + ImmutableSet permsWithWrite = + ImmutableSet.builder() + .addAll(perms) + .add(PosixFilePermission.OWNER_WRITE) + .build(); + + return Files.setAttribute(absPath, "posix:permissions", permsWithWrite); + } + private static final ConcurrentHashMap fileLocks = new ConcurrentHashMap<>(); - // Used here for locking "files" + // Used here as a simple lock for locking "files" (paths) private static class PathLock { // Not used elsewhere private PathLock() {} @@ -46,13 +64,10 @@ public static void copyFile(Path from, Path to) throws IOException { () -> { try { Files.copy(from, absTo, REPLACE_EXISTING, COPY_ATTRIBUTES); - boolean writeable = absTo.toFile().setWritable(true); - if (!writeable) { - return new IOException("copyFile() could not set writeable: " + absTo); - } + addPosixOwnerWrite(absTo); return null; } catch (IOException e) { - return e; + return new IOException("copyFile() could not set writeable: " + absTo, e); } }); if (ioException != null) { @@ -81,44 +96,10 @@ public static void moveFile(Path from, Path to) throws IOException { () -> { try { Files.move(from, absTo, REPLACE_EXISTING); - boolean writeable = absTo.toFile().setWritable(true); - if (!writeable) { - return new IOException("moveFile() could not set writeable: " + absTo); - } - return null; - } catch (IOException e) { - return e; - } - }); - if (ioException != null) { - throw ioException; - } - } - - /** - * Creates a symlink, creating necessary directories. Deletes pre-existing files/links which have - * the same path as the specified link, effectively overwriting any existing files/links. - * - * @param from - * @param to - * @throws IOException - */ - public static void linkFile(Path from, Path to) throws IOException { - Path absTo = to.toAbsolutePath(); - logger.finer("linkFile: " + from + " to " + absTo); - if (!Files.exists(from)) { - throw new IOException("linkFile: source file doesn't exist: " + from); - } - IOException ioException = - writeFileSafe( - absTo, - () -> { - try { - Files.deleteIfExists(absTo); - Files.createSymbolicLink(absTo, from); + addPosixOwnerWrite(absTo); return null; } catch (IOException e) { - return e; + return new IOException("copyFile() could not set writeable: " + absTo, e); } }); if (ioException != null) { diff --git a/src/main/java/build/buildfarm/worker/persistent/Keymaker.java b/src/main/java/build/buildfarm/worker/persistent/Keymaker.java index 36cd5ffef3..fb6a861acb 100644 --- a/src/main/java/build/buildfarm/worker/persistent/Keymaker.java +++ b/src/main/java/build/buildfarm/worker/persistent/Keymaker.java @@ -13,10 +13,12 @@ import persistent.bazel.client.PersistentWorker; import persistent.bazel.client.WorkerKey; +/** Much of the logic (hashing) is from Bazel itself (private library/methods, i.e. WorkerKey). */ public class Keymaker { // Constructs a key with its worker tool input files being relative paths public static WorkerKey make( Path opRoot, + Path workRootsDir, ImmutableList workerInitCmd, ImmutableList workerInitArgs, ImmutableMap workerEnv, @@ -29,7 +31,13 @@ public static WorkerKey make( Path workRoot = calculateWorkRoot( - workerInitCmd, workerInitArgs, workerEnv, executionName, sandboxed, cancellable); + workRootsDir, + workerInitCmd, + workerInitArgs, + workerEnv, + executionName, + sandboxed, + cancellable); Path toolsRoot = workRoot.resolve(PersistentWorker.TOOL_INPUT_SUBDIR); SortedMap hashedTools = workerFilesWithHashes(workerFiles); @@ -49,6 +57,7 @@ public static WorkerKey make( // Hash of a subset of the WorkerKey private static Path calculateWorkRoot( + Path workRootsDir, ImmutableList workerInitCmd, ImmutableList workerInitArgs, ImmutableMap workerEnv, @@ -57,7 +66,7 @@ private static Path calculateWorkRoot( boolean cancellable) { int workRootId = Objects.hash(workerInitCmd, workerInitArgs, workerEnv, sandboxed, cancellable); String workRootDirName = "work-root_" + executionName + "_" + workRootId; - return PersistentExecutor.workRootsDir.resolve(workRootDirName); + return workRootsDir.resolve(workRootDirName); } private static ImmutableSortedMap workerFilesWithHashes( diff --git a/src/main/java/build/buildfarm/worker/persistent/PersistentExecutor.java b/src/main/java/build/buildfarm/worker/persistent/PersistentExecutor.java index 2f8cb9b61b..5486ff7de2 100644 --- a/src/main/java/build/buildfarm/worker/persistent/PersistentExecutor.java +++ b/src/main/java/build/buildfarm/worker/persistent/PersistentExecutor.java @@ -24,23 +24,19 @@ * Executes an Action like Executor/DockerExecutor, writing to ActionResult. * *

Currently has special code for discriminating between Javac/Scalac, and other persistent - * workers. + * workers, likely for debugging purposes, but need to revisit. (Can't remember fully since it was + * so long ago!) */ public class PersistentExecutor { private static final Logger logger = Logger.getLogger(PersistentExecutor.class.getName()); - // How many workers can exist at once for a given WorkerKey - // There may be multiple WorkerKeys per mnemonic, - // e.g. if builds are run with different tool fingerprints - private static final int defaultMaxWorkersPerKey = 6; - private static final ProtoCoordinator coordinator = ProtoCoordinator.ofCommonsPool(getMaxWorkersPerKey()); // TODO load from config (i.e. {worker_root}/persistent) - static final Path workRootsDir = Paths.get("/tmp/worker/persistent/"); + public static final Path defaultWorkRootsDir = Paths.get("/tmp/worker/persistent/"); - static final String PERSISTENT_WORKER_FLAG = "--persistent_worker"; + public static final String PERSISTENT_WORKER_FLAG = "--persistent_worker"; // TODO Revisit hardcoded actions static final String JAVABUILDER_JAR = @@ -49,6 +45,11 @@ public class PersistentExecutor { private static final String SCALAC_EXEC_NAME = "Scalac"; private static final String JAVAC_EXEC_NAME = "JavaBuilder"; + // How many workers can exist at once for a given WorkerKey + // There may be multiple WorkerKeys per mnemonic, + // e.g. if builds are run with different tool fingerprints + private static final int defaultMaxWorkersPerKey = 6; + private static int getMaxWorkersPerKey() { try { return Integer.parseInt(System.getenv("BUILDFARM_MAX_WORKERS_PER_KEY")); @@ -73,6 +74,7 @@ public static Code runOnPersistentWorker( ImmutableMap envVars, ResourceLimits limits, Duration timeout, + Path workRootsDir, ActionResult.Builder resultBuilder) throws IOException { //// Pull out persistent worker start command from the overall action request @@ -87,6 +89,7 @@ public static Code runOnPersistentWorker( return Code.INVALID_ARGUMENT; } + // TODO revisit why this was necessary in the first place ImmutableMap env; if (executionName.equals(JAVAC_EXEC_NAME)) { env = ImmutableMap.of(); @@ -112,7 +115,13 @@ public static Code runOnPersistentWorker( WorkerKey key = Keymaker.make( - context.opRoot, workerExecCmd, workerInitArgs, env, executionName, workerFiles); + context.opRoot, + workRootsDir, + workerExecCmd, + workerInitArgs, + env, + executionName, + workerFiles); coordinator.copyToolInputsIntoWorkerToolRoot(key, workerFiles); diff --git a/src/main/java/build/buildfarm/worker/persistent/ProtoCoordinator.java b/src/main/java/build/buildfarm/worker/persistent/ProtoCoordinator.java index 3138b56529..69b02f05fd 100644 --- a/src/main/java/build/buildfarm/worker/persistent/ProtoCoordinator.java +++ b/src/main/java/build/buildfarm/worker/persistent/ProtoCoordinator.java @@ -206,7 +206,7 @@ private void copyNontoolInputs(WorkerInputs workerInputs, Path workerExecRoot) // Make outputs visible to the rest of Worker machinery // see DockerExecutor::copyOutputsOutOfContainer - private void moveOutputsToOperationRoot(WorkFilesContext context, Path workerExecRoot) + void moveOutputsToOperationRoot(WorkFilesContext context, Path workerExecRoot) throws IOException { Path opRoot = context.opRoot; @@ -216,9 +216,11 @@ private void moveOutputsToOperationRoot(WorkFilesContext context, Path workerExe } for (String relOutput : context.outputFiles) { - Path relPath = Paths.get(relOutput); - Path opOutputPath = opRoot.resolve(relPath); - Path execOutputPath = workerExecRoot.resolve(relPath); + System.out.println(relOutput); + Path execOutputPath = workerExecRoot.resolve(relOutput); + System.out.println(execOutputPath); + Path opOutputPath = opRoot.resolve(relOutput); + System.out.println(opOutputPath); FileAccessUtils.moveFile(execOutputPath, opOutputPath); } diff --git a/src/main/java/build/buildfarm/worker/persistent/WorkFilesContext.java b/src/main/java/build/buildfarm/worker/persistent/WorkFilesContext.java index 6a4de9d046..67197172a9 100644 --- a/src/main/java/build/buildfarm/worker/persistent/WorkFilesContext.java +++ b/src/main/java/build/buildfarm/worker/persistent/WorkFilesContext.java @@ -1,5 +1,6 @@ package build.buildfarm.worker.persistent; +import build.bazel.remote.execution.v2.Command; import build.buildfarm.v1test.Tree; import build.buildfarm.worker.util.InputsIndexer; import com.google.common.collect.ImmutableList; @@ -37,14 +38,23 @@ public WorkFilesContext( this.outputFiles = outputFiles; this.outputDirectories = outputDirectories; - this.inputsIndexer = new InputsIndexer(execTree); + this.inputsIndexer = new InputsIndexer(execTree, this.opRoot); + } + + public static WorkFilesContext fromContext(Path opRoot, Tree inputsTree, Command opCommand) { + return new WorkFilesContext( + opRoot, + inputsTree, + ImmutableList.copyOf(opCommand.getOutputPathsList()), + ImmutableList.copyOf(opCommand.getOutputFilesList()), + ImmutableList.copyOf(opCommand.getOutputDirectoriesList())); } // Paths are absolute paths from the opRoot; same as the Input.getPath(); public ImmutableMap getPathInputs() { synchronized (this) { if (pathInputs == null) { - pathInputs = inputsIndexer.getAllInputs(opRoot); + pathInputs = inputsIndexer.getAllInputs(); } } return pathInputs; @@ -53,7 +63,7 @@ public ImmutableMap getPathInputs() { public ImmutableMap getToolInputs() { synchronized (this) { if (toolInputs == null) { - toolInputs = inputsIndexer.getToolInputs(opRoot); + toolInputs = inputsIndexer.getToolInputs(); } } return toolInputs; diff --git a/src/main/java/build/buildfarm/worker/persistent/WorkerInputs.java b/src/main/java/build/buildfarm/worker/persistent/WorkerInputs.java index 82c8aad4c6..4c71a4aa8a 100644 --- a/src/main/java/build/buildfarm/worker/persistent/WorkerInputs.java +++ b/src/main/java/build/buildfarm/worker/persistent/WorkerInputs.java @@ -57,16 +57,6 @@ public void copyInputFile(Path from, Path to) throws IOException { FileAccessUtils.copyFile(from, to); } - public void moveInputFile(Path from, Path to) throws IOException { - checkFileIsInput("moveInputFile()", from); - FileAccessUtils.moveFile(from, to); - } - - public void linkInputFile(Path from, Path to) throws IOException { - checkFileIsInput("linkInputFile()", from); - FileAccessUtils.linkFile(from, to); - } - public void deleteInputFileIfExists(Path workerExecRoot, Path opPathInput) throws IOException { checkFileIsInput("deleteInputFile()", opPathInput); Path execPathInput = relativizeInput(workerExecRoot, opPathInput); @@ -109,6 +99,8 @@ public static WorkerInputs from(WorkFilesContext workFilesContext, List logger.fine(inputsDebugMsg); + System.out.println(inputsDebugMsg); + return new WorkerInputs(workFilesContext.opRoot, absToolInputs, toolInputs, pathInputs); } } diff --git a/src/main/java/build/buildfarm/worker/util/InputsIndexer.java b/src/main/java/build/buildfarm/worker/util/InputsIndexer.java index 21d84d34b6..39637db54a 100644 --- a/src/main/java/build/buildfarm/worker/util/InputsIndexer.java +++ b/src/main/java/build/buildfarm/worker/util/InputsIndexer.java @@ -8,8 +8,8 @@ import build.buildfarm.v1test.Tree; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.worker.WorkerProtocol.Input; +import java.nio.file.FileSystem; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.Map; /** @@ -25,22 +25,35 @@ public class InputsIndexer { final Tree tree; final Map proxyDirs; + final FileSystem fs; + + final Path opRoot; + ImmutableMap files = null; ImmutableMap absPathInputs = null; ImmutableMap toolInputs = null; - public InputsIndexer(Tree tree) { + public InputsIndexer(Tree tree, Path opRoot) { this.tree = tree; this.proxyDirs = new ProxyDirectoriesIndex(tree.getDirectoriesMap()); + this.opRoot = opRoot; + this.fs = opRoot.getFileSystem(); } - public ImmutableMap getAllInputs(Path opRoot) { + // https://stackoverflow.com/questions/22611919/why-do-i-get-providermismatchexception-when-i-try-to-relativize-a-path-agains + public Path pathTransform(final Path path) { + Path ret = fs.getPath(path.isAbsolute() ? fs.getSeparator() : ""); + for (final Path component : path) ret = ret.resolve(component.getFileName().toString()); + return ret; + } + + public ImmutableMap getAllInputs() { if (absPathInputs == null) { ImmutableMap relFiles = getAllFiles(); ImmutableMap.Builder inputs = ImmutableMap.builder(); for (Map.Entry pf : relFiles.entrySet()) { - Path absPath = opRoot.resolve(pf.getKey()); + Path absPath = this.opRoot.resolve(pf.getKey()).normalize(); inputs.put(absPath, inputFromFile(absPath, pf.getValue())); } absPathInputs = inputs.build(); @@ -48,7 +61,7 @@ public ImmutableMap getAllInputs(Path opRoot) { return absPathInputs; } - public ImmutableMap getToolInputs(Path opRoot) { + public ImmutableMap getToolInputs() { if (toolInputs == null) { ImmutableMap relFiles = getAllFiles(); ImmutableMap.Builder inputs = ImmutableMap.builder(); @@ -56,7 +69,7 @@ public ImmutableMap getToolInputs(Path opRoot) { for (Map.Entry pf : relFiles.entrySet()) { FileNode fn = pf.getValue(); if (isToolInput(fn)) { - Path absPath = opRoot.resolve(pf.getKey()); + Path absPath = this.opRoot.resolve(pf.getKey()); inputs.put(absPath, inputFromFile(absPath, fn)); } } @@ -69,7 +82,9 @@ private ImmutableMap getAllFiles() { if (files == null) { ImmutableMap.Builder accumulator = ImmutableMap.builder(); Directory rootDir = proxyDirs.get(tree.getRootDigest()); - files = getFilesFromDir(Paths.get("."), rootDir, accumulator).build(); + + Path fsRelative = fs.getPath("."); + files = getFilesFromDir(fsRelative, rootDir, accumulator).build(); } return files; } diff --git a/src/test/java/build/buildfarm/instance/shard/JedisCasWorkerMapTest.java b/src/test/java/build/buildfarm/instance/shard/JedisCasWorkerMapTest.java index caa69536c1..0b6f3d2020 100644 --- a/src/test/java/build/buildfarm/instance/shard/JedisCasWorkerMapTest.java +++ b/src/test/java/build/buildfarm/instance/shard/JedisCasWorkerMapTest.java @@ -20,7 +20,6 @@ @RunWith(JUnit4.class) public class JedisCasWorkerMapTest { - private static final String CAS_PREFIX = "ContentAddressableStorage"; private RedisServer redisServer; diff --git a/src/test/java/build/buildfarm/worker/persistent/BUILD b/src/test/java/build/buildfarm/worker/persistent/BUILD index 7d753777c2..e2e2e5a771 100644 --- a/src/test/java/build/buildfarm/worker/persistent/BUILD +++ b/src/test/java/build/buildfarm/worker/persistent/BUILD @@ -4,13 +4,18 @@ java_test( srcs = glob(["*.java"]), test_class = "build.buildfarm.AllTests", deps = [ + "//persistentworkers/src/main/java/persistent/bazel:bazel-persistent-workers", + "//persistentworkers/src/main/java/persistent/common:persistent-common", + "//persistentworkers/src/main/java/persistent/common/util", "//src/main/java/build/buildfarm/common", "//src/main/java/build/buildfarm/common/config", "//src/main/java/build/buildfarm/instance", "//src/main/java/build/buildfarm/worker", + "//src/main/java/build/buildfarm/worker/persistent", "//src/main/java/build/buildfarm/worker/resources", "//src/main/protobuf:build_buildfarm_v1test_buildfarm_java_proto", "//src/test/java/build/buildfarm:test_runner", + "//src/test/java/build/buildfarm/worker/util:worker_test_utils", "@bazel_tools//src/main/protobuf:worker_protocol_java_proto", "@googleapis//:google_rpc_code_java_proto", "@maven//:com_github_jnr_jnr_constants", diff --git a/src/test/java/build/buildfarm/worker/persistent/PersistentExecutorTest.java b/src/test/java/build/buildfarm/worker/persistent/PersistentExecutorTest.java index 2d116cb902..dace2fd9c2 100644 --- a/src/test/java/build/buildfarm/worker/persistent/PersistentExecutorTest.java +++ b/src/test/java/build/buildfarm/worker/persistent/PersistentExecutorTest.java @@ -7,7 +7,7 @@ @RunWith(JUnit4.class) public class PersistentExecutorTest { @Test - public void testProtoCoordinatorCreatesDirs() throws Exception { + public void testProtoCoordinatorCreatesDirs() { assert (true); } } diff --git a/src/test/java/build/buildfarm/worker/persistent/ProtoCoordinatorTest.java b/src/test/java/build/buildfarm/worker/persistent/ProtoCoordinatorTest.java new file mode 100644 index 0000000000..998d510cdf --- /dev/null +++ b/src/test/java/build/buildfarm/worker/persistent/ProtoCoordinatorTest.java @@ -0,0 +1,118 @@ +package build.buildfarm.worker.persistent; + +import build.bazel.remote.execution.v2.Command; +import build.buildfarm.v1test.Tree; +import build.buildfarm.worker.util.WorkerTestUtils; +import build.buildfarm.worker.util.WorkerTestUtils.TreeFile; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import com.google.common.jimfs.Configuration; +import com.google.common.jimfs.Jimfs; +import com.google.devtools.build.lib.worker.WorkerProtocol.Input; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import persistent.bazel.client.PersistentWorker; +import persistent.bazel.client.WorkerKey; + +@RunWith(JUnit4.class) +public class ProtoCoordinatorTest { + private WorkerKey makeWorkerKey( + WorkFilesContext ctx, WorkerInputs workerFiles, Path workRootsDir) { + return Keymaker.make( + ctx.opRoot, + workRootsDir, + ImmutableList.of("workerExecCmd"), + ImmutableList.of("workerInitArgs"), + ImmutableMap.of(), + "executionName", + workerFiles); + } + + private Path rootDir = null; + + public Path jimFsRoot() { + if (rootDir == null) { + rootDir = + Iterables.getFirst( + Jimfs.newFileSystem( + Configuration.unix() + .toBuilder() + .setAttributeViews("basic", "owner", "posix", "unix") + .build()) + .getRootDirectories(), + null); + } + return rootDir; + } + + @Test + public void testProtoCoordinator() throws Exception { + ProtoCoordinator pc = ProtoCoordinator.ofCommonsPool(4); + + Path fsRoot = jimFsRoot(); + Path opRoot = fsRoot.resolve("opRoot"); + assert (Files.notExists(opRoot)); + Files.createDirectory(opRoot); + + assert (Files.exists(opRoot)); + + String treeRootDir = opRoot.toString(); + List fileInputs = + ImmutableList.of( + new TreeFile("file_1", "file contents 1"), + new TreeFile("subdir/subdir_file_2", "file contents 2"), + new TreeFile("tools_dir/tool_file", "tool file contents", true), + new TreeFile("tools_dir/tool_file_2", "tool file contents 2", true)); + + Tree tree = WorkerTestUtils.makeTree(treeRootDir, fileInputs); + + Command command = WorkerTestUtils.makeCommand(); + WorkFilesContext ctx = WorkFilesContext.fromContext(opRoot, tree, command); + ImmutableList requestArgs = ImmutableList.of("reqArg1"); + + WorkerInputs workerFiles = WorkerInputs.from(ctx, requestArgs); + + for (Map.Entry entry : workerFiles.allInputs.entrySet()) { + Path file = entry.getKey(); + Files.createDirectories(file.getParent()); + Files.createFile(file); + } + + WorkerKey key = makeWorkerKey(ctx, workerFiles, fsRoot.resolve("workRootsDir")); + + Path workRoot = key.getExecRoot(); + Path toolsRoot = workRoot.resolve(PersistentWorker.TOOL_INPUT_SUBDIR); + + pc.copyToolInputsIntoWorkerToolRoot(key, workerFiles); + + assert Files.exists(workRoot); + List expectedToolInputs = new ArrayList<>(); + for (TreeFile file : fileInputs) { + if (file.isTool) { + expectedToolInputs.add(toolsRoot.resolve(file.path)); + } + } + WorkerTestUtils.assertFilesExistExactly(workRoot, expectedToolInputs); + + List expectedOpRootFiles = new ArrayList<>(); + + // Check that we move specified output files (assuming they exist) + for (String pathStr : ctx.outputFiles) { + Path file = workRoot.resolve(pathStr); + Files.createDirectories(file.getParent()); + Files.createFile(file); + expectedOpRootFiles.add(opRoot.resolve(pathStr)); + } + + pc.moveOutputsToOperationRoot(ctx, workRoot); + + WorkerTestUtils.assertFilesExistExactly(opRoot, expectedOpRootFiles); + } +} diff --git a/src/test/java/build/buildfarm/worker/util/BUILD b/src/test/java/build/buildfarm/worker/util/BUILD index 5e671f73ee..23397c285b 100644 --- a/src/test/java/build/buildfarm/worker/util/BUILD +++ b/src/test/java/build/buildfarm/worker/util/BUILD @@ -1,9 +1,40 @@ +java_library( + name = "worker_test_utils", + srcs = ["WorkerTestUtils.java"], + visibility = ["//src/test/java:__subpackages__"], + deps = [ + "//src/main/java/build/buildfarm/cas", + "//src/main/java/build/buildfarm/common", + "//src/main/java/build/buildfarm/common/config", + "//src/main/java/build/buildfarm/worker/util", + "//src/main/protobuf:build_buildfarm_v1test_buildfarm_java_proto", + "//src/test/java/build/buildfarm:test_runner", + "@bazel_tools//src/main/protobuf:worker_protocol_java_proto", + "@googleapis//:google_rpc_code_java_proto", + "@maven//:com_github_jnr_jnr_constants", + "@maven//:com_github_jnr_jnr_ffi", + "@maven//:com_github_serceman_jnr_fuse", + "@maven//:com_google_guava_guava", + "@maven//:com_google_jimfs_jimfs", + "@maven//:com_google_protobuf_protobuf_java", + "@maven//:com_google_truth_truth", + "@maven//:io_grpc_grpc_api", + "@maven//:io_grpc_grpc_context", + "@maven//:io_grpc_grpc_core", + "@maven//:io_grpc_grpc_protobuf", + "@maven//:org_mockito_mockito_core", + "@maven//:org_projectlombok_lombok", + "@remote_apis//:build_bazel_remote_execution_v2_remote_execution_java_proto", + ], +) + java_test( name = "tests", size = "small", - srcs = glob(["*.java"]), + srcs = glob(["*Test.java"]), test_class = "build.buildfarm.AllTests", deps = [ + ":worker_test_utils", "//src/main/java/build/buildfarm/cas", "//src/main/java/build/buildfarm/common", "//src/main/java/build/buildfarm/common/config", diff --git a/src/test/java/build/buildfarm/worker/util/InputsIndexerTest.java b/src/test/java/build/buildfarm/worker/util/InputsIndexerTest.java index f90051052d..d81048bced 100644 --- a/src/test/java/build/buildfarm/worker/util/InputsIndexerTest.java +++ b/src/test/java/build/buildfarm/worker/util/InputsIndexerTest.java @@ -3,7 +3,12 @@ import static build.buildfarm.worker.util.InputsIndexer.BAZEL_TOOL_INPUT_MARKER; import static com.google.common.truth.Truth.assertThat; -import build.bazel.remote.execution.v2.*; +import build.bazel.remote.execution.v2.Digest; +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.buildfarm.common.DigestUtil; import build.buildfarm.v1test.Tree; import com.google.common.collect.ImmutableMap; @@ -16,6 +21,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +// TODO: use WorkerTestUtils.makeTree @RunWith(JUnit4.class) public class InputsIndexerTest { private final DigestUtil DIGEST_UTIL = new DigestUtil(DigestUtil.HashFunction.SHA256); @@ -23,7 +29,7 @@ public class InputsIndexerTest { @Test public void basicEmptyTree() { Tree emptyTree = Tree.newBuilder().build(); - InputsIndexer indexer = new InputsIndexer(emptyTree); + InputsIndexer indexer = new InputsIndexer(emptyTree, Paths.get(".")); assertThat(indexer.tree).isEqualTo(emptyTree); } @@ -35,11 +41,11 @@ public void canGetRootDir() { Digest rootDirDigest = addDirToTree(treeBuilder, "my_root_dir", rootDir); treeBuilder.setRootDigest(rootDirDigest); - InputsIndexer indexer = new InputsIndexer(treeBuilder.build()); - assertThat(indexer.proxyDirs.get(rootDirDigest)).isEqualTo(rootDir); - Path arbitraryOpRoot = Paths.get("."); - assertThat(indexer.getAllInputs(arbitraryOpRoot).size()).isEqualTo(0); + + InputsIndexer indexer = new InputsIndexer(treeBuilder.build(), arbitraryOpRoot); + assertThat(indexer.proxyDirs.get(rootDirDigest)).isEqualTo(rootDir); + assertThat(indexer.getAllInputs().size()).isEqualTo(0); } @Test @@ -52,16 +58,16 @@ public void rootDirWithFiles() { Digest rootDirDigest = addDirToTree(treeBuilder, "my_root_dir", rootDir); treeBuilder.setRootDigest(rootDirDigest); - InputsIndexer indexer = new InputsIndexer(treeBuilder.build()); + Path arbitraryOpRoot = Paths.get("asdf"); + InputsIndexer indexer = new InputsIndexer(treeBuilder.build(), arbitraryOpRoot); assertThat(indexer.proxyDirs.get(rootDirDigest)).isEqualTo(rootDir); - Path arbitraryOpRoot = Paths.get("asdf"); Input myfileInput = makeInput(arbitraryOpRoot, myfile); ImmutableMap expectedInputs = ImmutableMap.of(Paths.get(myfileInput.getPath()), myfileInput); - assertThat(indexer.getAllInputs(arbitraryOpRoot)).isEqualTo(expectedInputs); + assertThat(indexer.getAllInputs()).isEqualTo(expectedInputs); } @Test @@ -92,11 +98,12 @@ public void canRecurseAndDistinguishToolInputs() { Digest rootDirDigest = addDirToTree(treeBuilder, "my_root_dir", rootDir); treeBuilder.setRootDigest(rootDirDigest); - InputsIndexer indexer = new InputsIndexer(treeBuilder.build()); + Path arbitraryOpRoot = Paths.get("asdf"); + + InputsIndexer indexer = new InputsIndexer(treeBuilder.build(), arbitraryOpRoot); assertThat(indexer.proxyDirs.get(rootDirDigest)).isEqualTo(rootDir); assertThat(indexer.proxyDirs.size()).isEqualTo(2); - Path arbitraryOpRoot = Paths.get("asdf"); Input myfileInput = makeInput(arbitraryOpRoot, myfile); Input subdirfileInput = makeInput(arbitraryOpRoot.resolve(subDirName), subdirfile); Input toolfileInput = makeInput(arbitraryOpRoot, toolfile); @@ -112,9 +119,9 @@ public void canRecurseAndDistinguishToolInputs() { ImmutableMap allInputs = ImmutableMap.builder().putAll(nonToolInputs).putAll(toolInputs).build(); - assertThat(indexer.getAllInputs(arbitraryOpRoot)).isEqualTo(allInputs); - assertThat(indexer.getAllInputs(arbitraryOpRoot).size()).isEqualTo(3); - assertThat(indexer.getToolInputs(arbitraryOpRoot)).isEqualTo(toolInputs); + assertThat(indexer.getAllInputs()).isEqualTo(allInputs); + assertThat(indexer.getAllInputs().size()).isEqualTo(3); + assertThat(indexer.getToolInputs()).isEqualTo(toolInputs); } Digest addDirToTree(Tree.Builder treeBuilder, String dirname, Directory dir) { diff --git a/src/test/java/build/buildfarm/worker/util/WorkerTestUtils.java b/src/test/java/build/buildfarm/worker/util/WorkerTestUtils.java new file mode 100644 index 0000000000..1a6a687e76 --- /dev/null +++ b/src/test/java/build/buildfarm/worker/util/WorkerTestUtils.java @@ -0,0 +1,212 @@ +package build.buildfarm.worker.util; + +import static build.buildfarm.worker.util.InputsIndexer.BAZEL_TOOL_INPUT_MARKER; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; + +import build.bazel.remote.execution.v2.Command; +import build.bazel.remote.execution.v2.Digest; +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.buildfarm.common.DigestUtil; +import build.buildfarm.v1test.Tree; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.worker.WorkerProtocol.Input; +import com.google.protobuf.ByteString; +import java.io.IOException; +import java.nio.file.FileVisitResult; +import java.nio.file.FileVisitor; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +public class WorkerTestUtils { + public static final DigestUtil DIGEST_UTIL = new DigestUtil(DigestUtil.HashFunction.SHA256); + + public static FileNode makeFileNode( + String filename, String content, NodeProperties nodeProperties) { + return FileNode.newBuilder() + .setName(filename) + .setDigest(DIGEST_UTIL.compute(ByteString.copyFromUtf8(content))) + .setIsExecutable(false) + .setNodeProperties(nodeProperties) + .build(); + } + + public static DirectoryNode makeDirNode(String dirname, Digest dirDigest) { + // Pretty sure we don't need the actual hash for our testing purposes + return DirectoryNode.newBuilder().setName(dirname).setDigest(dirDigest).build(); + } + + public static Digest addDirToTree(Tree.Builder treeBuilder, String dirname, Directory dir) { + ByteString dirnameBytes = ByteString.copyFromUtf8(dirname); + Digest digest = DIGEST_UTIL.compute(dirnameBytes); + String hash = digest.getHash(); + treeBuilder.putDirectories(hash, dir); + return digest; + } + + public static NodeProperties makeNodeProperties(ImmutableMap props) { + return NodeProperties.newBuilder() + .addAllProperties( + props.entrySet().stream() + .map( + kv -> + NodeProperty.newBuilder() + .setName(kv.getKey()) + .setValue(kv.getValue()) + .build()) + .collect(Collectors.toList())) + .build(); + } + + public static Input makeInput(Path fileDir, FileNode file) { + Path fileNodePath = fileDir.resolve(file.getName()); + return Input.newBuilder() + .setPath(fileNodePath.toString()) + .setDigest(file.getDigest().getHashBytes()) + .build(); + } + + public static Command makeCommand() { + ImmutableList outputFiles = ImmutableList.of("output_file", "out_subdir/out_subfile"); + ImmutableList outputDirs = ImmutableList.of("out_subdir"); + ImmutableList outputPaths = + ImmutableList.builder().addAll(outputFiles).addAll(outputDirs).build(); + + return Command.newBuilder() + .addAllOutputFiles(outputFiles) + .addAllOutputDirectories(outputDirs) + .addAllOutputPaths(outputPaths) + .build(); + } + + public static class TreeFile { + public final String path; + public final boolean isTool; + + // null means directory + public final String content; + + public TreeFile(String path) { + this(path, "", false); + } + + public TreeFile(String path, String content) { + this(path, content, false); + } + + public TreeFile(String path, String content, boolean isTool) { + this.path = path; + this.isTool = isTool; + this.content = content; + } + + public boolean isDir() { + return this.content == null; + } + + public String name() { + return Paths.get(this.path).getFileName().toString(); + } + } + + public static Tree makeTree(String rootDirPath, List files) { + Tree.Builder treeBuilder = Tree.newBuilder(); + if (files.isEmpty()) { + return treeBuilder.build(); + } + Directory.Builder rootDirBuilder = Directory.newBuilder(); + + Map dirBuilders = new HashMap<>(); + + for (TreeFile file : files) { + if (file.isDir()) { + dirBuilders.computeIfAbsent(file.path, (filePath) -> Directory.newBuilder()); + } else { + NodeProperties props = NodeProperties.getDefaultInstance(); + if (file.isTool) { + props = makeNodeProperties(ImmutableMap.of(BAZEL_TOOL_INPUT_MARKER, "")); + } + FileNode fileNode = makeFileNode(file.name(), file.content, props); + Path parentDirPath = Paths.get(file.path).getParent(); + if (parentDirPath != null) { + String parentDirPathStr = parentDirPath.normalize().toString(); + Directory.Builder parentDirBuilder = + dirBuilders.computeIfAbsent(parentDirPathStr, (filePath) -> Directory.newBuilder()); + parentDirBuilder.addFiles(fileNode); + } else { + rootDirBuilder.addFiles(fileNode); + } + } + } + + for (Map.Entry entry : dirBuilders.entrySet()) { + String subDirName = entry.getKey(); + Directory subDir = entry.getValue().build(); + Digest subDirDigest = addDirToTree(treeBuilder, subDirName, subDir); + rootDirBuilder.addDirectories(makeDirNode(subDirName, subDirDigest)); + } + + Digest rootDirDigest = addDirToTree(treeBuilder, rootDirPath, rootDirBuilder.build()); + treeBuilder.setRootDigest(rootDirDigest); + + return treeBuilder.build(); + } + + public static List listFilesRec(Path root) throws IOException { + List filesFound = new ArrayList<>(); + + Files.walkFileTree( + root, + new FileVisitor() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) + throws IOException { + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) + throws IOException { + filesFound.add(file); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + throw new IOException("visitFileFailed"); + } + + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + filesFound.add(dir); + return FileVisitResult.CONTINUE; + } + }); + + return filesFound; + } + + // Check all expected files exist and that only they exist + public static void assertFilesExistExactly(Path root, List expectedFiles) + throws IOException { + List listedPaths = listFilesRec(root); + for (Path filePath : listedPaths) { + assertWithMessage("Path not match prefix of any expected file: " + filePath) + .that(expectedFiles.stream().anyMatch(p -> p.startsWith(p))) + .isTrue(); + } + assertThat(listedPaths).containsAtLeastElementsIn(expectedFiles); + } +}