From de2e3a03f086fd0e85606034b7328b1e053f69e3 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 13 Jul 2021 07:18:46 -0700 Subject: [PATCH] Remote: Do not upload empty output to remote cache. An unintended side-effect of change cc2b3ecb9283ebdb297fc3fb6407f4697c850ac2 is that zero-sized blob won't be uploaded to gRPC cache. However, the behavior is existed for a long time and the REAPI spec is also updated accordingly. This change makes the behavior explicit and brings it to other remote cache backends. Context https://github.com/bazelbuild/bazel/issues/11063. Fixes https://github.com/bazelbuild/bazel/issues/13349. Closes #13594. PiperOrigin-RevId: 384457129 --- .../build/lib/remote/RemoteCache.java | 60 +++++++++++++++-- .../build/lib/remote/RemoteCacheTests.java | 66 +++++++++++++++++++ .../remote/worker/OnDiskBlobStoreCache.java | 12 ---- 3 files changed, 119 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index d978ad6250dea9..f4db0a604454bd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -139,6 +139,38 @@ public ActionResult downloadActionResult( return getFromFuture(cacheProtocol.downloadActionResult(context, actionKey, inlineOutErr)); } + /** + * Upload a local file to the remote cache. + * + * @param context the context for the action. + * @param digest the digest of the file. + * @param file the file to upload. + */ + public final ListenableFuture uploadFile( + RemoteActionExecutionContext context, Digest digest, Path file) { + if (digest.getSizeBytes() == 0) { + return COMPLETED_SUCCESS; + } + + return cacheProtocol.uploadFile(context, digest, file); + } + + /** + * Upload sequence of bytes to the remote cache. + * + * @param context the context for the action. + * @param digest the digest of the file. + * @param data the BLOB to upload. + */ + public final ListenableFuture uploadBlob( + RemoteActionExecutionContext context, Digest digest, ByteString data) { + if (digest.getSizeBytes() == 0) { + return COMPLETED_SUCCESS; + } + + return cacheProtocol.uploadBlob(context, digest, data); + } + /** * Upload the result of a locally executed action to the remote cache. * @@ -219,14 +251,14 @@ private void uploadOutputs( for (Digest digest : digestsToUpload) { Path file = digestToFile.get(digest); if (file != null) { - uploads.add(cacheProtocol.uploadFile(context, digest, file)); + uploads.add(uploadFile(context, digest, file)); } else { ByteString blob = digestToBlobs.get(digest); if (blob == null) { String message = "FindMissingBlobs call returned an unknown digest: " + digest; throw new IOException(message); } - uploads.add(cacheProtocol.uploadBlob(context, digest, blob)); + uploads.add(uploadBlob(context, digest, blob)); } } @@ -317,6 +349,15 @@ public void onFailure(Throwable t) { return outerF; } + private ListenableFuture downloadBlob( + RemoteActionExecutionContext context, Digest digest, OutputStream out) { + if (digest.getSizeBytes() == 0) { + return COMPLETED_SUCCESS; + } + + return cacheProtocol.downloadBlob(context, digest, out); + } + private static Path toTmpDownloadPath(Path actualPath) { return actualPath.getParentDirectory().getRelative(actualPath.getBaseName() + ".tmp"); } @@ -662,7 +703,14 @@ public void onFailure(Throwable t) { return outerF; } - private List> downloadOutErr( + /** + * Download the stdout and stderr of an executed action. + * + * @param context the context for the action. + * @param result the result of the action. + * @param outErr the {@link OutErr} that the stdout and stderr will be downloaded to. + */ + public final List> downloadOutErr( RemoteActionExecutionContext context, ActionResult result, OutErr outErr) { List> downloads = new ArrayList<>(); if (!result.getStdoutRaw().isEmpty()) { @@ -675,8 +723,7 @@ private List> downloadOutErr( } else if (result.hasStdoutDigest()) { downloads.add( Futures.transform( - cacheProtocol.downloadBlob( - context, result.getStdoutDigest(), outErr.getOutputStream()), + downloadBlob(context, result.getStdoutDigest(), outErr.getOutputStream()), (d) -> null, directExecutor())); } @@ -690,8 +737,7 @@ private List> downloadOutErr( } else if (result.hasStderrDigest()) { downloads.add( Futures.transform( - cacheProtocol.downloadBlob( - context, result.getStderrDigest(), outErr.getErrorStream()), + downloadBlob(context, result.getStderrDigest(), outErr.getErrorStream()), (d) -> null, directExecutor())); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index 6f84b4e68abc2e..4b457251d3c89b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -609,6 +609,7 @@ public void uploadSymlinkInDirectoryNoAllowError() throws Exception { assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload"); } + // TODO(chiwang): Cleanup the tests, e.g. use java test data builder pattern. @Test public void downloadRelativeFileSymlink() throws Exception { RemoteCache cache = newRemoteCache(); @@ -1363,6 +1364,27 @@ public void testDownloadEmptyBlobAndFile() throws Exception { assertThat(file.getFileSize()).isEqualTo(0); } + @Test + public void downloadOutErr_empty_doNotPerformDownload() throws Exception { + // Test that downloading empty stdout/stderr does not try to perform a download. + + InMemoryRemoteCache remoteCache = newRemoteCache(); + Digest emptyDigest = digestUtil.compute(new byte[0]); + ActionResult.Builder result = ActionResult.newBuilder(); + result.setStdoutDigest(emptyDigest); + result.setStderrDigest(emptyDigest); + + RemoteCache.waitForBulkTransfer( + remoteCache.downloadOutErr( + context, + result.build(), + new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))), + true); + + assertThat(remoteCache.getNumSuccessfulDownloads()).isEqualTo(0); + assertThat(remoteCache.getNumFailedDownloads()).isEqualTo(0); + } + @Test public void testDownloadFileWithSymlinkTemplate() throws Exception { // Test that when a symlink template is provided, we don't actually download files to disk. @@ -1626,6 +1648,50 @@ public void testUploadDirectory() throws Exception { assertThat(remoteCache.findMissingDigests(context, toQuery)).isEmpty(); } + @Test + public void upload_emptyBlobAndFile_doNotPerformUpload() throws Exception { + // Test that uploading an empty BLOB/file does not try to perform an upload. + InMemoryRemoteCache remoteCache = newRemoteCache(); + Digest emptyDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("file"), ""); + Path file = execRoot.getRelative("file"); + + Utils.getFromFuture(remoteCache.uploadBlob(context, emptyDigest, ByteString.EMPTY)); + assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + .containsExactly(emptyDigest); + + Utils.getFromFuture(remoteCache.uploadFile(context, emptyDigest, file)); + assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + .containsExactly(emptyDigest); + } + + @Test + public void upload_emptyOutputs_doNotPerformUpload() throws Exception { + // Test that uploading an empty output does not try to perform an upload. + + // arrange + Digest emptyDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/test/wobble"), ""); + Path file = execRoot.getRelative("bar/test/wobble"); + InMemoryRemoteCache remoteCache = newRemoteCache(); + Action action = Action.getDefaultInstance(); + ActionKey actionDigest = digestUtil.computeActionKey(action); + Command cmd = Command.getDefaultInstance(); + + // act + remoteCache.upload( + context, + remotePathResolver, + actionDigest, + action, + cmd, + ImmutableList.of(file), + new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); + + // assert + assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + .containsExactly(emptyDigest); + } + @Test public void testUploadEmptyDirectory() throws Exception { // Test that uploading an empty directory works. 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 1f39c313dbf60b..d8d423b8ffa19e 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 @@ -18,7 +18,6 @@ import build.bazel.remote.execution.v2.Directory; import build.bazel.remote.execution.v2.DirectoryNode; import build.bazel.remote.execution.v2.FileNode; -import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.remote.RemoteCache; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; @@ -27,7 +26,6 @@ import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.vfs.Path; -import com.google.protobuf.ByteString; import java.io.IOException; /** A {@link RemoteCache} backed by an {@link DiskCacheClient}. */ @@ -61,16 +59,6 @@ public void downloadTree( } } - public ListenableFuture uploadFile( - RemoteActionExecutionContext context, Digest digest, Path file) { - return cacheProtocol.uploadFile(context, digest, file); - } - - public ListenableFuture uploadBlob( - RemoteActionExecutionContext context, Digest digest, ByteString data) { - return cacheProtocol.uploadBlob(context, digest, data); - } - public void uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) throws IOException, InterruptedException {