diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java index c8c26d60a7cae8..619933f90a8aa8 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java @@ -328,8 +328,7 @@ private SpawnResult start() throws InterruptedException, IOException { ProcessWrapper.CommandLineBuilder commandLineBuilder = processWrapper .commandLineBuilder(spawn.getArguments()) - .setTimeout(context.getTimeout()) - .setKillDelay(Duration.ofSeconds(localExecutionOptions.localSigkillGraceSeconds)); + .setTimeout(context.getTimeout()); if (localExecutionOptions.collectLocalExecutionStatistics) { statisticsPath = tmpDir.getRelative("stats.out"); commandLineBuilder.setStatisticsPath(statisticsPath); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java b/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java index d85e6458cba9c9..27b45f175474c8 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.runtime; import com.google.common.annotations.VisibleForTesting; +import com.google.devtools.build.lib.exec.local.LocalExecutionOptions; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.OsUtils; import com.google.devtools.build.lib.vfs.Path; @@ -32,10 +33,14 @@ public final class ProcessWrapper { /** Path to the process-wrapper binary to use. */ private final Path binPath; + /** Grace delay between asking a process to stop and forcibly killing it, or null for none. */ + @Nullable private final Duration killDelay; + /** Creates a new process-wrapper instance from explicit values. */ @VisibleForTesting - public ProcessWrapper(Path binPath) { + public ProcessWrapper(Path binPath, @Nullable Duration killDelay) { this.binPath = binPath; + this.killDelay = killDelay; } /** @@ -46,9 +51,12 @@ public ProcessWrapper(Path binPath) { */ @Nullable public static ProcessWrapper fromCommandEnvironment(CommandEnvironment cmdEnv) { + LocalExecutionOptions options = cmdEnv.getOptions().getOptions(LocalExecutionOptions.class); + Duration killDelay = options == null ? null : options.getLocalSigkillGraceSeconds(); + Path path = cmdEnv.getBlazeWorkspace().getBinTools().getEmbeddedPath(BIN_BASENAME); if (OS.isPosixCompatible() && path != null && path.exists()) { - return new ProcessWrapper(path); + return new ProcessWrapper(path, killDelay); } else { return null; } @@ -56,7 +64,7 @@ public static ProcessWrapper fromCommandEnvironment(CommandEnvironment cmdEnv) { /** Returns a new {@link CommandLineBuilder} for the process-wrapper tool. */ public CommandLineBuilder commandLineBuilder(List commandArguments) { - return new CommandLineBuilder(binPath.getPathString(), commandArguments); + return new CommandLineBuilder(binPath.getPathString(), commandArguments, killDelay); } /** @@ -66,15 +74,18 @@ public CommandLineBuilder commandLineBuilder(List commandArguments) { public static class CommandLineBuilder { private final String processWrapperPath; private final List commandArguments; + @Nullable private final Duration killDelay; + private Path stdoutPath; private Path stderrPath; private Duration timeout; - private Duration killDelay; private Path statisticsPath; - private CommandLineBuilder(String processWrapperPath, List commandArguments) { + private CommandLineBuilder( + String processWrapperPath, List commandArguments, @Nullable Duration killDelay) { this.processWrapperPath = processWrapperPath; this.commandArguments = commandArguments; + this.killDelay = killDelay; } /** Sets the path to use for redirecting stdout, if any. */ @@ -95,15 +106,6 @@ public CommandLineBuilder setTimeout(Duration timeout) { return this; } - /** - * Sets the kill delay for commands run using the process-wrapper tool that exceed their - * timeout. - */ - public CommandLineBuilder setKillDelay(Duration killDelay) { - this.killDelay = killDelay; - return this; - } - /** Sets the path for writing execution statistics (e.g. resource usage). */ public CommandLineBuilder setStatisticsPath(Path statisticsPath) { this.statisticsPath = statisticsPath; 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 1901a11cef4520..d3d2ad8710be39 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 @@ -104,7 +104,6 @@ private static boolean computeIsSupported() { private final boolean allowNetwork; private final ProcessWrapper processWrapper; private final Path sandboxBase; - private final Duration timeoutKillDelay; @Nullable private final SandboxfsProcess sandboxfsProcess; private final boolean sandboxfsMapSymlinkTargets; private final TreeDeleter treeDeleter; @@ -124,7 +123,6 @@ private static boolean computeIsSupported() { * @param helpers common tools and state across all spawns during sandboxed execution * @param cmdEnv the command environment to use * @param sandboxBase path to the sandbox base directory - * @param timeoutKillDelay additional grace period before killing timing out commands * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in * which case the runner uses a symlinked sandbox * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true @@ -133,7 +131,6 @@ private static boolean computeIsSupported() { SandboxHelpers helpers, CommandEnvironment cmdEnv, Path sandboxBase, - Duration timeoutKillDelay, @Nullable SandboxfsProcess sandboxfsProcess, boolean sandboxfsMapSymlinkTargets, TreeDeleter treeDeleter) @@ -146,7 +143,6 @@ private static boolean computeIsSupported() { this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv); this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); this.sandboxBase = sandboxBase; - this.timeoutKillDelay = timeoutKillDelay; this.sandboxfsProcess = sandboxfsProcess; this.sandboxfsMapSymlinkTargets = sandboxfsMapSymlinkTargets; this.treeDeleter = treeDeleter; @@ -249,8 +245,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context ProcessWrapper.CommandLineBuilder processWrapperCommandLineBuilder = processWrapper.commandLineBuilder(spawn.getArguments()).setTimeout(timeout); - processWrapperCommandLineBuilder.setKillDelay(timeoutKillDelay); - final Path statisticsPath; if (getSandboxOptions().collectLocalSandboxExecutionStatistics) { statisticsPath = sandboxPath.getRelative("stats.out"); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerCommandLineBuilder.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerCommandLineBuilder.java index 62ad4495808edc..1b0ac27aac6e57 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerCommandLineBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerCommandLineBuilder.java @@ -33,7 +33,6 @@ final class DockerCommandLineBuilder { private Path sandboxExecRoot; private Map environmentVariables; private Duration timeout; - private Duration killDelay; private boolean createNetworkNamespace; private UUID uuid; private int uid; @@ -78,11 +77,6 @@ public DockerCommandLineBuilder setTimeout(Duration timeout) { return this; } - public DockerCommandLineBuilder setKillDelay(Duration killDelay) { - this.killDelay = killDelay; - return this; - } - public DockerCommandLineBuilder setCreateNetworkNamespace(boolean createNetworkNamespace) { this.createNetworkNamespace = createNetworkNamespace; return this; @@ -179,9 +173,6 @@ public List build() { if (timeout != null) { processWrapperCmdLine.setTimeout(timeout); } - if (killDelay != null) { - processWrapperCmdLine.setKillDelay(killDelay); - } return processWrapperCmdLine.build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index 6a5e5fdb599f97..39da19dc0321d0 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java @@ -144,7 +144,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) private final Path sandboxBase; private final String defaultImage; private final LocalEnvProvider localEnvProvider; - private final Duration timeoutKillDelay; private final String commandId; private final Reporter reporter; private final boolean useCustomizedImages; @@ -162,7 +161,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) * @param dockerClient path to the `docker` executable * @param sandboxBase path to the sandbox base directory * @param defaultImage the Docker image to use if the platform doesn't specify one - * @param timeoutKillDelay an additional grace period before killing timing out commands * @param useCustomizedImages whether to use customized images for execution * @param treeDeleter scheduler for tree deletions */ @@ -172,7 +170,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) Path dockerClient, Path sandboxBase, String defaultImage, - Duration timeoutKillDelay, boolean useCustomizedImages, TreeDeleter treeDeleter) { super(cmdEnv); @@ -184,7 +181,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient) this.sandboxBase = sandboxBase; this.defaultImage = defaultImage; this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); - this.timeoutKillDelay = timeoutKillDelay; this.commandId = cmdEnv.getCommandId().toString(); this.reporter = cmdEnv.getReporter(); this.useCustomizedImages = useCustomizedImages; @@ -258,7 +254,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context .setAdditionalMounts(getSandboxOptions().sandboxAdditionalMounts) .setPrivileged(getSandboxOptions().dockerPrivileged) .setEnvironmentVariables(environment) - .setKillDelay(timeoutKillDelay) .setCreateNetworkNamespace( !(allowNetwork || Spawns.requiresNetwork(spawn, getSandboxOptions().defaultSandboxAllowNetwork))) 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 6ae90109e57f98..38f40de91f0efe 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 @@ -42,7 +42,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { private final Path execRoot; private final Path sandboxBase; private final LocalEnvProvider localEnvProvider; - private final Duration timeoutKillDelay; @Nullable private final SandboxfsProcess sandboxfsProcess; private final boolean sandboxfsMapSymlinkTargets; private final TreeDeleter treeDeleter; @@ -53,7 +52,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { * @param helpers common tools and state across all spawns during sandboxed execution * @param cmdEnv the command environment to use * @param sandboxBase path to the sandbox base directory - * @param timeoutKillDelay additional grace period before killing timing out commands * @param sandboxfsProcess instance of the sandboxfs process to use; may be null for none, in * which case the runner uses a symlinked sandbox * @param sandboxfsMapSymlinkTargets map the targets of symlinks within the sandbox if true @@ -62,7 +60,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { SandboxHelpers helpers, CommandEnvironment cmdEnv, Path sandboxBase, - Duration timeoutKillDelay, @Nullable SandboxfsProcess sandboxfsProcess, boolean sandboxfsMapSymlinkTargets, TreeDeleter treeDeleter) { @@ -72,7 +69,6 @@ public static boolean isSupported(CommandEnvironment cmdEnv) { this.execRoot = cmdEnv.getExecRoot(); this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv()); this.sandboxBase = sandboxBase; - this.timeoutKillDelay = timeoutKillDelay; this.sandboxfsProcess = sandboxfsProcess; this.sandboxfsMapSymlinkTargets = sandboxfsMapSymlinkTargets; this.treeDeleter = treeDeleter; @@ -103,8 +99,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context ProcessWrapper.CommandLineBuilder commandLineBuilder = processWrapper.commandLineBuilder(spawn.getArguments()).setTimeout(timeout); - commandLineBuilder.setKillDelay(timeoutKillDelay); - Path statisticsPath = null; if (getSandboxOptions().collectLocalSandboxExecutionStatistics) { statisticsPath = sandboxPath.getRelative("stats.out"); 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 c539429c3ee035..cb96021790ec0f 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 @@ -289,7 +289,6 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil helpers, cmdEnv, sandboxBase, - timeoutKillDelay, sandboxfsProcess, options.sandboxfsMapSymlinkTargets, treeDeleter)); @@ -318,7 +317,6 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil pathToDocker, sandboxBase, defaultImage, - timeoutKillDelay, useCustomizedImages, treeDeleter)); spawnRunners.add(spawnRunner); @@ -360,7 +358,6 @@ private void setup(CommandEnvironment cmdEnv, SpawnStrategyRegistry.Builder buil helpers, cmdEnv, sandboxBase, - timeoutKillDelay, sandboxfsProcess, options.sandboxfsMapSymlinkTargets, treeDeleter)); diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index 5a0158e7e7a3b2..e0bad7330d81ce 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -312,8 +312,9 @@ private FileSystem setupEnvironmentForFakeExecution() { return new InMemoryFileSystem(); } - private static ProcessWrapper makeProcessWrapper(FileSystem fs) { - return new ProcessWrapper(fs.getPath("/process-wrapper")); + private static ProcessWrapper makeProcessWrapper(FileSystem fs, LocalExecutionOptions options) { + return new ProcessWrapper( + fs.getPath("/process-wrapper"), options.getLocalSigkillGraceSeconds()); } /** @@ -348,7 +349,7 @@ public void vanillaZeroExit() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); @@ -399,7 +400,7 @@ public void testParamFiles() throws Exception { execRoot, options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); ParamFileActionInput paramFileActionInput = new ParamFileActionInput( @@ -494,7 +495,7 @@ public void nonZeroExit() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); @@ -510,7 +511,6 @@ public void nonZeroExit() throws Exception { assertThat(captor.getValue().getArgv()) .containsExactlyElementsIn( ImmutableList.of( - // process-wrapper timeout grace_time stdout stderr "/process-wrapper", "--timeout=0", "--kill_delay=15", "/bin/echo", "Hi!")); assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value"); assertThat(captor.getValue().getStdout()).isEqualTo(StreamAction.REDIRECT); @@ -536,7 +536,7 @@ public void processStartupThrows() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); assertThat(fs.getPath("/out").createDirectory()).isTrue(); @@ -570,7 +570,7 @@ public void disallowLocalExecution() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); assertThat(fs.getPath("/execroot").createDirectory()).isTrue(); @@ -619,7 +619,7 @@ public void waitFor() throws InterruptedException { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); @@ -729,7 +729,7 @@ public void checkPrefetchCalled() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); @@ -754,7 +754,7 @@ public void checkNoPrefetchCalled() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), LocalSpawnRunnerTest::keepLocalEnvUnchanged); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); @@ -783,7 +783,7 @@ public void checkLocalEnvProviderCalled() throws Exception { fs.getPath("/execroot"), options, resourceManager, - makeProcessWrapper(fs), + makeProcessWrapper(fs, options), localEnvProvider); FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr")); @@ -910,7 +910,7 @@ public void hasExecutionStatistics_whenOptionIsEnabled() throws Exception { resourceManager, LocalSpawnRunnerTest::keepLocalEnvUnchanged, binTools, - new ProcessWrapper(processWrapperPath), + new ProcessWrapper(processWrapperPath, /*killDelay=*/ Duration.ZERO), Mockito.mock(RunfilesTreeUpdater.class)); Spawn spawn = @@ -974,7 +974,7 @@ public void hasNoExecutionStatistics_whenOptionIsDisabled() throws Exception { resourceManager, LocalSpawnRunnerTest::keepLocalEnvUnchanged, binTools, - new ProcessWrapper(processWrapperPath), + new ProcessWrapper(processWrapperPath, /*killDelay=*/ Duration.ZERO), Mockito.mock(RunfilesTreeUpdater.class)); Spawn spawn = diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java index 63b0746b38e14b..06de77cdbd07aa 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.time.Duration; import java.util.List; +import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -39,11 +40,12 @@ public void setUp() { testFS = new InMemoryFileSystem(); } - private ProcessWrapper getProcessWrapper(String path) throws IOException { + private ProcessWrapper getProcessWrapper(String path, @Nullable Duration killDelay) + throws IOException { Path processWrapperPath = testFS.getPath(path); processWrapperPath.getParentDirectory().createDirectoryAndParents(); processWrapperPath.getOutputStream().close(); - return new ProcessWrapper(processWrapperPath); + return new ProcessWrapper(processWrapperPath, killDelay); } @Test @@ -54,7 +56,7 @@ public void testProcessWrapperCommandLineBuilder_BuildsWithoutOptionalArguments( ImmutableList expectedCommandLine = ImmutableList.builder().add("/some/bin/path").addAll(commandArguments).build(); - ProcessWrapper processWrapper = getProcessWrapper("/some/bin/path"); + ProcessWrapper processWrapper = getProcessWrapper("/some/bin/path", /*killDelay=*/ null); List commandLine = processWrapper.commandLineBuilder(commandArguments).build(); assertThat(commandLine).containsExactlyElementsIn(expectedCommandLine).inOrder(); @@ -82,12 +84,11 @@ public void testProcessWrapperCommandLineBuilder_BuildsWithOptionalArguments() .addAll(commandArguments) .build(); - ProcessWrapper processWrapper = getProcessWrapper("/path/to/process-wrapper"); + ProcessWrapper processWrapper = getProcessWrapper("/path/to/process-wrapper", killDelay); List commandLine = processWrapper .commandLineBuilder(commandArguments) .setTimeout(timeout) - .setKillDelay(killDelay) .setStdoutPath(stdoutPath) .setStderrPath(stderrPath) .setStatisticsPath(statisticsPath) diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java index e6e87e82e67c6c..ce2056fb9efcd5 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java +++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java @@ -47,7 +47,8 @@ private ProcessWrapper getProcessWrapper() { return new ProcessWrapper( testFS .getPath(BlazeTestUtils.runfilesDir()) - .getRelative(TestConstants.PROCESS_WRAPPER_PATH)); + .getRelative(TestConstants.PROCESS_WRAPPER_PATH), + /*killDelay=*/ null); } private String getCpuTimeSpenderPath() {