remote: Don't cache test if marked "external". Fixes #3362

RELNOTES: None.
PiperOrigin-RevId: 161937673

6 files changed, 249 insertions, 8 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
index 46ff4b0a82..a07af7b558 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
@@ -143,4 +143,10 @@ public class ExecutionRequirements {
 
   public static final ImmutableMap<String, String> WORKER_MODE_ENABLED =
       ImmutableMap.of(SUPPORTS_WORKERS, "1");
+
+  /**
+   * Whether we should disable remote caching of an action. This can be set to force a rerun of an
+   * action even if there is a cache entry for it.
+   */
+  public static final String NO_CACHE = "no-cache";
 }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
index 35cfd5568e..d4827be875 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java
@@ -25,6 +25,13 @@ public final class Spawns {
   private Spawns() {}
 
   /**
+   * Returns {@code true} if the result of {@code spawn} may be cached.
+   */
+  public static boolean mayBeCached(Spawn spawn) {
+    return !spawn.getExecutionInfo().containsKey(ExecutionRequirements.NO_CACHE);
+  }
+
+  /**
    * Parse the timeout key in the spawn execution info, if it exists. Otherwise, return -1.
    */
   public static int getTimeoutSeconds(Spawn spawn) throws ExecException {
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 31913957bc..8ef6049ae8 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.ExecutionRequirements;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.SimpleSpawn;
@@ -47,7 +48,6 @@ import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
 import com.google.devtools.build.lib.view.test.TestStatus.TestResultData.Builder;
 import java.io.Closeable;
 import java.io.IOException;
-import java.util.HashMap;
 import java.util.Map;
 
 /** Runs TestRunnerAction actions. */
@@ -107,10 +107,13 @@ public class StandaloneTestStrategy extends TestStrategy {
 
     ResolvedPaths resolvedPaths = action.resolve(execRoot);
 
-    Map<String, String> info = new HashMap<>();
+    ImmutableMap.Builder<String, String> executionInfo = ImmutableMap.builder();
+    if (!action.shouldCacheResult()) {
+      executionInfo.put(ExecutionRequirements.NO_CACHE, "");
+    }
     // This key is only understood by StandaloneSpawnStrategy.
-    info.put("timeout", "" + getTimeout(action));
-    info.putAll(action.getTestProperties().getExecutionInfo());
+    executionInfo.put("timeout", "" + getTimeout(action));
+    executionInfo.putAll(action.getTestProperties().getExecutionInfo());
 
     ResourceSet localResourceUsage =
         action
@@ -123,7 +126,7 @@ public class StandaloneTestStrategy extends TestStrategy {
             action,
             getArgs(COLLECT_COVERAGE, action),
             ImmutableMap.copyOf(env),
-            ImmutableMap.copyOf(info),
+            executionInfo.build(),
             new RunfilesSupplierImpl(
                 runfilesDir.relativeTo(execRoot), action.getExecutionSettings().getRunfiles()),
             /*inputs=*/ ImmutableList.copyOf(action.getInputs()),
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java
index 68046fe2ca..06aabda492 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutor.java
@@ -40,7 +40,7 @@ import javax.annotation.Nullable;
 
 /** A remote work executor that uses gRPC for communicating the work, inputs and outputs. */
 @ThreadSafe
-final class GrpcRemoteExecutor {
+class GrpcRemoteExecutor {
 
   private final Channel channel;
   private final CallCredentials callCredentials;
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 7a50e1cd3f..58c6c668a6 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
@@ -96,7 +96,7 @@ final class RemoteSpawnRunner implements SpawnRunner {
     // Look up action cache, and reuse the action output if it is found.
     ActionKey actionKey = Digests.computeActionKey(action);
     try {
-      boolean acceptCachedResult = options.remoteAcceptCached;
+      boolean acceptCachedResult = options.remoteAcceptCached && Spawns.mayBeCached(spawn);
       ActionResult result =
           acceptCachedResult
               ? remoteCache.getCachedActionResult(actionKey)
@@ -198,7 +198,8 @@ final class RemoteSpawnRunner implements SpawnRunner {
       ActionKey actionKey)
       throws ExecException, IOException, InterruptedException {
     SpawnResult result = fallbackRunner.exec(spawn, policy);
-    if (options.remoteUploadLocalResults && remoteCache != null && actionKey != null) {
+    if (options.remoteUploadLocalResults && Spawns.mayBeCached(spawn) && remoteCache != null
+        && actionKey != null) {
       ArrayList<Path> outputFiles = new ArrayList<>();
       for (ActionInput output : spawn.getOutputFiles()) {
         Path outputFile = execRoot.getRelative(output.getExecPathString());
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
new file mode 100644
index 0000000000..0390994284
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -0,0 +1,224 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.remote;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.when;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.ExecutionRequirements;
+import com.google.devtools.build.lib.actions.ResourceSet;
+import com.google.devtools.build.lib.actions.SimpleSpawn;
+import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.exec.SpawnInputExpander;
+import com.google.devtools.build.lib.exec.SpawnRunner;
+import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
+import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
+import com.google.devtools.build.lib.exec.util.FakeOwner;
+import com.google.devtools.build.lib.remote.Digests.ActionKey;
+import com.google.devtools.build.lib.util.io.FileOutErr;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.common.options.Options;
+import com.google.devtools.remoteexecution.v1test.ActionResult;
+import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
+import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.SortedMap;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+
+import org.mockito.MockitoAnnotations;
+
+/** Tests for {@link com.google.devtools.build.lib.remote.RemoteSpawnRunner} */
+@RunWith(JUnit4.class)
+public class RemoteSpawnRunnerTest {
+
+  private static final ImmutableMap<String, String> NO_CACHE =
+      ImmutableMap.of(ExecutionRequirements.NO_CACHE, "");
+
+  private Path execRoot;
+  private FakeActionInputFileCache fakeFileCache;
+  private FileOutErr outErr;
+
+  @Mock
+  private RemoteActionCache cache;
+
+  @Mock
+  private GrpcRemoteExecutor executor;
+
+  @Mock
+  private SpawnRunner localRunner;
+
+  @Before
+  public final void setUp() throws Exception {
+    MockitoAnnotations.initMocks(this);
+
+    FileSystem fs = new InMemoryFileSystem();
+    execRoot = fs.getPath("/exec/root");
+    FileSystemUtils.createDirectoryAndParents(execRoot);
+    fakeFileCache = new FakeActionInputFileCache(execRoot);
+
+    Path stdout = fs.getPath("/tmp/stdout");
+    Path stderr = fs.getPath("/tmp/stderr");
+    FileSystemUtils.createDirectoryAndParents(stdout.getParentDirectory());
+    FileSystemUtils.createDirectoryAndParents(stderr.getParentDirectory());
+    outErr = new FileOutErr(stdout, stderr);
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception {
+    // Test that if a spawn is marked "NO_CACHE" that it's neither fetched from a remote cache
+    // nor uploaded to a remote cache. It should be executed remotely, however.
+
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteAcceptCached = true;
+    options.remoteLocalFallback = false;
+    options.remoteUploadLocalResults = true;
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(execRoot, options, localRunner, cache, executor);
+
+    ExecuteResponse succeeded = ExecuteResponse.newBuilder().setResult(
+        ActionResult.newBuilder().setExitCode(0).build()).build();
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenReturn(succeeded);
+
+    Spawn spawn = new SimpleSpawn(
+        new FakeOwner("foo", "bar"),
+        /*arguments=*/ ImmutableList.of(),
+        /*environment=*/ ImmutableMap.of(),
+        NO_CACHE,
+        /*inputs=*/ ImmutableList.of(),
+        /*outputs=*/ ImmutableList.<ActionInput>of(),
+        ResourceSet.ZERO);
+
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    runner.exec(spawn, policy);
+
+    ArgumentCaptor<ExecuteRequest> requestCaptor = ArgumentCaptor.forClass(ExecuteRequest.class);
+    verify(executor).executeRemotely(requestCaptor.capture());
+    assertThat(requestCaptor.getValue().getSkipCacheLookup()).isTrue();
+
+    verify(cache, never())
+        .getCachedActionResult(any(ActionKey.class));
+    verify(cache, never()).upload(any(ActionKey.class), any(Path.class), any(Collection.class),
+          any(FileOutErr.class));
+    verifyZeroInteractions(localRunner);
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void nonCachableSpawnsShouldNotBeCached_local() throws Exception {
+    // Test that if a spawn is executed locally, due to the local fallback, that its result is not
+    // uploaded to the remote cache.
+
+    RemoteOptions options = Options.getDefaults(RemoteOptions.class);
+    options.remoteAcceptCached = true;
+    options.remoteLocalFallback = true;
+    options.remoteUploadLocalResults = true;
+
+    RemoteSpawnRunner runner =
+        new RemoteSpawnRunner(execRoot, options, localRunner, cache, null);
+
+    // Throw an IOException to trigger the local fallback.
+    when(executor.executeRemotely(any(ExecuteRequest.class))).thenThrow(IOException.class);
+
+    Spawn spawn = new SimpleSpawn(
+        new FakeOwner("foo", "bar"),
+        /*arguments=*/ ImmutableList.of(),
+        /*environment=*/ ImmutableMap.of(),
+        NO_CACHE,
+        /*inputs=*/ ImmutableList.of(),
+        /*outputs=*/ ImmutableList.<ActionInput>of(),
+        ResourceSet.ZERO);
+
+    SpawnExecutionPolicy policy = new FakeSpawnExecutionPolicy(spawn);
+
+    runner.exec(spawn, policy);
+
+    verify(localRunner).exec(spawn, policy);
+
+    verify(cache, never())
+        .getCachedActionResult(any(ActionKey.class));
+    verify(cache, never()).upload(any(ActionKey.class), any(Path.class), any(Collection.class),
+        any(FileOutErr.class));
+  }
+
+  // TODO(buchgr): Extract a common class to be used for testing.
+  class FakeSpawnExecutionPolicy implements SpawnExecutionPolicy {
+
+    private final ArtifactExpander artifactExpander =
+        (artifact, output) -> output.add(artifact);
+
+    private final Spawn spawn;
+
+    FakeSpawnExecutionPolicy(Spawn spawn) {
+      this.spawn = spawn;
+    }
+
+    @Override
+    public void lockOutputFiles() throws InterruptedException {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public ActionInputFileCache getActionInputFileCache() {
+      return fakeFileCache;
+    }
+
+    @Override
+    public ArtifactExpander getArtifactExpander() {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public long getTimeoutMillis() {
+      return 0;
+    }
+
+    @Override
+    public FileOutErr getFileOutErr() {
+      return outErr;
+    }
+
+    @Override
+    public SortedMap<PathFragment, ActionInput> getInputMapping() throws IOException {
+      return new SpawnInputExpander(/*strict*/ false)
+          .getInputMapping(spawn, artifactExpander, fakeFileCache, "workspace");
+    }
+
+    @Override
+    public void report(ProgressStatus state) {
+      assertThat(state).isEqualTo(ProgressStatus.EXECUTING);
+    }
+  }
+}