Skip to content

Commit

Permalink
Add a mechanism to pass extra flags to the process-wrapper.
Browse files Browse the repository at this point in the history
    This adds a new --process_wrapper_extra_flags repeated flag that takes
    extra flags to pass to the proces-wrapper.  These flags are appended to
    the invocation built by Blaze, so we can use this to override builtin
    computed values.

    We'll use this to controlledly roll out the changes from bazelbuild/bazel@7828118
    (and, later, bazelbuild/bazel@9c1853a), both of which required a Bazel release rollback
    due to unforeseen problems.

    Part of bazelbuild/bazel#10245.

    RELNOTES: None.
    PiperOrigin-RevId: 311162177
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 2a1fac4 commit 85f5ca7
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,32 @@
package com.google.devtools.build.lib.exec.local;

import com.google.devtools.common.options.Converters;
import com.google.devtools.common.options.Converters.StringConverter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.RegexPatternOption;
import java.time.Duration;
import java.util.List;

/**
* Local execution options.
*/
public class LocalExecutionOptions extends OptionsBase {

@Option(
name = "process_wrapper_extra_flags",
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
effectTags = {OptionEffectTag.EXECUTION},
converter = StringConverter.class,
allowMultiple = true,
help =
"Extra flags to pass to the process-wrapper. These are appended to the invocation "
+ "constructed by Bazel, so this can be used to override any computed defaults.")
public List<String> processWrapperExtraFlags;

@Option(
name = "local_termination_grace_seconds",
oldName = "local_sigkill_grace_seconds",
Expand Down Expand Up @@ -71,38 +85,6 @@ public class LocalExecutionOptions extends OptionsBase {
+ "for this explicit cancellation.")
public boolean localLockfreeOutput;

@Option(
name = "experimental_process_wrapper_graceful_sigterm",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.EXECUTION},
help =
"When true, make the process-wrapper propagate SIGTERMs (used by the dynamic scheduler "
+ "to stop process trees) to the subprocesses themselves, giving them the grace "
+ "period in --local_termination_grace_seconds before forcibly sending a SIGKILL.")
public boolean processWrapperGracefulSigterm;

@Option(
name = "experimental_process_wrapper_wait_fix",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.EXECUTION},
help = "Helper to roll out the process-wrapper's --wait_fix bug fix in a controlled manner.")
public boolean processWrapperWaitFix;

@Option(
name = "experimental_local_retries_on_crash",
defaultValue = "0",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.EXECUTION},
help =
"Number of times to retry a local action when we detect that it crashed. This exists "
+ "to workaround a bug in OSXFUSE which is tickled by the use of the dynamic "
+ "scheduler and --experimental_local_lockfree_output due to constant process "
+ "churn. The bug can be triggered by a cancelled process that ran *before* the "
+ "process we are trying to run, introducing corruption in its file reads.")
public int localRetriesOnCrash;

