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.

Sign up for GitHub

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

[6.1.0]Do the AC integrity check for disk part of the combined cache. #17309

Merged
merged 3 commits into from
Jan 25, 2023
Merged

[6.1.0]Do the AC integrity check for disk part of the combined cache. #17309

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d8452a
Do the AC integrity check for disk part of the combined cache.
coeuvre Jan 18, 2023
3942494
Merge branch 'release-6.1.0' into ks/cherry-pick17201
ShreeM01 Jan 24, 2023
8bfadca
Merge branch 'release-6.1.0' into ks/cherry-pick17201
ShreeM01 Jan 25, 2023
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
60 changes: 41 additions & 19 deletions src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,31 +183,53 @@ public ListenableFuture<Void> downloadBlob(
}
}

private ListenableFuture<CachedActionResult> downloadActionResultFromRemote(
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
return Futures.transformAsync(
remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
(cachedActionResult) -> {
if (cachedActionResult == null) {
return immediateFuture(null);
} else {
return Futures.transform(
diskCache.uploadActionResult(context, actionKey, cachedActionResult.actionResult()),
v -> cachedActionResult,
directExecutor());
}
},
directExecutor());
}

@Override
public ListenableFuture<CachedActionResult> downloadActionResult(
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
if (context.getReadCachePolicy().allowDiskCache()
&& diskCache.containsActionResult(actionKey)) {
return diskCache.downloadActionResult(context, actionKey, inlineOutErr);
ListenableFuture<CachedActionResult> future = immediateFuture(null);

if (context.getReadCachePolicy().allowDiskCache()) {
// If Build without the Bytes is enabled, the future will likely return null
// and fallback to remote cache because AC integrity check is enabled and referenced blobs are
// probably missing from disk cache due to BwoB.
//
// TODO(chiwang): With lease service, instead of doing the integrity check against local
// filesystem, we can check whether referenced blobs are alive in the lease service to
// increase the cache-hit rate for disk cache.
future = diskCache.downloadActionResult(context, actionKey, inlineOutErr);
}

if (context.getReadCachePolicy().allowRemoteCache()) {
return Futures.transformAsync(
remoteCache.downloadActionResult(context, actionKey, inlineOutErr),
(cachedActionResult) -> {
if (cachedActionResult == null) {
return immediateFuture(null);
} else {
return Futures.transform(
diskCache.uploadActionResult(
context, actionKey, cachedActionResult.actionResult()),
v -> cachedActionResult,
directExecutor());
}
},
directExecutor());
} else {
return immediateFuture(null);
future =
Futures.transformAsync(
future,
(result) -> {
if (result == null) {
return downloadActionResultFromRemote(context, actionKey, inlineOutErr);
} else {
return immediateFuture(result);
}
},
directExecutor());
}

return future;
}
}
11 changes: 5 additions & 6 deletions src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,6 @@ public boolean contains(Digest digest) {
return toPath(digest.getHash(), /* actionResult= */ false).exists();
}

/** Returns {@code true} if the provided {@code key} is stored in the Action Cache. */
public boolean containsActionResult(ActionKey actionKey) {
return toPath(actionKey.getDigest().getHash(), /* actionResult= */ true).exists();
}

