From da8b1be208161eac380827724dbc66d84c69ca2b Mon Sep 17 00:00:00 2001 From: "bazel.build machine account" Date: Wed, 19 Jun 2024 04:28:50 -0400 Subject: [PATCH] [7.2.1] Do not fail a sandboxed spawn for missing execution statistics (#22790) This was already the case for "local" spawns. Statistics may be missing if the spawn wrapper exits abnormally. Fixes #22151. Closes #22780. PiperOrigin-RevId: 644378541 Change-Id: Ia3d792f380b78945523f21875c593744b60f0c81 Commit https://github.com/bazelbuild/bazel/commit/ec41dd1fe71380ac724399839f5bf22c3969f24b Co-authored-by: Fabian Meumertzheim --- .../build/lib/actions/SpawnResult.java | 25 +++++++++++++++++ .../lib/exec/local/LocalSpawnRunner.java | 27 ++----------------- .../sandbox/AbstractSandboxSpawnRunner.java | 24 +---------------- .../build/lib/shell/ExecutionStatistics.java | 5 ++++ .../devtools/build/lib/vfs/FileSystem.java | 1 + 5 files changed, 34 insertions(+), 48 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java index a137a2d6731787..60c14b8627550a 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java @@ -20,12 +20,15 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.exec.Protos.Digest; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.shell.ExecutionStatistics; import com.google.devtools.build.lib.shell.TerminationStatus; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.ExitCode; +import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.Path; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.ByteString; +import java.io.IOException; import java.io.InputStream; import java.time.Instant; import java.util.Locale; @@ -671,6 +674,28 @@ public Builder setDigest(Digest digest) { this.digest = digest; return this; } + + /** Adds execution statistics based on a {@code execution_statistics.proto} file. */ + @CanIgnoreReturnValue + public Builder setResourceUsageFromProto(Path statisticsPath) throws IOException { + ExecutionStatistics.getResourceUsage(statisticsPath) + .ifPresent( + resourceUsage -> { + setUserTimeInMs((int) resourceUsage.getUserExecutionTime().toMillis()); + setSystemTimeInMs((int) resourceUsage.getSystemExecutionTime().toMillis()); + setNumBlockOutputOperations(resourceUsage.getBlockOutputOperations()); + setNumBlockInputOperations(resourceUsage.getBlockInputOperations()); + setNumInvoluntaryContextSwitches(resourceUsage.getInvoluntaryContextSwitches()); + // The memory usage of the largest child process. For Darwin maxrss returns size in + // bytes. + if (OS.getCurrent() == OS.DARWIN) { + setMemoryInKb(resourceUsage.getMaximumResidentSetSize() / 1000); + } else { + setMemoryInKb(resourceUsage.getMaximumResidentSetSize()); + } + }); + return this; + } } /** A {@link Spawn}'s metadata name and {@link Path}. */ 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 916b5b41ba052c..1b2dfa41fdfac2 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 @@ -51,12 +51,10 @@ import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; -import com.google.devtools.build.lib.shell.ExecutionStatistics; import com.google.devtools.build.lib.shell.Subprocess; import com.google.devtools.build.lib.shell.SubprocessBuilder; import com.google.devtools.build.lib.shell.TerminationStatus; import com.google.devtools.build.lib.util.NetUtil; -import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Path; import com.google.errorprone.annotations.FormatMethod; @@ -481,29 +479,8 @@ private SpawnResult start() if (status != Status.SUCCESS) { spawnResultBuilder.setFailureDetail(makeFailureDetail(exitCode, status, actionType)); } - if (statisticsPath != null && statisticsPath.exists()) { - ExecutionStatistics.getResourceUsage(statisticsPath) - .ifPresent( - resourceUsage -> { - spawnResultBuilder.setUserTimeInMs( - (int) resourceUsage.getUserExecutionTime().toMillis()); - spawnResultBuilder.setSystemTimeInMs( - (int) resourceUsage.getSystemExecutionTime().toMillis()); - spawnResultBuilder.setNumBlockOutputOperations( - resourceUsage.getBlockOutputOperations()); - spawnResultBuilder.setNumBlockInputOperations( - resourceUsage.getBlockInputOperations()); - spawnResultBuilder.setNumInvoluntaryContextSwitches( - resourceUsage.getInvoluntaryContextSwitches()); - // The memory usage of the largest child process. For Darwin maxrss returns size - // in bytes. - if (OS.getCurrent() == OS.DARWIN) { - spawnResultBuilder.setMemoryInKb( - resourceUsage.getMaximumResidentSetSize() / 1000); - } else { - spawnResultBuilder.setMemoryInKb(resourceUsage.getMaximumResidentSetSize()); - } - }); + if (statisticsPath != null) { + spawnResultBuilder.setResourceUsageFromProto(statisticsPath); } spawnMetrics.setTotalTimeInMs((int) totalTimeStopwatch.elapsed().toMillis()); spawnResultBuilder.setSpawnMetrics(spawnMetrics.build()); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java index 6b69f1edc367e3..fd96a034e13638 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java @@ -46,7 +46,6 @@ import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Sandbox.Code; -import com.google.devtools.build.lib.shell.ExecutionStatistics; import com.google.devtools.build.lib.shell.Subprocess; import com.google.devtools.build.lib.shell.SubprocessBuilder; import com.google.devtools.build.lib.shell.TerminationStatus; @@ -309,28 +308,7 @@ private final SpawnResult run( Path statisticsPath = sandbox.getStatisticsPath(); if (statisticsPath != null) { - ExecutionStatistics.getResourceUsage(statisticsPath) - .ifPresent( - resourceUsage -> { - spawnResultBuilder.setUserTimeInMs( - (int) resourceUsage.getUserExecutionTime().toMillis()); - spawnResultBuilder.setSystemTimeInMs( - (int) resourceUsage.getSystemExecutionTime().toMillis()); - spawnResultBuilder.setNumBlockOutputOperations( - resourceUsage.getBlockOutputOperations()); - spawnResultBuilder.setNumBlockInputOperations( - resourceUsage.getBlockInputOperations()); - spawnResultBuilder.setNumInvoluntaryContextSwitches( - resourceUsage.getInvoluntaryContextSwitches()); - // The memory usage of the largest child process. For Darwin maxrss returns size in - // bytes. - if (OS.getCurrent() == OS.DARWIN) { - spawnResultBuilder.setMemoryInKb( - resourceUsage.getMaximumResidentSetSize() / 1000); - } else { - spawnResultBuilder.setMemoryInKb(resourceUsage.getMaximumResidentSetSize()); - } - }); + spawnResultBuilder.setResourceUsageFromProto(statisticsPath); } return spawnResultBuilder.build(); diff --git a/src/main/java/com/google/devtools/build/lib/shell/ExecutionStatistics.java b/src/main/java/com/google/devtools/build/lib/shell/ExecutionStatistics.java index bda5feafa3c775..c0603566212b61 100644 --- a/src/main/java/com/google/devtools/build/lib/shell/ExecutionStatistics.java +++ b/src/main/java/com/google/devtools/build/lib/shell/ExecutionStatistics.java @@ -16,6 +16,7 @@ import com.google.devtools.build.lib.vfs.Path; import java.io.BufferedInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.time.Duration; @@ -40,6 +41,10 @@ public static Optional getResourceUsage(Path executionStatisticsP } else { return Optional.empty(); } + } catch (FileNotFoundException e) { + // Collecting resource usage is best-effort and the file may be missing if the wrapper around + // the command terminated abnormally. + return Optional.empty(); } } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java index dc1cb4570a9f43..98f78829a7cf40 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java @@ -729,6 +729,7 @@ protected void chmod(PathFragment path, int mode) throws IOException { /** * Creates an InputStream accessing the file denoted by the path. * + * @throws FileNotFoundException if the file does not exist * @throws IOException if there was an error opening the file for reading */ protected abstract InputStream getInputStream(PathFragment path) throws IOException;