diff options
author | 2018-04-04 14:02:13 -0700 | |
---|---|---|
committer | 2018-04-04 14:03:33 -0700 | |
commit | 2a5512fa3041df96b140e96a30112d5137be8b63 (patch) | |
tree | 3e73629ba36153f846b6ab125308c1779c013d94 /src/main/java/com/google | |
parent | 7520dcce42217c8076b06ed88c0e4e04ed99a0f4 (diff) |
Internal change
PiperOrigin-RevId: 191642942
Diffstat (limited to 'src/main/java/com/google')
20 files changed, 252 insertions, 282 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java index ada8367946..d3489dbfe5 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.runtime; import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.ExecutorInitException; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; @@ -232,8 +231,7 @@ public abstract class BlazeModule { * @param request the build request * @param builder the builder to add action context providers and consumers to */ - public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) - throws ExecutorInitException {} + public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) {} /** * Called after each command. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java b/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java index c56a0fd8bf..f8f58308f0 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CacheFileDigestsModule.java @@ -59,6 +59,8 @@ public class CacheFileDigestsModule extends BlazeModule { @Override public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { + super.executorInit(env, request, builder); + ExecutionOptions options = request.getOptions(ExecutionOptions.class); if (lastKnownCacheSize == null || options.cacheSizeForComputedFileDigests != lastKnownCacheSize) { diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java index 9fee6544dd..33b21209da 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java @@ -50,11 +50,13 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { private static final String SANDBOX_DEBUG_SUGGESTION = "\n\nUse --sandbox_debug to see verbose messages from the sandbox"; + private final Path sandboxBase; private final SandboxOptions sandboxOptions; private final boolean verboseFailures; private final ImmutableSet<Path> inaccessiblePaths; - public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv) { + public AbstractSandboxSpawnRunner(CommandEnvironment cmdEnv, Path sandboxBase) { + this.sandboxBase = sandboxBase; this.sandboxOptions = cmdEnv.getOptions().getOptions(SandboxOptions.class); this.verboseFailures = cmdEnv.getOptions().getOptions(ExecutionOptions.class).verboseFailures; this.inaccessiblePaths = @@ -86,6 +88,7 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { SandboxedSpawn sandbox, SpawnExecutionPolicy policy, Path execRoot, + Path tmpDir, Duration timeout, Path statisticsPath) throws IOException, InterruptedException { @@ -94,7 +97,8 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { OutErr outErr = policy.getFileOutErr(); policy.prefetchInputs(); - SpawnResult result = run(originalSpawn, sandbox, outErr, timeout, execRoot, statisticsPath); + SpawnResult result = + run(originalSpawn, sandbox, outErr, timeout, execRoot, tmpDir, statisticsPath); policy.lockOutputFiles(); try { @@ -117,6 +121,7 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { OutErr outErr, Duration timeout, Path execRoot, + Path tmpDir, Path statisticsPath) throws IOException, InterruptedException { Command cmd = new Command( @@ -140,6 +145,9 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { long startTime = System.currentTimeMillis(); CommandResult commandResult; try { + if (!tmpDir.exists() && !tmpDir.createDirectory()) { + throw new IOException(String.format("Could not create temp directory '%s'", tmpDir)); + } commandResult = cmd.execute(outErr.getOutputStream(), outErr.getErrorStream()); if (Thread.currentThread().isInterrupted()) { throw new InterruptedException(); @@ -206,6 +214,17 @@ abstract class AbstractSandboxSpawnRunner implements SpawnRunner { } /** + * Returns a temporary directory that should be used as the sandbox directory for a single action. + */ + protected Path getSandboxRoot() throws IOException { + return sandboxBase.getRelative( + java.nio.file.Files.createTempDirectory( + java.nio.file.Paths.get(sandboxBase.getPathString()), "") + .getFileName() + .toString()); + } + + /** * Gets the list of directories that the spawn will assume to be writable. * * @throws IOException because we might resolve symlinks, which throws {@link IOException}. diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java index d20eec70b1..1060034ccc 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.io.ByteStreams; +import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionStrategy; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnActionContext; @@ -95,7 +96,6 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { private final Path execRoot; private final boolean allowNetwork; private final Path processWrapper; - private final Path sandboxBase; private final Duration timeoutKillDelay; private final @Nullable SandboxfsProcess sandboxfsProcess; @@ -123,14 +123,13 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { Duration timeoutKillDelay, @Nullable SandboxfsProcess sandboxfsProcess) throws IOException { - super(cmdEnv); + super(cmdEnv, sandboxBase); this.execRoot = cmdEnv.getExecRoot(); this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions()); this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem()); this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv); this.localEnvProvider = new XcodeLocalEnvProvider(cmdEnv.getRuntime().getProductName(), cmdEnv.getClientEnv()); - this.sandboxBase = sandboxBase; this.timeoutKillDelay = timeoutKillDelay; this.sandboxfsProcess = sandboxfsProcess; } @@ -194,19 +193,21 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { @Override protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy) - throws IOException, InterruptedException { + throws ExecException, IOException, InterruptedException { // Each invocation of "exec" gets its own sandbox. - Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId())); - sandboxPath.createDirectory(); - - // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like - // the normal execroot does. + Path sandboxPath = getSandboxRoot(); Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName()); - sandboxExecRoot.getParentDirectory().createDirectory(); - sandboxExecRoot.createDirectory(); + + // Each sandboxed action runs in its own directory so we don't need to make the temp directory's + // name unique (like we have to with standalone execution strategy). + // + // Note that, for sandboxfs-based executions, this temp directory lives outside of the sandboxfs + // instance. This is perfectly fine (because sandbox-exec controls accesses to this directory) + // and is actually desirable for performance reasons. + Path tmpDir = sandboxPath.getRelative("tmp"); Map<String, String> environment = - localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp"); + localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString()); final HashSet<Path> writableDirs = new HashSet<>(alwaysWritableDirs); ImmutableSet<Path> extraWritableDirs = getWritableDirs(sandboxExecRoot, environment); @@ -287,7 +288,7 @@ final class DarwinSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { } }; } - return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath); + return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath); } private void writeConfig( diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java index a4c05ea5a7..fc8a3c3fad 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java @@ -33,6 +33,6 @@ final class DarwinSandboxedStrategy extends AbstractSpawnStrategy { @Override public String toString() { - return "darwin-sandbox"; + return "sandboxed"; } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index 0d6c7e1410..e4c5ba6524 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java @@ -76,7 +76,6 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { private final Path execRoot; private final boolean allowNetwork; private final Path linuxSandbox; - private final Path sandboxBase; private final Path inaccessibleHelperFile; private final Path inaccessibleHelperDir; private final LocalEnvProvider localEnvProvider; @@ -97,13 +96,12 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { Path inaccessibleHelperFile, Path inaccessibleHelperDir, Duration timeoutKillDelay) { - super(cmdEnv); + super(cmdEnv, sandboxBase); this.fileSystem = cmdEnv.getRuntime().getFileSystem(); this.blazeDirs = cmdEnv.getDirectories(); this.execRoot = cmdEnv.getExecRoot(); this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions()); this.linuxSandbox = LinuxSandboxUtil.getLinuxSandbox(cmdEnv); - this.sandboxBase = sandboxBase; this.inaccessibleHelperFile = inaccessibleHelperFile; this.inaccessibleHelperDir = inaccessibleHelperDir; this.timeoutKillDelay = timeoutKillDelay; @@ -113,18 +111,16 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { @Override protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy) throws IOException, ExecException, InterruptedException { - // Each invocation of "exec" gets its own sandbox base, execroot and temporary directory. - Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId())); - sandboxPath.createDirectory(); - - // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like - // the normal execroot does. + // Each invocation of "exec" gets its own sandbox. + Path sandboxPath = getSandboxRoot(); Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName()); - sandboxExecRoot.getParentDirectory().createDirectory(); - sandboxExecRoot.createDirectory(); + + // Each sandboxed action runs in its own execroot, so we don't need to make the temp directory's + // name unique (like we have to with standalone execution strategy). + Path tmpDir = sandboxExecRoot.getRelative("tmp"); Map<String, String> environment = - localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp"); + localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString()); ImmutableSet<Path> writableDirs = getWritableDirs(sandboxExecRoot, environment); ImmutableSet<PathFragment> outputs = SandboxHelpers.getOutputFiles(spawn); @@ -137,13 +133,12 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { .setBindMounts(getReadOnlyBindMounts(blazeDirs, sandboxExecRoot)) .setUseFakeHostname(getSandboxOptions().sandboxFakeHostname) .setCreateNetworkNamespace(!(allowNetwork || Spawns.requiresNetwork(spawn))) - .setUseDebugMode(getSandboxOptions().sandboxDebug) - .setKillDelay(timeoutKillDelay); + .setUseDebugMode(getSandboxOptions().sandboxDebug); if (!timeout.isZero()) { commandLineBuilder.setTimeout(timeout); } - + commandLineBuilder.setKillDelay(timeoutKillDelay); if (spawn.getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_FAKEROOT)) { commandLineBuilder.setUseFakeRoot(true); } else if (getSandboxOptions().sandboxFakeUsername) { @@ -166,7 +161,7 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner { outputs, writableDirs); - return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath); + return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java index 1993fb8c20..bbebefa81d 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java @@ -37,7 +37,7 @@ public final class LinuxSandboxedStrategy extends AbstractSpawnStrategy { @Override public String toString() { - return "linux-sandbox"; + return "sandboxed"; } /** diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java index e79b2abcd7..fdc4abfdd7 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java @@ -35,9 +35,8 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne return OS.isPosixCompatible() && ProcessWrapperUtil.isSupported(cmdEnv); } - private final Path processWrapper; private final Path execRoot; - private final Path sandboxBase; + private final Path processWrapper; private final LocalEnvProvider localEnvProvider; private final Duration timeoutKillDelay; @@ -51,32 +50,29 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne */ ProcessWrapperSandboxedSpawnRunner( CommandEnvironment cmdEnv, Path sandboxBase, String productName, Duration timeoutKillDelay) { - super(cmdEnv); - this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv); + super(cmdEnv, sandboxBase); this.execRoot = cmdEnv.getExecRoot(); + this.timeoutKillDelay = timeoutKillDelay; + this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv); this.localEnvProvider = OS.getCurrent() == OS.DARWIN ? new XcodeLocalEnvProvider(productName, cmdEnv.getClientEnv()) : new PosixLocalEnvProvider(cmdEnv.getClientEnv()); - this.sandboxBase = sandboxBase; - this.timeoutKillDelay = timeoutKillDelay; } @Override protected SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy) throws ExecException, IOException, InterruptedException { // Each invocation of "exec" gets its own sandbox. - Path sandboxPath = sandboxBase.getRelative(Integer.toString(policy.getId())); - sandboxPath.createDirectory(); - - // b/64689608: The execroot of the sandboxed process must end with the workspace name, just like - // the normal execroot does. + Path sandboxPath = getSandboxRoot(); Path sandboxExecRoot = sandboxPath.getRelative("execroot").getRelative(execRoot.getBaseName()); - sandboxExecRoot.getParentDirectory().createDirectory(); - sandboxExecRoot.createDirectory(); + + // Each sandboxed action runs in its own execroot, so we don't need to make the temp directory's + // name unique (like we have to with standalone execution strategy). + Path tmpDir = sandboxExecRoot.getRelative("tmp"); Map<String, String> environment = - localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp"); + localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, tmpDir.getPathString()); Duration timeout = policy.getTimeout(); ProcessWrapperUtil.CommandLineBuilder commandLineBuilder = @@ -101,7 +97,7 @@ final class ProcessWrapperSandboxedSpawnRunner extends AbstractSandboxSpawnRunne SandboxHelpers.getOutputFiles(spawn), getWritableDirs(sandboxExecRoot, environment)); - return runSpawn(spawn, sandbox, policy, execRoot, timeout, statisticsPath); + return runSpawn(spawn, sandbox, policy, execRoot, tmpDir, timeout, statisticsPath); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java index 096f564ad8..83c279a726 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedStrategy.java @@ -33,6 +33,6 @@ final class ProcessWrapperSandboxedStrategy extends AbstractSpawnStrategy { @Override public String toString() { - return "processwrapper-sandbox"; + return "sandboxed"; } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java index a8fb190e48..3065393a96 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java @@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.collect.ImmutableList; import com.google.common.eventbus.Subscribe; -import com.google.devtools.build.lib.actions.ExecutorInitException; import com.google.devtools.build.lib.buildtool.BuildRequest; import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent; import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent; @@ -29,6 +28,8 @@ import com.google.devtools.build.lib.exec.ExecutorBuilder; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.Command; import com.google.devtools.build.lib.runtime.CommandEnvironment; +import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -66,16 +67,14 @@ public final class SandboxModule extends BlazeModule { } /** Computes the path to the sandbox base tree for the given running command. */ - private static Path computeSandboxBase(SandboxOptions options, CommandEnvironment env) - throws IOException { + private static Path computeSandboxBase(SandboxOptions options, CommandEnvironment env) { if (options.sandboxBase.isEmpty()) { return env.getOutputBase().getRelative("sandbox"); } else { String dirName = String.format("%s-sandbox.%s", env.getRuntime().getProductName(), Fingerprint.md5Digest(env.getOutputBase().toString())); FileSystem fileSystem = env.getRuntime().getFileSystem(); - Path resolvedSandboxBase = fileSystem.getPath(options.sandboxBase).resolveSymbolicLinks(); - return resolvedSandboxBase.getRelative(dirName); + return fileSystem.getPath(options.sandboxBase).getRelative(dirName); } } @@ -92,30 +91,18 @@ public final class SandboxModule extends BlazeModule { } @Override - public void executorInit(CommandEnvironment cmdEnv, BuildRequest request, ExecutorBuilder builder) - throws ExecutorInitException { + public void executorInit( + CommandEnvironment cmdEnv, BuildRequest request, ExecutorBuilder builder) { checkNotNull(env, "env not initialized; was beforeCommand called?"); SandboxOptions options = env.getOptions().getOptions(SandboxOptions.class); checkNotNull(options, "We were told to initialize the executor but the SandboxOptions are " + "not present; were they registered for all build commands?"); - try { - sandboxBase = computeSandboxBase(options, env); - } catch (IOException e) { - throw new ExecutorInitException( - "--experimental_sandbox_base points to an invalid directory", e); - } + sandboxBase = computeSandboxBase(options, env); ActionContextProvider provider; try { - // Ensure that each build starts with a clean sandbox base directory. Otherwise using the `id` - // that is provided by SpawnExecutionPolicy#getId to compute a base directory for a sandbox - // might result in an already existing directory. - if (sandboxBase.exists()) { - FileSystemUtils.deleteTree(sandboxBase); - } - sandboxBase.createDirectoryAndParents(); if (options.useSandboxfs) { Path mountPoint = sandboxBase.getRelative("sandboxfs"); @@ -130,7 +117,11 @@ public final class SandboxModule extends BlazeModule { provider = SandboxActionContextProvider.create(cmdEnv, sandboxBase, null); } } catch (IOException e) { - throw new ExecutorInitException("Failed to initialize sandbox", e); + env.getBlazeModuleEnvironment().exit( + new AbruptExitException( + "Failed to initialize sandbox: " + e, + ExitCode.LOCAL_ENVIRONMENTAL_ERROR)); + return; } builder.addActionContextProvider(provider); builder.addActionContextConsumer(new SandboxActionContextConsumer(cmdEnv)); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java index aba4454334..e6408dc3cc 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java @@ -14,11 +14,10 @@ package com.google.devtools.build.lib.sandbox; -import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.common.io.Files; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Symlinks; import java.io.IOException; import java.util.Collection; import java.util.List; @@ -77,33 +76,19 @@ interface SandboxedSpawn { for (PathFragment output : outputs) { Path source = sourceRoot.getRelative(output); Path target = targetRoot.getRelative(output); - - FileStatus stat = source.statNullable(Symlinks.NOFOLLOW); - if (stat != null) { + 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. - Path parentDir = target.getParentDirectory(); - if (parentDir != null) { - parentDir.createDirectoryAndParents(); - } - - if (stat.isSymbolicLink()) { - try { - source.renameTo(target); - } catch (IOException e) { - target.createSymbolicLink(source.readSymbolicLink()); - } - } else if (stat.isFile()) { - FileSystemUtils.moveFile(source, target); - } else if (stat.isDirectory()) { - try { - source.renameTo(target); - } catch (IOException e) { - // Failed to move directory directly, thus move it recursively. - target.createDirectory(); - FileSystemUtils.moveTreesBelow(source, target); - } + target.getParentDirectory().createDirectoryAndParents(); + Files.move(source.getPathFile(), target.getPathFile()); + } 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); } } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java index d2806db1a1..f4d41e38a3 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedSandboxedSpawn.java @@ -15,13 +15,14 @@ package com.google.devtools.build.lib.sandbox; import com.google.common.base.Preconditions; -import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; import java.io.IOException; import java.util.Collection; -import java.util.LinkedHashSet; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -74,8 +75,36 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { @Override public void createFileSystem() throws IOException { - createDirectories(); + Set<Path> createdDirs = new HashSet<>(); + cleanFileSystem(inputs.keySet()); + createDirectoryAndParentsWithCache(createdDirs, sandboxExecRoot); + createParentDirectoriesForInputs(createdDirs, inputs.keySet()); createInputs(inputs); + createWritableDirectories(createdDirs, writableDirs); + createDirectoriesForOutputs(createdDirs, outputs); + } + + private void cleanFileSystem(Set<PathFragment> allowedFiles) throws IOException { + if (sandboxExecRoot.exists(Symlinks.NOFOLLOW)) { + deleteExceptAllowedFiles(sandboxExecRoot, allowedFiles); + } + } + + private void deleteExceptAllowedFiles(Path root, Set<PathFragment> allowedFiles) + throws IOException { + for (Path p : root.getDirectoryEntries()) { + FileStatus stat = p.stat(Symlinks.NOFOLLOW); + if (!stat.isDirectory()) { + if (!allowedFiles.contains(p.relativeTo(sandboxExecRoot))) { + p.delete(); + } + } else { + deleteExceptAllowedFiles(p, allowedFiles); + if (p.readdir(Symlinks.NOFOLLOW).isEmpty()) { + p.delete(); + } + } + } } /** @@ -90,32 +119,29 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { * directories, too, because we'll get an IOException with EEXIST if inputs happen to be nested * once we start creating the symlinks for all inputs. */ - private void createDirectories() throws IOException { - LinkedHashSet<Path> dirsToCreate = new LinkedHashSet<>(); - - for (PathFragment path : Iterables.concat(inputs.keySet(), outputs)) { - Preconditions.checkArgument(!path.isAbsolute()); - Preconditions.checkArgument(!path.containsUplevelReferences()); - for (int i = 0; i < path.segmentCount(); i++) { - dirsToCreate.add(sandboxExecRoot.getRelative(path.subFragment(0, i))); - } - } - - for (Path path : dirsToCreate) { - path.createDirectory(); - } - - for (Path dir : writableDirs) { - if (dir.startsWith(sandboxExecRoot)) { - dir.createDirectoryAndParents(); - } + private void createParentDirectoriesForInputs(Set<Path> createdDirs, Set<PathFragment> inputs) + throws IOException { + for (PathFragment inputPath : inputs) { + Path dir = sandboxExecRoot.getRelative(inputPath).getParentDirectory(); + Preconditions.checkArgument( + dir.startsWith(sandboxExecRoot), "Bad relative path: '%s'", inputPath); + createDirectoryAndParentsWithCache(createdDirs, dir); } } - protected void createInputs(Map<PathFragment, Path> inputs) throws IOException { + private void createInputs(Map<PathFragment, Path> inputs) throws IOException { // All input files are relative to the execroot. for (Entry<PathFragment, Path> entry : inputs.entrySet()) { Path key = sandboxExecRoot.getRelative(entry.getKey()); + FileStatus keyStat = key.statNullable(Symlinks.NOFOLLOW); + if (keyStat != null) { + if (keyStat.isSymbolicLink() + && entry.getValue() != null + && key.readSymbolicLink().equals(entry.getValue().asFragment())) { + continue; + } + key.delete(); + } // A null value means that we're supposed to create an empty file as the input. if (entry.getValue() != null) { key.createSymbolicLink(entry.getValue()); @@ -125,6 +151,24 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { } } + private void createWritableDirectories(Set<Path> createdDirs, Set<Path> writableDirs) + throws IOException { + for (Path writablePath : writableDirs) { + if (writablePath.startsWith(sandboxExecRoot)) { + createDirectoryAndParentsWithCache(createdDirs, writablePath); + } + } + } + + /** Prepare the output directories in the sandbox. */ + private void createDirectoriesForOutputs(Set<Path> createdDirs, Collection<PathFragment> outputs) + throws IOException { + for (PathFragment output : outputs) { + createDirectoryAndParentsWithCache( + createdDirs, sandboxExecRoot.getRelative(output.getParentDirectory())); + } + } + @Override public void copyOutputs(Path execRoot) throws IOException { SandboxedSpawn.moveOutputs(outputs, sandboxExecRoot, execRoot); @@ -144,4 +188,11 @@ public class SymlinkedSandboxedSpawn implements SandboxedSpawn { // on here. } } + + private static void createDirectoryAndParentsWithCache(Set<Path> cache, Path dir) + throws IOException { + if (cache.add(dir)) { + dir.createDirectoryAndParents(); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java index bfc867d991..7647d17dd3 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java +++ b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java @@ -54,15 +54,20 @@ public class JavaSubprocessFactory implements SubprocessFactory { @Override public boolean finished() { - if (deadlineMillis > 0 - && System.currentTimeMillis() > deadlineMillis - && deadlineExceeded.compareAndSet(false, true)) { - // We use compareAndSet here to avoid calling destroy multiple times. Note that destroy - // returns immediately, and we don't want to wait in this method. - process.destroy(); + try { + if (deadlineMillis > 0 + && System.currentTimeMillis() > deadlineMillis + && deadlineExceeded.compareAndSet(false, true)) { + // We use compareAndSet here to avoid calling destroy multiple times. Note that destroy + // returns immediately, and we don't want to wait in this method. + process.destroy(); + } + // this seems to be the only non-blocking call for checking liveness + process.exitValue(); + return true; + } catch (IllegalThreadStateException e) { + return false; } - // this seems to be the only non-blocking call for checking liveness - return !process.isAlive(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java index a25ce97c0c..fedd037d1c 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/SandboxedWorker.java @@ -14,18 +14,17 @@ package com.google.devtools.build.lib.worker; -import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; -import java.util.Map; -import java.util.Set; /** A {@link Worker} that runs inside a sandboxed execution root. */ final class SandboxedWorker extends Worker { private final Path workDir; - private WorkerExecRoot workerExecRoot; SandboxedWorker(WorkerKey workerKey, int workerId, Path workDir, Path logFile) { super(workerKey, workerId, workDir, logFile); @@ -39,23 +38,35 @@ final class SandboxedWorker extends Worker { } @Override - public void prepareExecution( - Map<PathFragment, Path> inputFiles, - Set<PathFragment> outputFiles, - Set<PathFragment> workerFiles) - throws IOException { - Preconditions.checkState(workerExecRoot == null); - this.workerExecRoot = new WorkerExecRoot(workDir, inputFiles, outputFiles, workerFiles); - workerExecRoot.createFileSystem(); - - super.prepareExecution(inputFiles, outputFiles, workerFiles); + public void prepareExecution(WorkerKey key) throws IOException { + // Note: the key passed in here may be different from the key passed to the constructor for + // subsequent invocations of the same worker. + // TODO(ulfjack): Remove WorkerKey.getInputFiles and WorkerKey.getOutputFiles; they are only + // used to pass information to this method and the method below. Instead, don't pass the + // WorkerKey to this method but only the input and output files. + new SymlinkedSandboxedSpawn( + workDir, + workDir, + ImmutableList.of("/does_not_exist"), + ImmutableMap.of(), + key.getInputFiles(), + key.getOutputFiles(), + ImmutableSet.of()) + .createFileSystem(); } @Override - public void finishExecution(Path execRoot) throws IOException { - super.finishExecution(execRoot); - - workerExecRoot.copyOutputs(execRoot); - workerExecRoot = null; + public void finishExecution(WorkerKey key) throws IOException { + // Note: the key passed in here may be different from the key passed to the constructor for + // subsequent invocations of the same worker. + new SymlinkedSandboxedSpawn( + workDir, + workDir, + ImmutableList.of("/does_not_exist"), + ImmutableMap.of(), + key.getInputFiles(), + key.getOutputFiles(), + ImmutableSet.of()) + .copyOutputs(key.getExecRoot()); } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java index 1cbd077e81..55705f4485 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java @@ -24,8 +24,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.util.ArrayList; import java.util.List; -import java.util.Map; -import java.util.Set; import java.util.SortedMap; /** @@ -56,15 +54,17 @@ class Worker { final Worker self = this; this.shutdownHook = - new Thread( - () -> { - try { - self.shutdownHook = null; - self.destroy(); - } catch (IOException e) { - // We can't do anything here. - } - }); + new Thread() { + @Override + public void run() { + try { + self.shutdownHook = null; + self.destroy(); + } catch (IOException e) { + // We can't do anything here. + } + } + }; Runtime.getRuntime().addShutdownHook(shutdownHook); } @@ -80,6 +80,7 @@ class Worker { processBuilder.setWorkingDirectory(workDir.getPathFile()); processBuilder.setStderr(logFile.getPathFile()); processBuilder.setEnv(workerKey.getEnv()); + this.process = processBuilder.start(); } @@ -137,7 +138,12 @@ class Worker { boolean isAlive() { // This is horrible, but Process.isAlive() is only available from Java 8 on and this is the // best we can do prior to that. - return !process.finished(); + try { + process.exitValue(); + return false; + } catch (IllegalThreadStateException e) { + return true; + } } InputStream getInputStream() { @@ -148,17 +154,9 @@ class Worker { return process.getOutputStream(); } - public void prepareExecution( - Map<PathFragment, Path> inputFiles, - Set<PathFragment> outputFiles, - Set<PathFragment> workerFiles) - throws IOException { - if (process == null) { - createProcess(); - } - } + public void prepareExecution(WorkerKey key) throws IOException {} - public void finishExecution(Path execRoot) throws IOException {} + public void finishExecution(WorkerKey key) throws IOException {} public Path getLogFile() { return logFile; diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java deleted file mode 100644 index 4cf02ac5b7..0000000000 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java +++ /dev/null @@ -1,99 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// 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.devtools.build.lib.sandbox.SymlinkedSandboxedSpawn; -import com.google.devtools.build.lib.vfs.FileStatus; -import com.google.devtools.build.lib.vfs.FileSystemUtils; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Symlinks; -import java.io.IOException; -import java.util.Collection; -import java.util.Map; -import java.util.Map.Entry; -import java.util.Set; - -/** Creates and manages the contents of a working directory of a persistent worker. */ -final class WorkerExecRoot extends SymlinkedSandboxedSpawn { - private final Path workDir; - private final Set<PathFragment> workerFiles; - - public WorkerExecRoot( - Path workDir, - Map<PathFragment, Path> inputs, - Collection<PathFragment> outputs, - Set<PathFragment> workerFiles) { - super( - workDir, - workDir, - ImmutableList.of(), - ImmutableMap.of(), - inputs, - outputs, - ImmutableSet.of()); - this.workDir = workDir; - this.workerFiles = workerFiles; - } - - @Override - public void createFileSystem() throws IOException { - workDir.createDirectoryAndParents(); - deleteExceptAllowedFiles(workDir, workerFiles); - super.createFileSystem(); - } - - private void deleteExceptAllowedFiles(Path root, Set<PathFragment> allowedFiles) - throws IOException { - for (Path p : root.getDirectoryEntries()) { - FileStatus stat = p.stat(Symlinks.NOFOLLOW); - if (!stat.isDirectory()) { - if (!allowedFiles.contains(p.relativeTo(workDir))) { - p.delete(); - } - } else { - deleteExceptAllowedFiles(p, allowedFiles); - if (p.readdir(Symlinks.NOFOLLOW).isEmpty()) { - p.delete(); - } - } - } - } - - @Override - protected void createInputs(Map<PathFragment, Path> inputs) throws IOException { - // All input files are relative to the execroot. - for (Entry<PathFragment, Path> entry : inputs.entrySet()) { - Path key = workDir.getRelative(entry.getKey()); - FileStatus keyStat = key.statNullable(Symlinks.NOFOLLOW); - if (keyStat != null) { - if (keyStat.isSymbolicLink() - && entry.getValue() != null - && key.readSymbolicLink().equals(entry.getValue().asFragment())) { - continue; - } - key.delete(); - } - // A null value means that we're supposed to create an empty file as the input. - if (entry.getValue() != null) { - key.createSymbolicLink(entry.getValue()); - } else { - FileSystemUtils.createEmptyFile(key); - } - } - } -} diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java index 871a3a0c64..6f75fcf29d 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java @@ -65,6 +65,8 @@ final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker } else { worker = new Worker(key, workerId, key.getExecRoot(), logFile); } + worker.prepareExecution(key); + worker.createProcess(); if (workerOptions.workerVerbose) { reporter.handle( Event.info( diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java index 4211419531..4641b5b710 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.SortedMap; /** @@ -40,7 +41,11 @@ final class WorkerKey { * methods. */ private final HashCode workerFilesCombinedHash; + private final SortedMap<PathFragment, HashCode> workerFilesWithHashes; + + private final Map<PathFragment, Path> inputFiles; + private final Set<PathFragment> outputFiles; private final boolean mustBeSandboxed; WorkerKey( @@ -50,6 +55,8 @@ final class WorkerKey { String mnemonic, HashCode workerFilesCombinedHash, SortedMap<PathFragment, HashCode> workerFilesWithHashes, + Map<PathFragment, Path> inputFiles, + Set<PathFragment> outputFiles, boolean mustBeSandboxed) { this.args = ImmutableList.copyOf(Preconditions.checkNotNull(args)); this.env = ImmutableMap.copyOf(Preconditions.checkNotNull(env)); @@ -57,6 +64,8 @@ final class WorkerKey { this.mnemonic = Preconditions.checkNotNull(mnemonic); this.workerFilesCombinedHash = Preconditions.checkNotNull(workerFilesCombinedHash); this.workerFilesWithHashes = Preconditions.checkNotNull(workerFilesWithHashes); + this.inputFiles = Preconditions.checkNotNull(inputFiles); + this.outputFiles = Preconditions.checkNotNull(outputFiles); this.mustBeSandboxed = mustBeSandboxed; } @@ -84,6 +93,14 @@ final class WorkerKey { return workerFilesWithHashes; } + public Map<PathFragment, Path> getInputFiles() { + return inputFiles; + } + + public Set<PathFragment> getOutputFiles() { + return outputFiles; + } + public boolean mustBeSandboxed() { return mustBeSandboxed; } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index a615cf76cd..1be61e1f77 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -151,12 +151,14 @@ final class WorkerSpawnRunner implements SpawnRunner { spawn.getMnemonic(), workerFilesCombinedHash, workerFiles, + inputFiles, + outputFiles, policy.speculating()); WorkRequest workRequest = createWorkRequest(spawn, policy, flagFiles, inputFileCache); long startTime = System.currentTimeMillis(); - WorkResponse response = execInWorker(key, workRequest, policy, inputFiles, outputFiles); + WorkResponse response = execInWorker(key, workRequest, policy); Duration wallTime = Duration.ofMillis(System.currentTimeMillis() - startTime); FileOutErr outErr = policy.getFileOutErr(); @@ -255,12 +257,7 @@ final class WorkerSpawnRunner implements SpawnRunner { } } - private WorkResponse execInWorker( - WorkerKey key, - WorkRequest request, - SpawnExecutionPolicy policy, - Map<PathFragment, Path> inputFiles, - Set<PathFragment> outputFiles) + private WorkResponse execInWorker(WorkerKey key, WorkRequest request, SpawnExecutionPolicy policy) throws InterruptedException, ExecException { Worker worker = null; WorkResponse response; @@ -278,7 +275,7 @@ final class WorkerSpawnRunner implements SpawnRunner { } try { - worker.prepareExecution(inputFiles, outputFiles, key.getWorkerFilesWithHashes().keySet()); + worker.prepareExecution(key); } catch (IOException e) { throw new UserExecException( ErrorMessage.builder() @@ -340,7 +337,7 @@ final class WorkerSpawnRunner implements SpawnRunner { } try { - worker.finishExecution(execRoot); + worker.finishExecution(key); } catch (IOException e) { throw new UserExecException( ErrorMessage.builder() diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java index 013dbe226a..2302fc7c1e 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java @@ -144,9 +144,10 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { action.getMnemonic(), workerFilesCombinedHash, workerFiles, + ImmutableMap.<PathFragment, Path>of(), + ImmutableSet.<PathFragment>of(), /*mustBeSandboxed=*/ false); worker = workerPool.borrowObject(key); - worker.prepareExecution(ImmutableMap.of(), ImmutableSet.of(), workerFiles.keySet()); WorkRequest request = WorkRequest.getDefaultInstance(); request.writeDelimitedTo(worker.getOutputStream()); @@ -175,7 +176,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { throw e; } - worker.finishExecution(execRoot); + worker.finishExecution(key); if (response == null) { throw new UserExecException( |