Skip to content

Commit

Permalink
Remote: Do not upload empty output to remote cache.
Browse files Browse the repository at this point in the history 
An unintended side-effect of change cc2b3ec 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 bazelbuild#11063.

Fixes bazelbuild#13349.

Closes bazelbuild#13594.

PiperOrigin-RevId: 384457129
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Jul 13, 2021
1 parent b27fd22 commit 18c8216
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 19 deletions.
60 changes: 53 additions & 7 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Void> 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<Void> 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.
*
Expand Down Expand Up @@ -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));
}
}

Expand Down Expand Up @@ -317,6 +349,15 @@ public void onFailure(Throwable t) {
return outerF;
}

private ListenableFuture<Void> 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");
}
Expand Down Expand Up @@ -662,7 +703,14 @@ public void onFailure(Throwable t) {
return outerF;
}

private List<ListenableFuture<FileMetadata>> 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<ListenableFuture<FileMetadata>> downloadOutErr(
RemoteActionExecutionContext context, ActionResult result, OutErr outErr) {
List<ListenableFuture<FileMetadata>> downloads = new ArrayList<>();
if (!result.getStdoutRaw().isEmpty()) {
Expand All @@ -675,8 +723,7 @@ private List<ListenableFuture<FileMetadata>> 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()));
}
Expand All @@ -690,8 +737,7 @@ private List<ListenableFuture<FileMetadata>> 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()));
}
Expand Down
66 changes: 66 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,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();
Expand Down Expand Up @@ -1364,6 +1365,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.
Expand Down Expand Up @@ -1627,6 +1649,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.
Expand Down
12 changes: 0 additions & 12 deletions ...ls/remote/src/main/java/com/google/devtools/build/remote/worker/OnDiskBlobStoreCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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}. */
Expand Down Expand Up @@ -61,16 +59,6 @@ public void downloadTree(
}
}

public ListenableFuture<Void> uploadFile(
RemoteActionExecutionContext context, Digest digest, Path file) {
return cacheProtocol.uploadFile(context, digest, file);
}

public ListenableFuture<Void> 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 {
Expand Down

0 comments on commit 18c8216

Please sign in to comment.