From 23ffce53b92ef96432d8c6c58cc95ac362bab1bc Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 22 Nov 2022 09:28:49 -0800 Subject: [PATCH] =?UTF-8?q?Update=20GetActionResult=20for=20disk=20cache?= =?UTF-8?q?=20to=20check=20referenced=20files=20when=20=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …building without the bytes. Closes #16731. PiperOrigin-RevId: 490262565 Change-Id: I342ec2371b81b9e23fc7935e30d5fed8fb6d4005 --- .../lib/remote/RemoteCacheClientFactory.java | 20 ++- .../build/lib/remote/RemoteModule.java | 2 + .../lib/remote/disk/DiskCacheClient.java | 63 +++++++- .../google/devtools/build/lib/remote/BUILD | 16 ++ .../lib/remote/DiskCacheIntegrationTest.java | 141 ++++++++++++++++++ .../remote/worker/ActionCacheServer.java | 3 + .../remote/worker/OnDiskBlobStoreCache.java | 3 +- 7 files changed, 240 insertions(+), 8 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java index 9c6cd0fbbdb769..8f68923f9fffcd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCacheClientFactory.java @@ -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); } @@ -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" @@ -128,6 +134,7 @@ private static DiskCacheClient createDiskCache( Path workingDirectory, PathFragment diskCachePath, boolean verifyDownloads, + boolean checkActionResult, DigestUtil digestUtil) throws IOException { Path cacheDir = @@ -135,7 +142,7 @@ private static DiskCacheClient createDiskCache( if (!cacheDir.exists()) { cacheDir.createDirectoryAndParents(); } - return new DiskCacheClient(cacheDir, verifyDownloads, digestUtil); + return new DiskCacheClient(cacheDir, verifyDownloads, checkActionResult, digestUtil); } private static RemoteCacheClient createDiskAndHttpCache( @@ -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) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index b383a3796c6fa1..b57d116f99b25b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -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) { @@ -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) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index c82f9d387ccabe..d0d6bd406d7e9c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -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; @@ -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; @@ -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; } @@ -102,13 +114,58 @@ public ListenableFuture 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 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()); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 830c481ea452da..c0435551113347 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -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", @@ -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", + ], +) diff --git a/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java new file mode 100644 index 00000000000000..3cf339217551b4 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/DiskCacheIntegrationTest.java @@ -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 getSpawnModules() { + return ImmutableList.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(); + } +} diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java index 67109b1b4e1a17..8854449b38a598 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ActionCacheServer.java @@ -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; @@ -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)); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java index 322cd4b6b0af3f..54a6dd94d28667 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java @@ -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); }