diff --git a/src/main/java/com/google/devtools/build/lib/actions/RemoteUploadEvent.java b/src/main/java/com/google/devtools/build/lib/actions/RemoteUploadEvent.java new file mode 100644 index 00000000000000..6c40ee7c85d377 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/RemoteUploadEvent.java @@ -0,0 +1,35 @@ +// Copyright 2021 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.actions; + +import com.google.auto.value.AutoValue; +import com.google.devtools.build.lib.events.ExtendedEventHandler.ProgressLike; + +/** Notifications for the uploads to the remote server. */ +@AutoValue +public abstract class RemoteUploadEvent implements ProgressLike { + + public static RemoteUploadEvent create(String resourceId, String progress, boolean finished) { + return new AutoValue_RemoteUploadEvent(resourceId, progress, finished); + } + + /** The id that uniquely determines the resource being uploaded among all events within an build. */ + public abstract String resourceId(); + + /** Human readable description of the upload progress. */ + public abstract String progress(); + + /** Whether the upload progress reported about is finished already. */ + public abstract boolean finished(); +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index a1d74b604ec26b..c5e62c90ae7085 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -264,25 +264,23 @@ public ListenableFuture downloadActionResult( } @Override - public void uploadActionResult( - RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) - throws IOException, InterruptedException { - try { - Utils.refreshIfUnauthenticated( - () -> - retrier.execute( - () -> - acBlockingStub(context) - .updateActionResult( - UpdateActionResultRequest.newBuilder() - .setInstanceName(options.remoteInstanceName) - .setActionDigest(actionKey.getDigest()) - .setActionResult(actionResult) - .build())), - callCredentialsProvider); - } catch (StatusRuntimeException e) { - throw new IOException(e); - } + public ListenableFuture uploadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { + ListenableFuture upload = + Utils.refreshIfUnauthenticatedAsync( + () -> + retrier.executeAsync( + () -> + acFutureStub(context) + .updateActionResult( + UpdateActionResultRequest.newBuilder() + .setInstanceName(options.remoteInstanceName) + .setActionDigest(actionKey.getDigest()) + .setActionResult(actionResult) + .build())), + callCredentialsProvider); + + return Futures.transform(upload, ac -> null, MoreExecutors.directExecutor()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index a0e34f2656fd52..07bd58f92b59c5 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -18,11 +18,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ListeningScheduledExecutorService; -import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.exec.ExecutionOptions; -import com.google.devtools.build.lib.exec.ExecutorLifecycleListener; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.exec.SpawnCache; import com.google.devtools.build.lib.exec.SpawnStrategyRegistry; @@ -35,11 +32,10 @@ import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.vfs.Path; -import java.util.function.Supplier; import javax.annotation.Nullable; /** Provides a remote execution context. */ -final class RemoteActionContextProvider implements ExecutorLifecycleListener { +final class RemoteActionContextProvider { private final CommandEnvironment env; @Nullable private final RemoteCache cache; @@ -120,8 +116,12 @@ private RemoteExecutionService getRemoteExecutionService() { workingDirectory.getRelative(remoteOptions.remoteCaptureCorruptedOutputs); } + boolean verboseFailures = + checkNotNull(env.getOptions().getOptions(ExecutionOptions.class)).verboseFailures; remoteExecutionService = new RemoteExecutionService( + env.getReporter(), + verboseFailures, env.getExecRoot(), createRemotePathResolver(), env.getBuildRequestId(), @@ -132,6 +132,7 @@ private RemoteExecutionService getRemoteExecutionService() { executor, filesToDownload, captureCorruptedOutputsDir); + env.getEventBus().register(remoteExecutionService); } return remoteExecutionService; @@ -189,20 +190,9 @@ void setFilesToDownload(ImmutableSet topLevelOutputs) { this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload"); } - @Override - public void executorCreated() {} - - @Override - public void executionPhaseStarting( - ActionGraph actionGraph, Supplier> topLevelArtifacts) {} - - @Override - public void executionPhaseEnding() { - if (cache != null) { - cache.close(); - } - if (executor != null) { - executor.close(); + public void afterCommand() { + if (remoteExecutionService != null) { + remoteExecutionService.shutdown(); } } } 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 a6c5330c55db90..639d35960f0ebc 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 @@ -13,30 +13,28 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.base.Throwables.throwIfInstanceOf; import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.remote.common.ProgressStatusListener.NO_ACTION; import static com.google.devtools.build.lib.remote.util.Utils.bytesCountToDisplayString; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; -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.Tree; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.flogger.GoogleLogger; 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.actions.UserExecException; +import com.google.devtools.build.lib.actions.RemoteUploadEvent; import com.google.devtools.build.lib.concurrent.ThreadSafety; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.SpawnProgressEvent; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.remote.common.LazyFileOutputStream; @@ -45,38 +43,46 @@ import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; -import com.google.devtools.build.lib.remote.common.RemotePathResolver; import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.util.AsyncTaskCache; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.RxFutures; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code; -import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.util.io.OutErr; -import com.google.devtools.build.lib.vfs.Dirent; -import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.Symlinks; import com.google.protobuf.ByteString; +import io.netty.util.AbstractReferenceCounted; +import io.netty.util.ReferenceCounted; +import io.reactivex.rxjava3.core.Completable; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; -import java.util.Collection; -import java.util.Comparator; -import java.util.HashMap; import java.util.List; -import java.util.Map; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Matcher; import java.util.regex.Pattern; -import javax.annotation.Nullable; - -/** A cache for storing artifacts (input and output) as well as the output of running an action. */ +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; + +/** + * A cache for storing artifacts (input and output) as well as the output of running an action. + * + *

