Don't issue GetCapabilities calls if the requirement is NONE

Because the endpoint might not implement the `GetCapabilities` API, e.g. Remote Asset API. Fixes #20342. Closes #20355. PiperOrigin-RevId: 586642258 Change-Id: If9ca3a1909729ea5d9410b7198bcacb5d67781aa
bazelbuild · Nov 30, 2023 · d7adb9a · d7adb9a
1 parent d34dc2a
commit d7adb9a
Show file tree

Hide file tree

Showing 13 changed files with 76 additions and 18 deletions.
diff --git a/.../com/google/devtools/build/lib/remote/ChannelConnectionWithServerCapabilitiesFactory.java b/.../com/google/devtools/build/lib/remote/ChannelConnectionWithServerCapabilitiesFactory.java
@@ -28,15 +28,15 @@ public interface ChannelConnectionWithServerCapabilitiesFactory extends ChannelC
 
   /** A {@link ChannelConnection} that provides {@link ServerCapabilities}. */
   class ChannelConnectionWithServerCapabilities extends ChannelConnection {
-    private final ServerCapabilities serverCapabilities;
+    private final Single<ServerCapabilities> serverCapabilities;
 
     public ChannelConnectionWithServerCapabilities(
-        ManagedChannel channel, ServerCapabilities serverCapabilities) {
+        ManagedChannel channel, Single<ServerCapabilities> serverCapabilities) {
       super(channel);
       this.serverCapabilities = serverCapabilities;
     }
 
-    public ServerCapabilities getServerCapabilities() {
+    public Single<ServerCapabilities> getServerCapabilities() {
       return serverCapabilities;
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/GoogleChannelConnectionFactory.java b/src/main/java/com/google/devtools/build/lib/remote/GoogleChannelConnectionFactory.java
@@ -94,12 +94,21 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
     return Single.fromCallable(
             () -> channelFactory.newChannel(target, proxy, options, interceptors))
         .flatMap(
-            channel ->
-                RxFutures.toSingle(() -> getAndVerifyServerCapabilities(channel), directExecutor())
-                    .map(
-                        serverCapabilities ->
-                            new ChannelConnectionWithServerCapabilities(
-                                channel, serverCapabilities)));
+            channel -> {
+              var serverCapabilitiesSingle =
+                  RxFutures.toSingle(
+                      () -> getAndVerifyServerCapabilities(channel), directExecutor());
+
+              // Don't issue GetCapabilities calls if the requirement is NONE because the endpoint,
+              // e.g. Remote Asset API, might not implement the API. See #20342.
+              if (requirement == ServerCapabilitiesRequirement.NONE) {
+                return Single.just(
+                    new ChannelConnectionWithServerCapabilities(channel, serverCapabilitiesSingle));
+              }
+
+              return serverCapabilitiesSingle.map(
+                  sc -> new ChannelConnectionWithServerCapabilities(channel, Single.just(sc)));
+            });
   }
 
   private ListenableFuture<ServerCapabilities> getAndVerifyServerCapabilities(

diff --git a/src/main/java/com/google/devtools/build/lib/remote/ReferenceCountedChannel.java b/src/main/java/com/google/devtools/build/lib/remote/ReferenceCountedChannel.java
@@ -77,8 +77,7 @@ public ReferenceCountedChannel(
   public ServerCapabilities getServerCapabilities() throws IOException {
     try (var s = Profiler.instance().profile("getServerCapabilities")) {
       return blockingGet(
-          withChannelConnection(
-              channelConnection -> Single.just(channelConnection.getServerCapabilities())));
+          withChannelConnection(ChannelConnectionWithServerCapabilities::getServerCapabilities));
     } catch (ExecutionException e) {
       throw new IOException(e);
     } catch (InterruptedException e) {

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -1132,4 +1132,9 @@ static Credentials createCredentials(
 
     return credentials;
   }
+
+  @VisibleForTesting
+  MutableSupplier<Downloader> getRemoteDownloaderSupplier() {
+    return remoteDownloaderSupplier;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java
@@ -199,4 +199,9 @@ private OutputStream newOutputStream(Path destination, Optional<Checksum> checks
     }
     return out;
   }
+
+  @VisibleForTesting
+  public ReferenceCountedChannel getChannel() {
+    return channel;
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -104,6 +104,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/common:bulk_transfer_exception",
         "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",
         "//src/main/java/com/google/devtools/build/lib/remote/disk",
+        "//src/main/java/com/google/devtools/build/lib/remote/downloader",
         "//src/main/java/com/google/devtools/build/lib/remote/http",
         "//src/main/java/com/google/devtools/build/lib/remote/merkletree",
         "//src/main/java/com/google/devtools/build/lib/remote/options",

diff --git a/...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/...t/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java
@@ -131,7 +131,7 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
             return Single.just(
                 new ChannelConnectionWithServerCapabilities(
                     InProcessChannelBuilder.forName(serverName).build(),
-                    ServerCapabilities.getDefaultInstance()));
+                    Single.just(ServerCapabilities.getDefaultInstance())));
           }
 
           @Override

diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java
@@ -127,7 +127,7 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                 return Single.just(
                     new ChannelConnectionWithServerCapabilities(
                         InProcessChannelBuilder.forName(serverName).build(),
-                        ServerCapabilities.getDefaultInstance()));
+                        Single.just(ServerCapabilities.getDefaultInstance())));
               }
 
               @Override
