Skip to content

Commit

Permalink
Move flags experimental_collect_local_sandbox_action_metrics and `e…
Browse files Browse the repository at this point in the history 
…xperimental_collect_local_action_metrics` to graveyard.

Also fixed test which are assumed that this directory doesn't exist.

PiperOrigin-RevId: 540574109
Change-Id: I7293adadd23946ff93a54f4ed5bb48ba3cae2b30
  • Loading branch information
@wilwell
wilwell authored and Copybara-Service committed Jun 15, 2023
1 parent 7aaa204 commit 549254e
Show file tree
Hide file tree
Showing 11 changed files with 51 additions and 233 deletions.
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
View file
Expand Up @@ -370,6 +370,22 @@ public static class BuildGraveyardOptions extends OptionsBase {
effectTags = {OptionEffectTag.NO_OP},
help = "No-op. To be removed.")
public boolean skymeldUi;

@Option(
name = "experimental_collect_local_action_metrics",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.EXECUTION},
help = "Deprecated no-op.")
public boolean collectLocalExecutionStatistics;

@Option(
name = "experimental_collect_local_sandbox_action_metrics",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.LOGGING,
effectTags = {OptionEffectTag.EXECUTION},
help = "Deprecated no-op.")
public boolean collectLocalSandboxExecutionStatistics;
}

/** This is where deprecated Bazel-specific options only used by the build command go to die. */
Expand Down
10 changes: 0 additions & 10 deletions src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
View file
Expand Up @@ -48,16 +48,6 @@ public class LocalExecutionOptions extends OptionsBase {
+ "all actions are allowed to execute locally")
public RegexPatternOption allowedLocalAction;

@Option(
name = "experimental_collect_local_action_metrics",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.EXECUTION},
help =
"When enabled, execution statistics (such as user and system time) are recorded for "
+ "locally executed actions which don't use sandboxing")
public boolean collectLocalExecutionStatistics;

