From e44dc507f78e0614650484c37403d1bec76b1092 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Fri, 26 Jan 2024 06:27:12 -0800 Subject: [PATCH] [7.1.0] Distinguish the disk and remote caches in the action progress status. Previously, they were both displayed as `remote-cache`; there's now a separate `disk-cache` form. If a combined cache is used, one or both forms will appear, depending on which caches were looked up. As a result, the progress status reporting is moved to the individual cache implementations. While this is kind of unfortunate from an architectural standpoint, it's likely the best we can do until we recast cache lookups as spawn strategies (see #19904). Closes #20935. PiperOrigin-RevId: 601748051 Change-Id: I03710219973c95d4fca999d931b3513f6d240d94 --- .../build/lib/remote/GrpcCacheClient.java | 8 +++ .../lib/remote/RemoteExecutionService.java | 2 +- .../build/lib/remote/RemoteSpawnCache.java | 5 -- .../common/RemoteActionExecutionContext.java | 68 ++++++++++++++---- .../devtools/build/lib/remote/disk/BUILD | 1 + .../lib/remote/disk/DiskCacheClient.java | 9 +++ .../devtools/build/lib/remote/http/BUILD | 1 + .../lib/remote/http/HttpCacheClient.java | 8 +++ .../google/devtools/build/lib/remote/BUILD | 1 + .../build/lib/remote/GrpcCacheClientTest.java | 33 ++++++++- .../build/lib/remote/RemoteCacheTest.java | 71 +++++++++++++------ .../lib/remote/RemoteSpawnCacheTest.java | 13 ---- .../devtools/build/lib/remote/disk/BUILD | 33 +++++++++ .../lib/remote/disk/DiskCacheClientTest.java | 68 ++++++++++++++++++ .../devtools/build/lib/remote/http/BUILD | 2 + .../lib/remote/http/HttpCacheClientTest.java | 31 ++++++++ 16 files changed, 297 insertions(+), 57 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/remote/disk/BUILD create mode 100644 src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java 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 6d142bf5df60cb..3b3e1051dce7b0 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 @@ -48,6 +48,7 @@ import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; import com.google.devtools.build.lib.remote.RemoteRetrier.ProgressiveBackoff; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.MissingDigestsFinder; @@ -81,6 +82,9 @@ public class GrpcCacheClient implements RemoteCacheClient, MissingDigestsFinder { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT = + SpawnCheckingCacheEvent.create("remote-cache"); + private final CallCredentialsProvider callCredentialsProvider; private final ReferenceCountedChannel channel; private final RemoteOptions options; @@ -274,6 +278,10 @@ public ListenableFuture getAuthority() { @Override public ListenableFuture downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { + if (context.getSpawnExecutionContext() != null) { + context.getSpawnExecutionContext().report(SPAWN_CHECKING_CACHE_EVENT); + } + GetActionResultRequest request = GetActionResultRequest.newBuilder() .setInstanceName(options.remoteInstanceName) 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 e6890842321789..7a6dcb47253d78 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 @@ -605,7 +605,7 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner()); RemoteActionExecutionContext remoteActionExecutionContext = RemoteActionExecutionContext.create( - spawn, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn)); + spawn, context, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn)); return new RemoteAction( spawn, diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index bee863a5b3b4ec..ad0c51c65e37c9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -31,7 +31,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.exec.SpawnCache; -import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; @@ -51,9 +50,6 @@ @ThreadSafe // If the RemoteActionCache implementation is thread-safe. final class RemoteSpawnCache implements SpawnCache { - private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT = - SpawnCheckingCacheEvent.create("remote-cache"); - private final Path execRoot; private final RemoteOptions options; private final RemoteExecutionService remoteExecutionService; @@ -101,7 +97,6 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) Profiler prof = Profiler.instance(); if (shouldAcceptCachedResult) { - context.report(SPAWN_CHECKING_CACHE_EVENT); // Metadata will be available in context.current() until we detach. // This is done via a thread-local variable. try { diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java index 46a58e5927391b..fda0782ea15586 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteActionExecutionContext.java @@ -16,6 +16,8 @@ import build.bazel.remote.execution.v2.RequestMetadata; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.exec.SpawnRunner; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import javax.annotation.Nullable; /** A context that provide remote execution related information for executing an action remotely. */ @@ -61,27 +63,35 @@ public CachePolicy addRemoteCache() { } @Nullable private final Spawn spawn; + @Nullable private final SpawnExecutionContext spawnExecutionContext; private final RequestMetadata requestMetadata; private final NetworkTime networkTime; private final CachePolicy writeCachePolicy; private final CachePolicy readCachePolicy; private RemoteActionExecutionContext( - @Nullable Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) { - this.spawn = spawn; - this.requestMetadata = requestMetadata; - this.networkTime = networkTime; - this.writeCachePolicy = CachePolicy.ANY_CACHE; - this.readCachePolicy = CachePolicy.ANY_CACHE; + @Nullable Spawn spawn, + @Nullable SpawnRunner.SpawnExecutionContext spawnExecutionContext, + RequestMetadata requestMetadata, + NetworkTime networkTime) { + this( + spawn, + spawnExecutionContext, + requestMetadata, + networkTime, + CachePolicy.ANY_CACHE, + CachePolicy.ANY_CACHE); } private RemoteActionExecutionContext( @Nullable Spawn spawn, + @Nullable SpawnExecutionContext spawnExecutionContext, RequestMetadata requestMetadata, NetworkTime networkTime, CachePolicy writeCachePolicy, CachePolicy readCachePolicy) { this.spawn = spawn; + this.spawnExecutionContext = spawnExecutionContext; this.requestMetadata = requestMetadata; this.networkTime = networkTime; this.writeCachePolicy = writeCachePolicy; @@ -90,20 +100,42 @@ private RemoteActionExecutionContext( public RemoteActionExecutionContext withWriteCachePolicy(CachePolicy writeCachePolicy) { return new RemoteActionExecutionContext( - spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy); + spawn, + spawnExecutionContext, + requestMetadata, + networkTime, + writeCachePolicy, + readCachePolicy); } public RemoteActionExecutionContext withReadCachePolicy(CachePolicy readCachePolicy) { return new RemoteActionExecutionContext( - spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy); + spawn, + spawnExecutionContext, + requestMetadata, + networkTime, + writeCachePolicy, + readCachePolicy); } - /** Returns the {@link Spawn} of the action being executed or {@code null}. */ + /** + * Returns the {@link Spawn} of the {@link RemoteAction} being executed, or {@code null} if it has + * no associated {@link Spawn}. + */ @Nullable public Spawn getSpawn() { return spawn; } + /** + * Returns the {@link SpawnExecutionContext} of the {@link RemoteAction} being executed, or {@code + * null} if it has no associated {@link Spawn}. + */ + @Nullable + public SpawnExecutionContext getSpawnExecutionContext() { + return spawnExecutionContext; + } + /** Returns the {@link RequestMetadata} for the action being executed. */ public RequestMetadata getRequestMetadata() { return requestMetadata; @@ -137,7 +169,8 @@ public CachePolicy getReadCachePolicy() { /** Creates a {@link RemoteActionExecutionContext} with given {@link RequestMetadata}. */ public static RemoteActionExecutionContext create(RequestMetadata metadata) { - return new RemoteActionExecutionContext(/*spawn=*/ null, metadata, new NetworkTime()); + return new RemoteActionExecutionContext( + /* spawn= */ null, /* spawnExecutionContext= */ null, metadata, new NetworkTime()); } /** @@ -145,16 +178,23 @@ public static RemoteActionExecutionContext create(RequestMetadata metadata) { * RequestMetadata}. */ public static RemoteActionExecutionContext create( - @Nullable Spawn spawn, RequestMetadata metadata) { - return new RemoteActionExecutionContext(spawn, metadata, new NetworkTime()); + Spawn spawn, SpawnExecutionContext spawnExecutionContext, RequestMetadata metadata) { + return new RemoteActionExecutionContext( + spawn, spawnExecutionContext, metadata, new NetworkTime()); } public static RemoteActionExecutionContext create( - @Nullable Spawn spawn, + Spawn spawn, + SpawnExecutionContext spawnExecutionContext, RequestMetadata requestMetadata, CachePolicy writeCachePolicy, CachePolicy readCachePolicy) { return new RemoteActionExecutionContext( - spawn, requestMetadata, new NetworkTime(), writeCachePolicy, readCachePolicy); + spawn, + spawnExecutionContext, + requestMetadata, + new NetworkTime(), + writeCachePolicy, + readCachePolicy); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD b/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD index 4647fbb694d1a7..57271f3b0a45e2 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD @@ -15,6 +15,7 @@ java_library( name = "disk", srcs = glob(["*.java"]), deps = [ + "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/remote:store", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index dd3de658bb8ce9..cc54df8ae6783e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -30,6 +30,7 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; import com.google.devtools.build.lib.remote.Store; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; @@ -60,6 +61,10 @@ * safe to trim the cache at the same time a Bazel process is accessing it. */ public class DiskCacheClient implements RemoteCacheClient { + + private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT = + SpawnCheckingCacheEvent.create("disk-cache"); + private final Path root; private final boolean verifyDownloads; private final DigestUtil digestUtil; @@ -222,6 +227,10 @@ public ListenableFuture getAuthority() { @Override public ListenableFuture downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { + if (context.getSpawnExecutionContext() != null) { + context.getSpawnExecutionContext().report(SPAWN_CHECKING_CACHE_EVENT); + } + return Futures.transformAsync( Utils.downloadAsActionResult(actionKey, (digest, out) -> download(digest, out, Store.AC)), actionResult -> { diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/BUILD b/src/main/java/com/google/devtools/build/lib/remote/http/BUILD index 18409a937c5c38..525889da598839 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/http/BUILD @@ -21,6 +21,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info", "//src/main/java/com/google/devtools/build/lib/authandtls", + "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/remote:Retrier", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception", diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java index 608eae019e3d7e..ba9a8865d0e130 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java @@ -28,6 +28,7 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; +import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; import com.google.devtools.build.lib.remote.RemoteRetrier; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; @@ -122,6 +123,9 @@ public final class HttpCacheClient implements RemoteCacheClient { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + private static final SpawnCheckingCacheEvent SPAWN_CHECKING_CACHE_EVENT = + SpawnCheckingCacheEvent.create("remote-cache"); + public static final String AC_PREFIX = "ac/"; public static final String CAS_PREFIX = "cas/"; private static final Pattern INVALID_TOKEN_ERROR = @@ -617,6 +621,10 @@ public ListenableFuture getAuthority() { @Override public ListenableFuture downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { + if (context.getSpawnExecutionContext() != null) { + context.getSpawnExecutionContext().report(SPAWN_CHECKING_CACHE_EVENT); + } + return Futures.transform( retrier.executeAsync( () -> 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 9d52a5b9410c90..cfa1be5178d7d4 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -11,6 +11,7 @@ filegroup( testonly = 0, srcs = glob(["**"]) + [ "//src/test/java/com/google/devtools/build/lib/remote/circuitbreaker:srcs", + "//src/test/java/com/google/devtools/build/lib/remote/disk:srcs", "//src/test/java/com/google/devtools/build/lib/remote/downloader:srcs", "//src/test/java/com/google/devtools/build/lib/remote/grpc:srcs", "//src/test/java/com/google/devtools/build/lib/remote/http:srcs", diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 299224b1b38973..bbaad1bd15ac1a 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -22,7 +22,9 @@ import static org.mockito.AdditionalAnswers.answerVoid; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionCacheGrpc.ActionCacheImplBase; @@ -58,6 +60,7 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ArtifactPathResolver; +import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; @@ -65,6 +68,8 @@ import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; import com.google.devtools.build.lib.remote.Retrier.Backoff; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; @@ -257,7 +262,9 @@ public final void setUp() throws Exception { RequestMetadata metadata = TracingMetadataUtils.buildMetadata( "none", "none", Digest.getDefaultInstance().getHash(), null); - context = RemoteActionExecutionContext.create(metadata); + context = + RemoteActionExecutionContext.create( + mock(Spawn.class), mock(SpawnExecutionContext.class), metadata); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); } @@ -272,6 +279,30 @@ public void tearDown() throws Exception { fakeServer.awaitTermination(); } + @Test + public void testSpawnCheckingCacheEvent() throws Exception { + GrpcCacheClient client = newClient(); + + serviceRegistry.addService( + new ActionCacheImplBase() { + @Override + public void getActionResult( + GetActionResultRequest request, StreamObserver responseObserver) { + responseObserver.onError(Status.NOT_FOUND.asRuntimeException()); + } + }); + + var unused = + getFromFuture( + client.downloadActionResult( + context, + DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")), + /* inlineOutErr= */ false)); + + verify(context.getSpawnExecutionContext()) + .report(SpawnCheckingCacheEvent.create("remote-cache")); + } + @Test public void testVirtualActionInputSupport() throws Exception { RemoteOptions options = Options.getDefaults(RemoteOptions.class); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTest.java index 9293e38164aac7..5a724a7f4a0296 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTest.java @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; @@ -92,7 +93,7 @@ public class RemoteCacheTest { @Rule public final RxNoGlobalErrorsRule rxNoGlobalErrorsRule = new RxNoGlobalErrorsRule(); - private RemoteActionExecutionContext context; + private RemoteActionExecutionContext remoteActionExecutionContext; private FileSystem fs; private Path execRoot; ArtifactRoot artifactRoot; @@ -116,7 +117,9 @@ public void setUp() throws Exception { /* inputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), /* outputs= */ ImmutableSet.of(), ResourceSet.ZERO); - context = RemoteActionExecutionContext.create(spawn, metadata); + SpawnExecutionContext spawnExecutionContext = mock(SpawnExecutionContext.class); + remoteActionExecutionContext = + RemoteActionExecutionContext.create(spawn, spawnExecutionContext, metadata); fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); execRoot = fs.getPath("/execroot/main"); execRoot.createDirectoryAndParents(); @@ -142,10 +145,11 @@ public void testDownloadEmptyBlobAndFile() throws Exception { Digest emptyDigest = digestUtil.compute(new byte[0]); // act and assert - assertThat(getFromFuture(remoteCache.downloadBlob(context, emptyDigest))).isEmpty(); + assertThat(getFromFuture(remoteCache.downloadBlob(remoteActionExecutionContext, emptyDigest))) + .isEmpty(); try (OutputStream out = file.getOutputStream()) { - getFromFuture(remoteCache.downloadFile(context, file, emptyDigest)); + getFromFuture(remoteCache.downloadFile(remoteActionExecutionContext, file, emptyDigest)); } assertThat(file.exists()).isTrue(); assertThat(file.getFileSize()).isEqualTo(0); @@ -165,7 +169,8 @@ public void downloadFile_cancelled_cancelDownload() throws Exception { Path file = execRoot.getRelative("file"); // act - ListenableFuture download = remoteCache.downloadFile(context, file, digest); + ListenableFuture download = + remoteCache.downloadFile(remoteActionExecutionContext, file, digest); download.cancel(/* mayInterruptIfRunning= */ true); // assert @@ -184,7 +189,7 @@ public void downloadOutErr_empty_doNotPerformDownload() throws Exception { waitForBulkTransfer( remoteCache.downloadOutErr( - context, + remoteActionExecutionContext, result.build(), new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))), true); @@ -212,7 +217,7 @@ public void testDownloadFileWithSymlinkTemplate() throws Exception { RemoteCache remoteCache = new InMemoryRemoteCache(cas, options, digestUtil); // act - getFromFuture(remoteCache.downloadFile(context, file, helloDigest)); + getFromFuture(remoteCache.downloadFile(remoteActionExecutionContext, file, helloDigest)); // assert assertThat(file.isSymbolicLink()).isTrue(); @@ -229,12 +234,19 @@ public void upload_emptyBlobAndFile_doNotPerformUpload() throws Exception { Digest emptyDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), ""); Path file = execRoot.getRelative("file"); - getFromFuture(remoteCache.uploadBlob(context, emptyDigest, ByteString.EMPTY)); - assertThat(getFromFuture(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))) + getFromFuture( + remoteCache.uploadBlob(remoteActionExecutionContext, emptyDigest, ByteString.EMPTY)); + assertThat( + getFromFuture( + remoteCache.findMissingDigests( + remoteActionExecutionContext, ImmutableSet.of(emptyDigest)))) .containsExactly(emptyDigest); - getFromFuture(remoteCache.uploadFile(context, emptyDigest, file)); - assertThat(getFromFuture(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))) + getFromFuture(remoteCache.uploadFile(remoteActionExecutionContext, emptyDigest, file)); + assertThat( + getFromFuture( + remoteCache.findMissingDigests( + remoteActionExecutionContext, ImmutableSet.of(emptyDigest)))) .containsExactly(emptyDigest); } @@ -254,8 +266,8 @@ public void upload_deduplicationWorks() throws IOException { Digest digest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), "content"); Path file = execRoot.getRelative("file"); - remoteCache.uploadFile(context, digest, file); - remoteCache.uploadFile(context, digest, file); + remoteCache.uploadFile(remoteActionExecutionContext, digest, file); + remoteCache.uploadFile(remoteActionExecutionContext, digest, file); assertThat(times.get()).isEqualTo(1); } @@ -286,20 +298,26 @@ public void upload_failedUploads_doNotDeduplicate() throws Exception { RemoteCache remoteCache = newRemoteCache(remoteCacheClient); Digest digest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), "content"); Path file = execRoot.getRelative("file"); - assertThat(getFromFuture(remoteCache.findMissingDigests(context, ImmutableList.of(digest)))) + assertThat( + getFromFuture( + remoteCache.findMissingDigests( + remoteActionExecutionContext, ImmutableList.of(digest)))) .containsExactly(digest); Exception thrown = null; try { - getFromFuture(remoteCache.uploadFile(context, digest, file)); + getFromFuture(remoteCache.uploadFile(remoteActionExecutionContext, digest, file)); } catch (IOException e) { thrown = e; } assertThat(thrown).isNotNull(); assertThat(thrown).isInstanceOf(IOException.class); - getFromFuture(remoteCache.uploadFile(context, digest, file)); + getFromFuture(remoteCache.uploadFile(remoteActionExecutionContext, digest, file)); - assertThat(getFromFuture(remoteCache.findMissingDigests(context, ImmutableList.of(digest)))) + assertThat( + getFromFuture( + remoteCache.findMissingDigests( + remoteActionExecutionContext, ImmutableList.of(digest)))) .isEmpty(); } @@ -342,7 +360,8 @@ public void ensureInputsPresent_interruptedDuringUploadBlobs_cancelInProgressUpl new Thread( () -> { try { - remoteCache.ensureInputsPresent(context, merkleTree, ImmutableMap.of(), false); + remoteCache.ensureInputsPresent( + remoteActionExecutionContext, merkleTree, ImmutableMap.of(), false); } catch (IOException | InterruptedException ignored) { // ignored } finally { @@ -416,7 +435,8 @@ public void ensureInputsPresent_interruptedDuringUploadBlobs_cancelInProgressUpl Runnable work = () -> { try { - remoteCache.ensureInputsPresent(context, merkleTree, ImmutableMap.of(), false); + remoteCache.ensureInputsPresent( + remoteActionExecutionContext, merkleTree, ImmutableMap.of(), false); } catch (IOException ignored) { // ignored } catch (InterruptedException e) { @@ -509,7 +529,8 @@ public void ensureInputsPresent_interruptedDuringUploadBlobs_cancelInProgressUpl new Thread( () -> { try { - remoteCache.ensureInputsPresent(context, merkleTree1, ImmutableMap.of(), false); + remoteCache.ensureInputsPresent( + remoteActionExecutionContext, merkleTree1, ImmutableMap.of(), false); } catch (IOException ignored) { // ignored } catch (InterruptedException e) { @@ -522,7 +543,8 @@ public void ensureInputsPresent_interruptedDuringUploadBlobs_cancelInProgressUpl new Thread( () -> { try { - remoteCache.ensureInputsPresent(context, merkleTree2, ImmutableMap.of(), false); + remoteCache.ensureInputsPresent( + remoteActionExecutionContext, merkleTree2, ImmutableMap.of(), false); } catch (InterruptedException | IOException ignored) { // ignored } finally { @@ -584,7 +606,9 @@ public void ensureInputsPresent_uploadFailed_propagateErrors() throws Exception IOException e = Assert.assertThrows( IOException.class, - () -> remoteCache.ensureInputsPresent(context, merkleTree, ImmutableMap.of(), false)); + () -> + remoteCache.ensureInputsPresent( + remoteActionExecutionContext, merkleTree, ImmutableMap.of(), false)); assertThat(e).hasMessageThat().contains("upload failed"); } @@ -600,7 +624,8 @@ public void shutdownNow_cancelInProgressUploads() throws Exception { Digest digest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), "content"); Path file = execRoot.getRelative("file"); - ListenableFuture upload = remoteCache.uploadFile(context, digest, file); + ListenableFuture upload = + remoteCache.uploadFile(remoteActionExecutionContext, digest, file); assertThat(remoteCache.casUploadCache.getInProgressTasks()).contains(digest); remoteCache.shutdownNow(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index e1d8d2187162bc..33c00c53c637ab 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -60,7 +60,6 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.exec.SpawnCache.CacheHandle; -import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; import com.google.devtools.build.lib.exec.SpawnInputExpander; import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; @@ -88,8 +87,6 @@ import com.google.devtools.common.options.Options; import java.io.IOException; import java.time.Duration; -import java.util.ArrayList; -import java.util.List; import java.util.SortedMap; import javax.annotation.Nullable; import org.junit.Before; @@ -122,7 +119,6 @@ public class RemoteSpawnCacheTest { private FakeActionInputFileCache fakeFileCache; @Mock private RemoteCache remoteCache; private FileOutErr outErr; - private final List progressUpdates = new ArrayList<>(); private StoredEventHandler eventHandler = new StoredEventHandler(); @@ -203,7 +199,6 @@ public SortedMap getInputMapping( @Override public void report(ProgressStatus progress) { - progressUpdates.add(progress); } @Override @@ -347,7 +342,6 @@ public CachedActionResult answer(InvocationOnMock invocation) { // We expect the CachedLocalSpawnRunner to _not_ write to outErr at all. assertThat(outErr.hasRecordedOutput()).isFalse(); assertThat(outErr.hasRecordedStderr()).isFalse(); - assertThat(progressUpdates).containsExactly(SpawnCheckingCacheEvent.create("remote-cache")); } @Test @@ -373,7 +367,6 @@ public void cacheMiss() throws Exception { doNothing().when(service).uploadOutputs(any(), any()); entry.store(result); verify(service).uploadOutputs(any(), any()); - assertThat(progressUpdates).containsExactly(SpawnCheckingCacheEvent.create("remote-cache")); } @Test @@ -401,7 +394,6 @@ public void noCacheSpawns() throws Exception { .build(); entry.store(result); verifyNoMoreInteractions(remoteCache); - assertThat(progressUpdates).isEmpty(); } } } @@ -435,7 +427,6 @@ public void noRemoteCacheSpawns_remoteCache() throws Exception { .build(); entry.store(result); verifyNoMoreInteractions(remoteCache); - assertThat(progressUpdates).isEmpty(); } } @@ -469,7 +460,6 @@ public void noRemoteCacheSpawns_combinedCache() throws Exception { .build(); entry.store(result); verifyNoMoreInteractions(remoteCache); - assertThat(progressUpdates).isEmpty(); } } @@ -541,7 +531,6 @@ public void failedActionsAreNotUploaded() throws Exception { .build(); entry.store(result); verify(service, never()).uploadOutputs(any(), any()); - assertThat(progressUpdates).containsExactly(SpawnCheckingCacheEvent.create("remote-cache")); } @Test @@ -572,7 +561,6 @@ public void printWarningIfDownloadFails() throws Exception { Event evt = eventHandler.getEvents().get(0); assertThat(evt.getKind()).isEqualTo(EventKind.WARNING); assertThat(evt.getMessage()).contains("UNAVAILABLE"); - assertThat(progressUpdates).containsExactly(SpawnCheckingCacheEvent.create("remote-cache")); } @Test @@ -616,7 +604,6 @@ public CachedActionResult answer(InvocationOnMock invocation) { doNothing().when(service).uploadOutputs(any(), any()); entry.store(result); verify(service).uploadOutputs(any(), eq(result)); - assertThat(progressUpdates).containsExactly(SpawnCheckingCacheEvent.create("remote-cache")); assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed. } diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/BUILD b/src/test/java/com/google/devtools/build/lib/remote/disk/BUILD new file mode 100644 index 00000000000000..a154a394bfd136 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/BUILD @@ -0,0 +1,33 @@ +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 = "disk", + srcs = glob(["*.java"]), + test_class = "com.google.devtools.build.lib.AllTests", + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", + "//src/main/java/com/google/devtools/build/lib/remote/common", + "//src/main/java/com/google/devtools/build/lib/remote/disk", + "//src/main/java/com/google/devtools/build/lib/remote/util", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", + "//src/test/java/com/google/devtools/build/lib:test_runner", + "//third_party:junit4", + "//third_party:mockito", + "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", + ], +) diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java new file mode 100644 index 00000000000000..4e0b48a2e69b53 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheClientTest.java @@ -0,0 +1,68 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote.disk; + +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +import build.bazel.remote.execution.v2.RequestMetadata; +import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.SyscallCache; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link DiskCacheClient}. */ +@RunWith(JUnit4.class) +public class DiskCacheClientTest { + private static final DigestUtil DIGEST_UTIL = + new DigestUtil(SyscallCache.NO_CACHE, DigestHashFunction.SHA256); + + private final FileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256); + private final Path root = fs.getPath("/"); + private DiskCacheClient client; + private RemoteActionExecutionContext context; + + @Before + public void setUp() throws Exception { + client = new DiskCacheClient(root, /* verifyDownloads= */ true, DIGEST_UTIL); + context = + RemoteActionExecutionContext.create( + mock(Spawn.class), + mock(SpawnExecutionContext.class), + RequestMetadata.getDefaultInstance()); + } + + @Test + public void testSpawnCheckingCacheEvent() throws Exception { + var unused = + getFromFuture( + client.downloadActionResult( + context, + DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")), + /* inlineOutErr= */ false)); + + verify(context.getSpawnExecutionContext()).report(SpawnCheckingCacheEvent.create("disk-cache")); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/remote/http/BUILD b/src/test/java/com/google/devtools/build/lib/remote/http/BUILD index 0193b9ebe5782d..e232778809bf35 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/http/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/http/BUILD @@ -21,7 +21,9 @@ java_test( ], test_class = "com.google.devtools.build.lib.AllTests", deps = [ + "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/authandtls", + "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/remote:Retrier", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/http", diff --git a/src/test/java/com/google/devtools/build/lib/remote/http/HttpCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/http/HttpCacheClientTest.java index cfac498e07b677..2b850826fee926 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/http/HttpCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/http/HttpCacheClientTest.java @@ -34,7 +34,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; +import com.google.devtools.build.lib.exec.SpawnCheckingCacheEvent; +import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.remote.RemoteRetrier; import com.google.devtools.build.lib.remote.Retrier; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; @@ -345,10 +348,38 @@ private HttpCacheClient createHttpBlobStore( public void setUp() throws Exception { remoteActionExecutionContext = RemoteActionExecutionContext.create( + mock(Spawn.class), + mock(SpawnExecutionContext.class), TracingMetadataUtils.buildMetadata( "none", "none", Digest.getDefaultInstance().getHash(), null)); } + @Test + public void testSpawnCheckingCacheEvent() throws Exception { + ServerChannel server = null; + try { + ConcurrentHashMap cacheContents = new ConcurrentHashMap<>(); + server = testServer.start(new HttpCacheServerHandler(cacheContents)); + + HttpCacheClient blobStore = + createHttpBlobStore( + server, /* timeoutSeconds= */ 1, /* creds= */ null, new AuthAndTLSOptions()); + + var unused = + getFromFuture( + blobStore.downloadActionResult( + remoteActionExecutionContext, + DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")), + /* inlineOutErr= */ false)); + + verify(remoteActionExecutionContext.getSpawnExecutionContext()) + .report(SpawnCheckingCacheEvent.create("remote-cache")); + + } finally { + testServer.stop(server); + } + } + @Test public void testUpload() throws Exception { ServerChannel server = null;