From 852f2e874f49422fcef3c7710dd3e1fc03a96532 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 17 Sep 2024 17:08:00 +0200 Subject: [PATCH] Correctly propagate and abort the build for CredentialHelperException. --- .../GoogleChannelConnectionFactory.java | 30 ++++++++++------ .../build/lib/remote/RemoteSpawnCache.java | 17 ++++++++++ .../build/lib/remote/RemoteSpawnRunner.java | 23 +++++++++++++ .../lib/remote/common/ExecIOException.java | 34 +++++++++++++++++++ .../bazel/remote/remote_execution_test.sh | 4 +-- 5 files changed, 95 insertions(+), 13 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/common/ExecIOException.java 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 index 6481c5b7bb8091..e90e8aad25cbea 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GoogleChannelConnectionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GoogleChannelConnectionFactory.java @@ -25,15 +25,19 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement; +import com.google.devtools.build.lib.remote.common.ExecIOException; 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.RxFutures; -import com.google.devtools.build.lib.remote.util.Utils; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; +import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code; import io.grpc.ClientInterceptor; import io.grpc.ManagedChannel; import io.reactivex.rxjava3.core.Single; @@ -153,18 +157,24 @@ public void onSuccess(ServerCapabilities result) { @Override public void onFailure(Throwable error) { - String message = - "Failed to query remote execution capabilities: " - + Utils.grpcAwareErrorMessage(error, verboseFailures); - reporter.handle(Event.error(message)); - - IOException exception; + IOException cause; if (error instanceof IOException ioException) { - exception = ioException; + cause = ioException; + } else if (error.getCause() instanceof IOException ioException) { + cause = ioException; } else { - exception = new IOException(error); + cause = new IOException("Failed to query remote execution capabilities", error); } - serverCapabilities.setException(exception); + serverCapabilities.setException( + new ExecIOException( + new EnvironmentalExecException( + cause, + FailureDetail.newBuilder() + .setRemoteExecution( + RemoteExecution.newBuilder() + .setCode(Code.CAPABILITIES_QUERY_FAILURE) + .build()) + .build()))); } }, directExecutor()); 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 index ae802b93203935..a48df2a9b83c22 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -20,6 +20,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; @@ -27,6 +28,7 @@ import com.google.devtools.build.lib.actions.SpawnMetrics; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperException; 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; @@ -38,11 +40,15 @@ import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult; import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.common.ExecIOException; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; +import com.google.devtools.build.lib.server.FailureDetails; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.server.FailureDetails.RemoteOptions.Code; import com.google.devtools.build.lib.vfs.Path; import java.io.IOException; import java.util.NoSuchElementException; @@ -165,6 +171,17 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) } } catch (CacheNotFoundException e) { // Intentionally left blank + } catch (CredentialHelperException e) { + throw new EnvironmentalExecException( + e, + FailureDetail.newBuilder() + .setRemoteOptions( + FailureDetails.RemoteOptions.newBuilder() + .setCode(Code.CREDENTIALS_READ_FAILURE) + .build()) + .build()); + } catch (ExecIOException e){ + throw e.getExecException(); } catch (IOException e) { if (BulkTransferException.allCausedByCacheNotFoundException(e)) { // Intentionally left blank 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 286137c609bac1..b8aa5c4e9df69c 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 @@ -35,6 +35,7 @@ import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.Spawn; @@ -42,6 +43,7 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperException; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.clock.BlazeClock.MillisSinceEpochToNanosConverter; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -61,6 +63,7 @@ import com.google.devtools.build.lib.remote.RemoteExecutionService.ServerLogs; import com.google.devtools.build.lib.remote.circuitbreaker.CircuitBreakerFactory; import com.google.devtools.build.lib.remote.common.BulkTransferException; +import com.google.devtools.build.lib.remote.common.ExecIOException; import com.google.devtools.build.lib.remote.common.OperationObserver; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -241,6 +244,10 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) } } } + } catch (CredentialHelperException e) { + throw createExecExceptionForCredentialHelperException(e); + } catch (ExecIOException e){ + throw e.getExecException(); } catch (IOException e) { return execLocallyAndUploadOrFail(action, spawn, context, uploadLocalResults, e); } @@ -328,11 +335,27 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) throw e; } }); + } catch (CredentialHelperException e) { + throw createExecExceptionForCredentialHelperException(e); + } catch (ExecIOException e){ + throw e.getExecException(); } catch (IOException e) { return execLocallyAndUploadOrFail(action, spawn, context, uploadLocalResults, e); } } + private static ExecException createExecExceptionForCredentialHelperException( + CredentialHelperException e) { + return new EnvironmentalExecException( + e, + FailureDetail.newBuilder() + .setRemoteOptions( + FailureDetails.RemoteOptions.newBuilder() + .setCode(FailureDetails.RemoteOptions.Code.CREDENTIALS_READ_FAILURE) + .build()) + .build()); + } + private static void profileAccounting( long clampTimeNanos, ExecutedActionMetadata executedActionMetadata) { MillisSinceEpochToNanosConverter converter = diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/ExecIOException.java b/src/main/java/com/google/devtools/build/lib/remote/common/ExecIOException.java new file mode 100644 index 00000000000000..be3059c5328a9a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/common/ExecIOException.java @@ -0,0 +1,34 @@ +// Copyright 2024 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.common; + +import com.google.devtools.build.lib.actions.ExecException; +import java.io.IOException; + +/** + * An exception to indicate that the execution of an action has failed and {@link + * #getExecException()} should be propagated. + */ +public class ExecIOException extends IOException { + private final ExecException execException; + + public ExecIOException(ExecException execException) { + super(execException); + this.execException = execException; + } + + public ExecException getExecException() { + return execException; + } +} diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 064d58aad1813e..cef6b9ec21d0d0 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -99,11 +99,9 @@ function test_credential_helper_remote_cache() { bazel build \ --remote_cache=grpc://localhost:${worker_port} \ - //a:a >& $TEST_log || fail "Build without credentials should have succeeded" + //a:a >& $TEST_log && fail "Build without credentials should have failed" expect_log "Failed to query remote execution capabilities" - bazel clean - # Helper shouldn't have been called yet. expect_credential_helper_calls 0