wip: Add test utils and tests for ProtoCoordinator

Co-authored-by: Shane Delmore <[email protected]>

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);
     }
 

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;

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<PosixFilePermission> perms = Files.getPosixFilePermissions(absPath);
+
+    ImmutableSet<PosixFilePermission> permsWithWrite =
+        ImmutableSet.<PosixFilePermission>builder()
+            .addAll(perms)
+            .add(PosixFilePermission.OWNER_WRITE)
+            .build();
+
+    return Files.setAttribute(absPath, "posix:permissions", permsWithWrite);
+  }
+
   private static final ConcurrentHashMap<Path, PathLock> 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) {

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<String> workerInitCmd,
       ImmutableList<String> workerInitArgs,
       ImmutableMap<String, String> 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<Path, HashCode> 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<String> workerInitCmd,
       ImmutableList<String> workerInitArgs,
       ImmutableMap<String, String> 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<Path, HashCode> workerFilesWithHashes(

src/main/java/build/buildfarm/worker/persistent/PersistentExecutor.java

@@ -29,18 +29,13 @@
 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 +44,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 +73,7 @@ public static Code runOnPersistentWorker(
       ImmutableMap<String, String> envVars,
       ResourceLimits limits,
       Duration timeout,
+      Path workRootsDir,
       ActionResult.Builder resultBuilder)
       throws IOException {
     //// Pull out persistent worker start command from the overall action request
@@ -112,7 +113,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);
 

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<Path, Input> getPathInputs() {
     synchronized (this) {
       if (pathInputs == null) {
-        pathInputs = inputsIndexer.getAllInputs(opRoot);
+        pathInputs = inputsIndexer.getAllInputs();
       }
     }
     return pathInputs;
@@ -53,7 +63,7 @@ public ImmutableMap<Path, Input> getPathInputs() {
   public ImmutableMap<Path, Input> getToolInputs() {
     synchronized (this) {
       if (toolInputs == null) {
-        toolInputs = inputsIndexer.getToolInputs(opRoot);
+        toolInputs = inputsIndexer.getToolInputs();
       }
     }
     return toolInputs;

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);

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,38 +25,51 @@ public class InputsIndexer {
   final Tree tree;
   final Map<Digest, Directory> proxyDirs;
 
+  final FileSystem fs;
+
+  final Path opRoot;
+
   ImmutableMap<Path, FileNode> files = null;
   ImmutableMap<Path, Input> absPathInputs = null;
   ImmutableMap<Path, Input> 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<Path, Input> 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<Path, Input> getAllInputs() {
     if (absPathInputs == null) {
       ImmutableMap<Path, FileNode> relFiles = getAllFiles();
       ImmutableMap.Builder<Path, Input> inputs = ImmutableMap.builder();
 
       for (Map.Entry<Path, FileNode> 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();
     }
     return absPathInputs;
   }
 
-  public ImmutableMap<Path, Input> getToolInputs(Path opRoot) {
+  public ImmutableMap<Path, Input> getToolInputs() {
     if (toolInputs == null) {
       ImmutableMap<Path, FileNode> relFiles = getAllFiles();
       ImmutableMap.Builder<Path, Input> inputs = ImmutableMap.builder();
 
       for (Map.Entry<Path, FileNode> 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<Path, FileNode> getAllFiles() {
     if (files == null) {
       ImmutableMap.Builder<Path, FileNode> 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;
   }

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",