Fixing #3380: output stack traces and informative messages.
Also added tests specifically for the output, to ensure we don't break it again.
TESTED=remote worker, unit tests
RELNOTES: fixes #3380
PiperOrigin-RevId: 163283558
diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index f92cd8e..4641df0 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.exec;
-import com.google.common.base.Throwables;
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInput;
@@ -27,7 +26,6 @@
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.Spawns;
-import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.exec.SpawnResult.Status;
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
@@ -144,15 +142,6 @@
try {
result = spawnRunner.exec(spawn, policy);
} catch (IOException e) {
- if (verboseFailures) {
- actionExecutionContext
- .getEventHandler()
- .handle(
- Event.warn(
- spawn.getMnemonic()
- + " remote work failed:\n"
- + Throwables.getStackTraceAsString(e)));
- }
throw new EnvironmentalExecException("Unexpected IO error.", e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/CacheNotFoundException.java b/src/main/java/com/google/devtools/build/lib/remote/CacheNotFoundException.java
index cffc58f..15ff3ef 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/CacheNotFoundException.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/CacheNotFoundException.java
@@ -15,24 +15,21 @@
package com.google.devtools.build.lib.remote;
import com.google.devtools.remoteexecution.v1test.Digest;
+import java.io.IOException;
/**
* An exception to indicate cache misses.
* TODO(olaola): have a class of checked RemoteCacheExceptions.
*/
-public final class CacheNotFoundException extends Exception {
+public final class CacheNotFoundException extends IOException {
private final Digest missingDigest;
CacheNotFoundException(Digest missingDigest) {
+ super("Missing digest: " + missingDigest);
this.missingDigest = missingDigest;
}
public Digest getMissingDigest() {
return missingDigest;
}
-
- @Override
- public String toString() {
- return "Missing digest: " + missingDigest;
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
index c637a8d..ac12f6a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -19,7 +19,6 @@
import com.google.bytestream.ByteStreamProto.ReadRequest;
import com.google.bytestream.ByteStreamProto.ReadResponse;
import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
@@ -29,7 +28,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.remote.Digests.ActionKey;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
-import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
@@ -54,6 +52,7 @@
import io.grpc.Status;
import io.grpc.StatusRuntimeException;
import io.grpc.protobuf.StatusProto;
+import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.util.ArrayList;
@@ -197,7 +196,7 @@
*/
@Override
public void download(ActionResult result, Path execRoot, FileOutErr outErr)
- throws IOException, InterruptedException, CacheNotFoundException {
+ throws IOException, InterruptedException {
for (OutputFile file : result.getOutputFilesList()) {
Path path = execRoot.getRelative(file.getPath());
FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
@@ -211,25 +210,17 @@
file.getContent().writeTo(stream);
}
} else {
- try {
- retrier.execute(
- () -> {
- try (OutputStream stream = path.getOutputStream()) {
- Iterator<ReadResponse> replies = readBlob(digest);
- while (replies.hasNext()) {
- replies.next().getData().writeTo(stream);
- }
- }
- Digest receivedDigest = Digests.computeDigest(path);
- if (!receivedDigest.equals(digest)) {
- throw new IOException(
- "Digest does not match " + receivedDigest + " != " + digest);
- }
- return null;
- });
- } catch (RetryException e) {
- Throwables.throwIfInstanceOf(e.getCause(), CacheNotFoundException.class);
- throw e;
+ retrier.execute(
+ () -> {
+ try (OutputStream stream = path.getOutputStream()) {
+ readBlob(digest, stream);
+ }
+ return null;
+ });
+ Digest receivedDigest = Digests.computeDigest(path);
+ if (!receivedDigest.equals(digest)) {
+ throw new IOException(
+ "Digest does not match " + receivedDigest + " != " + digest);
}
}
}
@@ -243,7 +234,7 @@
}
private void downloadOutErr(ActionResult result, FileOutErr outErr)
- throws IOException, InterruptedException, CacheNotFoundException {
+ throws IOException, InterruptedException {
if (!result.getStdoutRaw().isEmpty()) {
result.getStdoutRaw().writeTo(outErr.getOutputStream());
outErr.getOutputStream().flush();
@@ -268,21 +259,24 @@
* {@link StatusRuntimeException}. Note that the retrier implicitly catches it, so if this is used
* in the context of {@link Retrier#execute}, that's perfectly safe.
*
- * <p>On the other hand, this method can also throw {@link CacheNotFoundException}, but the
- * retrier also implicitly catches that and wraps it in a {@link RetryException}, so any caller
- * that wants to propagate the {@link CacheNotFoundException} needs to catch
- * {@link RetryException} and rethrow the cause if it is a {@link CacheNotFoundException}.
+ * <p>This method also converts any NOT_FOUND code returned from the server into a
+ * {@link CacheNotFoundException}. TODO(olaola): this is not enough. NOT_FOUND can also be raised
+ * by execute, in which case the server should return the missing digest in the Status.details
+ * field. This should be part of the API.
*/
- private Iterator<ReadResponse> readBlob(Digest digest)
- throws CacheNotFoundException, StatusRuntimeException {
+ private void readBlob(Digest digest, OutputStream stream)
+ throws IOException, StatusRuntimeException {
String resourceName = "";
if (!options.remoteInstanceName.isEmpty()) {
resourceName += options.remoteInstanceName + "/";
}
resourceName += "blobs/" + digest.getHash() + "/" + digest.getSizeBytes();
try {
- return bsBlockingStub()
+ Iterator<ReadResponse> replies = bsBlockingStub()
.read(ReadRequest.newBuilder().setResourceName(resourceName).build());
+ while (replies.hasNext()) {
+ replies.next().getData().writeTo(stream);
+ }
} catch (StatusRuntimeException e) {
if (e.getStatus().getCode() == Status.Code.NOT_FOUND) {
throw new CacheNotFoundException(digest);
@@ -405,29 +399,16 @@
}
byte[] downloadBlob(Digest digest)
- throws IOException, InterruptedException, CacheNotFoundException {
+ throws IOException, InterruptedException {
if (digest.getSizeBytes() == 0) {
return new byte[0];
}
- byte[] result = new byte[(int) digest.getSizeBytes()];
- try {
- retrier.execute(
- () -> {
- Iterator<ReadResponse> replies = readBlob(digest);
- int offset = 0;
- while (replies.hasNext()) {
- ByteString data = replies.next().getData();
- data.copyTo(result, offset);
- offset += data.size();
- }
- Preconditions.checkState(digest.getSizeBytes() == offset);
- return null;
- });
- } catch (RetryException e) {
- Throwables.throwIfInstanceOf(e.getCause(), CacheNotFoundException.class);
- throw e;
- }
- return result;
+ return retrier.execute(
+ () -> {
+ ByteArrayOutputStream stream = new ByteArrayOutputStream((int) digest.getSizeBytes());
+ readBlob(digest, stream);
+ return stream.toByteArray();
+ });
}
// Execution Cache API
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionCache.java
index f8c5326..4643ef9 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionCache.java
@@ -55,10 +55,12 @@
/**
* Download the output files and directory trees of a remotely executed action to the local
* machine, as well stdin / stdout to the given files.
+ *
+ * @throws CacheNotFoundException in case of a cache miss.
*/
// TODO(olaola): will need to amend to include the TreeNodeRepository for updating.
void download(ActionResult result, Path execRoot, FileOutErr outErr)
- throws IOException, InterruptedException, CacheNotFoundException;
+ throws IOException, InterruptedException;
/**
* Attempts to look up the given action in the remote cache and return its result, if present.
* Returns {@code null} if there is no such entry. Note that a successful result from this method
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
index 79ad85b..f767a0c 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -56,6 +56,7 @@
env.getExecRoot(),
remoteOptions,
createFallbackRunner(env),
+ executionOptions.verboseFailures,
cache,
executor);
RemoteSpawnStrategy spawnStrategy =
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index abe8fcc..1943f82 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.remote;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputFileCache;
@@ -39,6 +40,7 @@
import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
import com.google.devtools.remoteexecution.v1test.Platform;
import com.google.protobuf.Duration;
+import io.grpc.Status.Code;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
@@ -58,6 +60,7 @@
// TODO(olaola): This will be set on a per-action basis instead.
private final Platform platform;
private final SpawnRunner fallbackRunner;
+ private final boolean verboseFailures;
@Nullable private final RemoteActionCache remoteCache;
@Nullable private final GrpcRemoteExecutor remoteExecutor;
@@ -66,6 +69,7 @@
Path execRoot,
RemoteOptions options,
SpawnRunner fallbackRunner,
+ boolean verboseFailures,
@Nullable RemoteActionCache remoteCache,
@Nullable GrpcRemoteExecutor remoteExecutor) {
this.execRoot = execRoot;
@@ -74,6 +78,7 @@
this.fallbackRunner = fallbackRunner;
this.remoteCache = remoteCache;
this.remoteExecutor = remoteExecutor;
+ this.verboseFailures = verboseFailures;
}
@Override
@@ -149,21 +154,20 @@
return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
}
- io.grpc.Status grpcStatus = io.grpc.Status.fromThrowable(e);
- final String message;
- if (io.grpc.Status.UNAVAILABLE.getCode().equals(grpcStatus.getCode())) {
- message = "The remote executor/cache is unavailable: " + grpcStatus.getDescription();
+ String message = "";
+ if (e instanceof RetryException
+ && ((RetryException) e).causedByStatusCode(Code.UNAVAILABLE)) {
+ message = "The remote executor/cache is unavailable";
+ } else if (e instanceof CacheNotFoundException) {
+ message = "Failed to download from remote cache";
} else {
- message = "I/O Error in remote cache/executor: " + e.getMessage();
+ message = "Error in remote cache/executor";
}
- throw new EnvironmentalExecException(message, true);
- } catch (CacheNotFoundException e) {
- if (options.remoteLocalFallback) {
- return execLocally(spawn, policy, inputMap, remoteCache, actionKey);
+ // TODO(olaola): reuse the ErrorMessage class for these errors.
+ if (verboseFailures) {
+ message += "\n" + Throwables.getStackTraceAsString(e);
}
-
- String message = "Failed to download from remote cache: " + e.getMessage();
- throw new EnvironmentalExecException(message, true);
+ throw new EnvironmentalExecException(message, e, true);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RetryException.java b/src/main/java/com/google/devtools/build/lib/remote/RetryException.java
index 24ddd36..49fa6ab 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RetryException.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RetryException.java
@@ -24,7 +24,7 @@
private final int attempts;
RetryException(Throwable cause, int retryAttempts) {
- super(cause);
+ super(String.format("after %d attempts: %s", retryAttempts + 1, cause), cause);
this.attempts = retryAttempts + 1;
}
@@ -40,9 +40,4 @@
}
return false;
}
-
- @Override
- public String toString() {
- return String.format("after %d attempts: %s", attempts, getCause());
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
index daa4ab0..a9d54ac 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
@@ -79,7 +79,7 @@
}
public void downloadTree(Digest rootDigest, Path rootLocation)
- throws IOException, CacheNotFoundException, InterruptedException {
+ throws IOException, InterruptedException {
Directory directory = Directory.parseFrom(downloadBlob(rootDigest));
for (FileNode file : directory.getFilesList()) {
downloadFileContents(
@@ -110,7 +110,7 @@
@Override
public void download(ActionResult result, Path execRoot, FileOutErr outErr)
- throws IOException, CacheNotFoundException, InterruptedException {
+ throws IOException, InterruptedException {
for (OutputFile file : result.getOutputFilesList()) {
if (!file.getContent().isEmpty()) {
createFile(
@@ -129,7 +129,7 @@
}
private void downloadOutErr(ActionResult result, FileOutErr outErr)
- throws IOException, CacheNotFoundException, InterruptedException {
+ throws IOException, InterruptedException {
if (!result.getStdoutRaw().isEmpty()) {
result.getStdoutRaw().writeTo(outErr.getOutputStream());
outErr.getOutputStream().flush();
@@ -202,7 +202,7 @@
}
private void downloadFileContents(Digest digest, Path dest, boolean executable)
- throws IOException, CacheNotFoundException, InterruptedException {
+ throws IOException, InterruptedException {
FileSystemUtils.createDirectoryAndParents(dest.getParentDirectory());
try (OutputStream out = dest.getOutputStream()) {
downloadBlob(digest, out);
@@ -248,7 +248,7 @@
}
public void downloadBlob(Digest digest, OutputStream out)
- throws IOException, CacheNotFoundException, InterruptedException {
+ throws IOException, InterruptedException {
if (digest.getSizeBytes() == 0) {
return;
}
@@ -259,7 +259,7 @@
}
public byte[] downloadBlob(Digest digest)
- throws IOException, CacheNotFoundException, InterruptedException {
+ throws IOException, InterruptedException {
if (digest.getSizeBytes() == 0) {
return new byte[0];
}
