From dbb9febd603d58045b9561d5d5265d1bc756348c Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Sun, 14 May 2023 22:49:40 -0700 Subject: [PATCH 01/18] Add ServerCapabilities into RemoteExecutionClient In #18202, we discussed the possibility of conditionally using the new field exclusively based on the Remote Execution Server's capabilities. Capture Remote Execution Server's capabilities and store it in RemoteExecutor implementations for furture usage. Closes #18269. PiperOrigin-RevId: 531999688 Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0 --- .../ExperimentalGrpcRemoteExecutor.java | 9 +++++++ .../build/lib/remote/GrpcRemoteExecutor.java | 9 +++++++ .../build/lib/remote/RemoteModule.java | 13 +++++++--- .../remote/common/RemoteExecutionClient.java | 4 +++ .../ExperimentalGrpcRemoteExecutorTest.java | 10 ++++++- ...SpawnRunnerWithGrpcRemoteExecutorTest.java | 26 ++++++++++++------- 6 files changed, 58 insertions(+), 13 deletions(-) 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 d50a77cd1c32b2..428da883fba69d 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 @@ -20,6 +20,7 @@ import build.bazel.remote.execution.v2.ExecutionGrpc; import build.bazel.remote.execution.v2.ExecutionGrpc.ExecutionBlockingStub; import build.bazel.remote.execution.v2.RequestMetadata; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.WaitExecutionRequest; import com.google.common.base.Preconditions; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; @@ -56,6 +57,7 @@ @ThreadSafe public class ExperimentalGrpcRemoteExecutor implements RemoteExecutionClient { + private final ServerCapabilities serverCapabilities; private final RemoteOptions remoteOptions; private final ReferenceCountedChannel channel; private final CallCredentialsProvider callCredentialsProvider; @@ -64,10 +66,12 @@ public class ExperimentalGrpcRemoteExecutor implements RemoteExecutionClient { private final AtomicBoolean closed = new AtomicBoolean(); public ExperimentalGrpcRemoteExecutor( + ServerCapabilities serverCapabilities, RemoteOptions remoteOptions, ReferenceCountedChannel channel, CallCredentialsProvider callCredentialsProvider, RemoteRetrier retrier) { + this.serverCapabilities = serverCapabilities; this.remoteOptions = remoteOptions; this.channel = channel; this.callCredentialsProvider = callCredentialsProvider; @@ -318,6 +322,11 @@ static ExecuteResponse extractResponseOrThrowIfError(Operation operation) throws } } + @Override + public ServerCapabilities getServerCapabilities() { + return this.serverCapabilities; + } + @Override public ExecuteResponse executeRemotely( RemoteActionExecutionContext context, ExecuteRequest request, OperationObserver observer) 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 df3872ebfaeb46..d30dff698408e7 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 @@ -19,6 +19,7 @@ import build.bazel.remote.execution.v2.ExecutionGrpc; import build.bazel.remote.execution.v2.ExecutionGrpc.ExecutionBlockingStub; import build.bazel.remote.execution.v2.RequestMetadata; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.WaitExecutionRequest; import com.google.common.base.Preconditions; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; @@ -43,6 +44,7 @@ @ThreadSafe class GrpcRemoteExecutor implements RemoteExecutionClient { + private final ServerCapabilities serverCapabilities; private final ReferenceCountedChannel channel; private final CallCredentialsProvider callCredentialsProvider; private final RemoteRetrier retrier; @@ -50,9 +52,11 @@ class GrpcRemoteExecutor implements RemoteExecutionClient { private final AtomicBoolean closed = new AtomicBoolean(); public GrpcRemoteExecutor( + ServerCapabilities serverCapabilities, ReferenceCountedChannel channel, CallCredentialsProvider callCredentialsProvider, RemoteRetrier retrier) { + this.serverCapabilities = serverCapabilities; this.channel = channel; this.callCredentialsProvider = callCredentialsProvider; this.retrier = retrier; @@ -89,6 +93,11 @@ private ExecuteResponse getOperationResponse(Operation op) throws IOException { return null; } + @Override + public ServerCapabilities getServerCapabilities() { + return this.serverCapabilities; + } + /* Execute has two components: the Execute call and (optionally) the WaitExecution call. * This is the simple flow without any errors: * 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 index 417ec5bf104f94..bf6d45a58c1933 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -489,11 +489,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { // // If they point to different endpoints, we check the endpoint with execution or cache // capabilities respectively. + ServerCapabilities executionCapabilities = null; ServerCapabilities cacheCapabilities = null; try { if (execChannel != null) { if (cacheChannel != execChannel) { - var unused = + executionCapabilities = getAndVerifyServerCapabilities( remoteOptions, execChannel, @@ -521,6 +522,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { env, digestUtil, ServerCapabilitiesRequirement.EXECUTION_AND_CACHE); + executionCapabilities = cacheCapabilities; } } else { cacheCapabilities = @@ -601,7 +603,11 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { Retrier.ALLOW_ALL_CALLS); remoteExecutor = new ExperimentalGrpcRemoteExecutor( - remoteOptions, execChannel.retain(), callCredentialsProvider, execRetrier); + executionCapabilities, + remoteOptions, + execChannel.retain(), + callCredentialsProvider, + execRetrier); } else { RemoteRetrier execRetrier = new RemoteRetrier( @@ -610,7 +616,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { retryScheduler, Retrier.ALLOW_ALL_CALLS); remoteExecutor = - new GrpcRemoteExecutor(execChannel.retain(), callCredentialsProvider, execRetrier); + new GrpcRemoteExecutor( + executionCapabilities, execChannel.retain(), callCredentialsProvider, execRetrier); } execChannel.release(); RemoteExecutionCache remoteCache = diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteExecutionClient.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteExecutionClient.java index ac2c283ee8ff2d..eff0c581dcbbac 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteExecutionClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteExecutionClient.java @@ -15,6 +15,7 @@ import build.bazel.remote.execution.v2.ExecuteRequest; import build.bazel.remote.execution.v2.ExecuteResponse; +import build.bazel.remote.execution.v2.ServerCapabilities; import java.io.IOException; /** @@ -24,6 +25,9 @@ */ public interface RemoteExecutionClient { + /** Returns the cache capabilities of the remote execution server */ + ServerCapabilities getServerCapabilities(); + /** Execute an action remotely using Remote Execution API. */ ExecuteResponse executeRemotely( RemoteActionExecutionContext context, ExecuteRequest request, OperationObserver observer) 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 ff3b6c79a732ef..d42021a4027141 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 @@ -21,8 +21,10 @@ 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; @@ -133,9 +135,15 @@ public int maxConcurrency() { } }); + ServerCapabilities caps = + ServerCapabilities.newBuilder() + .setExecutionCapabilities( + ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); + executor = new ExperimentalGrpcRemoteExecutor( - remoteOptions, channel, CallCredentialsProvider.NO_CREDENTIALS, retrier); + caps, remoteOptions, channel, CallCredentialsProvider.NO_CREDENTIALS, retrier); } @After diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index 7088859f7653da..8c3c003826eca3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -34,6 +34,7 @@ import build.bazel.remote.execution.v2.Directory; 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.ExecutionGrpc.ExecutionImplBase; import build.bazel.remote.execution.v2.FileNode; import build.bazel.remote.execution.v2.FindMissingBlobsRequest; @@ -42,6 +43,7 @@ import build.bazel.remote.execution.v2.OutputDirectory; import build.bazel.remote.execution.v2.OutputFile; import build.bazel.remote.execution.v2.RequestMetadata; +import build.bazel.remote.execution.v2.ServerCapabilities; import build.bazel.remote.execution.v2.Tree; import build.bazel.remote.execution.v2.WaitExecutionRequest; import com.google.bytestream.ByteStreamGrpc.ByteStreamImplBase; @@ -198,13 +200,13 @@ public final void setUp() throws Exception { new FakeOwner("Mnemonic", "Progress Message", "//dummy:label"), ImmutableList.of("/bin/echo", "Hi!"), ImmutableMap.of("VARIABLE", "value"), - /*executionInfo=*/ ImmutableMap.of(), - /*runfilesSupplier=*/ null, - /*filesetMappings=*/ ImmutableMap.of(), - /*inputs=*/ NestedSetBuilder.create( + /* executionInfo= */ ImmutableMap.of(), + /* runfilesSupplier= */ null, + /* filesetMappings= */ ImmutableMap.of(), + /* inputs= */ NestedSetBuilder.create( Order.STABLE_ORDER, ActionInputHelper.fromPath("input")), - /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), - /*outputs=*/ ImmutableSet.of( + /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /* outputs= */ ImmutableSet.of( new ActionInput() { @Override public String getExecPathString() { @@ -247,7 +249,7 @@ public PathFragment getExecPath() { return PathFragment.create("bar"); } }), - /*mandatoryOutputs=*/ ImmutableSet.of(), + /* mandatoryOutputs= */ ImmutableSet.of(), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout"); @@ -295,8 +297,14 @@ public int maxConcurrency() { return 100; } }); + ServerCapabilities caps = + ServerCapabilities.newBuilder() + .setExecutionCapabilities( + ExecutionCapabilities.newBuilder().setExecEnabled(true).build()) + .build(); GrpcRemoteExecutor executor = - new GrpcRemoteExecutor(channel.retain(), CallCredentialsProvider.NO_CREDENTIALS, retrier); + new GrpcRemoteExecutor( + caps, channel.retain(), CallCredentialsProvider.NO_CREDENTIALS, retrier); CallCredentialsProvider callCredentialsProvider = GoogleAuthUtils.newCallCredentialsProvider(null); GrpcCacheClient cacheProtocol = @@ -326,7 +334,7 @@ public int maxConcurrency() { remoteOptions, Options.getDefaults(ExecutionOptions.class), /* verboseFailures= */ true, - /*cmdlineReporter=*/ null, + /* cmdlineReporter= */ null, retryService, logDir, remoteExecutionService); From 92cc9030d30aa7719b648634eb13d83eb00c3aaa Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Tue, 23 May 2023 01:12:09 -0700 Subject: [PATCH 02/18] Report percentual download progress in repository rules (#18471) If the HTTP response to a download request contains a `Content-Length` header, download progress is now reported as `10.1 MiB (20.2%)` instead of `10.1 MiB (10,590,000B)`. Closes #18450. PiperOrigin-RevId: 534035444 Change-Id: I1c5144555eda1890652b4d3f62b414292ba909d5 Co-authored-by: Fabian Meumertzheim --- .../downloader/DownloadProgressEvent.java | 25 +++++++++++-- .../repository/downloader/HttpStream.java | 9 +++-- .../downloader/ProgressInputStream.java | 27 ++++++++++---- .../repository/downloader/HttpStreamTest.java | 2 +- .../downloader/ProgressInputStreamTest.java | 36 ++++++++++++++++--- 5 files changed, 81 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadProgressEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadProgressEvent.java index 413a4d331c8b3a..d6d99d11ac0155 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadProgressEvent.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadProgressEvent.java @@ -17,6 +17,10 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.util.Utils; import java.net.URL; +import java.text.DecimalFormat; +import java.text.DecimalFormatSymbols; +import java.util.Locale; +import java.util.OptionalLong; /** * Postable event reporting on progress made downloading an URL. It can be used to report the URL @@ -26,17 +30,20 @@ public class DownloadProgressEvent implements ExtendedEventHandler.FetchProgress private final URL originalUrl; private final URL actualUrl; private final long bytesRead; + private final OptionalLong totalBytes; private final boolean downloadFinished; - public DownloadProgressEvent(URL originalUrl, URL actualUrl, long bytesRead, boolean finished) { + public DownloadProgressEvent( + URL originalUrl, URL actualUrl, long bytesRead, OptionalLong totalBytes, boolean finished) { this.originalUrl = originalUrl; this.actualUrl = actualUrl; this.bytesRead = bytesRead; + this.totalBytes = totalBytes; this.downloadFinished = finished; } public DownloadProgressEvent(URL originalUrl, long bytesRead, boolean finished) { - this(originalUrl, null, bytesRead, finished); + this(originalUrl, null, bytesRead, OptionalLong.empty(), finished); } public DownloadProgressEvent(URL url, long bytesRead) { @@ -69,10 +76,22 @@ public long getBytesRead() { return bytesRead; } + private static final DecimalFormat PERCENTAGE_FORMAT = + new DecimalFormat("0.0%", new DecimalFormatSymbols(Locale.US)); + @Override public String getProgress() { if (bytesRead > 0) { - return String.format("%s (%,dB)", Utils.bytesCountToDisplayString(bytesRead), bytesRead); + if (totalBytes.isPresent()) { + double totalBytesDouble = this.totalBytes.getAsLong(); + double ratio = totalBytesDouble != 0 ? bytesRead / totalBytesDouble : 1; + // 10.1 MiB (20.2%) + return String.format( + "%s (%s)", Utils.bytesCountToDisplayString(bytesRead), PERCENTAGE_FORMAT.format(ratio)); + } else { + // 10.1 MiB (10,590,000B) + return String.format("%s (%,dB)", Utils.bytesCountToDisplayString(bytesRead), bytesRead); + } } else { return ""; } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStream.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStream.java index ec4c4b1febe243..bb12b02517c531 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStream.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStream.java @@ -30,6 +30,7 @@ import java.io.SequenceInputStream; import java.net.URL; import java.net.URLConnection; +import java.util.OptionalLong; import java.util.zip.GZIPInputStream; import javax.annotation.WillCloseWhenClosed; @@ -87,17 +88,19 @@ HttpStream create( stream = retrier; } + OptionalLong totalBytes = OptionalLong.empty(); try { String contentLength = connection.getHeaderField("Content-Length"); if (contentLength != null) { - long expectedSize = Long.parseLong(contentLength); - stream = new CheckContentLengthInputStream(stream, expectedSize); + totalBytes = OptionalLong.of(Long.parseUnsignedLong(contentLength)); + stream = new CheckContentLengthInputStream(stream, totalBytes.getAsLong()); } } catch (NumberFormatException ignored) { // ignored } - stream = progressInputStreamFactory.create(stream, connection.getURL(), originalUrl); + stream = + progressInputStreamFactory.create(stream, connection.getURL(), originalUrl, totalBytes); // Determine if we need to transparently gunzip. See RFC2616 § 3.5 and § 14.11. Please note // that some web servers will send Content-Encoding: gzip even when we didn't request it if diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStream.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStream.java index acea22dc9c6414..5a1bfe2ae2efbe 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStream.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStream.java @@ -24,6 +24,7 @@ import java.io.InputStream; import java.net.URL; import java.util.Locale; +import java.util.OptionalLong; import java.util.concurrent.atomic.AtomicLong; import javax.annotation.WillCloseWhenClosed; @@ -50,9 +51,20 @@ static class Factory { this.eventHandler = eventHandler; } - InputStream create(@WillCloseWhenClosed InputStream delegate, URL url, URL originalUrl) { + InputStream create( + @WillCloseWhenClosed InputStream delegate, + URL url, + URL originalUrl, + OptionalLong totalBytes) { return new ProgressInputStream( - locale, clock, eventHandler, PROGRESS_INTERVAL_MS, delegate, url, originalUrl); + locale, + clock, + eventHandler, + PROGRESS_INTERVAL_MS, + delegate, + url, + originalUrl, + totalBytes); } } @@ -63,6 +75,7 @@ InputStream create(@WillCloseWhenClosed InputStream delegate, URL url, URL origi private final long intervalMs; private final URL url; private final URL originalUrl; + private final OptionalLong totalBytes; private final AtomicLong toto = new AtomicLong(); private final AtomicLong nextEvent; @@ -73,7 +86,8 @@ InputStream create(@WillCloseWhenClosed InputStream delegate, URL url, URL origi long intervalMs, InputStream delegate, URL url, - URL originalUrl) { + URL originalUrl, + OptionalLong totalBytes) { Preconditions.checkArgument(intervalMs >= 0); this.locale = locale; this.clock = clock; @@ -82,8 +96,9 @@ InputStream create(@WillCloseWhenClosed InputStream delegate, URL url, URL origi this.delegate = delegate; this.url = url; this.originalUrl = originalUrl; + this.totalBytes = totalBytes; this.nextEvent = new AtomicLong(clock.currentTimeMillis() + intervalMs); - eventHandler.post(new DownloadProgressEvent(originalUrl, url, 0, false)); + eventHandler.post(new DownloadProgressEvent(originalUrl, url, 0, totalBytes, false)); } @Override @@ -112,7 +127,7 @@ public int available() throws IOException { @Override public void close() throws IOException { delegate.close(); - eventHandler.post(new DownloadProgressEvent(originalUrl, url, toto.get(), true)); + eventHandler.post(new DownloadProgressEvent(originalUrl, url, toto.get(), totalBytes, true)); } private void reportProgress(long bytesRead) { @@ -124,7 +139,7 @@ private void reportProgress(long bytesRead) { if (!url.getHost().equals(originalUrl.getHost())) { via = " via " + url.getHost(); } - eventHandler.post(new DownloadProgressEvent(originalUrl, url, bytesRead, false)); + eventHandler.post(new DownloadProgressEvent(originalUrl, url, bytesRead, totalBytes, false)); eventHandler.handle( Event.progress( String.format(locale, "Downloading %s%s: %,d bytes", originalUrl, via, bytesRead))); diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStreamTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStreamTest.java index 9f61f8f62d8979..f37d49423632bf 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStreamTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpStreamTest.java @@ -93,7 +93,7 @@ public void before() throws Exception { nRetries = 0; when(connection.getInputStream()).thenReturn(new ByteArrayInputStream(data)); - when(progress.create(any(InputStream.class), any(), any(URL.class))) + when(progress.create(any(InputStream.class), any(), any(URL.class), any())) .thenAnswer( new Answer() { @Override diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStreamTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStreamTest.java index df37a1d55bfae0..1e5596afc09365 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStreamTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/ProgressInputStreamTest.java @@ -37,6 +37,7 @@ import java.io.InputStream; import java.net.URL; import java.util.Locale; +import java.util.OptionalLong; import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; @@ -53,7 +54,8 @@ public class ProgressInputStreamTest { private final InputStream delegate = mock(InputStream.class); private final URL url = makeUrl("http://lol.example"); private ProgressInputStream stream = - new ProgressInputStream(Locale.US, clock, extendedEventHandler, 1, delegate, url, url); + new ProgressInputStream( + Locale.US, clock, extendedEventHandler, 1, delegate, url, url, OptionalLong.empty()); @After public void after() throws Exception { @@ -126,7 +128,15 @@ public void bufferReadsAfterInterval_emitsProgressOnce() throws Exception { @Test public void bufferReadsAfterIntervalInGermany_usesPeriodAsSeparator() throws Exception { stream = - new ProgressInputStream(Locale.GERMANY, clock, extendedEventHandler, 1, delegate, url, url); + new ProgressInputStream( + Locale.GERMANY, + clock, + extendedEventHandler, + 1, + delegate, + url, + url, + OptionalLong.empty()); byte[] buffer = new byte[1024]; when(delegate.read(any(byte[].class), anyInt(), anyInt())).thenReturn(1024); clock.advanceMillis(1); @@ -145,14 +155,30 @@ public void redirectedToDifferentServer_showsOriginalUrlWithVia() throws Excepti 1, delegate, new URL("http://cdn.example/foo"), - url); + url, + OptionalLong.empty()); when(delegate.read()).thenReturn(42); assertThat(stream.read()).isEqualTo(42); clock.advanceMillis(1); assertThat(stream.read()).isEqualTo(42); assertThat(stream.read()).isEqualTo(42); verify(delegate, times(3)).read(); - verify(eventHandler).handle( - Event.progress("Downloading http://lol.example via cdn.example: 2 bytes")); + verify(eventHandler) + .handle(Event.progress("Downloading http://lol.example via cdn.example: 2 bytes")); + } + + @Test + public void percentualProgress() { + DownloadProgressEvent event = + new DownloadProgressEvent( + url, url, 25 * 1024 * 1024, OptionalLong.of(100 * 1024 * 1024), false); + assertThat(event.getProgress()).isEqualTo("25.0 MiB (25.0%)"); + } + + @Test + public void percentualProgress_zeroTotalBytes() { + DownloadProgressEvent event = + new DownloadProgressEvent(url, url, 25 * 1024 * 1024, OptionalLong.of(0), false); + assertThat(event.getProgress()).isEqualTo("25.0 MiB (100.0%)"); } } From c4225657ad7cd86ca71060981c3b44dce1819c84 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 23 May 2023 18:43:37 +0200 Subject: [PATCH 03/18] [6.3.0] Support remote symlink outputs when building without the bytes. (#18476) Fixes #13355. PiperOrigin-RevId: 534008963 Change-Id: Ia4f22f16960bcdae86c1a8820e3d47a3689876d8 --- .../lib/remote/RemoteExecutionService.java | 76 +++++++------------ .../remote/build_without_the_bytes_test.sh | 61 +++++++++++---- 2 files changed, 76 insertions(+), 61 deletions(-) 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 index 68e3277d774013..c3cae384b5e7ee 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -844,12 +844,6 @@ private void injectRemoteArtifacts(RemoteAction action, ActionResultMetadata met for (Map.Entry entry : metadata.directories()) { DirectoryMetadata directory = entry.getValue(); - if (!directory.symlinks().isEmpty()) { - throw new IOException( - "Symlinks in action outputs are not yet supported by " - + "--experimental_remote_download_outputs=minimal"); - } - for (FileMetadata file : directory.files()) { remoteActionFileSystem.injectRemoteFile( file.path().asFragment(), @@ -1082,7 +1076,8 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re ImmutableList.Builder> downloadsBuilder = ImmutableList.builder(); - boolean downloadOutputs = shouldDownloadOutputsFor(result, metadata); + + boolean downloadOutputs = shouldDownloadOutputsFor(result); // Download into temporary paths, then move everything at the end. // This avoids holding the output lock while downloading, which would prevent the local branch @@ -1102,10 +1097,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re checkState( result.getExitCode() == 0, "injecting remote metadata is only supported for successful actions (exit code 0)."); - checkState( - metadata.symlinks.isEmpty(), - "Symlinks in action outputs are not yet supported by" - + " --remote_download_outputs=minimal"); } FileOutErr tmpOutErr = outErr.childOutErr(); @@ -1139,31 +1130,6 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re if (downloadOutputs) { moveOutputsToFinalLocation(downloads, realToTmpPath); - - List symlinksInDirectories = new ArrayList<>(); - for (Entry entry : metadata.directories()) { - for (SymlinkMetadata symlink : entry.getValue().symlinks()) { - // Symlinks should not be allowed inside directories because their semantics are unclear: - // tree artifacts are defined as a collection of regular files, and resolving the symlinks - // locally is asking for trouble. Sadly, we did start permitting relative symlinks at some - // point, so we can only ban the absolute ones. - // See https://github.com/bazelbuild/bazel/issues/16361. - if (symlink.target().isAbsolute()) { - throw new IOException( - String.format( - "Unsupported absolute symlink '%s' inside tree artifact '%s'", - symlink.path(), entry.getKey())); - } - symlinksInDirectories.add(symlink); - } - } - - Iterable symlinks = - Iterables.concat(metadata.symlinks(), symlinksInDirectories); - - // Create the symbolic links after all downloads are finished, because dangling symlinks - // might not be supported on all platforms. - createSymlinks(symlinks); } else { ActionInput inMemoryOutput = null; Digest inMemoryOutputDigest = null; @@ -1197,6 +1163,31 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } } + List symlinksInDirectories = new ArrayList<>(); + for (Entry entry : metadata.directories()) { + for (SymlinkMetadata symlink : entry.getValue().symlinks()) { + // Symlinks should not be allowed inside directories because their semantics are unclear: + // tree artifacts are defined as a collection of regular files, and resolving the symlinks + // locally is asking for trouble. Sadly, we did start permitting relative symlinks at some + // point, so we can only ban the absolute ones. + // See https://github.com/bazelbuild/bazel/issues/16361. + if (symlink.target().isAbsolute()) { + throw new IOException( + String.format( + "Unsupported absolute symlink '%s' inside tree artifact '%s'", + symlink.path(), entry.getKey())); + } + symlinksInDirectories.add(symlink); + } + } + + Iterable symlinks = + Iterables.concat(metadata.symlinks(), symlinksInDirectories); + + // Create the symbolic links after all downloads are finished, because dangling symlinks + // might not be supported on all platforms. + createSymlinks(symlinks); + if (result.success()) { // Check that all mandatory outputs are created. for (ActionInput output : action.getSpawn().getOutputFiles()) { @@ -1237,8 +1228,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } - private boolean shouldDownloadOutputsFor( - RemoteActionResult result, ActionResultMetadata metadata) { + private boolean shouldDownloadOutputsFor(RemoteActionResult result) { if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) { return true; } @@ -1247,16 +1237,6 @@ private boolean shouldDownloadOutputsFor( if (result.getExitCode() != 0) { return true; } - // Symlinks in actions output are not yet supported with BwoB. - if (!metadata.symlinks().isEmpty()) { - report( - Event.warn( - String.format( - "Symlinks in action outputs are not yet supported by --remote_download_minimal," - + " falling back to downloading all action outputs due to output symlink %s", - Iterables.get(metadata.symlinks(), 0).path()))); - return true; - } return false; } diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh index 0f884b2a155c54..470a0998920cd0 100755 --- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh +++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh @@ -360,7 +360,7 @@ EOF # Test that --remote_download_toplevel fetches inputs to symlink actions. In # particular, cc_binary links against a symlinked imported .so file, and only # the symlink is in the runfiles. -function test_downloads_toplevel_symlinks() { +function test_downloads_toplevel_symlink_action() { if [[ "$PLATFORM" == "darwin" ]]; then # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of @@ -409,25 +409,60 @@ EOF ./bazel-bin/a/foo${EXE_EXT} || fail "bazel-bin/a/foo${EXE_EXT} failed to run" } -function test_symlink_outputs_warning_with_minimal() { - mkdir -p a - cat > a/input.txt <<'EOF' -Input file +function setup_symlink_output() { + mkdir -p pkg + + cat > pkg/defs.bzl < a/BUILD <<'EOF' -genrule( - name = "foo", - srcs = ["input.txt"], - outs = ["output.txt", "output_symlink", "output_symlink_2"], - cmd = "cp $< $(location :output.txt) && ln -s output.txt $(location output_symlink) && ln -s output.txt $(location output_symlink_2)", + + cat > pkg/BUILD <& $TEST_log || fail "Expected build of //pkg:sym to succeed" + + if [[ "$(readlink bazel-bin/pkg/sym)" != "target.txt" ]]; then + fail "Expected bazel-bin/pkg/sym to be a symlink pointing to target.txt" + fi +} + +function test_downloads_toplevel_dangling_symlink_output() { + setup_symlink_output bazel build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_download_minimal \ - //a:foo >& $TEST_log || fail "Expected build of //a:foo to succeed" - expect_log "Symlinks in action outputs are not yet supported" + //pkg:sym >& $TEST_log || fail "Expected build of //pkg:sym to succeed" + + if [[ "$(readlink bazel-bin/pkg/sym)" != "target.txt" ]]; then + fail "Expected bazel-bin/pkg/sym to be a symlink pointing to target.txt" + fi } # Regression test that --remote_download_toplevel does not crash when the From 53d785b66e00e82e99d9a1f12ffa83e8b8666eaa Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Wed, 24 May 2023 01:33:38 -0700 Subject: [PATCH 04/18] Enrich local BEP upload errors with file path and digest possible. (#18481) Closes #18461. PiperOrigin-RevId: 534006853 Change-Id: I256297fe494393f13e178bf74f967b9b691db281 Co-authored-by: Benjamin Peterson --- .../ByteStreamBuildEventArtifactUploader.java | 20 ++++++++++++------- ...eStreamBuildEventArtifactUploaderTest.java | 2 +- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 8eab0e6666479f..31b3ba6bf5cf7b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -256,7 +256,7 @@ private Single> queryRemoteCache( return toSingle(() -> remoteCache.findMissingDigests(context, digestsToQuery), executor) .onErrorResumeNext( error -> { - reporterUploadError(error); + reportUploadError(error, null, null); // Assuming all digests are missing if failed to query return Single.just(ImmutableSet.copyOf(digestsToQuery)); }) @@ -267,13 +267,19 @@ private Single> queryRemoteCache( }); } - private void reporterUploadError(Throwable error) { + private void reportUploadError(Throwable error, Path path, Digest digest) { if (error instanceof CancellationException) { return; } - String errorMessage = - "Uploading BEP referenced local files: " + grpcAwareErrorMessage(error, verboseFailures); + String errorMessage = "Uploading BEP referenced local file"; + if (path != null) { + errorMessage += " " + path; + } + if (digest != null) { + errorMessage += " " + digest; + } + errorMessage += ": " + grpcAwareErrorMessage(error, verboseFailures); reporter.handle(Event.warn(errorMessage)); } @@ -298,11 +304,11 @@ private Single> uploadLocalFiles( path.isDirectory(), // set remote to true so the PathConverter will use bytestream:// // scheme to convert the URI for this file - /*remote=*/ true, + /* remote= */ true, path.isOmitted())) .onErrorResumeNext( error -> { - reporterUploadError(error); + reportUploadError(error, path.getPath(), path.getDigest()); return Single.just(path); }); }) @@ -329,7 +335,7 @@ private Single upload(Set files) { try { return readPathMetadata(file); } catch (IOException e) { - reporterUploadError(e); + reportUploadError(e, file, null); return new PathMetadata( file, /* digest= */ null, diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index bd9c625110864a..31176ef61beee7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -333,7 +333,7 @@ public void onCompleted() { assertThat(eventHandler.getEvents()).isNotEmpty(); assertThat(eventHandler.getEvents().get(0).getMessage()) - .contains("Uploading BEP referenced local files: "); + .contains("Uploading BEP referenced local file /file"); artifactUploader.release(); From f1485714fffed5c62729ca3d7855729889fdc652 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Wed, 24 May 2023 06:33:16 -0700 Subject: [PATCH 05/18] Set `GTEST_SHARD_STATUS_FILE` in test setup (#18482) googletest only reads `GTEST_SHARD_STATUS_FILE`, not `TEST_SHARD_STATUS_FILE`, so this variable has to be set to keep sharded `cc_test`s relying on the test framework working with `--incompatible_check_sharding_support`. Closes #18469. PiperOrigin-RevId: 534375567 Change-Id: I0ca909cc7eb0b0f28f756e90e333e6bf39a0954d Co-authored-by: Fabian Meumertzheim --- tools/test/test-setup.sh | 1 + tools/test/windows/tw.cc | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/test/test-setup.sh b/tools/test/test-setup.sh index 952754c82dd62d..b5c48def4a0b9c 100755 --- a/tools/test/test-setup.sh +++ b/tools/test/test-setup.sh @@ -102,6 +102,7 @@ export -n TEST_UNDECLARED_OUTPUTS_ANNOTATIONS if [[ -n "${TEST_TOTAL_SHARDS+x}" ]] && ((TEST_TOTAL_SHARDS != 0)); then export GTEST_SHARD_INDEX="${TEST_SHARD_INDEX}" export GTEST_TOTAL_SHARDS="${TEST_TOTAL_SHARDS}" + export GTEST_SHARD_STATUS_FILE="${TEST_SHARD_STATUS_FILE}" fi export GTEST_TMP_DIR="${TEST_TMPDIR}" diff --git a/tools/test/windows/tw.cc b/tools/test/windows/tw.cc index 8d5f61c4a3d915..011eebc6a42e6a 100644 --- a/tools/test/windows/tw.cc +++ b/tools/test/windows/tw.cc @@ -615,7 +615,10 @@ bool ExportGtestVariables(const Path& test_tmpdir) { } if (total_shards_value > 0) { std::wstring shard_index; - if (!GetEnv(L"TEST_SHARD_INDEX", &shard_index) || + std::wstring shard_status_file; + if (!GetEnv(L"TEST_SHARD_STATUS_FILE", &shard_status_file) || + !GetEnv(L"TEST_SHARD_INDEX", &shard_index) || + !SetEnv(L"GTEST_SHARD_STATUS_FILE", shard_status_file) || !SetEnv(L"GTEST_SHARD_INDEX", shard_index) || !SetEnv(L"GTEST_TOTAL_SHARDS", total_shards_str)) { return false; From 96124a0211a2956ea955c68b6ab2f65308fdc8f8 Mon Sep 17 00:00:00 2001 From: keertk Date: Wed, 24 May 2023 17:43:14 +0000 Subject: [PATCH 06/18] Fix relnotes script (#18491) - Update GitHub token retrieval - Allow checking out tags (final releases) in addition to branches (RCs) PiperOrigin-RevId: 534851155 Change-Id: I8b92898a8f5dd20139ca2467bb8604c6a8eabd67 --- scripts/release/relnotes.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/scripts/release/relnotes.py b/scripts/release/relnotes.py index 66581cff627514..402475a2932be4 100644 --- a/scripts/release/relnotes.py +++ b/scripts/release/relnotes.py @@ -14,7 +14,6 @@ """Script to generate release notes.""" -import os import re import subprocess import sys @@ -87,13 +86,13 @@ def get_relnotes_between(base, head, is_major_release): def get_label(issue_id): """Get team-X label added to issue.""" - auth = os.system( + auth = subprocess.check_output( "gsutil cat" " gs://bazel-trusted-encrypted-secrets/github-trusted-token.enc |" " gcloud kms decrypt --project bazel-public --location global" " --keyring buildkite --key github-trusted-token --ciphertext-file" - " - --plaintext-file -" - ) + " - --plaintext-file -", shell=True + ).decode("utf-8").strip().split("\n")[0] headers = { "Authorization": "Bearer " + auth, "Accept": "application/vnd.github+json", @@ -159,11 +158,16 @@ def get_external_authors_between(base, head): # e.g. if current_release is 5.3.3, last_release should be 5.3.2 even if # latest release is 6.1.1 current_release = git("rev-parse", "--abbrev-ref", "HEAD")[0] - if not current_release.startswith("release-"): - print("Error: Not a release- branch") - sys.exit(1) - current_release = re.sub(r"rc\d", "", current_release[len("release-"):]) + if current_release.startswith("release-"): + current_release = re.sub(r"rc\d", "", current_release[len("release-"):]) + else: + try: + current_release = git("describe", "--tags")[0] + except Exception: # pylint: disable=broad-exception-caught + print("Error: Not a release branch.") + sys.exit(1) + is_major = bool(re.fullmatch(r"\d+.0.0", current_release)) tags = [tag for tag in git("tag", "--sort=refname") if "pre" not in tag] From 15fc2925e7e45ee2fa562bf996b148a0c19230d6 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Thu, 25 May 2023 00:12:44 -0700 Subject: [PATCH 07/18] Fix Xcode 14.3 compatibility (#18490) With Xcode 14.3+ on x86_64 machines there is an opaque error when linking binaries on macOS because Apple removed an old support library. That library is only linked if the macOS target is < 10.11, so this bumps the default versions past that. This macOS version was released in September 2015. Fixes https://github.com/bazelbuild/bazel/issues/18278 Closes #18460. PiperOrigin-RevId: 534743568 Change-Id: I131880096c941df0097fe3b1faabd5a6afada4f3 Co-authored-by: Keith Smiley --- .bazelrc | 2 +- .../devtools/build/lib/rules/apple/AppleCommandLineOptions.java | 2 +- .../devtools/build/lib/rules/apple/XcodeVersionProperties.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.bazelrc b/.bazelrc index 20921ea73cf117..dc2e5ceb7d6cd2 100644 --- a/.bazelrc +++ b/.bazelrc @@ -25,7 +25,7 @@ build:ubuntu1804_java11 --config=remote_shared # Alias build:remote --config=ubuntu1804_java11 -build:macos --macos_minimum_os=10.10 +build:macos --macos_minimum_os=10.11 # Enable Bzlmod build:bzlmod --experimental_enable_bzlmod diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java index e6d9128f32598c..794df2293f98a1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java @@ -188,7 +188,7 @@ public class AppleCommandLineOptions extends FragmentOptions { @VisibleForTesting public static final String DEFAULT_IOS_SDK_VERSION = "8.4"; @VisibleForTesting public static final String DEFAULT_WATCHOS_SDK_VERSION = "2.0"; - @VisibleForTesting public static final String DEFAULT_MACOS_SDK_VERSION = "10.10"; + @VisibleForTesting public static final String DEFAULT_MACOS_SDK_VERSION = "10.11"; @VisibleForTesting public static final String DEFAULT_TVOS_SDK_VERSION = "9.0"; @VisibleForTesting static final String DEFAULT_IOS_CPU = "x86_64"; diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeVersionProperties.java b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeVersionProperties.java index d0b15f3c6d6ba5..f3e6e39e175423 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeVersionProperties.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeVersionProperties.java @@ -37,7 +37,7 @@ public class XcodeVersionProperties extends NativeInfo implements XcodePropertie @VisibleForTesting public static final String DEFAULT_IOS_SDK_VERSION = "8.4"; @VisibleForTesting public static final String DEFAULT_WATCHOS_SDK_VERSION = "2.0"; - @VisibleForTesting public static final String DEFAULT_MACOS_SDK_VERSION = "10.10"; + @VisibleForTesting public static final String DEFAULT_MACOS_SDK_VERSION = "10.11"; @VisibleForTesting public static final String DEFAULT_TVOS_SDK_VERSION = "9.0"; private final Optional xcodeVersion; From a93c4609facc87848abbb9212ef397809a0b535d Mon Sep 17 00:00:00 2001 From: Pavan Singh <127178870+Pavank1992@users.noreply.github.com> Date: Tue, 30 May 2023 14:22:52 +0530 Subject: [PATCH 08/18] Fix https://github.com/bazelbuild/bazel/issues/18493. (#18514) The use of the pipe in https://github.com/bazelbuild/bazel/commit/b8e92cc55378fb7b80f5abe0abf59d41f53ba483 made it swallow the exit code, so we need to set pipefail. Fix https://github.com/bazelbuild/bazel/issues/18493 Closes #18498. PiperOrigin-RevId: 535520323 Change-Id: Idf1a5c39bf5b7deec29b76c10ece2825b568ebf2 Co-authored-by: Tobias Werth --- src/test/shell/bazel/run_test.sh | 39 ++++++++++++++++++++++++++++++++ tools/test/test-setup.sh | 2 ++ 2 files changed, 41 insertions(+) diff --git a/src/test/shell/bazel/run_test.sh b/src/test/shell/bazel/run_test.sh index 8f30f03345e637..fd6c3c0de5d53e 100755 --- a/src/test/shell/bazel/run_test.sh +++ b/src/test/shell/bazel/run_test.sh @@ -125,4 +125,43 @@ eof echo "$output" | grep --fixed-strings 'ExecuteProgram(C:\first_part second_part)' || fail "Expected error message to contain unquoted path" } +function test_run_test_exit_code() { + # EXPERIMENTAL_SPLIT_XML_GENERATION is set by the outer bazel and influences + # the test setup of the inner bazel. To make sure we hit the codepath we want + # to test here, unset the variable. + unset EXPERIMENTAL_SPLIT_XML_GENERATION + + mkdir -p foo + cat > foo/BUILD <<'EOF' +sh_test( + name = "exit0", + srcs = ["exit0.sh"], +) + +sh_test( + name = "exit1", + srcs = ["exit1.sh"], +) +EOF + + cat > foo/exit0.sh <<'EOF' +set -x +exit 0 +EOF + chmod +x foo/exit0.sh + bazel run //foo:exit0 &>"$TEST_log" \ + || fail "Expected exit code 0, received $?" + + cat > foo/exit1.sh <<'EOF' +set -x +exit 1 +EOF + chmod +x foo/exit1.sh + bazel run --noexperimental_split_xml_generation //foo:exit1 &>"$TEST_log" \ + && fail "Expected exit code 1, received $?" + + # Avoid failing the test because of the last non-zero exit-code. + true +} + run_suite "run_under_tests" diff --git a/tools/test/test-setup.sh b/tools/test/test-setup.sh index b5c48def4a0b9c..7d1cbd79d0b47a 100755 --- a/tools/test/test-setup.sh +++ b/tools/test/test-setup.sh @@ -323,11 +323,13 @@ if [[ "${EXPERIMENTAL_SPLIT_XML_GENERATION}" == "1" ]]; then ("$1" "$TEST_PATH" "${@:3}" 2>&1) <&0 & fi else + set -o pipefail if [ -z "$COVERAGE_DIR" ]; then ("${TEST_PATH}" "$@" 2>&1 | tee -a "${XML_OUTPUT_FILE}.log") <&0 & else ("$1" "$TEST_PATH" "${@:3}" 2>&1 | tee -a "${XML_OUTPUT_FILE}.log") <&0 & fi + set +o pipefail fi childPid=$! From df283f271c2bbc285c9f55e5d3fce79b157d6891 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 30 May 2023 19:47:28 +0200 Subject: [PATCH 09/18] [6.3.0] Extend the credential helper default timeout to 10s. (#18527) The default timeout makes the integration tests 1% flaky on RBE. PiperOrigin-RevId: 535255555 Change-Id: I8270ffbfcbd00ec7c38a92a546d5726f1b10b68d --- .../google/devtools/build/lib/authandtls/AuthAndTLSOptions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java b/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java index ae592aafac8981..67fb9492012703 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java @@ -161,7 +161,7 @@ public class AuthAndTLSOptions extends OptionsBase { @Option( name = "experimental_credential_helper_timeout", - defaultValue = "5s", + defaultValue = "10s", converter = DurationConverter.class, documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.UNKNOWN}, From e4814dc764bd9ad22faf9657a9214dbb910abc43 Mon Sep 17 00:00:00 2001 From: keertk Date: Tue, 30 May 2023 18:12:38 +0000 Subject: [PATCH 10/18] Fix formatting of release notes (#18534) PiperOrigin-RevId: 536324450 Change-Id: I62c0aa7c91f5b38ddae9fae0127ea7b3fb601862 Co-authored-by: Ian (Hee) Cha --- scripts/release/relnotes.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/release/relnotes.py b/scripts/release/relnotes.py index 402475a2932be4..fcdfb86ef29135 100644 --- a/scripts/release/relnotes.py +++ b/scripts/release/relnotes.py @@ -209,11 +209,13 @@ def get_external_authors_between(base, head): print("+", note) print() else: + print() for note in filtered_relnotes: print("+", note) print() print("Acknowledgements:") external_authors = get_external_authors_between(merge_base, "HEAD") + print() print("This release contains contributions from many people at Google" + ("." if not external_authors else f", as well as {external_authors}.")) From 53d1d23f6a2d40f681b3644434ddc07f4239bd59 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Wed, 31 May 2023 01:18:31 -0700 Subject: [PATCH 11/18] Use extension rather than local names in ModuleExtensionMetadata (#18536) ModuleExtensionMetadata incorrectly identified repos by their local names rather than the names used by the generating extension, which resulted in incorrect fixup warnings when supplying keyword arguments to `use_repo`. Closes #18528. PiperOrigin-RevId: 536386347 Change-Id: I4368b0dcdc28d6b2827b74b3b0b73c31a0215c0f Co-authored-by: Fabian Meumertzheim --- .../bazel/bzlmod/ModuleExtensionMetadata.java | 2 +- .../bzlmod/ModuleExtensionResolutionTest.java | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index fde1c8fcd6887c..d680b3100402c3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -188,7 +188,7 @@ private static Optional generateFixupMessage( .collect(toImmutableSet()); var actualImports = rootUsages.stream() - .flatMap(usage -> usage.getImports().keySet().stream()) + .flatMap(usage -> usage.getImports().values().stream()) .filter(repo -> !actualDevImports.contains(repo)) .collect(toImmutableSet()); diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 06033115a3f683..9e115fe0596632 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -1599,13 +1599,23 @@ public void extensionMetadata() throws Exception { "bazel_dep(name='ext', version='1.0')", "bazel_dep(name='data_repo',version='1.0')", "ext = use_extension('@ext//:defs.bzl', 'ext')", - "use_repo(ext, 'direct_dep', 'indirect_dep', 'invalid_dep')", + "use_repo(", + " ext,", + " 'indirect_dep',", + " 'invalid_dep',", + " my_direct_dep = 'direct_dep',", + ")", "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", - "use_repo(ext_dev, 'direct_dev_dep', 'indirect_dev_dep', 'invalid_dev_dep')"); + "use_repo(", + " ext_dev,", + " 'indirect_dev_dep',", + " 'invalid_dev_dep',", + " my_direct_dev_dep = 'direct_dev_dep',", + ")"); scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); scratch.file( workspaceRoot.getRelative("data.bzl").getPathString(), - "load('@direct_dep//:data.bzl', direct_dep_data='data')", + "load('@my_direct_dep//:data.bzl', direct_dep_data='data')", "data = direct_dep_data"); registry.addModule( From 4afed7358786876cbdc4e27240f7dabd273166f0 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Wed, 31 May 2023 11:26:35 -0700 Subject: [PATCH 12/18] [credentialhelper] Ignore all errors when writing stdin (#18540) It's totally fine for helpers to not care about the request on stdin and close stdin early. ``` ERROR: java.io.IOException: Broken pipe at java.base/java.io.FileOutputStream.writeBytes(Native Method) at java.base/java.io.FileOutputStream.write(Unknown Source) at java.base/java.io.BufferedOutputStream.flushBuffer(Unknown Source) at java.base/java.io.BufferedOutputStream.flush(Unknown Source) at java.base/java.io.FilterOutputStream.close(Unknown Source) at java.base/sun.nio.cs.StreamEncoder.implClose(Unknown Source) at java.base/sun.nio.cs.StreamEncoder.close(Unknown Source) at java.base/java.io.OutputStreamWriter.close(Unknown Source) at com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelper.getCredentials(CredentialHelper.java:83) at com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperCredentials.getCredentialsFromHelper(CredentialHelperCredentials.java:108) at com.github.benmanes.caffeine.cache.BoundedLocalCache.lambda$doComputeIfAbsent$13(BoundedLocalCache.java:2451) at java.base/java.util.concurrent.ConcurrentHashMap.compute(Unknown Source) at com.github.benmanes.caffeine.cache.BoundedLocalCache.doComputeIfAbsent(BoundedLocalCache.java:2449) at com.github.benmanes.caffeine.cache.BoundedLocalCache.computeIfAbsent(BoundedLocalCache.java:2432) at com.github.benmanes.caffeine.cache.LocalCache.computeIfAbsent(LocalCache.java:107) at com.github.benmanes.caffeine.cache.LocalManualCache.get(LocalManualCache.java:62) at com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperCredentials.getRequestMetadata(CredentialHelperCredentials.java:80) at com.google.auth.Credentials.blockingGetToCallback(Credentials.java:112) at com.google.auth.Credentials$1.run(Credentials.java:98) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) at java.base/java.lang.Thread.run(Unknown Source) ``` Follow-up on https://github.com/bazelbuild/bazel/commit/7c235ff15190b1eefdbd34696a55272bbb221435 Closes #18501. PiperOrigin-RevId: 536403312 Change-Id: I837bae70978e29f748c0fcc398c8f64de2b7b7c6 Co-authored-by: Yannic --- .../lib/authandtls/credentialhelper/CredentialHelper.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java index cf15e0f917c35a..d493dac75a1e31 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java @@ -82,12 +82,8 @@ public GetCredentialsResponse getCredentials(CredentialHelperEnvironment environ GSON.toJson(GetCredentialsRequest.newBuilder().setUri(uri).build(), stdin); } catch (IOException e) { // This can happen if the helper prints a static set of credentials without reading from - // stdin (e.g., with a simple shell script running `echo "{...}"`). If the process is - // already finished even though we failed to write to its stdin, ignore the error and - // assume the process did not need the request payload. - if (!process.finished()) { - throw e; - } + // stdin (e.g., with a simple shell script running `echo "{...}"`). This is fine to + // ignore. } try { From 28ebcdcd4260f76ceb0831d4b5dc315760cbb726 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Thu, 1 Jun 2023 06:34:31 -0700 Subject: [PATCH 13/18] Improve error on invalid `-//foo` and `-@repo//foo` options (#18516) `-//foo` and `-@repo//foo` are always invalid Bazel options, but are usually meant to be either negative target patterns (which have to come after the `--` separator) or Starlark options (which always start with `--`). With this commit, the error shown to the user mentions these two situations and how to fix the issue in each case. Closes #16563. PiperOrigin-RevId: 486920951 Change-Id: I9390d3859aa62c2b67eac05cb96a06209a9b7c36 Co-authored-by: Fabian Meumertzheim --- .../common/options/OptionsParserImpl.java | 12 ++++++++ .../common/options/OptionsParserTest.java | 29 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java index 79b3abbdcba889..4bebaaf90eb35e 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -476,6 +476,18 @@ private OptionsParserImplResult parse( continue; // not an option arg } + if (arg.startsWith("-//") || arg.startsWith("-@")) { + // Fail with a helpful error when an invalid option looks like an absolute negative target + // pattern or a typoed Starlark option. + throw new OptionsParsingException( + String.format( + "Invalid options syntax: %s\n" + + "Note: Negative target patterns can only appear after the end of options" + + " marker ('--'). Flags corresponding to Starlark-defined build settings" + + " always start with '--', not '-'.", + arg)); + } + arg = swapShorthandAlias(arg); if (arg.equals("--")) { // "--" means all remaining args aren't options diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index 4bc56d1084881a..b6b63d9a6ece15 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -2369,6 +2369,35 @@ public void setOptionValueAtSpecificPriorityWithoutExpansion_expandedFlag_setsVa .containsExactly("--second=hello"); } + @Test + public void negativeTargetPatternsInOptions_failsDistinctively() { + OptionsParser parser = OptionsParser.builder().optionsClasses(ExampleFoo.class).build(); + OptionsParsingException e = + assertThrows(OptionsParsingException.class, () -> parser.parse("//foo", "-//bar", "//baz")); + assertThat(e).hasMessageThat().contains("-//bar"); + assertThat(e) + .hasMessageThat() + .contains("Negative target patterns can only appear after the end of options marker"); + assertThat(e) + .hasMessageThat() + .contains("Flags corresponding to Starlark-defined build settings always start with '--'"); + } + + @Test + public void negativeExternalTargetPatternsInOptions_failsDistinctively() { + OptionsParser parser = OptionsParser.builder().optionsClasses(ExampleFoo.class).build(); + OptionsParsingException e = + assertThrows( + OptionsParsingException.class, () -> parser.parse("//foo", "-@repo//bar", "//baz")); + assertThat(e).hasMessageThat().contains("-@repo//bar"); + assertThat(e) + .hasMessageThat() + .contains("Negative target patterns can only appear after the end of options marker"); + assertThat(e) + .hasMessageThat() + .contains("Flags corresponding to Starlark-defined build settings always start with '--'"); + } + private static OptionInstanceOrigin createInvocationPolicyOrigin() { return createInvocationPolicyOrigin(/*implicitDependent=*/ null, /*expandedFrom=*/ null); } From 31f07cc72f86bbd4cfceea9dfe2f89a40740b974 Mon Sep 17 00:00:00 2001 From: amishra-u <119983081+amishra-u@users.noreply.github.com> Date: Thu, 1 Jun 2023 10:42:28 -0700 Subject: [PATCH 14/18] [6.3.0] Implement failure circuit breaker (#18541) * feat: Implement failure circuit breaker Copy of #18120: I accidentally closed #18120 during rebase and doesn't have permission to reopen. We have noticed that any problems with the remote cache have a detrimental effect on build times. On investigation we found that the interface for the circuit breaker was left unimplemented. To address this issue, implemented a failure circuit breaker, which includes three new Bazel flags: 1) experimental_circuitbreaker_strategy, 2) experimental_remote_failure_threshold, and 3) experimental_emote_failure_window. In this implementation, I have implemented failure strategy for circuit breaker and used failure count to trip the circuit. Reasoning behind using failure count instead of failure rate : To measure failure rate I also need the success count. While both the failure and success count need to be an AtomicInteger as both will be modified concurrently by multiple threads. Even though getAndIncrement is very light weight operation, at very high request it might contribute to latency. Reasoning behind using failure circuit breaker : A new instance of Retrier.CircuitBreaker is created for each build. Therefore, if the circuit breaker trips during a build, the remote cache will be disabled for that build. However, it will be enabled again for the next build as a new instance of Retrier.CircuitBreaker will be created. If needed in the future we may add cool down strategy also. e.g. failure_and_cool_down_startegy. closes https://github.com/bazelbuild/bazel/issues/18136 Closes #18359. PiperOrigin-RevId: 536349954 Change-Id: I5e1c57d4ad0ce07ddc4808bf1f327bc5df6ce704 * remove target included in cherry-pick by mistake --- .../google/devtools/build/lib/remote/BUILD | 2 + .../build/lib/remote/GrpcCacheClient.java | 4 + .../build/lib/remote/GrpcRemoteExecutor.java | 4 + .../build/lib/remote/RemoteModule.java | 12 +- .../build/lib/remote/RemoteSpawnRunner.java | 5 +- .../devtools/build/lib/remote/Retrier.java | 34 +- .../build/lib/remote/circuitbreaker/BUILD | 23 ++ .../circuitbreaker/CircuitBreakerFactory.java | 45 +++ .../circuitbreaker/FailureCircuitBreaker.java | 83 +++++ .../remote/options/CommonRemoteOptions.java | 24 ++ .../lib/remote/options/RemoteOptions.java | 49 +++ .../google/devtools/build/lib/remote/BUILD | 2 + .../build/lib/remote/RemoteModuleTest.java | 315 +++++++++--------- .../build/lib/remote/RetrierTest.java | 9 +- .../build/lib/remote/circuitbreaker/BUILD | 31 ++ .../CircuitBreakerFactoryTest.java | 44 +++ .../FailureCircuitBreakerTest.java | 68 ++++ 17 files changed, 578 insertions(+), 176 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD create mode 100644 src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactory.java create mode 100644 src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreaker.java create mode 100644 src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD create mode 100644 src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactoryTest.java create mode 100644 src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreakerTest.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 1bce8a80426e37..0f2e76ed83aecf 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -7,6 +7,7 @@ package( filegroup( name = "srcs", srcs = glob(["*"]) + [ + "//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker:srcs", "//src/main/java/com/google/devtools/build/lib/remote/common:srcs", "//src/main/java/com/google/devtools/build/lib/remote/downloader:srcs", "//src/main/java/com/google/devtools/build/lib/remote/disk:srcs", @@ -84,6 +85,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", + "//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", "//src/main/java/com/google/devtools/build/lib/remote/disk", 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 index dfd9c2a61a571c..ce0e917837f0c5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -495,4 +495,8 @@ ListenableFuture uploadChunker( MoreExecutors.directExecutor()); return f; } + + Retrier getRetrier() { + return this.retrier; + } } 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 df3872ebfaeb46..8cd463a84ab74d 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 @@ -243,4 +243,8 @@ public void close() { } channel.release(); } + + RemoteRetrier getRetrier() { + return this.retrier; + } } 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 index 417ec5bf104f94..d0afeb2d27e00e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -62,6 +62,7 @@ import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement; import com.google.devtools.build.lib.remote.ToplevelArtifactsDownloader.PathToMetadataConverter; +import com.google.devtools.build.lib.remote.circuitbreaker.CircuitBreakerFactory; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.downloader.GrpcRemoteDownloader; @@ -475,12 +476,11 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { GoogleAuthUtils.newCallCredentialsProvider(credentials); CallCredentials callCredentials = callCredentialsProvider.getCallCredentials(); + Retrier.CircuitBreaker circuitBreaker = + CircuitBreakerFactory.createCircuitBreaker(remoteOptions); RemoteRetrier retrier = new RemoteRetrier( - remoteOptions, - RemoteRetrier.RETRIABLE_GRPC_ERRORS, - retryScheduler, - Retrier.ALLOW_ALL_CALLS); + remoteOptions, RemoteRetrier.RETRIABLE_GRPC_ERRORS, retryScheduler, circuitBreaker); // We only check required capabilities for a given endpoint. // @@ -598,7 +598,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions, RemoteRetrier.RETRIABLE_GRPC_ERRORS, // Handle NOT_FOUND internally retryScheduler, - Retrier.ALLOW_ALL_CALLS); + circuitBreaker); remoteExecutor = new ExperimentalGrpcRemoteExecutor( remoteOptions, execChannel.retain(), callCredentialsProvider, execRetrier); @@ -608,7 +608,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions, RemoteRetrier.RETRIABLE_GRPC_EXEC_ERRORS, retryScheduler, - Retrier.ALLOW_ALL_CALLS); + circuitBreaker); remoteExecutor = new GrpcRemoteExecutor(execChannel.retain(), callCredentialsProvider, execRetrier); } 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 1eba36e9f1d723..32f60bdb6f79f6 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 @@ -58,6 +58,7 @@ import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult; 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.OperationObserver; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -660,6 +661,8 @@ private void report(Event evt) { private static RemoteRetrier createExecuteRetrier( RemoteOptions options, ListeningScheduledExecutorService retryService) { return new ExecuteRetrier( - options.remoteMaxRetryAttempts, retryService, Retrier.ALLOW_ALL_CALLS); + options.remoteMaxRetryAttempts, + retryService, + CircuitBreakerFactory.createCircuitBreaker(options)); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/Retrier.java b/src/main/java/com/google/devtools/build/lib/remote/Retrier.java index 4711a06eb9e454..457880268764d5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/Retrier.java +++ b/src/main/java/com/google/devtools/build/lib/remote/Retrier.java @@ -100,7 +100,7 @@ enum State { State state(); /** Called after an execution failed. */ - void recordFailure(); + void recordFailure(Exception e); /** Called after an execution succeeded. */ void recordSuccess(); @@ -130,7 +130,7 @@ public State state() { } @Override - public void recordFailure() {} + public void recordFailure(Exception e) {} @Override public void recordSuccess() {} @@ -245,7 +245,7 @@ public T execute(Callable call, Backoff backoff) throws Exception { circuitBreaker.recordSuccess(); return r; } catch (Exception e) { - circuitBreaker.recordFailure(); + circuitBreaker.recordFailure(e); Throwables.throwIfInstanceOf(e, InterruptedException.class); if (State.TRIAL_CALL.equals(circuitState)) { throw e; @@ -272,19 +272,35 @@ public ListenableFuture executeAsync(AsyncCallable call) { * backoff. */ public ListenableFuture executeAsync(AsyncCallable call, Backoff backoff) { + final State circuitState = circuitBreaker.state(); + if (State.REJECT_CALLS.equals(circuitState)) { + return Futures.immediateFailedFuture(new CircuitBreakerException()); + } try { + ListenableFuture future = + Futures.transformAsync( + call.call(), + (f) -> { + circuitBreaker.recordSuccess(); + return Futures.immediateFuture(f); + }, + MoreExecutors.directExecutor()); return Futures.catchingAsync( - call.call(), + future, Exception.class, - t -> onExecuteAsyncFailure(t, call, backoff), + t -> onExecuteAsyncFailure(t, call, backoff, circuitState), MoreExecutors.directExecutor()); } catch (Exception e) { - return onExecuteAsyncFailure(e, call, backoff); + return onExecuteAsyncFailure(e, call, backoff, circuitState); } } private ListenableFuture onExecuteAsyncFailure( - Exception t, AsyncCallable call, Backoff backoff) { + Exception t, AsyncCallable call, Backoff backoff, State circuitState) { + circuitBreaker.recordFailure(t); + if (circuitState.equals(State.TRIAL_CALL)) { + return Futures.immediateFailedFuture(t); + } if (isRetriable(t)) { long waitMillis = backoff.nextDelayMillis(t); if (waitMillis >= 0) { @@ -310,4 +326,8 @@ public Backoff newBackoff() { public boolean isRetriable(Exception e) { return shouldRetry.test(e); } + + CircuitBreaker getCircuitBreaker() { + return this.circuitBreaker; + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD b/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD new file mode 100644 index 00000000000000..caa358ec8bbd44 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD @@ -0,0 +1,23 @@ +load("@rules_java//java:defs.bzl", "java_library") + +package( + default_applicable_licenses = ["//:license"], + default_visibility = ["//src:__subpackages__"], +) + +filegroup( + name = "srcs", + srcs = glob(["*"]), + visibility = ["//src:__subpackages__"], +) + +java_library( + name = "circuitbreaker", + srcs = glob(["*.java"]), + deps = [ + "//src/main/java/com/google/devtools/build/lib/remote:Retrier", + "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", + "//src/main/java/com/google/devtools/build/lib/remote/options", + "//third_party:guava", + ], +) diff --git a/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactory.java b/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactory.java new file mode 100644 index 00000000000000..be8835b7c0b2d3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactory.java @@ -0,0 +1,45 @@ +// 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.circuitbreaker; + +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.remote.Retrier; +import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import com.google.devtools.build.lib.remote.options.RemoteOptions; + +/** Factory for {@link Retrier.CircuitBreaker} */ +public class CircuitBreakerFactory { + + public static final ImmutableSet> DEFAULT_IGNORED_ERRORS = + ImmutableSet.of(CacheNotFoundException.class); + + private CircuitBreakerFactory() {} + + /** + * Creates the instance of the {@link Retrier.CircuitBreaker} as per the strategy defined in + * {@link RemoteOptions}. In case of undefined strategy defaults to {@link + * Retrier.ALLOW_ALL_CALLS} implementation. + * + * @param remoteOptions The configuration for the CircuitBreaker implementation. + * @return an instance of CircuitBreaker. + */ + public static Retrier.CircuitBreaker createCircuitBreaker(final RemoteOptions remoteOptions) { + if (remoteOptions.circuitBreakerStrategy == RemoteOptions.CircuitBreakerStrategy.FAILURE) { + return new FailureCircuitBreaker( + remoteOptions.remoteFailureThreshold, + (int) remoteOptions.remoteFailureWindowInterval.toMillis()); + } + return Retrier.ALLOW_ALL_CALLS; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreaker.java b/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreaker.java new file mode 100644 index 00000000000000..b1b5739fd44c96 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreaker.java @@ -0,0 +1,83 @@ +// 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.circuitbreaker; + +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.remote.Retrier; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * The {@link FailureCircuitBreaker} implementation of the {@link Retrier.CircuitBreaker} prevents + * further calls to a remote cache once the number of failures within a given window exceeds a + * specified threshold for a build. In the context of Bazel, a new instance of {@link + * Retrier.CircuitBreaker} is created for each build. Therefore, if the circuit breaker trips during + * a build, the remote cache will be disabled for that build. However, it will be enabled again for + * the next build as a new instance of {@link Retrier.CircuitBreaker} will be created. + */ +public class FailureCircuitBreaker implements Retrier.CircuitBreaker { + + private State state; + private final AtomicInteger failures; + private final int failureThreshold; + private final int slidingWindowSize; + private final ScheduledExecutorService scheduledExecutor; + private final ImmutableSet> ignoredErrors; + + /** + * Creates a {@link FailureCircuitBreaker}. + * + * @param failureThreshold is used to set the number of failures required to trip the circuit + * breaker in given time window. + * @param slidingWindowSize the size of the sliding window in milliseconds to calculate the number + * of failures. + */ + public FailureCircuitBreaker(int failureThreshold, int slidingWindowSize) { + this.failureThreshold = failureThreshold; + this.failures = new AtomicInteger(0); + this.slidingWindowSize = slidingWindowSize; + this.state = State.ACCEPT_CALLS; + this.scheduledExecutor = + slidingWindowSize > 0 ? Executors.newSingleThreadScheduledExecutor() : null; + this.ignoredErrors = CircuitBreakerFactory.DEFAULT_IGNORED_ERRORS; + } + + @Override + public State state() { + return this.state; + } + + @Override + public void recordFailure(Exception e) { + if (!ignoredErrors.contains(e.getClass())) { + int failureCount = failures.incrementAndGet(); + if (slidingWindowSize > 0) { + var unused = + scheduledExecutor.schedule( + failures::decrementAndGet, slidingWindowSize, TimeUnit.MILLISECONDS); + } + // Since the state can only be changed to the open state, synchronization is not required. + if (failureCount > this.failureThreshold) { + this.state = State.REJECT_CALLS; + } + } + } + + @Override + public void recordSuccess() { + // do nothing, implement if we need to set threshold on failure rate instead of count. + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java index 27da47a8fb2da0..ea18ccd85841fc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/CommonRemoteOptions.java @@ -13,11 +13,16 @@ // limitations under the License. package com.google.devtools.build.lib.remote.options; +import com.google.devtools.common.options.Converter; +import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionsBase; +import com.google.devtools.common.options.OptionsParsingException; +import java.time.Duration; import java.util.List; +import java.util.regex.Pattern; /** Options for remote execution and distributed caching that shared between Bazel and Blaze. */ public class CommonRemoteOptions extends OptionsBase { @@ -33,4 +38,23 @@ public class CommonRemoteOptions extends OptionsBase { + " the client to request certain artifacts that might be needed locally (e.g. IDE" + " support)") public List remoteDownloadRegex; + + /** Returns the specified duration. Assumes seconds if unitless. */ + public static class RemoteDurationConverter extends Converter.Contextless { + + private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$"); + + @Override + public Duration convert(String input) throws OptionsParsingException { + if (UNITLESS_REGEX.matcher(input).matches()) { + input += "s"; + } + return new Converters.DurationConverter().convert(input, /* conversionContext= */ null); + } + + @Override + public String getTypeDescription() { + return "An immutable length of time."; + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 4eeacdb58154ff..61dd3f8749c3e5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -682,6 +682,43 @@ public RemoteOutputsStrategyConverter() { + " blobs during the build.") public boolean useNewExitCodeForLostInputs; + @Option( + name = "experimental_circuit_breaker_strategy", + documentationCategory = OptionDocumentationCategory.REMOTE, + defaultValue = "null", + effectTags = {OptionEffectTag.EXECUTION}, + converter = CircuitBreakerStrategy.Converter.class, + help = + "Specifies the strategy for the circuit breaker to use. Available strategies are" + + " \"failure\". On invalid value for the option the behavior same as the option is" + + " not set.") + public CircuitBreakerStrategy circuitBreakerStrategy; + + @Option( + name = "experimental_remote_failure_threshold", + defaultValue = "100", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "Sets the allowed number of failures in a specific time window after which it stops" + + " calling to the remote cache/executor. By default the value is 100. Setting this" + + " to 0 or negative means no limitation.") + public int remoteFailureThreshold; + + @Option( + name = "experimental_remote_failure_window_interval", + defaultValue = "60s", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.EXECUTION}, + converter = RemoteDurationConverter.class, + help = + "The interval in which the failure count of the remote requests are computed. On zero or" + + " negative value the failure duration is computed the whole duration of the" + + " execution.Following units can be used: Days (d), hours (h), minutes (m), seconds" + + " (s), and milliseconds (ms). If the unit is omitted, the value is interpreted as" + + " seconds.") + public Duration remoteFailureWindowInterval; + // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. @@ -771,4 +808,16 @@ public boolean shouldPrintMessages(boolean success) { || this == ExecutionMessagePrintMode.ALL); } } + + /** An enum for specifying different strategy for circuit breaker. */ + public enum CircuitBreakerStrategy { + FAILURE; + + /** Converts to {@link CircuitBreakerStrategy}. */ + public static class Converter extends EnumConverter { + public Converter() { + super(CircuitBreakerStrategy.class, "CircuitBreaker strategy"); + } + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 299e68e617671c..bb054fa51e0f18 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -9,6 +9,7 @@ filegroup( name = "srcs", testonly = 0, srcs = glob(["**"]) + [ + "//src/test/java/com/google/devtools/build/lib/remote/circuitbreaker:srcs", "//src/test/java/com/google/devtools/build/lib/remote/downloader:srcs", "//src/test/java/com/google/devtools/build/lib/remote/http:srcs", "//src/test/java/com/google/devtools/build/lib/remote/grpc:srcs", @@ -74,6 +75,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote:abstract_action_input_prefetcher", + "//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", "//src/main/java/com/google/devtools/build/lib/remote/disk", 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 index a56673388d1779..f94953092b1bdc 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.exec.BinTools; 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.options.RemoteOptions; import com.google.devtools.build.lib.runtime.BlazeRuntime; import com.google.devtools.build.lib.runtime.BlazeServerStartupOptions; @@ -72,6 +73,7 @@ import java.net.URI; import java.time.Duration; import java.util.ArrayList; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -79,7 +81,8 @@ /** Tests for {@link RemoteModule}. */ @RunWith(JUnit4.class) public final class RemoteModuleTest { - + private static final String EXECUTION_SERVER_NAME = "execution-server"; + private static final String CACHE_SERVER_NAME = "cache-server"; private static final ServerCapabilities CACHE_ONLY_CAPS = ServerCapabilities.newBuilder() .setLowApiVersion(ApiVersion.current.toSemVer()) @@ -106,6 +109,32 @@ public final class RemoteModuleTest { CacheCapabilities.newBuilder().addDigestFunctions(Value.SHA256).build()) .build(); + private static final ServerCapabilities EXEC_ONLY_CAPS = + ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.current.toSemVer()) + .setHighApiVersion(ApiVersion.current.toSemVer()) + .setExecutionCapabilities( + ExecutionCapabilities.newBuilder() + .setExecEnabled(true) + .setDigestFunction(Value.SHA256) + .build()) + .build(); + + private static final ServerCapabilities NONE_CAPS = + ServerCapabilities.newBuilder() + .setLowApiVersion(ApiVersion.current.toSemVer()) + .setHighApiVersion(ApiVersion.current.toSemVer()) + .build(); + + private static final CapabilitiesImpl INACCESSIBLE_GRPC_REMOTE = + new CapabilitiesImpl(null) { + @Override + public void getCapabilities( + GetCapabilitiesRequest request, StreamObserver responseObserver) { + responseObserver.onError(new UnsupportedOperationException()); + } + }; + private static CommandEnvironment createTestCommandEnvironment( RemoteModule remoteModule, RemoteOptions remoteOptions) throws IOException, AbruptExitException { @@ -188,21 +217,27 @@ private static Server createFakeServer(String serverName, BindableService... ser .build(); } + private RemoteModule remoteModule; + private RemoteOptions remoteOptions; + + @Before + public void initialize() { + remoteModule = new RemoteModule(); + remoteModule.setChannelFactory( + (target, proxy, options, interceptors) -> + InProcessChannelBuilder.forName(target).directExecutor().build()); + remoteOptions = Options.getDefaults(RemoteOptions.class); + } + @Test public void testVerifyCapabilities_executionAndCacheForSingleEndpoint() throws Exception { CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS); - String executionServerName = "execution-server"; - Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl); + Server executionServer = + createFakeServer(EXECUTION_SERVER_NAME, executionServerCapabilitiesImpl); executionServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); - - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteExecutor = executionServerName; + remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME; CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -210,6 +245,7 @@ public void testVerifyCapabilities_executionAndCacheForSingleEndpoint() throws E assertThat(Thread.interrupted()).isFalse(); assertThat(executionServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); + assertCircuitBreakerInstance(); } finally { executionServer.shutdownNow(); executionServer.awaitTermination(); @@ -219,18 +255,11 @@ public void testVerifyCapabilities_executionAndCacheForSingleEndpoint() throws E @Test public void testVerifyCapabilities_cacheOnlyEndpoint() throws Exception { CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(CACHE_ONLY_CAPS); - String cacheServerName = "cache-server"; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheServerCapabilitiesImpl); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); - - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteCache = CACHE_SERVER_NAME; CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -238,6 +267,7 @@ public void testVerifyCapabilities_cacheOnlyEndpoint() throws Exception { assertThat(Thread.interrupted()).isFalse(); assertThat(cacheServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); + assertCircuitBreakerInstance(); } finally { cacheServer.shutdownNow(); cacheServer.awaitTermination(); @@ -246,37 +276,18 @@ public void testVerifyCapabilities_cacheOnlyEndpoint() throws Exception { @Test public void testVerifyCapabilities_executionAndCacheForDifferentEndpoints() throws Exception { - ServerCapabilities caps = - ServerCapabilities.newBuilder() - .setLowApiVersion(ApiVersion.current.toSemVer()) - .setHighApiVersion(ApiVersion.current.toSemVer()) - .setExecutionCapabilities( - ExecutionCapabilities.newBuilder() - .setExecEnabled(true) - .setDigestFunction(Value.SHA256) - .build()) - .setCacheCapabilities( - CacheCapabilities.newBuilder().addDigestFunctions(Value.SHA256).build()) - .build(); - CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(caps); - String executionServerName = "execution-server"; - Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl); + CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS); + Server executionServer = + createFakeServer(EXECUTION_SERVER_NAME, executionServerCapabilitiesImpl); executionServer.start(); - CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(caps); - String cacheServerName = "cache-server"; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheServerCapabilitiesImpl); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); - - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteExecutor = executionServerName; - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME; + remoteOptions.remoteCache = CACHE_SERVER_NAME; CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -285,6 +296,7 @@ public void testVerifyCapabilities_executionAndCacheForDifferentEndpoints() thro assertThat(Thread.interrupted()).isFalse(); assertThat(executionServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); assertThat(cacheServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); + assertCircuitBreakerInstance(); } finally { executionServer.shutdownNow(); cacheServer.shutdownNow(); @@ -296,42 +308,18 @@ public void testVerifyCapabilities_executionAndCacheForDifferentEndpoints() thro @Test public void testVerifyCapabilities_executionOnlyAndCacheOnlyEndpoints() throws Exception { - ServerCapabilities executionOnlyCaps = - ServerCapabilities.newBuilder() - .setLowApiVersion(ApiVersion.current.toSemVer()) - .setHighApiVersion(ApiVersion.current.toSemVer()) - .setExecutionCapabilities( - ExecutionCapabilities.newBuilder() - .setExecEnabled(true) - .setDigestFunction(Value.SHA256) - .build()) - .build(); - CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(executionOnlyCaps); - String executionServerName = "execution-server"; - Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl); + CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_ONLY_CAPS); + Server executionServer = + createFakeServer(EXECUTION_SERVER_NAME, executionServerCapabilitiesImpl); executionServer.start(); - ServerCapabilities cacheOnlyCaps = - ServerCapabilities.newBuilder() - .setLowApiVersion(ApiVersion.current.toSemVer()) - .setHighApiVersion(ApiVersion.current.toSemVer()) - .setCacheCapabilities( - CacheCapabilities.newBuilder().addDigestFunctions(Value.SHA256).build()) - .build(); - CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(cacheOnlyCaps); - String cacheServerName = "cache-server"; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(CACHE_ONLY_CAPS); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheServerCapabilitiesImpl); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); - - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteExecutor = executionServerName; - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME; + remoteOptions.remoteCache = CACHE_SERVER_NAME; CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -340,6 +328,7 @@ public void testVerifyCapabilities_executionOnlyAndCacheOnlyEndpoints() throws E assertThat(Thread.interrupted()).isFalse(); assertThat(executionServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); assertThat(cacheServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); + assertCircuitBreakerInstance(); } finally { executionServer.shutdownNow(); cacheServer.shutdownNow(); @@ -352,24 +341,13 @@ public void testVerifyCapabilities_executionOnlyAndCacheOnlyEndpoints() throws E @Test public void testLocalFallback_shouldErrorForRemoteCacheWithoutRequiredCapabilities() throws Exception { - ServerCapabilities noneCaps = - ServerCapabilities.newBuilder() - .setLowApiVersion(ApiVersion.current.toSemVer()) - .setHighApiVersion(ApiVersion.current.toSemVer()) - .build(); - CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(noneCaps); - String cacheServerName = "cache-server"; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(NONE_CAPS); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheServerCapabilitiesImpl); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteCache = CACHE_SERVER_NAME; remoteOptions.remoteLocalFallback = true; - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -383,26 +361,12 @@ public void testLocalFallback_shouldErrorForRemoteCacheWithoutRequiredCapabiliti @Test public void testLocalFallback_shouldErrorInaccessibleGrpcRemoteCacheIfFlagNotSet() throws Exception { - String cacheServerName = "cache-server"; - CapabilitiesImplBase cacheServerCapabilitiesImpl = - new CapabilitiesImplBase() { - @Override - public void getCapabilities( - GetCapabilitiesRequest request, StreamObserver responseObserver) { - responseObserver.onError(new UnsupportedOperationException()); - } - }; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, INACCESSIBLE_GRPC_REMOTE); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteCache = CACHE_SERVER_NAME; remoteOptions.remoteLocalFallback = false; - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -415,26 +379,12 @@ public void getCapabilities( @Test public void testLocalFallback_shouldIgnoreInaccessibleGrpcRemoteCache() throws Exception { - String cacheServerName = "cache-server"; - CapabilitiesImplBase cacheServerCapabilitiesImpl = - new CapabilitiesImplBase() { - @Override - public void getCapabilities( - GetCapabilitiesRequest request, StreamObserver responseObserver) { - responseObserver.onError(new UnsupportedOperationException()); - } - }; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, INACCESSIBLE_GRPC_REMOTE); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteCache = CACHE_SERVER_NAME; remoteOptions.remoteLocalFallback = true; - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -445,6 +395,7 @@ public void getCapabilities( assertThat(actionContextProvider).isNotNull(); assertThat(actionContextProvider.getRemoteCache()).isNull(); assertThat(actionContextProvider.getRemoteExecutionClient()).isNull(); + assertCircuitBreakerInstance(); } finally { cacheServer.shutdownNow(); cacheServer.awaitTermination(); @@ -453,26 +404,12 @@ public void getCapabilities( @Test public void testLocalFallback_shouldIgnoreInaccessibleGrpcRemoteExecutor() throws Exception { - CapabilitiesImplBase executionServerCapabilitiesImpl = - new CapabilitiesImplBase() { - @Override - public void getCapabilities( - GetCapabilitiesRequest request, StreamObserver responseObserver) { - responseObserver.onError(new UnsupportedOperationException()); - } - }; - String executionServerName = "execution-server"; - Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl); + Server executionServer = createFakeServer(EXECUTION_SERVER_NAME, INACCESSIBLE_GRPC_REMOTE); executionServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteExecutor = executionServerName; + remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME; remoteOptions.remoteLocalFallback = true; - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -483,6 +420,7 @@ public void getCapabilities( assertThat(actionContextProvider).isNotNull(); assertThat(actionContextProvider.getRemoteCache()).isNull(); assertThat(actionContextProvider.getRemoteExecutionClient()).isNull(); + assertCircuitBreakerInstance(); } finally { executionServer.shutdownNow(); executionServer.awaitTermination(); @@ -496,8 +434,6 @@ public void testNetrc_netrcWithoutRemoteCache() throws Exception { Scratch scratch = new Scratch(fileSystem); scratch.file(netrc, "machine foo.example.org login baruser password barpass"); AuthAndTLSOptions authAndTLSOptions = Options.getDefaults(AuthAndTLSOptions.class); - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - Cache>> credentialCache = Caffeine.newBuilder().build(); @@ -523,18 +459,11 @@ public void testNetrc_netrcWithoutRemoteCache() throws Exception { @Test public void testCacheCapabilities_propagatedToRemoteCache() throws Exception { CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(CACHE_ONLY_CAPS); - String cacheServerName = "cache-server"; - Server cacheServer = createFakeServer(cacheServerName, cacheServerCapabilitiesImpl); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheServerCapabilitiesImpl); cacheServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); - - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteCache = cacheServerName; + remoteOptions.remoteCache = CACHE_SERVER_NAME; CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -555,18 +484,12 @@ public void testCacheCapabilities_propagatedToRemoteCache() throws Exception { @Test public void testCacheCapabilities_propagatedToRemoteExecutionCache() throws Exception { CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS); - String executionServerName = "execution-server"; - Server executionServer = createFakeServer(executionServerName, executionServerCapabilitiesImpl); + Server executionServer = + createFakeServer(EXECUTION_SERVER_NAME, executionServerCapabilitiesImpl); executionServer.start(); try { - RemoteModule remoteModule = new RemoteModule(); - remoteModule.setChannelFactory( - (target, proxy, options, interceptors) -> - InProcessChannelBuilder.forName(target).directExecutor().build()); - - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - remoteOptions.remoteExecutor = executionServerName; + remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME; CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); @@ -583,4 +506,80 @@ public void testCacheCapabilities_propagatedToRemoteExecutionCache() throws Exce executionServer.awaitTermination(); } } + + @Test + public void testVerifyCapabilities_executionAndCacheForSingleEndpointWithCircuitBreaker() + throws Exception { + CapabilitiesImpl executionServerCapabilitiesImpl = new CapabilitiesImpl(EXEC_AND_CACHE_CAPS); + Server executionServer = + createFakeServer(EXECUTION_SERVER_NAME, executionServerCapabilitiesImpl); + executionServer.start(); + + try { + remoteOptions.remoteExecutor = EXECUTION_SERVER_NAME; + remoteOptions.circuitBreakerStrategy = RemoteOptions.CircuitBreakerStrategy.FAILURE; + + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); + + remoteModule.beforeCommand(env); + + assertThat(Thread.interrupted()).isFalse(); + assertThat(executionServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); + assertCircuitBreakerInstance(); + } finally { + executionServer.shutdownNow(); + executionServer.awaitTermination(); + } + } + + @Test + public void testVerifyCapabilities_cacheOnlyEndpointWithCircuitBreaker() throws Exception { + CapabilitiesImpl cacheServerCapabilitiesImpl = new CapabilitiesImpl(CACHE_ONLY_CAPS); + Server cacheServer = createFakeServer(CACHE_SERVER_NAME, cacheServerCapabilitiesImpl); + cacheServer.start(); + + try { + remoteOptions.remoteCache = CACHE_SERVER_NAME; + remoteOptions.circuitBreakerStrategy = RemoteOptions.CircuitBreakerStrategy.FAILURE; + + CommandEnvironment env = createTestCommandEnvironment(remoteModule, remoteOptions); + + remoteModule.beforeCommand(env); + + assertThat(Thread.interrupted()).isFalse(); + assertThat(cacheServerCapabilitiesImpl.getRequestCount()).isEqualTo(1); + assertCircuitBreakerInstance(); + } finally { + cacheServer.shutdownNow(); + cacheServer.awaitTermination(); + } + } + + private void assertCircuitBreakerInstance() { + RemoteActionContextProvider actionContextProvider = remoteModule.getActionContextProvider(); + assertThat(actionContextProvider).isNotNull(); + + Retrier.CircuitBreaker circuitBreaker; + if (actionContextProvider.getRemoteCache() != null) { + circuitBreaker = + ((GrpcCacheClient) actionContextProvider.getRemoteCache().cacheProtocol) + .getRetrier() + .getCircuitBreaker(); + } else if (actionContextProvider.getRemoteExecutionClient() != null) { + circuitBreaker = + ((GrpcRemoteExecutor) actionContextProvider.getRemoteExecutionClient()) + .getRetrier() + .getCircuitBreaker(); + } else { + // no remote cache or execution configured, circuitBreaker is null + return; + } + + if (remoteOptions.circuitBreakerStrategy == RemoteOptions.CircuitBreakerStrategy.FAILURE) { + assertThat(circuitBreaker).isInstanceOf(FailureCircuitBreaker.class); + } + if (remoteOptions.circuitBreakerStrategy == null) { + assertThat(circuitBreaker).isEqualTo(Retrier.ALLOW_ALL_CALLS); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java index a739309c21b2c1..7c30e1bf6eddc3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RetrierTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -93,7 +94,7 @@ public void retryShouldWork_failure() throws Exception { assertThat(e).hasMessageThat().isEqualTo("call failed"); assertThat(numCalls.get()).isEqualTo(3); - verify(alwaysOpen, times(3)).recordFailure(); + verify(alwaysOpen, times(3)).recordFailure(any(Exception.class)); verify(alwaysOpen, never()).recordSuccess(); } @@ -117,7 +118,7 @@ public void retryShouldWorkNoRetries_failure() throws Exception { assertThat(e).hasMessageThat().isEqualTo("call failed"); assertThat(numCalls.get()).isEqualTo(1); - verify(alwaysOpen, times(1)).recordFailure(); + verify(alwaysOpen, times(1)).recordFailure(e); verify(alwaysOpen, never()).recordSuccess(); } @@ -138,7 +139,7 @@ public void retryShouldWork_success() throws Exception { }); assertThat(val).isEqualTo(1); - verify(alwaysOpen, times(2)).recordFailure(); + verify(alwaysOpen, times(2)).recordFailure(any(Exception.class)); verify(alwaysOpen, times(1)).recordSuccess(); } @@ -350,7 +351,7 @@ public synchronized State state() { } @Override - public synchronized void recordFailure() { + public synchronized void recordFailure(Exception e) { consecutiveFailures++; if (consecutiveFailures >= maxConsecutiveFailures) { state = State.REJECT_CALLS; diff --git a/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD b/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD new file mode 100644 index 00000000000000..987a81bbcbadaa --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/BUILD @@ -0,0 +1,31 @@ +load("@rules_java//java:defs.bzl", "java_test") + +package( + default_applicable_licenses = ["//:license"], + default_testonly = 1, + default_visibility = ["//src:__subpackages__"], +) + +filegroup( + name = "srcs", + testonly = 0, + srcs = glob(["**"]), + visibility = ["//src:__subpackages__"], +) + +java_test( + name = "circuitbreaker", + srcs = glob(["*.java"]), + test_class = "com.google.devtools.build.lib.AllTests", + deps = [ + "//src/main/java/com/google/devtools/build/lib/remote", + "//src/main/java/com/google/devtools/build/lib/remote/circuitbreaker", + "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", + "//src/main/java/com/google/devtools/build/lib/remote/options", + "//src/main/java/com/google/devtools/common/options", + "//src/test/java/com/google/devtools/build/lib:test_runner", + "//third_party:junit4", + "//third_party:truth", + "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", + ], +) diff --git a/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactoryTest.java b/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactoryTest.java new file mode 100644 index 00000000000000..51baee1162f2eb --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/CircuitBreakerFactoryTest.java @@ -0,0 +1,44 @@ +// 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.circuitbreaker; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.devtools.build.lib.remote.Retrier; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOptions.CircuitBreakerStrategy; +import com.google.devtools.common.options.Options; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link CircuitBreakerFactory}. */ +@RunWith(JUnit4.class) +public class CircuitBreakerFactoryTest { + @Test + public void testCreateCircuitBreaker_failureStrategy() { + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + remoteOptions.circuitBreakerStrategy = CircuitBreakerStrategy.FAILURE; + + assertThat(CircuitBreakerFactory.createCircuitBreaker(remoteOptions)) + .isInstanceOf(FailureCircuitBreaker.class); + } + + @Test + public void testCreateCircuitBreaker_nullStrategy() { + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + assertThat(CircuitBreakerFactory.createCircuitBreaker(remoteOptions)) + .isEqualTo(Retrier.ALLOW_ALL_CALLS); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreakerTest.java b/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreakerTest.java new file mode 100644 index 00000000000000..2d00a8e0e816ab --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/circuitbreaker/FailureCircuitBreakerTest.java @@ -0,0 +1,68 @@ +// 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.circuitbreaker; + +import static com.google.common.truth.Truth.assertThat; + +import build.bazel.remote.execution.v2.Digest; +import com.google.devtools.build.lib.remote.Retrier.CircuitBreaker.State; +import com.google.devtools.build.lib.remote.common.CacheNotFoundException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class FailureCircuitBreakerTest { + + @Test + public void testRecordFailure() throws InterruptedException { + final int failureThreshold = 10; + final int windowInterval = 100; + FailureCircuitBreaker failureCircuitBreaker = + new FailureCircuitBreaker(failureThreshold, windowInterval); + + List listOfExceptionThrownOnFailure = new ArrayList<>(); + for (int index = 0; index < failureThreshold; index++) { + listOfExceptionThrownOnFailure.add(new Exception()); + } + for (int index = 0; index < failureThreshold * 9; index++) { + listOfExceptionThrownOnFailure.add(new CacheNotFoundException(Digest.newBuilder().build())); + } + + Collections.shuffle(listOfExceptionThrownOnFailure); + + // make calls equals to threshold number of not ignored failure calls in parallel. + listOfExceptionThrownOnFailure.stream() + .parallel() + .forEach(failureCircuitBreaker::recordFailure); + assertThat(failureCircuitBreaker.state()).isEqualTo(State.ACCEPT_CALLS); + + // Sleep for windowInterval + 1ms. + Thread.sleep(windowInterval + 1 /*to compensate any delay*/); + + // make calls equals to threshold number of not ignored failure calls in parallel. + listOfExceptionThrownOnFailure.stream() + .parallel() + .forEach(failureCircuitBreaker::recordFailure); + assertThat(failureCircuitBreaker.state()).isEqualTo(State.ACCEPT_CALLS); + + // Sleep for less than windowInterval. + Thread.sleep(windowInterval - 5); + failureCircuitBreaker.recordFailure(new Exception()); + assertThat(failureCircuitBreaker.state()).isEqualTo(State.REJECT_CALLS); + } +} From 40d3998a45326ff8bf61d971710ea3e6de8c0261 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 2 Jun 2023 20:42:25 +0200 Subject: [PATCH 15/18] Actually check `TEST_SHARD_STATUS_FILE` has been touched (#18418) Adds the missing check that a test runner running a sharded test actually declares support for sharding by generating the `TEST_SHARD_STATUS_FILE`. Previously, if it didn't, each shard would silently run all tests. This new behavior is guarded by the new `--incompatible_check_sharding_support` flag. Closes #18236. PiperOrigin-RevId: 530259354 Change-Id: If3b01b2c796e66ad7a77e542145efe3ab412eae9 Co-authored-by: keertk Co-authored-by: Yun Peng --- site/en/reference/test-encyclopedia.md | 5 +-- .../lib/analysis/test/TestConfiguration.java | 17 +++++++++ .../lib/analysis/test/TestRunnerAction.java | 4 +++ .../lib/exec/StandaloneTestStrategy.java | 19 ++++++++++ src/test/py/bazel/bazel_windows_test.py | 35 +++++++++++++++++++ src/test/shell/bazel/bazel_test_test.sh | 22 ++++++++++++ .../bazel/remote/remote_execution_test.sh | 26 ++++++++++++++ 7 files changed, 126 insertions(+), 2 deletions(-) diff --git a/site/en/reference/test-encyclopedia.md b/site/en/reference/test-encyclopedia.md index ca86c73d9d8ec2..8d8fac972f15e6 100644 --- a/site/en/reference/test-encyclopedia.md +++ b/site/en/reference/test-encyclopedia.md @@ -223,8 +223,9 @@ index, beginning at 0. Runners use this information to select which tests to run - for example, using a round-robin strategy. Not all test runners support sharding. If a runner supports sharding, it must create or update the last modified date of the file specified by -[`TEST_SHARD_STATUS_FILE`](#initial-conditions). Otherwise, Bazel assumes it -does not support sharding and will not launch additional runners. +[`TEST_SHARD_STATUS_FILE`](#initial-conditions). Otherwise, if +[`--incompatible_check_sharding_support`](/reference/command-line-reference#flag--incompatible_check_sharding_support) +is enabled, Bazel will fail the test if it is sharded. ## Initial conditions {:#initial-conditions} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java index 7da18906591961..df7ea2ba3da01b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java @@ -280,6 +280,19 @@ public static class TestOptions extends FragmentOptions { help = "If true, undeclared test outputs will be archived in a zip file.") public boolean zipUndeclaredTestOutputs; + @Option( + name = "incompatible_check_sharding_support", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "If true, Bazel will fail a sharded test if the test runner does not indicate that it " + + "supports sharding by touching the file at the path in TEST_SHARD_STATUS_FILE. " + + "If false, a test runner that does not support sharding will lead to all tests " + + "running in each shard.") + public boolean checkShardingSupport; + @Override public FragmentOptions getHost() { // Options here are either: @@ -380,6 +393,10 @@ public boolean getZipUndeclaredTestOutputs() { return options.zipUndeclaredTestOutputs; } + public boolean checkShardingSupport() { + return options.checkShardingSupport; + } + /** * Option converter that han handle two styles of value for "--runs_per_test": * diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index eadd82927a2ddd..53482bca7bbce4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -336,6 +336,10 @@ public boolean showsOutputUnconditionally() { return true; } + public boolean checkShardingSupport() { + return testConfiguration.checkShardingSupport(); + } + public List getSpawnOutputs() { final List outputs = new ArrayList<>(); outputs.add(ActionInputHelper.fromPath(getXmlOutputPath())); 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 d051842e4dce0c..029ce7ae6ff136 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 @@ -747,6 +747,25 @@ public TestAttemptContinuation execute() } long endTimeMillis = actionExecutionContext.getClock().currentTimeMillis(); + if (testAction.isSharded()) { + if (testAction.checkShardingSupport() + && !actionExecutionContext + .getPathResolver() + .convertPath(resolvedPaths.getTestShard()) + .exists()) { + TestExecException e = + createTestExecException( + TestAction.Code.LOCAL_TEST_PREREQ_UNMET, + "Sharding requested, but the test runner did not advertise support for it by " + + "touching TEST_SHARD_STATUS_FILE. Either remove the 'shard_count' attribute, " + + "use a test runner that supports sharding or temporarily disable this check " + + "via --noincompatible_check_sharding_support."); + closeSuppressed(e, streamed); + closeSuppressed(e, fileOutErr); + throw e; + } + } + // SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there // may be additional entries due to tree artifact handling). SpawnResult primaryResult = spawnResults.get(0); diff --git a/src/test/py/bazel/bazel_windows_test.py b/src/test/py/bazel/bazel_windows_test.py index d9dc71e8c31aad..2f8af03a7bfce9 100644 --- a/src/test/py/bazel/bazel_windows_test.py +++ b/src/test/py/bazel/bazel_windows_test.py @@ -451,6 +451,41 @@ def testZipUndeclaredTestOutputs(self): self.assertTrue(os.path.exists(output_file)) self.assertFalse(os.path.exists(output_zip)) + def testTestShardStatusFile(self): + self.CreateWorkspaceWithDefaultRepos('WORKSPACE') + self.ScratchFile( + 'BUILD', + [ + 'sh_test(', + ' name = "foo_test",', + ' srcs = ["foo.sh"],', + ' shard_count = 2,', + ')', + ], + ) + self.ScratchFile('foo.sh') + + exit_code, stdout, stderr = self.RunBazel( + ['test', '--incompatible_check_sharding_support', '//:foo_test'], + allow_failure=True, + ) + # Check for "tests failed" exit code + self.AssertExitCode(exit_code, 3, stderr, stdout) + self.assertTrue( + any( + 'Sharding requested, but the test runner did not advertise support' + ' for it by touching TEST_SHARD_STATUS_FILE.' + in line + for line in stderr + ) + ) + + self.ScratchFile('foo.sh', ['touch "$TEST_SHARD_STATUS_FILE"']) + + self.RunBazel( + ['test', '--incompatible_check_sharding_support', '//:foo_test'] + ) + if __name__ == '__main__': unittest.main() diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh index 709530a3ae0a84..4bee9e4a4178c4 100755 --- a/src/test/shell/bazel/bazel_test_test.sh +++ b/src/test/shell/bazel/bazel_test_test.sh @@ -997,4 +997,26 @@ EOF expect_log " BUILD +sh_test( + name = 'x', + srcs = ['x.sh'], + shard_count = 2, +) +EOF + touch x.sh + chmod +x x.sh + + bazel test \ + --incompatible_check_sharding_support \ + //:x &> $TEST_log && fail "expected failure" + expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE." + + echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh + bazel test \ + --incompatible_check_sharding_support \ + //:x &> $TEST_log || fail "expected success" +} + run_suite "bazel test tests" diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 3a7c1807e02875..754a8a7cd2ffa3 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3250,4 +3250,30 @@ function test_external_cc_binary_tool_with_dynamic_deps_sibling_repository_layou @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed" } +function test_shard_status_file_checked_remote_download_minimal() { + cat <<'EOF' > BUILD +sh_test( + name = 'x', + srcs = ['x.sh'], + shard_count = 2, +) +EOF + touch x.sh + chmod +x x.sh + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --incompatible_check_sharding_support \ + --remote_download_minimal \ + //:x &> $TEST_log && fail "expected failure" + expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE." + + echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --incompatible_check_sharding_support \ + --remote_download_minimal \ + //:x &> $TEST_log || fail "expected success" +} + run_suite "Remote execution and remote cache tests" From 7bdd173605514eb15ee69db79a5418b701d6c10d Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Mon, 5 Jun 2023 03:06:50 -0700 Subject: [PATCH 16/18] Ignore hash string casing (#18414) Fixes this crash when e.g. the `sha256` attribute is passed an upper case string: ``` Caused by: java.lang.IllegalArgumentException: Illegal hexadecimal character: D at com.google.common.hash.HashCode.decode(HashCode.java:360) at com.google.common.hash.HashCode.fromString(HashCode.java:346) at com.google.devtools.build.lib.bazel.repository.downloader.Checksum.fromString(Checksum.java:47) at com.google.devtools.build.lib.bazel.repository.starlark.StarlarkBaseExternalContext.validateChecksum(StarlarkBaseExternalContext.java:302) at com.google.devtools.build.lib.bazel.repository.starlark.StarlarkBaseExternalContext.downloadAndExtract(StarlarkBaseExternalContext.java:650) ... ``` Fixes #18291 Closes #18305. PiperOrigin-RevId: 529601921 Change-Id: I75deee47bcac80ee81603591c22f43d013ba0c29 Co-authored-by: Fabian Meumertzheim --- .../bazel/repository/downloader/Checksum.java | 3 ++- .../shell/bazel/external_integration_test.sh | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Checksum.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Checksum.java index a63e1b4f4a5bf9..09b3aeea884ac0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Checksum.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/Checksum.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.bazel.repository.downloader; +import com.google.common.base.Ascii; import com.google.common.hash.HashCode; import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType; import java.util.Base64; @@ -44,7 +45,7 @@ public static Checksum fromString(KeyType keyType, String hash) throws InvalidCh if (!keyType.isValid(hash)) { throw new InvalidChecksumException(keyType, hash); } - return new Checksum(keyType, HashCode.fromString(hash)); + return new Checksum(keyType, HashCode.fromString(Ascii.toLowerCase(hash))); } /** Constructs a new Checksum from a hash in Subresource Integrity format. */ diff --git a/src/test/shell/bazel/external_integration_test.sh b/src/test/shell/bazel/external_integration_test.sh index 7216fd1412760a..a6092e9110164b 100755 --- a/src/test/shell/bazel/external_integration_test.sh +++ b/src/test/shell/bazel/external_integration_test.sh @@ -221,6 +221,22 @@ EOF assert_contains "test content" "${base_external_path}/test_dir/test_file" } +function test_http_archive_upper_case_sha() { + cat >> $(create_workspace_with_default_repos WORKSPACE) <> $(create_workspace_with_default_repos WORKSPACE) < Date: Mon, 5 Jun 2023 11:00:38 -0700 Subject: [PATCH 17/18] Error if repository name isn't supplied (#18425) Currently, this results in a server crash along the lines of: ``` FATAL: bazel crashed due to an internal error. Printing stack trace: java.lang.RuntimeException: Unrecoverable error while evaluating node '[/redacted]/[WORKSPACE.bazel], 1' (requested by nodes 'com.google.devtools.build.lib.skyframe.ExternalPackageFunction$$Lambda$231/0x0000000800df7998@34f133c') at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:642) at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:382) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at java.base/java.lang.Thread.run(Thread.java:833) Caused by: net.starlark.java.eval.Starlark$UncheckedEvalException: NullPointerException thrown during Starlark evaluation (WORKSPACE) at .repository_rule(:0) at .(/redacted/WORKSPACE.bazel:3) Caused by: java.lang.NullPointerException: Cannot invoke "String.isEmpty()" because "name" is null at com.google.devtools.build.lib.cmdline.RepositoryName.create(RepositoryName.java:71) at com.google.devtools.build.lib.packages.WorkspaceFactoryHelper.addMainRepoEntry(WorkspaceFactoryHelper.java:106) at com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule$RepositoryRuleFunction.createRuleLegacy(StarlarkRepositoryModule.java:228) at com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule$RepositoryRuleFunction.call(StarlarkRepositoryModule.java:185) ``` Closes #18335. PiperOrigin-RevId: 530577557 Change-Id: Ic402e8fabac180aaa2da19cd3201b9fb2671dccb Co-authored-by: Daniel Wagner-Hall --- .../bazel/repository/starlark/StarlarkRepositoryModule.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index e3e58c754fb77c..3dd3e9d8f6d67e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -225,6 +225,9 @@ private Object createRuleLegacy(StarlarkThread thread, Dict kwar // TODO(adonovan): is this cast safe? Check. String name = (String) kwargs.get("name"); + if (name == null) { + throw Starlark.errorf("argument 'name' is required"); + } WorkspaceFactoryHelper.addMainRepoEntry(packageBuilder, name, thread.getSemantics()); WorkspaceFactoryHelper.addRepoMappings(packageBuilder, kwargs, name); Rule rule = From 2a026e6b382eea299963b4bbd0ee94aa3fba0db0 Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Tue, 6 Jun 2023 08:53:10 -0700 Subject: [PATCH 18/18] Track repo rule label attributes after the first non-existent one (#18412) Even with this commit, the fact that a particular label in a repository rule's label attributes does not resolve to a regular file is not tracked, which means that there is still a potential for incorrect incremental fetches. Work towards #13441 Closes #18352. PiperOrigin-RevId: 531150549 Change-Id: I086e4813ca88a3f49cde77d9be20adaaf954834b Co-authored-by: Fabian Meumertzheim Co-authored-by: keertk --- .../starlark/StarlarkRepositoryContext.java | 23 ++++++++-- .../starlark/StarlarkRepositoryFunction.java | 5 --- .../shell/bazel/starlark_prefetching_test.sh | 44 +++++++++++++++++++ 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 2a8b59d80e5d25..dd8ad14c9b9fc8 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.packages.StructProvider; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.repository.RepositoryFetchProgress; +import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException; import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException; import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper; import com.google.devtools.build.lib.runtime.ProcessWrapper; @@ -516,26 +517,42 @@ public String toString() { */ // TODO(wyv): somehow migrate this to the base context too. public void enforceLabelAttributes() throws EvalException, InterruptedException { + // TODO: If a labels fails to resolve to an existing regular file, we do not add a dependency on + // that fact - if the file is created later or changes its type, it will not trigger a rerun of + // the repository function. StructImpl attr = getAttr(); for (String name : attr.getFieldNames()) { Object value = attr.getValue(name); if (value instanceof Label) { - getPathFromLabel((Label) value); + dependOnLabelIgnoringErrors((Label) value); } if (value instanceof Sequence) { for (Object entry : (Sequence) value) { if (entry instanceof Label) { - getPathFromLabel((Label) entry); + dependOnLabelIgnoringErrors((Label) entry); } } } if (value instanceof Dict) { for (Object entry : ((Dict) value).keySet()) { if (entry instanceof Label) { - getPathFromLabel((Label) entry); + dependOnLabelIgnoringErrors((Label) entry); } } } } } + + private void dependOnLabelIgnoringErrors(Label label) + throws InterruptedException, NeedsSkyframeRestartException { + try { + getPathFromLabel(label); + } catch (NeedsSkyframeRestartException e) { + throw e; + } catch (EvalException e) { + // EvalExceptions indicate labels not referring to existing files. This is fine, + // as long as they are never resolved to files in the execution of the rule; we allow + // non-strict rules. + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 83772d1fd28194..140697445739bb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -198,11 +198,6 @@ public RepositoryDirectoryValue.Builder fetch( } catch (NeedsSkyframeRestartException e) { // Missing values are expected; just restart before we actually start the rule return null; - } catch (EvalException e) { - // EvalExceptions indicate labels not referring to existing files. This is fine, - // as long as they are never resolved to files in the execution of the rule; we allow - // non-strict rules. So now we have to start evaluating the actual rule, even if that - // means the rule might get restarted for legitimate reasons. } // This rule is mainly executed for its side effect. Nevertheless, the return value is diff --git a/src/test/shell/bazel/starlark_prefetching_test.sh b/src/test/shell/bazel/starlark_prefetching_test.sh index f12c7426c617bd..69e5bfda740c30 100755 --- a/src/test/shell/bazel/starlark_prefetching_test.sh +++ b/src/test/shell/bazel/starlark_prefetching_test.sh @@ -227,4 +227,48 @@ EOF bazel build @ext//:foo || fail "expected success" } +# Regression test for https://github.com/bazelbuild/bazel/issues/13441 +function test_files_tracked_with_non_existing_files() { + cat > rules.bzl <<'EOF' +def _repo_impl(ctx): + ctx.symlink(ctx.path(Label("@//:WORKSPACE")).dirname, "link") + print("b.txt: " + ctx.read("link/b.txt")) + print("c.txt: " + ctx.read("link/c.txt")) + + ctx.file("BUILD") + ctx.file("WORKSPACE") + +repo = repository_rule( + _repo_impl, + attrs = {"_files": attr.label_list( + default = [ + Label("@//:a.txt"), + Label("@//:b.txt"), + Label("@//:c.txt"), + ], + )}, +) +EOF + + cat > WORKSPACE <<'EOF' +load(":rules.bzl", "repo") +repo(name = "ext") +EOF + touch BUILD + + # a.txt is intentionally not created + echo "bbbb" > b.txt + echo "cccc" > c.txt + + # The missing file dependency is tolerated. + bazel build @ext//:all &> "$TEST_log" || fail "Expected repository rule to build" + expect_log "b.txt: bbbb" + expect_log "c.txt: cccc" + + echo "not_cccc" > c.txt + bazel build @ext//:all &> "$TEST_log" || fail "Expected repository rule to build" + expect_log "b.txt: bbbb" + expect_log "c.txt: not_cccc" +} + run_suite "Starlark repo prefetching tests"