Support verboseFailures/BTECNF in remote cache

The RemoteSpawnCache should respect the verbose_failures option consistent with RemoteSpawnRunner's behavior, and it should ignore (per existing behavior) a CacheNotFound-only BulkTransferException. Refactored the instance check for IOException into a utility method on BulkTransferException. Closes #12032. PiperOrigin-RevId: 329691374
bazelbuild · Sep 2, 2020 · 54b062c · 54b062c
1 parent f77ced7
commit 54b062c
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 18 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BulkTransferException.java b/src/main/java/com/google/devtools/build/lib/remote/BulkTransferException.java
@@ -46,4 +46,9 @@ void add(IOException e) {
   boolean onlyCausedByCacheNotFoundException() {
     return allCacheNotFoundException;
   }
+
+  static boolean isOnlyCausedByCacheNotFoundException(Exception e) {
+    return e instanceof BulkTransferException
+        && ((BulkTransferException) e).onlyCausedByCacheNotFoundException();
+  }
 }
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
@@ -120,6 +120,7 @@ public void registerSpawnCache(ModuleActionContextRegistry.Builder registryBuild
         new RemoteSpawnCache(
             env.getExecRoot(),
             checkNotNull(env.getOptions().getOptions(RemoteOptions.class)),
+            checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures,
             cache,
             env.getBuildRequestId(),
             env.getCommandId().toString(),

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -26,6 +26,7 @@
 import build.bazel.remote.execution.v2.Platform;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -73,6 +74,7 @@ final class RemoteSpawnCache implements SpawnCache {
 
   private final Path execRoot;
   private final RemoteOptions options;
+  private final boolean verboseFailures;
 
   private final RemoteCache remoteCache;
   private final String buildRequestId;
@@ -93,6 +95,7 @@ final class RemoteSpawnCache implements SpawnCache {
   RemoteSpawnCache(
       Path execRoot,
       RemoteOptions options,
+      boolean verboseFailures,
       RemoteCache remoteCache,
       String buildRequestId,
       String commandId,
@@ -101,6 +104,7 @@ final class RemoteSpawnCache implements SpawnCache {
       ImmutableSet<ActionInput> filesToDownload) {
     this.execRoot = execRoot;
     this.options = options;
+    this.verboseFailures = verboseFailures;
     this.remoteCache = remoteCache;
     this.cmdlineReporter = cmdlineReporter;
     this.buildRequestId = buildRequestId;
@@ -214,12 +218,22 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
       } catch (CacheNotFoundException e) {
         // Intentionally left blank
       } catch (IOException e) {
-        String errorMsg = Utils.grpcAwareErrorMessage(e);
-        if (isNullOrEmpty(errorMsg)) {
-          errorMsg = e.getClass().getSimpleName();
+        if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) {
+          // Intentionally left blank
+        } else {
+          String errorMessage;
+          if (!verboseFailures) {
+            errorMessage = Utils.grpcAwareErrorMessage(e);
+          } else {
+            // On --verbose_failures print the whole stack trace
+            errorMessage = Throwables.getStackTraceAsString(e);
+          }
+          if (isNullOrEmpty(errorMessage)) {
+            errorMessage = e.getClass().getSimpleName();
+          }
+          errorMessage = "Reading from Remote Cache:\n" + errorMessage;
+          report(Event.warn(errorMessage));
         }
-        errorMsg = "Reading from Remote Cache:\n" + errorMsg;
-        report(Event.warn(errorMsg));
       } finally {
         withMetadata.detach(previous);
       }
@@ -268,12 +282,18 @@ public void store(SpawnResult result) throws ExecException, InterruptedException
             remoteCache.upload(
                 actionKey, action, command, execRoot, files, context.getFileOutErr());
           } catch (IOException e) {
-            String errorMsg = Utils.grpcAwareErrorMessage(e);
-            if (isNullOrEmpty(errorMsg)) {
-              errorMsg = e.getClass().getSimpleName();
+            String errorMessage;
+            if (!verboseFailures) {
+              errorMessage = Utils.grpcAwareErrorMessage(e);
+            } else {
+              // On --verbose_failures print the whole stack trace
+              errorMessage = Throwables.getStackTraceAsString(e);
+            }
+            if (isNullOrEmpty(errorMessage)) {
+              errorMessage = e.getClass().getSimpleName();
             }
-            errorMsg = "Writing to Remote Cache:\n" + errorMsg;
-            report(Event.warn(errorMsg));
+            errorMessage = "Writing to Remote Cache:\n" + errorMessage;
+            report(Event.warn(errorMessage));
           } finally {
             withMetadata.detach(previous);
           }

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
@@ -116,9 +116,8 @@ public class RemoteSpawnRunner implements SpawnRunner {
   private static final String VIOLATION_TYPE_MISSING = "MISSING";
 
   private static boolean retriableExecErrors(Exception e) {
-    if (e instanceof BulkTransferException) {
-      BulkTransferException bulkTransferException = (BulkTransferException) e;
-      return bulkTransferException.onlyCausedByCacheNotFoundException();
+    if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) {
+      return true;
     }
     if (!RemoteRetrierUtils.causedByStatus(e, Code.FAILED_PRECONDITION)) {
       return false;
@@ -613,11 +612,8 @@ private SpawnResult execLocallyAndUploadOrFail(
   private SpawnResult handleError(
       IOException exception, FileOutErr outErr, ActionKey actionKey, SpawnExecutionContext context)
       throws ExecException, InterruptedException, IOException {
-    boolean remoteCacheFailed = false;
-    if (exception instanceof BulkTransferException) {
-      BulkTransferException e = (BulkTransferException) exception;
-      remoteCacheFailed = e.onlyCausedByCacheNotFoundException();
-    }
+    boolean remoteCacheFailed =
+        BulkTransferException.isOnlyCausedByCacheNotFoundException(exception);
     if (exception.getCause() instanceof ExecutionStatusException) {
       ExecutionStatusException e = (ExecutionStatusException) exception.getCause();
       if (e.getResponse() != null) {

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -201,6 +201,7 @@ private RemoteSpawnCache remoteSpawnCacheWithOptions(RemoteOptions options) {
     return new RemoteSpawnCache(
         execRoot,
         options,
+        /* verboseFailures=*/ true,
         remoteCache,
         "build-req-id",
         "command-id",