Skip to content

Commit

Permalink
[remote] Respect whether the server supports action cache updates
Browse files Browse the repository at this point in the history
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`.

Closes #16624.

PiperOrigin-RevId: 486901751
Change-Id: I0991f6891e21711df1e23ae0998a8bc95e2389bc
  • Loading branch information
Yannic authored and copybara-github committed Nov 8, 2022
1 parent 9cb5e0a commit 128d833
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ public ListenableFuture<ImmutableSet<Digest>> 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<Void> uploadActionResult(
RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import build.bazel.remote.execution.v2.PriorityCapabilities.PriorityRange;
import build.bazel.remote.execution.v2.RequestMetadata;
import build.bazel.remote.execution.v2.ServerCapabilities;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
import com.google.devtools.build.lib.remote.options.RemoteOptions;
Expand Down Expand Up @@ -234,25 +233,12 @@ 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.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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -324,9 +323,10 @@ 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.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;
Expand Down Expand Up @@ -398,8 +398,7 @@ public void testCheckClientServerCompatibility_remoteExecutionDoesNotSupportDige
}

@Test
public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate()
throws Exception {
public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate() {
ServerCapabilities caps =
ServerCapabilities.newBuilder()
.setLowApiVersion(ApiVersion.current.toSemVer())
Expand All @@ -423,9 +422,10 @@ 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.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;
Expand All @@ -435,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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();

Expand Down

0 comments on commit 128d833

Please sign in to comment.