From 83041b145d3966eb353aacb22b7e33ad01d9a239 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 16 Aug 2022 16:08:26 +0200 Subject: [PATCH] Refactor combined cache. (#16110) Instead of guessing when to use remote/disk part, combined cache now uses read/write cache policy provided by the context. The call sites can change the policy based on the requirements. Fixes #15934. But unfortunately, I am not able to add an integration test for it because our own remote worker doesn't support the asset API. Closes #16039. PiperOrigin-RevId: 465577383 Change-Id: I99effab1cdcba0890671ea64c4660ea31b059ce7 --- .../ByteStreamBuildEventArtifactUploader.java | 7 +- .../lib/remote/RemoteCacheClientFactory.java | 12 +- .../lib/remote/RemoteExecutionService.java | 156 +++++++++++------- .../build/lib/remote/RemoteModule.java | 6 +- .../build/lib/remote/RemoteSpawnCache.java | 6 +- .../build/lib/remote/RemoteSpawnRunner.java | 5 +- .../common/RemoteActionExecutionContext.java | 99 ++++++++--- .../remote/disk/DiskAndRemoteCacheClient.java | 120 +++++--------- .../devtools/build/lib/remote/util/Utils.java | 77 --------- 9 files changed, 232 insertions(+), 256 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 15c12bbb1b093e..ecc9a3ceef7ff6 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 @@ -30,7 +30,7 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; -import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.vfs.Path; @@ -274,8 +274,9 @@ private Single upload(Set files) { RequestMetadata metadata = TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload", null); - RemoteActionExecutionContext context = RemoteActionExecutionContext.create(metadata); - context.setStep(Step.UPLOAD_BES_FILES); + RemoteActionExecutionContext context = + RemoteActionExecutionContext.create(metadata) + .withWriteCachePolicy(CachePolicy.REMOTE_CACHE_ONLY); return Single.using( remoteCache::retain, diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java index 57741a8f28e26e..debde1aa47c176 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java @@ -44,12 +44,11 @@ public static RemoteCacheClient createDiskAndRemoteClient( PathFragment diskCachePath, boolean remoteVerifyDownloads, DigestUtil digestUtil, - RemoteCacheClient remoteCacheClient, - RemoteOptions options) + RemoteCacheClient remoteCacheClient) throws IOException { DiskCacheClient diskCacheClient = createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil); - return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient, options); + return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient); } public static RemoteCacheClient create( @@ -147,12 +146,7 @@ private static RemoteCacheClient createDiskAndHttpCache( RemoteCacheClient httpCache = createHttp(options, cred, digestUtil); return createDiskAndRemoteClient( - workingDirectory, - diskCachePath, - options.remoteVerifyDownloads, - digestUtil, - httpCache, - options); + workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache); } public static boolean isDiskCache(RemoteOptions options) { 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 e94875ec9e3f6a..63af731ebd421b 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 @@ -27,12 +27,7 @@ import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath; import static com.google.devtools.build.lib.remote.util.Utils.grpcAwareErrorMessage; import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload; -import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromCombinedCache; -import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromDiskCache; -import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromRemoteCache; import static com.google.devtools.build.lib.remote.util.Utils.shouldDownloadAllSpawnOutputs; -import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToCombinedDisk; -import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToDiskCache; import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToRemoteCache; import build.bazel.remote.execution.v2.Action; @@ -97,8 +92,9 @@ 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.common.OutputDigestMismatchException; +import com.google.devtools.build.lib.remote.common.ProgressStatusListener; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; -import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step; +import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.CachePolicy; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.CachedActionResult; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; @@ -281,41 +277,72 @@ private static boolean useDiskCache(RemoteOptions options) { return options.diskCache != null && !options.diskCache.isEmpty(); } - /** Returns {@code true} if the {@code spawn} should accept cached results from remote cache. */ - public boolean shouldAcceptCachedResult(Spawn spawn) { + public CachePolicy getReadCachePolicy(Spawn spawn) { if (remoteCache == null) { - return false; + return CachePolicy.NO_CACHE; } + boolean allowDiskCache = false; + boolean allowRemoteCache = false; + if (useRemoteCache(remoteOptions)) { + allowRemoteCache = remoteOptions.remoteAcceptCached && Spawns.mayBeCachedRemotely(spawn); if (useDiskCache(remoteOptions)) { - return shouldAcceptCachedResultFromCombinedCache(remoteOptions, spawn); - } else { - return shouldAcceptCachedResultFromRemoteCache(remoteOptions, spawn); + // Combined cache + if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { + // --incompatible_remote_results_ignore_disk is set. Disk cache is treated as local cache. + // Actions which are tagged with `no-remote-cache` can still hit the disk cache. + allowDiskCache = Spawns.mayBeCached(spawn); + } else { + // Disk cache is treated as a remote cache and disabled for `no-remote-cache`. + allowDiskCache = allowRemoteCache; + } } } else { - return shouldAcceptCachedResultFromDiskCache(remoteOptions, spawn); + // Disk cache only + if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { + allowDiskCache = Spawns.mayBeCached(spawn); + } else { + allowDiskCache = remoteOptions.remoteAcceptCached && Spawns.mayBeCached(spawn); + } } + + return CachePolicy.create(allowRemoteCache, allowDiskCache); } - /** - * Returns {@code true} if the local results of the {@code spawn} should be uploaded to remote - * cache. - */ - public boolean shouldUploadLocalResults(Spawn spawn) { + public CachePolicy getWriteCachePolicy(Spawn spawn) { if (remoteCache == null) { - return false; + return CachePolicy.NO_CACHE; } + boolean allowDiskCache = false; + boolean allowRemoteCache = false; + if (useRemoteCache(remoteOptions)) { + allowRemoteCache = + shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo()); if (useDiskCache(remoteOptions)) { - return shouldUploadLocalResultsToCombinedDisk(remoteOptions, spawn); - } else { - return shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn); + // Combined cache + if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { + // If --incompatible_remote_results_ignore_disk is set, we treat the disk cache part as + // local cache. Actions which are tagged with `no-remote-cache` can still hit the disk + // cache. + allowDiskCache = Spawns.mayBeCached(spawn); + } else { + // Otherwise, it's treated as a remote cache and disabled for `no-remote-cache`. + allowDiskCache = allowRemoteCache; + } } } else { - return shouldUploadLocalResultsToDiskCache(remoteOptions, spawn); + // Disk cache only + if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { + allowDiskCache = Spawns.mayBeCached(spawn); + } else { + allowDiskCache = remoteOptions.remoteUploadLocalResults && Spawns.mayBeCached(spawn); + } } + + return CachePolicy.create(allowRemoteCache, allowDiskCache); } /** Returns {@code true} if the spawn may be executed remotely. */ @@ -443,7 +470,8 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context TracingMetadataUtils.buildMetadata( buildRequestId, commandId, actionKey.getDigest().getHash(), spawn.getResourceOwner()); RemoteActionExecutionContext remoteActionExecutionContext = - RemoteActionExecutionContext.create(spawn, metadata); + RemoteActionExecutionContext.create( + spawn, metadata, getWriteCachePolicy(spawn), getReadCachePolicy(spawn)); return new RemoteAction( spawn, @@ -579,9 +607,9 @@ public int hashCode() { @Nullable public RemoteActionResult lookupCache(RemoteAction action) throws IOException, InterruptedException { - checkState(shouldAcceptCachedResult(action.getSpawn()), "spawn doesn't accept cached result"); - - action.getRemoteActionExecutionContext().setStep(Step.CHECK_ACTION_CACHE); + checkState( + action.getRemoteActionExecutionContext().getReadCachePolicy().allowAnyCache(), + "spawn doesn't accept cached result"); CachedActionResult cachedActionResult = remoteCache.downloadActionResult( @@ -601,18 +629,21 @@ private static Path toTmpDownloadPath(Path actualPath) { .getRelative(actualPath.getBaseName() + ".tmp"); } - private ListenableFuture downloadFile(RemoteAction action, FileMetadata file) { + private ListenableFuture downloadFile( + RemoteActionExecutionContext context, + ProgressStatusListener progressStatusListener, + FileMetadata file) { checkNotNull(remoteCache, "remoteCache can't be null"); try { ListenableFuture future = remoteCache.downloadFile( - action.getRemoteActionExecutionContext(), + context, remotePathResolver.localPathToOutputPath(file.path()), toTmpDownloadPath(file.path()), file.digest(), new RemoteCache.DownloadProgressReporter( - action.getSpawnExecutionContext()::report, + progressStatusListener, remotePathResolver.localPathToOutputPath(file.path()), file.digest().getSizeBytes())); return transform(future, (d) -> file, directExecutor()); @@ -905,7 +936,8 @@ private DirectoryMetadata parseDirectory( return new DirectoryMetadata(filesBuilder.build(), symlinksBuilder.build()); } - ActionResultMetadata parseActionResultMetadata(RemoteAction action, RemoteActionResult result) + ActionResultMetadata parseActionResultMetadata( + RemoteActionExecutionContext context, RemoteActionResult result) throws IOException, InterruptedException { checkNotNull(remoteCache, "remoteCache can't be null"); @@ -915,8 +947,7 @@ ActionResultMetadata parseActionResultMetadata(RemoteAction action, RemoteAction dirMetadataDownloads.put( remotePathResolver.outputPathToLocalPath(dir.getPath()), Futures.transformAsync( - remoteCache.downloadBlob( - action.getRemoteActionExecutionContext(), dir.getTreeDigest()), + remoteCache.downloadBlob(context, dir.getTreeDigest()), (treeBytes) -> immediateFuture(Tree.parseFrom(treeBytes, ExtensionRegistry.getEmptyRegistry())), directExecutor())); @@ -974,11 +1005,16 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re checkState(!shutdown.get(), "shutdown"); checkNotNull(remoteCache, "remoteCache can't be null"); - action.getRemoteActionExecutionContext().setStep(Step.DOWNLOAD_OUTPUTS); + ProgressStatusListener progressStatusListener = action.getSpawnExecutionContext()::report; + RemoteActionExecutionContext context = action.getRemoteActionExecutionContext(); + if (result.executeResponse != null) { + // Always read from remote cache for just remotely executed action. + context = context.withReadCachePolicy(context.getReadCachePolicy().addRemoteCache()); + } ActionResultMetadata metadata; try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) { - metadata = parseActionResultMetadata(action, result); + metadata = parseActionResultMetadata(context, result); } if (result.success()) { @@ -1009,11 +1045,10 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re // When downloading outputs from just remotely executed action, the action result comes from // Execution response which means, if disk cache is enabled, action result hasn't been // uploaded to it. Upload action result to disk cache here so next build could hit it. - if (useDiskCache(remoteOptions) - && action.getRemoteActionExecutionContext().getExecuteResponse() != null) { + if (useDiskCache(remoteOptions) && result.executeResponse != null) { getFromFuture( remoteCache.uploadActionResult( - action.getRemoteActionExecutionContext(), + context.withWriteCachePolicy(CachePolicy.DISK_CACHE_ONLY), action.getActionKey(), result.actionResult)); } @@ -1033,7 +1068,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re ImmutableList> forcedDownloads = ImmutableList.of(); if (downloadOutputs) { - downloadsBuilder.addAll(buildFilesToDownload(metadata, action)); + downloadsBuilder.addAll(buildFilesToDownload(context, progressStatusListener, metadata)); } else { checkState( result.getExitCode() == 0, @@ -1046,14 +1081,14 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } if (shouldForceDownloads) { forcedDownloads = - buildFilesToDownloadWithPredicate(metadata, action, shouldForceDownloadPredicate); + buildFilesToDownloadWithPredicate( + context, progressStatusListener, metadata, shouldForceDownloadPredicate); } } FileOutErr tmpOutErr = outErr.childOutErr(); List> outErrDownloads = - remoteCache.downloadOutErr( - action.getRemoteActionExecutionContext(), result.actionResult, tmpOutErr); + remoteCache.downloadOutErr(context, result.actionResult, tmpOutErr); for (ListenableFuture future : outErrDownloads) { downloadsBuilder.add(transform(future, (v) -> null, directExecutor())); } @@ -1136,8 +1171,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re try (SilentCloseable c = Profiler.instance().profile("Remote.downloadInMemoryOutput")) { if (inMemoryOutput != null) { ListenableFuture inMemoryOutputDownload = - remoteCache.downloadBlob( - action.getRemoteActionExecutionContext(), inMemoryOutputDigest); + remoteCache.downloadBlob(context, inMemoryOutputDigest); waitForBulkTransfer( ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true); byte[] data = getFromFuture(inMemoryOutputDownload); @@ -1150,20 +1184,25 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re } private ImmutableList> buildFilesToDownload( - ActionResultMetadata metadata, RemoteAction action) { + RemoteActionExecutionContext context, + ProgressStatusListener progressStatusListener, + ActionResultMetadata metadata) { Predicate alwaysTrue = unused -> true; - return buildFilesToDownloadWithPredicate(metadata, action, alwaysTrue); + return buildFilesToDownloadWithPredicate(context, progressStatusListener, metadata, alwaysTrue); } private ImmutableList> buildFilesToDownloadWithPredicate( - ActionResultMetadata metadata, RemoteAction action, Predicate predicate) { + RemoteActionExecutionContext context, + ProgressStatusListener progressStatusListener, + ActionResultMetadata metadata, + Predicate predicate) { HashSet queuedFilePaths = new HashSet<>(); ImmutableList.Builder> builder = new ImmutableList.Builder<>(); for (FileMetadata file : metadata.files()) { PathFragment filePath = file.path().asFragment(); if (queuedFilePaths.add(filePath) && predicate.test(file.path.toString())) { - builder.add(downloadFile(action, file)); + builder.add(downloadFile(context, progressStatusListener, file)); } } @@ -1171,7 +1210,7 @@ private ImmutableList> buildFilesToDownloadWithPr for (FileMetadata file : entry.getValue().files()) { PathFragment filePath = file.path().asFragment(); if (queuedFilePaths.add(filePath) && predicate.test(file.path.toString())) { - builder.add(downloadFile(action, file)); + builder.add(downloadFile(context, progressStatusListener, file)); } } } @@ -1235,13 +1274,13 @@ UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) public void uploadOutputs(RemoteAction action, SpawnResult spawnResult) throws InterruptedException, ExecException { checkState(!shutdown.get(), "shutdown"); - checkState(shouldUploadLocalResults(action.getSpawn()), "spawn shouldn't upload local result"); + checkState( + action.getRemoteActionExecutionContext().getWriteCachePolicy().allowAnyCache(), + "spawn shouldn't upload local result"); checkState( SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0, "shouldn't upload outputs of failed local action"); - action.getRemoteActionExecutionContext().setStep(Step.UPLOAD_OUTPUTS); - if (remoteOptions.remoteCacheAsync) { Single.using( remoteCache::retain, @@ -1303,15 +1342,18 @@ public void uploadInputsIfNotPresent(RemoteAction action, boolean force) checkState(!shutdown.get(), "shutdown"); checkState(mayBeExecutedRemotely(action.getSpawn()), "spawn can't be executed remotely"); - action.getRemoteActionExecutionContext().setStep(Step.UPLOAD_INPUTS); - RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache; // Upload the command and all the inputs into the remote cache. Map additionalInputs = Maps.newHashMapWithExpectedSize(2); additionalInputs.put(action.getActionKey().getDigest(), action.getAction()); additionalInputs.put(action.getCommandHash(), action.getCommand()); remoteExecutionCache.ensureInputsPresent( - action.getRemoteActionExecutionContext(), action.getMerkleTree(), additionalInputs, force); + action + .getRemoteActionExecutionContext() + .withWriteCachePolicy(CachePolicy.REMOTE_CACHE_ONLY), // Only upload to remote cache + action.getMerkleTree(), + additionalInputs, + force); } /** @@ -1326,8 +1368,6 @@ public RemoteActionResult executeRemotely( checkState(!shutdown.get(), "shutdown"); checkState(mayBeExecutedRemotely(action.getSpawn()), "spawn can't be executed remotely"); - action.getRemoteActionExecutionContext().setStep(Step.EXECUTE_REMOTELY); - ExecuteRequest.Builder requestBuilder = ExecuteRequest.newBuilder() .setInstanceName(remoteOptions.remoteInstanceName) @@ -1347,8 +1387,6 @@ public RemoteActionResult executeRemotely( ExecuteResponse reply = remoteExecutor.executeRemotely(action.getRemoteActionExecutionContext(), request, observer); - action.getRemoteActionExecutionContext().setExecuteResponse(reply); - return RemoteActionResult.createFromResponse(reply); } 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 5fed432abe4d3c..2a60fbc3d77322 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 @@ -567,8 +567,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions.diskCache, remoteOptions.remoteVerifyDownloads, digestUtil, - cacheClient, - remoteOptions); + cacheClient); } catch (IOException e) { handleInitFailure(env, e, Code.CACHE_INIT_FAILURE); return; @@ -626,8 +625,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions.diskCache, remoteOptions.remoteVerifyDownloads, digestUtil, - cacheClient, - remoteOptions); + cacheClient); } catch (IOException e) { handleInitFailure(env, e, Code.CACHE_INIT_FAILURE); return; 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 3ce198104d2e9d..9527b3436c4f6b 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 @@ -82,8 +82,10 @@ RemoteExecutionService getRemoteExecutionService() { @Override public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) throws InterruptedException, IOException, ExecException, ForbiddenActionInputException { - boolean shouldAcceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn); - boolean shouldUploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn); + boolean shouldAcceptCachedResult = + remoteExecutionService.getReadCachePolicy(spawn).allowAnyCache(); + boolean shouldUploadLocalResults = + remoteExecutionService.getWriteCachePolicy(spawn).allowAnyCache(); if (!shouldAcceptCachedResult && !shouldUploadLocalResults) { return SpawnCache.NO_RESULT_NO_STORE; } 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 218ba97b9e9d7f..c50dd30092f17a 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 @@ -183,10 +183,11 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) "Spawn can't be executed remotely. This is a bug."); Stopwatch totalTime = Stopwatch.createStarted(); - boolean uploadLocalResults = remoteExecutionService.shouldUploadLocalResults(spawn); - boolean acceptCachedResult = remoteExecutionService.shouldAcceptCachedResult(spawn); + boolean acceptCachedResult = remoteExecutionService.getReadCachePolicy(spawn).allowAnyCache(); + boolean uploadLocalResults = remoteExecutionService.getWriteCachePolicy(spawn).allowAnyCache(); RemoteAction action = remoteExecutionService.buildRemoteAction(spawn, context); + SpawnMetrics.Builder spawnMetrics = SpawnMetrics.Builder.forRemoteExec() .setInputBytes(action.getInputBytes()) 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 78cac3980c2a71..46a58e5927391b 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 @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.remote.common; -import build.bazel.remote.execution.v2.ExecuteResponse; import build.bazel.remote.execution.v2.RequestMetadata; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.Spawn; @@ -21,40 +20,82 @@ /** A context that provide remote execution related information for executing an action remotely. */ public class RemoteActionExecutionContext { - /** The current step of the context. */ - public enum Step { - INIT, - CHECK_ACTION_CACHE, - UPLOAD_INPUTS, - EXECUTE_REMOTELY, - UPLOAD_OUTPUTS, - DOWNLOAD_OUTPUTS, - UPLOAD_BES_FILES, + /** Determines whether to read/write remote cache, disk cache or both. */ + public enum CachePolicy { + NO_CACHE, + REMOTE_CACHE_ONLY, + DISK_CACHE_ONLY, + ANY_CACHE; + + public static CachePolicy create(boolean allowRemoteCache, boolean allowDiskCache) { + if (allowRemoteCache && allowDiskCache) { + return ANY_CACHE; + } else if (allowRemoteCache) { + return REMOTE_CACHE_ONLY; + } else if (allowDiskCache) { + return DISK_CACHE_ONLY; + } else { + return NO_CACHE; + } + } + + public boolean allowAnyCache() { + return this != NO_CACHE; + } + + public boolean allowRemoteCache() { + return this == REMOTE_CACHE_ONLY || this == ANY_CACHE; + } + + public boolean allowDiskCache() { + return this == DISK_CACHE_ONLY || this == ANY_CACHE; + } + + public CachePolicy addRemoteCache() { + if (this == DISK_CACHE_ONLY || this == ANY_CACHE) { + return ANY_CACHE; + } + + return REMOTE_CACHE_ONLY; + } } @Nullable private final Spawn spawn; private final RequestMetadata requestMetadata; private final NetworkTime networkTime; - - @Nullable private ExecuteResponse executeResponse; - private Step step; + 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.step = Step.INIT; + this.writeCachePolicy = CachePolicy.ANY_CACHE; + this.readCachePolicy = CachePolicy.ANY_CACHE; } - /** Returns current {@link Step} of the context. */ - public Step getStep() { - return step; + private RemoteActionExecutionContext( + @Nullable Spawn spawn, + RequestMetadata requestMetadata, + NetworkTime networkTime, + CachePolicy writeCachePolicy, + CachePolicy readCachePolicy) { + this.spawn = spawn; + this.requestMetadata = requestMetadata; + this.networkTime = networkTime; + this.writeCachePolicy = writeCachePolicy; + this.readCachePolicy = readCachePolicy; + } + + public RemoteActionExecutionContext withWriteCachePolicy(CachePolicy writeCachePolicy) { + return new RemoteActionExecutionContext( + spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy); } - /** Sets current {@link Step} of the context. */ - public void setStep(Step step) { - this.step = step; + public RemoteActionExecutionContext withReadCachePolicy(CachePolicy readCachePolicy) { + return new RemoteActionExecutionContext( + spawn, requestMetadata, networkTime, writeCachePolicy, readCachePolicy); } /** Returns the {@link Spawn} of the action being executed or {@code null}. */ @@ -86,13 +127,12 @@ public ActionExecutionMetadata getSpawnOwner() { return spawn.getResourceOwner(); } - public void setExecuteResponse(@Nullable ExecuteResponse executeResponse) { - this.executeResponse = executeResponse; + public CachePolicy getWriteCachePolicy() { + return writeCachePolicy; } - @Nullable - public ExecuteResponse getExecuteResponse() { - return executeResponse; + public CachePolicy getReadCachePolicy() { + return readCachePolicy; } /** Creates a {@link RemoteActionExecutionContext} with given {@link RequestMetadata}. */ @@ -108,4 +148,13 @@ public static RemoteActionExecutionContext create( @Nullable Spawn spawn, RequestMetadata metadata) { return new RemoteActionExecutionContext(spawn, metadata, new NetworkTime()); } + + public static RemoteActionExecutionContext create( + @Nullable Spawn spawn, + RequestMetadata requestMetadata, + CachePolicy writeCachePolicy, + CachePolicy readCachePolicy) { + return new RemoteActionExecutionContext( + spawn, requestMetadata, new NetworkTime(), writeCachePolicy, readCachePolicy); + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java index c337a5ebe3cdf1..1fc6200cd9e9d7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java @@ -13,9 +13,9 @@ // limitations under the License. package com.google.devtools.build.lib.remote.disk; +import static com.google.common.util.concurrent.Futures.immediateFailedFuture; +import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; -import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromRemoteCache; -import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToRemoteCache; import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.Digest; @@ -25,9 +25,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.remote.common.LazyFileOutputStream; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; -import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext.Step; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; -import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.vfs.Path; import com.google.protobuf.ByteString; import java.io.IOException; @@ -43,24 +41,22 @@ public final class DiskAndRemoteCacheClient implements RemoteCacheClient { private final RemoteCacheClient remoteCache; private final DiskCacheClient diskCache; - private final RemoteOptions options; - public DiskAndRemoteCacheClient( - DiskCacheClient diskCache, RemoteCacheClient remoteCache, RemoteOptions options) { + public DiskAndRemoteCacheClient(DiskCacheClient diskCache, RemoteCacheClient remoteCache) { this.diskCache = Preconditions.checkNotNull(diskCache); this.remoteCache = Preconditions.checkNotNull(remoteCache); - this.options = options; } @Override public ListenableFuture uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { - ListenableFuture future = diskCache.uploadActionResult(context, actionKey, actionResult); - // Only upload action result to remote cache if we are uploading local outputs. This method - // could be called when we are downloading outputs from remote executor if disk cache is enabled - // because we want to upload the action result to it. - if (context.getStep() == Step.UPLOAD_OUTPUTS - && shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { + ListenableFuture future = Futures.immediateVoidFuture(); + + if (context.getWriteCachePolicy().allowDiskCache()) { + future = diskCache.uploadActionResult(context, actionKey, actionResult); + } + + if (context.getWriteCachePolicy().allowRemoteCache()) { future = Futures.transformAsync( future, @@ -79,20 +75,13 @@ public void close() { @Override public ListenableFuture uploadFile( RemoteActionExecutionContext context, Digest digest, Path file) { - RemoteActionExecutionContext.Step step = context.getStep(); - - // For UPLOAD_INPUTS, only upload to remote cache. - if (step == Step.UPLOAD_INPUTS) { - return remoteCache.uploadFile(context, digest, file); - } + ListenableFuture future = Futures.immediateVoidFuture(); - // For UPLOAD_BES_FILES, only upload to remote cache. - if (step == Step.UPLOAD_BES_FILES) { - return remoteCache.uploadFile(context, digest, file); + if (context.getWriteCachePolicy().allowDiskCache()) { + future = diskCache.uploadFile(context, digest, file); } - ListenableFuture future = diskCache.uploadFile(context, digest, file); - if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { + if (context.getWriteCachePolicy().allowRemoteCache()) { future = Futures.transformAsync( future, v -> remoteCache.uploadFile(context, digest, file), directExecutor()); @@ -103,20 +92,13 @@ public ListenableFuture uploadFile( @Override public ListenableFuture uploadBlob( RemoteActionExecutionContext context, Digest digest, ByteString data) { - RemoteActionExecutionContext.Step step = context.getStep(); + ListenableFuture future = Futures.immediateVoidFuture(); - // For UPLOAD_INPUTS, only upload to remote cache. - if (step == Step.UPLOAD_INPUTS) { - return remoteCache.uploadBlob(context, digest, data); + if (context.getWriteCachePolicy().allowDiskCache()) { + future = diskCache.uploadBlob(context, digest, data); } - // For BES upload, only upload to the remote cache. - if (step == Step.UPLOAD_BES_FILES) { - return remoteCache.uploadBlob(context, digest, data); - } - - ListenableFuture future = diskCache.uploadBlob(context, digest, data); - if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { + if (context.getWriteCachePolicy().allowRemoteCache()) { future = Futures.transformAsync( future, v -> remoteCache.uploadBlob(context, digest, data), directExecutor()); @@ -127,38 +109,27 @@ public ListenableFuture uploadBlob( @Override public ListenableFuture> findMissingDigests( RemoteActionExecutionContext context, Iterable digests) { - RemoteActionExecutionContext.Step step = context.getStep(); - - // For UPLOAD_INPUTS, find missing digests should only look at - // the remote cache, not the disk cache because the remote executor only - // has access to the remote cache, not the disk cache. - // Also, the DiskCache always returns all digests as missing - // and we don't want to transfer all the files all the time. - if (step == Step.UPLOAD_INPUTS) { - return remoteCache.findMissingDigests(context, digests); + ListenableFuture> diskQuery = immediateFuture(ImmutableSet.of()); + if (context.getWriteCachePolicy().allowDiskCache()) { + diskQuery = diskCache.findMissingDigests(context, digests); } - // For UPLOAD_BES_FILES, we only check the remote cache. - if (step == Step.UPLOAD_BES_FILES) { - return remoteCache.findMissingDigests(context, digests); + ListenableFuture> remoteQuery = immediateFuture(ImmutableSet.of()); + if (context.getWriteCachePolicy().allowRemoteCache()) { + remoteQuery = remoteCache.findMissingDigests(context, digests); } - ListenableFuture> diskQuery = - diskCache.findMissingDigests(context, digests); - if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { - ListenableFuture> remoteQuery = - remoteCache.findMissingDigests(context, digests); - return Futures.whenAllSucceed(remoteQuery, diskQuery) - .call( - () -> - ImmutableSet.builder() - .addAll(remoteQuery.get()) - .addAll(diskQuery.get()) - .build(), - directExecutor()); - } else { - return diskQuery; - } + ListenableFuture> diskQueryFinal = diskQuery; + ListenableFuture> remoteQueryFinal = remoteQuery; + + return Futures.whenAllSucceed(remoteQueryFinal, diskQueryFinal) + .call( + () -> + ImmutableSet.builder() + .addAll(remoteQueryFinal.get()) + .addAll(diskQueryFinal.get()) + .build(), + directExecutor()); } private Path newTempPath() { @@ -176,7 +147,7 @@ private static ListenableFuture closeStreamOnError( } catch (IOException e) { rootCause.addSuppressed(e); } - return Futures.immediateFailedFuture(rootCause); + return immediateFailedFuture(rootCause); }, directExecutor()); } @@ -184,7 +155,7 @@ private static ListenableFuture closeStreamOnError( @Override public ListenableFuture downloadBlob( RemoteActionExecutionContext context, Digest digest, OutputStream out) { - if (diskCache.contains(digest)) { + if (context.getReadCachePolicy().allowDiskCache() && diskCache.contains(digest)) { return diskCache.downloadBlob(context, digest, out); } @@ -192,9 +163,7 @@ public ListenableFuture downloadBlob( final OutputStream tempOut; tempOut = new LazyFileOutputStream(tempPath); - // Always download outputs for just remotely executed action. - if (context.getExecuteResponse() != null - || shouldAcceptCachedResultFromRemoteCache(options, context.getSpawn())) { + if (context.getReadCachePolicy().allowRemoteCache()) { ListenableFuture download = closeStreamOnError(remoteCache.downloadBlob(context, digest, tempOut), tempOut); return Futures.transformAsync( @@ -204,29 +173,30 @@ public ListenableFuture downloadBlob( tempOut.close(); diskCache.captureFile(tempPath, digest, /* isActionCache= */ false); } catch (IOException e) { - return Futures.immediateFailedFuture(e); + return immediateFailedFuture(e); } return diskCache.downloadBlob(context, digest, out); }, directExecutor()); } else { - return Futures.immediateFuture(null); + return immediateFuture(null); } } @Override public ListenableFuture downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) { - if (diskCache.containsActionResult(actionKey)) { + if (context.getReadCachePolicy().allowDiskCache() + && diskCache.containsActionResult(actionKey)) { return diskCache.downloadActionResult(context, actionKey, inlineOutErr); } - if (shouldAcceptCachedResultFromRemoteCache(options, context.getSpawn())) { + if (context.getReadCachePolicy().allowRemoteCache()) { return Futures.transformAsync( remoteCache.downloadActionResult(context, actionKey, inlineOutErr), (cachedActionResult) -> { if (cachedActionResult == null) { - return Futures.immediateFuture(null); + return immediateFuture(null); } else { return Futures.transform( diskCache.uploadActionResult( @@ -237,7 +207,7 @@ public ListenableFuture downloadActionResult( }, directExecutor()); } else { - return Futures.immediateFuture(null); + return immediateFuture(null); } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index c7750404352206..2705f2cfc04149 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -25,7 +25,6 @@ import com.google.common.base.Preconditions; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.AsyncCallable; import com.google.common.util.concurrent.FluentFuture; @@ -583,33 +582,6 @@ public static String bytesCountToDisplayString(long bytes) { return String.format("%s %s", BYTE_COUNT_FORMAT.format(value / 1024.0), UNITS.get(unitIndex)); } - public static boolean shouldAcceptCachedResultFromRemoteCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { - return remoteOptions.remoteAcceptCached && (spawn == null || Spawns.mayBeCachedRemotely(spawn)); - } - - public static boolean shouldAcceptCachedResultFromDiskCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { - if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { - return spawn == null || Spawns.mayBeCached(spawn); - } else { - return remoteOptions.remoteAcceptCached && (spawn == null || Spawns.mayBeCached(spawn)); - } - } - - public static boolean shouldAcceptCachedResultFromCombinedCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { - if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { - // --incompatible_remote_results_ignore_disk is set. Disk cache is treated as local cache. - // Actions which are tagged with `no-remote-cache` can still hit the disk cache. - return spawn == null || Spawns.mayBeCached(spawn); - } else { - // Disk cache is treated as a remote cache and disabled for `no-remote-cache`. - return remoteOptions.remoteAcceptCached - && (spawn == null || Spawns.mayBeCachedRemotely(spawn)); - } - } - public static boolean shouldUploadLocalResultsToRemoteCache( RemoteOptions remoteOptions, @Nullable Map executionInfo) { return remoteOptions.remoteUploadLocalResults @@ -617,53 +589,4 @@ public static boolean shouldUploadLocalResultsToRemoteCache( || (Spawns.mayBeCachedRemotely(executionInfo) && !executionInfo.containsKey(ExecutionRequirements.NO_REMOTE_CACHE_UPLOAD))); } - - public static boolean shouldUploadLocalResultsToRemoteCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { - ImmutableMap executionInfo = null; - if (spawn != null) { - executionInfo = spawn.getExecutionInfo(); - } - return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); - } - - public static boolean shouldUploadLocalResultsToDiskCache( - RemoteOptions remoteOptions, @Nullable Map executionInfo) { - if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { - return executionInfo == null || Spawns.mayBeCached(executionInfo); - } else { - return remoteOptions.remoteUploadLocalResults - && (executionInfo == null || Spawns.mayBeCached(executionInfo)); - } - } - - public static boolean shouldUploadLocalResultsToDiskCache( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { - ImmutableMap executionInfo = null; - if (spawn != null) { - executionInfo = spawn.getExecutionInfo(); - } - return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); - } - - public static boolean shouldUploadLocalResultsToCombinedDisk( - RemoteOptions remoteOptions, @Nullable Map executionInfo) { - if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { - // If --incompatible_remote_results_ignore_disk is set, we treat the disk cache part as local - // cache. Actions which are tagged with `no-remote-cache` can still hit the disk cache. - return shouldUploadLocalResultsToDiskCache(remoteOptions, executionInfo); - } else { - // Otherwise, it's treated as a remote cache and disabled for `no-remote-cache`. - return shouldUploadLocalResultsToRemoteCache(remoteOptions, executionInfo); - } - } - - public static boolean shouldUploadLocalResultsToCombinedDisk( - RemoteOptions remoteOptions, @Nullable Spawn spawn) { - ImmutableMap executionInfo = null; - if (spawn != null) { - executionInfo = spawn.getExecutionInfo(); - } - return shouldUploadLocalResultsToCombinedDisk(remoteOptions, executionInfo); - } }