diff options
18 files changed, 331 insertions, 124 deletions
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 d8c42ff140..6745b1a96e 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 @@ -41,7 +41,6 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.time.Duration; -import java.util.ArrayList; import java.util.List; import java.util.SortedMap; import java.util.concurrent.atomic.AtomicInteger; @@ -95,8 +94,7 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte // Actual execution. spawnResult = spawnRunner.exec(spawn, context); if (cacheHandle.willStore()) { - cacheHandle.store( - spawnResult, listExistingOutputFiles(spawn, actionExecutionContext.getExecRoot())); + cacheHandle.store(spawnResult); } } } @@ -144,19 +142,6 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte return ImmutableList.of(spawnResult); } - private List<Path> listExistingOutputFiles(Spawn spawn, Path execRoot) { - ArrayList<Path> outputFiles = new ArrayList<>(); - for (ActionInput output : spawn.getOutputFiles()) { - Path outputPath = execRoot.getRelative(output.getExecPathString()); - // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead - // of statting the files here again. - if (outputPath.exists()) { - outputFiles.add(outputPath); - } - } - return outputFiles; - } - private final class SpawnExecutionContextImpl implements SpawnExecutionContext { private final Spawn spawn; private final ActionExecutionContext actionExecutionContext; diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java index afc9d9376b..48bdaf5c81 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java @@ -19,10 +19,8 @@ import com.google.devtools.build.lib.actions.ExecutionStrategy; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext; -import com.google.devtools.build.lib.vfs.Path; import java.io.Closeable; import java.io.IOException; -import java.util.Collection; import java.util.NoSuchElementException; /** @@ -33,32 +31,31 @@ import java.util.NoSuchElementException; */ public interface SpawnCache extends ActionContext { /** A no-op implementation that has no result, and performs no upload. */ - public static CacheHandle NO_RESULT_NO_STORE = new CacheHandle() { - @Override - public boolean hasResult() { - return false; - } - - @Override - public SpawnResult getResult() { - throw new NoSuchElementException(); - } - - @Override - public boolean willStore() { - return false; - } - - @Override - public void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException { - // Do nothing. - } - - @Override - public void close() { - } - }; + public static CacheHandle NO_RESULT_NO_STORE = + new CacheHandle() { + @Override + public boolean hasResult() { + return false; + } + + @Override + public SpawnResult getResult() { + throw new NoSuchElementException(); + } + + @Override + public boolean willStore() { + return false; + } + + @Override + public void store(SpawnResult result) throws InterruptedException, IOException { + // Do nothing. + } + + @Override + public void close() {} + }; /** * Helper method to create a {@link CacheHandle} from a successful {@link SpawnResult} instance. @@ -81,14 +78,12 @@ public interface SpawnCache extends ActionContext { } @Override - public void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException { + public void store(SpawnResult result) throws InterruptedException, IOException { throw new IllegalStateException(); } @Override - public void close() { - } + public void close() {} }; } @@ -148,8 +143,7 @@ public interface SpawnCache extends ActionContext { * <p>If the current thread is interrupted, then this method should return as quickly as * possible with an {@link InterruptedException}. */ - void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException; + void store(SpawnResult result) throws ExecException, InterruptedException, IOException; } /** 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 be81ad6cfe..009eddb991 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 @@ -15,13 +15,16 @@ package com.google.devtools.build.lib.remote; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.concurrent.ThreadSafety; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Dirent; +import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.remoteexecution.v1test.ActionResult; import com.google.devtools.remoteexecution.v1test.Command; import com.google.devtools.remoteexecution.v1test.Digest; @@ -45,9 +48,11 @@ 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 { + protected final RemoteOptions options; protected final DigestUtil digestUtil; - public AbstractRemoteActionCache(DigestUtil digestUtil) { + public AbstractRemoteActionCache(RemoteOptions options, DigestUtil digestUtil) { + this.options = options; this.digestUtil = digestUtil; } @@ -93,7 +98,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { Collection<Path> files, FileOutErr outErr, boolean uploadAction) - throws IOException, InterruptedException; + throws ExecException, IOException, InterruptedException; /** * Download a remote blob to a local destination. @@ -253,12 +258,12 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { } } - /** - * The UploadManifest is used to mutualize upload between the RemoteActionCache implementations. - */ - public class UploadManifest { + /** UploadManifest adds output metadata to a {@link ActionResult}. */ + static class UploadManifest { + private final DigestUtil digestUtil; private final ActionResult.Builder result; private final Path execRoot; + private final boolean allowSymlinks; private final Map<Digest, Path> digestToFile; private final Map<Digest, Chunker> digestToChunkers; @@ -266,33 +271,45 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult * builder is populated through a call to {@link #addFile(Digest, Path)}. */ - public UploadManifest(ActionResult.Builder result, Path execRoot) { + public UploadManifest( + DigestUtil digestUtil, ActionResult.Builder result, Path execRoot, boolean allowSymlinks) { + this.digestUtil = digestUtil; this.result = result; this.execRoot = execRoot; + this.allowSymlinks = allowSymlinks; this.digestToFile = new HashMap<>(); this.digestToChunkers = new HashMap<>(); } /** - * Add a collection of files (and directories) to the UploadManifest. Adding a directory has the + * Add a collection of files or directories to the UploadManifest. Adding a directory has the * effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the * directory, including the descendants, can be reconstructed and 2) uploading all the * non-directory descendant files. + * + * <p>Attempting to a upload symlink results in a {@link + * com.google.build.lib.actions.ExecException}, since cachable actions shouldn't emit symlinks. */ - public void addFiles(Collection<Path> files) throws IOException, InterruptedException { + public void addFiles(Collection<Path> files) + throws ExecException, IOException, InterruptedException { for (Path file : files) { // TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and // rely on the local spawn runner to stat the files, instead of statting here. - if (!file.exists()) { + FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW); + if (stat == null) { // We ignore requested results that have not been generated by the action. continue; } - if (file.isDirectory()) { + if (stat.isDirectory()) { addDirectory(file); - } else { - Digest digest = digestUtil.compute(file); + } else if (stat.isFile()) { + Digest digest = digestUtil.compute(file, stat.getSize()); addFile(digest, file); + } else if (allowSymlinks && stat.isSymbolicLink()) { + addFile(digestUtil.compute(file), file); + } else { + illegalOutput(file); } } } @@ -321,7 +338,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { digestToFile.put(digest, file); } - private void addDirectory(Path dir) throws IOException { + private void addDirectory(Path dir) throws ExecException, IOException { Tree.Builder tree = Tree.newBuilder(); Directory root = computeDirectory(dir, tree); tree.setRoot(root); @@ -340,7 +357,8 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { digestToChunkers.put(chunker.digest(), chunker); } - private Directory computeDirectory(Path path, Tree.Builder tree) throws IOException { + private Directory computeDirectory(Path path, Tree.Builder tree) + throws ExecException, IOException { Directory.Builder b = Directory.newBuilder(); List<Dirent> sortedDirent = new ArrayList<>(path.readdir(TreeNodeRepository.SYMLINK_POLICY)); @@ -353,15 +371,28 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { Directory dir = computeDirectory(child, tree); b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); tree.addChildren(dir); - } else { + } else if (dirent.getType() == Dirent.Type.FILE + || (dirent.getType() == Dirent.Type.SYMLINK && allowSymlinks)) { Digest digest = digestUtil.compute(child); b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); digestToFile.put(digest, child); + } else { + illegalOutput(child); } } return b.build(); } + + private void illegalOutput(Path what) throws ExecException, IOException { + String kind = what.isSymbolicLink() ? "symbolic link" : "special file"; + throw new UserExecException( + String.format( + "Output %s is a %s. Only regular files and directories may be " + + "uploaded to a remote cache. " + + "Change the file type or use --remote_allow_symlink_upload.", + what.relativeTo(execRoot), kind)); + } } /** Release resources associated with the cache. The cache may not be used after calling this. */ 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 3f5a48176b..8f65021709 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 @@ -25,6 +25,7 @@ import com.google.common.collect.Iterables; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.remote.Retrier.RetryException; @@ -65,7 +66,6 @@ import java.util.concurrent.TimeUnit; /** A RemoteActionCache implementation that uses gRPC calls to a remote cache server. */ @ThreadSafe public class GrpcRemoteCache extends AbstractRemoteActionCache { - private final RemoteOptions options; private final CallCredentials credentials; private final Channel channel; private final RemoteRetrier retrier; @@ -80,8 +80,7 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache { RemoteOptions options, RemoteRetrier retrier, DigestUtil digestUtil) { - super(digestUtil); - this.options = options; + super(options, digestUtil); this.credentials = credentials; this.channel = channel; this.retrier = retrier; @@ -240,7 +239,7 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache { Collection<Path> files, FileOutErr outErr, boolean uploadAction) - throws IOException, InterruptedException { + throws ExecException, IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); upload(execRoot, files, outErr, result); if (!uploadAction) { @@ -266,8 +265,9 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache { } void upload(Path execRoot, Collection<Path> files, FileOutErr outErr, ActionResult.Builder result) - throws IOException, InterruptedException { - UploadManifest manifest = new UploadManifest(result, execRoot); + throws ExecException, IOException, InterruptedException { + UploadManifest manifest = + new UploadManifest(digestUtil, result, execRoot, options.allowSymlinkUpload); manifest.addFiles(files); List<Chunker> filesToUpload = new ArrayList<>(); 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 d9ab87a9f1..5453a98566 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 @@ -145,6 +145,7 @@ public final class RemoteModule extends BlazeModule { if (remoteOrLocalCache) { cache = new SimpleBlobStoreActionCache( + remoteOptions, SimpleBlobStoreFactory.create( remoteOptions, GoogleAuthUtils.newCredentials(authAndTlsOptions), diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java index e3b303db42..3431e95dac 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java @@ -214,4 +214,19 @@ public final class RemoteOptions extends OptionsBase { + "LogEntry.writeDelimitedTo(OutputStream)." ) public String experimentalRemoteGrpcLog; + + @Option( + name = "remote_allow_symlink_upload", + defaultValue = "true", + category = "remote", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "If true, upload action symlink outputs to the remote cache. " + + "The remote cache currently doesn't support symlinks, " + + "so symlink outputs are converted into regular files. " + + "If this option is not enabled, " + + "otherwise cachable actions that output symlinks will fail." + ) + public boolean allowSymlinkUpload; } 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 2e31f83b48..ff83eb7a66 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 @@ -168,8 +168,8 @@ final class RemoteSpawnCache implements SpawnCache { } @Override - public void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException { + public void store(SpawnResult result) + throws ExecException, InterruptedException, IOException { if (options.experimentalGuardAgainstConcurrentChanges) { try { checkForConcurrentModifications(); @@ -183,6 +183,8 @@ final class RemoteSpawnCache implements SpawnCache { && Status.SUCCESS.equals(result.status()) && result.exitCode() == 0; Context previous = withMetadata.attach(); + Collection<Path> files = + RemoteSpawnRunner.resolveActionInputs(execRoot, spawn.getOutputFiles()); try { remoteCache.upload(actionKey, execRoot, files, context.getFileOutErr(), uploadAction); } catch (IOException e) { 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 9034e40c2e..fb28fffe96 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 @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.remote; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; @@ -441,12 +442,12 @@ class RemoteSpawnRunner implements SpawnRunner { return result; } } - List<Path> outputFiles = listExistingOutputFiles(execRoot, spawn); + boolean uploadAction = + Spawns.mayBeCached(spawn) + && Status.SUCCESS.equals(result.status()) + && result.exitCode() == 0; + Collection<Path> outputFiles = resolveActionInputs(execRoot, spawn.getOutputFiles()); try { - boolean uploadAction = - Spawns.mayBeCached(spawn) - && Status.SUCCESS.equals(result.status()) - && result.exitCode() == 0; remoteCache.upload(actionKey, execRoot, outputFiles, context.getFileOutErr(), uploadAction); } catch (IOException e) { if (verboseFailures) { @@ -470,16 +471,12 @@ class RemoteSpawnRunner implements SpawnRunner { } } - static List<Path> listExistingOutputFiles(Path execRoot, Spawn spawn) { - ArrayList<Path> outputFiles = new ArrayList<>(); - for (ActionInput output : spawn.getOutputFiles()) { - Path outputPath = execRoot.getRelative(output.getExecPathString()); - // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead - // of statting the files here again. - if (outputPath.exists()) { - outputFiles.add(outputPath); - } - } - return outputFiles; + /** Resolve a collection of {@link com.google.build.lib.actions.ActionInput}s to {@link Path}s. */ + static Collection<Path> resolveActionInputs( + Path execRoot, Collection<? extends ActionInput> actionInputs) { + 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 2d4f4e9bbc..803946cc61 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 @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.remote; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -55,8 +56,9 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache private final SimpleBlobStore blobStore; - public SimpleBlobStoreActionCache(SimpleBlobStore blobStore, DigestUtil digestUtil) { - super(digestUtil); + public SimpleBlobStoreActionCache( + RemoteOptions options, SimpleBlobStore blobStore, DigestUtil digestUtil) { + super(options, digestUtil); this.blobStore = blobStore; } @@ -114,7 +116,7 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache Collection<Path> files, FileOutErr outErr, boolean uploadAction) - throws IOException, InterruptedException { + throws ExecException, IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); upload(result, execRoot, files); if (outErr.getErrorPath().exists()) { @@ -131,8 +133,9 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache } public void upload(ActionResult.Builder result, Path execRoot, Collection<Path> files) - throws IOException, InterruptedException { - UploadManifest manifest = new UploadManifest(result, execRoot); + throws ExecException, IOException, InterruptedException { + UploadManifest manifest = + new UploadManifest(digestUtil, result, execRoot, options.allowSymlinkUpload); manifest.addFiles(files); for (Map.Entry<Digest, Path> entry : manifest.getDigestToFile().entrySet()) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java index afd31ccdeb..e4dc92a46f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java @@ -60,9 +60,11 @@ public class DigestUtil { } public Digest compute(Path file) throws IOException { - long fileSize = file.getFileSize(); - byte[] digest = DigestUtils.getDigestOrFail(file, fileSize); - return buildDigest(digest, fileSize); + return compute(file, file.getFileSize()); + } + + public Digest compute(Path file, long fileSize) throws IOException { + return buildDigest(DigestUtils.getDigestOrFail(file, fileSize), fileSize); } public Digest compute(VirtualActionInput input) throws IOException { diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java index ab3bc4286e..9b9f85e42c 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java @@ -44,7 +44,6 @@ import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; -import java.util.Collection; import java.util.List; import org.junit.Before; import org.junit.Test; @@ -161,7 +160,7 @@ public class AbstractSpawnStrategyTest { // Must only be called exactly once. verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionContext.class)); - verify(entry).store(eq(spawnResult), any(Collection.class)); + verify(entry).store(eq(spawnResult)); } @SuppressWarnings("unchecked") @@ -192,7 +191,7 @@ public class AbstractSpawnStrategyTest { } // Must only be called exactly once. verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionContext.class)); - verify(entry).store(eq(result), any(Collection.class)); + verify(entry).store(eq(result)); } @Test 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 new file mode 100644 index 0000000000..821bb17a94 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java @@ -0,0 +1,95 @@ +// 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 static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.ExecException; +import com.google.devtools.build.lib.clock.JavaClock; +import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest; +import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystem.HashFunction; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.remoteexecution.v1test.ActionResult; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link AbstractRemoteActionCache}. */ +@RunWith(JUnit4.class) +public class AbstractRemoteActionCacheTests { + + private FileSystem fs; + private Path execRoot; + private final DigestUtil digestUtil = new DigestUtil(HashFunction.SHA256); + + @Before + public void setUp() throws Exception { + fs = new InMemoryFileSystem(new JavaClock(), HashFunction.SHA256); + execRoot = fs.getPath("/execroot"); + execRoot.createDirectory(); + } + + @Test + public void uploadSymlinkAsFile() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path link = fs.getPath("/execroot/link"); + Path target = fs.getPath("/execroot/target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + link.createSymbolicLink(target); + + UploadManifest um = new UploadManifest(digestUtil, result, execRoot, true); + um.addFiles(ImmutableList.of(link)); + assertThat(um.getDigestToFile()).containsExactly(digestUtil.compute(target), link); + + assertThat( + assertThrows( + ExecException.class, + () -> + new UploadManifest(digestUtil, result, execRoot, false) + .addFiles(ImmutableList.of(link)))) + .hasMessageThat() + .contains("Only regular files and directories may be uploaded to a remote cache."); + } + + @Test + public void uploadSymlinkInDirectory() throws Exception { + ActionResult.Builder result = ActionResult.newBuilder(); + Path dir = fs.getPath("/execroot/dir"); + dir.createDirectory(); + Path target = fs.getPath("/execroot/target"); + FileSystemUtils.writeContent(target, new byte[] {1, 2, 3, 4, 5}); + Path link = fs.getPath("/execroot/dir/link"); + link.createSymbolicLink(fs.getPath("/execroot/target")); + + UploadManifest um = new UploadManifest(digestUtil, result, fs.getPath("/execroot"), true); + um.addFiles(ImmutableList.of(link)); + assertThat(um.getDigestToFile()).containsExactly(digestUtil.compute(target), link); + + assertThat( + assertThrows( + ExecException.class, + () -> + new UploadManifest(digestUtil, result, execRoot, false) + .addFiles(ImmutableList.of(link)))) + .hasMessageThat() + .contains("Only regular files and directories may be uploaded to a remote cache."); + } +} 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 8baaa26564..cae9840216 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 @@ -169,7 +169,7 @@ public class RemoteSpawnCacheTest { ImmutableMap.of("VARIABLE", "value"), /*executionInfo=*/ ImmutableMap.<String, String>of(), /*inputs=*/ ImmutableList.of(ActionInputHelper.fromPath("input")), - /*outputs=*/ ImmutableList.<ActionInput>of(), + /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout"); @@ -273,7 +273,7 @@ public class RemoteSpawnCacheTest { }) .when(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); - entry.store(result, outputFiles); + entry.store(result); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); assertThat(progressUpdates) @@ -285,14 +285,15 @@ public class RemoteSpawnCacheTest { // Checks that spawns that have mayBeCached false are not looked up in the remote cache, // and also that their result is not uploaded to the remote cache. The artifacts, however, // are uploaded. - SimpleSpawn uncacheableSpawn = new SimpleSpawn( - new FakeOwner("foo", "bar"), - /*arguments=*/ ImmutableList.of(), - /*environment=*/ ImmutableMap.of(), - ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""), - /*inputs=*/ ImmutableList.of(), - /*outputs=*/ ImmutableList.<ActionInput>of(), - ResourceSet.ZERO); + SimpleSpawn uncacheableSpawn = + new SimpleSpawn( + new FakeOwner("foo", "bar"), + /*arguments=*/ ImmutableList.of(), + /*environment=*/ ImmutableMap.of(), + ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""), + /*inputs=*/ ImmutableList.of(), + /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")), + ResourceSet.ZERO); CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy); verify(remoteCache, never()) .getCachedActionResult(any(ActionKey.class)); @@ -303,8 +304,8 @@ public class RemoteSpawnCacheTest { .setStatus(Status.SUCCESS) .setRunnerName("test") .build(); + entry.store(result); ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file")); - entry.store(result, outputFiles); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false)); assertThat(progressUpdates).containsExactly(); @@ -324,7 +325,7 @@ public class RemoteSpawnCacheTest { .setRunnerName("test") .build(); ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file")); - entry.store(result, outputFiles); + entry.store(result); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false)); assertThat(progressUpdates) @@ -347,7 +348,7 @@ public class RemoteSpawnCacheTest { .when(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); - entry.store(result, outputFiles); + entry.store(result); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); 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 0f241649cb..9ef1add565 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 @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.vfs.FileSystem.HashFunction; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.common.options.Options; import com.google.devtools.remoteexecution.v1test.ActionResult; import com.google.devtools.remoteexecution.v1test.Digest; import com.google.devtools.remoteexecution.v1test.Directory; @@ -74,7 +75,8 @@ public class SimpleBlobStoreActionCacheTest { } private SimpleBlobStoreActionCache newClient(ConcurrentMap<String, byte[]> map) { - return new SimpleBlobStoreActionCache(new ConcurrentMapBlobStore(map), DIGEST_UTIL); + return new SimpleBlobStoreActionCache( + Options.getDefaults(RemoteOptions.class), new ConcurrentMapBlobStore(map), DIGEST_UTIL); } @Test diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh index fa21adab53..215ea74219 100755 --- a/src/test/shell/bazel/remote/remote_execution_http_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh @@ -127,6 +127,56 @@ EOF fi } +function test_refuse_to_upload_symlink() { + cat > BUILD <<'EOF' +genrule( + name = 'make-link', + outs = ['l', 't'], + cmd = 'touch $(location t) && ln -s t $(location l)', +) +EOF + bazel build \ + --genrule_strategy=remote \ + --noremote_allow_symlink_upload \ + --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ + //:make-link &> $TEST_log \ + && fail "should have failed" || true + expect_log "/l is a symbolic link" + + bazel build \ + --experimental_remote_spawn_cache \ + --noremote_allow_symlink_upload \ + --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ + //:make-link &> $TEST_log \ + && fail "should have failed" || true + expect_log "/l is a symbolic link" +} + +function test_refuse_to_upload_symlink_in_directory() { + cat > BUILD <<'EOF' +genrule( + name = 'make-link', + outs = ['dir'], + cmd = 'mkdir $(location dir) && touch $(location dir)/t && ln -s t $(location dir)/l', +) +EOF + bazel build \ + --genrule_strategy=remote \ + --noremote_allow_symlink_upload \ + --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ + //:make-link &> $TEST_log \ + && fail "should have failed" || true + expect_log "dir/l is a symbolic link" + + bazel build \ + --experimental_remote_spawn_cache \ + --noremote_allow_symlink_upload \ + --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ + //:make-link &> $TEST_log \ + && fail "should have failed" || true + expect_log "dir/l is a symbolic link" +} + function set_directory_artifact_testfixtures() { mkdir -p a cat > a/BUILD <<'EOF' diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 277b882c6d..ecbddb6532 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -34,6 +34,7 @@ function set_up() { --work_path="${work_path}" \ --listen_port=${worker_port} \ --cas_path=${cas_path} \ + --noremote_allow_symlink_upload \ --pid_file="${pid_file}" >& $TEST_log & local wait_seconds=0 until [ -s "${pid_file}" ] || [ "$wait_seconds" -eq 15 ]; do @@ -522,6 +523,23 @@ EOF expect_log "Remote connection/protocol failed" } +function test_refuse_symlink_output() { + cat > BUILD <<'EOF' +genrule( + name = 'make-link', + outs = ['l', 't'], + cmd = 'touch $(location t) && ln -s t $(location l)', +) +EOF + + bazel build \ + --genrule_strategy=remote \ + --remote_executor=localhost:${worker_port} \ + //:make-link >& TEST_log \ + && fail "should have failed"# || true + expect_log "/l is a symbolic link" +} + # TODO(alpha): Add a test that fails remote execution when remote worker # supports sandbox. diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index e9ea69df1d..0912b1f375 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -24,6 +24,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; +import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.remote.CacheNotFoundException; import com.google.devtools.build.lib.remote.ExecutionStatusException; import com.google.devtools.build.lib.remote.SimpleBlobStoreActionCache; @@ -252,6 +253,7 @@ final class ExecutionServer extends ExecutionImplBase { (cmdResult != null && cmdResult.getTerminationStatus().timedOut()) || wasTimeout(timeoutMillis, System.currentTimeMillis() - startTime); final int exitCode; + Status errStatus = null; ExecuteResponse.Builder resp = ExecuteResponse.newBuilder(); if (wasTimeout) { final String errMessage = @@ -259,11 +261,11 @@ final class ExecutionServer extends ExecutionImplBase { "Command:\n%s\nexceeded deadline of %f seconds.", Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0); logger.warning(errMessage); - resp.setStatus( + errStatus = Status.newBuilder() .setCode(Code.DEADLINE_EXCEEDED.getNumber()) .setMessage(errMessage) - .build()); + .build(); exitCode = LOCAL_EXEC_ERROR; } else if (cmdResult == null) { exitCode = LOCAL_EXEC_ERROR; @@ -272,19 +274,29 @@ final class ExecutionServer extends ExecutionImplBase { } ActionResult.Builder result = ActionResult.newBuilder(); - cache.upload(result, execRoot, outputs); + try { + cache.upload(result, execRoot, outputs); + } catch (ExecException e) { + if (errStatus == null) { + errStatus = + Status.newBuilder() + .setCode(Code.FAILED_PRECONDITION.getNumber()) + .setMessage(e.getMessage()) + .build(); + } + } byte[] stdout = cmdResult.getStdout(); byte[] stderr = cmdResult.getStderr(); cache.uploadOutErr(result, stdout, stderr); ActionResult finalResult = result.setExitCode(exitCode).build(); - if (exitCode == 0 && !action.getDoNotCache()) { + resp.setResult(finalResult); + if (errStatus != null) { + resp.setStatus(errStatus); + throw new ExecutionStatusException(errStatus, resp.build()); + } else if (exitCode == 0 && !action.getDoNotCache()) { ActionKey actionKey = digestUtil.computeActionKey(action); cache.setCachedActionResult(actionKey, finalResult); } - resp.setResult(finalResult); - if (wasTimeout) { - throw new ExecutionStatusException(resp.getStatus(), resp.build()); - } return finalResult; } 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 5bf8cbbad9..a55d2d9a6a 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 @@ -272,7 +272,7 @@ public final class RemoteWorker { new RemoteWorker( fs, remoteWorkerOptions, - new SimpleBlobStoreActionCache(blobStore, digestUtil), + new SimpleBlobStoreActionCache(remoteOptions, blobStore, digestUtil), sandboxPath, digestUtil); |