From 484ffae78ff592fa44c2da9b131c1437800c8283 Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Wed, 19 Jun 2019 06:04:12 -0700 Subject: [PATCH] remote: enable bes for --experimental_remote_download_outputs=(minimal|toplevel) --experimental_remote_download_outputs=(minimal|toplevel) This is the last patch to enable BEP/BES file URI rewriting when being used with --experimental_remote_download_outputs=(minimal|toplevel). Closes #8669. PiperOrigin-RevId: 253981837 --- .../ByteStreamBuildEventArtifactUploader.java | 8 ++ .../lib/remote/RemoteActionFileSystem.java | 9 ++- .../build/lib/remote/RemoteModule.java | 26 ++----- .../build/lib/remote/RemoteOutputService.java | 29 ++++++- ...eStreamBuildEventArtifactUploaderTest.java | 75 +++++++++++++++++++ .../bazel/remote/remote_execution_test.sh | 36 +++++++++ 6 files changed, 161 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index f5d6f4b5f45e01..8b25fddddbe50e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -68,6 +68,11 @@ class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader Executors.newFixedThreadPool(Math.min(maxUploadThreads, 1000))); } + private static boolean isRemoteFile(Path file) { + return file.getFileSystem() instanceof RemoteActionFileSystem + && ((RemoteActionFileSystem) file.getFileSystem()).isRemote(file); + } + @Override public ListenableFuture upload(Map files) { if (files.isEmpty()) { @@ -86,6 +91,9 @@ public ListenableFuture upload(Map files) { } DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction()); Digest digest = digestUtil.compute(file); + if (isRemoteFile(file)) { + return Futures.immediateFuture(new PathDigestPair(file, digest)); + } Chunker chunker = Chunker.builder().setInput(digest.getSizeBytes(), file).build(); final ListenableFuture upload; Context prevCtx = ctx.attach(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index 0491c2d9546a54..fbdcee74f2ee4b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -40,14 +40,14 @@ * *

