Switching Bazel to use the new remote execution API: https://docs.google.com/document/d/1AaGk7fOPByEvpAbqeXIyE8HX_A3_axxNnvroblTZ_6s/edit

Also refactored away the various *Interface* files, no need since unit testing can be done with mocking the appropriate gRPC Impl classes directly (see tests). This also fixes the RemoteSpawnRunner, which should use different objects for remote caching and remote execution, the same way RemoteSpawnStrategy does.

RELNOTES: n/a
PiperOrigin-RevId: 158473700

1 files changed, 65 insertions, 71 deletions

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 bbb0166e4e..8807d0652b 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,32 +14,28 @@
 
 package com.google.devtools.build.lib.remote;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
+import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.exec.SpawnResult;
 import com.google.devtools.build.lib.exec.SpawnResult.Status;
 import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.remote.ContentDigests.ActionKey;
-import com.google.devtools.build.lib.remote.RemoteProtocol.Action;
-import com.google.devtools.build.lib.remote.RemoteProtocol.ActionResult;
-import com.google.devtools.build.lib.remote.RemoteProtocol.Command;
-import com.google.devtools.build.lib.remote.RemoteProtocol.ContentDigest;
-import com.google.devtools.build.lib.remote.RemoteProtocol.ExecuteReply;
-import com.google.devtools.build.lib.remote.RemoteProtocol.ExecuteRequest;
-import com.google.devtools.build.lib.remote.RemoteProtocol.ExecutionStatus;
-import com.google.devtools.build.lib.remote.RemoteProtocol.Platform;
+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.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.remoteexecution.v1test.Action;
+import com.google.devtools.remoteexecution.v1test.ActionResult;
+import com.google.devtools.remoteexecution.v1test.Command;
+import com.google.devtools.remoteexecution.v1test.Digest;
+import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
+import com.google.devtools.remoteexecution.v1test.Platform;
+import com.google.protobuf.Duration;
 import com.google.protobuf.TextFormat;
 import com.google.protobuf.TextFormat.ParseException;
 import io.grpc.StatusRuntimeException;
@@ -49,9 +45,7 @@ import java.util.List;
 import java.util.SortedMap;
 import java.util.TreeSet;
 
