Skip to content

Commit

Permalink
Update GetActionResult for disk cache to check referenced files when …
Browse files Browse the repository at this point in the history
…building without the bytes.

Closes #16731.

PiperOrigin-RevId: 490262565
Change-Id: I342ec2371b81b9e23fc7935e30d5fed8fb6d4005
  • Loading branch information
coeuvre authored and copybara-github committed Nov 22, 2022
1 parent 7201e74 commit 23ffce5
Show file tree
Hide file tree
Showing 7 changed files with 240 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@ public static RemoteCacheClient createDiskAndRemoteClient(
Path workingDirectory,
PathFragment diskCachePath,
boolean remoteVerifyDownloads,
boolean checkActionResult,
DigestUtil digestUtil,
RemoteCacheClient remoteCacheClient)
throws IOException {
DiskCacheClient diskCacheClient =
createDiskCache(workingDirectory, diskCachePath, remoteVerifyDownloads, digestUtil);
createDiskCache(
workingDirectory, diskCachePath, remoteVerifyDownloads, checkActionResult, digestUtil);
return new DiskAndRemoteCacheClient(diskCacheClient, remoteCacheClient);
}

Expand All @@ -69,7 +71,11 @@ public static RemoteCacheClient create(
}
if (isDiskCache(options)) {
return createDiskCache(
workingDirectory, options.diskCache, options.remoteVerifyDownloads, digestUtil);
workingDirectory,
options.diskCache,
options.remoteVerifyDownloads,
!options.remoteOutputsMode.downloadAllOutputs(),
digestUtil);
}
throw new IllegalArgumentException(
"Unrecognized RemoteOptions configuration: remote Http cache URL and/or local disk cache"
Expand Down Expand Up @@ -128,14 +134,15 @@ private static DiskCacheClient createDiskCache(
Path workingDirectory,
PathFragment diskCachePath,
boolean verifyDownloads,
boolean checkActionResult,
DigestUtil digestUtil)
throws IOException {
Path cacheDir =
workingDirectory.getRelative(Preconditions.checkNotNull(diskCachePath, "diskCachePath"));
if (!cacheDir.exists()) {
cacheDir.createDirectoryAndParents();
}
return new DiskCacheClient(cacheDir, verifyDownloads, digestUtil);
return new DiskCacheClient(cacheDir, verifyDownloads, checkActionResult, digestUtil);
}

private static RemoteCacheClient createDiskAndHttpCache(
Expand All @@ -154,7 +161,12 @@ private static RemoteCacheClient createDiskAndHttpCache(

RemoteCacheClient httpCache = createHttp(options, cred, authAndTlsOptions, digestUtil);
return createDiskAndRemoteClient(
workingDirectory, diskCachePath, options.remoteVerifyDownloads, digestUtil, httpCache);
workingDirectory,
diskCachePath,
options.remoteVerifyDownloads,
!options.remoteOutputsMode.downloadAllOutputs(),
digestUtil,
httpCache);
}

public static boolean isDiskCache(RemoteOptions options) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
env.getWorkingDirectory(),
remoteOptions.diskCache,
remoteOptions.remoteVerifyDownloads,
!remoteOptions.remoteOutputsMode.downloadAllOutputs(),
digestUtil,
cacheClient);
} catch (IOException e) {
Expand Down Expand Up @@ -644,6 +645,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
env.getWorkingDirectory(),
remoteOptions.diskCache,
remoteOptions.remoteVerifyDownloads,
!remoteOptions.remoteOutputsMode.downloadAllOutputs(),
digestUtil,
cacheClient);
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

