Skip to content

Commit 23ffce5

Browse files
coeuvrecopybara-github
authored andcommitted
Update GetActionResult for disk cache to check referenced files when …
…building without the bytes. Closes bazelbuild#16731. PiperOrigin-RevId: 490262565 Change-Id: I342ec2371b81b9e23fc7935e30d5fed8fb6d4005
1 parent 7201e74 commit 23ffce5

File tree

7 files changed

+240
-8
lines changed

7 files changed

+240
-8
lines changed

src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java

+16-4
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,13 @@ public static RemoteCacheClient createDiskAndRemoteClient(
4444
Path workingDirectory,
4545
PathFragment diskCachePath,
4646
boolean remoteVerifyDownloads,
47+
boolean checkActionResult,
4748
DigestUtil digestUtil,
4849
RemoteCacheClient remoteCacheClient)
4950
throws IOException {
5051
DiskCacheClient diskCacheClient =
51-
createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil);
52+
createDiskCache(
53+
workingDirectory, diskCachePath, remoteVerifyDownloads, checkActionResult, digestUtil);
5254
return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
5355
}
5456

@@ -69,7 +71,11 @@ public static RemoteCacheClient create(
6971
}
7072
if (isDiskCache(options)) {
7173
return createDiskCache(
72-
workingDirectory, options.diskCache, options.remoteVerifyDownloads, digestUtil);
74+
workingDirectory,
75+
options.diskCache,
76+
options.remoteVerifyDownloads,
77+
!options.remoteOutputsMode.downloadAllOutputs(),
78+
digestUtil);
7379
}
7480
throw new IllegalArgumentException(
7581
"Unrecognized RemoteOptions configuration: remote Http cache URL and/or local disk cache"
@@ -128,14 +134,15 @@ private static DiskCacheClient createDiskCache(
128134
Path workingDirectory,
129135
PathFragment diskCachePath,
130136
boolean verifyDownloads,
137+
boolean checkActionResult,
131138
DigestUtil digestUtil)
132139
throws IOException {
133140
Path cacheDir =
134141
workingDirectory.getRelative(Preconditions.checkNotNull(diskCachePath, "diskCachePath"));
135142
if (!cacheDir.exists()) {
136143
cacheDir.createDirectoryAndParents();
137144
}
138-
return new DiskCacheClient(cacheDir, verifyDownloads, digestUtil);
145+
return new DiskCacheClient(cacheDir, verifyDownloads, checkActionResult, digestUtil);
139146
}
140147

141148
private static RemoteCacheClient createDiskAndHttpCache(
@@ -154,7 +161,12 @@ private static RemoteCacheClient createDiskAndHttpCache(
154161

155162
RemoteCacheClient httpCache = createHttp(options, cred, authAndTlsOptions, digestUtil);
156163
return createDiskAndRemoteClient(
157-
workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache);
164+
workingDirectory,
165+
diskCachePath,
166+
options.remoteVerifyDownloads,
167+
!options.remoteOutputsMode.downloadAllOutputs(),
168+
digestUtil,
169+
httpCache);
158170
}
159171

160172
public static boolean isDiskCache(RemoteOptions options) {

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

+2
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
585585
env.getWorkingDirectory(),
586586
remoteOptions.diskCache,
587587
remoteOptions.remoteVerifyDownloads,
588+
!remoteOptions.remoteOutputsMode.downloadAllOutputs(),
588589
digestUtil,
589590
cacheClient);
590591
} catch (IOException e) {
@@ -644,6 +645,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
644645
env.getWorkingDirectory(),
645646
remoteOptions.diskCache,
646647
remoteOptions.remoteVerifyDownloads,
648+
!remoteOptions.remoteOutputsMode.downloadAllOutputs(),
647649
digestUtil,
648650
cacheClient);
649651
} catch (IOException e) {

src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java

+60-3
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
import build.bazel.remote.execution.v2.ActionResult;
1717
import build.bazel.remote.execution.v2.Digest;
18+
import build.bazel.remote.execution.v2.Directory;
19+
import build.bazel.remote.execution.v2.Tree;
1820
import com.google.common.collect.ImmutableSet;
1921
import com.google.common.io.ByteStreams;
2022
import com.google.common.util.concurrent.Futures;
@@ -28,6 +30,7 @@
2830
import com.google.devtools.build.lib.remote.util.Utils;
2931
import com.google.devtools.build.lib.vfs.Path;
3032
import com.google.protobuf.ByteString;
33+
import com.google.protobuf.ExtensionRegistryLite;
3134
import java.io.FileOutputStream;
3235
import java.io.IOException;
3336
import java.io.InputStream;
@@ -43,11 +46,20 @@ public class DiskCacheClient implements RemoteCacheClient {
4346

4447
private final Path root;
4548
private final boolean verifyDownloads;
49+
private final boolean checkActionResult;
4650
private final DigestUtil digestUtil;
4751

48-
public DiskCacheClient(Path root, boolean verifyDownloads, DigestUtil digestUtil) {
52+
/**
53+
* @param verifyDownloads whether verify the digest of downloaded content are the same as the
54+
* digest used to index that file.
55+
* @param checkActionResult whether check referenced blobs exist in CAS when checking AC. If this
56+
* is {@code true} and blobs referenced by the AC are missing, ignore the AC.
57+
*/
58+
public DiskCacheClient(
59+
Path root, boolean verifyDownloads, boolean checkActionResult, DigestUtil digestUtil) {
4960
this.root = root;
5061
this.verifyDownloads = verifyDownloads;
62+
this.checkActionResult = checkActionResult;
5163
this.digestUtil = digestUtil;
5264
}
5365

@@ -102,13 +114,58 @@ public ListenableFuture<Void> downloadBlob(
102114
MoreExecutors.directExecutor());
103115
}
104116

117+
private void checkDigestExists(Digest digest) throws CacheNotFoundException {
118+
if (digest.getSizeBytes() == 0) {
119+
return;
120+
}
121+
122+
if (!toPath(digest.getHash(), /* actionResult= */ false).exists()) {
123+
throw new CacheNotFoundException(digest);
124+
}
125+
}
126+
127+
private void checkOutputDirectory(Directory dir) throws CacheNotFoundException {
128+
for (var file : dir.getFilesList()) {
129+
checkDigestExists(file.getDigest());
130+
}
131+
}
132+
133+
private void checkActionResult(ActionResult actionResult) throws IOException {
134+
for (var outputFile : actionResult.getOutputFilesList()) {
135+
checkDigestExists(outputFile.getDigest());
136+
}
137+
138+
for (var outputDirectory : actionResult.getOutputDirectoriesList()) {
139+
var treeDigest = outputDirectory.getTreeDigest();
140+
checkDigestExists(treeDigest);
141+
142+
var treePath = toPath(treeDigest.getHash(), /* actionResult= */ false);
143+
var tree =
144+
Tree.parseFrom(treePath.getInputStream(), ExtensionRegistryLite.getEmptyRegistry());
145+
checkOutputDirectory(tree.getRoot());
146+
for (var dir : tree.getChildrenList()) {
147+
checkOutputDirectory(dir);
148+
}
149+
}
150+
}
151+
105152
@Override
106153
public ListenableFuture<CachedActionResult> downloadActionResult(
107154
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
108-
return Futures.transform(
155+
return Futures.transformAsync(
109156
Utils.downloadAsActionResult(
110157
actionKey, (digest, out) -> download(digest, out, /* isActionCache= */ true)),
111-
CachedActionResult::disk,
158+
actionResult -> {
159+
if (actionResult == null) {
160+
return Futures.immediateFuture(null);
161+
}
162+
163+
if (checkActionResult) {
164+
checkActionResult(actionResult);
165+
}
166+
167+
return Futures.immediateFuture(CachedActionResult.disk(actionResult));
168+
},
112169
MoreExecutors.directExecutor());
113170
}
114171

src/test/java/com/google/devtools/build/lib/remote/BUILD

+16
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ java_test(
4040
exclude = NATIVE_SSL_TEST + [
4141
"BuildWithoutTheBytesIntegrationTest.java",
4242
"BuildWithoutTheBytesIntegrationTestBase.java",
43+
"DiskCacheIntegrationTest.java",
4344
],
4445
) + NATIVE_SSL_TEST_MAYBE,
4546
test_class = "com.google.devtools.build.lib.AllTests",
@@ -163,3 +164,18 @@ java_test(
163164
"//third_party:truth",
164165
],
165166
)
167+
168+
java_test(
169+
name = "DiskCacheIntegrationTest",
170+
srcs = ["DiskCacheIntegrationTest.java"],
171+
deps = [
172+
"//src/main/java/com/google/devtools/build/lib:runtime",
173+
"//src/main/java/com/google/devtools/build/lib/remote",
174+
"//src/main/java/com/google/devtools/build/lib/standalone",
175+
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
176+
"//src/test/java/com/google/devtools/build/lib/buildtool/util",
177+
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
178+
"//third_party:guava",
179+
"//third_party:junit4",
180+
],
181+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
// Copyright 2022 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.remote;
15+
16+
import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile;
17+
18+
import com.google.common.collect.ImmutableList;
19+
import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
20+
import com.google.devtools.build.lib.runtime.BlazeModule;
21+
import com.google.devtools.build.lib.runtime.BlazeRuntime;
22+
import com.google.devtools.build.lib.runtime.BlockWaitingModule;
23+
import com.google.devtools.build.lib.runtime.BuildSummaryStatsModule;
24+
import com.google.devtools.build.lib.standalone.StandaloneModule;
25+
import com.google.devtools.build.lib.vfs.PathFragment;
26+
import java.io.IOException;
27+
import org.junit.After;
28+
import org.junit.Test;
29+
import org.junit.runner.RunWith;
30+
import org.junit.runners.JUnit4;
31+
32+
@RunWith(JUnit4.class)
33+
public class DiskCacheIntegrationTest extends BuildIntegrationTestCase {
34+
35+
private static PathFragment getDiskCacheDir() {
36+
PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
37+
return testTmpDir.getRelative("disk_cache");
38+
}
39+
40+
@Override
41+
protected void setupOptions() throws Exception {
42+
super.setupOptions();
43+
44+
addOptions("--disk_cache=" + getDiskCacheDir());
45+
}
46+
47+
@After
48+
public void tearDown() throws IOException {
49+
getWorkspace().getFileSystem().getPath(getDiskCacheDir()).deleteTree();
50+
}
51+
52+
@Override
53+
protected ImmutableList<BlazeModule> getSpawnModules() {
54+
return ImmutableList.<BlazeModule>builder()
55+
.addAll(super.getSpawnModules())
56+
.add(new StandaloneModule())
57+
.build();
58+
}
59+
60+
@Override
61+
protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception {
62+
return super.getRuntimeBuilder()
63+
.addBlazeModule(new RemoteModule())
64+
.addBlazeModule(new BuildSummaryStatsModule())
65+
.addBlazeModule(new BlockWaitingModule());
66+
}
67+
68+
@Test
69+
public void hitDiskCache() throws Exception {
70+
write(
71+
"BUILD",
72+
"genrule(",
73+
" name = 'foo',",
74+
" srcs = ['foo.in'],",
75+
" outs = ['foo.out'],",
76+
" cmd = 'cat $(SRCS) > $@',",
77+
")",
78+
"genrule(",
79+
" name = 'foobar',",
80+
" srcs = [':foo.out', 'bar.in'],",
81+
" outs = ['foobar.out'],",
82+
" cmd = 'cat $(SRCS) > $@',",
83+
")");
84+
write("foo.in", "foo");
85+
write("bar.in", "bar");
86+
buildTarget("//:foobar");
87+
cleanAndRestartServer();
88+
89+
buildTarget("//:foobar");
90+
91+
events.assertContainsInfo("2 disk cache hit");
92+
}
93+
94+
private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOptions)
95+
throws Exception {
96+
// Arrange: Prepare the workspace and populate disk cache
97+
write(
98+
"BUILD",
99+
"genrule(",
100+
" name = 'foo',",
101+
" srcs = ['foo.in'],",
102+
" outs = ['foo.out'],",
103+
" cmd = 'cat $(SRCS) > $@',",
104+
")",
105+
"genrule(",
106+
" name = 'foobar',",
107+
" srcs = [':foo.out', 'bar.in'],",
108+
" outs = ['foobar.out'],",
109+
" cmd = 'cat $(SRCS) > $@',",
110+
")");
111+
write("foo.in", "foo");
112+
write("bar.in", "bar");
113+
addOptions(additionalOptions);
114+
buildTarget("//:foobar");
115+
cleanAndRestartServer();
116+
117+
// Act: Delete blobs in CAS from disk cache and do a clean build
118+
getWorkspace().getFileSystem().getPath(getDiskCacheDir().getRelative("cas")).deleteTree();
119+
addOptions(additionalOptions);
120+
buildTarget("//:foobar");
121+
122+
// Assert: Should ignore the stale AC and rerun the generating action
123+
events.assertDoesNotContainEvent("disk cache hit");
124+
}
125+
126+
@Test
127+
public void blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
128+
blobsReferencedInAAreMissingCasIgnoresAc();
129+
}
130+
131+
@Test
132+
public void bwob_blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
133+
blobsReferencedInAAreMissingCasIgnoresAc("--remote_download_minimal");
134+
}
135+
136+
private void cleanAndRestartServer() throws Exception {
137+
getOutputBase().getRelative("action_cache").deleteTreesBelow();
138+
// Simulates a server restart
139+
createRuntimeWrapper();
140+
}
141+
}

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