This implementation only supports creating local action outputs. */ -public class RemoteActionFileSystem extends DelegateFileSystem { +class RemoteActionFileSystem extends DelegateFileSystem { private final Path execRoot; private final Path outputBase; private final ActionInputMap inputArtifactData; private final RemoteActionInputFetcher inputFetcher; - public RemoteActionFileSystem( + RemoteActionFileSystem( FileSystem localDelegate, PathFragment execRootFragment, String relativeOutputPath, @@ -61,6 +61,11 @@ public RemoteActionFileSystem( this.inputFetcher = Preconditions.checkNotNull(inputFetcher, "inputFetcher"); } + /** Returns true if {@code path} is a file that's stored remotely. */ + boolean isRemote(Path path) { + return getRemoteInputMetadata(path) != null; + } + @Override public String getFileSystemType(Path path) { return "remoteActionFS"; 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 0a64207846782c..b09163c50e30a0 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 @@ -255,24 +255,14 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { digestUtil, uploader.retain()); uploader.release(); - if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) { - Context requestContext = - TracingMetadataUtils.contextWithMetadata(buildRequestId, invocationId, "bes-upload"); - buildEventArtifactUploaderFactoryDelegate.init( - new ByteStreamBuildEventArtifactUploaderFactory( - uploader, - cacheChannel.authority(), - requestContext, - remoteOptions.remoteInstanceName)); - } else { - // TODO(buchgr): Fix BES local file upload to work with - // --experimental_remote_download_outputs - env.getReporter() - .handle( - Event.warn( - "BES artifact upload is disabled when using " - + "--experimental_remote_download_outputs=minimal")); - } + Context requestContext = + TracingMetadataUtils.contextWithMetadata(buildRequestId, invocationId, "bes-upload"); + buildEventArtifactUploaderFactoryDelegate.init( + new ByteStreamBuildEventArtifactUploaderFactory( + uploader, + cacheChannel.authority(), + requestContext, + remoteOptions.remoteInstanceName)); } if (enableBlobStoreCache) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java index 29a3ef1e167b41..bb607e189036bd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.vfs.BatchStat; @@ -29,6 +30,8 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; +import java.util.Collection; +import java.util.Map; import java.util.UUID; import java.util.function.Function; import javax.annotation.Nullable; @@ -113,7 +116,29 @@ public void clean() { } @Override - public boolean isRemoteFile(Artifact file) { - return false; + public boolean isRemoteFile(Artifact artifact) { + Path path = artifact.getPath(); + return path.getFileSystem() instanceof RemoteActionFileSystem + && ((RemoteActionFileSystem) path.getFileSystem()).isRemote(path); + } + + @Override + public boolean supportsPathResolverForArtifactValues() { + return actionFileSystemType() != ActionFileSystemType.DISABLED; + } + + @Override + public ArtifactPathResolver createPathResolverForArtifactValues( + PathFragment execRoot, + String relativeOutputPath, + FileSystem fileSystem, + ImmutableList pathEntries, + ActionInputMap actionInputMap, + Map> expandedArtifacts, + Iterable filesets) { + FileSystem remoteFileSystem = + new RemoteActionFileSystem( + fileSystem, execRoot, relativeOutputPath, actionInputMap, actionInputFetcher); + return ArtifactPathResolver.createPathResolver(remoteFileSystem, fileSystem.getPath(execRoot)); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 110f3cbd1e035d..00053cbee1d0ac 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -16,14 +16,22 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoMoreInteractions; import build.bazel.remote.execution.v2.Digest; import com.google.bytestream.ByteStreamProto.WriteRequest; import com.google.bytestream.ByteStreamProto.WriteResponse; +import com.google.common.collect.ImmutableMap; import com.google.common.hash.HashCode; import com.google.common.io.BaseEncoding; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.actions.ActionInputMap; +import com.google.devtools.build.lib.actions.Artifact; +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.util.ActionsTestUtil; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType; import com.google.devtools.build.lib.buildeventstream.PathConverter; @@ -46,6 +54,7 @@ import io.grpc.stub.StreamObserver; import io.grpc.util.MutableHandlerRegistry; import java.io.OutputStream; +import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; import java.util.Random; @@ -59,6 +68,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; /** Test for {@link ByteStreamBuildEventArtifactUploader}. */ @@ -76,6 +86,9 @@ public class ByteStreamBuildEventArtifactUploaderTest { private Context prevContext; private final FileSystem fs = new InMemoryFileSystem(new JavaClock(), DigestHashFunction.SHA256); + private final Path execRoot = fs.getPath("/execroot"); + private ArtifactRoot outputRoot; + @BeforeClass public static void beforeEverything() { retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); @@ -98,6 +111,9 @@ public final void setUp() throws Exception { // Needs to be repeated in every test that uses the timeout setting, since the tests run // on different threads than the setUp. prevContext = withEmptyMetadata.attach(); + + outputRoot = ArtifactRoot.asDerivedRoot(execRoot, execRoot.getRelative("out")); + outputRoot.getRoot().asPath().createDirectoryAndParents(); } @After @@ -247,4 +263,63 @@ public void onCompleted() { assertThat(uploader.refCnt()).isEqualTo(0); assertThat(refCntChannel.isShutdown()).isTrue(); } + + @Test + public void remoteFileShouldNotBeUploaded() throws Exception { + // Test that we don't attempt to upload remotely stored file but convert the remote path + // to a bytestream:// URI. + + // arrange + + ByteStreamUploader uploader = Mockito.mock(ByteStreamUploader.class); + RemoteActionInputFetcher actionInputFetcher = Mockito.mock(RemoteActionInputFetcher.class); + ByteStreamBuildEventArtifactUploader artifactUploader = + new ByteStreamBuildEventArtifactUploader( + uploader, "localhost", withEmptyMetadata, "instance", /* maxUploadThreads= */ 100); + + ActionInputMap outputs = new ActionInputMap(2); + Artifact artifact = createRemoteArtifact("file1.txt", "foo", outputs); + + RemoteActionFileSystem remoteFs = + new RemoteActionFileSystem( + fs, + execRoot.asFragment(), + outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(), + outputs, + actionInputFetcher); + Path remotePath = remoteFs.getPath(artifact.getPath().getPathString()); + assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs); + LocalFile file = new LocalFile(remotePath, LocalFileType.OUTPUT); + + // act + + PathConverter pathConverter = artifactUploader.upload(ImmutableMap.of(remotePath, file)).get(); + + FileArtifactValue metadata = outputs.getMetadata(artifact); + Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); + + // assert + + String conversion = pathConverter.apply(remotePath); + assertThat(conversion) + .isEqualTo( + "bytestream://localhost/instance/blobs/" + + digest.getHash() + + "/" + + digest.getSizeBytes()); + verifyNoMoreInteractions(uploader); + } + + /** Returns a remote artifact and puts its metadata into the action input map. */ + private Artifact createRemoteArtifact( + String pathFragment, String contents, ActionInputMap inputs) { + Path p = outputRoot.getRoot().asPath().getRelative(pathFragment); + Artifact a = ActionsTestUtil.createArtifact(outputRoot, p); + byte[] b = contents.getBytes(StandardCharsets.UTF_8); + HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash()); + FileArtifactValue f = + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ 1); + inputs.putWithNoDepOwner(a, f); + return a; + } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 95478cb57cb5b7..5de687eb85c25c 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1061,6 +1061,42 @@ EOF } + +function test_downloads_minimal_bep() { + # Test that when using --experimental_remote_download_outputs=minimal all URI's in the BEP + # are rewritten as bytestream://.. + mkdir -p a + cat > a/success.sh <<'EOF' +#!/bin/sh +exit 0 +EOF + chmod 755 a/success.sh + cat > a/BUILD <<'EOF' +sh_test( + name = "success_test", + srcs = ["success.sh"], +) + +genrule( + name = "foo", + srcs = [], + outs = ["foo.txt"], + cmd = "echo \"foo\" > \"$@\"", +) +EOF + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_inmemory_jdeps_files \ + --experimental_inmemory_dotd_files \ + --experimental_remote_download_outputs=minimal \ + --build_event_text_file=$TEST_log \ + //a:foo //a:success_test || fail "Failed to test //a:foo //a:success_test" + + expect_not_log 'uri:.*file://' + expect_log "uri:.*bytestream://localhost" +} + # TODO(alpha): Add a test that fails remote execution when remote worker # supports sandbox.