Skip to content

Commit c9b7e22

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Remote: Don't load remote metadata from action cache if remote cache is disabled.
Part of #14252. Closes #14252. PiperOrigin-RevId: 410448656
1 parent 1e1a740 commit c9b7e22

4 files changed

Lines changed: 62 additions & 12 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,8 @@ public Token getTokenIfNeedToExecute(
414414
EventHandler handler,
415415
MetadataHandler metadataHandler,
416416
ArtifactExpander artifactExpander,
417-
Map<String, String> remoteDefaultPlatformProperties)
417+
Map<String, String> remoteDefaultPlatformProperties,
418+
boolean isRemoteCacheEnabled)
418419
throws InterruptedException {
419420
// TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that
420421
// produce only symlinks we should not check whether inputs are valid at all - all that matters
@@ -456,8 +457,11 @@ public Token getTokenIfNeedToExecute(
456457
}
457458
ActionCache.Entry entry = getCacheEntry(action);
458459
CachedOutputMetadata cachedOutputMetadata = null;
459-
// load remote metadata from action cache
460-
if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) {
460+
if (entry != null
461+
&& !entry.isCorrupted()
462+
&& cacheConfig.storeOutputMetadata()
463+
&& isRemoteCacheEnabled) {
464+
// load remote metadata from action cache
461465
cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler);
462466
}
463467

‎src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,7 @@ Token checkActionCache(
599599
remoteOptions != null
600600
? remoteOptions.getRemoteDefaultExecProperties()
601601
: ImmutableSortedMap.of();
602+
boolean isRemoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled();
602603
token =
603604
actionCacheChecker.getTokenIfNeedToExecute(
604605
action,
@@ -609,7 +610,8 @@ Token checkActionCache(
609610
: null,
610611
metadataHandler,
611612
artifactExpander,
612-
remoteDefaultProperties);
613+
remoteDefaultProperties,
614+
isRemoteCacheEnabled);
613615
} catch (UserExecException e) {
614616
throw e.toActionExecutionException(action);
615617
}

‎src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,8 @@ private void runAction(
183183
/*handler=*/ null,
184184
metadataHandler,
185185
/*artifactExpander=*/ null,
186-
platform);
186+
platform,
187+
/*isRemoteCacheEnabled=*/ true);
187188
if (token != null) {
188189
// Real action execution would happen here.
189190
ActionExecutionContext context = mock(ActionExecutionContext.class);
@@ -440,7 +441,8 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio
440441
/*handler=*/ null,
441442
new FakeMetadataHandler(),
442443
/*artifactExpander=*/ null,
443-
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of()))
444+
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
445+
/*isRemoteCacheEnabled=*/ true))
444446
.isNotNull();
445447
}
446448

@@ -557,7 +559,8 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception {
557559
/*handler=*/ null,
558560
metadataHandler,
559561
/*artifactExpander=*/ null,
560-
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
562+
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
563+
/*isRemoteCacheEnabled=*/ true);
561564

562565
assertThat(output.getPath().exists()).isFalse();
563566
assertThat(token).isNull();
@@ -567,6 +570,33 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception {
567570
assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content));
568571
}
569572

573+
@Test
574+
public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoaded()
575+
throws Exception {
576+
cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true);
577+
Artifact output = createArtifact(artifactRoot, "bin/dummy");
578+
String content = "content";
579+
Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content));
580+
MetadataHandler metadataHandler = new FakeMetadataHandler();
581+
582+
runAction(action);
583+
Token token =
584+
cacheChecker.getTokenIfNeedToExecute(
585+
action,
586+
/*resolvedCacheArtifacts=*/ null,
587+
/*clientEnv=*/ ImmutableMap.of(),
588+
/*handler=*/ null,
589+
metadataHandler,
590+
/*artifactExpander=*/ null,
591+
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
592+
/*isRemoteCacheEnabled=*/ false);
593+
594+
assertThat(output.getPath().exists()).isFalse();
595+
assertThat(token).isNotNull();
596+
ActionCache.Entry entry = cache.get(output.getExecPathString());
597+
assertThat(entry).isNull();
598+
}
599+
570600
@Test
571601
public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached() throws Exception {
572602
cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true);
@@ -679,7 +709,8 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception {
679709
/*handler=*/ null,
680710
metadataHandler,
681711
/*artifactExpander=*/ null,
682-
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
712+
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
713+
/*isRemoteCacheEnabled=*/ true);
683714

684715
assertThat(token).isNull();
685716
assertThat(output.getPath().exists()).isFalse();
@@ -763,7 +794,8 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex
763794
/*handler=*/ null,
764795
metadataHandler,
765796
/*artifactExpander=*/ null,
766-
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
797+
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
798+
/*isRemoteCacheEnabled=*/ true);
767799

768800
TreeArtifactValue expectedMetadata = createTreeMetadata(output, children, Optional.empty());
769801
assertThat(token).isNull();
@@ -890,7 +922,8 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th
890922
/*handler=*/ null,
891923
metadataHandler,
892924
/*artifactExpander=*/ null,
893-
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of());
925+
/*remoteDefaultPlatformProperties=*/ ImmutableMap.of(),
926+
/*isRemoteCacheEnabled=*/ true);
894927

895928
assertThat(token).isNull();
896929
assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build());
@@ -905,8 +938,9 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th
905938
output,
906939
ImmutableMap.of(
907940
"file1",
908-
FileArtifactValue.createForTesting(output.getPath().getRelative("file1")),
909-
"file2", createRemoteFileMetadata("content2")),
941+
FileArtifactValue.createForTesting(output.getPath().getRelative("file1")),
942+
"file2",
943+
createRemoteFileMetadata("content2")),
910944
Optional.empty()));
911945
}
912946

‎src/test/shell/bazel/remote/remote_execution_test.sh‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3205,6 +3205,14 @@ EOF
32053205
}
32063206

32073207
function test_download_toplevel_when_turn_remote_cache_off() {
3208+
download_toplevel_when_turn_remote_cache_off
3209+
}
3210+
3211+
function test_download_toplevel_when_turn_remote_cache_off_with_metadata() {
3212+
download_toplevel_when_turn_remote_cache_off --experimental_action_cache_store_output_metadata
3213+
}
3214+
3215+
function download_toplevel_when_turn_remote_cache_off() {
32083216
# Test that BwtB doesn't cause build failure if remote cache is disabled in a following build.
32093217
# See https://github.com/bazelbuild/bazel/issues/13882.
32103218

@@ -3239,6 +3247,7 @@ EOF
32393247
bazel build \
32403248
--remote_cache=grpc://localhost:${worker_port} \
32413249
--remote_download_toplevel \
3250+
"$@" \
32423251
//a:consumer >& $TEST_log || fail "Failed to download outputs"
32433252
[[ -f bazel-bin/a/a.txt ]] || [[ -f bazel-bin/a/b.txt ]] \
32443253
&& fail "Expected outputs of producer are not downloaded"
@@ -3247,6 +3256,7 @@ EOF
32473256
echo 'bar' > a/in.txt
32483257
bazel build \
32493258
--remote_download_toplevel \
3259+
"$@" \
32503260
//a:consumer >& $TEST_log || fail "Failed to build without remote cache"
32513261
}
32523262

0 commit comments

Comments
 (0)