import build.bazel.remote.execution.v2.ActionResult;
import build.bazel.remote.execution.v2.Digest;
import build.bazel.remote.execution.v2.Directory;
import build.bazel.remote.execution.v2.Tree;
import com.google.common.collect.ImmutableSet;
import com.google.common.io.ByteStreams;
import com.google.common.util.concurrent.Futures;
Expand All @@ -28,6 +30,7 @@
import com.google.devtools.build.lib.remote.util.Utils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.protobuf.ByteString;
import com.google.protobuf.ExtensionRegistryLite;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
Expand All @@ -43,11 +46,20 @@ public class DiskCacheClient implements RemoteCacheClient {

private final Path root;
private final boolean verifyDownloads;
private final boolean checkActionResult;
private final DigestUtil digestUtil;

public DiskCacheClient(Path root, boolean verifyDownloads, DigestUtil digestUtil) {
/**
* @param verifyDownloads whether verify the digest of downloaded content are the same as the
* digest used to index that file.
* @param checkActionResult whether check referenced blobs exist in CAS when checking AC. If this
* is {@code true} and blobs referenced by the AC are missing, ignore the AC.
*/
public DiskCacheClient(
Path root, boolean verifyDownloads, boolean checkActionResult, DigestUtil digestUtil) {
this.root = root;
this.verifyDownloads = verifyDownloads;
this.checkActionResult = checkActionResult;
this.digestUtil = digestUtil;
}

Expand Down Expand Up @@ -102,13 +114,58 @@ public ListenableFuture<Void> downloadBlob(
MoreExecutors.directExecutor());
}

private void checkDigestExists(Digest digest) throws CacheNotFoundException {
if (digest.getSizeBytes() == 0) {
return;
}

if (!toPath(digest.getHash(), /* actionResult= */ false).exists()) {
throw new CacheNotFoundException(digest);
}
}

private void checkOutputDirectory(Directory dir) throws CacheNotFoundException {
for (var file : dir.getFilesList()) {
checkDigestExists(file.getDigest());
}
}

private void checkActionResult(ActionResult actionResult) throws IOException {
for (var outputFile : actionResult.getOutputFilesList()) {
checkDigestExists(outputFile.getDigest());
}

for (var outputDirectory : actionResult.getOutputDirectoriesList()) {
var treeDigest = outputDirectory.getTreeDigest();
checkDigestExists(treeDigest);

var treePath = toPath(treeDigest.getHash(), /* actionResult= */ false);
var tree =
Tree.parseFrom(treePath.getInputStream(), ExtensionRegistryLite.getEmptyRegistry());
checkOutputDirectory(tree.getRoot());
for (var dir : tree.getChildrenList()) {
checkOutputDirectory(dir);
}
}
}

@Override
public ListenableFuture<CachedActionResult> downloadActionResult(
RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) {
return Futures.transform(
return Futures.transformAsync(
Utils.downloadAsActionResult(
actionKey, (digest, out) -> download(digest, out, /* isActionCache= */ true)),
CachedActionResult::disk,
actionResult -> {
if (actionResult == null) {
return Futures.immediateFuture(null);
}

if (checkActionResult) {
checkActionResult(actionResult);
}

return Futures.immediateFuture(CachedActionResult.disk(actionResult));
},
MoreExecutors.directExecutor());
}

Expand Down
16 changes: 16 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ java_test(
exclude = NATIVE_SSL_TEST + [
"BuildWithoutTheBytesIntegrationTest.java",
"BuildWithoutTheBytesIntegrationTestBase.java",
"DiskCacheIntegrationTest.java",
],
) + NATIVE_SSL_TEST_MAYBE,
test_class = "com.google.devtools.build.lib.AllTests",
Expand Down Expand Up @@ -163,3 +164,18 @@ java_test(
"//third_party:truth",
],
)

java_test(
name = "DiskCacheIntegrationTest",
srcs = ["DiskCacheIntegrationTest.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//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/testutil:TestUtils",
"//third_party:guava",
"//third_party:junit4",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
// Copyright 2022 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.devtools.build.lib.remote;

import static com.google.devtools.build.lib.testutil.TestUtils.tmpDirFile;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.BlazeRuntime;
import com.google.devtools.build.lib.runtime.BlockWaitingModule;
import com.google.devtools.build.lib.runtime.BuildSummaryStatsModule;
import com.google.devtools.build.lib.standalone.StandaloneModule;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import org.junit.After;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class DiskCacheIntegrationTest extends BuildIntegrationTestCase {

private static PathFragment getDiskCacheDir() {
PathFragment testTmpDir = PathFragment.create(tmpDirFile().getAbsolutePath());
return testTmpDir.getRelative("disk_cache");
}

@Override
protected void setupOptions() throws Exception {
super.setupOptions();

addOptions("--disk_cache=" + getDiskCacheDir());
}

@After
public void tearDown() throws IOException {
getWorkspace().getFileSystem().getPath(getDiskCacheDir()).deleteTree();
}

@Override
protected ImmutableList<BlazeModule> getSpawnModules() {
return ImmutableList.<BlazeModule>builder()
.addAll(super.getSpawnModules())
.add(new StandaloneModule())
.build();
}

@Override
protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception {
return super.getRuntimeBuilder()
.addBlazeModule(new RemoteModule())
.addBlazeModule(new BuildSummaryStatsModule())
.addBlazeModule(new BlockWaitingModule());
}

@Test
public void hitDiskCache() throws Exception {
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();

buildTarget("//:foobar");

events.assertContainsInfo("2 disk cache hit");
}

private void blobsReferencedInAAreMissingCasIgnoresAc(String... additionalOptions)
throws Exception {
// Arrange: Prepare the workspace and populate disk cache
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");
addOptions(additionalOptions);
buildTarget("//:foobar");
cleanAndRestartServer();

// Act: Delete blobs in CAS from disk cache and do a clean build
getWorkspace().getFileSystem().getPath(getDiskCacheDir().getRelative("cas")).deleteTree();
addOptions(additionalOptions);
buildTarget("//:foobar");

// Assert: Should ignore the stale AC and rerun the generating action
events.assertDoesNotContainEvent("disk cache hit");
}

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

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

private void cleanAndRestartServer() throws Exception {
getOutputBase().getRelative("action_cache").deleteTreesBelow();
// Simulates a server restart
createRuntimeWrapper();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import build.bazel.remote.execution.v2.RequestMetadata;
import build.bazel.remote.execution.v2.UpdateActionResultRequest;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.CachedActionResult;
Expand Down Expand Up @@ -59,6 +60,8 @@ public void getActionResult(

responseObserver.onNext(result.actionResult());
responseObserver.onCompleted();
} catch (CacheNotFoundException e) {
responseObserver.onError(StatusUtils.notFoundError(request.getActionDigest()));
} catch (Exception e) {
logger.atWarning().withCause(e).log("getActionResult request failed");
responseObserver.onError(StatusUtils.internalError(e));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ class OnDiskBlobStoreCache extends RemoteCache {
public OnDiskBlobStoreCache(RemoteOptions options, Path cacheDir, DigestUtil digestUtil) {
super(
CAPABILITIES,
new DiskCacheClient(cacheDir, /* verifyDownloads= */ true, digestUtil),
new DiskCacheClient(
cacheDir, /* verifyDownloads= */ true, /* checkActionResult= */ true, digestUtil),
options,
digestUtil);
}
Expand Down

0 comments on commit 23ffce5

Please sign in to comment.