diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 6a0faef31bebef..607e7171ea6c58 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -43,6 +43,7 @@ filegroup( "//src/main/java/com/google/devtools/build/lib/remote/blobstore:srcs", "//src/main/java/com/google/devtools/build/lib/remote/blobstore/http:srcs", "//src/main/java/com/google/devtools/build/lib/remote/logging:srcs", + "//src/main/java/com/google/devtools/build/lib/remote/options:srcs", "//src/main/java/com/google/devtools/build/lib/remote/util:srcs", "//src/main/java/com/google/devtools/build/lib/rules/apple/cpp:srcs", "//src/main/java/com/google/devtools/build/lib/rules/apple:srcs", @@ -668,6 +669,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/profiler/memory:current_rule_tracker", "//src/main/java/com/google/devtools/build/lib/query2:aquery-utils", + "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/rules/cpp:cpp_interface", "//src/main/java/com/google/devtools/build/lib/shell", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index fe4bf4221eb0e1..582fba32335edf 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -32,6 +32,7 @@ import com.google.devtools.common.options.OptionsProvider; import java.io.Closeable; import java.io.IOException; +import java.util.Collection; import java.util.Map; import javax.annotation.Nullable; @@ -72,6 +73,8 @@ public enum ShowSubcommands { private final ArtifactPathResolver pathResolver; + private final ImmutableList requiredLocalOutputs; + private ActionExecutionContext( Executor executor, MetadataProvider actionInputFileCache, @@ -82,6 +85,7 @@ private ActionExecutionContext( ExtendedEventHandler eventHandler, Map clientEnv, ImmutableMap> topLevelFilesets, + ImmutableList requiredLocalOutputs, @Nullable ArtifactExpander artifactExpander, @Nullable Environment env, @Nullable FileSystem actionFileSystem, @@ -102,6 +106,7 @@ private ActionExecutionContext( this.pathResolver = ArtifactPathResolver.createPathResolver(actionFileSystem, // executor is only ever null in testing. executor == null ? null : executor.getExecRoot()); + this.requiredLocalOutputs = Preconditions.checkNotNull(requiredLocalOutputs); } public ActionExecutionContext( @@ -114,6 +119,7 @@ public ActionExecutionContext( ExtendedEventHandler eventHandler, Map clientEnv, ImmutableMap> topLevelFilesets, + ImmutableList requiredLocalOutputs, ArtifactExpander artifactExpander, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { @@ -127,6 +133,7 @@ public ActionExecutionContext( eventHandler, clientEnv, topLevelFilesets, + requiredLocalOutputs, artifactExpander, /*env=*/ null, actionFileSystem, @@ -154,6 +161,7 @@ public static ActionExecutionContext forInputDiscovery( eventHandler, clientEnv, ImmutableMap.of(), + /*requiredLocalOutputs=*/ ImmutableList.of(), /*artifactExpander=*/ null, env, actionFileSystem, @@ -307,6 +315,18 @@ public ActionKeyContext getActionKeyContext() { return actionKeyContext; } + /** + * Returns the collection of files that this command must write and make available via + * the local {@link com.google.devtools.build.lib.vfs.FileSystem}. The returned output + * artifacts are a subset of the action's output artifacts. + * + *

This is for use with remote execution, where as an optimization we don't want to + * download all output files. + */ + public Collection getRequiredLocalOutputs() { + return requiredLocalOutputs; + } + @Override public void close() throws IOException { fileOutErr.close(); @@ -327,6 +347,29 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) { eventHandler, clientEnv, topLevelFilesets, + requiredLocalOutputs, + artifactExpander, + env, + actionFileSystem, + skyframeDepsResult); + } + + /** + * Allows us to create a new context that provides a list of output artifacts that need to + * be staged on the local filesystem. + */ + public ActionExecutionContext withRequiredLocalOutputs(Collection requiredLocalOutputs) { + return new ActionExecutionContext( + executor, + actionInputFileCache, + actionInputPrefetcher, + actionKeyContext, + metadataHandler, + fileOutErr, + eventHandler, + clientEnv, + topLevelFilesets, + ImmutableList.copyOf(requiredLocalOutputs), artifactExpander, env, actionFileSystem, diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java index 92c6325c212bac..be64ef5e162715 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputPrefetcher.java @@ -13,12 +13,15 @@ // limitations under the License. package com.google.devtools.build.lib.actions; +import java.io.IOException; + /** Prefetches files to local disk. */ public interface ActionInputPrefetcher { public static final ActionInputPrefetcher NONE = new ActionInputPrefetcher() { @Override - public void prefetchFiles(Iterable input) { + public void prefetchFiles(Iterable inputs, + MetadataProvider metadataProvider) { // Do nothing. } }; @@ -28,5 +31,5 @@ public void prefetchFiles(Iterable input) { * *

For any path not under this prefetcher's control, the call should be a no-op. */ - void prefetchFiles(Iterable input); + void prefetchFiles(Iterable inputs, MetadataProvider metadataProvider) throws IOException, InterruptedException; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java index 91b98c7c3aa1eb..567059d326de21 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java @@ -25,7 +25,9 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.vfs.OsPathPolicy; import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; @@ -382,4 +384,16 @@ public ImmutableList getActions() { return actions; } } + + public static void prefetchInputs(Iterable inputs, + ActionExecutionContext actionExecutionContext, Action action) throws ActionExecutionException, + InterruptedException { + try { + actionExecutionContext + .getActionInputPrefetcher() + .prefetchFiles(inputs, actionExecutionContext.getMetadataProvider()); + } catch (IOException e) { + throw new ActionExecutionException("Failed to (pre)fetch remote input files.", e, action, false); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactRoot.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactRoot.java index ad3399f64c1833..817a1583fb123f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactRoot.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactRoot.java @@ -125,6 +125,10 @@ public boolean isSourceRoot() { return rootType == RootType.Source; } + public boolean isOutputRoot() { + return rootType == RootType.Output; + } + boolean isMiddlemanRoot() { return rootType == RootType.Middleman; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index 6b063d608ca0fa..7f999efc64f0b3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -51,6 +51,8 @@ *

  • a "middleman marker" object, which has a null digest, 0 size, and mtime of 0. *
  • The "self data" of a TreeArtifact, where we would expect to see a digest representing the * artifact's contents, and a size of 0. + *
  • a file that's only stored by a remote caching/execution system, in which case we would + * expect to see a digest and size. * */ @Immutable @@ -138,6 +140,13 @@ public final boolean isMarkerValue() { */ public abstract boolean wasModifiedSinceDigest(Path path) throws IOException; + /** + * Returns {@code true} if the file only exists remotely. + */ + public boolean isRemote() { + return false; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -492,7 +501,12 @@ public int getLocationIndex() { @Override public boolean wasModifiedSinceDigest(Path path) { - throw new UnsupportedOperationException(); + return false; + } + + @Override + public boolean isRemote() { + return true; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java index fc61397318f283..76a8bcd897e818 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java @@ -31,7 +31,7 @@ *

    Note that implementations of this interface call chmod on output files if {@link * #discardOutputMetadata} has been called. */ -public interface MetadataHandler extends MetadataProvider { +public interface MetadataHandler extends MetadataProvider, MetadataInjector { @Override FileArtifactValue getMetadata(ActionInput actionInput) throws IOException; @@ -56,9 +56,6 @@ public interface MetadataHandler extends MetadataProvider { */ void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest); - /** Injects a file that is only stored remotely. */ - void injectRemoteFile(Artifact output, byte[] digest, long size, int locationIndex); - /** * Marks an artifact as intentionally omitted. Acknowledges that this Artifact could have existed, * but was intentionally not saved, most likely as an optimization. diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java new file mode 100644 index 00000000000000..370a6903cce252 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java @@ -0,0 +1,49 @@ +// Copyright 2018 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.cache; + +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Map; + +/** + * Allows to inject metadata of action outputs into skyframe. + */ +public interface MetadataInjector { + + /** + * Injects metadata of a file that is stored remotely. + * + * @param file a regular output file. + * @param digest the digest of the file. + * @param sizeBytes the size of the file in bytes. + * @param locationIndex is only used in Blaze. + */ + default void injectRemoteFile(Artifact file, byte[] digest, long sizeBytes, int locationIndex) { + throw new UnsupportedOperationException(); + } + + /** + * Inject the metadata of a tree artifact whose contents are stored remotely. + * + * @param treeArtifact an output directory. + * @param children the metadata of the files stored in the directory. + */ + default void injectRemoteDirectory(SpecialArtifact treeArtifact, + Map children) { + throw new UnsupportedOperationException(); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java index 3568dc8f944952..8787507ce1753e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/AbstractFileWriteAction.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; @@ -28,6 +29,7 @@ import com.google.protobuf.ByteString; import java.io.IOException; import java.io.OutputStream; +import java.util.Collections; /** * Abstract Action to write to a file. @@ -59,6 +61,15 @@ public boolean makeExecutable() { @Override public final ActionResult execute(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { + Actions.prefetchInputs(getInputs(), actionExecutionContext, this); + try { + actionExecutionContext + .getActionInputPrefetcher() + .prefetchFiles(getInputs(), actionExecutionContext.getMetadataProvider()); + } catch (IOException e) { + throw new ActionExecutionException("Failed to fetch remote input file.", e, this, false); + } + ActionResult actionResult; try { DeterministicWriter deterministicWriter; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java index b630313b76d372..3300a3a0e54b98 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; @@ -30,6 +31,8 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; import javax.annotation.Nullable; /** Action to create a symbolic link. */ @@ -162,7 +165,9 @@ public Path getOutputPath(ActionExecutionContext actionExecutionContext) { @Override public ActionResult execute(ActionExecutionContext actionExecutionContext) - throws ActionExecutionException { + throws ActionExecutionException, InterruptedException { + Actions.prefetchInputs(Collections.singletonList(getPrimaryInput()), actionExecutionContext, + this); maybeVerifyTargetIsExecutable(actionExecutionContext); Path srcPath; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java index 137b6abd44097c..0eb2a4491307f5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; +import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ExecException; @@ -34,6 +35,7 @@ import com.google.devtools.build.lib.util.Fingerprint; import java.io.IOException; import java.util.Collection; +import java.util.Collections; import java.util.List; /** Action to expand a template and write the expanded content to a file. */ @@ -127,6 +129,7 @@ public String getSkylarkContent() throws IOException { @Override public final ActionResult execute(ActionExecutionContext actionExecutionContext) throws ActionExecutionException, InterruptedException { + Actions.prefetchInputs(getInputs(), actionExecutionContext, this); TemplateExpansionContext expansionContext = actionExecutionContext.getContext(TemplateExpansionContext.class); try { diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index e95413b00f4e58..e32323c6378c43 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionStatusMessage; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.EnvironmentalExecException; @@ -32,6 +33,7 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.Spawns; +import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.exec.SpawnCache.CacheHandle; @@ -43,6 +45,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.time.Duration; +import java.util.Collection; import java.util.List; import java.util.SortedMap; import java.util.concurrent.atomic.AtomicInteger; @@ -181,11 +184,11 @@ public int getId() { } @Override - public void prefetchInputs() throws IOException { + public void prefetchInputs() throws IOException, InterruptedException { if (Spawns.shouldPrefetchInputsForLocalExecution(spawn)) { actionExecutionContext .getActionInputPrefetcher() - .prefetchFiles(getInputMapping(true).values()); + .prefetchFiles(getInputMapping(true).values(), getMetadataProvider()); } } @@ -194,6 +197,11 @@ public MetadataProvider getMetadataProvider() { return actionExecutionContext.getMetadataProvider(); } + @Override + public MetadataHandler getMetadataInjector() { + return actionExecutionContext.getMetadataHandler(); + } + @Override public ArtifactExpander getArtifactExpander() { return actionExecutionContext.getArtifactExpander(); @@ -271,5 +279,10 @@ public void report(ProgressStatus state, String name) { break; } } + + @Override + public Collection getRequiredLocalOutputs() { + return actionExecutionContext.getRequiredLocalOutputs(); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java index 0d70563e407ebc..3b9536177e17d2 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnRunner.java @@ -13,7 +13,9 @@ // limitations under the License. package com.google.devtools.build.lib.exec; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ExecException; @@ -21,10 +23,12 @@ import com.google.devtools.build.lib.actions.MetadataProvider; 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.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.time.Duration; +import java.util.Collection; import java.util.SortedMap; /** @@ -129,6 +133,26 @@ public enum ProgressStatus { * by different threads, so they MUST not call any shared non-thread-safe objects. */ interface SpawnExecutionContext { + + /** + * Returns a {@link MetadataInjector} that allows a caller to inject metadata about spawn + * outputs that are stored remotely. + */ + default MetadataInjector getMetadataInjector() { + throw new UnsupportedOperationException(); + } + + /** + * Returns the collection of files that this command must write and make available via + * the local {@link com.google.devtools.build.lib.vfs.FileSystem}. The returned output + * artifacts are a subset of the action's output artifacts. + * + *

    This is for use with remote execution, where as an optimization we don't want to + * download all output files. + */ + default Collection getRequiredLocalOutputs() { + return ImmutableList.of(); + } /** * Returns a unique id for this spawn, to be used for logging. Note that a single spawn may be * passed to multiple {@link SpawnRunner} implementations, so any log entries should also @@ -137,25 +161,15 @@ interface SpawnExecutionContext { int getId(); /** - * Prefetches the Spawns input files to the local machine. There are cases where Bazel runs on a - * network file system, and prefetching the files in parallel is a significant performance win. - * This should only be called by local strategies when local execution is imminent. - * - *

    Should be called with the equivalent of: - * - * policy.prefetchInputs( - * Iterables.filter(policy.getInputMapping().values(), Predicates.notNull())); - * + * Prefetches the {@link Spawn}'s input files to the local machine. * - *

    Note in particular that {@link #getInputMapping} may return {@code null} values, but - * this method does not accept {@code null} values. + *

    This is used by remote caching / execution to only download remote build outputs to the + * local machine that are strictly required. It's also used as a performance optimization in + * cases when Bazel runs on a network filesystem. * - *

    The reason why this method requires passing in the inputs is that getInputMapping may be - * slow to compute, so if the implementation already called it, we don't want to compute it - * again. I suppose we could require implementations to memoize getInputMapping (but not compute - * it eagerly), and that may change in the future. + *

    This should only be called by local strategies when local execution is imminent. */ - void prefetchInputs() throws IOException; + void prefetchInputs() throws IOException, InterruptedException; /** * The input file metadata cache for this specific spawn, which can be used to efficiently diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java index dc8711c7e65017..53bdf200f13d4f 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.ResourceSet; diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index fe5d9373057d60..37d0373eced2d7 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +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; @@ -27,42 +29,62 @@ import build.bazel.remote.execution.v2.Tree; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +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.hash.HashCode; 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.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.UserExecException; +import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.concurrent.ThreadSafety; +import com.google.devtools.build.lib.profiler.Profiler; +import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.ActionOutputMetadata.DirectoryMetadata; +import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.ActionOutputMetadata.FileMetadata; +import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.ActionOutputMetadata.SymlinkMetadata; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.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.InvalidProtocolBufferException; import io.grpc.Context; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Map.Entry; +import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.annotation.Nullable; /** A cache for storing artifacts (input and output) as well as the output of running an action. */ @ThreadSafety.ThreadSafe -public abstract class AbstractRemoteActionCache implements AutoCloseable { +abstract class AbstractRemoteActionCache implements AutoCloseable { private static final ListenableFuture COMPLETED_SUCCESS = SettableFuture.create(); private static final ListenableFuture EMPTY_BYTES = SettableFuture.create(); @@ -164,10 +186,210 @@ public void onFailure(Throwable t) { outerF.setException(t); } }, - MoreExecutors.directExecutor()); + directExecutor()); return outerF; } + /** Value class for action output metadata. */ + static class ActionOutputMetadata { + + public static class SymlinkMetadata { + private final Path path; + private final PathFragment target; + + public SymlinkMetadata(Path path, PathFragment target) { + this.path = path; + this.target = target; + } + + public Path path() { + return path; + } + + public PathFragment target() { + return target; + } + } + + public static class FileMetadata { + private final Path path; + private final byte[] digest; + private final long sizeBytes; + private final boolean isExecutable; + + public FileMetadata(Path path, byte[] digest, long sizeBytes, boolean isExecutable) { + this.path = path; + this.digest = digest; + this.sizeBytes = sizeBytes; + this.isExecutable = isExecutable; + } + + public Path path() { + return path; + } + + public byte[] digest() { + return digest; + } + + public long sizeBytes() { + return sizeBytes; + } + + public boolean isExecutable() { + return isExecutable; + } + } + + public static class DirectoryMetadata { + private final ImmutableList files; + private final ImmutableList symlinks; + + public DirectoryMetadata( + ImmutableList files, ImmutableList symlinks) { + this.files = files; + this.symlinks = symlinks; + } + + public ImmutableList files() { + return files; + } + + public ImmutableList symlinks() { + return symlinks; + } + } + + private final ImmutableMap files; + private final ImmutableMap symlinks; + private final ImmutableMap directories; + + public ActionOutputMetadata( + ImmutableMap files, + ImmutableMap symlinks, + ImmutableMap directories) { + this.files = files; + this.symlinks = symlinks; + this.directories = directories; + } + + @Nullable + public FileMetadata file(Path path) { + return files.get(path); + } + + @Nullable + public DirectoryMetadata directory(Path path) { + return directories.get(path); + } + + public Collection files() { + return files.values(); + } + + public ImmutableSet> directories() { + return directories.entrySet(); + } + + public Collection symlinks() { + return symlinks.values(); + } + } + + public DirectoryMetadata parseDirectory( + Path parent, Directory dir, Map childDirectoriesMap) throws IOException { + ImmutableList.Builder filesBuilder = ImmutableList.builder(); + for (FileNode file : dir.getFilesList()) { + byte[] digest = HashCode.fromString(file.getDigest().getHash()).asBytes(); + filesBuilder.add( + new FileMetadata( + parent.getRelative(file.getName()), + digest, + file.getDigest().getSizeBytes(), + file.getIsExecutable())); + } + + ImmutableList.Builder symlinksBuilder = ImmutableList.builder(); + for (SymlinkNode symlink : dir.getSymlinksList()) { + symlinksBuilder.add( + new SymlinkMetadata( + parent.getRelative(symlink.getName()), PathFragment.create(symlink.getTarget()))); + } + + for (DirectoryNode directoryNode : dir.getDirectoriesList()) { + Path childPath = parent.getRelative(directoryNode.getName()); + Directory childDir = + Preconditions.checkNotNull(childDirectoriesMap.get(directoryNode.getDigest())); + DirectoryMetadata childMetadata = + parseDirectory(childPath, childDir, childDirectoriesMap); + filesBuilder.addAll(childMetadata.files()); + symlinksBuilder.addAll(childMetadata.symlinks()); + } + + return new DirectoryMetadata(filesBuilder.build(), symlinksBuilder.build()); + } + + private ActionOutputMetadata parseActionResultMetadata(ActionResult actionResult, Path execRoot) + throws IOException, InterruptedException { + Preconditions.checkNotNull(actionResult, "actionResult must not be null"); + Context ctx = Context.current(); + Map> dirMetadataDownloads = + Maps.newHashMapWithExpectedSize(actionResult.getOutputDirectoriesCount()); + for (OutputDirectory dir : actionResult.getOutputDirectoriesList()) { + dirMetadataDownloads.put( + execRoot.getRelative(dir.getPath()), + Futures.transform( + retrier.executeAsync(() -> ctx.call(() -> downloadBlob(dir.getTreeDigest()))), + (treeBytes) -> { + try { + return Tree.parseFrom(treeBytes); + } catch (InvalidProtocolBufferException e) { + throw new RuntimeException(e); + } + }, + directExecutor())); + } + + ImmutableMap.Builder directories = ImmutableMap.builder(); + for (Map.Entry> metadataDownload : + dirMetadataDownloads.entrySet()) { + Path path = metadataDownload.getKey(); + Tree directoryTree = getFromFuture(metadataDownload.getValue()); + Map childrenMap = new HashMap<>(); + for (Directory childDir : directoryTree.getChildrenList()) { + childrenMap.put(digestUtil.compute(childDir), childDir); + } + + directories.put(path, parseDirectory(path, directoryTree.getRoot(), childrenMap)); + } + + ImmutableMap.Builder files = ImmutableMap.builder(); + for (OutputFile outputFile : actionResult.getOutputFilesList()) { + byte[] digest = HashCode.fromString(outputFile.getDigest().getHash()).asBytes(); + files.put( + execRoot.getRelative(outputFile.getPath()), + new FileMetadata( + execRoot.getRelative(outputFile.getPath()), + digest, + outputFile.getDigest().getSizeBytes(), + outputFile.getIsExecutable())); + } + + ImmutableMap.Builder symlinks = ImmutableMap.builder(); + Iterable outputSymlinks = + Iterables.concat( + actionResult.getOutputFileSymlinksList(), + actionResult.getOutputDirectorySymlinksList()); + for (OutputSymlink symlink : outputSymlinks) { + symlinks.put( + execRoot.getRelative(symlink.getPath()), + new SymlinkMetadata( + execRoot.getRelative(symlink.getPath()), PathFragment.create(symlink.getTarget()))); + } + + return new ActionOutputMetadata(files.build(), symlinks.build(), directories.build()); + } + /** * Download the output files and directory trees of a remotely executed action to the local * machine, as well stdin / stdout to the given files. @@ -177,53 +399,23 @@ public void onFailure(Throwable t) { * @throws IOException in case of a cache miss or if the remote cache is unavailable. * @throws ExecException in case clean up after a failed download failed. */ - // TODO(olaola): will need to amend to include the TreeNodeRepository for updating. public void download(ActionResult result, Path execRoot, FileOutErr outErr) throws ExecException, IOException, InterruptedException { Context ctx = Context.current(); - List fileDownloads = - Collections.synchronizedList( - new ArrayList<>(result.getOutputFilesCount() + result.getOutputDirectoriesCount())); - for (OutputFile file : result.getOutputFilesList()) { - Path path = execRoot.getRelative(file.getPath()); - ListenableFuture download = - retrier.executeAsync( - () -> ctx.call(() -> downloadFile(path, file.getDigest()))); - fileDownloads.add(new FuturePathBooleanTuple(download, path, file.getIsExecutable())); - } - - List> dirDownloads = new ArrayList<>(result.getOutputDirectoriesCount()); - for (OutputDirectory dir : result.getOutputDirectoriesList()) { - SettableFuture dirDownload = SettableFuture.create(); - ListenableFuture protoDownload = - retrier.executeAsync(() -> ctx.call(() -> downloadBlob(dir.getTreeDigest()))); - Futures.addCallback( - protoDownload, - new FutureCallback() { - @Override - public void onSuccess(byte[] b) { - try { - Tree tree = Tree.parseFrom(b); - Map childrenMap = new HashMap<>(); - for (Directory child : tree.getChildrenList()) { - childrenMap.put(digestUtil.compute(child), child); - } - Path path = execRoot.getRelative(dir.getPath()); - fileDownloads.addAll(downloadDirectory(path, tree.getRoot(), childrenMap, ctx)); - dirDownload.set(null); - } catch (IOException e) { - dirDownload.setException(e); - } - } - - @Override - public void onFailure(Throwable t) { - dirDownload.setException(t); - } - }, - MoreExecutors.directExecutor()); - dirDownloads.add(dirDownload); - } + ActionOutputMetadata metadata = parseActionResultMetadata(result, execRoot); + + List> downloads = + Stream.concat( + metadata.files().stream(), + metadata.directories().stream().flatMap((entry) -> entry.getValue().files().stream())) + .map( + (file) -> { + Digest digest = DigestUtil.buildDigest(file.digest(), file.sizeBytes()); + ListenableFuture download = + retrier.executeAsync(() -> ctx.call(() -> downloadFile(file.path(), digest))); + return Futures.transform(download, (d) -> file, directExecutor()); + }) + .collect(Collectors.toList()); // Subsequently we need to wait for *every* download to finish, even if we already know that // one failed. That's so that when exiting this method we can be sure that all downloads have @@ -231,33 +423,27 @@ public void onFailure(Throwable t) { // TODO(buchgr): Look into cancellation. IOException downloadException = null; + InterruptedException interruptedException = null; try { - fileDownloads.addAll(downloadOutErr(result, outErr, ctx)); + downloads.addAll(downloadOutErr(result, outErr)); } catch (IOException e) { downloadException = e; } - for (ListenableFuture dirDownload : dirDownloads) { - // Block on all directory download futures, so that we can be sure that we have discovered - // all file downloads and can subsequently safely iterate over the list of file downloads. - try { - getFromFuture(dirDownload); - } catch (IOException e) { - downloadException = downloadException == null ? e : downloadException; - } - } - for (FuturePathBooleanTuple download : fileDownloads) { + for (ListenableFuture download : downloads) { try { - getFromFuture(download.getFuture()); - if (download.getPath() != null) { - download.getPath().setExecutable(download.isExecutable()); + FileMetadata outputFile = getFromFuture(download); + if (outputFile != null) { + outputFile.path().setExecutable(outputFile.isExecutable()); } } catch (IOException e) { downloadException = downloadException == null ? e : downloadException; + } catch (InterruptedException e) { + interruptedException = interruptedException == null ? e : interruptedException; } } - if (downloadException != null) { + if (downloadException != null || interruptedException != null) { try { // Delete any (partially) downloaded output files, since any subsequent local execution // of this action may expect none of the output files to exist. @@ -285,118 +471,46 @@ public void onFailure(Throwable t) { e, true); } - throw downloadException; } - // We create the symbolic links after all regular downloads are finished, because dangling - // links will not work on Windows. - createSymbolicLinks( - execRoot, - Iterables.concat( - result.getOutputFileSymlinksList(), result.getOutputDirectorySymlinksList())); - } - - // Creates a local symbolic link. Only relative symlinks are supported. - private void createSymbolicLink(Path path, String target) throws IOException { - PathFragment targetPath = PathFragment.create(target); - if (targetPath.isAbsolute()) { - // Error, we do not support absolute symlinks as outputs. - throw new IOException( - String.format( - "Action output %s is a symbolic link to an absolute path %s. " - + "Symlinks to absolute paths in action outputs are not supported.", - path, target)); - } - path.createSymbolicLink(targetPath); - } - - // Creates symbolic links locally as created remotely by the action. Only relative symbolic - // links are supported, because absolute symlinks break action hermeticity. - private void createSymbolicLinks(Path execRoot, Iterable symlinks) - throws IOException { - for (OutputSymlink symlink : symlinks) { - Path path = execRoot.getRelative(symlink.getPath()); - Preconditions.checkNotNull( - path.getParentDirectory(), "Failed creating directory and parents for %s", path) - .createDirectoryAndParents(); - createSymbolicLink(path, symlink.getTarget()); + if (interruptedException != null) { + throw interruptedException; } - } - - @VisibleForTesting - protected T getFromFuture(ListenableFuture f) throws IOException, InterruptedException { - return Utils.getFromFuture(f); - } - - /** Tuple of {@code ListenableFuture, Path, boolean}. */ - private static class FuturePathBooleanTuple { - private final ListenableFuture future; - private final Path path; - private final boolean isExecutable; - public FuturePathBooleanTuple(ListenableFuture future, Path path, boolean isExecutable) { - this.future = future; - this.path = path; - this.isExecutable = isExecutable; + if (downloadException != null) { + throw downloadException; } - public ListenableFuture getFuture() { - return future; + List symlinksInDirectories = new ArrayList<>(); + for (Entry entry : metadata.directories()) { + entry.getKey().createDirectoryAndParents(); + symlinksInDirectories.addAll(entry.getValue().symlinks()); } - public Path getPath() { - return path; - } + Iterable symlinks = + Iterables.concat(metadata.symlinks(), symlinksInDirectories); - public boolean isExecutable() { - return isExecutable; - } + // Create the symbolic links after all downloads are finished, because dangling symlinks + // might not be supported on all platforms + createSymlinks(symlinks); } - /** - * Download a directory recursively. The directory is represented by a {@link Directory} protobuf - * message, and the descendant directories are in {@code childrenMap}, accessible through their - * digest. - */ - private List downloadDirectory( - Path path, Directory dir, Map childrenMap, Context ctx) + private void createSymlinks(Iterable symlinks) throws IOException { - // Ensure that the directory is created here even though the directory might be empty - path.createDirectoryAndParents(); - - for (SymlinkNode symlink : dir.getSymlinksList()) { - createSymbolicLink(path.getRelative(symlink.getName()), symlink.getTarget()); - } - - List downloads = new ArrayList<>(dir.getFilesCount()); - for (FileNode child : dir.getFilesList()) { - Path childPath = path.getRelative(child.getName()); - downloads.add( - new FuturePathBooleanTuple( - retrier.executeAsync( - () -> ctx.call(() -> downloadFile(childPath, child.getDigest()))), - childPath, - child.getIsExecutable())); - } - - for (DirectoryNode child : dir.getDirectoriesList()) { - Path childPath = path.getRelative(child.getName()); - Digest childDigest = child.getDigest(); - Directory childDir = childrenMap.get(childDigest); - if (childDir == null) { + for (SymlinkMetadata symlink : symlinks) { + if (symlink.target().isAbsolute()) { + // We do not support absolute symlinks as outputs. throw new IOException( - "could not find subdirectory " - + child.getName() - + " of directory " - + path - + " for download: digest " - + childDigest - + "not found"); + String.format( + "Action output %s is a symbolic link to an absolute path %s. " + + "Symlinks to absolute paths in action outputs are not supported.", + symlink.path(), symlink.target())); } - downloads.addAll(downloadDirectory(childPath, childDir, childrenMap, ctx)); + Preconditions.checkNotNull(symlink.path().getParentDirectory(), + "Failed creating directory and parents for %s", symlink.path()) + .createDirectoryAndParents(); + symlink.path().createSymbolicLink(symlink.target()); } - - return downloads; } /** Download a file (that is not a directory). The content is fetched from the digest. */ @@ -436,38 +550,39 @@ public void onFailure(Throwable t) { } } }, - MoreExecutors.directExecutor()); + directExecutor()); return outerF; } - private List downloadOutErr( - ActionResult result, FileOutErr outErr, Context ctx) throws IOException { - List downloads = new ArrayList<>(); + private List> downloadOutErr( + ActionResult result, OutErr outErr) throws IOException { + Context ctx = Context.current(); + List> downloads = new ArrayList<>(); if (!result.getStdoutRaw().isEmpty()) { result.getStdoutRaw().writeTo(outErr.getOutputStream()); outErr.getOutputStream().flush(); } else if (result.hasStdoutDigest()) { downloads.add( - new FuturePathBooleanTuple( + Futures.transform( retrier.executeAsync( () -> ctx.call( () -> downloadBlob(result.getStdoutDigest(), outErr.getOutputStream()))), - null, - false)); + (d) -> null, + directExecutor())); } if (!result.getStderrRaw().isEmpty()) { result.getStderrRaw().writeTo(outErr.getErrorStream()); outErr.getErrorStream().flush(); } else if (result.hasStderrDigest()) { downloads.add( - new FuturePathBooleanTuple( + Futures.transform( retrier.executeAsync( () -> ctx.call( () -> downloadBlob(result.getStderrDigest(), outErr.getErrorStream()))), - null, - false)); + (d) -> null, + directExecutor())); } return downloads; } @@ -690,7 +805,7 @@ private Directory computeDirectory(Path path, Tree.Builder tree) return b.build(); } - private void illegalOutput(Path what) throws ExecException, IOException { + private void illegalOutput(Path what) throws ExecException { String kind = what.isSymbolicLink() ? "symbolic link" : "special file"; throw new UserExecException( String.format( @@ -701,6 +816,118 @@ private void illegalOutput(Path what) throws ExecException, IOException { } } + private void downloadRequiredLocalOutputs( + Collection outputs, ActionOutputMetadata metadata, Path execRoot) + throws IOException, InterruptedException { + // TODO(buchgr): Implement support for tree artifacts + List> downloads = new ArrayList<>(outputs.size()); + for (ActionInput output : outputs) { + Path path = execRoot.getRelative(output.getExecPath()); + FileMetadata fileMetadata = metadata.file(path); + downloads.add( + Futures.transform( + downloadFile( + path, DigestUtil.buildDigest(fileMetadata.digest(), fileMetadata.sizeBytes())), + (d) -> fileMetadata, + MoreExecutors.directExecutor())); + } + + for (ListenableFuture download : downloads) { + getFromFuture(download); + } + } + + private void injectRemoteArtifacts( + Collection remoteOutputs, + ActionOutputMetadata metadata, + Path execRoot, + MetadataInjector metadataInjector) throws IOException { + for (Artifact remoteOutput : remoteOutputs) { + if (remoteOutput.isTreeArtifact()) { + DirectoryMetadata directory = + metadata.directory(execRoot.getRelative(remoteOutput.getExecPathString())); + if (directory == null) { + // A declared output wasn't created. It might have been an optional output and if not + // SkyFrame will make sure to fail. + continue; + } + if (!directory.symlinks().isEmpty()) { + throw new IOException("Symlinks in action outputs are not yet supported by " + + "--experimental_remote_fetch_outputs"); + } + ImmutableMap.Builder childMetadata = + ImmutableMap.builder(); + for (FileMetadata file : directory.files()) { + childMetadata.put( + file.path().relativeTo(remoteOutput.getPath()), + new RemoteFileArtifactValue(file.digest(), file.sizeBytes(), 1)); + } + metadataInjector.injectRemoteDirectory( + (SpecialArtifact) remoteOutput, childMetadata.build()); + } else { + FileMetadata outputMetadata = + metadata.file(execRoot.getRelative(remoteOutput.getExecPathString())); + if (outputMetadata == null) { + // A declared output wasn't created. It might have been an optional output and if not + // SkyFrame will make sure to fail. + continue; + } + metadataInjector.injectRemoteFile( + remoteOutput, outputMetadata.digest(), outputMetadata.sizeBytes(), 1); + } + } + } + + public void injectRemoteMetadata( + ActionResult result, + Collection outputs, + Collection requiredLocalOutputs, + OutErr outErr, + Path execRoot, + MetadataInjector metadataInjector) + throws IOException, InterruptedException { + Preconditions.checkState( + result.getExitCode() == 0, + "injecting remote metadata is only supported for successful actions (exit code 0)."); + + ActionOutputMetadata metadata; + try (SilentCloseable c = Profiler.instance().profile("Remote.parseActionResultMetadata")) { + metadata = parseActionResultMetadata(result, execRoot); + } + + if (!metadata.symlinks().isEmpty()) { + throw new IOException( + "Symlinks in action outputs are not yet supported by " + + "--experimental_remote_fetch_outputs"); + } + + Map outputsToDownload = + Maps.newHashMapWithExpectedSize(requiredLocalOutputs.size()); + for (ActionInput output : requiredLocalOutputs) { + outputsToDownload.put(output.getExecPath(), output); + } + int numOutputsToInject = outputs.size() - requiredLocalOutputs.size(); + List outputsToInject = new ArrayList<>(numOutputsToInject); + for (ActionInput output : outputs) { + if (outputsToDownload.containsKey(output.getExecPath()) || !(output instanceof Artifact)) { + continue; + } + outputsToInject.add((Artifact) output); + } + + try (SilentCloseable c = Profiler.instance().profile("Remote.downloadRequiredLocalOutputs")) { + downloadRequiredLocalOutputs(requiredLocalOutputs, metadata, execRoot); + } + + try (SilentCloseable c = Profiler.instance().profile("Remote.downloadStdoutStderr")) { + for (ListenableFuture download : downloadOutErr(result, outErr)) { + getFromFuture(download); + } + } + + injectRemoteArtifacts(outputsToInject, metadata, execRoot, metadataInjector); + } + /** Release resources associated with the cache. The cache may not be used after calling this. */ @Override public abstract void close(); @@ -755,4 +982,9 @@ private void ensureOpen() throws IOException { } } } + + @VisibleForTesting + protected T getFromFuture(ListenableFuture f) throws IOException, InterruptedException { + return Utils.getFromFuture(f); + } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index cfb24de3156777..284a1bbf323f6e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -30,10 +30,12 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote/blobstore", "//src/main/java/com/google/devtools/build/lib/remote/blobstore/http", "//src/main/java/com/google/devtools/build/lib/remote/logging", + "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/common/options", "//third_party:auth", + "//third_party:auto_value_value", "//third_party:guava", "//third_party:netty", "//third_party/grpc:grpc-jar", diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java index 53157d50ef27a6..c719b75fd35568 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderFactory.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.runtime.BuildEventArtifactUploaderFactory; import com.google.devtools.build.lib.runtime.CommandEnvironment; import io.grpc.Context; diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java index 127248dc00012f..249f64df7e6c2f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +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.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; @@ -83,10 +84,10 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache { public GrpcRemoteCache( ReferenceCountedChannel channel, CallCredentials credentials, + ByteStreamUploader uploader, RemoteOptions options, - RemoteRetrier retrier, DigestUtil digestUtil, - ByteStreamUploader uploader) { + RemoteRetrier retrier) { super(options, digestUtil, retrier); this.credentials = credentials; this.channel = channel; 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 7bb24c72bd1bb3..dcff6e49a3020b 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 @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.exec.ActionContextProvider; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.SpawnRunner; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.util.ExitCode; @@ -140,6 +141,11 @@ public void executorCreated(Iterable usedContexts) throws Executo } } + @Nullable + AbstractRemoteActionCache getRemoteCache() { + return cache; + } + @Override public void executionPhaseEnding() { if (cache != null) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java new file mode 100644 index 00000000000000..d10eb5af8d24f9 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -0,0 +1,144 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.remote; + +import com.google.common.base.Preconditions; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.MetadataProvider; +import com.google.devtools.build.lib.actions.cache.VirtualActionInput; +import com.google.devtools.build.lib.profiler.Profiler; +import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.Utils; +import com.google.devtools.build.lib.vfs.Path; +import io.grpc.Context; +import java.io.IOException; +import java.io.OutputStream; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import net.jcip.annotations.GuardedBy; + +/** + * Stages output files that are stored remotely to the local filesystem. + * + *

    This is necessary for remote caching/execution when --experimental_remote_fetch_outputs=minimal + * is specified. + */ +public class RemoteActionInputFetcher implements ActionInputPrefetcher { + + private final Object lock = new Object(); + + @GuardedBy("lock") + private final Set downloadedPaths = new HashSet<>(); + + @GuardedBy("lock") + private final Map> downloadsInProgress = new HashMap<>(); + + private final AbstractRemoteActionCache remoteCache; + private final Path execRoot; + private final Context ctx; + + public RemoteActionInputFetcher(AbstractRemoteActionCache remoteCache, Path execRoot, + Context ctx) { + this.remoteCache = Preconditions.checkNotNull(remoteCache); + this.execRoot = Preconditions.checkNotNull(execRoot); + this.ctx = Preconditions.checkNotNull(ctx); + } + + @Override + public void prefetchFiles( + Iterable inputs, MetadataProvider metadataProvider) + throws IOException, InterruptedException { + try (SilentCloseable c = Profiler.instance().profile("Remote.prefetchInputs")) { + Map> downloadsToWaitFor = new HashMap<>(); + for (ActionInput input : inputs) { + if (input instanceof VirtualActionInput) { + VirtualActionInput paramFileActionInput = (VirtualActionInput) input; + Path outputPath = execRoot.getRelative(paramFileActionInput.getExecPath()); + outputPath.getParentDirectory().createDirectoryAndParents(); + try (OutputStream out = outputPath.getOutputStream()) { + paramFileActionInput.writeTo(out); + } + } else { + FileArtifactValue metadata = metadataProvider.getMetadata(input); + if (metadata == null || !metadata.isRemote()) { + continue; + } + + Path path = execRoot.getRelative(input.getExecPath()); + synchronized (lock) { + if (downloadedPaths.contains(path)) { + continue; + } + + ListenableFuture download = downloadsInProgress.get(path); + if (download == null) { + Context prevCtx = ctx.attach(); + try { + download = remoteCache.downloadFile( + path, DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize())); + downloadsInProgress.put(path, download); + } finally { + ctx.detach(prevCtx); + } + } + downloadsToWaitFor.putIfAbsent(path, download); + } + } + } + + IOException ioException = null; + InterruptedException interruptedException = null; + try { + for (Map.Entry> entry : downloadsToWaitFor.entrySet()) { + try { + Utils.getFromFuture(entry.getValue()); + entry.getKey().setExecutable(true); + } catch (IOException e) { + if (e instanceof CacheNotFoundException) { + e = new IOException(String.format("Failed to fetch file with hash '%s' because it " + + "does not exist remotely. --experimental_remote_fetch_outputs=minimal does not " + + "work if your remote cache evicts files during builds.", + ((CacheNotFoundException) e).getMissingDigest().getHash())); + } + ioException = ioException == null ? e : ioException; + } catch (InterruptedException e) { + interruptedException = interruptedException == null ? e : interruptedException; + } + } + } finally { + synchronized (lock) { + for (Path path : downloadsToWaitFor.keySet()) { + downloadsInProgress.remove(path); + downloadedPaths.add(path); + } + } + } + + if (interruptedException != null) { + throw interruptedException; + } + if (ioException != null) { + throw ioException; + } + } + } +} 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 34a5d9bebd0685..a8cddbc2deb37e 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 @@ -30,6 +30,8 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.exec.ExecutorBuilder; import com.google.devtools.build.lib.remote.logging.LoggingInterceptor; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOptions.FetchRemoteOutputsStrategy; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -112,6 +114,11 @@ public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builde @Override public void beforeCommand(CommandEnvironment env) throws AbruptExitException { + RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); + if (!remoteEnabled(remoteOptions)) { + return; + } + env.getEventBus().register(this); String invocationId = env.getCommandId().toString(); @@ -131,16 +138,10 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { .handle(Event.error("Could not create base directory for remote logs: " + logDir)); throw new AbruptExitException(ExitCode.LOCAL_ENVIRONMENTAL_ERROR, e); } - RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); DigestHashFunction hashFn = env.getRuntime().getFileSystem().getDigestFunction(); DigestUtil digestUtil = new DigestUtil(hashFn); - // Quit if no remote options specified. - if (remoteOptions == null) { - return; - } - boolean enableRestCache = SimpleBlobStoreFactory.isRestUrlOptions(remoteOptions); boolean enableDiskCache = SimpleBlobStoreFactory.isDiskCache(remoteOptions); if (enableRestCache && enableDiskCache) { @@ -232,19 +233,19 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { new GrpcRemoteCache( cacheChannel.retain(), credentials, + uploader.retain(), remoteOptions, - rpcRetrier, digestUtil, - uploader.retain()); + rpcRetrier); uploader.release(); - Context requestContext = - TracingMetadataUtils.contextWithMetadata(buildRequestId, invocationId, "bes-upload"); - buildEventArtifactUploaderFactoryDelegate.init( - new ByteStreamBuildEventArtifactUploaderFactory( - uploader, - cacheChannel.authority(), - requestContext, - remoteOptions.remoteInstanceName)); + // Context requestContext = + // TracingMetadataUtils.contextWithMetadata(buildRequestId, invocationId, "bes-upload"); + // buildEventArtifactUploaderFactoryDelegate.init( + // new ByteStreamBuildEventArtifactUploaderFactory( + // uploader, + // cacheChannel.authority(), + // requestContext, + // remoteOptions.remoteInstanceName)); } if (enableBlobStoreCache) { @@ -257,11 +258,11 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { executeRetrier = null; cache = new SimpleBlobStoreActionCache( - remoteOptions, SimpleBlobStoreFactory.create( remoteOptions, GoogleAuthUtils.newCredentials(authAndTlsOptions), env.getWorkingDirectory()), + remoteOptions, retrier, digestUtil); } @@ -327,10 +328,35 @@ public void afterCommand() { buildEventArtifactUploaderFactoryDelegate.reset(); } + private boolean remoteEnabled(RemoteOptions options) { + return options != null && (!Strings.isNullOrEmpty(options.remoteHttpCache) || + !Strings.isNullOrEmpty(options.remoteCache) || + !Strings.isNullOrEmpty(options.remoteExecutor)); + } + @Override public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { - if (actionContextProvider != null) { - builder.addActionContextProvider(actionContextProvider); + RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); + if (!remoteEnabled(remoteOptions)) { + return; + } + + RemoteActionContextProvider actionContextProvider = this.actionContextProvider; + if (actionContextProvider == null) { + return; + } + builder.addActionContextProvider(actionContextProvider); + + FetchRemoteOutputsStrategy remoteOutputsStrategy = remoteOptions.experimentalRemoteFetchOutputs; + switch (remoteOutputsStrategy) { + case ALL: + break; + case MINIMAL: + Context ctx = TracingMetadataUtils.contextWithMetadata(env.getBuildRequestId(), + env.getCommandId().toString(), "prefetch-inputs"); + builder.setActionInputPrefetcher( + new RemoteActionInputFetcher(actionContextProvider.getRemoteCache(), env.getExecRoot(), ctx)); + break; } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java index c0d4906678dda6..ae9cb47737367d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteRetrier.java @@ -17,6 +17,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.ListeningScheduledExecutorService; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import io.grpc.Status; import io.grpc.StatusException; import io.grpc.StatusRuntimeException; diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java index a8f36ddddd16e1..b3d093d30dba9a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteServerCapabilities.java @@ -25,6 +25,7 @@ import build.bazel.remote.execution.v2.ServerCapabilities; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import io.grpc.CallCredentials; import io.grpc.Context; 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 5057f8f1ea6bc1..54d202eec65444 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 @@ -36,6 +36,8 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOptions.FetchRemoteOutputsStrategy; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; @@ -114,6 +116,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) spawn.getArguments(), spawn.getEnvironment(), spawn.getExecutionPlatform()); + FetchRemoteOutputsStrategy remoteOutputsStrategy = options.experimentalRemoteFetchOutputs; Action action; final ActionKey actionKey; try (SilentCloseable c = Profiler.instance().profile("RemoteCache.buildAction")) { @@ -140,19 +143,24 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) result = remoteCache.getCachedActionResult(actionKey); } if (result != null) { - // We don't cache failed actions, so we know the outputs exist. - // For now, download all outputs locally; in the future, we can reuse the digests to - // just update the TreeNodeRepository and continue the build. - try (SilentCloseable c = Profiler.instance().profile("RemoteCache.download")) { - remoteCache.download(result, execRoot, context.getFileOutErr()); + switch (remoteOutputsStrategy) { + case MINIMAL: + remoteCache.injectRemoteMetadata(result, spawn.getOutputFiles(), + context.getRequiredLocalOutputs(), context.getFileOutErr(), execRoot, + context.getMetadataInjector()); + break; + case ALL: + try (SilentCloseable c = Profiler.instance().profile("RemoteCache.download")) { + remoteCache.download(result, execRoot, context.getFileOutErr()); + } + break; } - SpawnResult spawnResult = - new SpawnResult.Builder() - .setStatus(Status.SUCCESS) - .setExitCode(result.getExitCode()) - .setCacheHit(true) - .setRunnerName("remote cache hit") - .build(); + SpawnResult spawnResult = new SpawnResult.Builder() + .setStatus(Status.SUCCESS) + .setExitCode(result.getExitCode()) + .setCacheHit(true) + .setRunnerName("remote cache hit") + .build(); return SpawnCache.success(spawnResult); } } catch (IOException e) { @@ -168,6 +176,9 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) withMetadata.detach(previous); } } + + context.prefetchInputs(); + if (options.remoteUploadLocalResults) { return new CacheHandle() { @Override 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 f4e03cdcf9a099..9b72f94f10bcb0 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 @@ -52,6 +52,8 @@ import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOptions.FetchRemoteOutputsStrategy; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; @@ -82,7 +84,7 @@ /** A client for the remote execution service. */ @ThreadSafe class RemoteSpawnRunner implements SpawnRunner { - private static final int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/128 + /*SIGALRM=*/14; + private static final int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/ 128 + /*SIGALRM=*/ 14; private final Path execRoot; private final RemoteOptions remoteOptions; @@ -146,21 +148,24 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) context.report(ProgressStatus.EXECUTING, getName()); // Temporary hack: the TreeNodeRepository should be created and maintained upstream! MetadataProvider inputFileCache = context.getMetadataProvider(); + FetchRemoteOutputsStrategy remoteOutputsStrategy = remoteOptions.experimentalRemoteFetchOutputs; TreeNodeRepository repository = new TreeNodeRepository( execRoot, inputFileCache, digestUtil, remoteOptions.incompatibleRemoteSymlinks); SortedMap inputMap = context.getInputMapping(true); + TreeNode inputRoot; try (SilentCloseable c = Profiler.instance().profile("Remote.computeMerkleDigests")) { inputRoot = repository.buildFromActionInputs(inputMap); repository.computeMerkleDigests(inputRoot); } maybeWriteParamFilesLocally(spawn); - Command command = buildCommand( - spawn.getOutputFiles(), - spawn.getArguments(), - spawn.getEnvironment(), - spawn.getExecutionPlatform()); + Command command = + buildCommand( + spawn.getOutputFiles(), + spawn.getArguments(), + spawn.getEnvironment(), + spawn.getExecutionPlatform()); Action action; ActionKey actionKey; try (SilentCloseable c = Profiler.instance().profile("Remote.buildAction")) { @@ -195,10 +200,27 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) + " served a failed action. Hash of the action: " + actionKey.getDigest()); } - try (SilentCloseable c = Profiler.instance().profile("Remote.downloadRemoteResults")) { - return downloadRemoteResults(cachedResult, context.getFileOutErr()) - .setCacheHit(true) + + try { + switch (remoteOutputsStrategy) { + case MINIMAL: + remoteCache.injectRemoteMetadata(cachedResult, spawn.getOutputFiles(), + context.getRequiredLocalOutputs(), context.getFileOutErr(), execRoot, + context.getMetadataInjector()); + break; + case ALL: + try (SilentCloseable c = + Profiler.instance().profile("Remote.downloadRemoteResults")) { + remoteCache.download(cachedResult, execRoot, context.getFileOutErr()); + } + break; + } + return new SpawnResult.Builder() + .setStatus( + cachedResult.getExitCode() == 0 ? Status.SUCCESS : Status.NON_ZERO_EXIT) + .setExitCode(cachedResult.getExitCode()) .setRunnerName("remote cache hit") + .setCacheHit(true) .build(); } catch (RetryException e) { if (!AbstractRemoteActionCache.causedByCacheMiss(e)) { @@ -252,13 +274,34 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) maybeDownloadServerLogs(reply, actionKey); } - try (SilentCloseable c = - Profiler.instance().profile("Remote.downloadRemoteResults")) { - return downloadRemoteResults(reply.getResult(), context.getFileOutErr()) - .setRunnerName(reply.getCachedResult() ? "remote cache hit" : getName()) - .setCacheHit(reply.getCachedResult()) - .build(); + ActionResult actionResult = reply.getResult(); + SpawnResult.Status actionStatus = + actionResult.getExitCode() == 0 ? Status.SUCCESS : Status.NON_ZERO_EXIT; + // In case the action failed, download all outputs. It might be helpful for debugging + // and there is no point in injecting output metadata of a failed action. + FetchRemoteOutputsStrategy effectiveOutputsStrategy = actionStatus == Status.SUCCESS + ? remoteOutputsStrategy + : FetchRemoteOutputsStrategy.ALL; + switch (effectiveOutputsStrategy) { + case MINIMAL: + remoteCache.injectRemoteMetadata(actionResult, spawn.getOutputFiles(), + context.getRequiredLocalOutputs(), context.getFileOutErr(), execRoot, + context.getMetadataInjector()); + break; + + case ALL: + try (SilentCloseable c = + Profiler.instance().profile("Remote.downloadRemoteResults")) { + remoteCache.download(actionResult, execRoot, context.getFileOutErr()); + } + break; } + return new SpawnResult.Builder() + .setStatus(actionStatus) + .setExitCode(actionResult.getExitCode()) + .setRunnerName(reply.getCachedResult() ? "remote cache hit" : getName()) + .setCacheHit(reply.getCachedResult()) + .build(); }); } catch (IOException e) { return execLocallyAndUploadOrFail( @@ -315,15 +358,6 @@ private void maybeDownloadServerLogs(ExecuteResponse resp, ActionKey actionKey) } } - private SpawnResult.Builder downloadRemoteResults(ActionResult result, FileOutErr outErr) - throws ExecException, IOException, InterruptedException { - remoteCache.download(result, execRoot, outErr); - int exitCode = result.getExitCode(); - return new SpawnResult.Builder() - .setStatus(exitCode == 0 ? Status.SUCCESS : Status.NON_ZERO_EXIT) - .setExitCode(exitCode); - } - private SpawnResult execLocally(Spawn spawn, SpawnExecutionContext context) throws ExecException, InterruptedException, IOException { return fallbackRunner.get().exec(spawn, context); @@ -395,11 +429,7 @@ private SpawnResult handleError(IOException exception, FileOutErr outErr, Action /* forciblyRunRemotely= */ false); } - static Action buildAction( - Digest command, - Digest inputRoot, - Duration timeout, - boolean cacheable) { + static Action buildAction(Digest command, Digest inputRoot, Duration timeout, boolean cacheable) { Action.Builder action = Action.newBuilder(); action.setCommandDigest(command); @@ -469,7 +499,7 @@ static Command buildCommand( } private Map getInputCtimes(SortedMap inputMap) { - HashMap ctimes = new HashMap<>(); + HashMap ctimes = new HashMap<>(); for (Map.Entry e : inputMap.entrySet()) { ActionInput input = e.getValue(); if (input instanceof VirtualActionInput) { @@ -547,8 +577,7 @@ private void report(Event evt) { */ static Collection resolveActionInputs( Path execRoot, Collection actionInputs) { - return actionInputs - .stream() + return actionInputs.stream() .map((inp) -> execRoot.getRelative(inp.getExecPath())) .collect(ImmutableList.toImmutableList()); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java index 13331981e06180..9919315b67d719 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java @@ -34,8 +34,10 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.blobstore.SimpleBlobStore; +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.DigestUtil.ActionKey; +import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -65,8 +67,8 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache private final ConcurrentHashMap storedBlobs; - public SimpleBlobStoreActionCache( - RemoteOptions options, SimpleBlobStore blobStore, Retrier retrier, DigestUtil digestUtil) { + public SimpleBlobStoreActionCache(SimpleBlobStore blobStore, RemoteOptions options, + Retrier retrier, DigestUtil digestUtil) { super(options, digestUtil, retrier); this.blobStore = blobStore; this.storedBlobs = new ConcurrentHashMap<>(); @@ -90,10 +92,10 @@ public void ensureInputsPresent( public void downloadTree(Digest rootDigest, Path rootLocation) throws IOException, InterruptedException { rootLocation.createDirectoryAndParents(); - Directory directory = Directory.parseFrom(getFromFuture(downloadBlob(rootDigest))); + Directory directory = Directory.parseFrom(Utils.getFromFuture(downloadBlob(rootDigest))); for (FileNode file : directory.getFilesList()) { Path dst = rootLocation.getRelative(file.getName()); - getFromFuture(downloadFile(dst, file.getDigest())); + Utils.getFromFuture(downloadFile(dst, file.getDigest())); dst.setExecutable(file.getIsExecutable()); } for (DirectoryNode child : directory.getDirectoriesList()) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java index ef8ffe22f76033..b3af4d7e3f6cb0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.remote.blobstore.OnDiskBlobStore; import com.google.devtools.build.lib.remote.blobstore.SimpleBlobStore; import com.google.devtools.build.lib.remote.blobstore.http.HttpBlobStore; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import io.netty.channel.unix.DomainSocketAddress; diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/BUILD b/src/main/java/com/google/devtools/build/lib/remote/options/BUILD new file mode 100644 index 00000000000000..c876d5ead9ae0f --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/options/BUILD @@ -0,0 +1,17 @@ +package(default_visibility = ["//src:__subpackages__"]) + +filegroup( + name = "srcs", + srcs = glob(["**"]), + visibility = ["//src/main/java/com/google/devtools/build/lib:__pkg__"], +) + +java_library( + name = "options", + srcs = ["RemoteOptions.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib:util", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/common/options", + ], +) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java similarity index 90% rename from src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java rename to src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 748da099769216..e194f2f0f9e2d8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -12,10 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.devtools.build.lib.remote; +package com.google.devtools.build.lib.remote.options; import com.google.devtools.build.lib.util.OptionsUtils; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -273,6 +274,36 @@ public final class RemoteOptions extends OptionsBase { + "cachable actions that output symlinks will fail.") public boolean allowSymlinkUpload; + /** + * Describes what kind of remote build outputs to download locally. + */ + public enum FetchRemoteOutputsStrategy { + /** Download all remote outputs locally. */ + ALL, + /** Generally don't download remote outputs locally. */ + MINIMAL, + } + + public static class FetchRemoteOutputsStrategyConverter extends EnumConverter { + public FetchRemoteOutputsStrategyConverter() { + super(FetchRemoteOutputsStrategy.class, "fetch remote outputs"); + } + } + + @Option( + name = "experimental_remote_fetch_outputs", + defaultValue = "all", + category = "remote", + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + converter = FetchRemoteOutputsStrategyConverter.class, + help = "If set to 'minimal' doesn't download any remote build outputs to the local machine, " + + "except the ones required by local actions. This option can significantly reduce build " + + "times if network bandwidth is a bottleneck." + ) + public FetchRemoteOutputsStrategy experimentalRemoteFetchOutputs; + + // The below options are not configurable by users, only tests. // This is part of the effort to reduce the overall number of flags. diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java index d1419f3c90a45e..a7e5328ded2e5e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import java.util.Collections; /** * Creates mangled symlinks in the solib directory for all shared libraries. Libraries that have a @@ -62,7 +63,9 @@ public final class SolibSymlinkAction extends AbstractAction { @Override public ActionResult execute(ActionExecutionContext actionExecutionContext) - throws ActionExecutionException { + throws ActionExecutionException, InterruptedException { + Actions.prefetchInputs(Collections.singletonList(getPrimaryInput()), actionExecutionContext, this); + Path mangledPath = actionExecutionContext.getInputPath(symlink); try { mangledPath.createSymbolicLink(actionExecutionContext.getInputPath(getPrimaryInput())); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java index 0ba465a6b8a1ed..aa348c3c116934 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java @@ -28,13 +28,10 @@ import com.google.devtools.build.lib.actions.SpawnResult; import java.util.List; -/** - * A context for C++ compilation that calls into a {@link SpawnActionContext}. - */ +/** A context for C++ compilation that calls into a {@link SpawnActionContext}. */ @ExecutionStrategy( - contextType = CppCompileActionContext.class, - name = {"spawn"} -) + contextType = CppCompileActionContext.class, + name = {"spawn"}) public class SpawnGccStrategy implements CppCompileActionContext { @Override public CppCompileActionResult execWithReply( @@ -61,8 +58,17 @@ public CppCompileActionResult execWithReply( action.getOutputs().asList(), action.estimateResourceConsumptionLocal()); + if (action.getDotdFile() != null) { + // .d file scanning happens locally even if the compiler was run remotely. We thus need + // to ensure that the .d file is staged on the local fileystem. + actionExecutionContext = + actionExecutionContext.withRequiredLocalOutputs( + ImmutableList.of(action.getDotdFile().artifact())); + } + List spawnResults = - actionExecutionContext.getContext(SpawnActionContext.class) + actionExecutionContext + .getContext(SpawnActionContext.class) .exec(spawn, actionExecutionContext); return CppCompileActionResult.builder().setSpawnResults(spawnResults).build(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 16b5bd5fb38ae2..5787e67fa3cd8c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -120,6 +120,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) PrecomputedValue.BUILD_ID.get(env); } + PrecomputedValue.FETCH_REMOTE_OUTPUTS.get(env); + // Look up the parts of the environment that influence the action. Map clientEnvLookup = env.getValues( @@ -723,10 +725,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( state.inputArtifactData.putWithNoDepOwner( ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue()); } - // TODO(ulfjack): This causes information loss about omitted and injected outputs. Also see - // the documentation on MetadataHandler.artifactOmitted. This works by accident because - // markOmitted is only called for remote execution, and this code only gets executed for - // local execution. metadataHandler = new ActionMetadataHandler( state.inputArtifactData, @@ -734,7 +732,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( action.getOutputs(), tsgm.get(), pathResolver, - state.actionFileSystem == null ? new OutputStore() : new MinimalOutputStore()); + metadataHandler.getOutputStore()); } } Preconditions.checkState(!env.valuesMissing(), action); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 50391f5edbd55a..70985fcd6326c4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -16,6 +16,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -29,6 +30,7 @@ import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.cache.Md5Digest; import com.google.devtools.build.lib.actions.cache.MetadataHandler; @@ -486,6 +488,18 @@ public void injectRemoteFile(Artifact output, byte[] digest, long size, int loca store.injectRemoteFile(output, digest, size, locationIndex); } + @Override + public void injectRemoteDirectory(SpecialArtifact output, + Map children) { + ImmutableMap.Builder childFileValues = ImmutableMap.builder(); + for (Map.Entry child : children.entrySet()) { + childFileValues.put(ActionInputHelper.treeFileArtifact(output, child.getKey()), child.getValue()); + } + + TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + store.putTreeArtifactData(output, treeArtifactValue); + } + @Override public void markOmitted(ActionInput output) { Preconditions.checkState(executionMode.get()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 9efc2b67a78305..52736c1dee3ad1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -26,6 +26,7 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFileMetadata; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; @@ -80,6 +81,7 @@ public class FilesystemValueChecker { private static final Predicate ACTION_FILTER = SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION); + @Nullable private final TimestampGranularityMonitor tsgm; @Nullable private final Range lastExecutionTimeRange; @@ -398,6 +400,9 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) try { Set currentDirectoryValue = TreeArtifactValue.explodeDirectory(artifact.getPath()); + if (currentDirectoryValue.isEmpty() && value.isRemote()) { + return false; + } Set valuePaths = value.getChildPaths(); return !currentDirectoryValue.equals(valuePaths); } catch (IOException e) { @@ -417,6 +422,14 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act try { ArtifactFileMetadata fileMetadata = ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm); + FileArtifactValue fileValue = actionValue.getArtifactValue(file); + boolean lastSeenRemotely = fileValue != null && fileValue.isRemote(); + if (!fileMetadata.exists() && lastSeenRemotely) { + // The output file does not exist in the output tree, but the last time we created it + // it was stored on a remotely so there is no need to invalidate it. + continue; + } + if (!fileMetadata.equals(lastKnownData)) { updateIntraBuildModifiedCounter( fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java index d2bc4e5ea91cce..b0cc1b9a4900e6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.remote.options.RemoteOptions.FetchRemoteOutputsStrategy; import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ConflictException; import com.google.devtools.build.lib.skyframe.serialization.UnshareableValue; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -118,6 +119,9 @@ public static boolean isInMemoryToolsDefaults(SkyFunction.Environment env) public static final Precomputed PATH_PACKAGE_LOCATOR = new Precomputed<>(Key.create("path_package_locator")); + public static final Precomputed FETCH_REMOTE_OUTPUTS = + new Precomputed<>(Key.create("fetch_remote_outputs")); + private final Object value; @AutoCodec.Instantiator diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index edf40b760f04c2..45c99691ceb466 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -595,6 +595,7 @@ public ActionExecutionContext getContext( : progressSuppressingEventHandler, clientEnv, topLevelFilesets, + ImmutableList.of(), new ArtifactExpanderImpl(expandedInputs, expandedFilesets), actionFileSystem, skyframeDepsResult); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 25f25dbd5c626a..13e595e39c240a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -117,6 +117,8 @@ import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; +import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOptions.FetchRemoteOutputsStrategy; import com.google.devtools.build.lib.rules.repository.ResolvedFileFunction; import com.google.devtools.build.lib.rules.repository.ResolvedHashesFunction; import com.google.devtools.build.lib.runtime.KeepGoingOption; @@ -1289,6 +1291,10 @@ public void setActionOutputRoot(Path actionOutputRoot) { this.skyframeActionExecutor.setActionLogBufferPathGenerator(actionLogBufferPathGenerator); } + public void setFetchRemoteActionOutputs(FetchRemoteOutputsStrategy fetchRemoteOutputs) { + PrecomputedValue.FETCH_REMOTE_OUTPUTS.set(injectable(), fetchRemoteOutputs); + } + /** * Asks the Skyframe evaluator to build the value for BuildConfigurationCollection and returns the * result. @@ -2251,6 +2257,7 @@ public void sync( OptionsProvider options) throws InterruptedException, AbruptExitException { getActionEnvFromOptions(options); + setFetchRemoteActionOutputs(options.getOptions(RemoteOptions.class).experimentalRemoteFetchOutputs); syncPackageLoading( packageCacheOptions, pathPackageLocator, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 9221ebc61ff732..aa8413165d1fd5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -56,11 +56,13 @@ public PathFragment apply(Artifact artifact) { private final byte[] digest; private final Map childData; private BigInteger valueFingerprint; + private final boolean isRemote; @AutoCodec.VisibleForSerialization - TreeArtifactValue(byte[] digest, Map childData) { + TreeArtifactValue(byte[] digest, Map childData, boolean isRemote) { this.digest = digest; this.childData = ImmutableMap.copyOf(childData); + this.isRemote = isRemote; } /** @@ -70,13 +72,16 @@ public PathFragment apply(Artifact artifact) { static TreeArtifactValue create(Map childFileValues) { Map digestBuilder = Maps.newHashMapWithExpectedSize(childFileValues.size()); + boolean isRemote = true; for (Map.Entry e : childFileValues.entrySet()) { + isRemote = isRemote && e.getValue().isRemote(); digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue()); } return new TreeArtifactValue( DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(), - ImmutableMap.copyOf(childFileValues)); + ImmutableMap.copyOf(childFileValues), + isRemote); } FileArtifactValue getSelfData() { @@ -104,6 +109,13 @@ Map getChildValues() { return childData; } + /** + * Returns {@code true} if at least one {@link TreeFileArtifact} is only stored remotely. + */ + public boolean isRemote() { + return isRemote; + } + @Override public BigInteger getValueFingerprint() { if (valueFingerprint == null) { @@ -150,7 +162,7 @@ public String toString() { * Java's concurrent collections disallow null members. */ static final TreeArtifactValue MISSING_TREE_ARTIFACT = - new TreeArtifactValue(null, ImmutableMap.of()) { + new TreeArtifactValue(null, ImmutableMap.of(), false) { @Override FileArtifactValue getSelfData() { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index bef9b21ec86f38..5f7629664f6798 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -1305,6 +1305,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/remote/blobstore", "//src/main/java/com/google/devtools/build/lib/remote/blobstore/http", "//src/main/java/com/google/devtools/build/lib/remote/logging", + "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", diff --git a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java index 9fa6ae3ceb647d..3deb3f6dedd388 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java @@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; import static org.junit.Assert.fail; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.util.DummyExecutor; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; @@ -66,6 +67,7 @@ private ActionExecutionContext createContext() { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + /*requiredLocalOutputs=*/ ImmutableList.of(), null, null, null); diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 7054d57b0ee960..d6778ad92a1cf0 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -147,6 +147,7 @@ public static ActionExecutionContext createContext( executor != null ? executor.getEventHandler() : null, ImmutableMap.copyOf(clientEnv), ImmutableMap.of(), + ImmutableList.of(), actionGraph == null ? createDummyArtifactExpander() : ActionInputHelper.actionGraphArtifactExpander(actionGraph), @@ -187,6 +188,7 @@ public static ActionExecutionContext createContext(ExtendedEventHandler eventHan eventHandler, ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), createDummyArtifactExpander(), /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 5406b4ff0eee77..57934e7c1367e0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -2229,6 +2229,7 @@ public ActionExecutionContext build() { reporter, clientEnv, ImmutableMap.of(), + ImmutableList.of(), artifactExpander, /*actionFileSystem=*/ null, /*skyframeDepsResult*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java index 14776d6cb67cdc..c96b10f83e06be 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java @@ -16,6 +16,10 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.fail; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionResult; @@ -24,31 +28,45 @@ 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.OutputDirectory; import build.bazel.remote.execution.v2.OutputFile; import build.bazel.remote.execution.v2.SymlinkNode; import build.bazel.remote.execution.v2.Tree; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.hash.HashCode; 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.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; +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.SpecialArtifactType; +import com.google.devtools.build.lib.actions.ArtifactOwner; +import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +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.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.util.io.OutErr; +import com.google.devtools.build.lib.util.io.RecordingOutErr; 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.protobuf.Message; import java.io.IOException; import java.io.OutputStream; import java.io.UnsupportedEncodingException; @@ -66,6 +84,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mockito; /** Tests for {@link AbstractRemoteActionCache}. */ @RunWith(JUnit4.class) @@ -667,6 +686,101 @@ public void onErrorWaitForRemainingDownloadsToComplete() throws Exception { } } + @Test + public void testInjectRemoteFileMetadata() throws Exception { + // Test that injecting the metadata for a remote output file works. + + DefaultRemoteActionCache c = newTestCache(); + Digest d1 = c.addContents("content1"); + Digest d2 = c.addContents("content2"); + ActionResult r = + ActionResult.newBuilder() + .setExitCode(0) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file1").setDigest(d1)) + .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file2").setDigest(d2)) + .build(); + + ArtifactRoot ar = ArtifactRoot.asDerivedRoot(execRoot, execRoot.getChild("outputs")); + Artifact a1 = new Artifact(PathFragment.create("file1"), ar); + Artifact a2 = new Artifact(PathFragment.create("file2"), ar); + + MetadataInjector injector = mock(MetadataInjector.class); + c.injectRemoteMetadata(r, ImmutableList.of(a1, a2), ImmutableList.of(), new FileOutErr(), + execRoot, injector); + + verify(injector).injectRemoteFile(eq(a1), eq(hexToBytes(d1)), eq(d1.getSizeBytes()), anyInt()); + verify(injector).injectRemoteFile(eq(a2), eq(hexToBytes(d2)), eq(d2.getSizeBytes()), anyInt()); + } + + @Test + public void testInjectRemoteDirectoryMetadata() throws Exception { + // Test that injecting the metadata for a tree artifact / remote output directory works. + + DefaultRemoteActionCache c = newTestCache(); + // Output Directory: + // dir/file1 + // dir/a/file2 + Digest d1 = c.addContents("content1"); + Digest d2 = c.addContents("content2"); + FileNode file1 = FileNode.newBuilder().setName("file1").setDigest(d1).build(); + FileNode file2 = FileNode.newBuilder().setName("file2").setDigest(d2).build(); + Directory a = Directory.newBuilder().addFiles(file2).build(); + Digest da = c.addContents(a); + Directory root = Directory.newBuilder() + .addFiles(file1) + .addDirectories(DirectoryNode.newBuilder().setName("a").setDigest(da)) + .build(); + Tree t = Tree.newBuilder().setRoot(root).addChildren(a).build(); + Digest dt = digestUtil.compute(t);// c.addContents(t); + ActionResult r = ActionResult.newBuilder() + .setExitCode(0) + .addOutputDirectories(OutputDirectory.newBuilder().setPath("outputs/dir") + .setTreeDigest(dt)) + .build(); + + ArtifactRoot ar = ArtifactRoot.asDerivedRoot(execRoot, execRoot.getChild("outputs")); + SpecialArtifact dir = new SpecialArtifact(ar, PathFragment.create("outputs/dir"), + ArtifactOwner.NullArtifactOwner.INSTANCE, SpecialArtifactType.TREE); + Artifact a1 = new Artifact(PathFragment.create("dir/file1"), ar); + Artifact a2 = new Artifact(PathFragment.create("dir/a/file2"), ar); + + MetadataInjector injector = mock(MetadataInjector.class); + c.injectRemoteMetadata(r, ImmutableList.of(dir), ImmutableList.of(), new FileOutErr(), + execRoot, injector); + + Map m = ImmutableMap.builder() + .put(PathFragment.create("file1"), new RemoteFileArtifactValue(hexToBytes(d1), d1.getSizeBytes(), 1)) + .put(PathFragment.create("a/file2"), new RemoteFileArtifactValue(hexToBytes(d2), d2.getSizeBytes(), 1)) + .build(); + + verify(injector).injectRemoteDirectory(eq(dir), eq(m)); + } + + @Test + public void testInjectRemoteFileWithStdoutStderr() throws Exception { + // Test that downloading of non-embedded stdout and stderr works. + + DefaultRemoteActionCache c = newTestCache(); + Digest dOut = c.addContents("stdout"); + Digest dErr = c.addContents("stderr"); + ActionResult r = ActionResult.newBuilder().setExitCode(0) + .setStdoutDigest(dOut) + .setStderrDigest(dErr) + .build(); + + RecordingOutErr outErr = new RecordingOutErr(); + MetadataInjector injector = mock(MetadataInjector.class); + c.injectRemoteMetadata(r, ImmutableList.of(), ImmutableList.of(), outErr, execRoot, + injector); + + assertThat(outErr.outAsLatin1()).isEqualTo("stdout"); + assertThat(outErr.errAsLatin1()).isEqualTo("stderr"); + } + + private byte[] hexToBytes(Digest digest) { + return HashCode.fromString(digest.getHash()).asBytes(); + } + private DefaultRemoteActionCache newTestCache() { RemoteOptions options = new RemoteOptions(); RemoteRetrier retrier = @@ -685,17 +799,21 @@ public DefaultRemoteActionCache(RemoteOptions options, DigestUtil digestUtil, Re super(options, digestUtil, retrier); } - public Digest addContents(String txt) throws UnsupportedEncodingException { + public Digest addContents(String txt) { return addContents(txt.getBytes(UTF_8)); } - public Digest addContents(byte[] bytes) throws UnsupportedEncodingException { + public Digest addContents(byte[] bytes) { Digest digest = digestUtil.compute(bytes); downloadResults.put(digest, Futures.immediateFuture(bytes)); return digest; } - public Digest addException(String txt, Exception e) throws UnsupportedEncodingException { + public Digest addContents(Message m) { + return addContents(m.toByteArray()); + } + + public Digest addException(String txt, Exception e) { Digest digest = digestUtil.compute(txt.getBytes(UTF_8)); downloadResults.put(digest, Futures.immediateFailedFuture(e)); return digest; @@ -749,8 +867,11 @@ void upload( @Override protected ListenableFuture downloadBlob(Digest digest, OutputStream out) { SettableFuture result = SettableFuture.create(); + ListenableFuture downloadResult = downloadResults.get(digest); Futures.addCallback( - downloadResults.get(digest), + downloadResult != null + ? downloadResult + : Futures.immediateFailedFuture(new CacheNotFoundException(digest, digestUtil)), new FutureCallback() { @Override public void onSuccess(byte[] bytes) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java index bd677f62d6b484..291815c3d8e2fa 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java @@ -51,6 +51,7 @@ import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +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.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; @@ -207,10 +208,10 @@ private GrpcRemoteCache newClient(RemoteOptions remoteOptions) throws IOExceptio remoteOptions.remoteTimeout, retrier); return new GrpcRemoteCache(channel.retain(), creds, + uploader, remoteOptions, - retrier, DIGEST_UTIL, - uploader); + retrier); } static class StringVirtualActionInput implements VirtualActionInput { diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java index 1c856d1e8b9166..f706b4d83594c4 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java @@ -62,6 +62,7 @@ import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.util.io.FileOutErr; @@ -139,7 +140,8 @@ public void expand(Artifact artifact, Collection output) { private static ListeningScheduledExecutorService retryService; private static final OutputFile DUMMY_OUTPUT = - OutputFile.newBuilder().setPath("dummy.txt").build(); + OutputFile.newBuilder().setPath("dummy.txt") + .setDigest(Digest.newBuilder().setHash("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855").setSizeBytes(0).build()).build(); private final SpawnExecutionContext simplePolicy = new SpawnExecutionContext() { @@ -150,7 +152,6 @@ public int getId() { @Override public void prefetchInputs() { - throw new UnsupportedOperationException(); } @Override @@ -273,7 +274,7 @@ public PathFragment getExecPath() { new ByteStreamUploader(remoteOptions.remoteInstanceName, channel.retain(), creds, remoteOptions.remoteTimeout, retrier); GrpcRemoteCache remoteCache = - new GrpcRemoteCache(channel.retain(), creds, remoteOptions, retrier, DIGEST_UTIL, uploader); + new GrpcRemoteCache(channel.retain(), creds, uploader, remoteOptions, DIGEST_UTIL, retrier); client = new RemoteSpawnRunner( execRoot, diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java index d9b08d26ce5627..a7d52479d57bc1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteRetrierTest.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.remote.Retrier.Backoff; import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.Retrier.Sleeper; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.common.options.Options; import io.grpc.Status; import io.grpc.StatusRuntimeException; diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java index ba596a24ddf89b..3f622ba2ac460c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteServerCapabilitiesTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.analysis.BlazeVersionInfo; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.common.options.Options; import io.grpc.CallCredentials; 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 cba05a7883d189..5cf978fc44ecfd 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 @@ -53,6 +53,7 @@ import com.google.devtools.build.lib.exec.util.FakeOwner; import com.google.devtools.build.lib.remote.Retrier.RetryException; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +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.DigestUtil.ActionKey; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; @@ -113,8 +114,6 @@ public int getId() { @Override public void prefetchInputs() { - // CachedLocalSpawnRunner should never prefetch itself, though the nested SpawnRunner may. - throw new UnsupportedOperationException(); } @Override 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 902dfd04671c72..029d9447a2a9bb 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 @@ -66,6 +66,7 @@ import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; import com.google.devtools.build.lib.exec.util.FakeOwner; +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.DigestUtil.ActionKey; import com.google.devtools.build.lib.util.ExitCode; diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java index 2c059417ff1920..85cd5a2bd1a07b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.remote.Retrier.Backoff; import com.google.devtools.build.lib.remote.blobstore.ConcurrentMapBlobStore; +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.vfs.DigestHashFunction; @@ -122,8 +123,8 @@ private SimpleBlobStoreActionCache newClient() { private SimpleBlobStoreActionCache newClient(ConcurrentMap map) { return new SimpleBlobStoreActionCache( - Options.getDefaults(RemoteOptions.class), new ConcurrentMapBlobStore(map), + Options.getDefaults(RemoteOptions.class), retrier, DIGEST_UTIL); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 0120b3b2c431ee..b3835f9cae92f8 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -21,8 +21,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Runnables; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -32,8 +34,10 @@ import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ServerDirectories; @@ -49,6 +53,7 @@ import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.BatchStat; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; @@ -73,9 +78,11 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; +import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -785,6 +792,113 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List /*actionDependsOnBuildId=*/ false); } + private ActionExecutionValue actionValueWithRemoteTreeArtifact(SpecialArtifact output, Map children) { + ImmutableMap.Builder childFileValues = ImmutableMap.builder(); + for (Map.Entry child : children.entrySet()) { + childFileValues.put(ActionInputHelper.treeFileArtifact(output, child.getKey()), child.getValue()); + } + TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + return ActionExecutionValue.create(Collections.emptyMap(), + Collections.singletonMap(output, treeArtifactValue), ImmutableMap.of(), null, null, false); + } + + private ActionExecutionValue actionValueWithRemoteArtifact(Artifact output, RemoteFileArtifactValue value) { + return ActionExecutionValue.create( + Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(), + Collections.singletonMap(output, value), null, null, false); + } + + private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + byte[] data = contents.getBytes(); + DigestHashFunction hashFn = fs.getDigestFunction(); + HashCode hash = hashFn.getHashFunction().hashBytes(data); + return new RemoteFileArtifactValue(hash.asBytes(), data.length, -1); + } + + @Test + public void testRemoteAndLocalArtifacts() throws Exception { + // Test that injected remote artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey1 = ActionExecutionValue.key(actionLookupKey, 0); + SkyKey actionKey2 = ActionExecutionValue.key(actionLookupKey, 1); + + Artifact out1 = createDerivedArtifact("foo"); + Artifact out2 = createDerivedArtifact("bar"); + Map metadataToInject = new HashMap<>(); + metadataToInject.put(actionKey1, actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + metadataToInject.put(actionKey2, actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content"))); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext).hasError()).isFalse(); + assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(), + null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty(); + + // Create the "out1" artifact on the filesystem and test that it invalidates the generating + // action's SkyKey. + FileSystemUtils.writeContentAsLatin1(out1.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(null, null) + .getDirtyActionValues( + evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey1); + } + + @Test + public void testRemoteAndLocalTreeArtifacts() throws Exception { + // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey = ActionExecutionValue.key(actionLookupKey, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + Map treeArtifactMetadata = new HashMap<>(); + treeArtifactMetadata.put(PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content")); + treeArtifactMetadata.put(PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content")); + + Map metadataToInject = new HashMap<>(); + metadataToInject.put(actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata)); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()).isFalse(); + assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(), + null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty(); + + // Create dir/foo on the local disk and test that it invalidates the associated sky key. + TreeFileArtifact fooArtifact = treeFileArtifact(treeArtifact, "foo"); + FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(null, null) + .getDirtyActionValues( + evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey); + } + @Test public void testPropagatesRuntimeExceptions() throws Exception { Collection values = diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java index 8272493e94337c..1fa251eb1cd5db 100644 --- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java @@ -195,6 +195,7 @@ private ActionExecutionContext createContext() { executor.getEventHandler(), ImmutableMap.of(), ImmutableMap.of(), + ImmutableList.of(), SIMPLE_ARTIFACT_EXPANDER, /*actionFileSystem=*/ null, /*skyframeDepsResult=*/ null); 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 fede8c7aa0512a..aace0051a5dab9 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 @@ -21,6 +21,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/blobstore", + "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/sandbox", "//src/main/java/com/google/devtools/build/lib/shell", diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java index 34bef33d437ca2..3fd3ba5728a2b5 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/RemoteWorker.java @@ -31,7 +31,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; -import com.google.devtools.build.lib.remote.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.RemoteRetrier; import com.google.devtools.build.lib.remote.Retrier; import com.google.devtools.build.lib.remote.SimpleBlobStoreActionCache; @@ -271,7 +271,7 @@ public static void main(String[] args) throws Exception { new RemoteWorker( fs, remoteWorkerOptions, - new SimpleBlobStoreActionCache(remoteOptions, blobStore, retrier, digestUtil), + new SimpleBlobStoreActionCache(blobStore, remoteOptions, retrier, digestUtil), sandboxPath, digestUtil);