public void captureFile(Path src, Digest digest, boolean isActionCache) throws IOException {
Path target = toPath(digest.getHash(), isActionCache);
target.getParentDirectory().createDirectoryAndParents();
Expand Down Expand Up @@ -161,7 +156,11 @@ public ListenableFuture<CachedActionResult> downloadActionResult(
}

if (checkActionResult) {
checkActionResult(actionResult);
try {
checkActionResult(actionResult);
} catch (CacheNotFoundException e) {
return Futures.immediateFuture(null);
}
}

return Futures.immediateFuture(CachedActionResult.disk(actionResult));
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,20 @@ java_test(
java_test(
name = "DiskCacheIntegrationTest",
srcs = ["DiskCacheIntegrationTest.java"],
runtime_deps = [
"//third_party/grpc-java:grpc-jar",
],
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module",
"//src/main/java/com/google/devtools/build/lib/remote",
"//src/main/java/com/google/devtools/build/lib/standalone",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/test/java/com/google/devtools/build/lib/buildtool/util",
"//src/test/java/com/google/devtools/build/lib/remote/util:integration_test_utils",
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
"//third_party:guava",
"//third_party:junit4",
"//third_party:truth",
],
)
95 changes: 90 additions & 5 deletions src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@
// limitations under the License.
package com.google.devtools.build.lib.remote;

import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule;
import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
import com.google.devtools.build.lib.remote.util.IntegrationTestUtils;
import com.google.devtools.build.lib.remote.util.IntegrationTestUtils.WorkerInstance;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.BlazeRuntime;
import com.google.devtools.build.lib.runtime.BlockWaitingModule;
Expand All @@ -32,6 +35,25 @@

@RunWith(JUnit4.class)
public class DiskCacheIntegrationTest extends BuildIntegrationTestCase {
private WorkerInstance worker;

private void startWorker() throws Exception {
if (worker == null) {
worker = IntegrationTestUtils.startWorker();
}
}

private void enableRemoteExec(String... additionalOptions) {
assertThat(worker).isNotNull();
addOptions("--remote_executor=grpc://localhost:" + worker.getPort());
addOptions(additionalOptions);
}

private void enableRemoteCache(String... additionalOptions) {
assertThat(worker).isNotNull();
addOptions("--remote_cache=grpc://localhost:" + worker.getPort());
addOptions(additionalOptions);
}

private static PathFragment getDiskCacheDir() {
PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
Expand All @@ -48,6 +70,10 @@ protected void setupOptions() throws Exception {
@After
public void tearDown() throws IOException {
getWorkspace().getFileSystem().getPath(getDiskCacheDir()).deleteTree();

if (worker != null) {
worker.stop();
}
}

@Override
Expand Down Expand Up @@ -93,7 +119,7 @@ public void hitDiskCache() throws Exception {
events.assertContainsInfo("2 disk cache hit");
}

private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOptions)
private void doBlobsReferencedInAcAreMissingFromCasIgnoresAc(String... additionalOptions)
throws Exception {
// Arrange: Prepare the workspace and populate disk cache
write(
Expand Down Expand Up @@ -126,13 +152,72 @@ private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOption
}

@Test
public void blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
blobsReferencedInAAreMissingCasIgnoresAc();
public void blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
}

@Test
public void bwob_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
doBlobsReferencedInAcAreMissingFromCasIgnoresAc("--remote_download_minimal");
}

@Test
public void bwob_blobsReferencedInAcAreMissingCas_ignoresAc() throws Exception {
blobsReferencedInAAreMissingCasIgnoresAc("--remote_download_minimal");
public void bwobAndRemoteExec_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
startWorker();
enableRemoteExec("--remote_download_minimal");
doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
}

@Test
public void bwobAndRemoteCache_blobsReferencedInAcAreMissingFromCas_ignoresAc() throws Exception {
startWorker();
enableRemoteCache("--remote_download_minimal");
doBlobsReferencedInAcAreMissingFromCasIgnoresAc();
}

private void doRemoteExecWithDiskCache(String... additionalOptions) throws Exception {
// Arrange: Prepare the workspace and populate disk cache
startWorker();
enableRemoteExec(additionalOptions);
write(
"BUILD",
"genrule(",
" name = 'foo',",
" srcs = ['foo.in'],",
" outs = ['foo.out'],",
" cmd = 'cat $(SRCS) > $@',",
")",
"genrule(",
" name = 'foobar',",
" srcs = [':foo.out', 'bar.in'],",
" outs = ['foobar.out'],",
" cmd = 'cat $(SRCS) > $@',",
")");
write("foo.in", "foo");
write("bar.in", "bar");
buildTarget("//:foobar");
cleanAndRestartServer();

// Act: Do a clean build
enableRemoteExec("--remote_download_minimal");
buildTarget("//:foobar");
}

@Test
public void remoteExecWithDiskCache_hitDiskCache() throws Exception {
doRemoteExecWithDiskCache();

// Assert: Should hit the disk cache
events.assertContainsInfo("2 disk cache hit");
}

@Test
public void bwob_remoteExecWithDiskCache_hitRemoteCache() throws Exception {
doRemoteExecWithDiskCache("--remote_download_minimal");

// Assert: Should hit the remote cache because blobs referenced by the AC are missing from disk
// cache due to BwoB.
events.assertContainsInfo("2 remote cache hit");
}

private void cleanAndRestartServer() throws Exception {
Expand Down