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/scripts/release/relnotes.py b/scripts/release/relnotes.py index 66581cff627514..fcdfb86ef29135 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] @@ -205,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}.")) 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/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}, 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 { 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/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/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/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/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 = 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/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/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/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/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..97abfd36eefbf2 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: * @@ -243,4 +252,8 @@ public void close() { } channel.release(); } + + RemoteRetrier getRetrier() { + return this.retrier; + } } 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/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 417ec5bf104f94..2795b133a1224f 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. // @@ -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 = @@ -598,19 +600,24 @@ 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); + executionCapabilities, + remoteOptions, + execChannel.retain(), + callCredentialsProvider, + execRetrier); } else { RemoteRetrier execRetrier = new RemoteRetrier( remoteOptions, RemoteRetrier.RETRIABLE_GRPC_EXEC_ERRORS, retryScheduler, - Retrier.ALLOW_ALL_CALLS); + circuitBreaker); 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/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/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/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/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; 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/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( 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%)"); } } 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/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(); 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/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/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); 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); + } +} 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); } 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/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) < 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 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" 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/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" diff --git a/tools/test/test-setup.sh b/tools/test/test-setup.sh index 952754c82dd62d..7d1cbd79d0b47a 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}" @@ -322,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=$! 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;