Skip to content

Commit 128d833

Browse files
Yanniccopybara-github
Yannic
authored and
copybara-github
committed
[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`. Closes #16624. PiperOrigin-RevId: 486901751 Change-Id: I0991f6891e21711df1e23ae0998a8bc95e2389bc
1 parent 9cb5e0a commit 128d833

File tree

7 files changed

+38
-40
lines changed

7 files changed

+38
-40
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,11 @@ public ListenableFuture<ImmutableSet<Digest>> findMissingDigests(
122122
return cacheProtocol.findMissingDigests(context, digests);
123123
}
124124

125+
/** Returns whether the action cache supports updating action results. */
126+
public boolean actionCacheSupportsUpdate() {
127+
return cacheCapabilities.getActionCacheUpdateCapabilities().getUpdateEnabled();
128+
}
129+
125130
/** Upload the action result to the remote cache. */
126131
public ListenableFuture<Void> uploadActionResult(
127132
RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) {

‎src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,8 @@ public CachePolicy getWriteCachePolicy(Spawn spawn) {
305305

306306
if (useRemoteCache(remoteOptions)) {
307307
allowRemoteCache =
308-
shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo());
308+
shouldUploadLocalResultsToRemoteCache(remoteOptions, spawn.getExecutionInfo())
309+
&& remoteCache.actionCacheSupportsUpdate();
309310
if (useDiskCache(remoteOptions)) {
310311
// Combined cache
311312
if (remoteOptions.incompatibleRemoteResultsIgnoreDisk) {

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import static java.util.concurrent.TimeUnit.SECONDS;
1818

19+
import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities;
1920
import build.bazel.remote.execution.v2.CacheCapabilities;
2021
import build.bazel.remote.execution.v2.DigestFunction;
2122
import build.bazel.remote.execution.v2.ServerCapabilities;
@@ -27,7 +28,6 @@
2728
import com.google.common.base.Strings;
2829
import com.google.common.base.Throwables;
2930
import com.google.common.collect.ImmutableList;
30-
import com.google.common.flogger.GoogleLogger;
3131
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
3232
import com.google.common.util.concurrent.MoreExecutors;
3333
import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -112,11 +112,10 @@
112112

113113
/** RemoteModule provides distributed cache and remote execution for Bazel. */
114114
public final class RemoteModule extends BlazeModule {
115-
116-
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
117-
118-
private static final CacheCapabilities DISK_CACHE_CAPABILITIES =
115+
private static final CacheCapabilities HTTP_AND_DISK_CACHE_CAPABILITIES =
119116
CacheCapabilities.newBuilder()
117+
.setActionCacheUpdateCapabilities(
118+
ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build())
120119
.setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.ALLOWED)
121120
.build();
122121

@@ -247,7 +246,7 @@ private void initHttpAndDiskCache(
247246
return;
248247
}
249248
RemoteCache remoteCache =
250-
new RemoteCache(DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil);
249+
new RemoteCache(HTTP_AND_DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil);
251250
actionContextProvider =
252251
RemoteActionContextProvider.createForRemoteCaching(
253252
executorService, env, remoteCache, /* retryScheduler= */ null, digestUtil);

‎src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java‎

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import build.bazel.remote.execution.v2.PriorityCapabilities.PriorityRange;
2626
import build.bazel.remote.execution.v2.RequestMetadata;
2727
import build.bazel.remote.execution.v2.ServerCapabilities;
28-
import com.google.common.base.Strings;
2928
import com.google.common.collect.ImmutableList;
3029
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
3130
import com.google.devtools.build.lib.remote.options.RemoteOptions;
@@ -234,25 +233,12 @@ public static ClientServerCompatibilityStatus checkClientServerCompatibility(
234233
digestFunction, cacheCap.getDigestFunctionsList()));
235234
}
236235

237-
// Check updating remote cache is allowed, if we ever need to do that.
238-
boolean remoteExecution = !Strings.isNullOrEmpty(remoteOptions.remoteExecutor);
239-
if (remoteExecution) {
240-
if (remoteOptions.remoteLocalFallback
241-
&& remoteOptions.remoteUploadLocalResults
242-
&& !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) {
243-
result.addError(
244-
"--remote_local_fallback and --remote_upload_local_results are set, "
245-
+ "but the current account is not authorized to write local results "
246-
+ "to the remote cache.");
247-
}
248-
} else {
249-
// Local execution: check updating remote cache is allowed.
250-
if (remoteOptions.remoteUploadLocalResults
251-
&& !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) {
252-
result.addError(
253-
"--remote_upload_local_results is set, but the current account is not authorized "
254-
+ "to write local results to the remote cache.");
255-
}
236+
if (remoteOptions.remoteUploadLocalResults
237+
&& !cacheCap.getActionCacheUpdateCapabilities().getUpdateEnabled()) {
238+
result.addWarning(
239+
"--remote_upload_local_results is set, but the remote cache does not support uploading "
240+
+ "action results or the current account is not authorized to write local results "
241+
+ "to the remote cache.");
256242
}
257243

258244
if (remoteOptions.cacheCompression

‎src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,9 @@ public String getTypeDescription() {
273273
defaultValue = "true",
274274
documentationCategory = OptionDocumentationCategory.REMOTE,
275275
effectTags = {OptionEffectTag.UNKNOWN},
276-
help = "Whether to upload locally executed action results to the remote cache.")
276+
help =
277+
"Whether to upload locally executed action results to the remote cache if the remote "
278+
+ "cache supports it and the user is authorized to do so.")
277279
public boolean remoteUploadLocalResults;
278280

279281
@Deprecated

‎src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,7 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportDigestFu
308308
}
309309

310310
@Test
311-
public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate()
312-
throws Exception {
311+
public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate() {
313312
ServerCapabilities caps =
314313
ServerCapabilities.newBuilder()
315314
.setLowApiVersion(ApiVersion.current.toSemVer())
@@ -324,9 +323,10 @@ public void testCheckClientServerCompatibility_remoteCacheDoesNotSupportUpdate()
324323
RemoteServerCapabilities.ClientServerCompatibilityStatus st =
325324
RemoteServerCapabilities.checkClientServerCompatibility(
326325
caps, remoteOptions, DigestFunction.Value.SHA256, ServerCapabilitiesRequirement.CACHE);
327-
assertThat(st.getErrors()).hasSize(1);
328-
assertThat(st.getErrors().get(0))
329-
.containsMatch("not authorized to write local results to the remote cache");
326+
assertThat(st.getErrors()).isEmpty();
327+
assertThat(st.getWarnings()).hasSize(1);
328+
assertThat(st.getWarnings().get(0))
329+
.contains("remote cache does not support uploading action results");
330330

331331
// Ignored when no local upload.
332332
remoteOptions.remoteUploadLocalResults = false;
@@ -398,8 +398,7 @@ public void testCheckClientServerCompatibility_remoteExecutionDoesNotSupportDige
398398
}
399399

400400
@Test
401-
public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate()
402-
throws Exception {
401+
public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate() {
403402
ServerCapabilities caps =
404403
ServerCapabilities.newBuilder()
405404
.setLowApiVersion(ApiVersion.current.toSemVer())
@@ -423,9 +422,10 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate(
423422
remoteOptions,
424423
DigestFunction.Value.SHA256,
425424
ServerCapabilitiesRequirement.EXECUTION_AND_CACHE);
426-
assertThat(st.getErrors()).hasSize(1);
427-
assertThat(st.getErrors().get(0))
428-
.containsMatch("not authorized to write local results to the remote cache");
425+
assertThat(st.getErrors()).isEmpty();
426+
assertThat(st.getWarnings()).hasSize(1);
427+
assertThat(st.getWarnings().get(0))
428+
.contains("remote cache does not support uploading action results");
429429

430430
// Ignored when no fallback.
431431
remoteOptions.remoteLocalFallback = false;
@@ -435,7 +435,10 @@ public void testCheckClientServerCompatibility_localFallbackNoRemoteCacheUpdate(
435435
remoteOptions,
436436
DigestFunction.Value.SHA256,
437437
ServerCapabilitiesRequirement.EXECUTION_AND_CACHE);
438-
assertThat(st.isOk()).isTrue();
438+
assertThat(st.getErrors()).isEmpty();
439+
assertThat(st.getWarnings()).hasSize(1);
440+
assertThat(st.getWarnings().get(0))
441+
.contains("remote cache does not support uploading action results");
439442

440443
// Ignored when no uploading local results.
441444
remoteOptions.remoteLocalFallback = true;

‎src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
1717

18+
import build.bazel.remote.execution.v2.ActionCacheUpdateCapabilities;
1819
import build.bazel.remote.execution.v2.CacheCapabilities;
1920
import build.bazel.remote.execution.v2.Digest;
2021
import build.bazel.remote.execution.v2.Directory;
@@ -33,9 +34,10 @@
3334

3435
/** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */
3536
class OnDiskBlobStoreCache extends RemoteCache {
36-
3737
private static final CacheCapabilities CAPABILITIES =
3838
CacheCapabilities.newBuilder()
39+
.setActionCacheUpdateCapabilities(
40+
ActionCacheUpdateCapabilities.newBuilder().setUpdateEnabled(true).build())
3941
.setSymlinkAbsolutePathStrategy(SymlinkAbsolutePathStrategy.Value.ALLOWED)
4042
.build();
4143

0 commit comments

Comments
 (0)