@Option(
name = "experimental_local_lockfree_output",
defaultValue = "false",
Expand Down
11 changes: 5 additions & 6 deletions src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
View file
Expand Up @@ -66,6 +66,7 @@
import java.io.IOException;
import java.io.InterruptedIOException;
import java.io.OutputStream;
import java.nio.file.Files;
import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
Expand Down Expand Up @@ -193,7 +194,7 @@ public boolean handlesCaching() {

protected Path createActionTemp(Path execRoot) throws IOException {
return execRoot.getRelative(
java.nio.file.Files.createTempDirectory(
Files.createTempDirectory(
java.nio.file.Paths.get(execRoot.getPathString()), "local-spawn-runner.")
.getFileName()
.toString());
Expand Down Expand Up @@ -410,10 +411,8 @@ private SpawnResult start()
.commandLineBuilder(spawn.getArguments())
.addExecutionInfo(spawn.getExecutionInfo())
.setTimeout(context.getTimeout());
if (localExecutionOptions.collectLocalExecutionStatistics) {
statisticsPath = tmpDir.getRelative("stats.out");
commandLineBuilder.setStatisticsPath(statisticsPath);
}
statisticsPath = tmpDir.getRelative("stats.out");
commandLineBuilder.setStatisticsPath(statisticsPath);
args = ImmutableList.copyOf(commandLineBuilder.build());
} else {
subprocessBuilder.setTimeoutMillis(context.getTimeout().toMillis());
Expand Down Expand Up @@ -489,7 +488,7 @@ private SpawnResult start()
if (status != Status.SUCCESS) {
spawnResultBuilder.setFailureDetail(makeFailureDetail(exitCode, status, actionType));
}
if (statisticsPath != null) {
if (statisticsPath != null && statisticsPath.exists()) {
ExecutionStatistics.getResourceUsage(statisticsPath)
.ifPresent(
resourceUsage -> {
Expand Down
9 changes: 2 additions & 7 deletions src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
View file
Expand Up @@ -252,13 +252,8 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
.addExecutionInfo(spawn.getExecutionInfo())
.setTimeout(timeout);

final Path statisticsPath;
if (getSandboxOptions().collectLocalSandboxExecutionStatistics) {
statisticsPath = sandboxPath.getRelative("stats.out");
processWrapperCommandLineBuilder.setStatisticsPath(statisticsPath);
} else {
statisticsPath = null;
}
final Path statisticsPath = sandboxPath.getRelative("stats.out");
processWrapperCommandLineBuilder.setStatisticsPath(statisticsPath);

ImmutableList<String> commandLine =
ImmutableList.<String>builder()
Expand Down
7 changes: 2 additions & 5 deletions src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java
View file
Expand Up @@ -362,11 +362,8 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
} else if (sandboxOptions.sandboxFakeUsername) {
commandLineBuilder.setUseFakeUsername(true);
}
Path statisticsPath = null;
if (sandboxOptions.collectLocalSandboxExecutionStatistics) {
statisticsPath = sandboxPath.getRelative("stats.out");
commandLineBuilder.setStatisticsPath(statisticsPath);
}
Path statisticsPath = sandboxPath.getRelative("stats.out");
commandLineBuilder.setStatisticsPath(statisticsPath);
if (sandboxfsProcess != null) {
return new SandboxfsSandboxedSpawn(
sandboxfsProcess,
Expand Down
7 changes: 2 additions & 5 deletions src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
View file
Expand Up @@ -107,11 +107,8 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
.addExecutionInfo(spawn.getExecutionInfo())
.setTimeout(timeout);

Path statisticsPath = null;
if (getSandboxOptions().collectLocalSandboxExecutionStatistics) {
statisticsPath = sandboxPath.getRelative("stats.out");
commandLineBuilder.setStatisticsPath(statisticsPath);
}
Path statisticsPath = sandboxPath.getRelative("stats.out");
commandLineBuilder.setStatisticsPath(statisticsPath);

SandboxInputs inputs =
helpers.processInputFiles(
Expand Down
10 changes: 0 additions & 10 deletions src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
View file
Expand Up @@ -258,16 +258,6 @@ public ImmutableSet<Path> getInaccessiblePaths(FileSystem fs) {
return ImmutableSet.copyOf(inaccessiblePaths);
}

@Option(
name = "experimental_collect_local_sandbox_action_metrics",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.LOGGING,
effectTags = {OptionEffectTag.EXECUTION},
help =
"When enabled, execution statistics (such as user and system time) are recorded for "
+ "locally executed actions which use sandboxing")
public boolean collectLocalSandboxExecutionStatistics;

@Option(
name = "experimental_enable_docker_sandbox",
defaultValue = "false",
Expand Down
99 changes: 24 additions & 75 deletions src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
View file
Expand Up @@ -111,6 +111,8 @@
public class LocalSpawnRunnerTest {

private static class TestedLocalSpawnRunner extends LocalSpawnRunner {
private Path tmpDirPath;

public TestedLocalSpawnRunner(
Path execRoot,
LocalExecutionOptions localExecutionOptions,
Expand Down Expand Up @@ -144,8 +146,13 @@ protected Path createActionTemp(Path execRoot) throws IOException {
if (!tempDirPath.createDirectory()) {
throw new IOException(String.format("Could not create temp directory '%s'", tempDirPath));
}
this.tmpDirPath = tempDirPath;
return tempDirPath;
}

public Path getActionTemp() {
return tmpDirPath;
}
}

private static class FinishedSubprocess implements Subprocess {
Expand Down Expand Up @@ -376,14 +383,14 @@ public void vanillaZeroExit() throws Exception {

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.localSigkillGraceSeconds = 456;
options.collectLocalExecutionStatistics = false;
LocalSpawnRunner runner =
TestedLocalSpawnRunner testedRunner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
options,
resourceManager,
makeProcessWrapper(fs, options),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
LocalSpawnRunner runner = testedRunner;

FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr);
Expand All @@ -399,7 +406,12 @@ public void vanillaZeroExit() throws Exception {
assertThat(captor.getValue().getArgv())
.containsExactlyElementsIn(
ImmutableList.of(
"/process-wrapper", "--timeout=123", "--kill_delay=456", "/bin/echo", "Hi!"));
"/process-wrapper",
"--timeout=123",
"--kill_delay=456",
"--stats=" + testedRunner.getActionTemp().getRelative("stats.out"),
"/bin/echo",
"Hi!"));
assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(0);
assertThat(captor.getValue().getStdout()).isEqualTo(StreamAction.REDIRECT);
Expand Down Expand Up @@ -428,7 +440,6 @@ public void testParamFiles() throws Exception {

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.localSigkillGraceSeconds = 456;
options.collectLocalExecutionStatistics = false;
Path execRoot = fs.getPath("/execroot");
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
Expand Down Expand Up @@ -474,7 +485,6 @@ public void exec_materializesVirtualInputAsExecutable() throws Exception {
SubprocessBuilder.setDefaultSubprocessFactory(factory);
Path execRoot = fs.getPath("/execroot");
LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = false;
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
execRoot,
Expand Down Expand Up @@ -556,14 +566,14 @@ public void nonZeroExit() throws Exception {
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = false;
LocalSpawnRunner runner =
TestedLocalSpawnRunner testedRunner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
options,
resourceManager,
makeProcessWrapper(fs, options),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
LocalSpawnRunner runner = testedRunner;

assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
Expand All @@ -578,7 +588,12 @@ public void nonZeroExit() throws Exception {
assertThat(captor.getValue().getArgv())
.containsExactlyElementsIn(
ImmutableList.of(
"/process-wrapper", "--timeout=0", "--kill_delay=15", "/bin/echo", "Hi!"));
"/process-wrapper",
"--timeout=0",
"--kill_delay=15",
"--stats=" + testedRunner.getActionTemp().getRelative("stats.out"),
"/bin/echo",
"Hi!"));
assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
assertThat(captor.getValue().getStdout()).isEqualTo(StreamAction.REDIRECT);
assertThat(captor.getValue().getStdoutFile()).isEqualTo(new File("/out/stdout"));
Expand Down Expand Up @@ -791,7 +806,6 @@ public void checkPrefetchCalled() throws Exception {
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = false;
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
Expand All @@ -817,7 +831,6 @@ public void checkNoPrefetchCalled() throws Exception {
SubprocessBuilder.setDefaultSubprocessFactory(factory);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = false;
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
Expand Down Expand Up @@ -849,7 +862,6 @@ public void checkLocalEnvProviderCalled() throws Exception {
LocalEnvProvider localEnvProvider = mock(LocalEnvProvider.class);

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = false;
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
Expand Down Expand Up @@ -939,14 +951,13 @@ private Path getTemporaryEmbeddedBin(FileSystem fs) throws IOException {
}

@Test
public void hasExecutionStatistics_whenOptionIsEnabled() throws Exception {
public void hasExecutionStatistics() throws Exception {
// TODO(b/62588075) Currently no process-wrapper or execution statistics support in Windows.
assumeTrue(OS.getCurrent() != OS.WINDOWS);

FileSystem fs = new UnixFileSystem(DigestHashFunction.SHA256, /*hashAttributeName=*/ "");

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = true;

int minimumWallTimeToSpendInMs = 10 * 1000;

Expand Down Expand Up @@ -1011,68 +1022,6 @@ public void hasExecutionStatistics_whenOptionIsEnabled() throws Exception {
assertThat(spawnResult.getNumInvoluntaryContextSwitches()).isAtLeast(0L);
}

@Test
public void hasNoExecutionStatistics_whenOptionIsDisabled() throws Exception {
// TODO(b/62588075) Currently no process-wrapper or execution statistics support in Windows.
assumeTrue(OS.getCurrent() != OS.WINDOWS);

FileSystem fs = new UnixFileSystem(DigestHashFunction.SHA256, /*hashAttributeName=*/ "");

LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
options.collectLocalExecutionStatistics = false;

int minimumWallTimeToSpendInMs = 1 * 1000;

int minimumUserTimeToSpendInMs = minimumWallTimeToSpendInMs;
int minimumSystemTimeToSpendInMs = 0;

Path execRoot = getTemporaryExecRoot(fs);
Path embeddedBinaries = getTemporaryEmbeddedBin(fs);
BinTools binTools =
BinTools.forEmbeddedBin(embeddedBinaries, ImmutableList.of("process-wrapper"));
Path processWrapperPath = binTools.getEmbeddedPath("process-wrapper");
copyProcessWrapperIntoExecRoot(processWrapperPath);
Path cpuTimeSpenderPath = copyCpuTimeSpenderIntoExecRoot(execRoot);

LocalSpawnRunner runner =
new LocalSpawnRunner(
execRoot,
options,
resourceManager,
LocalSpawnRunnerTest::keepLocalEnvUnchanged,
binTools,
new ProcessWrapper(
processWrapperPath, /*killDelay=*/ Duration.ZERO, /*gracefulSigterm=*/ false),
SyscallCache.NO_CACHE,
Mockito.mock(RunfilesTreeUpdater.class));

Spawn spawn =
new SpawnBuilder(
cpuTimeSpenderPath.getPathString(),
String.valueOf(minimumUserTimeToSpendInMs / 1000L),
String.valueOf(minimumSystemTimeToSpendInMs / 1000L))
.build();

FileOutErr fileOutErr = new FileOutErr(fs.getPath("/dev/null"), fs.getPath("/dev/null"));
SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr);

SpawnResult spawnResult = runner.exec(spawn, policy);

assertThat(spawnResult.status()).isEqualTo(SpawnResult.Status.SUCCESS);
assertThat(spawnResult.exitCode()).isEqualTo(0);
assertThat(spawnResult.setupSuccess()).isTrue();
assertThat(spawnResult.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName());

assertThat(spawnResult.getWallTimeInMs()).isNotNull();
assertThat(spawnResult.getWallTimeInMs()).isAtLeast(minimumWallTimeToSpendInMs);
// Under heavy starvation, max wall time could be anything, so don't check it here.
assertThat(spawnResult.getUserTimeInMs()).isEqualTo(0);
assertThat(spawnResult.getSystemTimeInMs()).isEqualTo(0);
assertThat(spawnResult.getNumBlockOutputOperations()).isNull();
assertThat(spawnResult.getNumBlockInputOperations()).isNull();
assertThat(spawnResult.getNumInvoluntaryContextSwitches()).isNull();
}

// Check that relative paths in the Spawn are absolutized relative to the execroot passed to the
// LocalSpawnRunner.
@Test
Expand Down
34 changes: 0 additions & 34 deletions src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunnerTest.java
View file
Expand Up @@ -164,40 +164,6 @@ public void exec_collectsExecutionStatistics() throws Exception {
assertThat(spawnResult.getNumInvoluntaryContextSwitches()).isAtLeast(0L);
}

@Test
public void exec_statisticsCollectionDisabled_returnsEmptyStatistics() throws Exception {
CommandEnvironment commandEnvironment =
getCommandEnvironmentWithExecutionStatisticsOptionDisabled("workspace");
LinuxSandboxedSpawnRunner runner = setupSandboxAndCreateRunner(commandEnvironment);
Path cpuTimeSpenderPath =
SpawnRunnerTestUtil.copyCpuTimeSpenderIntoPath(commandEnvironment.getExecRoot());
Duration minimumWallTimeToSpend = Duration.ofSeconds(10);
// Because of e.g. interference, wall time taken may be much larger than CPU time used.
Duration maximumWallTimeToSpend = Duration.ofSeconds(40);
Duration minimumUserTimeToSpend = minimumWallTimeToSpend;
Duration minimumSystemTimeToSpend = Duration.ZERO;
Spawn spawn =
new SpawnBuilder(
cpuTimeSpenderPath.getPathString(),
String.valueOf(minimumUserTimeToSpend.getSeconds()),
String.valueOf(minimumSystemTimeToSpend.getSeconds()))
.build();
SpawnExecutionContext policy = createSpawnExecutionContext(spawn);

SpawnResult spawnResult = runner.exec(spawn, policy);

assertThat(spawnResult.status()).isEqualTo(SpawnResult.Status.SUCCESS);
assertThat(spawnResult.exitCode()).isEqualTo(0);
assertThat(spawnResult.setupSuccess()).isTrue();
assertThat(spawnResult.getWallTimeInMs()).isAtLeast((int) minimumWallTimeToSpend.toMillis());
assertThat(spawnResult.getWallTimeInMs()).isAtMost((int) maximumWallTimeToSpend.toMillis());
assertThat(spawnResult.getUserTimeInMs()).isEqualTo(0);
assertThat(spawnResult.getSystemTimeInMs()).isEqualTo(0);
assertThat(spawnResult.getNumBlockOutputOperations()).isNull();
assertThat(spawnResult.getNumBlockInputOperations()).isNull();
assertThat(spawnResult.getNumInvoluntaryContextSwitches()).isNull();
}

@Test
public void hermeticTmp_tmpCreatedAndMounted() throws Exception {
runtimeWrapper.addOptions("--incompatible_sandbox_hermetic_tmp");
Expand Down

0 comments on commit 549254e

Please sign in to comment.