Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[remote] Respect whether the server supports action cache updates #16624

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -234,25 +234,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