From b3cb165c860f3b846e118c1addebdd998418483f Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Tue, 1 Nov 2022 14:25:39 +0100 Subject: [PATCH 1/6] [remote] Respect whether the server supports action cache updates Only a subset of users may be allowed to update the action cache (e.g., only CI but not devs). Today, there are 2 ways to achive the desired behavior: - `GetCapabilities` returning that all users are allowed to update, and `UpdateActionResult` returning an error that Bazel prints and ignores, or - have the users that are not allowed to update the action cache set `--remote_upload_local_results=false`. Why don't we instead respect whether the remote cache supports uploading action results? Note that this requires support from the remote system to fully work (i.e., it needs to return `update_enabled = false` for users that don't have permission). Otherwise, Bazel's behavior will be the same as before this change: failed `UpdateActionResult` do not cause the build to fail. The only change this introduces is that Bazel will no longer error if `--remote_upload_local_results=true` and `GetCapabilities` returning `update_enabled = false`. --- .../build/lib/remote/RemoteCache.java | 5 +++++ .../lib/remote/RemoteExecutionService.java | 3 ++- .../lib/remote/RemoteServerCapabilities.java | 21 ------------------- .../lib/remote/options/RemoteOptions.java | 4 +++- .../remote/RemoteServerCapabilitiesTest.java | 14 ++++--------- 5 files changed, 14 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 60ebc08c0417c4..43f9994623069a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -122,6 +122,11 @@ public ListenableFuture> findMissingDigests( return cacheProtocol.findMissingDigests(context, digests); } + /** Returns whether the action cache supports updating action results. */ + public boolean actionCacheSupportsUpdate() { + return cacheCapabilities.getActionCacheUpdateCapabilities().getUpdateEnabled(); + } + /** Upload the action result to the remote cache. */ public ListenableFuture uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { 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 149339bcab2b37..ffa3871a369970 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 @@ -305,7 +305,8 @@ public CachePolicy getWriteCachePolicy(Spawn spawn) { if (useRemoteCache(remoteOptions)) { allowRemoteCache = - shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo()); + shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo()) + && remoteCache.actionCacheSupportsUpdate(); if (useDiskCache(remoteOptions)) { // Combined cache if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java index 6d486480e30b5d..9d558d29f2216e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java @@ -234,27 +234,6 @@ public static ClientServerCompatibilityStatus checkClientServerCompatibility( digestFunction, cacheCap.getDigestFunctionsList())); } - // Check updating remote cache is allowed, if we ever need to do that. - boolean remoteExecution = !Strings.isNullOrEmpty(remoteOptions.remoteExecutor); - if (remoteExecution) { - if (remoteOptions.remoteLocalFallback - && remoteOptions.remoteUploadLocalResults - && !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) { - result.addError( - "--remote_local_fallback and --remote_upload_local_results are set, " - + "but the current account is not authorized to write local results " - + "to the remote cache."); - } - } else { - // Local execution: check updating remote cache is allowed. - if (remoteOptions.remoteUploadLocalResults - && !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) { - result.addError( - "--remote_upload_local_results is set, but the current account is not authorized " - + "to write local results to the remote cache."); - } - } - if (remoteOptions.cacheCompression && !cacheCap.getSupportedCompressorsList().contains(Compressor.Value.ZSTD)) { result.addError( 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 1899b7a1e52ec4..dbe1cff71237dc 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 @@ -273,7 +273,9 @@ public String getTypeDescription() { defaultValue = "true", documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.UNKNOWN}, - help = "Whether to upload locally executed action results to the remote cache.") + help = + "Whether to upload locally executed action results to the remote cache if the remote " + + "cache supports it and the user is authorized to do so.") public boolean remoteUploadLocalResults; @Deprecated diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java index d817bf215b5925..e7c4853dece2a5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java @@ -308,8 +308,7 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportDigestFu } @Test - public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate() - throws Exception { + public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate() { ServerCapabilities caps = ServerCapabilities.newBuilder() .setLowApiVersion(ApiVersion.current.toSemVer()) @@ -324,9 +323,7 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate() RemoteServerCapabilities.ClientServerCompatibilityStatus st = RemoteServerCapabilities.checkClientServerCompatibility( caps, remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.CACHE); - assertThat(st.getErrors()).hasSize(1); - assertThat(st.getErrors().get(0)) - .containsMatch("not authorized to write local results to the remote cache"); + assertThat(st.isOk()).isTrue(); // Ignored when no local upload. remoteOptions.remoteUploadLocalResults = false; @@ -398,8 +395,7 @@ public void testCheckClientServerCompatibility_remoteExecutionDoesNotSupportDige } @Test - public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate() - throws Exception { + public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate() { ServerCapabilities caps = ServerCapabilities.newBuilder() .setLowApiVersion(ApiVersion.current.toSemVer()) @@ -423,9 +419,7 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate( remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.EXECUTION_AND_CACHE); - assertThat(st.getErrors()).hasSize(1); - assertThat(st.getErrors().get(0)) - .containsMatch("not authorized to write local results to the remote cache"); + assertThat(st.isOk()).isTrue(); // Ignored when no fallback. remoteOptions.remoteLocalFallback = false; From c13e5519921d23deafaac3b1607d97bfef9ad430 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Mon, 7 Nov 2022 19:45:11 +0100 Subject: [PATCH 2/6] fix http and disk capabilities --- .../devtools/build/lib/remote/RemoteModule.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 d97d90110e5882..05b2bff825f69a 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 @@ -16,6 +16,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; +import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities; import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.DigestFunction; import build.bazel.remote.execution.v2.ServerCapabilities; @@ -27,7 +28,6 @@ import com.google.common.base.Strings; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; -import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; @@ -112,11 +112,10 @@ /** RemoteModule provides distributed cache and remote execution for Bazel. */ public final class RemoteModule extends BlazeModule { - - private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - - private static final CacheCapabilities DISK_CACHE_CAPABILITIES = + private static final CacheCapabilities HTTP_AND_DISK_CACHE_CAPABILITIES = CacheCapabilities.newBuilder() + .setActionCacheUpdateCapabilities( + ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build()) .setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.ALLOWED) .build(); @@ -247,7 +246,7 @@ private void initHttpAndDiskCache( return; } RemoteCache remoteCache = - new RemoteCache(DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil); + new RemoteCache(HTTP_AND_DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil); actionContextProvider = RemoteActionContextProvider.createForRemoteCaching( executorService, env, remoteCache, /* retryScheduler= */ null, digestUtil); From dedfae096f92163b3178827ffc0fecb100774dcf Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Mon, 7 Nov 2022 19:55:14 +0100 Subject: [PATCH 3/6] add warning --- .../build/lib/remote/RemoteServerCapabilities.java | 8 ++++++++ .../build/remote/worker/OnDiskBlobStoreCache.java | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java index 9d558d29f2216e..69ef611652da9b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java @@ -234,6 +234,14 @@ public static ClientServerCompatibilityStatus checkClientServerCompatibility( digestFunction, cacheCap.getDigestFunctionsList())); } + if (remoteOptions.remoteUploadLocalResults && + !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) { + result.addWarning( + "--remote_upload_local_results is set, but the remote cache does not support uploading " + + "action results or the current account is not authorized to write local results " + + "to the remote cache."); + } + if (remoteOptions.cacheCompression && !cacheCap.getSupportedCompressorsList().contains(Compressor.Value.ZSTD)) { result.addError( diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java index 8ce78cf4f6577b..322cd4b6b0af3f 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java @@ -15,6 +15,7 @@ import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; +import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities; import build.bazel.remote.execution.v2.CacheCapabilities; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.Directory; @@ -33,9 +34,10 @@ /** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */ class OnDiskBlobStoreCache extends RemoteCache { - private static final CacheCapabilities CAPABILITIES = CacheCapabilities.newBuilder() + .setActionCacheUpdateCapabilities( + ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build()) .setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.ALLOWED) .build(); From 3fa1c8c08ae3b5e2d00193d3a2538958e37fdd07 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Mon, 7 Nov 2022 20:26:34 +0100 Subject: [PATCH 4/6] fix some tests --- .../lib/remote/RemoteServerCapabilitiesTest.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java index e7c4853dece2a5..a91bd7833e5b0c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java @@ -323,7 +323,10 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate() RemoteServerCapabilities.ClientServerCompatibilityStatus st = RemoteServerCapabilities.checkClientServerCompatibility( caps, remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.CACHE); - assertThat(st.isOk()).isTrue(); + assertThat(st.getErrors()).isEmpty(); + assertThat(st.getWarnings()).hasSize(1); + assertThat(st.getWarnings().get(0)) + .contains("remote cache does not support uploading action results"); // Ignored when no local upload. remoteOptions.remoteUploadLocalResults = false; @@ -419,7 +422,10 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate( remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.EXECUTION_AND_CACHE); - assertThat(st.isOk()).isTrue(); + assertThat(st.getErrors()).isEmpty(); + assertThat(st.getWarnings()).hasSize(1); + assertThat(st.getWarnings().get(0)) + .contains("remote cache does not support uploading action results"); // Ignored when no fallback. remoteOptions.remoteLocalFallback = false; @@ -429,7 +435,10 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate( remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.EXECUTION_AND_CACHE); - assertThat(st.isOk()).isTrue(); + assertThat(st.getErrors()).isEmpty(); + assertThat(st.getWarnings()).hasSize(1); + assertThat(st.getWarnings().get(0)) + .contains("remote cache does not support uploading action results"); // Ignored when no uploading local results. remoteOptions.remoteLocalFallback = true; From d63c5c80fead8ab20983c652312b9158d07ec708 Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Mon, 7 Nov 2022 20:30:39 +0100 Subject: [PATCH 5/6] verify internal state --- .../java/com/google/devtools/build/lib/remote/RemoteCache.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 43f9994623069a..86ecbfcca11431 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -130,6 +130,9 @@ public boolean actionCacheSupportsUpdate() { /** Upload the action result to the remote cache. */ public ListenableFuture uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { + Preconditions.checkState( + actionCacheSupportsUpdate(), + "Cannot upload action result to remote cache that does not support action cache updates"); Completable upload = RxFutures.toCompletable( From fd7346dd7fb841cbbcb3a529b3bb8b2aa9b7b6fa Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Mon, 7 Nov 2022 20:37:37 +0100 Subject: [PATCH 6/6] Revert "verify internal state" This reverts commit d63c5c80fead8ab20983c652312b9158d07ec708. --- .../java/com/google/devtools/build/lib/remote/RemoteCache.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index 86ecbfcca11431..43f9994623069a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -130,9 +130,6 @@ public boolean actionCacheSupportsUpdate() { /** Upload the action result to the remote cache. */ public ListenableFuture uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { - Preconditions.checkState( - actionCacheSupportsUpdate(), - "Cannot upload action result to remote cache that does not support action cache updates"); Completable upload = RxFutures.toCompletable(