diff options
author | Philipp Wollermann <philwo@google.com> | 2018-03-26 09:06:29 -0700 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-03-26 09:08:26 -0700 |
commit | 656a0bab1e025ff3c27d595284a4bf1c5a8d8028 (patch) | |
tree | 2307d1a23794923db1ab44e59f3e600abf380061 /src/main/java/com/google/devtools/build/lib/worker | |
parent | 26e7280deecc11172c1b16637d513c2d0d242c09 (diff) |
Big round of sandbox fixes / performance improvements.
- The number of stat() syscalls in the SymlinkedSandboxedSpawn was way too high. Do less, feel better.
- When using --experimental_sandbox_base, ensure that symlinks in the path are resolved. Before this, you had to check whether on your system /dev/shm is a symlink to /run/shm and then use that instead. Now it no longer matters, as symlinks are resolved.
- Remove an unnecessary directory creation from each sandboxed invocation. Turns out that the "tmpdir" that we created was no longer used after some changes to Bazel's TMPDIR handling.
- Use simpler sandbox paths, by using the unique ID for each Spawn provided by SpawnExecutionPolicy instead of a randomly generated temp folder name. This also saves a round-trip from our VFS to NIO and back. Clean up the sandbox base before each build to ensure that the unique IDs are actually unique. ;)
- Use Java 8's Process#isAlive to check whether a process is alive instead of trying to get the exitcode and catching an exception.
Closes #4913.
PiperOrigin-RevId: 190472170
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib/worker')
7 files changed, 152 insertions, 79 deletions
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 fedd037d1c..a25ce97c0c 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,17 +14,18 @@ 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.common.base.Preconditions; 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); @@ -38,35 +39,23 @@ final class SandboxedWorker extends Worker { } @Override - 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(); + 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); } @Override - 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()); + public void finishExecution(Path execRoot) throws IOException { + super.finishExecution(execRoot); + + workerExecRoot.copyOutputs(execRoot); + workerExecRoot = null; } } 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 55705f4485..1cbd077e81 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,6 +24,8 @@ 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; /** @@ -54,17 +56,15 @@ class Worker { final Worker self = this; this.shutdownHook = - new Thread() { - @Override - public void run() { - try { - self.shutdownHook = null; - self.destroy(); - } catch (IOException e) { - // We can't do anything here. - } - } - }; + new Thread( + () -> { + try { + self.shutdownHook = null; + self.destroy(); + } catch (IOException e) { + // We can't do anything here. + } + }); Runtime.getRuntime().addShutdownHook(shutdownHook); } @@ -80,7 +80,6 @@ class Worker { processBuilder.setWorkingDirectory(workDir.getPathFile()); processBuilder.setStderr(logFile.getPathFile()); processBuilder.setEnv(workerKey.getEnv()); - this.process = processBuilder.start(); } @@ -138,12 +137,7 @@ 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. - try { - process.exitValue(); - return false; - } catch (IllegalThreadStateException e) { - return true; - } + return !process.finished(); } InputStream getInputStream() { @@ -154,9 +148,17 @@ class Worker { return process.getOutputStream(); } - public void prepareExecution(WorkerKey key) throws IOException {} + public void prepareExecution( + Map<PathFragment, Path> inputFiles, + Set<PathFragment> outputFiles, + Set<PathFragment> workerFiles) + throws IOException { + if (process == null) { + createProcess(); + } + } - public void finishExecution(WorkerKey key) throws IOException {} + public void finishExecution(Path execRoot) 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 new file mode 100644 index 0000000000..4cf02ac5b7 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerExecRoot.java @@ -0,0 +1,99 @@ +// 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 6f75fcf29d..871a3a0c64 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,8 +65,6 @@ 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 4641b5b710..4211419531 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,7 +22,6 @@ 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; /** @@ -41,11 +40,7 @@ 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( @@ -55,8 +50,6 @@ 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)); @@ -64,8 +57,6 @@ 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; } @@ -93,14 +84,6 @@ 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 1be61e1f77..a615cf76cd 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,14 +151,12 @@ 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); + WorkResponse response = execInWorker(key, workRequest, policy, inputFiles, outputFiles); Duration wallTime = Duration.ofMillis(System.currentTimeMillis() - startTime); FileOutErr outErr = policy.getFileOutErr(); @@ -257,7 +255,12 @@ final class WorkerSpawnRunner implements SpawnRunner { } } - private WorkResponse execInWorker(WorkerKey key, WorkRequest request, SpawnExecutionPolicy policy) + private WorkResponse execInWorker( + WorkerKey key, + WorkRequest request, + SpawnExecutionPolicy policy, + Map<PathFragment, Path> inputFiles, + Set<PathFragment> outputFiles) throws InterruptedException, ExecException { Worker worker = null; WorkResponse response; @@ -275,7 +278,7 @@ final class WorkerSpawnRunner implements SpawnRunner { } try { - worker.prepareExecution(key); + worker.prepareExecution(inputFiles, outputFiles, key.getWorkerFilesWithHashes().keySet()); } catch (IOException e) { throw new UserExecException( ErrorMessage.builder() @@ -337,7 +340,7 @@ final class WorkerSpawnRunner implements SpawnRunner { } try { - worker.finishExecution(key); + worker.finishExecution(execRoot); } 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 2302fc7c1e..013dbe226a 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,10 +144,9 @@ 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()); @@ -176,7 +175,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { throw e; } - worker.finishExecution(key); + worker.finishExecution(execRoot); if (response == null) { throw new UserExecException( |