-/**
- * A client for the remote execution service.
- */
+/** A client for the remote execution service. */
 @ThreadSafe
 final class RemoteSpawnRunner implements SpawnRunner {
   private final Path execRoot;
@@ -60,11 +54,13 @@ final class RemoteSpawnRunner implements SpawnRunner {
   private final Platform platform;
 
   private final GrpcRemoteExecutor executor;
+  private final GrpcActionCache remoteCache;
 
   RemoteSpawnRunner(
       Path execRoot,
       RemoteOptions options,
-      GrpcRemoteExecutor executor) {
+      GrpcRemoteExecutor executor,
+      GrpcActionCache remoteCache) {
     this.execRoot = execRoot;
     this.options = options;
     if (options.experimentalRemotePlatformOverride != null) {
@@ -79,27 +75,12 @@ final class RemoteSpawnRunner implements SpawnRunner {
       platform = null;
     }
     this.executor = executor;
-  }
-
-  RemoteSpawnRunner(
-      Path execRoot,
-      RemoteOptions options,
-      AuthAndTLSOptions authTlsOptions) {
-    this(execRoot, options, connect(options, authTlsOptions));
-  }
-
-  private static GrpcRemoteExecutor connect(RemoteOptions options,
-      AuthAndTLSOptions authTlsOptions) {
-    Preconditions.checkArgument(GrpcRemoteExecutor.isRemoteExecutionOptions(options));
-    ChannelOptions channelOptions = ChannelOptions.create(authTlsOptions,
-        options.grpcMaxChunkSizeBytes);
-    return new GrpcRemoteExecutor(
-        RemoteUtils.createChannel(options.remoteExecutor, channelOptions), channelOptions, options);
+    this.remoteCache = remoteCache;
   }
 
   @Override
   public SpawnResult exec(Spawn spawn, SpawnExecutionPolicy policy)
-      throws InterruptedException, IOException {
+      throws ExecException, InterruptedException, IOException {
     ActionExecutionMetadata owner = spawn.getResourceOwner();
     if (owner.getOwner() != null) {
       policy.report(ProgressStatus.EXECUTING);
@@ -116,46 +97,38 @@ final class RemoteSpawnRunner implements SpawnRunner {
       Action action =
           buildAction(
               spawn.getOutputFiles(),
-              ContentDigests.computeDigest(command),
-              repository.getMerkleDigest(inputRoot));
+              Digests.computeDigest(command),
+              repository.getMerkleDigest(inputRoot),
+              // TODO(olaola): set sensible local and remote timouts.
+              Spawns.getTimeoutSeconds(spawn, 120));
 
-      ActionKey actionKey = ContentDigests.computeActionKey(action);
+      ActionKey actionKey = Digests.computeActionKey(action);
       ActionResult result =
-          this.options.remoteAcceptCached ? executor.getCachedActionResult(actionKey) : null;
+          options.remoteAcceptCached ? remoteCache.getCachedActionResult(actionKey) : null;
       if (result == null) {
         // Cache miss or we don't accept cache hits.
         // Upload the command and all the inputs into the remote cache.
-        executor.uploadBlob(command.toByteArray());
-        // TODO(olaola): this should use the ActionInputFileCache for SHA1 digests!
-        executor.uploadTree(repository, execRoot, inputRoot);
+        remoteCache.uploadBlob(command.toByteArray());
+        remoteCache.uploadTree(repository, execRoot, inputRoot);
         // TODO(olaola): set BuildInfo and input total bytes as well.
         ExecuteRequest.Builder request =
             ExecuteRequest.newBuilder()
+                .setInstanceName(options.remoteInstanceName)
                 .setAction(action)
-                .setAcceptCached(this.options.remoteAcceptCached)
+                .setWaitForCompletion(true)
                 .setTotalInputFileCount(inputMap.size())
-                .setTimeoutMillis(policy.getTimeoutMillis());
-        ExecuteReply reply = executor.executeRemotely(request.build());
-        ExecutionStatus status = reply.getStatus();
-
-        if (!status.getSucceeded()
-            && (status.getError() != ExecutionStatus.ErrorCode.EXEC_FAILED)) {
-          return new SpawnResult.Builder()
-              // TODO(ulfjack): Improve the translation of the error status.
-              .setStatus(Status.EXECUTION_FAILED)
-              .setExitCode(-1)
-              .build();
-        }
-
-        result = reply.getResult();
+                .setSkipCacheLookup(!options.remoteAcceptCached);
+        result = executor.executeRemotely(request.build()).getResult();
       }
 
       // TODO(ulfjack): Download stdout, stderr, and the output files in a single call.
-      passRemoteOutErr(executor, result, policy.getFileOutErr());
-      executor.downloadAllResults(result, execRoot);
+      passRemoteOutErr(remoteCache, result, policy.getFileOutErr());
+      if (result.getExitCode() == 0) {
+        remoteCache.downloadAllResults(result, execRoot);
+      }
       return new SpawnResult.Builder()
-          .setStatus(Status.SUCCESS)
-          .setExitCode(result.getReturnCode())
+          .setStatus(Status.SUCCESS)  // Even if the action failed with non-zero exit code.
+          .setExitCode(result.getExitCode())
           .build();
     } catch (StatusRuntimeException | CacheNotFoundException e) {
       throw new IOException(e);
@@ -163,37 +136,58 @@ final class RemoteSpawnRunner implements SpawnRunner {
   }
 
   private Action buildAction(
-      Collection<? extends ActionInput> outputs, ContentDigest command, ContentDigest inputRoot) {
+      Collection<? extends ActionInput> outputs,
+      Digest command,
+      Digest inputRoot,
+      long timeoutSeconds) {
     Action.Builder action = Action.newBuilder();
     action.setCommandDigest(command);
     action.setInputRootDigest(inputRoot);
     // Somewhat ugly: we rely on the stable order of outputs here for remote action caching.
     for (ActionInput output : outputs) {
-      action.addOutputPath(output.getExecPathString());
+      // TODO: output directories should be handled here, when they are supported.
+      action.addOutputFiles(output.getExecPathString());
     }
     if (platform != null) {
       action.setPlatform(platform);
     }
+    action.setTimeout(Duration.newBuilder().setSeconds(timeoutSeconds));
     return action.build();
   }
 
   private Command buildCommand(List<String> arguments, ImmutableMap<String, String> environment) {
     Command.Builder command = Command.newBuilder();
-    command.addAllArgv(arguments);
+    command.addAllArguments(arguments);
     // Sorting the environment pairs by variable name.
     TreeSet<String> variables = new TreeSet<>(environment.keySet());
     for (String var : variables) {
-      command.addEnvironmentBuilder().setVariable(var).setValue(environment.get(var));
+      command.addEnvironmentVariablesBuilder().setName(var).setValue(environment.get(var));
     }
     return command.build();
   }
 
   private static void passRemoteOutErr(
-      RemoteActionCache cache, ActionResult result, FileOutErr outErr)
-      throws CacheNotFoundException {
-    ImmutableList<byte[]> streams =
-        cache.downloadBlobs(ImmutableList.of(result.getStdoutDigest(), result.getStderrDigest()));
-    outErr.printOut(new String(streams.get(0), UTF_8));
-    outErr.printErr(new String(streams.get(1), UTF_8));
+      RemoteActionCache cache, ActionResult result, FileOutErr outErr) throws IOException {
+    try {
+      if (!result.getStdoutRaw().isEmpty()) {
+        result.getStdoutRaw().writeTo(outErr.getOutputStream());
+        outErr.getOutputStream().flush();
+      } else if (result.hasStdoutDigest()) {
+        byte[] stdoutBytes = cache.downloadBlob(result.getStdoutDigest());
+        outErr.getOutputStream().write(stdoutBytes);
+        outErr.getOutputStream().flush();
+      }
+      if (!result.getStderrRaw().isEmpty()) {
+        result.getStderrRaw().writeTo(outErr.getErrorStream());
+        outErr.getErrorStream().flush();
+      } else if (result.hasStderrDigest()) {
+        byte[] stderrBytes = cache.downloadBlob(result.getStderrDigest());
+        outErr.getErrorStream().write(stderrBytes);
+        outErr.getErrorStream().flush();
+      }
+    } catch (CacheNotFoundException e) {
+      outErr.printOutLn("Failed to fetch remote stdout/err due to cache miss.");
+      outErr.getOutputStream().flush();
+    }
   }
 }