+3
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import build.bazel.remote.execution.v2.RequestMetadata;
2323
import build.bazel.remote.execution.v2.UpdateActionResultRequest;
2424
import com.google.common.flogger.GoogleLogger;
25+
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
2526
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
2627
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
2728
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.CachedActionResult;
@@ -59,6 +60,8 @@ public void getActionResult(
5960

6061
responseObserver.onNext(result.actionResult());
6162
responseObserver.onCompleted();
63+
} catch (CacheNotFoundException e) {
64+
responseObserver.onError(StatusUtils.notFoundError(request.getActionDigest()));
6265
} catch (Exception e) {
6366
logger.atWarning().withCause(e).log("getActionResult request failed");
6467
responseObserver.onError(StatusUtils.internalError(e));

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ class OnDiskBlobStoreCache extends RemoteCache {
4444
public OnDiskBlobStoreCache(RemoteOptions options, Path cacheDir, DigestUtil digestUtil) {
4545
super(
4646
CAPABILITIES,
47-
new DiskCacheClient(cacheDir, /* verifyDownloads= */ true, digestUtil),
47+
new DiskCacheClient(
48+
cacheDir, /* verifyDownloads= */ true, /* checkActionResult= */ true, digestUtil),
4849
options,
4950
digestUtil);
5051
}

0 commit comments

Comments
 (0)