Skip to content

Commit

Permalink
Make WorkerExecRoot not be a subclass of SandboxedSpawn.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
larsrc-google authored and copybara-github committed Feb 9, 2021
1 parent b075c2c commit 4b68532
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 73 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand All @@ -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.
Expand Down Expand Up @@ -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 {

Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();

Expand Down Expand Up @@ -173,4 +158,8 @@ private void createInputs(Iterable<PathFragment> inputsToCreate) throws IOExcept
}
}
}

public void copyOutputs(Path execRoot) throws IOException {
SandboxHelpers.moveOutputs(outputs, workDir, execRoot);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);

Expand All @@ -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();
Expand Down

0 comments on commit 4b68532

Please sign in to comment.