Simplify the sandboxfs map/unmap interface to work in terms of "sandboxes".
Bazel creates a top-level directory within sandboxfs for each action and
makes sure that these hierarchies remain disjoint. We'll take advantage
of this in sandboxfs to optimize the way mappings are applied. But, for
now, make sure to propagate this information down to the sandboxfs module
so that we can later push it into sandboxfs itself.
To do this, replace the previous map/unmap primitives, which worked at
the path level, with higher-level createSandbox/destroySandbox abstractions,
which work at the level of a whole top-level subdirectory of the mount
point.
It's very likely that the RPC system of sandboxfs will adopt this same
interface, although that's still under development.
RELNOTES: None.
PiperOrigin-RevId: 276753593
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java b/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
index bb55df9..c685210 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.sandbox;
+import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -99,12 +100,12 @@
* Mounts a new sandboxfs instance.
*
* <p>The root of the file system instance is left unmapped which means that it remains as
- * read-only throughout the lifetime of this instance. Writable subdirectories can later be
- * mapped via {@link #map(List)}.
+ * read-only throughout the lifetime of this instance. Writable subdirectories can later be mapped
+ * via {@link #createSandbox}.
*
- * @param binary path to the sandboxfs binary. This is a {@link PathFragment} and not a
- * {@link Path} because we want to support "bare" (non-absolute) names for the location of
- * the sandboxfs binary; such names are automatically looked for in the {@code PATH}.
+ * @param binary path to the sandboxfs binary. This is a {@link PathFragment} and not a {@link
+ * Path} because we want to support "bare" (non-absolute) names for the location of the
+ * sandboxfs binary; such names are automatically looked for in the {@code PATH}.
* @param mountPoint directory on which to mount the sandboxfs instance
* @param logFile path to the file that will receive all sandboxfs logging output
* @return a new handle that represents the running process
@@ -224,11 +225,17 @@
}
@Override
- public void map(List<Mapping> mappings) throws IOException {
+ public void createSandbox(String name, List<Mapping> mappings) throws IOException {
+ checkArgument(!PathFragment.containsSeparator(name));
+ PathFragment root = PathFragment.create("/").getRelative(name);
+
Function<Mapping, String> formatMapping =
- (mapping) -> String.format(
- "{\"Map\": {\"Mapping\": \"%s\", \"Target\": \"%s\", \"Writable\": %s}}",
- mapping.path(), mapping.target(), mapping.writable() ? "true" : "false");
+ (mapping) ->
+ String.format(
+ "{\"Map\": {\"Mapping\": \"%s\", \"Target\": \"%s\", \"Writable\": %s}}",
+ root.getRelative(mapping.path().toRelative()),
+ mapping.target(),
+ mapping.writable() ? "true" : "false");
StringBuilder sb = new StringBuilder();
sb.append("[\n");
@@ -238,7 +245,10 @@
}
@Override
- public void unmap(PathFragment mapping) throws IOException {
- reconfigure(String.format("[{\"Unmap\": \"%s\"}]\n\n", mapping));
+ public void destroySandbox(String name) throws IOException {
+ checkArgument(!PathFragment.containsSeparator(name));
+ PathFragment root = PathFragment.create("/").getRelative(name);
+
+ reconfigure(String.format("[{\"Unmap\": \"%s\"}]\n\n", root.getPathString()));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsProcess.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsProcess.java
index 4b56ed3..3286709 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsProcess.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsProcess.java
@@ -107,22 +107,22 @@
void destroy();
/**
- * Adds new mappings to the sandboxfs instance.
+ * Creates a top-level directory with the given name and adds all given mappings inside it.
*
+ * @param name basename of the top-level directory to create
* @param mappings the collection of mappings to add, which must not have yet been previously
- * mapped
+ * mapped and which will be contained within the given top-level directory
* @throws IOException if sandboxfs cannot be reconfigured either because of an error in the
* configuration or because we failed to communicate with the subprocess
*/
- void map(List<Mapping> mappings) throws IOException;
+ void createSandbox(String name, List<Mapping> mappings) throws IOException;
/**
- * Removes a mapping from the sandboxfs instance.
+ * Destroys a top-level directory and all of its contents.
*
- * @param mapping the mapping to remove, which must have been previously mapped. This looks like
- * an absolute path but is treated as relative to the sandbox's root.
+ * @param name basename of the top-level directory to destroy
* @throws IOException if sandboxfs cannot be reconfigured either because of an error in the
* configuration or because we failed to communicate with the subprocess
*/
- void unmap(PathFragment mapping) throws IOException;
+ void destroySandbox(String name) throws IOException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
index 7117683..af31555 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java
@@ -45,6 +45,9 @@
/** Sequence number to assign a unique subtree to each action within the mount point. */
private static final AtomicInteger lastId = new AtomicInteger();
+ /** Single instance of a path fragment representing a root directory. */
+ private static final PathFragment rootFragment = PathFragment.create("/");
+
/** The sandboxfs instance to use for this spawn. */
private final SandboxfsProcess process;
@@ -85,10 +88,10 @@
private final Path execRoot;
/**
- * Path to the working directory of the command, seen as an absolute path that starts at
- * the sandboxfs's mount point.
+ * Name of the sandbox within the sandboxfs mount point, which is just the basename of the
+ * top-level directory where all execroot paths start.
*/
- private final PathFragment innerExecRoot;
+ private final String sandboxName;
@Nullable private final Path statisticsPath;
@@ -139,8 +142,8 @@
this.sandboxScratchDir = sandboxPath.getRelative("scratch");
int id = lastId.getAndIncrement();
- this.execRoot = process.getMountPoint().getRelative("" + id);
- this.innerExecRoot = PathFragment.create("/" + id);
+ this.sandboxName = "" + id;
+ this.execRoot = process.getMountPoint().getRelative(this.sandboxName);
this.statisticsPath = statisticsPath;
}
@@ -168,8 +171,7 @@
public void createFileSystem() throws IOException {
sandboxScratchDir.createDirectory();
- List<Mapping> mappings =
- createMappings(innerExecRoot, sandboxScratchDir, inputs, mapSymlinkTargets);
+ List<Mapping> mappings = createMappings(sandboxScratchDir, inputs, mapSymlinkTargets);
Set<PathFragment> dirsToCreate = new HashSet<>(writableDirs);
for (PathFragment output : outputs.files()) {
@@ -180,7 +182,7 @@
sandboxScratchDir.getRelative(dir).createDirectoryAndParents();
}
- process.map(mappings);
+ process.createSandbox(sandboxName, mappings);
}
@Override
@@ -194,12 +196,12 @@
@Override
public void delete() {
try {
- process.unmap(innerExecRoot);
+ process.destroySandbox(sandboxName);
} catch (IOException e) {
// We use independent subdirectories for each action, so a failure to unmap one, while
// annoying, is not a big deal. The sandboxfs instance will be unmounted anyway after
// the build, which will cause these to go away anyway.
- log.warning("Cannot unmap " + innerExecRoot + ": " + e);
+ log.warning("Cannot unmap " + sandboxName + ": " + e);
}
try {
@@ -271,8 +273,6 @@
/**
* Creates a new set of mappings to sandbox the given inputs.
*
- * @param root unique working directory for the command, specified as an absolute path anchored at
- * the sandboxfs' mount point
* @param scratchDir writable used as the target for all writable mappings
* @param inputs collection of paths to expose within the sandbox as read-only mappings, given as
* a map of mapped path to target path. The target path may be null, in which case an empty
@@ -282,13 +282,13 @@
* @throws IOException if we fail to resolve symbolic links
*/
private static List<Mapping> createMappings(
- PathFragment root, Path scratchDir, SandboxInputs inputs, boolean sandboxfsMapSymlinkTargets)
+ Path scratchDir, SandboxInputs inputs, boolean sandboxfsMapSymlinkTargets)
throws IOException {
List<Mapping> mappings = new ArrayList<>();
mappings.add(
Mapping.builder()
- .setPath(root)
+ .setPath(rootFragment)
.setTarget(scratchDir.asFragment())
.setWritable(true)
.build());
@@ -324,7 +324,7 @@
}
mappings.add(
Mapping.builder()
- .setPath(root.getRelative(entry.getKey()))
+ .setPath(rootFragment.getRelative(entry.getKey()))
.setTarget(target)
.setWritable(false)
.build());
@@ -335,7 +335,7 @@
if (!inputs.getFiles().containsKey(entry.getKey())) {
mappings.add(
Mapping.builder()
- .setPath(root.getRelative(entry.getKey()))
+ .setPath(rootFragment.getRelative(entry.getKey()))
.setTarget(entry.getValue().asFragment())
.setWritable(false)
.build());
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/BaseSandboxfsProcessTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/BaseSandboxfsProcessTest.java
index 9f5fa0c..ec38cb4 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/BaseSandboxfsProcessTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/BaseSandboxfsProcessTest.java
@@ -93,37 +93,31 @@
// Create a file outside of the mount point to ensure it's not touched.
mountPoint.getRelative("../unrelated").createDirectory();
- // Create twp mappings: one to be deleted and one to be kept around throughout the test.
- Path keepMeFile = tmpDir.getRelative("one");
- FileSystemUtils.createEmptyFile(keepMeFile);
+ // Create first sandbox.
Path oneFile = tmpDir.getRelative("one");
FileSystemUtils.writeContent(oneFile, UTF_8, "One test data");
- process.map(
+ process.createSandbox(
+ "first",
ImmutableList.of(
Mapping.builder()
- .setPath(PathFragment.create("/keep-me"))
- .setTarget(keepMeFile.asFragment())
- .setWritable(false)
- .build(),
- Mapping.builder()
.setPath(PathFragment.create("/foo"))
.setTarget(oneFile.asFragment())
.setWritable(false)
.build()));
- assertThat(
- mountPoint.getDirectoryEntries())
- .containsExactly(mountPoint.getRelative("foo"), mountPoint.getRelative("keep-me"));
- assertThat(
- FileSystemUtils.readContent(mountPoint.getRelative("foo"), UTF_8))
+ Path first = mountPoint.getRelative("first");
+ assertThat(mountPoint.getDirectoryEntries()).containsExactly(first);
+ assertThat(first.getDirectoryEntries()).containsExactly(first.getRelative("foo"));
+ assertThat(FileSystemUtils.readContent(first.getRelative("foo"), UTF_8))
.isEqualTo("One test data");
- // Replace the previous mapping and create a new one.
+ // Create second sandbox, which is expected to be fully disjoint from the first one.
Path twoFile = tmpDir.getRelative("two");
FileSystemUtils.writeContent(twoFile, UTF_8, "Two test data");
- Path bazFile = tmpDir.getRelative("baz");
- FileSystemUtils.writeContent(bazFile, UTF_8, "Baz test data");
- process.unmap(PathFragment.create("/foo"));
- process.map(
+ Path longLink = tmpDir.getRelative("long/link");
+ longLink.getParentDirectory().createDirectoryAndParents();
+ longLink.createSymbolicLink(oneFile); // The target is irrelevant but must exist.
+ process.createSandbox(
+ "second",
ImmutableList.of(
Mapping.builder()
.setPath(PathFragment.create("/foo"))
@@ -131,43 +125,26 @@
.setWritable(false)
.build(),
Mapping.builder()
- .setPath(PathFragment.create("/bar"))
- .setTarget(bazFile.asFragment())
- .setWritable(true)
- .build()));
- assertThat(
- mountPoint.getDirectoryEntries())
- .containsExactly(mountPoint.getRelative("foo"), mountPoint.getRelative("bar"),
- mountPoint.getRelative("keep-me"));
- assertThat(
- FileSystemUtils.readContent(mountPoint.getRelative("foo"), UTF_8))
- .isEqualTo("Two test data");
- assertThat(
- FileSystemUtils.readContent(mountPoint.getRelative("bar"), UTF_8))
- .isEqualTo("Baz test data");
-
- // Replace all existing mappings, and try with a nested one.
- Path longLink = tmpDir.getRelative("long/link");
- longLink.getParentDirectory().createDirectoryAndParents();
- longLink.createSymbolicLink(oneFile); // The target is irrelevant but must exist.
- process.unmap(PathFragment.create("/foo"));
- process.unmap(PathFragment.create("/bar"));
- process.map(
- ImmutableList.of(
- Mapping.builder()
.setPath(PathFragment.create("/something/complex"))
.setTarget(longLink.asFragment())
.setWritable(false)
.build()));
- assertThat(
- mountPoint.getDirectoryEntries())
- .containsExactly(mountPoint.getRelative("keep-me"), mountPoint.getRelative("something"));
- assertThat(
- FileSystemUtils.readContent(mountPoint.getRelative("something/complex"), UTF_8))
+ Path second = mountPoint.getRelative("second");
+ assertThat(mountPoint.getDirectoryEntries()).containsExactly(first, second);
+ assertThat(second.getDirectoryEntries())
+ .containsExactly(second.getRelative("foo"), second.getRelative("something"));
+ assertThat(FileSystemUtils.readContent(first.getRelative("foo"), UTF_8))
+ .isEqualTo("One test data");
+ assertThat(FileSystemUtils.readContent(second.getRelative("foo"), UTF_8))
+ .isEqualTo("Two test data");
+ assertThat(FileSystemUtils.readContent(second.getRelative("something/complex"), UTF_8))
.isEqualTo("One test data");
+ // Destroy one sandbox and ensure the other remains.
+ process.destroySandbox("first");
+ assertThat(mountPoint.getDirectoryEntries()).containsExactly(second);
+
// Ensure that files that should not have been touched throughout the test are still there.
- assertThat(mountPoint.getRelative("keep-me").exists()).isTrue();
assertThat(mountPoint.getRelative("../unrelated").exists()).isTrue();
} finally {
process.destroy();
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcess.java b/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcess.java
index 86cdf42..c992aff 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcess.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcess.java
@@ -14,21 +14,19 @@
package com.google.devtools.build.lib.sandbox;
+import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
/**
* A fake in-process sandboxfs implementation that uses symlinks on the Bazel file system API.
- *
- * <p>TODO(jmmv): It's possible that this could replace {@link SymlinkedSandboxedSpawn} altogether,
- * simplifying all callers that need to perform a sandboxed spawn because they would all go through
- * the sandboxfs worker interface. Evaluate this idea once we are confident enough that we won't
- * just remove all sandboxfs support code.
*/
final class FakeSandboxfsProcess implements SandboxfsProcess {
@@ -39,7 +37,15 @@
private final PathFragment mountPoint;
/**
- * Whether this "process" is valid or not. Used to better represent the workflow of a real
+ * Tracker for the sandbox names that currently exist in the sandbox.
+ *
+ * <p>Used to catch mistakes in creating an already-existing sandbox or deleting a non-existent
+ * sandbox.
+ */
+ private final Set<String> activeSandboxes = new HashSet<>();
+
+ /**
+ * Whether this "process" is valid or not. Used to better represent the workflow of a real
* sandboxfs subprocess.
*/
private boolean alive = true;
@@ -80,13 +86,20 @@
}
@Override
- public synchronized void map(List<Mapping> mappings) throws IOException {
+ public synchronized void createSandbox(String name, List<Mapping> mappings) throws IOException {
checkState(alive, "Cannot be called after destroy()");
+ checkArgument(!PathFragment.containsSeparator(name));
+ checkArgument(!activeSandboxes.contains(name), "Sandbox %s mapped more than once", name);
+ activeSandboxes.add(name);
+
for (Mapping mapping : mappings) {
- checkState(mapping.path().isAbsolute(), "Mapping specifications are expected to be absolute"
- + " but %s is not", mapping.path());
- Path link = fileSystem.getPath(mountPoint).getRelative(mapping.path().toRelative());
+ checkArgument(
+ mapping.path().isAbsolute(),
+ "Mapping specifications are expected to be" + "absolute but %s is not",
+ mapping.path());
+ Path link =
+ fileSystem.getPath(mountPoint).getRelative(name).getRelative(mapping.path().toRelative());
link.getParentDirectory().createDirectoryAndParents();
Path target = fileSystem.getPath(mapping.target());
@@ -108,11 +121,13 @@
}
@Override
- public synchronized void unmap(PathFragment mapping) throws IOException {
+ public synchronized void destroySandbox(String name) throws IOException {
checkState(alive, "Cannot be called after destroy()");
- checkState(mapping.isAbsolute(), "Mapping specifications are expected to be absolute"
- + " but %s is not", mapping);
- fileSystem.getPath(mountPoint).getRelative(mapping.toRelative()).deleteTree();
+ checkArgument(!PathFragment.containsSeparator(name));
+ checkArgument(activeSandboxes.contains(name), "Sandbox %s not previously created", name);
+ activeSandboxes.remove(name);
+
+ fileSystem.getPath(mountPoint).getRelative(name).deleteTree();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcessTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcessTest.java
index 06c6b2b..28241f4 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcessTest.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/FakeSandboxfsProcessTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
@@ -50,4 +51,39 @@
IOException.class, () -> mount(tmpDir.getRelative("file")));
assertThat(expected).hasMessageThat().matches(".*/file.*not a directory");
}
+
+ @Test
+ public void testSandboxCreate_EnforcesNonRepeatedNames() throws IOException {
+ Path mountPoint = tmpDir.getRelative("mnt");
+ mountPoint.createDirectory();
+ SandboxfsProcess process = mount(mountPoint);
+
+ process.createSandbox("first", ImmutableList.of());
+ process.createSandbox("second", ImmutableList.of());
+ assertThrows(
+ IllegalArgumentException.class, () -> process.createSandbox("first", ImmutableList.of()));
+ }
+
+ @Test
+ public void testSandboxDestroy_EnforcesExistingName() throws IOException {
+ Path mountPoint = tmpDir.getRelative("mnt");
+ mountPoint.createDirectory();
+ SandboxfsProcess process = mount(mountPoint);
+
+ process.createSandbox("first", ImmutableList.of());
+ process.destroySandbox("first");
+ assertThrows(IllegalArgumentException.class, () -> process.destroySandbox("first"));
+ }
+
+ @Test
+ public void testCreateAndDestroySandbox_ReuseName() throws IOException {
+ Path mountPoint = tmpDir.getRelative("mnt");
+ mountPoint.createDirectory();
+ SandboxfsProcess process = mount(mountPoint);
+
+ process.createSandbox("first", ImmutableList.of());
+ process.destroySandbox("first");
+ process.createSandbox("first", ImmutableList.of());
+ process.destroySandbox("first");
+ }
}
