Skip to content

Commit

Permalink
Add the workspace name to the sandbox path for sandboxed workers.
Browse files Browse the repository at this point in the history
    PiperOrigin-RevId: 165571541
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 9b0b67e commit b76bcd0
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@

package com.google.devtools.build.lib.worker;

import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
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);
Expand All @@ -34,27 +34,39 @@ final class SandboxedWorker extends Worker {
@Override
void destroy() throws IOException {
super.destroy();
workDir.deleteTree();
FileSystemUtils.deleteTree(workDir);
}

@Override
public void prepareExecution(
Map<PathFragment, Path> inputFiles, SandboxOutputs outputs, Set<PathFragment> workerFiles)
throws IOException {
// Note that workerExecRoot isn't necessarily null at this point, so we can't do a Preconditions
// check for it: If a WorkerSpawnStrategy gets interrupted, finishExecution is not guaranteed to
// be called.
workerExecRoot = new WorkerExecRoot(workDir, inputFiles, outputs, workerFiles);
workerExecRoot.createFileSystem();

super.prepareExecution(inputFiles, outputs, 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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ public Worker create(WorkerKey key) throws Exception {
} else {
worker = new Worker(key, workerId, key.getExecRoot(), logFile);
}
worker.prepareExecution(key);
worker.createProcess();
if (workerOptions.workerVerbose) {
reporter.handle(
Event.info(
Expand Down Expand Up @@ -124,8 +126,8 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
files.addAll(key.getWorkerFilesWithHashes().keySet());
files.addAll(worker.getWorkerFilesWithHashes().keySet());
for (PathFragment file : files) {
HashCode oldHash = worker.getWorkerFilesWithHashes().get(file);
HashCode newHash = key.getWorkerFilesWithHashes().get(file);
HashCode oldHash = key.getWorkerFilesWithHashes().get(file);
HashCode newHash = worker.getWorkerFilesWithHashes().get(file);
if (!oldHash.equals(newHash)) {
msg.append("\n")
.append(file.getPathString())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.worker;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.Subscribe;
import com.google.devtools.build.lib.buildtool.BuildRequest;
Expand All @@ -26,6 +25,7 @@
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.commands.CleanCommand.CleanStartingEvent;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.OptionsBase;
import java.io.IOException;
Expand Down Expand Up @@ -105,7 +105,7 @@ public void buildStarting(BuildStartingEvent event) {

if (workerPool == null) {
workerPoolConfig = newConfig;
workerPool = new WorkerPool(workerFactory, workerPoolConfig, options.highPriorityWorkers);
workerPool = new WorkerPool(workerFactory, workerPoolConfig);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@
import com.google.devtools.build.lib.actions.ResourceManager;
import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.UserExecException;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.exec.SpawnResult;
import com.google.devtools.build.lib.exec.SpawnRunner;
import com.google.devtools.build.lib.sandbox.SandboxHelpers;
import com.google.devtools.build.lib.util.Preconditions;
Expand All @@ -47,7 +47,6 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -154,15 +153,15 @@ private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionPolicy policy)

long startTime = System.currentTimeMillis();
WorkResponse response = execInWorker(key, workRequest, policy);
Duration wallTime = Duration.ofMillis(System.currentTimeMillis() - startTime);
long wallTimeMillis = System.currentTimeMillis() - startTime;

FileOutErr outErr = policy.getFileOutErr();
response.getOutputBytes().writeTo(outErr.getErrorStream());

return new SpawnResult.Builder()
.setExitCode(response.getExitCode())
.setStatus(SpawnResult.Status.SUCCESS)
.setWallTime(wallTime)
.setWallTimeMillis(wallTimeMillis)
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.hash.HashCode;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
Expand All @@ -31,7 +31,7 @@
@RunWith(JUnit4.class)
public class WorkerFactoryTest {

final FileSystem fs = new InMemoryFileSystem(DigestHashFunction.MD5);
final FileSystem fs = new InMemoryFileSystem();

/**
* Regression test for b/64689608: The execroot of the sandboxed worker process must end with the
Expand All @@ -49,6 +49,8 @@ public void sandboxedWorkerPathEndsWithWorkspaceName() throws Exception {
"dummy",
HashCode.fromInt(0),
ImmutableSortedMap.of(),
ImmutableMap.of(),
ImmutableSet.of(),
true);
Path sandboxedWorkerPath = workerFactory.getSandboxedWorkerPath(workerKey, 1);

Expand Down

0 comments on commit b76bcd0

Please sign in to comment.