@@ -1069,7 +1069,7 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                         InProcessChannelBuilder.forName(serverName)
                             .intercept(MetadataUtils.newAttachHeadersInterceptor(metadata))
                             .build(),
-                        ServerCapabilities.getDefaultInstance()));
+                        Single.just(ServerCapabilities.getDefaultInstance())));
               }
 
               @Override

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
@@ -195,7 +195,7 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                         .build();
                 return Single.just(
                     new ChannelConnectionWithServerCapabilities(
-                        ch, ServerCapabilities.getDefaultInstance()));
+                        ch, Single.just(ServerCapabilities.getDefaultInstance())));
               }
 
               @Override

diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTestBase.java
@@ -117,7 +117,8 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                         .setExecutionCapabilities(
                             ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
                         .build();
-                return Single.just(new ChannelConnectionWithServerCapabilities(ch, caps));
+                return Single.just(
+                    new ChannelConnectionWithServerCapabilities(ch, Single.just(caps)));
               }
 
               @Override

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java
@@ -43,6 +43,7 @@
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.pkgcache.PackageOptions;
 import com.google.devtools.build.lib.remote.circuitbreaker.FailureCircuitBreaker;
+import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions;
@@ -221,6 +222,42 @@ public void initialize() {
     remoteOptions = Options.getDefaults(RemoteOptions.class);
   }
 
+  @Test
+  public void testVerifyCapabilities_none() throws Exception {
+    // Test that Bazel doesn't issue GetCapabilities calls if the requirement is NONE.
+    // Regression test for https://github.com/bazelbuild/bazel/issues/20342.
+    CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS);
+    Server executionServer =
+        createFakeServer(EXECUTION_SERVER_NAME, executionServerCapabilitiesImpl);
+    executionServer.start();
+
+    CapabilitiesImpl cacheCapabilitiesImpl = new CapabilitiesImpl(CACHE_ONLY_CAPS);
+    Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheCapabilitiesImpl);
+    cacheServer.start();
+
+    try {
+      remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME;
+      remoteOptions.remoteDownloader = CACHE_SERVER_NAME;
+
+      beforeCommand();
+
+      // Wait for the channel to be connected.
+      var downloader = (GrpcRemoteDownloader) remoteModule.getRemoteDownloaderSupplier().get();
+      var unused = downloader.getChannel().withChannelBlocking(ch -> new Object());
+
+      // Remote downloader uses Remote Asset API, and Bazel doesn't have any capability requirement
+      // on the endpoint. Expecting the request count is 0.
+      assertThat(cacheCapabilitiesImpl.getRequestCount()).isEqualTo(0);
+      assertCircuitBreakerInstance();
+    } finally {
+      executionServer.shutdownNow();
+      cacheServer.shutdownNow();
+
+      executionServer.awaitTermination();
+      cacheServer.awaitTermination();
+    }
+  }
+
   @Test
   public void testVerifyCapabilities_executionAndCacheForSingleEndpoint() throws Exception {
     CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS);

diff --git a/...ava/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/...ava/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
@@ -313,7 +313,8 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                         .setExecutionCapabilities(
                             ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
                         .build();
-                return Single.just(new ChannelConnectionWithServerCapabilities(ch, caps));
+                return Single.just(
+                    new ChannelConnectionWithServerCapabilities(ch, Single.just(caps)));
               }
 
               @Override

diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java
@@ -145,7 +145,7 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
                     InProcessChannelBuilder.forName(fakeServerName).directExecutor().build();
                 return Single.just(
                     new ChannelConnectionWithServerCapabilities(
-                        ch, ServerCapabilities.getDefaultInstance()));
+                        ch, Single.just(ServerCapabilities.getDefaultInstance())));
               }
 
               @Override