Make WorkerExecRoot not be a subclass of SandboxedSpawn.

It was only reusing a static method, yet inheriting a bunch of other things. This change should be a no-op. Prework for sandboxing multiplex workers.

RELNOTES: None.
PiperOrigin-RevId: 356456267

Author: larsrc-google

src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java

@@ -16,20 +16,17 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Iterables;
-import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
-import com.google.devtools.build.lib.vfs.FileSystemUtils.MoveResult;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.Nullable;
 
 /**
@@ -38,10 +35,6 @@
  */
 public abstract class AbstractContainerizingSandboxedSpawn implements SandboxedSpawn {
 
-  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
-
-  private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false);
-
   private final Path sandboxPath;
   private final Path sandboxExecRoot;
   private final List<String> arguments;
@@ -170,50 +163,9 @@ protected void createInputs(SandboxInputs inputs) throws IOException {
 
   protected abstract void copyFile(Path source, Path target) throws IOException;
 
-  /**
-   * Moves all given outputs from a root to another.
-   *
-   * <p>This is a support function to help with the implementation of {@link #copyOutputs(Path)}.
-   *
-   * @param outputs outputs to move as relative paths to a root
-   * @param sourceRoot source directory from which to resolve outputs
-   * @param targetRoot target directory to which to move the resolved outputs from the source
-   * @throws IOException if any of the moves fails
-   */
-  static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path targetRoot)
-      throws IOException {
-    for (PathFragment output : Iterables.concat(outputs.files(), outputs.dirs())) {
-      Path source = sourceRoot.getRelative(output);
-      Path target = targetRoot.getRelative(output);
-      if (source.isFile() || source.isSymbolicLink()) {
-        // Ensure the target directory exists in the target. The directories for the action outputs
-        // have already been created, but the spawn outputs may be different from the overall action
-        // outputs. This is the case for test actions.
-        target.getParentDirectory().createDirectoryAndParents();
-        if (FileSystemUtils.moveFile(source, target).equals(MoveResult.FILE_COPIED)) {
-          if (warnedAboutMovesBeingCopies.compareAndSet(false, true)) {
-            logger.atWarning().log(
-                "Moving files out of the sandbox (e.g. from %s to %s"
-                    + ") had to be done with a file copy, which is detrimental to performance; are "
-                    + " the two trees in different file systems?",
-                source, target);
-          }
-        }
-      } else if (source.isDirectory()) {
-        try {
-          source.renameTo(target);
-        } catch (IOException e) {
-          // Failed to move directory directly, thus move it recursively.
-          target.createDirectory();
-          FileSystemUtils.moveTreesBelow(source, target);
-        }
-      }
-    }
-  }
-
   @Override
   public void copyOutputs(Path execRoot) throws IOException {
-    moveOutputs(outputs, sandboxExecRoot, execRoot);
+    SandboxHelpers.moveOutputs(outputs, sandboxExecRoot, execRoot);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -16,13 +16,17 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.FileSystemUtils.MoveResult;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.OptionsParsingResult;
@@ -34,6 +38,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -42,7 +47,9 @@
  * <p>All sandboxed strategies within a build should share the same instance of this object.
  */
 public final class SandboxHelpers {
+  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
+  private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false);
   /**
    * If true, materialize virtual inputs only inside the sandbox, not the output tree. This flag
    * exists purely to support rolling this out as the defaut in a controlled manner.
@@ -101,6 +108,48 @@ public static void atomicallyWriteVirtualInput(
     }
   }
 
+  /**
+   * Moves all given outputs from a root to another.
+   *
+   * <p>This is a support function to help with the implementation of {@link
+   * SandboxfsSandboxedSpawn#copyOutputs(Path)}.
+   *
+   * @param outputs outputs to move as relative paths to a root
+   * @param sourceRoot source directory from which to resolve outputs
+   * @param targetRoot target directory to which to move the resolved outputs from the source
+   * @throws IOException if any of the moves fails
+   */
+  public static void moveOutputs(SandboxOutputs outputs, Path sourceRoot, Path targetRoot)
+      throws IOException {
+    for (PathFragment output : Iterables.concat(outputs.files(), outputs.dirs())) {
+      Path source = sourceRoot.getRelative(output);
+      Path target = targetRoot.getRelative(output);
+      if (source.isFile() || source.isSymbolicLink()) {
+        // Ensure the target directory exists in the target. The directories for the action outputs
+        // have already been created, but the spawn outputs may be different from the overall action
+        // outputs. This is the case for test actions.
+        target.getParentDirectory().createDirectoryAndParents();
+        if (FileSystemUtils.moveFile(source, target).equals(MoveResult.FILE_COPIED)) {
+          if (warnedAboutMovesBeingCopies.compareAndSet(false, true)) {
+            logger.atWarning().log(
+                "Moving files out of the sandbox (e.g. from %s to %s"
+                    + ") had to be done with a file copy, which is detrimental to performance; are "
+                    + "the two trees in different file systems?",
+                source, target);
+          }
+        }
+      } else if (source.isDirectory()) {
+        try {
+          source.renameTo(target);
+        } catch (IOException e) {
+          // Failed to move directory directly, thus move it recursively.
+          target.createDirectory();
+          FileSystemUtils.moveTreesBelow(source, target);
+        }
+      }
+    }
+  }
+
   /** Wrapper class for the inputs of a sandbox. */
   public static final class SandboxInputs {
 
@@ -178,7 +227,7 @@ private static void materializeVirtualInput(
      */
     public void materializeVirtualInputs(Path sandboxExecRoot) throws IOException {
       for (VirtualActionInput input : virtualInputs) {
-        materializeVirtualInput(input, sandboxExecRoot, /*needsDelete=*/ false);
+        materializeVirtualInput(input, sandboxExecRoot, /*isExecRootSandboxed=*/ false);
       }
     }
   }
@@ -257,7 +306,7 @@ public SandboxInputs processInputFiles(
       } else {
         if (actionInput instanceof VirtualActionInput) {
           SandboxInputs.materializeVirtualInput(
-              (VirtualActionInput) actionInput, execRoot, /*needsDelete=*/ true);
+              (VirtualActionInput) actionInput, execRoot, /* isExecRootSandboxed=*/ true);
         }
 
         if (actionInput.isSymlink()) {

src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java

@@ -201,7 +201,7 @@ public void copyOutputs(Path targetExecRoot) throws IOException {
     // TODO(jmmv): If we knew the targetExecRoot when setting up the spawn, we may be able to
     // configure sandboxfs so that the output files are written directly to their target locations.
     // This would avoid having to move them after-the-fact.
-    AbstractContainerizingSandboxedSpawn.moveOutputs(outputs, sandboxScratchDir, targetExecRoot);
+    SandboxHelpers.moveOutputs(outputs, sandboxScratchDir, targetExecRoot);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java

@@ -13,14 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.lib.worker;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.sandbox.SandboxHelpers;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
-import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn;
-import com.google.devtools.build.lib.sandbox.SynchronousTreeDeleter;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -32,31 +28,20 @@
 import java.util.Set;
 
 /** Creates and manages the contents of a working directory of a persistent worker. */
-final class WorkerExecRoot extends SymlinkedSandboxedSpawn {
+final class WorkerExecRoot {
   private final Path workDir;
   private final SandboxInputs inputs;
   private final SandboxOutputs outputs;
   private final Set<PathFragment> workerFiles;
 
   public WorkerExecRoot(
       Path workDir, SandboxInputs inputs, SandboxOutputs outputs, Set<PathFragment> workerFiles) {
-    super(
-        workDir,
-        workDir,
-        ImmutableList.of(),
-        ImmutableMap.of(),
-        inputs,
-        outputs,
-        ImmutableSet.of(),
-        new SynchronousTreeDeleter(),
-        /*statisticsPath=*/ null);
     this.workDir = workDir;
     this.inputs = inputs;
     this.outputs = outputs;
     this.workerFiles = workerFiles;
   }
 
-  @Override
   public void createFileSystem() throws IOException {
     workDir.createDirectoryAndParents();
 
@@ -173,4 +158,8 @@ private void createInputs(Iterable<PathFragment> inputsToCreate) throws IOExcept
       }
     }
   }
+
+  public void copyOutputs(Path execRoot) throws IOException {
+    SandboxHelpers.moveOutputs(outputs, workDir, execRoot);
+  }
 }

src/test/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawnTest.java

@@ -78,8 +78,7 @@ public void testMoveOutputs() throws Exception {
     Path outputsDir = testRoot.getRelative("outputs");
     outputsDir.createDirectory();
     outputsDir.getRelative("very").createDirectory();
-    AbstractContainerizingSandboxedSpawn.moveOutputs(
-        SandboxOutputs.create(outputs, outputDirs), execRoot, outputsDir);
+    SandboxHelpers.moveOutputs(SandboxOutputs.create(outputs, outputDirs), execRoot, outputsDir);
 
     assertThat(outputsDir.getRelative("very/output.txt").isFile(Symlinks.NOFOLLOW)).isTrue();
     assertThat(outputsDir.getRelative("very/output.link").isSymbolicLink()).isTrue();
@@ -130,7 +129,7 @@ public void renameTo(Path source, Path target) throws IOException {
     testRoot.createDirectoryAndParents();
 
     FileCopyWarningTracker tracker = new FileCopyWarningTracker();
-    Logger logger = Logger.getLogger(AbstractContainerizingSandboxedSpawn.class.getName());
+    Logger logger = Logger.getLogger(SandboxHelpers.class.getName());
     logger.setUseParentHandlers(false);
     logger.addHandler(tracker);
 
@@ -153,7 +152,7 @@ public void renameTo(Path source, Path target) throws IOException {
     outputsDir.createDirectory();
     outputsDir.getRelative("very").createDirectory();
     outputsDir.getRelative("much").createDirectory();
-    AbstractContainerizingSandboxedSpawn.moveOutputs(
+    SandboxHelpers.moveOutputs(
         SandboxOutputs.create(outputs, ImmutableSet.of()), execRoot, outputsDir);
 
     assertThat(outputsDir.getRelative("very/output1.txt").isFile(Symlinks.NOFOLLOW)).isTrue();