diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index d53bb72a225d3a..12ca6b6f3094c3 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java @@ -29,6 +29,7 @@ import build.bazel.remote.execution.v2.Tree; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -79,6 +80,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -120,20 +122,7 @@ public AbstractRemoteActionCache(RemoteOptions options, DigestUtil digestUtil) { abstract ActionResult getCachedActionResult(ActionKey actionKey) throws IOException, InterruptedException; - /** - * Upload the result of a locally executed action to the remote cache. - * - * @throws IOException if there was an error uploading to the remote cache - * @throws ExecException if uploading any of the action outputs is not supported - */ - abstract void upload( - SimpleBlobStore.ActionKey actionKey, - Action action, - Command command, - Path execRoot, - Collection files, - FileOutErr outErr) - throws ExecException, IOException, InterruptedException; + protected abstract void setCachedActionResult(ActionKey actionKey, ActionResult action) throws IOException, InterruptedException; /** * Uploads a file @@ -157,6 +146,114 @@ abstract void upload( */ protected abstract ListenableFuture uploadBlob(Digest digest, ByteString data); + protected abstract ImmutableSet getMissingDigests(Iterable digests) throws IOException, InterruptedException; + + /** + * Upload the result of a locally executed action to the remote cache. + * + * @throws IOException if there was an error uploading to the remote cache + * @throws ExecException if uploading any of the action outputs is not supported + */ + public ActionResult upload( + ActionKey actionKey, + Action action, + Command command, + Path execRoot, + Collection outputs, + FileOutErr outErr, + int exitCode) + throws ExecException, IOException, InterruptedException { + ActionResult.Builder resultBuilder = ActionResult.newBuilder(); + uploadOutputs(execRoot, actionKey, action, command, outputs, outErr, resultBuilder); + resultBuilder.setExitCode(exitCode); + ActionResult result = resultBuilder.build(); + if (exitCode == 0 && !action.getDoNotCache()) { + setCachedActionResult(actionKey, result); + } + return result; + } + + public ActionResult upload( + ActionKey actionKey, + Action action, + Command command, + Path execRoot, + Collection outputs, + FileOutErr outErr) throws ExecException, IOException, InterruptedException { + return upload(actionKey, action, command, execRoot, outputs, outErr, /* exitCode= */ 0); + } + + private void uploadOutputs( + Path execRoot, + ActionKey actionKey, + Action action, + Command command, + Collection files, + FileOutErr outErr, + ActionResult.Builder result) + throws ExecException, IOException, InterruptedException { + UploadManifest manifest = + new UploadManifest( + digestUtil, + result, + execRoot, + options.incompatibleRemoteSymlinks, + options.allowSymlinkUpload); + manifest.addFiles(files); + manifest.setStdoutStderr(outErr); + manifest.addAction(actionKey, action, command); + + Map digestToFile = manifest.getDigestToFile(); + Map digestToBlobs = manifest.getDigestToBlobs(); + Collection digests = new ArrayList<>(); + digests.addAll(digestToFile.keySet()); + digests.addAll(digestToBlobs.keySet()); + + ImmutableSet digestsToUpload = getMissingDigests(digests); + ImmutableList.Builder> uploads = ImmutableList.builder(); + for (Digest digest : digestsToUpload) { + Path file = digestToFile.get(digest); + if (file != null) { + uploads.add(uploadFile(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(uploadBlob(digest, blob)); + } + } + + waitForUploads(uploads.build()); + + if (manifest.getStderrDigest() != null) { + result.setStderrDigest(manifest.getStderrDigest()); + } + if (manifest.getStdoutDigest() != null) { + result.setStdoutDigest(manifest.getStdoutDigest()); + } + } + + private static void waitForUploads(List> uploads) + throws IOException, InterruptedException { + try { + for (ListenableFuture upload : uploads) { + upload.get(); + } + } catch (ExecutionException e) { + // TODO(buchgr): Add support for cancellation and factor this method out to be shared + // between ByteStreamUploader as well. + Throwable cause = e.getCause(); + Throwables.throwIfInstanceOf(cause, IOException.class); + Throwables.throwIfInstanceOf(cause, InterruptedException.class); + if (cause != null) { + throw new IOException(cause); + } + throw new IOException(e); + } + } + /** * Downloads a blob with a content hash {@code digest} to {@code out}. * diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java index e6c5c6328a2404..c8b2afaa9e1705 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java @@ -166,17 +166,7 @@ public static boolean isRemoteCacheOptions(RemoteOptions options) { || Ascii.toLowerCase(options.remoteCache).startsWith("https://")); } - private ListenableFuture getMissingDigests( - FindMissingBlobsRequest request) throws IOException, InterruptedException { - Context ctx = Context.current(); - try { - return retrier.executeAsync(() -> ctx.call(() -> casFutureStub().findMissingBlobs(request))); - } catch (StatusRuntimeException e) { - throw new IOException(e); - } - } - - private ImmutableSet getMissingDigests(Iterable digests) + protected ImmutableSet getMissingDigests(Iterable digests) throws IOException, InterruptedException { if (Iterables.isEmpty(digests)) { return ImmutableSet.of(); @@ -208,6 +198,16 @@ private ImmutableSet getMissingDigests(Iterable digests) return result.build(); } + private ListenableFuture getMissingDigests( + FindMissingBlobsRequest request) throws IOException, InterruptedException { + Context ctx = Context.current(); + try { + return retrier.executeAsync(() -> ctx.call(() -> casFutureStub().findMissingBlobs(request))); + } catch (StatusRuntimeException e) { + throw new IOException(e); + } + } + /** * Ensures that the tree structure of the inputs, the input files themselves, and the command are * available in the remote cache, such that the tree can be reassembled and executed on another @@ -376,32 +376,6 @@ public void onCompleted() { return future; } - @Override - public void upload( - ActionKey actionKey, - Action action, - Command command, - Path execRoot, - Collection files, - FileOutErr outErr) - throws ExecException, IOException, InterruptedException { - ActionResult.Builder result = ActionResult.newBuilder(); - upload(execRoot, actionKey, action, command, files, outErr, result); - try { - retrier.execute( - () -> - acBlockingStub() - .updateActionResult( - UpdateActionResultRequest.newBuilder() - .setInstanceName(options.remoteInstanceName) - .setActionDigest(actionKey.getDigest()) - .setActionResult(result) - .build())); - } catch (StatusRuntimeException e) { - throw new IOException(e); - } - } - @Override protected ListenableFuture uploadFile(Digest digest, Path path) { return uploader.uploadBlobAsync( @@ -411,82 +385,9 @@ protected ListenableFuture uploadFile(Digest digest, Path path) { } @Override - protected ListenableFuture uploadBlob(Digest digest, ByteString data) { - return uploader.uploadBlobAsync( - HashCode.fromString(digest.getHash()), - Chunker.builder().setInput(data.toByteArray()).build(), - /* forceUpload= */ true); - } - - void upload( - Path execRoot, - ActionKey actionKey, - Action action, - Command command, - Collection files, - FileOutErr outErr, - ActionResult.Builder result) - throws ExecException, IOException, InterruptedException { - UploadManifest manifest = - new UploadManifest( - digestUtil, - result, - execRoot, - options.incompatibleRemoteSymlinks, - options.allowSymlinkUpload); - manifest.addFiles(files); - manifest.setStdoutStderr(outErr); - manifest.addAction(actionKey, action, command); - - Map digestToFile = manifest.getDigestToFile(); - Map digestToBlobs = manifest.getDigestToBlobs(); - Collection digests = new ArrayList<>(); - digests.addAll(digestToFile.keySet()); - digests.addAll(digestToBlobs.keySet()); - - ImmutableSet digestsToUpload = getMissingDigests(digests); - ImmutableList.Builder> uploads = ImmutableList.builder(); - for (Digest digest : digestsToUpload) { - Path file = digestToFile.get(digest); - if (file != null) { - uploads.add(uploadFile(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(uploadBlob(digest, blob)); - } - } - - waitForUploads(uploads.build()); - - if (manifest.getStderrDigest() != null) { - result.setStderrDigest(manifest.getStderrDigest()); - } - if (manifest.getStdoutDigest() != null) { - result.setStdoutDigest(manifest.getStdoutDigest()); - } - } - - private static void waitForUploads(List> uploads) - throws IOException, InterruptedException { - try { - for (ListenableFuture upload : uploads) { - upload.get(); - } - } catch (ExecutionException e) { - // TODO(buchgr): Add support for cancellation and factor this method out to be shared - // between ByteStreamUploader as well. - Throwable cause = e.getCause(); - Throwables.throwIfInstanceOf(cause, IOException.class); - Throwables.throwIfInstanceOf(cause, InterruptedException.class); - if (cause != null) { - throw new IOException(cause); - } - throw new IOException(e); - } + protected ListenableFuture uploadBlob(Digest digest, ByteString data) { + return uploader.uploadBlobAsync(HashCode.fromString(digest.getHash()), + Chunker.builder().setInput(data.toByteArray()).build(), /* forceUpload= */ true); } // Execution Cache API @@ -511,4 +412,21 @@ public ActionResult getCachedActionResult(ActionKey actionKey) throw new IOException(e); } } + + @Override + protected void setCachedActionResult(ActionKey actionKey, ActionResult result) throws IOException, InterruptedException { + try { + retrier.execute( + () -> + acBlockingStub() + .updateActionResult( + UpdateActionResultRequest.newBuilder() + .setInstanceName(options.remoteInstanceName) + .setActionDigest(actionKey.getDigest()) + .setActionResult(result) + .build())); + } catch (StatusRuntimeException e) { + throw new IOException(e); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java index fba3ba0faa1427..8822c0d07c4547 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java @@ -14,35 +14,30 @@ package com.google.devtools.build.lib.remote; -import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionResult; -import build.bazel.remote.execution.v2.Command; import build.bazel.remote.execution.v2.Digest; 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.collect.ImmutableSet; import com.google.common.hash.HashingOutputStream; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; -import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.remote.common.SimpleBlobStore; import com.google.devtools.build.lib.remote.common.SimpleBlobStore.ActionKey; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.Utils; -import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Path; import com.google.protobuf.ByteString; import com.google.protobuf.InvalidProtocolBufferException; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; -import java.util.Collection; -import java.util.Map; import javax.annotation.Nullable; /** @@ -76,60 +71,6 @@ public void downloadTree(Digest rootDigest, Path rootLocation) } } - @Override - public void upload( - SimpleBlobStore.ActionKey actionKey, - Action action, - Command command, - Path execRoot, - Collection files, - FileOutErr outErr) - throws ExecException, IOException, InterruptedException { - ActionResult.Builder result = ActionResult.newBuilder(); - upload(result, actionKey, action, command, execRoot, files, /* uploadAction= */ true); - if (outErr.getErrorPath().exists()) { - Digest stdErrDigest = digestUtil.compute(outErr.getErrorPath()); - getFromFuture(uploadFile(stdErrDigest, outErr.getErrorPath())); - result.setStderrDigest(stdErrDigest); - } - if (outErr.getOutputPath().exists()) { - Digest stdoutDigest = digestUtil.compute(outErr.getOutputPath()); - getFromFuture(uploadFile(stdoutDigest, outErr.getOutputPath())); - result.setStdoutDigest(stdoutDigest); - } - blobStore.putActionResult(actionKey, result.build()); - } - - public void upload( - ActionResult.Builder result, - SimpleBlobStore.ActionKey actionKey, - Action action, - Command command, - Path execRoot, - Collection files, - boolean uploadAction) - throws ExecException, IOException, InterruptedException { - UploadManifest manifest = - new UploadManifest( - digestUtil, - result, - execRoot, - options.incompatibleRemoteSymlinks, - options.allowSymlinkUpload); - manifest.addFiles(files); - if (uploadAction) { - manifest.addAction(actionKey, action, command); - } - - for (Map.Entry entry : manifest.getDigestToFile().entrySet()) { - getFromFuture(uploadFile(entry.getKey(), entry.getValue())); - } - - for (Map.Entry entry : manifest.getDigestToBlobs().entrySet()) { - getFromFuture(uploadBlob(entry.getKey(), entry.getValue())); - } - } - @Override public ListenableFuture uploadFile(Digest digest, Path file) { return blobStore.uploadFile(digest, file); @@ -140,6 +81,11 @@ public ListenableFuture uploadBlob(Digest digest, ByteString data) { return blobStore.uploadBlob(digest, data); } + @Override + protected ImmutableSet getMissingDigests(Iterable digests) { + return ImmutableSet.copyOf(digests); + } + public boolean containsKey(Digest digest) throws IOException, InterruptedException { return blobStore.contains(digest.getHash()); } diff --git a/src/main/java/com/google/devtools/build/lib/util/ExitCode.java b/src/main/java/com/google/devtools/build/lib/util/ExitCode.java index 34b7bbbf83abe3..a951055c741d31 100644 --- a/src/main/java/com/google/devtools/build/lib/util/ExitCode.java +++ b/src/main/java/com/google/devtools/build/lib/util/ExitCode.java @@ -1,4 +1,4 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. + // Copyright 2014 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. diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java index d8e6959be0041b..e60c2de4acad87 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java @@ -38,6 +38,7 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -1150,20 +1151,12 @@ protected T getFromFuture(ListenableFuture f) throws IOException, Interru @Nullable @Override - ActionResult getCachedActionResult(ActionKey actionKey) - throws IOException, InterruptedException { + ActionResult getCachedActionResult(ActionKey actionKey) { throw new UnsupportedOperationException(); } @Override - void upload( - ActionKey actionKey, - Action action, - Command command, - Path execRoot, - Collection files, - FileOutErr outErr) - throws ExecException, IOException, InterruptedException { + protected void setCachedActionResult(ActionKey actionKey, ActionResult action) { throw new UnsupportedOperationException(); } @@ -1177,6 +1170,11 @@ protected ListenableFuture uploadBlob(Digest digest, ByteString data) { throw new UnsupportedOperationException(); } + @Override + protected ImmutableSet getMissingDigests(Iterable digests) { + return null; + } + @Override protected ListenableFuture downloadBlob(Digest digest, OutputStream out) { SettableFuture result = SettableFuture.create(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java index bfa532095b2d98..02b56b2db222fc 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java @@ -530,6 +530,16 @@ public void findMissingBlobs( responseObserver.onCompleted(); } }); + serviceRegistry.addService( + new ActionCacheImplBase() { + @Override + public void updateActionResult(UpdateActionResultRequest request, + StreamObserver responseObserver) { + responseObserver.onNext(request.getActionResult()); + responseObserver.onCompleted(); + } + } + ); ActionResult result = uploadDirectory(client, ImmutableList.of(fooFile, barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -558,6 +568,16 @@ public void findMissingBlobs( responseObserver.onCompleted(); } }); + serviceRegistry.addService( + new ActionCacheImplBase() { + @Override + public void updateActionResult(UpdateActionResultRequest request, + StreamObserver responseObserver) { + responseObserver.onNext(request.getActionResult()); + responseObserver.onCompleted(); + } + } + ); ActionResult result = uploadDirectory(client, ImmutableList.of(barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -608,6 +628,16 @@ public void findMissingBlobs( responseObserver.onCompleted(); } }); + serviceRegistry.addService( + new ActionCacheImplBase() { + @Override + public void updateActionResult(UpdateActionResultRequest request, + StreamObserver responseObserver) { + responseObserver.onNext(request.getActionResult()); + responseObserver.onCompleted(); + } + } + ); ActionResult result = uploadDirectory(client, ImmutableList.of(barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); @@ -617,12 +647,10 @@ public void findMissingBlobs( private ActionResult uploadDirectory(GrpcRemoteCache client, List outputs) throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); Action action = Action.getDefaultInstance(); ActionKey actionKey = DIGEST_UTIL.computeActionKey(action); Command cmd = Command.getDefaultInstance(); - client.upload(execRoot, actionKey, action, cmd, outputs, outErr, result); - return result.build(); + return client.upload(actionKey, action, cmd, execRoot, outputs, outErr); } @Test @@ -662,16 +690,24 @@ public void findMissingBlobs( responseObserver.onCompleted(); } }); + serviceRegistry.addService( + new ActionCacheImplBase() { + @Override + public void updateActionResult(UpdateActionResultRequest request, + StreamObserver responseObserver) { + responseObserver.onNext(request.getActionResult()); + responseObserver.onCompleted(); + } + } + ); - ActionResult.Builder result = ActionResult.newBuilder(); - client.upload( - execRoot, + ActionResult result = client.upload( DIGEST_UTIL.asActionKey(actionDigest), action, command, - ImmutableList.of(fooFile, barFile), - outErr, - result); + execRoot, + ImmutableList.of(fooFile, barFile), + outErr); ActionResult.Builder expectedResult = ActionResult.newBuilder(); expectedResult.setStdoutDigest(stdoutDigest); expectedResult.setStderrDigest(stderrDigest); @@ -681,7 +717,7 @@ public void findMissingBlobs( .setPath("bar") .setDigest(barDigest) .setIsExecutable(true); - assertThat(result.build()).isEqualTo(expectedResult.build()); + assertThat(result).isEqualTo(expectedResult.build()); } @Test @@ -711,19 +747,27 @@ public void findMissingBlobs( responseObserver.onCompleted(); } }); + serviceRegistry.addService( + new ActionCacheImplBase() { + @Override + public void updateActionResult(UpdateActionResultRequest request, + StreamObserver responseObserver) { + responseObserver.onNext(request.getActionResult()); + responseObserver.onCompleted(); + } + } + ); RemoteOptions options = Options.getDefaults(RemoteOptions.class); options.maxOutboundMessageSize = 80; // Enough for one digest, but not two. final GrpcRemoteCache client = newClient(options); - ActionResult.Builder result = ActionResult.newBuilder(); - client.upload( - execRoot, + ActionResult result = client.upload( DIGEST_UTIL.asActionKey(actionDigest), action, command, - ImmutableList.of(fooFile, barFile), - outErr, - result); + execRoot, + ImmutableList.of(fooFile, barFile), + outErr); ActionResult.Builder expectedResult = ActionResult.newBuilder(); expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); expectedResult @@ -731,7 +775,7 @@ public void findMissingBlobs( .setPath("bar") .setDigest(barDigest) .setIsExecutable(true); - assertThat(result.build()).isEqualTo(expectedResult.build()); + assertThat(result).isEqualTo(expectedResult.build()); assertThat(numGetMissingCalls.get()).isEqualTo(4); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index 24ad516c01c735..3361fee64a30cf 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -22,6 +22,7 @@ import build.bazel.remote.execution.v2.Digest; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -232,13 +233,7 @@ ActionResult getCachedActionResult(ActionKey actionKey) { } @Override - void upload( - ActionKey actionKey, - Action action, - Command command, - Path execRoot, - Collection files, - FileOutErr outErr) { + protected void setCachedActionResult(ActionKey actionKey, ActionResult action) { throw new UnsupportedOperationException(); } @@ -252,6 +247,11 @@ protected ListenableFuture uploadBlob(Digest digest, ByteString data) { throw new UnsupportedOperationException(); } + @Override + protected ImmutableSet getMissingDigests(Iterable digests) { + return null; + } + @Override protected ListenableFuture downloadBlob(Digest digest, OutputStream out) { ByteString data = cacheEntries.get(digest); diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java index 271dc69ce55d74..9e43b1a42e62a1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java @@ -37,6 +37,7 @@ import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -315,22 +316,20 @@ public void testUploadDirectory() throws Exception { final ConcurrentMap map = new ConcurrentHashMap<>(); final SimpleBlobStoreActionCache client = newClient(map); - ActionResult.Builder result = ActionResult.newBuilder(); - client.upload( - result, + ActionResult result = client.upload( DIGEST_UTIL.asActionKey(actionDigest), action, cmd, execRoot, - ImmutableList.of(fooFile, barDir), - /* uploadAction= */ true); + ImmutableList.of(fooFile, barDir), + new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); ActionResult.Builder expectedResult = ActionResult.newBuilder(); expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); + assertThat(result).isEqualTo(expectedResult.build()); assertThat(map.keySet()) - .containsExactly( + .containsAtLeast( fooDigest.getHash(), quxDigest.getHash(), barDigest.getHash(), @@ -402,12 +401,11 @@ public void testUploadDirectoryNested() throws Exception { private ActionResult uploadDirectory(SimpleBlobStoreActionCache client, List outputs) throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); Action action = Action.getDefaultInstance(); ActionKey actionKey = DIGEST_UTIL.computeActionKey(action); Command cmd = Command.getDefaultInstance(); - client.upload(result, actionKey, action, cmd, execRoot, outputs, /* uploadAction= */ true); - return result.build(); + return client.upload(actionKey, action, cmd, execRoot, outputs, + new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); } @Test diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD index 4c6659f57ff32b..e9dc09d3905a64 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD @@ -16,6 +16,7 @@ java_library( visibility = ["//src/tools/remote:__subpackages__"], deps = [ "//src/main/java/com/google/devtools/build/lib:build-base", + "//src/main/java/com/google/devtools/build/lib:io", "//src/main/java/com/google/devtools/build/lib:os_util", "//src/main/java/com/google/devtools/build/lib:packages-internal", "//src/main/java/com/google/devtools/build/lib:process_util", diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index abc3e6f47dfe40..fa21e7a97b8a79 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.shell.CommandException; import com.google.devtools.build.lib.shell.CommandResult; import com.google.devtools.build.lib.shell.FutureCommandResult; +import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.longrunning.Operation; @@ -284,84 +285,80 @@ private ActionResult execute(Digest actionDigest, Path execRoot) long startTime = System.currentTimeMillis(); CommandResult cmdResult = null; - FutureCommandResult futureCmdResult = null; - try { - futureCmdResult = cmd.executeAsync(); - } catch (CommandException e) { - Throwables.throwIfInstanceOf(e.getCause(), IOException.class); - } + String uuid = UUID.randomUUID().toString(); + Path stdout = execRoot.getChild("stdout-" + uuid); + Path stderr = execRoot.getChild("stderr-" + uuid); + try (FileOutErr outErr = new FileOutErr(stdout, stderr)) { - if (futureCmdResult != null) { + FutureCommandResult futureCmdResult = null; try { - cmdResult = futureCmdResult.get(); - } catch (AbnormalTerminationException e) { - cmdResult = e.getResult(); + futureCmdResult = cmd.executeAsync(outErr.getOutputStream(), outErr.getErrorStream()); + } catch (CommandException e) { + Throwables.throwIfInstanceOf(e.getCause(), IOException.class); } - } - long timeoutMillis = - action.hasTimeout() - ? Durations.toMillis(action.getTimeout()) - : TimeUnit.MINUTES.toMillis(15); - boolean wasTimeout = - (cmdResult != null && cmdResult.getTerminationStatus().timedOut()) - || wasTimeout(timeoutMillis, System.currentTimeMillis() - startTime); - final int exitCode; - Status errStatus = null; - ExecuteResponse.Builder resp = ExecuteResponse.newBuilder(); - if (wasTimeout) { - final String errMessage = - String.format( - "Command:\n%s\nexceeded deadline of %f seconds.", - Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0); - logger.warning(errMessage); - errStatus = - Status.newBuilder() - .setCode(Code.DEADLINE_EXCEEDED.getNumber()) - .setMessage(errMessage) - .build(); - exitCode = LOCAL_EXEC_ERROR; - } else if (cmdResult == null) { - exitCode = LOCAL_EXEC_ERROR; - } else { - exitCode = cmdResult.getTerminationStatus().getRawExitCode(); - } + if (futureCmdResult != null) { + try { + cmdResult = futureCmdResult.get(); + } catch (AbnormalTerminationException e) { + cmdResult = e.getResult(); + } + } - ActionResult.Builder result = ActionResult.newBuilder(); - boolean setResult = exitCode == 0 && !action.getDoNotCache(); - try { - cache.upload(result, actionKey, action, command, execRoot, outputs, setResult); - } catch (ExecException e) { - if (errStatus == null) { + long timeoutMillis = + action.hasTimeout() + ? Durations.toMillis(action.getTimeout()) + : TimeUnit.MINUTES.toMillis(15); + boolean wasTimeout = + (cmdResult != null && cmdResult.getTerminationStatus().timedOut()) + || wasTimeout(timeoutMillis, System.currentTimeMillis() - startTime); + final int exitCode; + Status errStatus = null; + ExecuteResponse.Builder resp = ExecuteResponse.newBuilder(); + if (wasTimeout) { + final String errMessage = + String.format( + "Command:\n%s\nexceeded deadline of %f seconds.", + Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0); + logger.warning(errMessage); errStatus = Status.newBuilder() - .setCode(Code.FAILED_PRECONDITION.getNumber()) - .setMessage(e.getMessage()) + .setCode(Code.DEADLINE_EXCEEDED.getNumber()) + .setMessage(errMessage) .build(); + exitCode = LOCAL_EXEC_ERROR; + } else if (cmdResult == null) { + exitCode = LOCAL_EXEC_ERROR; + } else { + exitCode = cmdResult.getTerminationStatus().getRawExitCode(); + } + + ActionResult result = null; + try { + result = cache.upload(actionKey, action, command, execRoot, outputs, outErr, exitCode); + } catch (ExecException e) { + if (errStatus == null) { + errStatus = + Status.newBuilder() + .setCode(Code.FAILED_PRECONDITION.getNumber()) + .setMessage(e.getMessage()) + .build(); + } } - } - byte[] stdout = cmdResult.getStdout(); - if (stdout.length > 0) { - Digest stdoutDigest = digestUtil.compute(stdout); - getFromFuture(cache.uploadBlob(stdoutDigest, ByteString.copyFrom(stdout))); - result.setStdoutDigest(stdoutDigest); - } - byte[] stderr = cmdResult.getStderr(); - if (stderr.length > 0) { - Digest stderrDigest = digestUtil.compute(stderr); - getFromFuture(cache.uploadBlob(stderrDigest, ByteString.copyFrom(stderr))); - result.setStderrDigest(stderrDigest); - } - ActionResult finalResult = result.setExitCode(exitCode).build(); - resp.setResult(finalResult); - if (errStatus != null) { - resp.setStatus(errStatus); - throw new ExecutionStatusException(errStatus, resp.build()); - } else if (setResult) { - cache.setCachedActionResult(actionKey, finalResult); + if (result == null) { + result = ActionResult.newBuilder().setExitCode(exitCode).build(); + } + + resp.setResult(result); + + if (errStatus != null) { + resp.setStatus(errStatus); + throw new ExecutionStatusException(errStatus, resp.build()); + } + + return result; } - return finalResult; } // Returns true if the OS being run on is Windows (or some close approximation thereof).