The cache is reference counted. Initially, the reference count is 1. Use {@link #retain()} to + * increase and {@link #release()} to decrease the reference count respectively. Once the reference + * count is reached to 0, the underlying resources will be released (after network I/Os finished). + * + *

Use {@link #awaitTermination()} to wait for the underlying network I/Os to finish. Use {@link + * #shutdownNow()} to cancel all active network I/Os and reject new requests. + */ @ThreadSafety.ThreadSafe -public class RemoteCache implements AutoCloseable { +public class RemoteCache extends AbstractReferenceCounted { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); /** See {@link SpawnExecutionContext#lockOutputFiles()}. */ @@ -88,154 +94,177 @@ interface OutputFilesLocker { private static final ListenableFuture COMPLETED_SUCCESS = immediateFuture(null); private static final ListenableFuture EMPTY_BYTES = immediateFuture(new byte[0]); + private final Reporter reporter; + private final CountDownLatch closedLatch = new CountDownLatch(1); + private final AtomicBoolean closed = new AtomicBoolean(false); + private final AsyncTaskCache.NoResult casUploadCache = AsyncTaskCache.NoResult.create(); + protected final RemoteCacheClient cacheProtocol; protected final RemoteOptions options; protected final DigestUtil digestUtil; public RemoteCache( - RemoteCacheClient cacheProtocol, RemoteOptions options, DigestUtil digestUtil) { + Reporter reporter, + RemoteCacheClient cacheProtocol, + RemoteOptions options, + DigestUtil digestUtil) { + this.reporter = reporter; this.cacheProtocol = cacheProtocol; this.options = options; this.digestUtil = digestUtil; } + /** Downloads an action result for the {@code actionKey} from remote cache. */ public ActionResult downloadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, boolean inlineOutErr) throws IOException, InterruptedException { return getFromFuture(cacheProtocol.downloadActionResult(context, actionKey, inlineOutErr)); } + /** + * Returns a set of digests that the remote cache does not know about. The returned set is + * guaranteed to be a subset of {@code digests}. + * + *

Digests whose content are being uploaded via {@link #uploadBlob} or {@link #uploadFile} are + * considered "missing" hence will be returned by this call. Since the upload calls can + * deduplicate uploads, this can be combined for the case where different clients want to wait for + * the same blob to be uploaded. + */ + public ListenableFuture> findMissingDigests( + RemoteActionExecutionContext context, Iterable digests) { + checkState(!closed.get(), "closed"); + + Set digestsInProgress = casUploadCache.getInProgressTasks(); + // Since this is a separate call, we may have digest that is in the digestsInProgress above and + // then finishes resulting a duplicate digest in digestsUploaded. However, this race is fine for + // our purpose as long as we check "in progress" before "uploaded". + Set digestsUploaded = casUploadCache.getFinishedTasks(); + + Set digestsRequested = + StreamSupport.stream(digests.spliterator(), false).collect(Collectors.toSet()); + + // Find digests that are neither in the progress of upload nor already uploaded. + Set digestsToUpload = + digestsRequested.stream() + .filter( + digest -> !digestsInProgress.contains(digest) && !digestsUploaded.contains(digest)) + .collect(Collectors.toSet()); + + ListenableFuture> missingDigestsFuture; + if (Iterables.isEmpty(digestsToUpload)) { + missingDigestsFuture = immediateFuture(ImmutableSet.of()); + } else { + missingDigestsFuture = cacheProtocol.findMissingDigests(context, digestsToUpload); + } + + // Combine digests that are being uploaded and that are missing from cache. + return Futures.transform( + missingDigestsFuture, + missingDigests -> { + ImmutableSet.Builder builder = ImmutableSet.builder(); + for (Digest digest : digestsInProgress) { + if (digestsRequested.contains(digest)) { + builder.add(digest); + } + } + builder.addAll(missingDigests); + return builder.build(); + }, + directExecutor()); + } + + /** Upload the action result to the remote cache. */ + public ListenableFuture uploadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { + checkState(!closed.get(), "closed"); + + String resourceId = "ac/" + actionKey.getDigest().getHash(); + String progress = "Uploading action result " + actionKey.getDigest().getHash(); + + Completable completable = + RxFutures.toCompletable( + () -> cacheProtocol.uploadActionResult(context, actionKey, actionResult), + directExecutor()) + .doOnSubscribe( + disposable -> + reporter.post( + RemoteUploadEvent.create(resourceId, progress, /* finished= */ false))) + .doFinally( + () -> + reporter.post( + RemoteUploadEvent.create(resourceId, progress, /* finished= */ true))); + + return RxFutures.toListenableFuture(completable); + } + /** * Upload a local file to the remote cache. * + *

Trying to upload the same file multiple times concurrently, results in only one upload being + * performed. + * * @param context the context for the action. * @param digest the digest of the file. * @param file the file to upload. */ - public final ListenableFuture uploadFile( + public ListenableFuture uploadFile( RemoteActionExecutionContext context, Digest digest, Path file) { if (digest.getSizeBytes() == 0) { return COMPLETED_SUCCESS; } - return cacheProtocol.uploadFile(context, digest, file); + String resourceId = "cas/" + digest.getHash(); + String progress = "Uploading file " + file.getPathString(); + + Completable upload = + casUploadCache.executeIfNot( + digest, + RxFutures.toCompletable( + () -> cacheProtocol.uploadFile(context, digest, file), directExecutor()) + .doOnSubscribe( + disposable -> + reporter.post( + RemoteUploadEvent.create(resourceId, progress, /* finished= */ false))) + .doFinally( + () -> + reporter.post( + RemoteUploadEvent.create(resourceId, progress, /* finished= */ true)))); + return RxFutures.toListenableFuture(upload); } /** * Upload sequence of bytes to the remote cache. * + *

Trying to upload the same BLOB multiple times concurrently, results in only one upload being + * performed. + * * @param context the context for the action. * @param digest the digest of the file. * @param data the BLOB to upload. */ - public final ListenableFuture uploadBlob( + public 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. - * - * @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( - RemoteActionExecutionContext context, - RemotePathResolver remotePathResolver, - ActionKey actionKey, - Action action, - Command command, - Collection outputs, - FileOutErr outErr, - int exitCode) - throws ExecException, IOException, InterruptedException { - ActionResult.Builder resultBuilder = ActionResult.newBuilder(); - uploadOutputs( - context, remotePathResolver, actionKey, action, command, outputs, outErr, resultBuilder); - resultBuilder.setExitCode(exitCode); - ActionResult result = resultBuilder.build(); - if (exitCode == 0) { - cacheProtocol.uploadActionResult(context, actionKey, result); - } - return result; - } - - public ActionResult upload( - RemoteActionExecutionContext context, - RemotePathResolver remotePathResolver, - ActionKey actionKey, - Action action, - Command command, - Collection outputs, - FileOutErr outErr) - throws ExecException, IOException, InterruptedException { - return upload( - context, - remotePathResolver, - actionKey, - action, - command, - outputs, - outErr, - /* exitCode= */ 0); - } - - private void uploadOutputs( - RemoteActionExecutionContext context, - RemotePathResolver remotePathResolver, - ActionKey actionKey, - Action action, - Command command, - Collection files, - FileOutErr outErr, - ActionResult.Builder result) - throws ExecException, IOException, InterruptedException { - UploadManifest manifest = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - 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 = - getFromFuture(cacheProtocol.findMissingDigests(context, digests)); - ImmutableList.Builder> uploads = ImmutableList.builder(); - for (Digest digest : digestsToUpload) { - Path file = digestToFile.get(digest); - if (file != null) { - 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(uploadBlob(context, digest, blob)); - } - } - - waitForBulkTransfer(uploads.build(), /* cancelRemainingOnInterrupt=*/ false); - - if (manifest.getStderrDigest() != null) { - result.setStderrDigest(manifest.getStderrDigest()); - } - if (manifest.getStdoutDigest() != null) { - result.setStdoutDigest(manifest.getStdoutDigest()); - } + String resourceId = "cas/" + digest.getHash(); + String progress = "Uploading blob " + digest.getHash(); + + Completable upload = + casUploadCache.executeIfNot( + digest, + RxFutures.toCompletable( + () -> cacheProtocol.uploadBlob(context, digest, data), directExecutor()) + .doOnSubscribe( + disposable -> + reporter.post( + RemoteUploadEvent.create(resourceId, progress, /* finished= */ false))) + .doFinally( + () -> + reporter.post( + RemoteUploadEvent.create(resourceId, progress, /* finished= */ true)))); + return RxFutures.toListenableFuture(upload); } public static void waitForBulkTransfer( @@ -510,245 +539,59 @@ public final List> downloadOutErr( return downloads; } - /** UploadManifest adds output metadata to a {@link ActionResult}. */ - static class UploadManifest { - private final DigestUtil digestUtil; - private final RemotePathResolver remotePathResolver; - private final ActionResult.Builder result; - private final boolean allowSymlinks; - private final boolean uploadSymlinks; - private final Map digestToFile = new HashMap<>(); - private final Map digestToBlobs = new HashMap<>(); - private Digest stderrDigest; - private Digest stdoutDigest; - - /** - * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult - * builder is populated through a call to {@link #addFile(Digest, Path)}. - */ - public UploadManifest( - DigestUtil digestUtil, - RemotePathResolver remotePathResolver, - ActionResult.Builder result, - boolean uploadSymlinks, - boolean allowSymlinks) { - this.digestUtil = digestUtil; - this.remotePathResolver = remotePathResolver; - this.result = result; - this.uploadSymlinks = uploadSymlinks; - this.allowSymlinks = allowSymlinks; - } - - public void setStdoutStderr(FileOutErr outErr) throws IOException { - if (outErr.getErrorPath().exists()) { - stderrDigest = digestUtil.compute(outErr.getErrorPath()); - digestToFile.put(stderrDigest, outErr.getErrorPath()); - } - if (outErr.getOutputPath().exists()) { - stdoutDigest = digestUtil.compute(outErr.getOutputPath()); - digestToFile.put(stdoutDigest, outErr.getOutputPath()); - } - } - - /** - * Add a collection of files or directories to the UploadManifest. Adding a directory has the - * effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the - * directory, including the descendants, can be reconstructed and 2) uploading all the - * non-directory descendant files. - */ - public void addFiles(Collection files) throws ExecException, IOException { - for (Path file : files) { - // TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and - // rely on the local spawn runner to stat the files, instead of statting here. - FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW); - // TODO(#6547): handle the case where the parent directory of the output file is an - // output symlink. - if (stat == null) { - // We ignore requested results that have not been generated by the action. - continue; - } - if (stat.isDirectory()) { - addDirectory(file); - } else if (stat.isFile() && !stat.isSpecialFile()) { - Digest digest = digestUtil.compute(file, stat.getSize()); - addFile(digest, file); - } else if (stat.isSymbolicLink() && allowSymlinks) { - PathFragment target = file.readSymbolicLink(); - // Need to resolve the symbolic link to know what to add, file or directory. - FileStatus statFollow = file.statIfFound(Symlinks.FOLLOW); - if (statFollow == null) { - throw new IOException( - String.format("Action output %s is a dangling symbolic link to %s ", file, target)); - } - if (statFollow.isSpecialFile()) { - illegalOutput(file); - } - Preconditions.checkState( - statFollow.isFile() || statFollow.isDirectory(), "Unknown stat type for %s", file); - if (uploadSymlinks && !target.isAbsolute()) { - if (statFollow.isFile()) { - addFileSymbolicLink(file, target); - } else { - addDirectorySymbolicLink(file, target); - } - } else { - if (statFollow.isFile()) { - addFile(digestUtil.compute(file), file); - } else { - addDirectory(file); - } - } - } else { - illegalOutput(file); - } - } - } - - /** - * Adds an action and command protos to upload. They need to be uploaded as part of the action - * result. - */ - public void addAction(RemoteCacheClient.ActionKey actionKey, Action action, Command command) { - digestToBlobs.put(actionKey.getDigest(), action.toByteString()); - digestToBlobs.put(action.getCommandDigest(), command.toByteString()); - } - - /** Map of digests to file paths to upload. */ - public Map getDigestToFile() { - return digestToFile; - } - - /** - * Map of digests to chunkers to upload. When the file is a regular, non-directory file it is - * transmitted through {@link #getDigestToFile()}. When it is a directory, it is transmitted as - * a {@link Tree} protobuf message through {@link #getDigestToBlobs()}. - */ - public Map getDigestToBlobs() { - return digestToBlobs; - } - - @Nullable - public Digest getStdoutDigest() { - return stdoutDigest; - } - - @Nullable - public Digest getStderrDigest() { - return stderrDigest; - } - - private void addFileSymbolicLink(Path file, PathFragment target) throws IOException { - result - .addOutputFileSymlinksBuilder() - .setPath(remotePathResolver.localPathToOutputPath(file)) - .setTarget(target.toString()); - } - - private void addDirectorySymbolicLink(Path file, PathFragment target) throws IOException { - result - .addOutputDirectorySymlinksBuilder() - .setPath(remotePathResolver.localPathToOutputPath(file)) - .setTarget(target.toString()); - } - - private void addFile(Digest digest, Path file) throws IOException { - result - .addOutputFilesBuilder() - .setPath(remotePathResolver.localPathToOutputPath(file)) - .setDigest(digest) - .setIsExecutable(file.isExecutable()); + @Override + public RemoteCache retain() { + super.retain(); + return this; + } - digestToFile.put(digest, file); - } + @Override + public ReferenceCounted touch(Object hint) { + return this; + } - private void addDirectory(Path dir) throws ExecException, IOException { - Tree.Builder tree = Tree.newBuilder(); - Directory root = computeDirectory(dir, tree); - tree.setRoot(root); + /** Release resources associated with the cache. The cache may not be used after calling this. */ + @Override + protected void deallocate() { + checkState(!closed.get(), "closed"); + checkState( + casUploadCache.getInProgressTasks().isEmpty(), "There are still in progress uploads."); - ByteString data = tree.build().toByteString(); - Digest digest = digestUtil.compute(data.toByteArray()); + closed.set(true); - if (result != null) { - result - .addOutputDirectoriesBuilder() - .setPath(remotePathResolver.localPathToOutputPath(dir)) - .setTreeDigest(digest); - } + casUploadCache.shutdown(); - digestToBlobs.put(digest, data); - } + cacheProtocol.close(); - private Directory computeDirectory(Path path, Tree.Builder tree) - throws ExecException, IOException { - Directory.Builder b = Directory.newBuilder(); - - List sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW)); - sortedDirent.sort(Comparator.comparing(Dirent::getName)); - - for (Dirent dirent : sortedDirent) { - String name = dirent.getName(); - Path child = path.getRelative(name); - if (dirent.getType() == Dirent.Type.DIRECTORY) { - Directory dir = computeDirectory(child, tree); - b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); - tree.addChildren(dir); - } else if (dirent.getType() == Dirent.Type.SYMLINK && allowSymlinks) { - PathFragment target = child.readSymbolicLink(); - if (uploadSymlinks && !target.isAbsolute()) { - // Whether it is dangling or not, we're passing it on. - b.addSymlinksBuilder().setName(name).setTarget(target.toString()); - continue; - } - // Need to resolve the symbolic link now to know whether to upload a file or a directory. - FileStatus statFollow = child.statIfFound(Symlinks.FOLLOW); - if (statFollow == null) { - throw new IOException( - String.format( - "Action output %s is a dangling symbolic link to %s ", child, target)); - } - if (statFollow.isFile() && !statFollow.isSpecialFile()) { - Digest digest = digestUtil.compute(child); - b.addFilesBuilder() - .setName(name) - .setDigest(digest) - .setIsExecutable(child.isExecutable()); - digestToFile.put(digest, child); - } else if (statFollow.isDirectory()) { - Directory dir = computeDirectory(child, tree); - b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); - tree.addChildren(dir); - } else { - illegalOutput(child); - } - } else if (dirent.getType() == Dirent.Type.FILE) { - Digest digest = digestUtil.compute(child); - b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); - digestToFile.put(digest, child); - } else { - illegalOutput(child); - } - } + closedLatch.countDown(); + } - return b.build(); - } + public boolean isClosed() { + return closed.get(); + } - private void illegalOutput(Path what) throws ExecException { - String kind = what.isSymbolicLink() ? "symbolic link" : "special file"; - String message = - String.format( - "Output %s is a %s. Only regular files and directories may be " - + "uploaded to a remote cache. " - + "Change the file type or use --remote_allow_symlink_upload.", - remotePathResolver.localPathToOutputPath(what), kind); - throw new UserExecException(createFailureDetail(message, Code.ILLEGAL_OUTPUT)); + /** Cancels all active network I/Os and rejects new requests. */ + public void shutdownNow() { + casUploadCache.shutdownNow(); + try { + closedLatch.await(); + } catch (InterruptedException e) { + throw new RuntimeException("Failed to close remote cache", e); } } - /** Release resources associated with the cache. The cache may not be used after calling this. */ - @Override - public void close() { - cacheProtocol.close(); + /** + * Waits the cache to terminate. Only returns if a) the internal reference count is reached to 0 + * and b) All network I/Os are finished. + */ + public void awaitTermination() throws InterruptedException { + try { + casUploadCache.awaitTermination().blockingAwait(); + } catch (RuntimeException e) { + Throwable cause = e.getCause(); + throwIfInstanceOf(cause, InterruptedException.class); + throw e; + } } public static FailureDetail createFailureDetail(String message, Code detailedCode) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java index c0db565594bd55..4dde498da5dce1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java @@ -22,6 +22,7 @@ import com.google.common.collect.Iterables; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; @@ -38,8 +39,8 @@ public class RemoteExecutionCache extends RemoteCache { public RemoteExecutionCache( - RemoteCacheClient protocolImpl, RemoteOptions options, DigestUtil digestUtil) { - super(protocolImpl, options, digestUtil); + Reporter reporter, RemoteCacheClient protocolImpl, RemoteOptions options, DigestUtil digestUtil) { + super(reporter, protocolImpl, options, digestUtil); } /** @@ -62,7 +63,7 @@ public void ensureInputsPresent( Iterable allDigests = Iterables.concat(merkleTree.getAllDigests(), additionalInputs.keySet()); ImmutableSet missingDigests = - getFromFuture(cacheProtocol.findMissingDigests(context, allDigests)); + getFromFuture(findMissingDigests(context, allDigests)); List> uploadFutures = new ArrayList<>(); for (Digest missingDigest : missingDigests) { @@ -79,20 +80,20 @@ private ListenableFuture uploadBlob( Map additionalInputs) { Directory node = merkleTree.getDirectoryByDigest(digest); if (node != null) { - return cacheProtocol.uploadBlob(context, digest, node.toByteString()); + return uploadBlob(context, digest, node.toByteString()); } PathOrBytes file = merkleTree.getFileByDigest(digest); if (file != null) { if (file.getBytes() != null) { - return cacheProtocol.uploadBlob(context, digest, file.getBytes()); + return uploadBlob(context, digest, file.getBytes()); } - return cacheProtocol.uploadFile(context, digest, file.getPath()); + return uploadFile(context, digest, file.getPath()); } Message message = additionalInputs.get(digest); if (message != null) { - return cacheProtocol.uploadBlob(context, digest, message.toByteString()); + return uploadBlob(context, digest, message.toByteString()); } return Futures.immediateFailedFuture( diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 8d3adf6614c32d..c08751ff50077d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -21,10 +21,10 @@ import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.Futures.transform; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; -import static com.google.devtools.build.lib.remote.RemoteCache.createFailureDetail; import static com.google.devtools.build.lib.remote.RemoteCache.waitForBulkTransfer; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath; +import static com.google.devtools.build.lib.remote.util.Utils.grpcAwareErrorMessage; import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload; import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromCombinedCache; import static com.google.devtools.build.lib.remote.util.Utils.shouldAcceptCachedResultFromDiskCache; @@ -52,13 +52,16 @@ import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.SymlinkNode; import build.bazel.remote.execution.v2.Tree; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +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.collect.Iterables; import com.google.common.collect.Maps; +import com.google.common.eventbus.Subscribe; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionInput; @@ -70,10 +73,14 @@ import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.Spawns; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.analysis.platform.PlatformUtils; +import com.google.devtools.build.lib.buildtool.buildevent.BuildInterruptedEvent; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; @@ -91,9 +98,11 @@ import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.RxFutures; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.io.FileOutErr; @@ -104,6 +113,11 @@ import com.google.protobuf.ExtensionRegistry; import com.google.protobuf.Message; import io.grpc.Status.Code; +import io.reactivex.rxjava3.annotations.NonNull; +import io.reactivex.rxjava3.core.Completable; +import io.reactivex.rxjava3.core.CompletableObserver; +import io.reactivex.rxjava3.disposables.Disposable; +import io.reactivex.rxjava3.schedulers.Schedulers; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; @@ -116,6 +130,9 @@ import java.util.Objects; import java.util.SortedMap; import java.util.TreeSet; +import java.util.concurrent.CancellationException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; /** @@ -123,6 +140,9 @@ * cache and execution with spawn specific types. */ public class RemoteExecutionService { + private final AtomicBoolean shutdown = new AtomicBoolean(false); + private final Reporter reporter; + private final boolean verboseFailures; private final Path execRoot; private final RemotePathResolver remotePathResolver; private final String buildRequestId; @@ -134,7 +154,11 @@ public class RemoteExecutionService { private final ImmutableSet filesToDownload; @Nullable private final Path captureCorruptedOutputsDir; + private boolean remoteCacheInterrupted = false; + public RemoteExecutionService( + Reporter reporter, + boolean verboseFailures, Path execRoot, RemotePathResolver remotePathResolver, String buildRequestId, @@ -145,6 +169,8 @@ public RemoteExecutionService( @Nullable RemoteExecutionClient remoteExecutor, ImmutableSet filesToDownload, @Nullable Path captureCorruptedOutputsDir) { + this.reporter = reporter; + this.verboseFailures = verboseFailures; this.execRoot = execRoot; this.remotePathResolver = remotePathResolver; this.buildRequestId = buildRequestId; @@ -483,6 +509,7 @@ public int hashCode() { @Nullable public RemoteActionResult lookupCache(RemoteAction action) throws IOException, InterruptedException { + checkState(!shutdown.get(), "shutdown"); checkState(shouldAcceptCachedResult(action.spawn), "spawn doesn't accept cached result"); ActionResult actionResult = @@ -869,6 +896,7 @@ ActionResultMetadata parseActionResultMetadata(RemoteAction action, RemoteAction @Nullable public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult result) throws InterruptedException, IOException, ExecException { + checkState(!shutdown.get(), "shutdown"); checkNotNull(remoteCache, "remoteCache can't be null"); ActionResultMetadata metadata; @@ -987,23 +1015,120 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } - /** Upload outputs of a remote action which was executed locally to remote cache. */ - public void uploadOutputs(RemoteAction action) - throws InterruptedException, IOException, ExecException { - checkState(shouldUploadLocalResults(action.spawn), "spawn shouldn't upload local result"); + private static FailureDetail createFailureDetail(String message, RemoteExecution.Code detailedCode) { + return FailureDetail.newBuilder() + .setMessage(message) + .setRemoteExecution(RemoteExecution.newBuilder().setCode(detailedCode)) + .build(); + } + + private Completable uploadActionResult( + RemoteCache remoteCache, RemoteAction action, ActionResult result) { + return RxFutures.toCompletable( + () -> + remoteCache.uploadActionResult( + action.remoteActionExecutionContext, action.actionKey, result), + directExecutor()); + } + @VisibleForTesting + UploadManifest buildUploadManifest(RemoteAction action, SpawnResult spawnResult) throws ExecException, IOException { Collection outputFiles = action.spawn.getOutputFiles().stream() .map((inp) -> execRoot.getRelative(inp.getExecPath())) .collect(ImmutableList.toImmutableList()); - remoteCache.upload( - action.remoteActionExecutionContext, + return UploadManifest.create( + remoteOptions, + digestUtil, remotePathResolver, action.actionKey, action.action, action.command, outputFiles, - action.spawnExecutionContext.getFileOutErr()); + action.spawnExecutionContext.getFileOutErr(), + spawnResult.exitCode()); + } + + /** Upload outputs of a remote action which was executed locally to remote cache. */ + public void uploadOutputs(RemoteAction action, SpawnResult spawnResult) + throws IOException, ExecException, InterruptedException { + checkState(!shutdown.get(), "shutdown"); + checkState(shouldUploadLocalResults(action.spawn), "spawn shouldn't upload local result"); + checkState( + SpawnResult.Status.SUCCESS.equals(spawnResult.status()) && spawnResult.exitCode() == 0, + "shouldn't upload outputs of failed local action"); + + UploadManifest manifest = buildUploadManifest(action, spawnResult); + + CountDownLatch uploadStartedLatch = new CountDownLatch(1); + + Completable uploads = Completable.using( + remoteCache::retain, + remoteCache -> { + uploadStartedLatch.countDown(); + + return Completable.concatArray( + manifest.uploadOutputs(action.remoteActionExecutionContext, remoteCache), + uploadActionResult(remoteCache, action, manifest.getActionResult())); + }, + RemoteCache::release); + + if (remoteOptions.remoteCacheAsync) { + uploads.subscribeOn(Schedulers.io()).subscribe(reportUploadErrorObserver); + } else { + try { + uploads.blockingAwait(); + } catch (RuntimeException e) { + Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class); + Throwables.throwIfInstanceOf(e.getCause(), IOException.class); + Throwables.throwIfInstanceOf(e.getCause(), ExecException.class); + throw e; + } + } + + uploadStartedLatch.await(); + } + + private final CompletableObserver reportUploadErrorObserver = new CompletableObserver() { + @Override + public void onSubscribe(@NonNull Disposable d) { + + } + + @Override + public void onComplete() { + } + + @Override + public void onError(@NonNull Throwable e) { + reportUploadError(e); + } + }; + + private void reportUploadError(Throwable error) { + if (remoteCacheInterrupted) { + // If we interrupt manually, ignore cancellation errors. + if (error instanceof CancellationException) { + return; + } + } + + String errorMessage; + if (!verboseFailures) { + if (error instanceof IOException) { + errorMessage = grpcAwareErrorMessage((IOException) error); + } else { + errorMessage = error.getMessage(); + } + } else { + // On --verbose_failures print the whole stack trace + errorMessage = Throwables.getStackTraceAsString(error); + } + if (isNullOrEmpty(errorMessage)) { + errorMessage = error.getClass().getSimpleName(); + } + errorMessage = "Writing to Remote Cache:\n" + errorMessage; + reporter.handle(Event.warn(errorMessage)); } /** @@ -1013,6 +1138,7 @@ public void uploadOutputs(RemoteAction action) */ public void uploadInputsIfNotPresent(RemoteAction action) throws IOException, InterruptedException { + checkState(!shutdown.get(), "shutdown"); checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely"); RemoteExecutionCache remoteExecutionCache = (RemoteExecutionCache) remoteCache; @@ -1033,6 +1159,7 @@ public void uploadInputsIfNotPresent(RemoteAction action) public RemoteActionResult executeRemotely( RemoteAction action, boolean acceptCachedResult, OperationObserver observer) throws IOException, InterruptedException { + checkState(!shutdown.get(), "shutdown"); checkState(mayBeExecutedRemotely(action.spawn), "spawn can't be executed remotely"); ExecuteRequest.Builder requestBuilder = @@ -1067,7 +1194,9 @@ public static class ServerLogs { /** Downloads server logs from a remotely executed action if any. */ public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse resp, Path logDir) throws InterruptedException, IOException { + checkState(!shutdown.get(), "shutdown"); checkNotNull(remoteCache, "remoteCache can't be null"); + ServerLogs serverLogs = new ServerLogs(); serverLogs.directory = logDir.getRelative(action.getActionId()); @@ -1089,4 +1218,40 @@ public ServerLogs maybeDownloadServerLogs(RemoteAction action, ExecuteResponse r return serverLogs; } + + @Subscribe + public void buildInterrupted(BuildInterruptedEvent event) { + remoteCacheInterrupted = true; + } + + /** + * Shuts the service down. Wait for active network I/O to finish but new requests are rejected. + */ + public void shutdown() { + if (!shutdown.compareAndSet(false, true)) { + return; + } + + if (remoteCache != null) { + remoteCache.release(); + + if (remoteCacheInterrupted) { + Thread.currentThread().interrupt(); + } + + try { + remoteCache.awaitTermination(); + } catch (InterruptedException e) { + remoteCacheInterrupted = true; + reporter.handle(Event.warn("remote cache interrupted")); + remoteCache.shutdownNow(); + } + + checkState(remoteCache.isClosed(), "remote cache is not closed properly."); + } + + if (remoteExecutor != null) { + remoteExecutor.close(); + } + } } 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 bc0394b32296d8..f6ecbca1d3c09b 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 @@ -220,7 +220,7 @@ private void initHttpAndDiskCache( handleInitFailure(env, e, Code.CACHE_INIT_FAILURE); return; } - RemoteCache remoteCache = new RemoteCache(cacheClient, remoteOptions, digestUtil); + RemoteCache remoteCache = new RemoteCache(env.getReporter(), cacheClient, remoteOptions, digestUtil); actionContextProvider = RemoteActionContextProvider.createForRemoteCaching( env, remoteCache, /* retryScheduler= */ null, digestUtil); @@ -527,7 +527,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { } execChannel.release(); RemoteExecutionCache remoteCache = - new RemoteExecutionCache(cacheClient, remoteOptions, digestUtil); + new RemoteExecutionCache(env.getReporter(), cacheClient, remoteOptions, digestUtil); actionContextProvider = RemoteActionContextProvider.createForRemoteExecution( env, remoteCache, remoteExecutor, retryScheduler, digestUtil, logDir); @@ -557,7 +557,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { } } - RemoteCache remoteCache = new RemoteCache(cacheClient, remoteOptions, digestUtil); + RemoteCache remoteCache = new RemoteCache(env.getReporter(), cacheClient, remoteOptions, digestUtil); actionContextProvider = RemoteActionContextProvider.createForRemoteCaching( env, remoteCache, retryScheduler, digestUtil); @@ -742,6 +742,10 @@ public void afterCommand() throws AbruptExitException { Code failureCode = null; String failureMessage = null; + if (actionContextProvider != null) { + actionContextProvider.afterCommand(); + } + try { closeRpcLogFile(); } catch (IOException e) { @@ -805,7 +809,6 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB if (actionContextProvider == null) { return; } - builder.addExecutorLifecycleListener(actionContextProvider); RemoteOptions remoteOptions = Preconditions.checkNotNull( env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index da862250068cec..4690438391c330 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -198,7 +198,7 @@ public void store(SpawnResult result) throws ExecException, InterruptedException } try (SilentCloseable c = prof.profile(ProfilerTask.UPLOAD_TIME, "upload outputs")) { - remoteExecutionService.uploadOutputs(action); + remoteExecutionService.uploadOutputs(action, result); } catch (IOException e) { String errorMessage; if (!verboseFailures) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index 653559fbd4c917..7e43afb82e522d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -123,6 +123,11 @@ public class RemoteSpawnRunner implements SpawnRunner { this.remoteExecutionService = remoteExecutionService; } + @VisibleForTesting + RemoteExecutionService getRemoteExecutionService() { + return remoteExecutionService; + } + @Override public String getName() { return "remote"; @@ -562,7 +567,7 @@ SpawnResult execLocallyAndUpload( } try (SilentCloseable c = Profiler.instance().profile(UPLOAD_TIME, "upload outputs")) { - remoteExecutionService.uploadOutputs(action); + remoteExecutionService.uploadOutputs(action, result); } catch (IOException e) { if (verboseFailures) { report(Event.debug("Upload to remote cache failed: " + e.getMessage())); diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java new file mode 100644 index 00000000000000..833146f9eb7b89 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -0,0 +1,356 @@ +package com.google.devtools.build.lib.remote; + +import static com.google.common.base.Throwables.throwIfInstanceOf; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; + +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.Tree; +import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.UserExecException; +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.RemotePathResolver; +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.RxFutures; +import com.google.devtools.build.lib.server.FailureDetails; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.Dirent; +import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.protobuf.ByteString; +import io.reactivex.rxjava3.core.Completable; +import io.reactivex.rxjava3.core.Flowable; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Comparator; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; + +/** UploadManifest adds output metadata to a {@link ActionResult}. */ +public class UploadManifest { + private final DigestUtil digestUtil; + private final RemotePathResolver remotePathResolver; + private final ActionResult.Builder result; + private final boolean allowSymlinks; + private final boolean uploadSymlinks; + private final Map digestToFile = new HashMap<>(); + private final Map digestToBlobs = new HashMap<>(); + private Digest stderrDigest; + private Digest stdoutDigest; + + public static UploadManifest create( + RemoteOptions remoteOptions, + DigestUtil digestUtil, + RemotePathResolver remotePathResolver, + ActionKey actionKey, + Action action, + Command command, + Collection outputFiles, + FileOutErr outErr, + int exitCode) + throws ExecException, IOException { + ActionResult.Builder result = ActionResult.newBuilder(); + result.setExitCode(exitCode); + + UploadManifest manifest = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + remoteOptions.incompatibleRemoteSymlinks, + remoteOptions.allowSymlinkUpload); + manifest.addFiles(outputFiles); + manifest.setStdoutStderr(outErr); + manifest.addAction(actionKey, action, command); + if (manifest.getStderrDigest() != null) { + result.setStderrDigest(manifest.getStderrDigest()); + } + if (manifest.getStdoutDigest() != null) { + result.setStdoutDigest(manifest.getStdoutDigest()); + } + + return manifest; + } + + /** + * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult + * builder is populated through a call to {@link #addFile(Digest, Path)}. + */ + public UploadManifest( + DigestUtil digestUtil, + RemotePathResolver remotePathResolver, + ActionResult.Builder result, + boolean uploadSymlinks, + boolean allowSymlinks) { + this.digestUtil = digestUtil; + this.remotePathResolver = remotePathResolver; + this.result = result; + this.uploadSymlinks = uploadSymlinks; + this.allowSymlinks = allowSymlinks; + } + + public void setStdoutStderr(FileOutErr outErr) throws IOException { + if (outErr.getErrorPath().exists()) { + stderrDigest = digestUtil.compute(outErr.getErrorPath()); + digestToFile.put(stderrDigest, outErr.getErrorPath()); + } + if (outErr.getOutputPath().exists()) { + stdoutDigest = digestUtil.compute(outErr.getOutputPath()); + digestToFile.put(stdoutDigest, outErr.getOutputPath()); + } + } + + /** + * Add a collection of files or directories to the UploadManifest. Adding a directory has the + * effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the + * directory, including the descendants, can be reconstructed and 2) uploading all the + * non-directory descendant files. + */ + public void addFiles(Collection files) throws ExecException, IOException { + for (Path file : files) { + // TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and + // rely on the local spawn runner to stat the files, instead of statting here. + FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW); + // TODO(#6547): handle the case where the parent directory of the output file is an + // output symlink. + if (stat == null) { + // We ignore requested results that have not been generated by the action. + continue; + } + if (stat.isDirectory()) { + addDirectory(file); + } else if (stat.isFile() && !stat.isSpecialFile()) { + Digest digest = digestUtil.compute(file, stat.getSize()); + addFile(digest, file); + } else if (stat.isSymbolicLink() && allowSymlinks) { + PathFragment target = file.readSymbolicLink(); + // Need to resolve the symbolic link to know what to add, file or directory. + FileStatus statFollow = file.statIfFound(Symlinks.FOLLOW); + if (statFollow == null) { + throw new IOException( + String.format("Action output %s is a dangling symbolic link to %s ", file, target)); + } + if (statFollow.isSpecialFile()) { + illegalOutput(file); + } + Preconditions.checkState( + statFollow.isFile() || statFollow.isDirectory(), "Unknown stat type for %s", file); + if (uploadSymlinks && !target.isAbsolute()) { + if (statFollow.isFile()) { + addFileSymbolicLink(file, target); + } else { + addDirectorySymbolicLink(file, target); + } + } else { + if (statFollow.isFile()) { + addFile(digestUtil.compute(file), file); + } else { + addDirectory(file); + } + } + } else { + illegalOutput(file); + } + } + } + + /** + * Adds an action and command protos to upload. They need to be uploaded as part of the action + * result. + */ + public void addAction(ActionKey actionKey, Action action, Command command) { + digestToBlobs.put(actionKey.getDigest(), action.toByteString()); + digestToBlobs.put(action.getCommandDigest(), command.toByteString()); + } + + /** Map of digests to file paths to upload. */ + public Map getDigestToFile() { + return digestToFile; + } + + /** + * Map of digests to chunkers to upload. When the file is a regular, non-directory file it is + * transmitted through {@link #getDigestToFile()}. When it is a directory, it is transmitted as a + * {@link Tree} protobuf message through {@link #getDigestToBlobs()}. + */ + public Map getDigestToBlobs() { + return digestToBlobs; + } + + @Nullable + public Digest getStdoutDigest() { + return stdoutDigest; + } + + @Nullable + public Digest getStderrDigest() { + return stderrDigest; + } + + private void addFileSymbolicLink(Path file, PathFragment target) throws IOException { + result + .addOutputFileSymlinksBuilder() + .setPath(remotePathResolver.localPathToOutputPath(file)) + .setTarget(target.toString()); + } + + private void addDirectorySymbolicLink(Path file, PathFragment target) throws IOException { + result + .addOutputDirectorySymlinksBuilder() + .setPath(remotePathResolver.localPathToOutputPath(file)) + .setTarget(target.toString()); + } + + private void addFile(Digest digest, Path file) throws IOException { + result + .addOutputFilesBuilder() + .setPath(remotePathResolver.localPathToOutputPath(file)) + .setDigest(digest) + .setIsExecutable(file.isExecutable()); + + digestToFile.put(digest, file); + } + + private void addDirectory(Path dir) throws ExecException, IOException { + Tree.Builder tree = Tree.newBuilder(); + Directory root = computeDirectory(dir, tree); + tree.setRoot(root); + + ByteString data = tree.build().toByteString(); + Digest digest = digestUtil.compute(data.toByteArray()); + + if (result != null) { + result + .addOutputDirectoriesBuilder() + .setPath(remotePathResolver.localPathToOutputPath(dir)) + .setTreeDigest(digest); + } + + digestToBlobs.put(digest, data); + } + + private Directory computeDirectory(Path path, Tree.Builder tree) + throws ExecException, IOException { + Directory.Builder b = Directory.newBuilder(); + + List sortedDirent = new ArrayList<>(path.readdir(Symlinks.NOFOLLOW)); + sortedDirent.sort(Comparator.comparing(Dirent::getName)); + + for (Dirent dirent : sortedDirent) { + String name = dirent.getName(); + Path child = path.getRelative(name); + if (dirent.getType() == Dirent.Type.DIRECTORY) { + Directory dir = computeDirectory(child, tree); + b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); + tree.addChildren(dir); + } else if (dirent.getType() == Dirent.Type.SYMLINK && allowSymlinks) { + PathFragment target = child.readSymbolicLink(); + if (uploadSymlinks && !target.isAbsolute()) { + // Whether it is dangling or not, we're passing it on. + b.addSymlinksBuilder().setName(name).setTarget(target.toString()); + continue; + } + // Need to resolve the symbolic link now to know whether to upload a file or a directory. + FileStatus statFollow = child.statIfFound(Symlinks.FOLLOW); + if (statFollow == null) { + throw new IOException( + String.format("Action output %s is a dangling symbolic link to %s ", child, target)); + } + if (statFollow.isFile() && !statFollow.isSpecialFile()) { + Digest digest = digestUtil.compute(child); + b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); + digestToFile.put(digest, child); + } else if (statFollow.isDirectory()) { + Directory dir = computeDirectory(child, tree); + b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); + tree.addChildren(dir); + } else { + illegalOutput(child); + } + } else if (dirent.getType() == Dirent.Type.FILE) { + Digest digest = digestUtil.compute(child); + b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); + digestToFile.put(digest, child); + } else { + illegalOutput(child); + } + } + + return b.build(); + } + + private void illegalOutput(Path what) throws ExecException { + String kind = what.isSymbolicLink() ? "symbolic link" : "special file"; + String message = + String.format( + "Output %s is a %s. Only regular files and directories may be " + + "uploaded to a remote cache. " + + "Change the file type or use --remote_allow_symlink_upload.", + remotePathResolver.localPathToOutputPath(what), kind); + + FailureDetail failureDetail = + FailureDetail.newBuilder() + .setMessage(message) + .setRemoteExecution( + FailureDetails.RemoteExecution.newBuilder() + .setCode(FailureDetails.RemoteExecution.Code.ILLEGAL_OUTPUT)) + .build(); + throw new UserExecException(failureDetail); + } + + public ActionResult getActionResult() { + return result.build(); + } + + public Completable uploadOutputs(RemoteActionExecutionContext context, RemoteCache remoteCache) { + Map digestToFile = getDigestToFile(); + Map digestToBlobs = getDigestToBlobs(); + Collection digests = new ArrayList<>(); + digests.addAll(digestToFile.keySet()); + digests.addAll(digestToBlobs.keySet()); + return RxFutures.toSingle( + () -> remoteCache.findMissingDigests(context, digests), directExecutor()) + .flatMapPublisher(Flowable::fromIterable) + .flatMapCompletable( + digest -> { + Path file = digestToFile.get(digest); + + if (file != null) { + return RxFutures.toCompletable( + () -> remoteCache.uploadFile(context, digest, file), directExecutor()); + } else { + ByteString blob = digestToBlobs.get(digest); + if (blob == null) { + String message = "FindMissingBlobs call returned an unknown digest: " + digest; + return Completable.error(new IOException(message)); + } + return RxFutures.toCompletable( + () -> remoteCache.uploadBlob(context, digest, blob), directExecutor()); + } + }); + } + + public void awaitUploadOutputs(RemoteActionExecutionContext context, RemoteCache remoteCache) + throws InterruptedException, IOException, ExecException { + try { + uploadOutputs(context, remoteCache).blockingAwait(); + } catch (RuntimeException e) { + throwIfInstanceOf(e.getCause(), InterruptedException.class); + throwIfInstanceOf(e.getCause(), IOException.class); + throwIfInstanceOf(e.getCause(), ExecException.class); + throw e; + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java index 628d214bec973b..e3fc6aace98ea8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java @@ -21,7 +21,6 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.vfs.Path; import com.google.protobuf.ByteString; -import java.io.IOException; import java.io.OutputStream; /** @@ -82,12 +81,10 @@ ListenableFuture downloadActionResult( * @param context the context for the action. * @param actionKey The digest of the {@link Action} that generated the action result. * @param actionResult The action result to associate with the {@code actionKey}. - * @throws IOException If there is an error uploading the action result. - * @throws InterruptedException In case the thread + * @return A Future representing pending completion of the upload. */ - void uploadActionResult( - RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) - throws IOException, InterruptedException; + ListenableFuture uploadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult); /** * Downloads a BLOB for the given {@code digest} and writes it to {@code out}. diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java index b2b042be96a804..a0edf243d97e1b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskAndRemoteCacheClient.java @@ -53,13 +53,21 @@ public DiskAndRemoteCacheClient( } @Override - public void uploadActionResult( - RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) - throws IOException, InterruptedException { - diskCache.uploadActionResult(context, actionKey, actionResult); - if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { - remoteCache.uploadActionResult(context, actionKey, actionResult); - } + public ListenableFuture uploadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { + ListenableFuture diskUploadFuture = + diskCache.uploadActionResult(context, actionKey, actionResult); + + return Futures.transformAsync( + diskUploadFuture, + v -> { + if (shouldUploadLocalResultsToRemoteCache(options, context.getSpawn())) { + return remoteCache.uploadActionResult(context, actionKey, actionResult); + } else { + return Futures.immediateFuture(null); + } + }, + MoreExecutors.directExecutor()); } @Override 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 26ce20ad35867f..e4d472f831b072 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 @@ -109,11 +109,15 @@ public ListenableFuture downloadActionResult( } @Override - public void uploadActionResult( - RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) - throws IOException { - try (InputStream data = actionResult.toByteString().newInput()) { - saveFile(actionKey.getDigest().getHash(), data, /* actionResult= */ true); + public ListenableFuture uploadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { + try { + try (InputStream data = actionResult.toByteString().newInput()) { + saveFile(actionKey.getDigest().getHash(), data, /* actionResult= */ true); + } + return Futures.immediateFuture(null); + } catch (IOException e) { + return Futures.immediateFailedFuture(e); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java index 1efecd3bb160ae..2a9375a7180ce2 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/http/HttpCacheClient.java @@ -16,7 +16,6 @@ import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.Digest; import com.google.auth.Credentials; -import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.GoogleLogger; @@ -81,7 +80,6 @@ import java.util.Map.Entry; import java.util.NoSuchElementException; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; @@ -709,24 +707,14 @@ private boolean reset(InputStream in) throws IOException { } @Override - public void uploadActionResult( - RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) - throws IOException, InterruptedException { + public ListenableFuture uploadActionResult( + RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { ByteString serialized = actionResult.toByteString(); - ListenableFuture uploadFuture = - uploadAsync( - actionKey.getDigest().getHash(), - serialized.size(), - serialized.newInput(), - /* casUpload= */ false); - try { - uploadFuture.get(); - } catch (ExecutionException e) { - Throwables.throwIfUnchecked(e.getCause()); - Throwables.throwIfInstanceOf(e.getCause(), IOException.class); - Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class); - throw new IOException(e.getCause()); - } + return uploadAsync( + actionKey.getDigest().getHash(), + serialized.size(), + serialized.newInput(), + /* casUpload= */ false); } /** diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index cb2f1639807279..3aa4e70fb8bf2c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -97,6 +97,15 @@ public final class RemoteOptions extends OptionsBase { help = "A path to a directory where the corrupted outputs will be captured to.") public PathFragment remoteCaptureCorruptedOutputs; + @Option( + name = "experimental_remote_cache_async", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = "If true, remote cache I/O will happen in the background instead of taking place as the part of a spawn." + ) + public boolean remoteCacheAsync; + @Option( name = "remote_cache", oldName = "remote_http_cache", diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/AsyncTaskCache.java b/src/main/java/com/google/devtools/build/lib/remote/util/AsyncTaskCache.java index f6f70a643764c4..4f388294021274 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/AsyncTaskCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/AsyncTaskCache.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableSet; import io.reactivex.rxjava3.annotations.NonNull; import io.reactivex.rxjava3.core.Completable; +import io.reactivex.rxjava3.core.CompletableEmitter; import io.reactivex.rxjava3.core.Single; import io.reactivex.rxjava3.core.SingleObserver; import io.reactivex.rxjava3.disposables.Disposable; @@ -27,6 +28,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.CancellationException; import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; @@ -50,21 +52,26 @@ public final class AsyncTaskCache { private final Object lock = new Object(); + private static final int STATE_ACTIVE = 0; + private static final int STATE_PENDING_SHUTDOWN = 1; + private static final int STATE_SHUTDOWN = 2; + + @GuardedBy("lock") + private int state = STATE_ACTIVE; + + @GuardedBy("lock") + private final List terminationSubscriber = new ArrayList<>(); + @GuardedBy("lock") - private final Map finished; + private final Map finished = new HashMap<>(); @GuardedBy("lock") - private final Map inProgress; + private final Map inProgress = new HashMap<>(); public static AsyncTaskCache create() { return new AsyncTaskCache<>(); } - private AsyncTaskCache() { - this.finished = new HashMap<>(); - this.inProgress = new HashMap<>(); - } - /** Returns a set of keys for tasks which is finished. */ public ImmutableSet getFinishedTasks() { synchronized (lock) { @@ -165,6 +172,8 @@ public void onSuccess(@NonNull ValueT value) { for (SingleObserver observer : ImmutableList.copyOf(observers)) { observer.onSuccess(value); } + + maybeNotifyTermination(); } } } @@ -179,6 +188,8 @@ public void onError(@NonNull Throwable error) { for (SingleObserver observer : ImmutableList.copyOf(observers)) { observer.onError(error); } + + maybeNotifyTermination(); } } } @@ -197,6 +208,18 @@ void remove(SingleObserver observer) { } } } + + void cancel() { + synchronized (lock) { + if (!terminated) { + if (upstreamDisposable != null) { + upstreamDisposable.dispose(); + } + + onError(new CancellationException("cancelled")); + } + } + } } class ExecutionDisposable implements Disposable { @@ -225,6 +248,8 @@ public boolean isDisposed() { /** * Executes a task. * + *

If the cache is already shutdown, a {@link CancellationException} will be emitted. + * * @param key identifies the task. * @param force re-execute a finished task if set to {@code true}. * @return a {@link Single} which turns to completed once the task is finished or propagates the @@ -234,6 +259,11 @@ public Single execute(KeyT key, Single task, boolean force) { return Single.create( emitter -> { synchronized (lock) { + if (state != STATE_ACTIVE) { + emitter.onError(new CancellationException("already shutdown")); + return; + } + if (!force && finished.containsKey(key)) { emitter.onSuccess(finished.get(key)); return; @@ -273,6 +303,72 @@ public void onError(@NonNull Throwable e) { }); } + /** + * Shuts the cache down. Any in progress tasks will continue running while new tasks will be + * injected with {@link CancellationException}. + */ + public void shutdown() { + synchronized (lock) { + if (state == STATE_ACTIVE) { + state = STATE_PENDING_SHUTDOWN; + maybeNotifyTermination(); + } + } + } + + /** Returns a {@link Completable} which will complete once all the in progress tasks finished. */ + public Completable awaitTermination() { + return Completable.create( + emitter -> { + synchronized (lock) { + if (state == STATE_SHUTDOWN) { + emitter.onComplete(); + } else { + terminationSubscriber.add(emitter); + + emitter.setCancellable( + () -> { + synchronized (lock) { + if (state != STATE_SHUTDOWN) { + terminationSubscriber.remove(emitter); + } + } + }); + } + } + }); + } + + /** + * Shuts the cache down. All in progress and new tasks will be cancelled with {@link + * CancellationException}. + */ + public void shutdownNow() { + shutdown(); + + synchronized (lock) { + if (state == STATE_PENDING_SHUTDOWN) { + for (Execution execution : ImmutableList.copyOf(inProgress.values())) { + execution.cancel(); + } + } + } + + awaitTermination().blockingAwait(); + } + + @GuardedBy("lock") + private void maybeNotifyTermination() { + if (state == STATE_PENDING_SHUTDOWN && inProgress.isEmpty()) { + state = STATE_SHUTDOWN; + + for (CompletableEmitter emitter : terminationSubscriber) { + emitter.onComplete(); + } + terminationSubscriber.clear(); + } + } + /** An {@link AsyncTaskCache} without result. */ public static final class NoResult { private final AsyncTaskCache> cache; @@ -311,5 +407,28 @@ public ImmutableSet getInProgressTasks() { public int getSubscriberCount(KeyT key) { return cache.getSubscriberCount(key); } + + /** + * Shuts the cache down. Any in progress tasks will continue running while new tasks will be + * injected with {@link CancellationException}. + */ + public void shutdown() { + cache.shutdown(); + } + + /** + * Returns a {@link Completable} which will complete once all the in progress tasks finished. + */ + public Completable awaitTermination() { + return cache.awaitTermination(); + } + + /** + * Shuts the cache down. All in progress and active tasks will be cancelled with {@link + * CancellationException}. + */ + public void shutdownNow() { + cache.shutdownNow(); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/RxFutures.java b/src/main/java/com/google/devtools/build/lib/remote/util/RxFutures.java index 7eef8807b964d4..10ae740f68544d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/RxFutures.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/RxFutures.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.remote.util; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.util.concurrent.AbstractFuture; @@ -24,6 +25,9 @@ import io.reactivex.rxjava3.core.CompletableEmitter; import io.reactivex.rxjava3.core.CompletableObserver; import io.reactivex.rxjava3.core.CompletableOnSubscribe; +import io.reactivex.rxjava3.core.Single; +import io.reactivex.rxjava3.core.SingleEmitter; +import io.reactivex.rxjava3.core.SingleOnSubscribe; import io.reactivex.rxjava3.disposables.Disposable; import io.reactivex.rxjava3.exceptions.Exceptions; import java.util.concurrent.Callable; @@ -110,6 +114,77 @@ public void onFailure(Throwable throwable) { } } + private static class OnceSingleOnSubscribe implements SingleOnSubscribe { + private final AtomicBoolean subscribed = new AtomicBoolean(false); + + private final Callable> callable; + private final Executor executor; + + private OnceSingleOnSubscribe(Callable> callable, Executor executor) { + this.callable = callable; + this.executor = executor; + } + + @Override + public void subscribe(@NonNull SingleEmitter emitter) throws Throwable { + try { + checkState(!subscribed.getAndSet(true), "This completable cannot be subscribed to twice"); + ListenableFuture future = callable.call(); + Futures.addCallback( + future, + new FutureCallback() { + @Override + public void onSuccess(@Nullable T t) { + checkNotNull(t, "value in future onSuccess callback is null"); + emitter.onSuccess(t); + } + + @Override + public void onFailure(Throwable throwable) { + /* + * CancellationException can be thrown in two cases: + * 1. The ListenableFuture itself is cancelled. + * 2. Single is disposed by downstream. + * + * This check is used to prevent propagating CancellationException to downstream + * when it has already disposed the Single. + */ + if (throwable instanceof CancellationException && emitter.isDisposed()) { + return; + } + + emitter.onError(throwable); + } + }, + executor); + emitter.setCancellable(() -> future.cancel(true)); + } catch (Throwable t) { + // We failed to construct and listen to the LF. Following RxJava's own behaviour, prefer + // to pass RuntimeExceptions and Errors down to the subscriber except for certain + // "fatal" exceptions. + Exceptions.throwIfFatal(t); + executor.execute(() -> emitter.onError(t)); + } + } + } + + /** + * Returns a {@link Single} that is complete once the supplied {@link ListenableFuture} has + * completed. + * + *

A {@link ListenableFuture>} represents some computation that is already in progress. We use + * {@link Callable} here to defer the execution of the thing that produces ListenableFuture until + * there is subscriber. + * + *

Errors are also propagated except for certain "fatal" exceptions defined by rxjava. Multiple + * subscriptions are not allowed. + * + *

Disposes the Single to cancel the underlying ListenableFuture. + */ + public static Single toSingle(Callable> callable, Executor executor) { + return Single.create(new OnceSingleOnSubscribe<>(callable, executor)); + } + /** * Returns a {@link ListenableFuture} that is complete once the {@link Completable} has completed. * diff --git a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java index 1c56e04ed90ca8..bd429f3496e5b7 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.ActionScanningCompletedEvent; import com.google.devtools.build.lib.actions.ActionStartedEvent; import com.google.devtools.build.lib.actions.CachingActionEvent; +import com.google.devtools.build.lib.actions.RemoteUploadEvent; import com.google.devtools.build.lib.actions.RunningActionEvent; import com.google.devtools.build.lib.actions.ScanningActionEvent; import com.google.devtools.build.lib.actions.SchedulingActionEvent; @@ -567,9 +568,8 @@ public void buildComplete(BuildCompleteEvent event) { ignoreRefreshLimitOnce(); refresh(); - // After a build has completed, only stop updating the UI if there is no more BEP - // upload happening. - if (stateTracker.pendingTransports() == 0) { + // After a build has completed, only stop updating the UI if there is no more activities. + if (stateTracker.shouldStopUpdateProgressBar()) { buildRunning = false; done = true; } @@ -721,6 +721,20 @@ public void testFilteringComplete(TestFilteringCompleteEvent event) { refresh(); } + @Subscribe + public void remoteUpload(RemoteUploadEvent event) { + stateTracker.remoteUpload(event); + + if (event.finished() && stateTracker.shouldStopUpdateProgressBar()) { + stopUpdateThread(); + flushStdOutStdErrBuffers(); + ignoreRefreshLimitOnce(); + refresh(); + } else { + refreshSoon(); + } + } + /** * Return true, if the test summary provides information that is both worth being shown in the * scroll-back buffer and new with respect to the alreay shown failure messages. @@ -789,7 +803,7 @@ public void buildEventTransportClosed(BuildEventTransportClosedEvent event) { this.handle(Event.info(null, "Transport " + event.transport().name() + " closed")); } - if (stateTracker.pendingTransports() == 0) { + if (stateTracker.shouldStopUpdateProgressBar()) { stopUpdateThread(); flushStdOutStdErrBuffers(); ignoreRefreshLimitOnce(); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java b/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java index eef7ea5c64ae7c..07c955a3106949 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/UiStateTracker.java @@ -15,6 +15,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Comparators; @@ -30,6 +31,7 @@ import com.google.devtools.build.lib.actions.ActionStartedEvent; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CachingActionEvent; +import com.google.devtools.build.lib.actions.RemoteUploadEvent; import com.google.devtools.build.lib.actions.RunningActionEvent; import com.google.devtools.build.lib.actions.ScanningActionEvent; import com.google.devtools.build.lib.actions.SchedulingActionEvent; @@ -68,6 +70,7 @@ import java.util.TreeMap; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; @@ -393,6 +396,72 @@ synchronized String describe() { private final Set bepOpenTransports = new HashSet<>(); // The point in time when closing of BEP transports was started. private long bepTransportClosingStartTimeMillis; + private final ActiveRemoteCacheIO activeRemoteCacheIO = new ActiveRemoteCacheIO(); + + static class ActiveRemoteCacheIO { + @GuardedBy("this") + private final LinkedHashMap states = new LinkedHashMap<>(); + private long nanoWaitStartTime = 0; + private int uploads; + private int downloads; + + void onBuildComplete(long nanoChangeTime) { + nanoWaitStartTime = nanoChangeTime; + } + + synchronized void onRemoteUpload(RemoteUploadEvent event, long nanoChangeTime) { + String id = event.resourceId(); + + RemoteCacheIOState state = states.get(id); + + if (event.finished()) { + if (state != null) { + states.remove(id); + uploads -= 1; + } + } else { + if (state == null) { + state = new RemoteCacheIOState(id, nanoChangeTime); + states.put(id, state); + uploads += 1; + } + + state.latestUploadEvent = event; + } + } + + synchronized List firstNStates(int limit) { + return states.values().stream().limit(limit).collect(Collectors.toList()); + } + + long waitSeconds(long nanoTime) { + return NANOSECONDS.toSeconds(nanoTime - nanoWaitStartTime); + } + + int numDownloads() { + return downloads; + } + + int numUploads() { + return uploads; + } + + synchronized int activeIOCount() { + return states.size(); + } + } + + static class RemoteCacheIOState { + final String id; + final long nanoStartTime; + RemoteUploadEvent latestUploadEvent; + + RemoteCacheIOState(String id, long nanoStartTime) { + this.id = id; + this.nanoStartTime = nanoStartTime; + } + } + UiStateTracker(Clock clock, int targetWidth) { this.activeActions = new ConcurrentHashMap<>(); @@ -474,6 +543,7 @@ void buildComplete(BuildCompleteEvent event) { buildComplete = true; // Build event protocol transports are closed right after the build complete event. bepTransportClosingStartTimeMillis = clock.currentTimeMillis(); + activeRemoteCacheIO.onBuildComplete(clock.nanoTime()); if (event.getResult().getSuccess()) { status = "INFO"; @@ -973,6 +1043,11 @@ synchronized void testFilteringComplete(TestFilteringCompleteEvent event) { } } + void remoteUpload(RemoteUploadEvent event) { + long now = clock.nanoTime(); + activeRemoteCacheIO.onRemoteUpload(event, now); + } + public synchronized void testSummary(TestSummary summary) { completedTests++; mostRecentTest = summary; @@ -990,8 +1065,10 @@ synchronized void buildEventTransportClosed(BuildEventTransportClosedEvent event bepOpenTransports.remove(event.transport()); } - synchronized int pendingTransports() { - return bepOpenTransports.size(); + synchronized boolean shouldStopUpdateProgressBar() { + return buildComplete + && bepOpenTransports.size() == 0 + && activeRemoteCacheIO.activeIOCount() == 0; } /** @@ -1006,6 +1083,9 @@ boolean progressBarTimeDependent() { if (runningDownloads.size() >= 1) { return true; } + if (buildComplete && activeRemoteCacheIO.activeIOCount() > 0) { + return true; + } if (buildComplete && !bepOpenTransports.isEmpty()) { return true; } @@ -1168,6 +1248,55 @@ private void maybeReportBepTransports(PositionAwareAnsiTerminalWriter terminalWr } } + /** + * Display any remote cache network I/Os that are still active after the build. Most likely, + * because uploading/downloading takes longer than the build itself. + */ + private void maybeReportActiveRemoteCacheIOs(PositionAwareAnsiTerminalWriter terminalWriter) + throws IOException { + if (!buildComplete || activeRemoteCacheIO.activeIOCount() == 0) { + return; + } + + int uploads = activeRemoteCacheIO.numUploads(); + int downloads = activeRemoteCacheIO.numDownloads(); + + long now = clock.nanoTime(); + long waitSeconds = activeRemoteCacheIO.waitSeconds(now); + if (waitSeconds == 0) { + // Special case for when bazel was interrupted, in which case we don't want to have a message. + return; + } + + String suffix = ""; + if (waitSeconds > SHOW_TIME_THRESHOLD_SECONDS) { + suffix = "; " + waitSeconds + "s"; + } + + String message = "Waiting for remote cache: "; + if (uploads != 0) { + if (uploads == 1) { + message += "1 upload"; + } else { + message += uploads + " uploads"; + } + } + + if (downloads != 0) { + if (uploads != 0) { + message += ", "; + } + + if (downloads == 1) { + message += "1 download"; + } else { + message += downloads + " downloads"; + } + } + + terminalWriter.newline().append(message).append(suffix); + } + synchronized void writeProgressBar( AnsiTerminalWriter rawTerminalWriter, boolean shortVersion, String timestamp) throws IOException { @@ -1197,6 +1326,7 @@ synchronized void writeProgressBar( } if (!shortVersion) { reportOnDownloads(terminalWriter); + maybeReportActiveRemoteCacheIOs(terminalWriter); maybeReportBepTransports(terminalWriter); } return; @@ -1269,6 +1399,7 @@ synchronized void writeProgressBar( } if (!shortVersion) { reportOnDownloads(terminalWriter); + maybeReportActiveRemoteCacheIOs(terminalWriter); maybeReportBepTransports(terminalWriter); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 3faa64560ef7e9..b010eb09c652c2 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -18,39 +18,30 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; -import static org.mockito.AdditionalAnswers.answerVoid; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.when; -import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionCacheGrpc.ActionCacheImplBase; import build.bazel.remote.execution.v2.ActionResult; -import build.bazel.remote.execution.v2.Command; import build.bazel.remote.execution.v2.ContentAddressableStorageGrpc.ContentAddressableStorageImplBase; 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 build.bazel.remote.execution.v2.FindMissingBlobsRequest; import build.bazel.remote.execution.v2.FindMissingBlobsResponse; import build.bazel.remote.execution.v2.GetActionResultRequest; import build.bazel.remote.execution.v2.RequestMetadata; -import build.bazel.remote.execution.v2.Tree; import build.bazel.remote.execution.v2.UpdateActionResultRequest; import com.google.api.client.json.GenericJson; import com.google.api.client.json.jackson2.JacksonFactory; import com.google.bytestream.ByteStreamGrpc.ByteStreamImplBase; -import com.google.bytestream.ByteStreamProto.QueryWriteStatusRequest; -import com.google.bytestream.ByteStreamProto.QueryWriteStatusResponse; import com.google.bytestream.ByteStreamProto.ReadRequest; import com.google.bytestream.ByteStreamProto.ReadResponse; import com.google.bytestream.ByteStreamProto.WriteRequest; import com.google.bytestream.ByteStreamProto.WriteResponse; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Maps; +import com.google.common.eventbus.EventBus; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInputHelper; @@ -60,11 +51,11 @@ import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; import com.google.devtools.build.lib.remote.Retrier.Backoff; 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.RemotePathResolver; import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -72,7 +63,6 @@ import com.google.devtools.build.lib.remote.util.TestUtils; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.testutil.Scratch; -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; @@ -104,7 +94,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; -import java.util.List; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -115,10 +104,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.ArgumentMatchers; import org.mockito.Mockito; -import org.mockito.invocation.InvocationOnMock; -import org.mockito.stubbing.Answer; /** Tests for {@link GrpcCacheClient}. */ @RunWith(JUnit4.class) @@ -126,16 +112,14 @@ public class GrpcCacheClientTest { private static final DigestUtil DIGEST_UTIL = new DigestUtil(DigestHashFunction.SHA256); - private FileSystem fs; private Path execRoot; - private FileOutErr outErr; private FakeActionInputFileCache fakeFileCache; private final MutableHandlerRegistry serviceRegistry = new MutableHandlerRegistry(); private final String fakeServerName = "fake server for " + getClass(); private Server fakeServer; private RemoteActionExecutionContext context; - private RemotePathResolver remotePathResolver; private ListeningScheduledExecutorService retryService; + private Reporter reporter; @Before public final void setUp() throws Exception { @@ -147,22 +131,18 @@ public final void setUp() throws Exception { .build() .start(); Chunker.setDefaultChunkSizeForTesting(1000); // Enough for everything to be one chunk. - fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); + FileSystem fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); execRoot = fs.getPath("/execroot/main"); FileSystemUtils.createDirectoryAndParents(execRoot); fakeFileCache = new FakeActionInputFileCache(execRoot); - remotePathResolver = RemotePathResolver.createDefault(execRoot); - Path stdout = fs.getPath("/tmp/stdout"); - Path stderr = fs.getPath("/tmp/stderr"); - FileSystemUtils.createDirectoryAndParents(stdout.getParentDirectory()); - FileSystemUtils.createDirectoryAndParents(stderr.getParentDirectory()); - outErr = new FileOutErr(stdout, stderr); RequestMetadata metadata = TracingMetadataUtils.buildMetadata( "none", "none", Digest.getDefaultInstance().getHash(), null); context = RemoteActionExecutionContext.create(metadata); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + + reporter = new Reporter(new EventBus()); } @After @@ -268,7 +248,7 @@ private static byte[] downloadBlob( public void testVirtualActionInputSupport() throws Exception { RemoteOptions options = Options.getDefaults(RemoteOptions.class); RemoteExecutionCache client = - new RemoteExecutionCache(newClient(options), options, DIGEST_UTIL); + new RemoteExecutionCache(reporter, newClient(options), options, DIGEST_UTIL); PathFragment execPath = PathFragment.create("my/exec/path"); VirtualActionInput virtualActionInput = ActionsTestUtil.createVirtualActionInput(execPath, "hello"); @@ -378,7 +358,7 @@ public void testDownloadAllResults() throws Exception { // arrange RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); + RemoteCache remoteCache = new RemoteCache(reporter, client, remoteOptions, DIGEST_UTIL); Digest fooDigest = DIGEST_UTIL.computeAsUtf8("foo-contents"); Digest barDigest = DIGEST_UTIL.computeAsUtf8("bar-contents"); @@ -397,173 +377,6 @@ public void testDownloadAllResults() throws Exception { assertThat(DIGEST_UTIL.compute(execRoot.getRelative("a/bar"))).isEqualTo(barDigest); } - @Test - public void testUploadDirectory() throws Exception { - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); - - final Digest fooDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz"); - final Digest quxDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/qux"), "abc"); - final Digest barDigest = - fakeFileCache.createScratchInputDirectory( - ActionInputHelper.fromPath("bar"), - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles( - FileNode.newBuilder() - .setIsExecutable(true) - .setName("qux") - .setDigest(quxDigest) - .build()) - .build()) - .build()); - final Path fooFile = execRoot.getRelative("a/foo"); - final Path quxFile = execRoot.getRelative("bar/qux"); - quxFile.setExecutable(true); - final Path barDir = execRoot.getRelative("bar"); - serviceRegistry.addService( - new ContentAddressableStorageImplBase() { - @Override - public void findMissingBlobs( - FindMissingBlobsRequest request, - StreamObserver responseObserver) { - assertThat(request.getBlobDigestsList()) - .containsAtLeast(fooDigest, quxDigest, barDigest); - // Nothing is missing. - responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance()); - responseObserver.onCompleted(); - } - }); - serviceRegistry.addService( - new ActionCacheImplBase() { - @Override - public void updateActionResult( - UpdateActionResultRequest request, StreamObserver responseObserver) { - responseObserver.onNext(request.getActionResult()); - responseObserver.onCompleted(); - } - }); - - ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(fooFile, barDir)); - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result).isEqualTo(expectedResult.build()); - } - - @Test - public void testUploadDirectoryEmpty() throws Exception { - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); - - final Digest barDigest = - fakeFileCache.createScratchInputDirectory( - ActionInputHelper.fromPath("bar"), - Tree.newBuilder().setRoot(Directory.newBuilder().build()).build()); - final Path barDir = execRoot.getRelative("bar"); - serviceRegistry.addService( - new ContentAddressableStorageImplBase() { - @Override - public void findMissingBlobs( - FindMissingBlobsRequest request, - StreamObserver responseObserver) { - assertThat(request.getBlobDigestsList()).contains(barDigest); - // Nothing is missing. - responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance()); - responseObserver.onCompleted(); - } - }); - serviceRegistry.addService( - new ActionCacheImplBase() { - @Override - public void updateActionResult( - UpdateActionResultRequest request, StreamObserver responseObserver) { - responseObserver.onNext(request.getActionResult()); - responseObserver.onCompleted(); - } - }); - - ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(barDir)); - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result).isEqualTo(expectedResult.build()); - } - - @Test - public void testUploadDirectoryNested() throws Exception { - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); - - final Digest wobbleDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/test/wobble"), "xyz"); - final Digest quxDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/qux"), "abc"); - final Directory testDirMessage = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("wobble").setDigest(wobbleDigest).build()) - .build(); - final Digest testDigest = DIGEST_UTIL.compute(testDirMessage); - final Tree barTree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles( - FileNode.newBuilder() - .setIsExecutable(true) - .setName("qux") - .setDigest(quxDigest)) - .addDirectories( - DirectoryNode.newBuilder().setName("test").setDigest(testDigest))) - .addChildren(testDirMessage) - .build(); - final Digest barDigest = - fakeFileCache.createScratchInputDirectory(ActionInputHelper.fromPath("bar"), barTree); - final Path quxFile = execRoot.getRelative("bar/qux"); - quxFile.setExecutable(true); - final Path barDir = execRoot.getRelative("bar"); - serviceRegistry.addService( - new ContentAddressableStorageImplBase() { - @Override - public void findMissingBlobs( - FindMissingBlobsRequest request, - StreamObserver responseObserver) { - assertThat(request.getBlobDigestsList()) - .containsAtLeast(quxDigest, barDigest, wobbleDigest); - // Nothing is missing. - responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance()); - responseObserver.onCompleted(); - } - }); - serviceRegistry.addService( - new ActionCacheImplBase() { - @Override - public void updateActionResult( - UpdateActionResultRequest request, StreamObserver responseObserver) { - responseObserver.onNext(request.getActionResult()); - responseObserver.onCompleted(); - } - }); - - ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(barDir)); - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result).isEqualTo(expectedResult.build()); - } - - private ActionResult uploadDirectory(RemoteCache remoteCache, List outputs) - throws Exception { - Action action = Action.getDefaultInstance(); - ActionKey actionKey = DIGEST_UTIL.computeActionKey(action); - Command cmd = Command.getDefaultInstance(); - return remoteCache.upload(context, remotePathResolver, actionKey, action, cmd, outputs, outErr); - } - @Test public void extraHeaders() throws Exception { RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); @@ -628,102 +441,24 @@ public void getActionResult( serviceRegistry.addService(ServerInterceptors.intercept(actionCache, interceptor)); GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); + RemoteCache remoteCache = new RemoteCache(reporter, client, remoteOptions, DIGEST_UTIL); remoteCache.downloadActionResult( context, DIGEST_UTIL.asActionKey(DIGEST_UTIL.computeAsUtf8("key")), /* inlineOutErr= */ false); } - @Test - public void testUpload() throws Exception { - RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); - GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); - - final Digest fooDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz"); - final Digest barDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar"), "x"); - final Path fooFile = execRoot.getRelative("a/foo"); - final Path barFile = execRoot.getRelative("bar"); - barFile.setExecutable(true); - Command command = Command.newBuilder().addOutputFiles("a/foo").build(); - final Digest cmdDigest = DIGEST_UTIL.compute(command.toByteArray()); - Action action = Action.newBuilder().setCommandDigest(cmdDigest).build(); - final Digest actionDigest = DIGEST_UTIL.compute(action.toByteArray()); - - outErr.getOutputStream().write("foo out".getBytes(UTF_8)); - outErr.getOutputStream().close(); - outErr.getErrorStream().write("foo err".getBytes(UTF_8)); - outErr.getOutputStream().close(); - - final Digest stdoutDigest = DIGEST_UTIL.compute(outErr.getOutputPath()); - final Digest stderrDigest = DIGEST_UTIL.compute(outErr.getErrorPath()); - - serviceRegistry.addService( - new ContentAddressableStorageImplBase() { - @Override - public void findMissingBlobs( - FindMissingBlobsRequest request, - StreamObserver responseObserver) { - assertThat(request.getBlobDigestsList()) - .containsExactly( - cmdDigest, actionDigest, fooDigest, barDigest, stdoutDigest, stderrDigest); - // Nothing is missing. - responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance()); - responseObserver.onCompleted(); - } - }); - serviceRegistry.addService( - new ActionCacheImplBase() { - @Override - public void updateActionResult( - UpdateActionResultRequest request, StreamObserver responseObserver) { - responseObserver.onNext(request.getActionResult()); - responseObserver.onCompleted(); - } - }); - - ActionResult result = - remoteCache.upload( - context, - remotePathResolver, - DIGEST_UTIL.asActionKey(actionDigest), - action, - command, - ImmutableList.of(fooFile, barFile), - outErr); - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.setStdoutDigest(stdoutDigest); - expectedResult.setStderrDigest(stderrDigest); - expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); - expectedResult - .addOutputFilesBuilder() - .setPath("bar") - .setDigest(barDigest) - .setIsExecutable(true); - assertThat(result).isEqualTo(expectedResult.build()); - } - @Test public void testUploadSplitMissingDigestsCall() throws Exception { RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); remoteOptions.maxOutboundMessageSize = 80; // Enough for one digest, but not two. GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); + RemoteCache remoteCache = new RemoteCache(reporter, client, remoteOptions, DIGEST_UTIL); final Digest fooDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz"); final Digest barDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar"), "x"); - final Path fooFile = execRoot.getRelative("a/foo"); - final Path barFile = execRoot.getRelative("bar"); - barFile.setExecutable(true); - Command command = Command.newBuilder().addOutputFiles("a/foo").build(); - final Digest cmdDigest = DIGEST_UTIL.compute(command.toByteArray()); - Action action = Action.newBuilder().setCommandDigest(cmdDigest).build(); - final Digest actionDigest = DIGEST_UTIL.compute(action.toByteArray()); AtomicInteger numGetMissingCalls = new AtomicInteger(); serviceRegistry.addService( new ContentAddressableStorageImplBase() { @@ -738,53 +473,23 @@ 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 = - remoteCache.upload( - context, - remotePathResolver, - DIGEST_UTIL.asActionKey(actionDigest), - action, - command, - ImmutableList.of(fooFile, barFile), - outErr); - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); - expectedResult - .addOutputFilesBuilder() - .setPath("bar") - .setDigest(barDigest) - .setIsExecutable(true); - assertThat(result).isEqualTo(expectedResult.build()); - assertThat(numGetMissingCalls.get()).isEqualTo(4); + getFromFuture(remoteCache.findMissingDigests(context, ImmutableList.of(fooDigest, barDigest))); + + assertThat(numGetMissingCalls.get()).isEqualTo(2); } @Test - public void testUploadCacheMissesWithRetries() throws Exception { + public void testFindMissingBlobsWithRetries() throws Exception { RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); GrpcCacheClient client = newClient(remoteOptions); - RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); - + RemoteCache remoteCache = new RemoteCache(reporter, client, remoteOptions, DIGEST_UTIL); final Digest fooDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz"); final Digest barDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar"), "x"); final Digest bazDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("baz"), "z"); - final Path fooFile = execRoot.getRelative("a/foo"); - final Path barFile = execRoot.getRelative("bar"); - final Path bazFile = execRoot.getRelative("baz"); - ActionKey actionKey = DIGEST_UTIL.asActionKey(fooDigest); // Could be any key. - barFile.setExecutable(true); serviceRegistry.addService( new ContentAddressableStorageImplBase() { private int numErrors = 4; @@ -807,11 +512,33 @@ public void findMissingBlobs( } } }); + + ImmutableSet missingDigests = + getFromFuture( + remoteCache.findMissingDigests( + context, ImmutableList.of(fooDigest, barDigest, bazDigest))); + + assertThat(missingDigests).containsExactly(fooDigest, barDigest, bazDigest); + } + + @Test + public void testUpdateActionResultWithRetries() throws Exception { + RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); + GrpcCacheClient client = newClient(remoteOptions); + RemoteCache remoteCache = new RemoteCache(reporter, client, remoteOptions, DIGEST_UTIL); + final Digest fooDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz"); + final Digest barDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar"), "x"); + final Digest bazDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("baz"), "z"); ActionResult.Builder rb = ActionResult.newBuilder(); rb.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); rb.addOutputFilesBuilder().setPath("bar").setDigest(barDigest).setIsExecutable(true); rb.addOutputFilesBuilder().setPath("baz").setDigest(bazDigest); ActionResult result = rb.build(); + ActionKey actionKey = DIGEST_UTIL.asActionKey(fooDigest); // Could be any key. + AtomicBoolean completed = new AtomicBoolean(false); serviceRegistry.addService( new ActionCacheImplBase() { private int numErrors = 4; @@ -826,6 +553,7 @@ public void updateActionResult( .setActionResult(result) .build()); if (numErrors-- <= 0) { + completed.set(true); responseObserver.onNext(result); responseObserver.onCompleted(); } else { @@ -833,82 +561,10 @@ public void updateActionResult( } } }); - ByteStreamImplBase mockByteStreamImpl = Mockito.mock(ByteStreamImplBase.class); - serviceRegistry.addService(mockByteStreamImpl); - when(mockByteStreamImpl.write(ArgumentMatchers.>any())) - .thenAnswer( - new Answer>() { - private int numErrors = 4; - @Override - @SuppressWarnings("unchecked") - public StreamObserver answer(InvocationOnMock invocation) { - StreamObserver responseObserver = - (StreamObserver) invocation.getArguments()[0]; - return new StreamObserver() { - @Override - public void onNext(WriteRequest request) { - numErrors--; - if (numErrors >= 0) { - responseObserver.onError(Status.UNAVAILABLE.asRuntimeException()); - return; - } - assertThat(request.getFinishWrite()).isTrue(); - String resourceName = request.getResourceName(); - String dataStr = request.getData().toStringUtf8(); - int size = 0; - if (resourceName.contains(fooDigest.getHash())) { - assertThat(dataStr).isEqualTo("xyz"); - size = 3; - } else if (resourceName.contains(barDigest.getHash())) { - assertThat(dataStr).isEqualTo("x"); - size = 1; - } else if (resourceName.contains(bazDigest.getHash())) { - assertThat(dataStr).isEqualTo("z"); - size = 1; - } else { - fail("Unexpected resource name in upload: " + resourceName); - } - responseObserver.onNext( - WriteResponse.newBuilder().setCommittedSize(size).build()); - } - - @Override - public void onCompleted() { - responseObserver.onCompleted(); - } - - @Override - public void onError(Throwable t) { - fail("An error occurred: " + t); - } - }; - } - }); - doAnswer( - answerVoid( - (QueryWriteStatusRequest request, - StreamObserver responseObserver) -> { - responseObserver.onNext( - QueryWriteStatusResponse.newBuilder() - .setCommittedSize(0) - .setComplete(false) - .build()); - responseObserver.onCompleted(); - })) - .when(mockByteStreamImpl) - .queryWriteStatus(any(), any()); - remoteCache.upload( - context, - remotePathResolver, - actionKey, - Action.getDefaultInstance(), - Command.getDefaultInstance(), - ImmutableList.of(fooFile, barFile, bazFile), - outErr); - // 4 times for the errors, 3 times for the successful uploads. - Mockito.verify(mockByteStreamImpl, Mockito.times(7)) - .write(ArgumentMatchers.>any()); + getFromFuture(remoteCache.uploadActionResult(context, actionKey, result)); + + assertThat(completed.get()).isTrue(); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/remote/InMemoryRemoteCache.java b/src/test/java/com/google/devtools/build/lib/remote/InMemoryRemoteCache.java index dec9a7dc848dad..117585e79dd4e8 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/InMemoryRemoteCache.java +++ b/src/test/java/com/google/devtools/build/lib/remote/InMemoryRemoteCache.java @@ -16,7 +16,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import build.bazel.remote.execution.v2.Digest; -import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -30,12 +30,13 @@ class InMemoryRemoteCache extends RemoteCache { InMemoryRemoteCache( + Reporter reporter, Map casEntries, RemoteOptions options, DigestUtil digestUtil) { - super(new InMemoryCacheClient(casEntries), options, digestUtil); + super(reporter, new InMemoryCacheClient(casEntries), options, digestUtil); } - InMemoryRemoteCache(RemoteOptions options, DigestUtil digestUtil) { - super(new InMemoryCacheClient(), options, digestUtil); + InMemoryRemoteCache(Reporter reporter, RemoteOptions options, DigestUtil digestUtil) { + super(reporter, new InMemoryCacheClient(), options, digestUtil); } Digest addContents(RemoteActionExecutionContext context, String txt) @@ -74,15 +75,4 @@ int getNumSuccessfulDownloads() { int getNumFailedDownloads() { return ((InMemoryCacheClient) cacheProtocol).getNumFailedDownloads(); } - - ImmutableSet findMissingDigests( - RemoteActionExecutionContext context, Iterable digests) - throws IOException, InterruptedException { - return Utils.getFromFuture(cacheProtocol.findMissingDigests(context, digests)); - } - - @Override - public void close() { - cacheProtocol.close(); - } } 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 406130daec8d47..274fb8e2b40ef7 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 @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; +import com.google.common.eventbus.EventBus; import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.SettableFuture; @@ -36,6 +37,7 @@ import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.events.Reporter; 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.InMemoryCacheClient; @@ -391,6 +393,10 @@ private static RemoteCache newCache( for (Map.Entry entry : cacheEntries.entrySet()) { cacheEntriesByteArray.put(entry.getKey(), entry.getValue().toByteArray()); } - return new RemoteCache(new InMemoryCacheClient(cacheEntriesByteArray), options, digestUtil); + return new RemoteCache( + new Reporter(new EventBus()), + new InMemoryCacheClient(cacheEntriesByteArray), + options, + digestUtil); } } 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 e527d05a020569..441958a56d9ca3 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 @@ -14,46 +14,33 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; -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 build.bazel.remote.execution.v2.RequestMetadata; -import build.bazel.remote.execution.v2.SymlinkNode; -import build.bazel.remote.execution.v2.Tree; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.eventbus.EventBus; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; -import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.clock.JavaClock; -import com.google.devtools.build.lib.remote.RemoteCache.UploadManifest; +import com.google.devtools.build.lib.events.Reporter; 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.RemotePathResolver; 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.remote.util.Utils; import com.google.devtools.build.lib.testutil.TestUtils; 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; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; -import java.io.IOException; import java.io.OutputStream; import java.nio.charset.StandardCharsets; import java.util.concurrent.ConcurrentHashMap; @@ -72,12 +59,12 @@ public class RemoteCacheTests { private RemoteActionExecutionContext context; - private RemotePathResolver remotePathResolver; private FileSystem fs; private Path execRoot; ArtifactRoot artifactRoot; private final DigestUtil digestUtil = new DigestUtil(DigestHashFunction.SHA256); private FakeActionInputFileCache fakeFileCache; + private Reporter reporter; private ListeningScheduledExecutorService retryService; @@ -90,11 +77,11 @@ public void setUp() throws Exception { fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); execRoot = fs.getPath("/execroot/main"); execRoot.createDirectoryAndParents(); - remotePathResolver = RemotePathResolver.createDefault(execRoot); fakeFileCache = new FakeActionInputFileCache(execRoot); artifactRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "outputs"); artifactRoot.getRoot().asPath().createDirectoryAndParents(); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + reporter = new Reporter(new EventBus()); } @After @@ -103,483 +90,6 @@ public void afterEverything() throws InterruptedException { retryService.awaitTermination(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS); } - @Test - public void uploadAbsoluteFileSymlinkAsFile() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path link = fs.getPath("/execroot/main/link"); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - link.createSymbolicLink(target); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(link)); - Digest digest = digestUtil.compute(target); - assertThat(um.getDigestToFile()).containsExactly(digest, link); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadAbsoluteDirectorySymlinkAsDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path foo = fs.getPath("/execroot/main/dir/foo"); - FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/link"); - link.createSymbolicLink(dir); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(link)); - Digest digest = digestUtil.compute(foo); - assertThat(um.getDigestToFile()).containsExactly(digest, fs.getPath("/execroot/main/link/foo")); - - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeFileSymlinkAsFile() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path link = fs.getPath("/execroot/main/link"); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - link.createSymbolicLink(target.relativeTo(execRoot)); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ false, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(link)); - Digest digest = digestUtil.compute(target); - assertThat(um.getDigestToFile()).containsExactly(digest, link); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeDirectorySymlinkAsDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path foo = fs.getPath("/execroot/main/dir/foo"); - FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/link"); - link.createSymbolicLink(dir.relativeTo(execRoot)); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ false, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(link)); - Digest digest = digestUtil.compute(foo); - assertThat(um.getDigestToFile()).containsExactly(digest, fs.getPath("/execroot/main/link/foo")); - - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeFileSymlink() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path link = fs.getPath("/execroot/main/link"); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - link.createSymbolicLink(target.relativeTo(execRoot)); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(link)); - assertThat(um.getDigestToFile()).isEmpty(); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputFileSymlinksBuilder().setPath("link").setTarget("target"); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeDirectorySymlink() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path file = fs.getPath("/execroot/main/dir/foo"); - FileSystemUtils.writeContent(file, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/link"); - link.createSymbolicLink(dir.relativeTo(execRoot)); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(link)); - assertThat(um.getDigestToFile()).isEmpty(); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectorySymlinksBuilder().setPath("link").setTarget("dir"); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadDanglingSymlinkError() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path link = fs.getPath("/execroot/main/link"); - Path target = fs.getPath("/execroot/main/target"); - link.createSymbolicLink(target.relativeTo(execRoot)); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - IOException e = assertThrows(IOException.class, () -> um.addFiles(ImmutableList.of(link))); - assertThat(e).hasMessageThat().contains("dangling"); - assertThat(e).hasMessageThat().contains("/execroot/main/link"); - assertThat(e).hasMessageThat().contains("target"); - } - - @Test - public void uploadSymlinksNoAllowError() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path link = fs.getPath("/execroot/main/link"); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - link.createSymbolicLink(target.relativeTo(execRoot)); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ false); - ExecException e = assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(link))); - assertThat(e).hasMessageThat().contains("symbolic link"); - assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload"); - } - - @Test - public void uploadAbsoluteFileSymlinkInDirectoryAsFile() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(target); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(dir)); - Digest digest = digestUtil.compute(target); - assertThat(um.getDigestToFile()).containsExactly(digest, link); - - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("link").setDigest(digest))) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadAbsoluteDirectorySymlinkInDirectoryAsDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path bardir = fs.getPath("/execroot/main/bardir"); - bardir.createDirectory(); - Path foo = fs.getPath("/execroot/main/bardir/foo"); - FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(bardir); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(dir)); - Digest digest = digestUtil.compute(foo); - assertThat(um.getDigestToFile()) - .containsExactly(digest, fs.getPath("/execroot/main/dir/link/foo")); - - Directory barDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)) - .build(); - Digest barDigest = digestUtil.compute(barDir); - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addDirectories( - DirectoryNode.newBuilder().setName("link").setDigest(barDigest))) - .addChildren(barDir) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeFileSymlinkInDirectoryAsFile() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(PathFragment.create("../target")); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ false, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(dir)); - Digest digest = digestUtil.compute(target); - assertThat(um.getDigestToFile()).containsExactly(digest, link); - - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("link").setDigest(digest))) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeDirectorySymlinkInDirectoryAsDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path bardir = fs.getPath("/execroot/main/bardir"); - bardir.createDirectory(); - Path foo = fs.getPath("/execroot/main/bardir/foo"); - FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(PathFragment.create("../bardir")); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ false, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(dir)); - Digest digest = digestUtil.compute(foo); - assertThat(um.getDigestToFile()) - .containsExactly(digest, fs.getPath("/execroot/main/dir/link/foo")); - - Directory barDir = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)) - .build(); - Digest barDigest = digestUtil.compute(barDir); - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addDirectories( - DirectoryNode.newBuilder().setName("link").setDigest(barDigest))) - .addChildren(barDir) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeFileSymlinkInDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(PathFragment.create("../target")); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(dir)); - assertThat(um.getDigestToFile()).isEmpty(); - - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../target"))) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadRelativeDirectorySymlinkInDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path bardir = fs.getPath("/execroot/main/bardir"); - bardir.createDirectory(); - Path foo = fs.getPath("/execroot/main/bardir/foo"); - FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(PathFragment.create("../bardir")); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - um.addFiles(ImmutableList.of(dir)); - assertThat(um.getDigestToFile()).isEmpty(); - - Tree tree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../bardir"))) - .build(); - Digest treeDigest = digestUtil.compute(tree); - - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); - assertThat(result.build()).isEqualTo(expectedResult.build()); - } - - @Test - public void uploadDanglingSymlinkInDirectoryError() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/dir"); - dir.createDirectory(); - Path target = fs.getPath("/execroot/target"); - Path link = fs.getPath("/execroot/dir/link"); - link.createSymbolicLink(target); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ true); - IOException e = assertThrows(IOException.class, () -> um.addFiles(ImmutableList.of(dir))); - assertThat(e).hasMessageThat().contains("dangling"); - assertThat(e).hasMessageThat().contains("/execroot/dir/link"); - assertThat(e).hasMessageThat().contains("/execroot/target"); - } - - @Test - public void uploadSymlinkInDirectoryNoAllowError() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - Path dir = fs.getPath("/execroot/main/dir"); - dir.createDirectory(); - Path target = fs.getPath("/execroot/main/target"); - FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); - Path link = fs.getPath("/execroot/main/dir/link"); - link.createSymbolicLink(target); - - UploadManifest um = - new UploadManifest( - digestUtil, - remotePathResolver, - result, - /*uploadSymlinks=*/ true, - /*allowSymlinks=*/ false); - ExecException e = assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(dir))); - assertThat(e).hasMessageThat().contains("symbolic link"); - assertThat(e).hasMessageThat().contains("dir/link"); - assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload"); - } - - - - - @Test public void testDownloadEmptyBlobAndFile() throws Exception { // Test that downloading an empty BLOB/file does not try to perform a download. @@ -590,10 +100,10 @@ public void testDownloadEmptyBlobAndFile() throws Exception { Digest emptyDigest = digestUtil.compute(new byte[0]); // act and assert - assertThat(Utils.getFromFuture(remoteCache.downloadBlob(context, emptyDigest))).isEmpty(); + assertThat(getFromFuture(remoteCache.downloadBlob(context, emptyDigest))).isEmpty(); try (OutputStream out = file.getOutputStream()) { - Utils.getFromFuture(remoteCache.downloadFile(context, file, emptyDigest)); + getFromFuture(remoteCache.downloadFile(context, file, emptyDigest)); } assertThat(file.exists()).isTrue(); assertThat(file.getFileSize()).isEqualTo(0); @@ -636,10 +146,10 @@ public void testDownloadFileWithSymlinkTemplate() throws Exception { Path file = fs.getPath("/execroot/symlink-to-file"); RemoteOptions options = Options.getDefaults(RemoteOptions.class); options.remoteDownloadSymlinkTemplate = "/home/alice/cas/{hash}-{size_bytes}"; - RemoteCache remoteCache = new InMemoryRemoteCache(cas, options, digestUtil); + RemoteCache remoteCache = new InMemoryRemoteCache(reporter, cas, options, digestUtil); // act - Utils.getFromFuture(remoteCache.downloadFile(context, file, helloDigest)); + getFromFuture(remoteCache.downloadFile(context, file, helloDigest)); // assert assertThat(file.isSymbolicLink()).isTrue(); @@ -649,60 +159,6 @@ public void testDownloadFileWithSymlinkTemplate() throws Exception { "/home/alice/cas/a378b939ad2e1d470a9a28b34b0e256b189e85cb236766edc1d46ec3b6ca82e5-14")); } - @Test - public void testUploadDirectory() throws Exception { - // Test that uploading a directory works. - - // arrange - Digest fooDigest = fakeFileCache.createScratchInput(ActionInputHelper.fromPath("a/foo"), "xyz"); - Digest quxDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/qux"), "abc"); - Digest barDigest = - fakeFileCache.createScratchInputDirectory( - ActionInputHelper.fromPath("bar"), - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles( - FileNode.newBuilder() - .setIsExecutable(true) - .setName("qux") - .setDigest(quxDigest) - .build()) - .build()) - .build()); - Path fooFile = execRoot.getRelative("a/foo"); - Path quxFile = execRoot.getRelative("bar/qux"); - quxFile.setExecutable(true); - Path barDir = execRoot.getRelative("bar"); - Command cmd = Command.newBuilder().addOutputFiles("bla").build(); - Digest cmdDigest = digestUtil.compute(cmd); - Action action = Action.newBuilder().setCommandDigest(cmdDigest).build(); - Digest actionDigest = digestUtil.compute(action); - - // act - InMemoryRemoteCache remoteCache = newRemoteCache(); - ActionResult result = - remoteCache.upload( - context, - remotePathResolver, - digestUtil.asActionKey(actionDigest), - action, - cmd, - ImmutableList.of(fooFile, barDir), - new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); - - // assert - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputFilesBuilder().setPath("a/foo").setDigest(fooDigest); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result).isEqualTo(expectedResult.build()); - - ImmutableList toQuery = - ImmutableList.of(fooDigest, quxDigest, barDigest, cmdDigest, actionDigest); - 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. @@ -710,137 +166,17 @@ public void upload_emptyBlobAndFile_doNotPerformUpload() throws Exception { 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))) + getFromFuture(remoteCache.uploadBlob(context, emptyDigest, ByteString.EMPTY)); + assertThat(getFromFuture(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest)))) .containsExactly(emptyDigest); - Utils.getFromFuture(remoteCache.uploadFile(context, emptyDigest, file)); - assertThat(remoteCache.findMissingDigests(context, ImmutableSet.of(emptyDigest))) + getFromFuture(remoteCache.uploadFile(context, emptyDigest, file)); + assertThat(getFromFuture(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. - - // arrange - final Digest barDigest = - fakeFileCache.createScratchInputDirectory( - ActionInputHelper.fromPath("bar"), - Tree.newBuilder().setRoot(Directory.newBuilder().build()).build()); - final Path barDir = execRoot.getRelative("bar"); - Action action = Action.getDefaultInstance(); - ActionKey actionDigest = digestUtil.computeActionKey(action); - Command cmd = Command.getDefaultInstance(); - - // act - InMemoryRemoteCache remoteCache = newRemoteCache(); - ActionResult result = - remoteCache.upload( - context, - remotePathResolver, - actionDigest, - action, - cmd, - ImmutableList.of(barDir), - new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); - - // assert - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result).isEqualTo(expectedResult.build()); - assertThat(remoteCache.findMissingDigests(context, ImmutableList.of(barDigest))).isEmpty(); - } - - @Test - public void testUploadNestedDirectory() throws Exception { - // Test that uploading a nested directory works. - - // arrange - final Digest wobbleDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/test/wobble"), "xyz"); - final Digest quxDigest = - fakeFileCache.createScratchInput(ActionInputHelper.fromPath("bar/qux"), "abc"); - final Directory testDirMessage = - Directory.newBuilder() - .addFiles(FileNode.newBuilder().setName("wobble").setDigest(wobbleDigest).build()) - .build(); - final Digest testDigest = digestUtil.compute(testDirMessage); - final Tree barTree = - Tree.newBuilder() - .setRoot( - Directory.newBuilder() - .addFiles( - FileNode.newBuilder() - .setIsExecutable(true) - .setName("qux") - .setDigest(quxDigest)) - .addDirectories( - DirectoryNode.newBuilder().setName("test").setDigest(testDigest))) - .addChildren(testDirMessage) - .build(); - final Digest barDigest = - fakeFileCache.createScratchInputDirectory(ActionInputHelper.fromPath("bar"), barTree); - - final Path quxFile = execRoot.getRelative("bar/qux"); - quxFile.setExecutable(true); - final Path barDir = execRoot.getRelative("bar"); - - Action action = Action.getDefaultInstance(); - ActionKey actionDigest = digestUtil.computeActionKey(action); - Command cmd = Command.getDefaultInstance(); - - // act - InMemoryRemoteCache remoteCache = newRemoteCache(); - ActionResult result = - remoteCache.upload( - context, - remotePathResolver, - actionDigest, - action, - cmd, - ImmutableList.of(barDir), - new FileOutErr(execRoot.getRelative("stdout"), execRoot.getRelative("stderr"))); - - // assert - ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); - assertThat(result).isEqualTo(expectedResult.build()); - - ImmutableList toQuery = ImmutableList.of(wobbleDigest, quxDigest, barDigest); - assertThat(remoteCache.findMissingDigests(context, toQuery)).isEmpty(); - } - private InMemoryRemoteCache newRemoteCache() { RemoteOptions options = Options.getDefaults(RemoteOptions.class); - return new InMemoryRemoteCache(options, digestUtil); + return new InMemoryRemoteCache(reporter, options, digestUtil); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 33f7fffdbef6d8..8627804760c952 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS; import static com.google.devtools.build.lib.remote.util.DigestUtil.toBinaryDigest; +import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContent; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; @@ -45,7 +46,9 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; @@ -55,11 +58,13 @@ import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.SimpleSpawn; import com.google.devtools.build.lib.actions.Spawn; +import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteAction; import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult; @@ -103,6 +108,7 @@ public class RemoteExecutionServiceTest { private FakeActionInputFileCache fakeFileCache; private RemotePathResolver remotePathResolver; private FileOutErr outErr; + private Reporter reporter; private InMemoryRemoteCache cache; private RemoteActionExecutionContext remoteActionExecutionContext; @@ -125,7 +131,9 @@ public final void setUp() throws Exception { checkNotNull(stderr.getParentDirectory()).createDirectoryAndParents(); outErr = new FileOutErr(stdout, stderr); - cache = new InMemoryRemoteCache(remoteOptions, digestUtil); + reporter = new Reporter(new EventBus()); + + cache = new InMemoryRemoteCache(reporter, remoteOptions, digestUtil); RequestMetadata metadata = TracingMetadataUtils.buildMetadata("none", "none", "action-id", null); @@ -1044,6 +1052,185 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1)); } + @Test + public void uploadOutputs_uploadDirectory_works() throws Exception { + // Test that uploading a directory works. + + // arrange + Digest fooDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("outputs/a/foo"), "xyz"); + Digest quxDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("outputs/bar/qux"), "abc"); + Digest barDigest = + fakeFileCache.createScratchInputDirectory( + ActionInputHelper.fromPath("outputs/bar"), + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setIsExecutable(true) + .setName("qux") + .setDigest(quxDigest) + .build()) + .build()) + .build()); + Path fooFile = execRoot.getRelative("outputs/a/foo"); + Path quxFile = execRoot.getRelative("outputs/bar/qux"); + quxFile.setExecutable(true); + Path barDir = execRoot.getRelative("outputs/bar"); + Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, fooFile); + Artifact outputDirectory = ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, barDir.relativeTo(execRoot)); + RemoteExecutionService service = newRemoteExecutionService(); + Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputFile, outputDirectory)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteAction action = service.buildRemoteAction(spawn, context); + SpawnResult spawnResult = new SpawnResult.Builder() + .setExitCode(0) + .setStatus(SpawnResult.Status.SUCCESS) + .setRunnerName("test") + .build(); + + // act + UploadManifest manifest = service.buildUploadManifest(action, spawnResult); + service.uploadOutputs(action, spawnResult); + + // assert + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("outputs/a/foo").setDigest(fooDigest); + expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); + + ImmutableList toQuery = ImmutableList.of(fooDigest, quxDigest, barDigest); + assertThat(getFromFuture(cache.findMissingDigests(remoteActionExecutionContext, toQuery))).isEmpty(); + } + + @Test + public void uploadOutputs_uploadEmptyDirectory_works() throws Exception { + // Test that uploading an empty directory works. + + // arrange + final Digest barDigest = + fakeFileCache.createScratchInputDirectory( + ActionInputHelper.fromPath("outputs/bar"), + Tree.newBuilder().setRoot(Directory.newBuilder().build()).build()); + final Path barDir = execRoot.getRelative("outputs/bar"); + Artifact outputDirectory = ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, barDir.relativeTo(execRoot)); + RemoteExecutionService service = newRemoteExecutionService(); + Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputDirectory)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteAction action = service.buildRemoteAction(spawn, context); + SpawnResult spawnResult = new SpawnResult.Builder() + .setExitCode(0) + .setStatus(SpawnResult.Status.SUCCESS) + .setRunnerName("test") + .build(); + + // act + UploadManifest manifest = service.buildUploadManifest(action, spawnResult); + service.uploadOutputs(action, spawnResult); + + // assert + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); + assertThat( + getFromFuture( + cache.findMissingDigests( + remoteActionExecutionContext, ImmutableList.of(barDigest)))) + .isEmpty(); + } + + @Test + public void uploadOutputs_uploadNestedDirectory_works() throws Exception { + // Test that uploading a nested directory works. + + // arrange + final Digest wobbleDigest = + fakeFileCache.createScratchInput( + ActionInputHelper.fromPath("outputs/bar/test/wobble"), "xyz"); + final Digest quxDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("outputs/bar/qux"), "abc"); + final Directory testDirMessage = + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("wobble").setDigest(wobbleDigest).build()) + .build(); + final Digest testDigest = digestUtil.compute(testDirMessage); + final Tree barTree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles( + FileNode.newBuilder() + .setIsExecutable(true) + .setName("qux") + .setDigest(quxDigest)) + .addDirectories( + DirectoryNode.newBuilder().setName("test").setDigest(testDigest))) + .addChildren(testDirMessage) + .build(); + final Digest barDigest = + fakeFileCache.createScratchInputDirectory( + ActionInputHelper.fromPath("outputs/bar"), barTree); + + final Path quxFile = execRoot.getRelative("outputs/bar/qux"); + quxFile.setExecutable(true); + final Path barDir = execRoot.getRelative("outputs/bar"); + + Artifact outputDirectory = ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, barDir.relativeTo(execRoot)); + RemoteExecutionService service = newRemoteExecutionService(); + Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputDirectory)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteAction action = service.buildRemoteAction(spawn, context); + SpawnResult spawnResult = new SpawnResult.Builder() + .setExitCode(0) + .setStatus(SpawnResult.Status.SUCCESS) + .setRunnerName("test") + .build(); + + // act + UploadManifest manifest = service.buildUploadManifest(action, spawnResult); + service.uploadOutputs(action, spawnResult); + + // assert + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); + + ImmutableList toQuery = ImmutableList.of(wobbleDigest, quxDigest, barDigest); + assertThat(getFromFuture(cache.findMissingDigests(remoteActionExecutionContext, toQuery))).isEmpty(); + } + + @Test + public void uploadOutputs_emptyOutputs_doNotPerformUpload() throws Exception { + // Test that uploading an empty output does not try to perform an upload. + + // arrange + Digest emptyDigest = + fakeFileCache.createScratchInput(ActionInputHelper.fromPath("outputs/bar/test/wobble"), ""); + Path file = execRoot.getRelative("outputs/bar/test/wobble"); + Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file); + RemoteExecutionService service = newRemoteExecutionService(); + Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputFile)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteAction action = service.buildRemoteAction(spawn, context); + SpawnResult spawnResult = new SpawnResult.Builder() + .setExitCode(0) + .setStatus(SpawnResult.Status.SUCCESS) + .setRunnerName("test") + .build(); + + // act + service.uploadOutputs(action, spawnResult); + + // assert + assertThat(getFromFuture(cache.findMissingDigests(remoteActionExecutionContext, ImmutableSet.of(emptyDigest)))) + .containsExactly(emptyDigest); + } + private Spawn newSpawnFromResult(RemoteActionResult result) { return newSpawnFromResult(ImmutableMap.of(), result); } @@ -1125,6 +1312,8 @@ private RemoteExecutionService newRemoteExecutionService(RemoteOptions remoteOpt private RemoteExecutionService newRemoteExecutionService( RemoteOptions remoteOptions, Collection topLevelOutputs) { return new RemoteExecutionService( + reporter, + /*verboseFailures=*/ true, execRoot, remotePathResolver, "none", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index a85d398d0f7c4b..a5e346d5595862 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -17,6 +17,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -24,9 +25,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; -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.OutputFile; import build.bazel.remote.execution.v2.RequestMetadata; @@ -83,7 +82,6 @@ import java.io.IOException; import java.time.Duration; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.SortedMap; import org.junit.Before; @@ -129,8 +127,7 @@ public int getId() { public void prefetchInputs() {} @Override - public void lockOutputFiles() { - } + public void lockOutputFiles() {} @Override public boolean speculating() { @@ -209,6 +206,8 @@ private RemoteSpawnCache remoteSpawnCacheWithOptions(RemoteOptions options) { RemoteExecutionService service = spy( new RemoteExecutionService( + reporter, + /*verboseFailures=*/ true, execRoot, remotePathResolver, BUILD_REQUEST_ID, @@ -289,15 +288,7 @@ public ActionResult answer(InvocationOnMock invocation) { // assert // All other methods on RemoteActionCache have side effects, so we verify all of them. verify(service).downloadOutputs(any(), eq(RemoteActionResult.createFromCache(actionResult))); - verify(remoteCache, never()) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - any(Collection.class), - any(FileOutErr.class)); + verify(service, never()).uploadOutputs(any(), any()); assertThat(result.setupSuccess()).isTrue(); assertThat(result.exitCode()).isEqualTo(0); assertThat(result.isCacheHit()).isTrue(); @@ -310,6 +301,7 @@ public ActionResult answer(InvocationOnMock invocation) { @Test public void cacheMiss() throws Exception { RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteExecutionService service = cache.getRemoteExecutionService(); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isFalse(); SpawnResult result = @@ -318,37 +310,9 @@ public void cacheMiss() throws Exception { .setStatus(Status.SUCCESS) .setRunnerName("test") .build(); - ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); - doAnswer( - new Answer() { - @Override - public Void answer(InvocationOnMock invocation) { - RemoteActionExecutionContext context = invocation.getArgument(0); - RequestMetadata meta = context.getRequestMetadata(); - assertThat(meta.getCorrelatedInvocationsId()).isEqualTo(BUILD_REQUEST_ID); - assertThat(meta.getToolInvocationId()).isEqualTo(COMMAND_ID); - return null; - } - }) - .when(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + doNothing().when(service).uploadOutputs(any(), any()); entry.store(result); - verify(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + verify(service).uploadOutputs(any(), any()); assertThat(progressUpdates) .containsExactly( SpawnCheckingCacheEvent.create("remote-cache"), @@ -489,6 +453,7 @@ public void noRemoteExecStillUsesCache() throws Exception { public void failedActionsAreNotUploaded() throws Exception { // Only successful action results are uploaded to the remote cache. RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteExecutionService service = cache.getRemoteExecutionService(); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); verify(remoteCache) .downloadActionResult( @@ -506,17 +471,8 @@ public void failedActionsAreNotUploaded() throws Exception { .build()) .setRunnerName("test") .build(); - ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); entry.store(result); - verify(remoteCache, never()) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + verify(service, never()).uploadOutputs(any(), any()); assertThat(progressUpdates) .containsExactly( SpawnCheckingCacheEvent.create("remote-cache"), @@ -526,6 +482,7 @@ public void failedActionsAreNotUploaded() throws Exception { @Test public void printWarningIfUploadFails() throws Exception { RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteExecutionService service = cache.getRemoteExecutionService(); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); assertThat(entry.hasResult()).isFalse(); SpawnResult result = @@ -536,27 +493,10 @@ public void printWarningIfUploadFails() throws Exception { .build(); ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); - doThrow(new IOException("cache down")) - .when(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + doThrow(new IOException("cache down")).when(service).uploadOutputs(any(), any()); entry.store(result); - verify(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + verify(service).uploadOutputs(any(), eq(result)); assertThat(eventHandler.getEvents()).hasSize(1); Event evt = eventHandler.getEvents().get(0); @@ -571,6 +511,7 @@ public void printWarningIfUploadFails() throws Exception { @Test public void printWarningIfDownloadFails() throws Exception { RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteExecutionService service = cache.getRemoteExecutionService(); doThrow(new IOException(io.grpc.Status.UNAVAILABLE.asRuntimeException())) .when(remoteCache) .downloadActionResult( @@ -586,38 +527,10 @@ public void printWarningIfDownloadFails() throws Exception { .setStatus(Status.SUCCESS) .setRunnerName("test") .build(); - ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); - doAnswer( - new Answer() { - @Override - public Void answer(InvocationOnMock invocation) { - RemoteActionExecutionContext context = invocation.getArgument(0); - RequestMetadata meta = context.getRequestMetadata(); - assertThat(meta.getCorrelatedInvocationsId()).isEqualTo(BUILD_REQUEST_ID); - assertThat(meta.getToolInvocationId()).isEqualTo(COMMAND_ID); - return null; - } - }) - .when(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + doNothing().when(service).uploadOutputs(any(), any()); entry.store(result); - verify(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + verify(service).uploadOutputs(any(), eq(result)); assertThat(eventHandler.getEvents()).hasSize(1); Event evt = eventHandler.getEvents().get(0); @@ -632,6 +545,7 @@ public Void answer(InvocationOnMock invocation) { @Test public void orphanedCachedResultIgnored() throws Exception { RemoteSpawnCache cache = createRemoteSpawnCache(); + RemoteExecutionService service = cache.getRemoteExecutionService(); Digest digest = digestUtil.computeAsUtf8("bla"); ActionResult actionResult = ActionResult.newBuilder() @@ -653,7 +567,7 @@ public ActionResult answer(InvocationOnMock invocation) { } }); doThrow(new CacheNotFoundException(digest)) - .when(cache.getRemoteExecutionService()) + .when(service) .downloadOutputs(any(), eq(RemoteActionResult.createFromCache(actionResult))); CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy); @@ -664,38 +578,10 @@ public ActionResult answer(InvocationOnMock invocation) { .setStatus(Status.SUCCESS) .setRunnerName("test") .build(); - ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); - doAnswer( - new Answer() { - @Override - public Void answer(InvocationOnMock invocation) { - RemoteActionExecutionContext context = invocation.getArgument(0); - RequestMetadata meta = context.getRequestMetadata(); - assertThat(meta.getCorrelatedInvocationsId()).isEqualTo(BUILD_REQUEST_ID); - assertThat(meta.getToolInvocationId()).isEqualTo(COMMAND_ID); - return null; - } - }) - .when(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + doNothing().when(service).uploadOutputs(any(), any()); entry.store(result); - verify(remoteCache) - .upload( - any(RemoteActionExecutionContext.class), - any(RemotePathResolver.class), - any(ActionKey.class), - any(Action.class), - any(Command.class), - eq(outputFiles), - eq(outErr)); + verify(service).uploadOutputs(any(), eq(result)); assertThat(progressUpdates) .containsExactly( SpawnCheckingCacheEvent.create("remote-cache"), diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index 6ecd3f819efa4c..738da096ee4ed2 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; @@ -147,8 +148,6 @@ public class RemoteSpawnRunnerTest { @Mock private SpawnRunner localRunner; - private RemoteExecutionService service; - // The action key of the Spawn returned by newSimpleSpawn(). private final String simpleActionId = "eb45b20cc979d504f96b9efc9a08c48103c6f017afa09c0df5c70a5f92a98ea8"; @@ -193,6 +192,7 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception { remoteOptions.remoteExecutionPriority = 2; RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService();; ExecuteResponse succeeded = ExecuteResponse.newBuilder() @@ -221,7 +221,7 @@ public void nonCachableSpawnsShouldNotBeCached_remote() throws Exception { // TODO(olaola): verify that the uploaded action has the doNotCache set. verify(service, never()).lookupCache(any()); - verify(cache, never()).upload(any(), any(), any(), any(), any(), any(), any()); + verify(service, never()).uploadOutputs(any(), any()); verifyNoMoreInteractions(localRunner); } @@ -272,6 +272,8 @@ public void cachableSpawnsShouldBeCached_localFallback() throws Exception { remoteOptions.remoteUploadLocalResults = true; RemoteSpawnRunner runner = spy(newSpawnRunner()); + RemoteExecutionService service = runner.getRemoteExecutionService(); + doNothing().when(service).uploadOutputs(any(), any()); // Throw an IOException to trigger the local fallback. when(executor.executeRemotely( @@ -297,7 +299,7 @@ public void cachableSpawnsShouldBeCached_localFallback() throws Exception { verify(localRunner).exec(eq(spawn), eq(policy)); verify(runner) .execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true)); - verify(cache).upload(any(), any(), any(), any(), any(), any(), any()); + verify(service).uploadOutputs(any(), eq(res)); } @Test @@ -308,6 +310,7 @@ public void failedLocalActionShouldNotBeUploaded() throws Exception { remoteOptions.remoteUploadLocalResults = true; RemoteSpawnRunner runner = spy(newSpawnRunner()); + RemoteExecutionService service = runner.getRemoteExecutionService(); // Throw an IOException to trigger the local fallback. when(executor.executeRemotely( @@ -329,7 +332,7 @@ public void failedLocalActionShouldNotBeUploaded() throws Exception { verify(localRunner).exec(eq(spawn), eq(policy)); verify(runner) .execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true)); - verify(cache, never()).upload(any(), any(), any(), any(), any(), any(), any()); + verify(service, never()).uploadOutputs(any(), any()); } @Test @@ -348,12 +351,14 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { .thenReturn(failedAction); RemoteSpawnRunner runner = spy(newSpawnRunner()); + RemoteExecutionService service = runner.getRemoteExecutionService(); // Throw an IOException to trigger the local fallback. when(executor.executeRemotely( any(RemoteActionExecutionContext.class), any(ExecuteRequest.class), any(OperationObserver.class))) .thenThrow(IOException.class); + doNothing().when(service).uploadOutputs(any(), any()); Spawn spawn = newSimpleSpawn(); SpawnExecutionContext policy = getSpawnContext(spawn); @@ -366,12 +371,12 @@ public void treatFailedCachedActionAsCacheMiss_local() throws Exception { .build(); when(localRunner.exec(eq(spawn), eq(policy))).thenReturn(succeeded); - runner.exec(spawn, policy); + SpawnResult result = runner.exec(spawn, policy); verify(localRunner).exec(eq(spawn), eq(policy)); verify(runner) .execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true)); - verify(service).uploadOutputs(any()); + verify(service).uploadOutputs(any(), eq(result)); verify(service, never()).downloadOutputs(any(), any()); } @@ -388,6 +393,7 @@ public void treatFailedCachedActionAsCacheMiss_remote() throws Exception { .thenReturn(failedAction); RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() @@ -418,6 +424,8 @@ public void printWarningIfCacheIsDown() throws Exception { reporter.addHandler(eventHandler); RemoteSpawnRunner runner = newSpawnRunner(reporter); + RemoteExecutionService service = runner.getRemoteExecutionService(); + // Trigger local fallback when(executor.executeRemotely( any(RemoteActionExecutionContext.class), @@ -428,15 +436,12 @@ public void printWarningIfCacheIsDown() throws Exception { Spawn spawn = newSimpleSpawn(); SpawnExecutionContext policy = getSpawnContext(spawn); - when(cache.downloadActionResult( - any(RemoteActionExecutionContext.class), - any(ActionKey.class), - /* inlineOutErr= */ eq(false))) - .thenThrow(new IOException("cache down")); - doThrow(new IOException("cache down")) - .when(cache) - .upload(any(), any(), any(), any(), any(), any(), any()); + .when(service) + .lookupCache(any()); + doThrow(new IOException("cache down")) + .when(service) + .uploadOutputs(any(), any()); SpawnResult res = new SpawnResult.Builder() @@ -556,6 +561,7 @@ public void testLocalFallbackFailureRemoteExecutorFailure() throws Exception { @Test public void testHumanReadableServerLogsSavedForFailingAction() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); Digest logDigest = digestUtil.computeAsUtf8("bla"); Path logPath = logDir.getRelative(simpleActionId).getRelative("logname"); ExecuteResponse resp = @@ -593,6 +599,7 @@ public void testHumanReadableServerLogsSavedForFailingAction() throws Exception public void testHumanReadableServerLogsSavedForFailingActionWithSiblingRepositoryLayout() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(new SiblingRepositoryLayoutResolver(execRoot)); + RemoteExecutionService service = runner.getRemoteExecutionService(); Digest logDigest = digestUtil.computeAsUtf8("bla"); Path logPath = logDir @@ -632,6 +639,7 @@ public void testHumanReadableServerLogsSavedForFailingActionWithSiblingRepositor @Test public void testHumanReadableServerLogsSavedForFailingActionWithStatus() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); Digest logDigest = digestUtil.computeAsUtf8("bla"); Path logPath = logDir.getRelative(simpleActionId).getRelative("logname"); com.google.rpc.Status timeoutStatus = @@ -671,6 +679,7 @@ public void testHumanReadableServerLogsSavedForFailingActionWithStatus() throws public void testNonHumanReadableServerLogsNotSaved() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); Digest logDigest = digestUtil.computeAsUtf8("bla"); ActionResult result = ActionResult.newBuilder().setExitCode(31).build(); @@ -707,6 +716,7 @@ public void testNonHumanReadableServerLogsNotSaved() throws Exception { @Test public void testServerLogsNotSavedForSuccessfulAction() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); Digest logDigest = digestUtil.computeAsUtf8("bla"); ActionResult result = ActionResult.newBuilder().setExitCode(0).build(); @@ -745,6 +755,7 @@ public void cacheDownloadFailureTriggersRemoteExecution() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); when(cache.downloadActionResult( @@ -793,6 +804,7 @@ public void resultsDownloadFailureTriggersRemoteExecutionWithSkipCacheLookup() t // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); when(cache.downloadActionResult( any(RemoteActionExecutionContext.class), @@ -850,6 +862,7 @@ public void testRemoteExecutionTimeout() throws Exception { remoteOptions.remoteLocalFallback = false; RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); when(cache.downloadActionResult( @@ -894,6 +907,7 @@ public void testRemoteExecutionTimeoutDoesNotTriggerFallback() throws Exception remoteOptions.remoteLocalFallback = true; RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ActionResult cachedResult = ActionResult.newBuilder().setExitCode(0).build(); when(cache.downloadActionResult( @@ -936,6 +950,7 @@ public void testRemoteExecutionCommandFailureDoesNotTriggerFallback() throws Exc remoteOptions.remoteLocalFallback = true; RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); when(cache.downloadActionResult( any(RemoteActionExecutionContext.class), @@ -1066,6 +1081,8 @@ private void testParamFilesAreMaterializedForFlag(String flag) throws Exception executionOptions.materializeParamFiles = true; RemoteExecutionService remoteExecutionService = new RemoteExecutionService( + /*reporter=*/ null, + /*verboseFailures=*/ false, execRoot, RemotePathResolver.createDefault(execRoot), "build-req-id", @@ -1132,6 +1149,7 @@ public void testDownloadMinimalOnCacheHit() throws Exception { RemoteActionResult actionResult = RemoteActionResult.createFromCache(succeededAction); RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); doReturn(actionResult).when(service).lookupCache(any()); Spawn spawn = newSimpleSpawn(); @@ -1160,6 +1178,7 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { .thenReturn(succeeded); RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); Spawn spawn = newSimpleSpawn(); FakeSpawnExecutionContext policy = getSpawnContext(spawn); @@ -1186,6 +1205,7 @@ public void testDownloadMinimalIoError() throws Exception { IOException downloadFailure = new IOException("downloadMinimal failed"); RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); doReturn(cachedActionResult).when(service).lookupCache(any()); doThrow(downloadFailure).when(service).downloadOutputs(any(), eq(cachedActionResult)); @@ -1215,6 +1235,7 @@ public void testDownloadTopLevel() throws Exception { RemoteActionResult cachedActionResult = RemoteActionResult.createFromCache(succeededAction); RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput)); + RemoteExecutionService service = runner.getRemoteExecutionService(); doReturn(cachedActionResult).when(service).lookupCache(any()); Spawn spawn = newSimpleSpawn(topLevelOutput); @@ -1278,6 +1299,7 @@ public void accountingAddsDurationsForStages() { public void shouldReportCheckingCacheBeforeScheduling() throws Exception { // Arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() .setResult(ActionResult.newBuilder().setExitCode(0).build()) @@ -1320,6 +1342,7 @@ public void shouldReportCheckingCacheBeforeScheduling() throws Exception { public void shouldReportExecutingStatusWithoutMetadata() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() .setResult(ActionResult.newBuilder().setExitCode(0).build()) @@ -1362,6 +1385,7 @@ public void shouldReportExecutingStatusWithoutMetadata() throws Exception { public void shouldReportExecutingStatusAfterGotExecutingStageFromMetadata() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() .setResult(ActionResult.newBuilder().setExitCode(0).build()) @@ -1421,6 +1445,7 @@ public void shouldReportExecutingStatusAfterGotExecutingStageFromMetadata() thro public void shouldIgnoreInvalidMetadata() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() .setResult(ActionResult.newBuilder().setExitCode(0).build()) @@ -1468,6 +1493,7 @@ public void shouldIgnoreInvalidMetadata() throws Exception { public void shouldReportExecutingStatusIfNoExecutingStatusFromMetadata() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() .setResult(ActionResult.newBuilder().setExitCode(0).build()) @@ -1516,6 +1542,7 @@ public void shouldReportExecutingStatusIfNoExecutingStatusFromMetadata() throws public void shouldReportExecutingStatusEvenNoOperationFromServer() throws Exception { // arrange RemoteSpawnRunner runner = newSpawnRunner(); + RemoteExecutionService service = runner.getRemoteExecutionService(); ExecuteResponse succeeded = ExecuteResponse.newBuilder() .setResult(ActionResult.newBuilder().setExitCode(0).build()) @@ -1605,9 +1632,11 @@ private RemoteSpawnRunner newSpawnRunner( @Nullable Reporter reporter, ImmutableSet topLevelOutputs, RemotePathResolver remotePathResolver) { - service = + RemoteExecutionService service = spy( new RemoteExecutionService( + reporter, + verboseFailures, execRoot, remotePathResolver, "build-req-id", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index f9b58ec2948ca7..08ef24bd6ac2c5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -53,6 +53,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; +import com.google.common.eventbus.EventBus; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInput; @@ -67,6 +68,7 @@ import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.RemoteRetrier.ExponentialBackoff; @@ -294,10 +296,13 @@ public int maxConcurrency() { retrier, DIGEST_UTIL, uploader); + Reporter reporter = new Reporter(new EventBus()); RemoteExecutionCache remoteCache = - new RemoteExecutionCache(cacheProtocol, remoteOptions, DIGEST_UTIL); + new RemoteExecutionCache(reporter, cacheProtocol, remoteOptions, DIGEST_UTIL); RemoteExecutionService remoteExecutionService = new RemoteExecutionService( + reporter, + /*verboseFailures=*/true, execRoot, RemotePathResolver.createDefault(execRoot), "build-req-id", diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java new file mode 100644 index 00000000000000..ee209227a41d02 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java @@ -0,0 +1,520 @@ +package com.google.devtools.build.lib.remote; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +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.DirectoryNode; +import build.bazel.remote.execution.v2.FileNode; +import build.bazel.remote.execution.v2.SymlinkNode; +import build.bazel.remote.execution.v2.Tree; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.remote.common.RemotePathResolver; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import java.io.IOException; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link UploadManifest}. */ +@RunWith(JUnit4.class) +public class UploadManifestTest { + private final DigestUtil digestUtil = new DigestUtil(DigestHashFunction.SHA256); + + private Path execRoot; + private RemotePathResolver remotePathResolver; + + @Before + public final void setUp() throws Exception { + FileSystem fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); + execRoot = fs.getPath("/execroot"); + execRoot.createDirectoryAndParents(); + + remotePathResolver = new RemotePathResolver.DefaultRemotePathResolver(execRoot); + } + + @Test + public void actionResult_uploadSymlinks_absoluteFileSymlinkAsFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path link = execRoot.getRelative("link"); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + link.createSymbolicLink(target); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(link)); + Digest digest = digestUtil.compute(target); + assertThat(um.getDigestToFile()).containsExactly(digest, link); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_absoluteDirectorySymlinkAsDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path foo = execRoot.getRelative("dir/foo"); + FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("link"); + link.createSymbolicLink(dir); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(link)); + Digest digest = digestUtil.compute(foo); + assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo")); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_noUploadSymlinks_relativeFileSymlinkAsFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path link = execRoot.getRelative("link"); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + link.createSymbolicLink(target.relativeTo(execRoot)); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ false, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(link)); + Digest digest = digestUtil.compute(target); + assertThat(um.getDigestToFile()).containsExactly(digest, link); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_noUploadSymlinks_relativeDirectorySymlinkAsDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path foo = execRoot.getRelative("dir/foo"); + FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("link"); + link.createSymbolicLink(dir.relativeTo(execRoot)); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ false, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(link)); + Digest digest = digestUtil.compute(foo); + assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo")); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + + @Test + public void actionResult_uploadSymlinks_relativeFileSymlinkAsSymlink() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path link = execRoot.getRelative("link"); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + link.createSymbolicLink(target.relativeTo(execRoot)); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(link)); + assertThat(um.getDigestToFile()).isEmpty(); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputFileSymlinksBuilder().setPath("link").setTarget("target"); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_relativeDirectorySymlinkAsSymlink() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path file = execRoot.getRelative("dir/foo"); + FileSystemUtils.writeContent(file, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("link"); + link.createSymbolicLink(dir.relativeTo(execRoot)); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(link)); + assertThat(um.getDigestToFile()).isEmpty(); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectorySymlinksBuilder().setPath("link").setTarget("dir"); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_danglingSymlinkError() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path link = execRoot.getRelative("link"); + Path target = execRoot.getRelative("target"); + link.createSymbolicLink(target.relativeTo(execRoot)); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + IOException e = assertThrows(IOException.class, () -> um.addFiles(ImmutableList.of(link))); + assertThat(e).hasMessageThat().contains("dangling"); + assertThat(e).hasMessageThat().contains("/execroot/link"); + assertThat(e).hasMessageThat().contains("target"); + } + + @Test + public void actionResult_noAllowSymlinks_symlinksCauseError() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path link = execRoot.getRelative("link"); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + link.createSymbolicLink(target.relativeTo(execRoot)); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ false); + ExecException e = assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(link))); + assertThat(e).hasMessageThat().contains("symbolic link"); + assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload"); + } + + @Test + public void actionResult_uploadSymlinks_absoluteFileSymlinkInDirectoryAsFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(target); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(dir)); + Digest digest = digestUtil.compute(target); + assertThat(um.getDigestToFile()).containsExactly(digest, link); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("link").setDigest(digest))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_absoluteDirectorySymlinkInDirectoryAsDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path bardir = execRoot.getRelative("bardir"); + bardir.createDirectory(); + Path foo = execRoot.getRelative("bardir/foo"); + FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(bardir); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(dir)); + Digest digest = digestUtil.compute(foo); + assertThat(um.getDigestToFile()) + .containsExactly(digest, execRoot.getRelative("dir/link/foo")); + + Directory barDir = + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)) + .build(); + Digest barDigest = digestUtil.compute(barDir); + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addDirectories( + DirectoryNode.newBuilder().setName("link").setDigest(barDigest))) + .addChildren(barDir) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_noUploadSymlinks_relativeFileSymlinkInDirectoryAsFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(PathFragment.create("../target")); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ false, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(dir)); + Digest digest = digestUtil.compute(target); + assertThat(um.getDigestToFile()).containsExactly(digest, link); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("link").setDigest(digest))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_noUploadSymlinks_relativeDirectorySymlinkInDirectoryAsDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path bardir = execRoot.getRelative("bardir"); + bardir.createDirectory(); + Path foo = execRoot.getRelative("bardir/foo"); + FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(PathFragment.create("../bardir")); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ false, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(dir)); + Digest digest = digestUtil.compute(foo); + assertThat(um.getDigestToFile()) + .containsExactly(digest, execRoot.getRelative("dir/link/foo")); + + Directory barDir = + Directory.newBuilder() + .addFiles(FileNode.newBuilder().setName("foo").setDigest(digest)) + .build(); + Digest barDigest = digestUtil.compute(barDir); + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addDirectories( + DirectoryNode.newBuilder().setName("link").setDigest(barDigest))) + .addChildren(barDir) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_relativeFileSymlinkInDirectoryAsSymlink() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(PathFragment.create("../target")); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(dir)); + assertThat(um.getDigestToFile()).isEmpty(); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../target"))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_relativeDirectorySymlinkInDirectoryAsSymlink() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path bardir = execRoot.getRelative("bardir"); + bardir.createDirectory(); + Path foo = execRoot.getRelative("bardir/foo"); + FileSystemUtils.writeContent(foo, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(PathFragment.create("../bardir")); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + um.addFiles(ImmutableList.of(dir)); + assertThat(um.getDigestToFile()).isEmpty(); + + Tree tree = + Tree.newBuilder() + .setRoot( + Directory.newBuilder() + .addSymlinks(SymlinkNode.newBuilder().setName("link").setTarget("../bardir"))) + .build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + + @Test + public void actionResult_uploadSymlinks_danglingSymlinkInDirectoryError() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path target = execRoot.getRelative("target"); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(target); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ true); + IOException e = assertThrows(IOException.class, () -> um.addFiles(ImmutableList.of(dir))); + assertThat(e).hasMessageThat().contains("dangling"); + assertThat(e).hasMessageThat().contains("/execroot/dir/link"); + assertThat(e).hasMessageThat().contains("/execroot/target"); + } + + @Test + public void actionResult_noAllowSymlinks_symlinkInDirectoryError() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + Path target = execRoot.getRelative("target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + Path link = execRoot.getRelative("dir/link"); + link.createSymbolicLink(target); + + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*uploadSymlinks=*/ true, + /*allowSymlinks=*/ false); + ExecException e = assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(dir))); + assertThat(e).hasMessageThat().contains("symbolic link"); + assertThat(e).hasMessageThat().contains("dir/link"); + assertThat(e).hasMessageThat().contains("--remote_allow_symlink_upload"); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/AsyncTaskCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/util/AsyncTaskCacheTest.java index 279c342baa7f94..9fd2d0234ff27e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/AsyncTaskCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/AsyncTaskCacheTest.java @@ -22,6 +22,7 @@ import io.reactivex.rxjava3.observers.TestObserver; import java.io.IOException; import java.util.Random; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -386,4 +387,70 @@ public void execute_executeWithFutureAndCancelLoop_noErrors() throws Throwable { throw error.get(); } } + + @Test + public void execute_pendingShutdown_getCancellationError() { + AsyncTaskCache cache = AsyncTaskCache.create(); + cache.executeIfNot("key1", Single.create(emitter -> { + // never complete + })).test().assertNotComplete(); + cache.shutdown(); + cache.awaitTermination().test().assertNotComplete(); + + TestObserver ob = cache.executeIfNot("key2", Single.just("value2")).test(); + + ob.assertError(e -> e instanceof CancellationException); + } + + @Test + public void execute_afterShutdown_getCancellationError() { + AsyncTaskCache cache = AsyncTaskCache.create(); + cache.shutdown(); + cache.awaitTermination().blockingAwait(); + + TestObserver ob = cache.executeIfNot("key", Single.just("value")).test(); + + ob.assertError(e -> e instanceof CancellationException); + } + + @Test + public void shutdownNow_cancelInProgressTasks() { + AsyncTaskCache cache = AsyncTaskCache.create(); + TestObserver ob = cache.executeIfNot("key", Single.create(emitter -> { + // never complete + })).test(); + cache.shutdown(); + cache.awaitTermination().test().assertNotComplete(); + ob.assertNotComplete(); + + cache.shutdownNow(); + + ob.assertError(e -> e instanceof CancellationException); + cache.awaitTermination().test().assertComplete(); + } + + @Test + public void awaitTermination_pendingShutdown_completeAfterTaskFinished() { + AsyncTaskCache cache = AsyncTaskCache.create(); + AtomicReference> emitterRef = new AtomicReference<>(null); + cache.executeIfNot("key", Single.create(emitterRef::set)).test().assertNotComplete(); + assertThat(emitterRef.get()).isNotNull(); + cache.shutdown(); + TestObserver ob = cache.awaitTermination().test(); + ob.assertNotComplete(); + + emitterRef.get().onSuccess("value"); + + ob.assertComplete(); + } + + @Test + public void awaitTermination_afterShutdown_complete() { + AsyncTaskCache cache = AsyncTaskCache.create(); + cache.shutdownNow(); + + TestObserver ob = cache.awaitTermination().test(); + + ob.assertComplete(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java b/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java index b5ce3ee7b1946f..9c6bb36928d5db 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/InMemoryCacheClient.java @@ -101,9 +101,10 @@ public ListenableFuture downloadActionResult( } @Override - public void uploadActionResult( + public ListenableFuture uploadActionResult( RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) { ac.put(actionKey, actionResult); + return Futures.immediateFuture(null); } @Override 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 a354c12b6e506c..ac718a9e6e1cad 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 @@ -20,6 +20,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", + "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/disk", 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 1c675bb2038de4..c0fc99a2103270 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 @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.remote.common.RemotePathResolver; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.remote.UploadManifest; import com.google.devtools.build.lib.shell.AbnormalTerminationException; import com.google.devtools.build.lib.shell.CommandException; import com.google.devtools.build.lib.shell.CommandResult; @@ -344,9 +345,10 @@ private ActionResult execute( ActionResult result = null; try { - result = - cache.upload( - context, + UploadManifest manifest = + UploadManifest.create( + cache.getRemoteOptions(), + digestUtil, RemotePathResolver.createDefault(workingDirectory), actionKey, action, @@ -354,6 +356,10 @@ private ActionResult execute( outputs, outErr, exitCode); + manifest.awaitUploadOutputs(context, cache); + ActionResult actionResult = manifest.getActionResult(); + getFromFuture(cache.uploadActionResult(context, actionKey, actionResult)); + result = actionResult; } catch (ExecException e) { if (errStatus == null) { errStatus = 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 d8d423b8ffa19e..b7479f57f340ba 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 @@ -13,14 +13,14 @@ // limitations under the License. package com.google.devtools.build.remote.worker; -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.DirectoryNode; import build.bazel.remote.execution.v2.FileNode; +import com.google.common.eventbus.EventBus; +import com.google.devtools.build.lib.events.Reporter; 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; import com.google.devtools.build.lib.remote.disk.DiskCacheClient; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; @@ -33,6 +33,7 @@ class OnDiskBlobStoreCache extends RemoteCache { public OnDiskBlobStoreCache(RemoteOptions options, Path cacheDir, DigestUtil digestUtil) { super( + new Reporter(new EventBus()), new DiskCacheClient(cacheDir, /* verifyDownloads= */ true, digestUtil), options, digestUtil); @@ -59,13 +60,11 @@ public void downloadTree( } } - public void uploadActionResult( - RemoteActionExecutionContext context, ActionKey actionKey, ActionResult actionResult) - throws IOException, InterruptedException { - cacheProtocol.uploadActionResult(context, actionKey, actionResult); - } - public DigestUtil getDigestUtil() { return digestUtil; } + + public RemoteOptions getRemoteOptions() { + return options; + } }