public Duration getLocalSigkillGraceSeconds() {
// TODO(ulfjack): Change localSigkillGraceSeconds type to Duration.
return Duration.ofSeconds(localSigkillGraceSeconds);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package com.google.devtools.build.lib.runtime;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
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;
Expand All @@ -32,10 +34,18 @@ 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;

/** Optional list of extra flags to append to the process-wrapper invocations. */
private final List<String> extraFlags;

/** Creates a new process-wrapper instance from explicit values. */
@VisibleForTesting
public ProcessWrapper(Path binPath) {
public ProcessWrapper(Path binPath, @Nullable Duration killDelay, List<String> extraFlags) {
this.binPath = binPath;
this.killDelay = killDelay;
this.extraFlags = extraFlags;
}

/**
Expand All @@ -46,17 +56,22 @@ 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();
List<String> extraFlags =
options == null ? ImmutableList.of() : options.processWrapperExtraFlags;

Path path = cmdEnv.getBlazeWorkspace().getBinTools().getEmbeddedPath(BIN_BASENAME);
if (OS.isPosixCompatible() && path != null && path.exists()) {
return new ProcessWrapper(path);
return new ProcessWrapper(path, killDelay, extraFlags);
} else {
return null;
}
}

/** Returns a new {@link CommandLineBuilder} for the process-wrapper tool. */
public CommandLineBuilder commandLineBuilder(List<String> commandArguments) {
return new CommandLineBuilder(binPath.getPathString(), commandArguments);
return new CommandLineBuilder(binPath.getPathString(), commandArguments, killDelay, extraFlags);
}

/**
Expand All @@ -66,15 +81,23 @@ public CommandLineBuilder commandLineBuilder(List<String> commandArguments) {
public static class CommandLineBuilder {
private final String processWrapperPath;
private final List<String> commandArguments;
@Nullable private final Duration killDelay;
private final List<String> extraFlags;

private Path stdoutPath;
private Path stderrPath;
private Duration timeout;
private Duration killDelay;
private Path statisticsPath;

private CommandLineBuilder(String processWrapperPath, List<String> commandArguments) {
private CommandLineBuilder(
String processWrapperPath,
List<String> commandArguments,
@Nullable Duration killDelay,
List<String> extraFlags) {
this.processWrapperPath = processWrapperPath;
this.commandArguments = commandArguments;
this.killDelay = killDelay;
this.extraFlags = extraFlags;
}

/** Sets the path to use for redirecting stdout, if any. */
Expand All @@ -95,15 +118,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;
Expand Down Expand Up @@ -131,6 +145,8 @@ public List<String> build() {
fullCommandLine.add("--stats=" + statisticsPath);
}

fullCommandLine.addAll(extraFlags);

fullCommandLine.addAll(commandArguments);

return fullCommandLine;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,8 @@ public FileOutErr getFileOutErr() {
}

@Override
public SortedMap<PathFragment, ActionInput> getInputMapping() {
public SortedMap<PathFragment, ActionInput> getInputMapping(
boolean expandTreeArtifactsInRunfiles) {
return inputMapping;
}

Expand Down Expand Up @@ -313,7 +314,9 @@ private FileSystem setupEnvironmentForFakeExecution() {

private static ProcessWrapper makeProcessWrapper(FileSystem fs, LocalExecutionOptions options) {
return new ProcessWrapper(
fs.getPath("/process-wrapper"), options.getLocalSigkillGraceSeconds(), ImmutableList.of());
fs.getPath("/process-wrapper"),
options.getLocalSigkillGraceSeconds(),
options.processWrapperExtraFlags);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ private Path makeProcessWrapperBin(String path) throws IOException {
}

@Test
public void testProcessWrapperCommandLineBuilder_buildsWithoutOptionalArguments()
public void testProcessWrapperCommandLineBuilder_BuildsWithoutOptionalArguments()
throws IOException {
ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, world");

Expand All @@ -63,7 +63,7 @@ public void testProcessWrapperCommandLineBuilder_buildsWithoutOptionalArguments(
}

@Test
public void testProcessWrapperCommandLineBuilder_buildsWithOptionalArguments()
public void testProcessWrapperCommandLineBuilder_BuildsWithOptionalArguments()
throws IOException {
ImmutableList<String> extraFlags = ImmutableList.of("--debug");
ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, world");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ private String getCpuTimeSpenderPath() {
}

@Test
public void testCommand_echo() throws Exception {
public void testCommand_Echo() throws Exception {
ImmutableList<String> commandArguments = ImmutableList.of("echo", "worker bees can leave");

Command command = new Command(commandArguments.toArray(new String[0]));
Expand All @@ -68,7 +68,7 @@ public void testCommand_echo() throws Exception {
}

@Test
public void testProcessWrappedCommand_echo() throws Exception {
public void testProcessWrappedCommand_Echo() throws Exception {
ImmutableList<String> commandArguments = ImmutableList.of("echo", "even drones can fly away");

List<String> fullCommandLine = getProcessWrapper().commandLineBuilder(commandArguments).build();
Expand All @@ -81,7 +81,7 @@ public void testProcessWrappedCommand_echo() throws Exception {
}

private void checkProcessWrapperStatistics(Duration userTimeToSpend, Duration systemTimeToSpend)
throws IOException, CommandException, InterruptedException {
throws IOException, CommandException {
ImmutableList<String> commandArguments =
ImmutableList.of(
getCpuTimeSpenderPath(),
Expand All @@ -102,26 +102,26 @@ private void checkProcessWrapperStatistics(Duration userTimeToSpend, Duration sy
}

@Test
public void testProcessWrappedCommand_withStatistics_spendUserTime()
throws CommandException, IOException, InterruptedException {
public void testProcessWrappedCommand_WithStatistics_SpendUserTime()
throws CommandException, IOException {
Duration userTimeToSpend = Duration.ofSeconds(10);
Duration systemTimeToSpend = Duration.ZERO;

checkProcessWrapperStatistics(userTimeToSpend, systemTimeToSpend);
}

@Test
public void testProcessWrappedCommand_withStatistics_spendSystemTime()
throws CommandException, IOException, InterruptedException {
public void testProcessWrappedCommand_WithStatistics_SpendSystemTime()
throws CommandException, IOException {
Duration userTimeToSpend = Duration.ZERO;
Duration systemTimeToSpend = Duration.ofSeconds(10);

checkProcessWrapperStatistics(userTimeToSpend, systemTimeToSpend);
}

@Test
public void testProcessWrappedCommand_withStatistics_spendUserAndSystemTime()
throws CommandException, IOException, InterruptedException {
public void testProcessWrappedCommand_WithStatistics_SpendUserAndSystemTime()
throws CommandException, IOException {
Duration userTimeToSpend = Duration.ofSeconds(10);
Duration systemTimeToSpend = Duration.ofSeconds(10);

Expand Down

0 comments on commit 85f5ca7

Please sign in to comment.