[7.1.0] Distinguish the disk and remote caches in the action progress…

… status. Previously, they were both displayed as `remote-cache`; there's now a separate `disk-cache` form. If a combined cache is used, one or both forms will appear, depending on which caches were looked up. As a result, the progress status reporting is moved to the individual cache implementations. While this is kind of unfortunate from an architectural standpoint, it's likely the best we can do until we recast cache lookups as spawn strategies (see bazelbuild#19904). Closes bazelbuild#20935. PiperOrigin-RevId: 601748051 Change-Id: I03710219973c95d4fca999d931b3513f6d240d94
tjgq · Jan 26, 2024 · e44dc50 · e44dc50
1 parent c6c7693
commit e44dc50
Show file tree

Hide file tree

Showing 16 changed files with 297 additions and 57 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java
@@ -48,6 +48,7 @@
 import com.google.common.util.concurrent.SettableFuture;
 import com.google.devtools.build.lib.authandtls.CallCredentialsProvider;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
 import com.google.devtools.build.lib.remote.RemoteRetrier.ProgressiveBackoff;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.MissingDigestsFinder;
@@ -81,6 +82,9 @@
 public class GrpcCacheClient implements RemoteCacheClient, MissingDigestsFinder {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
+  private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT =
+      SpawnCheckingCacheEvent.create("remote-cache");
+
   private final CallCredentialsProvider callCredentialsProvider;
   private final ReferenceCountedChannel channel;
   private final RemoteOptions options;
@@ -274,6 +278,10 @@ public ListenableFuture<String> getAuthority() {
   @Override
   public ListenableFuture<CachedActionResult> downloadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
+    if (context.getSpawnExecutionContext() != null) {
+      context.getSpawnExecutionContext().report(SPAWN_CHECKING_CACHE_EVENT);
+    }
+
     GetActionResultRequest request =
         GetActionResultRequest.newBuilder()
             .setInstanceName(options.remoteInstanceName)

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -605,7 +605,7 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
               buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner());
       RemoteActionExecutionContext remoteActionExecutionContext =
           RemoteActionExecutionContext.create(
-              spawn, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn));
+              spawn, context, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn));
 
       return new RemoteAction(
           spawn,

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
@@ -31,7 +31,6 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.exec.SpawnCache;
-import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -51,9 +50,6 @@
 @ThreadSafe // If the RemoteActionCache implementation is thread-safe.
 final class RemoteSpawnCache implements SpawnCache {
 
-  private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT =
-      SpawnCheckingCacheEvent.create("remote-cache");
-
   private final Path execRoot;
   private final RemoteOptions options;
   private final RemoteExecutionService remoteExecutionService;
@@ -101,7 +97,6 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
 
     Profiler prof = Profiler.instance();
     if (shouldAcceptCachedResult) {
-      context.report(SPAWN_CHECKING_CACHE_EVENT);
       // Metadata will be available in context.current() until we detach.
       // This is done via a thread-local variable.
       try {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java
@@ -16,6 +16,8 @@
 import build.bazel.remote.execution.v2.RequestMetadata;
 import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
 import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.exec.SpawnRunner;
+import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import javax.annotation.Nullable;
 
 /** A context that provide remote execution related information for executing an action remotely. */
@@ -61,27 +63,35 @@ public CachePolicy addRemoteCache() {
   }
 
   @Nullable private final Spawn spawn;
+  @Nullable private final SpawnExecutionContext spawnExecutionContext;
   private final RequestMetadata requestMetadata;
   private final NetworkTime networkTime;
   private final CachePolicy writeCachePolicy;
   private final CachePolicy readCachePolicy;
 
   private RemoteActionExecutionContext(
-      @Nullable Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
-    this.spawn = spawn;
-    this.requestMetadata = requestMetadata;
-    this.networkTime = networkTime;
-    this.writeCachePolicy = CachePolicy.ANY_CACHE;
-    this.readCachePolicy = CachePolicy.ANY_CACHE;
+      @Nullable Spawn spawn,
+      @Nullable SpawnRunner.SpawnExecutionContext spawnExecutionContext,
+      RequestMetadata requestMetadata,
+      NetworkTime networkTime) {
+    this(
+        spawn,
+        spawnExecutionContext,
+        requestMetadata,
+        networkTime,
+        CachePolicy.ANY_CACHE,
+        CachePolicy.ANY_CACHE);
   }
 
   private RemoteActionExecutionContext(
       @Nullable Spawn spawn,
+      @Nullable SpawnExecutionContext spawnExecutionContext,
       RequestMetadata requestMetadata,
       NetworkTime networkTime,
       CachePolicy writeCachePolicy,
       CachePolicy readCachePolicy) {
     this.spawn = spawn;
+    this.spawnExecutionContext = spawnExecutionContext;
     this.requestMetadata = requestMetadata;
     this.networkTime = networkTime;
     this.writeCachePolicy = writeCachePolicy;
@@ -90,20 +100,42 @@ private RemoteActionExecutionContext(
 
   public RemoteActionExecutionContext withWriteCachePolicy(CachePolicy writeCachePolicy) {
     return new RemoteActionExecutionContext(
-        spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy);
+        spawn,
+        spawnExecutionContext,
+        requestMetadata,
+        networkTime,
+        writeCachePolicy,
+        readCachePolicy);
   }
 
   public RemoteActionExecutionContext withReadCachePolicy(CachePolicy readCachePolicy) {
     return new RemoteActionExecutionContext(
-        spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy);
+        spawn,
+        spawnExecutionContext,
+        requestMetadata,
+        networkTime,
+        writeCachePolicy,
+        readCachePolicy);
   }
 
-  /** Returns the {@link Spawn} of the action being executed or {@code null}. */
+  /**
+   * Returns the {@link Spawn} of the {@link RemoteAction} being executed, or {@code null} if it has
+   * no associated {@link Spawn}.
+   */
   @Nullable
   public Spawn getSpawn() {
     return spawn;
   }
 
+  /**
+   * Returns the {@link SpawnExecutionContext} of the {@link RemoteAction} being executed, or {@code
+   * null} if it has no associated {@link Spawn}.
+   */
+  @Nullable
+  public SpawnExecutionContext getSpawnExecutionContext() {
+    return spawnExecutionContext;
+  }
+
   /** Returns the {@link RequestMetadata} for the action being executed. */
   public RequestMetadata getRequestMetadata() {
     return requestMetadata;
@@ -137,24 +169,32 @@ public CachePolicy getReadCachePolicy() {
 
   /** Creates a {@link RemoteActionExecutionContext} with given {@link RequestMetadata}. */
   public static RemoteActionExecutionContext create(RequestMetadata metadata) {
-    return new RemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime());
+    return new RemoteActionExecutionContext(
+        /* spawn= */ null, /* spawnExecutionContext= */ null, metadata, new NetworkTime());
   }
 
   /**
    * Creates a {@link RemoteActionExecutionContext} with given {@link Spawn} and {@link
    * RequestMetadata}.
    */
   public static RemoteActionExecutionContext create(
-      @Nullable Spawn spawn, RequestMetadata metadata) {
-    return new RemoteActionExecutionContext(spawn, metadata, new NetworkTime());
+      Spawn spawn, SpawnExecutionContext spawnExecutionContext, RequestMetadata metadata) {
+    return new RemoteActionExecutionContext(
+        spawn, spawnExecutionContext, metadata, new NetworkTime());
   }
 
   public static RemoteActionExecutionContext create(
-      @Nullable Spawn spawn,
+      Spawn spawn,
+      SpawnExecutionContext spawnExecutionContext,
       RequestMetadata requestMetadata,
       CachePolicy writeCachePolicy,
       CachePolicy readCachePolicy) {
     return new RemoteActionExecutionContext(
-        spawn, requestMetadata, new NetworkTime(), writeCachePolicy, readCachePolicy);
+        spawn,
+        spawnExecutionContext,
+        requestMetadata,
+        new NetworkTime(),
+        writeCachePolicy,
+        readCachePolicy);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD b/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD
@@ -15,6 +15,7 @@ java_library(
     name = "disk",
     srcs = glob(["*.java"]),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
         "//src/main/java/com/google/devtools/build/lib/remote:store",
         "//src/main/java/com/google/devtools/build/lib/remote/common",
         "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java
@@ -30,6 +30,7 @@
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.MoreExecutors;
+import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
 import com.google.devtools.build.lib.remote.Store;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
@@ -60,6 +61,10 @@
  * safe to trim the cache at the same time a Bazel process is accessing it.
  */
 public class DiskCacheClient implements RemoteCacheClient {
+
+  private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT =
+      SpawnCheckingCacheEvent.create("disk-cache");
+
   private final Path root;
   private final boolean verifyDownloads;
   private final DigestUtil digestUtil;
@@ -222,6 +227,10 @@ public ListenableFuture<String> getAuthority() {
   @Override
   public ListenableFuture<CachedActionResult> downloadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
+    if (context.getSpawnExecutionContext() != null) {
+      context.getSpawnExecutionContext().report(SPAWN_CHECKING_CACHE_EVENT);
+    }
+
     return Futures.transformAsync(
         Utils.downloadAsActionResult(actionKey, (digest, out) -> download(digest, out, Store.AC)),
         actionResult -> {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/BUILD b/src/main/java/com/google/devtools/build/lib/remote/http/BUILD
@@ -21,6 +21,7 @@ java_library(
     deps = [
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
+        "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
         "//src/main/java/com/google/devtools/build/lib/remote:Retrier",
         "//src/main/java/com/google/devtools/build/lib/remote/common",
         "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java
@@ -28,6 +28,7 @@
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.SettableFuture;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
+import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
 import com.google.devtools.build.lib.remote.RemoteRetrier;
 import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
@@ -122,6 +123,9 @@
 public final class HttpCacheClient implements RemoteCacheClient {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
+  private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT =
+      SpawnCheckingCacheEvent.create("remote-cache");
+
   public static final String AC_PREFIX = "ac/";
   public static final String CAS_PREFIX = "cas/";
   private static final Pattern INVALID_TOKEN_ERROR =
@@ -617,6 +621,10 @@ public ListenableFuture<String> getAuthority() {
   @Override
   public ListenableFuture<CachedActionResult> downloadActionResult(
       RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
+    if (context.getSpawnExecutionContext() != null) {
+      context.getSpawnExecutionContext().report(SPAWN_CHECKING_CACHE_EVENT);
+    }
+
     return Futures.transform(
         retrier.executeAsync(
             () ->

diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -11,6 +11,7 @@ filegroup(
     testonly = 0,
     srcs = glob(["**"]) + [
         "//src/test/java/com/google/devtools/build/lib/remote/circuitbreaker:srcs",
+        "//src/test/java/com/google/devtools/build/lib/remote/disk:srcs",
         "//src/test/java/com/google/devtools/build/lib/remote/downloader:srcs",
         "//src/test/java/com/google/devtools/build/lib/remote/grpc:srcs",
         "//src/test/java/com/google/devtools/build/lib/remote/http:srcs",

diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
@@ -22,7 +22,9 @@
 import static org.mockito.AdditionalAnswers.answerVoid;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
 
 import build.bazel.remote.execution.v2.Action;
 import build.bazel.remote.execution.v2.ActionCacheGrpc.ActionCacheImplBase;
@@ -58,13 +60,16 @@
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
+import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.authandtls.CallCredentialsProvider;
 import com.google.devtools.build.lib.authandtls.GoogleAuthUtils;
 import com.google.devtools.build.lib.clock.JavaClock;
 import com.google.devtools.build.lib.events.NullEventHandler;
+import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent;
+import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
 import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff;
 import com.google.devtools.build.lib.remote.Retrier.Backoff;
 import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
@@ -257,7 +262,9 @@ public final void setUp() throws Exception {
     RequestMetadata metadata =
         TracingMetadataUtils.buildMetadata(
             "none", "none", Digest.getDefaultInstance().getHash(), null);
-    context = RemoteActionExecutionContext.create(metadata);
+    context =
+        RemoteActionExecutionContext.create(
+            mock(Spawn.class), mock(SpawnExecutionContext.class), metadata);
     retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1));
   }
 
@@ -272,6 +279,30 @@ public void tearDown() throws Exception {
     fakeServer.awaitTermination();
   }
 
+  @Test
+  public void testSpawnCheckingCacheEvent() throws Exception {
+    GrpcCacheClient client = newClient();
+
+    serviceRegistry.addService(
+        new ActionCacheImplBase() {
+          @Override
+          public void getActionResult(
+              GetActionResultRequest request, StreamObserver<ActionResult> responseObserver) {
+            responseObserver.onError(Status.NOT_FOUND.asRuntimeException());
+          }
+        });
+
+    var unused =
+        getFromFuture(
+            client.downloadActionResult(
+                context,
+                DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")),
+                /* inlineOutErr= */ false));
+
+    verify(context.getSpawnExecutionContext())
+        .report(SpawnCheckingCacheEvent.create("remote-cache"));
+  }
+
   @Test
   public void testVirtualActionInputSupport() throws Exception {
     RemoteOptions options = Options.getDefaults(RemoteOptions.class);