From 0248fcb50971151944b7d0ae1fdb9721ab3370c1 Mon Sep 17 00:00:00 2001 From: George Gensure Date: Fri, 23 Jun 2023 19:11:03 -0400 Subject: [PATCH] Operation stream termination is not an error According to the GrpcRemoteExecutor when it occurs after a !done operation response. Remove the error from the ExperimentalRemoteGrpcExecutor and reinforce both with tests. Update the FakeExecutionService to generate compatible error responses that appear in the ExecuteResponse, rather than the operation error field, per the REAPI spec. Made required adjustments to ExGRE Test invocations to avoid the ExecutionStatusException interpretation of DEADLINE_EXCEEDED -> FAILED_PRECONDITION in ExecuteResponse. --- .../ExperimentalGrpcRemoteExecutor.java | 14 +- .../ExperimentalGrpcRemoteExecutorTest.java | 294 +--------------- .../lib/remote/FakeExecutionService.java | 16 +- .../lib/remote/GrpcRemoteExecutorTest.java | 120 +++++++ .../remote/GrpcRemoteExecutorTestBase.java | 321 ++++++++++++++++++ 5 files changed, 475 insertions(+), 290 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTest.java create mode 100644 src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTestBase.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutor.java b/src/main/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutor.java index 428da883fba69d..da9f4a4b371c16 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutor.java @@ -146,10 +146,12 @@ ExecuteResponse start() throws IOException, InterruptedException { // retrying when received a unauthenticated error, and propagate to refreshIfUnauthenticated // which will then call retrier again. It will reset the retry time counter so we could // retry more than --remote_retry times which is not expected. - response = - retrier.execute( - () -> Utils.refreshIfUnauthenticated(this::execute, callCredentialsProvider), - executeBackoff); + if (lastOperation == null) { + response = + retrier.execute( + () -> Utils.refreshIfUnauthenticated(this::execute, callCredentialsProvider), + executeBackoff); + } // If no response from Execute(), use WaitExecution() in a "loop" which is implemented // inside the retry block. @@ -258,8 +260,8 @@ ExecuteResponse handleOperationStream(Iterator operationStream) throw } } - // The operation completed successfully but without a result. - throw new IOException("Remote server error: execution terminated with no result"); + // The operation stream completed successfully but without a result. + return null; } finally { close(operationStream); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutorTest.java index d42021a4027141..135d031c00416b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ExperimentalGrpcRemoteExecutorTest.java @@ -17,258 +17,60 @@ import static java.util.concurrent.TimeUnit.SECONDS; import static org.junit.Assert.assertThrows; -import build.bazel.remote.execution.v2.ActionResult; -import build.bazel.remote.execution.v2.Digest; -import build.bazel.remote.execution.v2.ExecuteRequest; import build.bazel.remote.execution.v2.ExecuteResponse; -import build.bazel.remote.execution.v2.ExecutionCapabilities; -import build.bazel.remote.execution.v2.OutputFile; -import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.ServerCapabilities; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; import com.google.devtools.build.lib.remote.common.OperationObserver; -import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; -import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory; -import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.util.TestUtils; -import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; -import com.google.devtools.common.options.Options; -import com.google.longrunning.Operation; import com.google.rpc.Code; -import io.grpc.ManagedChannel; -import io.grpc.Server; import io.grpc.Status; -import io.grpc.inprocess.InProcessChannelBuilder; -import io.grpc.inprocess.InProcessServerBuilder; -import io.reactivex.rxjava3.core.Single; import java.io.IOException; -import java.util.ArrayList; -import java.util.List; import java.util.concurrent.Executors; -import org.junit.After; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; /** Tests for {@link ExperimentalGrpcRemoteExecutor}. */ @RunWith(JUnit4.class) -public class ExperimentalGrpcRemoteExecutorTest { - - private RemoteActionExecutionContext context; - private FakeExecutionService executionService; - private RemoteOptions remoteOptions; - private Server fakeServer; +public class ExperimentalGrpcRemoteExecutorTest extends GrpcRemoteExecutorTestBase { private ListeningScheduledExecutorService retryService; - ExperimentalGrpcRemoteExecutor executor; - - private static final int MAX_RETRY_ATTEMPTS = 5; - - private static final OutputFile DUMMY_OUTPUT = - OutputFile.newBuilder() - .setPath("dummy.txt") - .setDigest( - Digest.newBuilder() - .setHash("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") - .setSizeBytes(0) - .build()) - .build(); - - private static final ExecuteRequest DUMMY_REQUEST = - ExecuteRequest.newBuilder() - .setInstanceName("dummy") - .setActionDigest( - Digest.newBuilder() - .setHash("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") - .setSizeBytes(0) - .build()) - .build(); - - private static final ExecuteResponse DUMMY_RESPONSE = - ExecuteResponse.newBuilder() - .setResult(ActionResult.newBuilder().addOutputFiles(DUMMY_OUTPUT).build()) - .build(); - - @Before - public final void setUp() throws Exception { - context = RemoteActionExecutionContext.create(RequestMetadata.getDefaultInstance()); - - executionService = new FakeExecutionService(); - - String fakeServerName = "fake server for " + getClass(); - // Use a mutable service registry for later registering the service impl for each test case. - fakeServer = - InProcessServerBuilder.forName(fakeServerName) - .addService(executionService) - .directExecutor() - .build() - .start(); - remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteMaxRetryAttempts = MAX_RETRY_ATTEMPTS; - - retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + @Override + protected RemoteExecutionClient createExecutionService(ServerCapabilities caps, ReferenceCountedChannel channel) throws Exception { RemoteRetrier retrier = TestUtils.newRemoteRetrier( () -> new ExponentialBackoff(remoteOptions), RemoteRetrier.RETRIABLE_GRPC_ERRORS, retryService); - ReferenceCountedChannel channel = - new ReferenceCountedChannel( - new ChannelConnectionFactory() { - @Override - public Single create() { - ManagedChannel ch = - InProcessChannelBuilder.forName(fakeServerName) - .intercept(TracingMetadataUtils.newExecHeadersInterceptor(remoteOptions)) - .directExecutor() - .build(); - return Single.just(new ChannelConnection(ch)); - } - - @Override - public int maxConcurrency() { - return 100; - } - }); - ServerCapabilities caps = - ServerCapabilities.newBuilder() - .setExecutionCapabilities( - ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) - .build(); + return new ExperimentalGrpcRemoteExecutor( + caps, remoteOptions, channel, CallCredentialsProvider.NO_CREDENTIALS, retrier); + } - executor = - new ExperimentalGrpcRemoteExecutor( - caps, remoteOptions, channel, CallCredentialsProvider.NO_CREDENTIALS, retrier); + @Override + public void setUp() throws Exception { + retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + super.setUp(); } - @After + @Override public void tearDown() throws Exception { retryService.shutdownNow(); retryService.awaitTermination( com.google.devtools.build.lib.testutil.TestUtils.WAIT_TIMEOUT_SECONDS, SECONDS); - - fakeServer.shutdownNow(); - fakeServer.awaitTermination(); - - executor.close(); - } - - @Test - public void executeRemotely_smoke() throws Exception { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenAck().thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(response).isEqualTo(DUMMY_RESPONSE); - assertThat(executionService.getExecTimes()).isEqualTo(1); - } - - @Test - public void executeRemotely_errorInOperation_retryExecute() throws Exception { - executionService.whenExecute(DUMMY_REQUEST).thenError(new RuntimeException("Unavailable")); - executionService.whenExecute(DUMMY_REQUEST).thenError(Code.UNAVAILABLE); - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(executionService.getExecTimes()).isEqualTo(3); - assertThat(response).isEqualTo(DUMMY_RESPONSE); - } - - @Test - public void executeRemotely_errorInResponse_retryExecute() throws Exception { - executionService - .whenExecute(DUMMY_REQUEST) - .thenDone( - ExecuteResponse.newBuilder() - .setStatus(com.google.rpc.Status.newBuilder().setCode(Code.UNAVAILABLE_VALUE)) - .build()); - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(executionService.getExecTimes()).isEqualTo(2); - assertThat(response).isEqualTo(DUMMY_RESPONSE); - } - - @Test - public void executeRemotely_unretriableErrorInResponse_reportError() { - executionService - .whenExecute(DUMMY_REQUEST) - .thenDone( - ExecuteResponse.newBuilder() - .setStatus(com.google.rpc.Status.newBuilder().setCode(Code.INVALID_ARGUMENT_VALUE)) - .build()); - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); - - IOException e = - assertThrows( - IOException.class, - () -> { - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - }); - - assertThat(e).hasMessageThat().contains("INVALID_ARGUMENT"); - assertThat(executionService.getExecTimes()).isEqualTo(1); - } - - @Test - public void executeRemotely_retryExecuteAndFail() { - for (int i = 0; i <= MAX_RETRY_ATTEMPTS * 2; ++i) { - executionService.whenExecute(DUMMY_REQUEST).thenError(Code.UNAVAILABLE); - } - - IOException exception = - assertThrows( - IOException.class, - () -> { - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - }); - - assertThat(executionService.getExecTimes()).isEqualTo(MAX_RETRY_ATTEMPTS + 1); - assertThat(exception).hasMessageThat().contains("UNAVAILABLE"); - } - - @Test - public void executeRemotely_executeAndWait() throws Exception { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); - executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(executionService.getExecTimes()).isEqualTo(1); - assertThat(executionService.getWaitTimes()).isEqualTo(1); - assertThat(response).isEqualTo(DUMMY_RESPONSE); - } - - @Test - public void executeRemotely_executeAndRetryWait() throws Exception { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); - executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(executionService.getExecTimes()).isEqualTo(1); - assertThat(executionService.getWaitTimes()).isEqualTo(1); - assertThat(response).isEqualTo(DUMMY_RESPONSE); + super.tearDown(); } @Test public void executeRemotely_executeAndRetryWait_forever() throws Exception { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); int errorTimes = MAX_RETRY_ATTEMPTS * 2; for (int i = 0; i < errorTimes; ++i) { - executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenError(Code.DEADLINE_EXCEEDED); + executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenError(Status.DEADLINE_EXCEEDED.asRuntimeException()); } executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); @@ -282,7 +84,7 @@ public void executeRemotely_executeAndRetryWait_forever() throws Exception { @Test public void executeRemotely_executeAndRetryWait_failForConsecutiveErrors() { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); for (int i = 0; i < MAX_RETRY_ATTEMPTS * 2; ++i) { executionService.whenWaitExecution(DUMMY_REQUEST).thenError(Code.UNAVAILABLE); } @@ -340,23 +142,10 @@ public void executeRemotely_responseWithoutResult_shouldNotCrash() { assertThat(executionService.getExecTimes()).isEqualTo(1); } - @Test - public void executeRemotely_retryExecuteWhenUnauthenticated() - throws IOException, InterruptedException { - executionService.whenExecute(DUMMY_REQUEST).thenError(Code.UNAUTHENTICATED); - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(executionService.getExecTimes()).isEqualTo(2); - assertThat(response).isEqualTo(DUMMY_RESPONSE); - } - @Test public void executeRemotely_retryWaitExecutionWhenUnauthenticated() throws IOException, InterruptedException { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenError(Code.UNAUTHENTICATED); executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); @@ -367,53 +156,4 @@ public void executeRemotely_retryWaitExecutionWhenUnauthenticated() assertThat(executionService.getWaitTimes()).isEqualTo(2); assertThat(response).isEqualTo(DUMMY_RESPONSE); } - - @Test - public void executeRemotely_retryExecuteIfNotFound() throws IOException, InterruptedException { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); - executionService.whenWaitExecution(DUMMY_REQUEST).thenError(Code.NOT_FOUND); - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); - executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); - - ExecuteResponse response = - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - - assertThat(executionService.getExecTimes()).isEqualTo(2); - assertThat(executionService.getWaitTimes()).isEqualTo(2); - assertThat(response).isEqualTo(DUMMY_RESPONSE); - } - - @Test - public void executeRemotely_notFoundLoop_reportError() { - for (int i = 0; i <= MAX_RETRY_ATTEMPTS * 2; ++i) { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); - executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenError(Code.NOT_FOUND); - } - - IOException e = - assertThrows( - IOException.class, - () -> { - executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); - }); - - assertThat(e).hasCauseThat().isInstanceOf(ExecutionStatusException.class); - ExecutionStatusException executionStatusException = (ExecutionStatusException) e.getCause(); - assertThat(executionStatusException.getStatus().getCode()).isEqualTo(Status.Code.NOT_FOUND); - assertThat(executionService.getExecTimes()).isEqualTo(MAX_RETRY_ATTEMPTS + 1); - assertThat(executionService.getWaitTimes()).isEqualTo(MAX_RETRY_ATTEMPTS + 1); - } - - @Test - public void executeRemotely_notifyObserver() throws IOException, InterruptedException { - executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); - - List notified = new ArrayList<>(); - executor.executeRemotely(context, DUMMY_REQUEST, notified::add); - - assertThat(notified) - .containsExactly( - FakeExecutionService.ackOperation(DUMMY_REQUEST), - FakeExecutionService.doneOperation(DUMMY_REQUEST, DUMMY_RESPONSE)); - } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/FakeExecutionService.java b/src/test/java/com/google/devtools/build/lib/remote/FakeExecutionService.java index 4c9d93989449dc..5eaebc89d390be 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/FakeExecutionService.java +++ b/src/test/java/com/google/devtools/build/lib/remote/FakeExecutionService.java @@ -115,12 +115,14 @@ public void thenDone(ExecuteResponse response) { } public void thenError(Code code) { - Operation operation = - Operation.newBuilder() - .setName(getResourceName(request)) - .setDone(true) - .setError(Status.newBuilder().setCode(code.getNumber())) - .build(); + // From REAPI Spec: + // > Errors discovered during creation of the `Operation` will be reported + // > as gRPC Status errors, while errors that occurred while running the + // > action will be reported in the `status` field of the `ExecuteResponse`. The + // > server MUST NOT set the `error` field of the `Operation` proto. + Operation operation = doneOperation(request, ExecuteResponse.newBuilder() + .setStatus(Status.newBuilder().setCode(code.getNumber())) + .build()); operations.add(() -> operation); finish(); } @@ -133,7 +135,7 @@ public void thenError(RuntimeException e) { finish(); } - private void finish() { + public void finish() { String name = getResourceName(request); provider.append(name, ImmutableList.copyOf(operations)); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTest.java new file mode 100644 index 00000000000000..a895f327c3d111 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTest.java @@ -0,0 +1,120 @@ +// Copyright 2023 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 java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.Assert.assertThrows; + +import build.bazel.remote.execution.v2.ExecuteResponse; +import build.bazel.remote.execution.v2.ServerCapabilities; +import com.google.common.util.concurrent.ListeningScheduledExecutorService; +import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; +import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; +import com.google.devtools.build.lib.remote.common.OperationObserver; +import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; +import com.google.devtools.build.lib.remote.util.TestUtils; +import com.google.rpc.Code; +import java.io.IOException; +import java.util.concurrent.Executors; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link GrpcRemoteExecutor}. */ +@RunWith(JUnit4.class) +public class GrpcRemoteExecutorTest extends GrpcRemoteExecutorTestBase { + private ListeningScheduledExecutorService retryService; + + @Override + public void setUp() throws Exception { + retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + super.setUp(); + } + + @Override + protected RemoteExecutionClient createExecutionService(ServerCapabilities caps, ReferenceCountedChannel channel) throws Exception { + RemoteRetrier retrier = + TestUtils.newRemoteRetrier( + () -> new ExponentialBackoff(remoteOptions), + RemoteRetrier.RETRIABLE_GRPC_EXEC_ERRORS, + retryService); + + return new GrpcRemoteExecutor( + caps, channel, CallCredentialsProvider.NO_CREDENTIALS, retrier); + } + + @Override + public void tearDown() throws Exception { + retryService.shutdownNow(); + retryService.awaitTermination( + com.google.devtools.build.lib.testutil.TestUtils.WAIT_TIMEOUT_SECONDS, SECONDS); + super.tearDown(); + } + + @Test + public void executeRemotely_operationWithoutResult_crashes() { + executionService.whenExecute(DUMMY_REQUEST).thenDone(); + + assertThrows( + IllegalStateException.class, + () -> { + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + }); + // Shouldn't retry in this case + assertThat(executionService.getExecTimes()).isEqualTo(1); + } + + @Test + public void executeRemotely_responseWithoutResult_crashes() { + executionService.whenExecute(DUMMY_REQUEST).thenDone(ExecuteResponse.getDefaultInstance()); + + assertThrows( + IllegalStateException.class, + () -> { + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + }); + + assertThat(executionService.getExecTimes()).isEqualTo(1); + } + + @Test + public void executeRemotely_retryWaitExecutionWhenUnauthenticated() + throws IOException, InterruptedException { + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenError(Code.UNAUTHENTICATED); + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(2); + assertThat(executionService.getWaitTimes()).isEqualTo(1); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_retryExecuteOnNoResult() throws IOException, InterruptedException { + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenError(Code.UNAVAILABLE); + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(2); + assertThat(executionService.getWaitTimes()).isEqualTo(0); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } +} 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 new file mode 100644 index 00000000000000..2113ba632a48a3 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutorTestBase.java @@ -0,0 +1,321 @@ +// Copyright 2023 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.junit.Assert.assertThrows; + +import build.bazel.remote.execution.v2.ActionResult; +import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.ExecuteRequest; +import build.bazel.remote.execution.v2.ExecuteResponse; +import build.bazel.remote.execution.v2.ExecutionCapabilities; +import build.bazel.remote.execution.v2.OutputFile; +import build.bazel.remote.execution.v2.RequestMetadata; +import build.bazel.remote.execution.v2.ServerCapabilities; +import com.google.devtools.build.lib.remote.common.OperationObserver; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; +import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.common.options.Options; +import com.google.longrunning.Operation; +import com.google.rpc.Code; +import io.grpc.ManagedChannel; +import io.grpc.Server; +import io.grpc.Status; +import io.grpc.inprocess.InProcessChannelBuilder; +import io.grpc.inprocess.InProcessServerBuilder; +import io.reactivex.rxjava3.core.Single; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** Base test class for {@link RemoteExecutionClient} gRPC implementations. */ +public abstract class GrpcRemoteExecutorTestBase { + + protected RemoteActionExecutionContext context; + protected FakeExecutionService executionService; + protected RemoteOptions remoteOptions; + private Server fakeServer; + protected RemoteExecutionClient executor; + + protected static final int MAX_RETRY_ATTEMPTS = 5; + + private static final OutputFile DUMMY_OUTPUT = + OutputFile.newBuilder() + .setPath("dummy.txt") + .setDigest( + Digest.newBuilder() + .setHash("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") + .setSizeBytes(0) + .build()) + .build(); + + protected static final ExecuteRequest DUMMY_REQUEST = + ExecuteRequest.newBuilder() + .setInstanceName("dummy") + .setActionDigest( + Digest.newBuilder() + .setHash("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855") + .setSizeBytes(0) + .build()) + .build(); + + protected static final ExecuteResponse DUMMY_RESPONSE = + ExecuteResponse.newBuilder() + .setResult(ActionResult.newBuilder().addOutputFiles(DUMMY_OUTPUT).build()) + .build(); + + protected abstract RemoteExecutionClient createExecutionService(ServerCapabilities caps, ReferenceCountedChannel channel) throws Exception; + + @Before + public void setUp() throws Exception { + context = RemoteActionExecutionContext.create(RequestMetadata.getDefaultInstance()); + + executionService = new FakeExecutionService(); + + String fakeServerName = "fake server for " + getClass(); + // Use a mutable service registry for later registering the service impl for each test case. + fakeServer = + InProcessServerBuilder.forName(fakeServerName) + .addService(executionService) + .directExecutor() + .build() + .start(); + + remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.remoteMaxRetryAttempts = MAX_RETRY_ATTEMPTS; + + ReferenceCountedChannel channel = + new ReferenceCountedChannel( + new ChannelConnectionFactory() { + @Override + public Single create() { + ManagedChannel ch = + InProcessChannelBuilder.forName(fakeServerName) + .intercept(TracingMetadataUtils.newExecHeadersInterceptor(remoteOptions)) + .directExecutor() + .build(); + return Single.just(new ChannelConnection(ch)); + } + + @Override + public int maxConcurrency() { + return 100; + } + }); + + ServerCapabilities caps = + ServerCapabilities.newBuilder() + .setExecutionCapabilities( + ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + + executor = createExecutionService(caps, channel); + } + + @After + public void tearDown() throws Exception { + fakeServer.shutdownNow(); + fakeServer.awaitTermination(); + + executor.close(); + } + + @Test + public void executeRemotely_smoke() throws Exception { + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(response).isEqualTo(DUMMY_RESPONSE); + assertThat(executionService.getExecTimes()).isEqualTo(1); + } + + @Test + public void executeRemotely_errorInOperation_retryExecute() throws Exception { + executionService.whenExecute(DUMMY_REQUEST).thenError(new RuntimeException("Unavailable")); + executionService.whenExecute(DUMMY_REQUEST).thenError(Code.UNAVAILABLE); + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(3); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_errorInResponse_retryExecute() throws Exception { + executionService + .whenExecute(DUMMY_REQUEST) + .thenDone( + ExecuteResponse.newBuilder() + .setStatus(com.google.rpc.Status.newBuilder().setCode(Code.UNAVAILABLE_VALUE)) + .build()); + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(2); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_unretriableErrorInResponse_reportError() { + executionService + .whenExecute(DUMMY_REQUEST) + .thenDone( + ExecuteResponse.newBuilder() + .setStatus(com.google.rpc.Status.newBuilder().setCode(Code.INVALID_ARGUMENT_VALUE)) + .build()); + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + IOException e = + assertThrows( + IOException.class, + () -> { + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + }); + + assertThat(e).hasMessageThat().contains("INVALID_ARGUMENT"); + assertThat(executionService.getExecTimes()).isEqualTo(1); + } + + @Test + public void executeRemotely_retryExecuteAndFail() { + for (int i = 0; i <= MAX_RETRY_ATTEMPTS * 2; ++i) { + executionService.whenExecute(DUMMY_REQUEST).thenError(Code.UNAVAILABLE); + } + + IOException exception = + assertThrows( + IOException.class, + () -> { + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + }); + + assertThat(executionService.getExecTimes()).isEqualTo(MAX_RETRY_ATTEMPTS + 1); + assertThat(exception).hasMessageThat().contains("UNAVAILABLE"); + } + + @Test + public void executeRemotely_executeAndWait() throws Exception { + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(1); + assertThat(executionService.getWaitTimes()).isEqualTo(1); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_executeAndRetryWait() throws Exception { + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(1); + assertThat(executionService.getWaitTimes()).isEqualTo(1); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_retryExecuteWhenUnauthenticated() + throws IOException, InterruptedException { + executionService.whenExecute(DUMMY_REQUEST).thenError(Code.UNAUTHENTICATED); + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(2); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_retryExecuteIfNotFound() throws IOException, InterruptedException { + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenError(Code.NOT_FOUND); + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(2); + assertThat(executionService.getWaitTimes()).isEqualTo(2); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_retryExecuteOnFinish() throws IOException, InterruptedException { + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + ExecuteResponse response = + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + + assertThat(executionService.getExecTimes()).isEqualTo(1); + assertThat(executionService.getWaitTimes()).isEqualTo(2); + assertThat(response).isEqualTo(DUMMY_RESPONSE); + } + + @Test + public void executeRemotely_notFoundLoop_reportError() { + for (int i = 0; i <= MAX_RETRY_ATTEMPTS; ++i) { + executionService.whenExecute(DUMMY_REQUEST).thenAck().finish(); + executionService.whenWaitExecution(DUMMY_REQUEST).thenAck().thenError(Code.NOT_FOUND); + } + + IOException e = + assertThrows( + IOException.class, + () -> { + executor.executeRemotely(context, DUMMY_REQUEST, OperationObserver.NO_OP); + }); + + assertThat(e).hasCauseThat().isInstanceOf(ExecutionStatusException.class); + ExecutionStatusException executionStatusException = (ExecutionStatusException) e.getCause(); + assertThat(executionStatusException.getStatus().getCode()).isEqualTo(Status.Code.NOT_FOUND); + assertThat(executionService.getExecTimes()).isEqualTo(MAX_RETRY_ATTEMPTS + 1); + assertThat(executionService.getWaitTimes()).isEqualTo(MAX_RETRY_ATTEMPTS + 1); + } + + @Test + public void executeRemotely_notifyObserver() throws IOException, InterruptedException { + executionService.whenExecute(DUMMY_REQUEST).thenAck().thenDone(DUMMY_RESPONSE); + + List notified = new ArrayList<>(); + executor.executeRemotely(context, DUMMY_REQUEST, notified::add); + + assertThat(notified) + .containsExactly( + FakeExecutionService.ackOperation(DUMMY_REQUEST), + FakeExecutionService.doneOperation(DUMMY_REQUEST, DUMMY_RESPONSE)); + } +}