Fix noremote_upload_local_results behavior in combined blobstore
Disable the upload action to remote cache when `--noremote_upload_local_results` is set.
This problem would only happen in `CombinedDiskHttpBlobStore` at this moment when users want read/write permission to disk cache but only read permission to remote cache.
Once the GrpcCache refactoring (making GrpcCache a SimpleBlobStore implementation) is done, `CombinedDiskHttpBlobStore` should be renamed to `CombinedDiskRemoteBlobStore`. Then this fix will apply to both GrpcCache and HttpCache.
Fix https://github.com/bazelbuild/bazel/issues/8216
Closes #9338.
PiperOrigin-RevId: 286382882
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 a594454..ff00eb6 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
@@ -47,11 +47,12 @@
PathFragment diskCachePath,
boolean remoteVerifyDownloads,
DigestUtil digestUtil,
- RemoteCacheClient remoteCacheClient)
+ RemoteCacheClient remoteCacheClient,
+ RemoteOptions options)
throws IOException {
DiskCacheClient diskCacheClient =
createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil);
- return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
+ return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient, options);
}
public static ReferenceCountedChannel createGrpcChannel(
@@ -159,7 +160,12 @@
RemoteCacheClient httpCache = createHttp(options, cred, digestUtil);
return createDiskAndRemoteClient(
- workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache);
+ workingDirectory,
+ diskCachePath,
+ options.remoteVerifyDownloads,
+ digestUtil,
+ httpCache,
+ options);
}
public static boolean isDiskCache(RemoteOptions options) {
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 51e9156..0fa1b5d 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
@@ -299,7 +299,8 @@
remoteOptions.diskCache,
remoteOptions.remoteVerifyDownloads,
digestUtil,
- cacheClient);
+ cacheClient,
+ remoteOptions);
}
RemoteCache remoteCache = new RemoteCache(cacheClient, remoteOptions, digestUtil);
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 5c25075..689c024 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
@@ -114,16 +114,11 @@
public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
throws InterruptedException, IOException, ExecException {
if (!Spawns.mayBeCached(spawn)
- || (!Spawns.mayBeCachedRemotely(spawn) && isRemoteCache(options))) {
+ || (!Spawns.mayBeCachedRemotely(spawn) && useRemoteCache(options))) {
// returning SpawnCache.NO_RESULT_NO_STORE in case the caching is disabled or in case
// the remote caching is disabled and the only configured cache is remote.
return SpawnCache.NO_RESULT_NO_STORE;
}
- boolean checkCache = options.remoteAcceptCached;
-
- if (checkCache) {
- context.report(ProgressStatus.CHECKING_CACHE, "remote-cache");
- }
SortedMap<PathFragment, ActionInput> inputMap = context.getInputMapping(true);
MerkleTree merkleTree =
@@ -150,7 +145,9 @@
TracingMetadataUtils.contextWithMetadata(buildRequestId, commandId, actionKey);
Profiler prof = Profiler.instance();
- if (checkCache) {
+ if (options.remoteAcceptCached
+ || (options.incompatibleRemoteResultsIgnoreDisk && useDiskCache(options))) {
+ context.report(ProgressStatus.CHECKING_CACHE, "remote-cache");
// Metadata will be available in context.current() until we detach.
// This is done via a thread-local variable.
Context previous = withMetadata.attach();
@@ -211,7 +208,8 @@
context.prefetchInputs();
- if (options.remoteUploadLocalResults) {
+ if (options.remoteUploadLocalResults
+ || (options.incompatibleRemoteResultsIgnoreDisk && useDiskCache(options))) {
return new CacheHandle() {
@Override
public boolean hasResult() {
@@ -297,7 +295,11 @@
}
}
- private static boolean isRemoteCache(RemoteOptions options) {
+ private static boolean useRemoteCache(RemoteOptions options) {
return !isNullOrEmpty(options.remoteCache);
}
+
+ private static boolean useDiskCache(RemoteOptions options) {
+ return options.diskCache != null && !options.diskCache.isEmpty();
+ }
}
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 434efb7..97a78cf 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
@@ -14,6 +14,7 @@
tags = ["bazel"],
deps = [
"//src/main/java/com/google/devtools/build/lib/remote/common",
+ "//src/main/java/com/google/devtools/build/lib/remote/options",
"//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/common/options",
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 8307f8d..b14221b 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
@@ -21,6 +21,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.MoreExecutors;
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;
@@ -37,17 +38,22 @@
private final RemoteCacheClient remoteCache;
private final DiskCacheClient diskCache;
+ private final RemoteOptions options;
- public DiskAndRemoteCacheClient(DiskCacheClient diskCache, RemoteCacheClient remoteCache) {
+ public DiskAndRemoteCacheClient(
+ DiskCacheClient diskCache, RemoteCacheClient remoteCache, RemoteOptions options) {
this.diskCache = Preconditions.checkNotNull(diskCache);
this.remoteCache = Preconditions.checkNotNull(remoteCache);
+ this.options = options;
}
@Override
public void uploadActionResult(ActionKey actionKey, ActionResult actionResult)
throws IOException, InterruptedException {
diskCache.uploadActionResult(actionKey, actionResult);
- remoteCache.uploadActionResult(actionKey, actionResult);
+ if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteUploadLocalResults) {
+ remoteCache.uploadActionResult(actionKey, actionResult);
+ }
}
@Override
@@ -60,7 +66,9 @@
public ListenableFuture<Void> uploadFile(Digest digest, Path file) {
try {
diskCache.uploadFile(digest, file).get();
- remoteCache.uploadFile(digest, file).get();
+ if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteUploadLocalResults) {
+ remoteCache.uploadFile(digest, file).get();
+ }
} catch (ExecutionException e) {
return Futures.immediateFailedFuture(e.getCause());
} catch (InterruptedException e) {
@@ -73,7 +81,9 @@
public ListenableFuture<Void> uploadBlob(Digest digest, ByteString data) {
try {
diskCache.uploadBlob(digest, data).get();
- remoteCache.uploadBlob(digest, data).get();
+ if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteUploadLocalResults) {
+ remoteCache.uploadBlob(digest, data).get();
+ }
} catch (ExecutionException e) {
return Futures.immediateFailedFuture(e.getCause());
} catch (InterruptedException e) {
@@ -130,22 +140,26 @@
return Futures.immediateFailedFuture(e);
}
- ListenableFuture<Void> download =
- closeStreamOnError(remoteCache.downloadBlob(digest, tempOut), tempOut);
- ListenableFuture<Void> saveToDiskAndTarget =
- Futures.transformAsync(
- download,
- (unused) -> {
- try {
- tempOut.close();
- diskCache.captureFile(tempPath, digest, /* isActionCache= */ false);
- } catch (IOException e) {
- return Futures.immediateFailedFuture(e);
- }
- return diskCache.downloadBlob(digest, out);
- },
- MoreExecutors.directExecutor());
- return saveToDiskAndTarget;
+ if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteAcceptCached) {
+ ListenableFuture<Void> download =
+ closeStreamOnError(remoteCache.downloadBlob(digest, tempOut), tempOut);
+ ListenableFuture<Void> saveToDiskAndTarget =
+ Futures.transformAsync(
+ download,
+ (unused) -> {
+ try {
+ tempOut.close();
+ diskCache.captureFile(tempPath, digest, /* isActionCache= */ false);
+ } catch (IOException e) {
+ return Futures.immediateFailedFuture(e);
+ }
+ return diskCache.downloadBlob(digest, out);
+ },
+ MoreExecutors.directExecutor());
+ return saveToDiskAndTarget;
+ } else {
+ return Futures.immediateFuture(null);
+ }
}
@Override
@@ -154,16 +168,20 @@
return diskCache.downloadActionResult(actionKey);
}
- return Futures.transformAsync(
- remoteCache.downloadActionResult(actionKey),
- (actionResult) -> {
- if (actionResult == null) {
- return Futures.immediateFuture(null);
- } else {
- diskCache.uploadActionResult(actionKey, actionResult);
- return Futures.immediateFuture(actionResult);
- }
- },
- MoreExecutors.directExecutor());
+ if (!options.incompatibleRemoteResultsIgnoreDisk || options.remoteAcceptCached) {
+ return Futures.transformAsync(
+ remoteCache.downloadActionResult(actionKey),
+ (actionResult) -> {
+ if (actionResult == null) {
+ return Futures.immediateFuture(null);
+ } else {
+ diskCache.uploadActionResult(actionKey, actionResult);
+ return Futures.immediateFuture(actionResult);
+ }
+ },
+ MoreExecutors.directExecutor());
+ } else {
+ return Futures.immediateFuture(null);
+ }
}
}
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 7fc27ad..290934e 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
@@ -171,6 +171,26 @@
public boolean remoteUploadLocalResults;
@Option(
+ name = "incompatible_remote_results_ignore_disk",
+ defaultValue = "false",
+ category = "remote",
+ documentationCategory = OptionDocumentationCategory.REMOTE,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ metadataTags = {
+ OptionMetadataTag.INCOMPATIBLE_CHANGE,
+ OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+ },
+ help =
+ "If set to true, --noremote_upload_local_results and --noremote_accept_cached will not"
+ + " apply to the disk cache. If a combined cache is used:\n"
+ + "\t--noremote_upload_local_results will cause results to be written to the disk"
+ + " cache, but not uploaded to the remote cache.\n"
+ + "\t--noremote_accept_cached will result in Bazel checking for results in the disk"
+ + " cache, but not in the remote cache.\n"
+ + "See #8216 for details.")
+ public boolean incompatibleRemoteResultsIgnoreDisk;
+
+ @Option(
name = "remote_instance_name",
defaultValue = "",
documentationCategory = OptionDocumentationCategory.REMOTE,
