From 280ef6915b0f507218a073974825d6aa7effddee Mon Sep 17 00:00:00 2001 From: chiwang Date: Mon, 9 May 2022 08:20:19 -0700 Subject: [PATCH] Remote: Prefetch input files into a temporary path first. When building with build without bytes and dynamic execution, we need prefetch input files for local actions. Sometimes, multiple local actions could share the same input files, so there could be a case where multiple call sites share the same download instance. If the local action is cancelled (due to remote branch wins), the download it requested should also be cancelled only if that download is not shared with other local action (or all the releated local actions are cancelled). Before this change, the inputs are written to their final destination directly. This is fine if we can make sure no race or bug in the prefetcher. However, this is not true: https://github.com/bazelbuild/bazel/issues/15010. The root cause is, when cancelling the downloads, sometimes, the partially downloaded files on the disk are not deleted. By making the prefetcher download input to a temporary path first, we can: 1. Mitigate the race: only the final move step will potentially cause the race condition. 2. Provide a way to observe the race: if these is no race, all temporary files should be either moved or deleted. But when running with this change, many temporary files exist. Working towards https://github.com/bazelbuild/bazel/issues/12454. PiperOrigin-RevId: 447473693 --- .../remote/AbstractActionInputPrefetcher.java | 49 ++++++++---- .../lib/remote/RemoteActionInputFetcher.java | 9 ++- .../build/lib/remote/RemoteModule.java | 21 +++++- .../lib/remote/util/TempPathGenerator.java | 40 ++++++++++ .../remote/RemoteActionInputFetcherTest.java | 74 ++++++++++++------- 5 files changed, 149 insertions(+), 44 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/remote/util/TempPathGenerator.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index c0d2981eca3d4a..836022b18681fe 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -33,11 +33,14 @@ import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; +import com.google.devtools.build.lib.remote.util.TempPathGenerator; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Flowable; import io.reactivex.rxjava3.functions.Function; import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; /** @@ -48,11 +51,13 @@ public abstract class AbstractActionInputPrefetcher implements ActionInputPrefet private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); private final AsyncTaskCache.NoResult downloadCache = AsyncTaskCache.NoResult.create(); + private final TempPathGenerator tempPathGenerator; protected final Path execRoot; - protected AbstractActionInputPrefetcher(Path execRoot) { + protected AbstractActionInputPrefetcher(Path execRoot, TempPathGenerator tempPathGenerator) { this.execRoot = execRoot; + this.tempPathGenerator = tempPathGenerator; } protected abstract boolean shouldDownloadInput( @@ -113,25 +118,41 @@ private Completable prefetchInput(MetadataProvider metadataProvider, ActionInput return downloadFileIfNot(path, (p) -> downloadInput(p, input, metadata)); } - /** Downloads file into the {@code path} with given downloader. */ + /** + * Downloads file into the {@code path} with given downloader. + * + *

The file will be written into a temporary file and moved to the final destination after the + * download finished. + */ protected Completable downloadFileIfNot( Path path, Function> downloader) { + AtomicBoolean completed = new AtomicBoolean(false); Completable download = - toCompletable(() -> downloader.apply(path), directExecutor()) - .doOnComplete(() -> finalizeDownload(path)) - .doOnError(error -> deletePartialDownload(path)) - .doOnDispose(() -> deletePartialDownload(path)); + Completable.using( + tempPathGenerator::generateTempPath, + tempPath -> + toCompletable(() -> downloader.apply(tempPath), directExecutor()) + .doOnComplete( + () -> { + finalizeDownload(tempPath, path); + completed.set(true); + }), + tempPath -> { + if (!completed.get()) { + deletePartialDownload(tempPath); + } + }, + // Set eager=false here because we want cleanup the download *after* upstream is + // disposed. + /* eager= */ false); return downloadCache.executeIfNot(path, download); } - private void finalizeDownload(Path path) { - try { - // The permission of output file is changed to 0555 after action execution. We manually change - // the permission here for the downloaded file to keep this behaviour consistent. - path.chmod(0555); - } catch (IOException e) { - logger.atWarning().withCause(e).log("Failed to chmod 555 on %s", path); - } + private void finalizeDownload(Path tmpPath, Path path) throws IOException { + // The permission of output file is changed to 0555 after action execution. We manually change + // the permission here for the downloaded file to keep this behaviour consistent. + tmpPath.chmod(0555); + FileSystemUtils.moveFile(tmpPath, path); } private void deletePartialDownload(Path path) { 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 index ff40c8bda114d2..eb8d255039105f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcher.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TempPathGenerator; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.sandbox.SandboxHelpers; import com.google.devtools.build.lib.vfs.Path; @@ -49,8 +50,12 @@ class RemoteActionInputFetcher extends AbstractActionInputPrefetcher { private final RemoteCache remoteCache; RemoteActionInputFetcher( - String buildRequestId, String commandId, RemoteCache remoteCache, Path execRoot) { - super(execRoot); + String buildRequestId, + String commandId, + RemoteCache remoteCache, + Path execRoot, + TempPathGenerator tempPathGenerator) { + super(execRoot, tempPathGenerator); this.buildRequestId = Preconditions.checkNotNull(buildRequestId); this.commandId = Preconditions.checkNotNull(commandId); this.remoteCache = Preconditions.checkNotNull(remoteCache); 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 55c6ba1761cc89..afe2666401d41b 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 @@ -71,6 +71,7 @@ import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.remote.util.TempPathGenerator; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -906,7 +907,8 @@ public void registerActionContexts( } @Override - public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) { + public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorBuilder builder) + throws AbruptExitException { Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null"); Preconditions.checkNotNull(remoteOptions, "remoteOptions must not be null"); @@ -918,12 +920,27 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB env.getOptions().getOptions(RemoteOptions.class), "RemoteOptions"); RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode; if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) { + Path tempDir = env.getActionTempsDirectory().getChild("remote"); + try { + if (tempDir.exists() + && (!tempDir.isDirectory() || !tempDir.getDirectoryEntries().isEmpty())) { + env.getReporter() + .handle(Event.warn("Found incomplete downloads from previous build, deleting...")); + tempDir.deleteTree(); + } + } catch (IOException e) { + throw createExitException( + e.getMessage(), + ExitCode.LOCAL_ENVIRONMENTAL_ERROR, + Code.DOWNLOADED_INPUTS_DELETION_FAILURE); + } actionInputFetcher = new RemoteActionInputFetcher( env.getBuildRequestId(), env.getCommandId().toString(), actionContextProvider.getRemoteCache(), - env.getExecRoot()); + env.getExecRoot(), + new TempPathGenerator(tempDir)); builder.setActionInputPrefetcher(actionInputFetcher); remoteOutputService.setActionInputFetcher(actionInputFetcher); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/TempPathGenerator.java b/src/main/java/com/google/devtools/build/lib/remote/util/TempPathGenerator.java new file mode 100644 index 00000000000000..321d686248044f --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/remote/util/TempPathGenerator.java @@ -0,0 +1,40 @@ +// Copyright 2022 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.util; + +import com.google.common.annotations.VisibleForTesting; +import com.google.devtools.build.lib.vfs.Path; +import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.concurrent.ThreadSafe; + +/** A generator that generate temporary path under a given directory. */ +@ThreadSafe +public class TempPathGenerator { + private final Path tempDir; + private final AtomicInteger index = new AtomicInteger(); + + public TempPathGenerator(Path tempDir) { + this.tempDir = tempDir; + } + + /** Generates a temporary path */ + public Path generateTempPath() { + return tempDir.getChild(index.getAndIncrement() + ".tmp"); + } + + @VisibleForTesting + public Path getTempDir() { + return tempDir; + } +} diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java index 970b7c2d2e0c34..1d0d3dfcf1fe26 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionInputFetcherTest.java @@ -21,6 +21,7 @@ import static org.mockito.Mockito.when; import build.bazel.remote.execution.v2.Digest; +import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; @@ -43,6 +44,7 @@ import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.StaticMetadataProvider; +import com.google.devtools.build.lib.remote.util.TempPathGenerator; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -70,6 +72,7 @@ public class RemoteActionInputFetcherTest { private static final DigestHashFunction HASH_FUNCTION = DigestHashFunction.SHA256; private Path execRoot; + private TempPathGenerator tempPathGenerator; private ArtifactRoot artifactRoot; private RemoteOptions options; private DigestUtil digestUtil; @@ -79,6 +82,9 @@ public void setUp() throws IOException { FileSystem fs = new InMemoryFileSystem(new JavaClock(), HASH_FUNCTION); execRoot = fs.getPath("/exec"); execRoot.createDirectoryAndParents(); + Path tempDir = fs.getPath("/tmp"); + tempDir.createDirectoryAndParents(); + tempPathGenerator = new TempPathGenerator(tempDir); Path dev = fs.getPath("/dev"); dev.createDirectory(); dev.setWritable(false); @@ -98,7 +104,7 @@ public void testFetching() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); RemoteCache remoteCache = newCache(options, digestUtil, cacheEntries); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); // act wait(actionInputFetcher.prefetchFiles(metadata.keySet(), metadataProvider)); @@ -121,7 +127,7 @@ public void testStagingVirtualActionInput() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(new HashMap<>()); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); VirtualActionInput a = ActionsTestUtil.createVirtualActionInput("file1", "hello world"); // act @@ -141,7 +147,7 @@ public void testStagingEmptyVirtualActionInput() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(new HashMap<>()); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); // act wait( @@ -164,7 +170,7 @@ public void testFileNotFound() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); // act assertThrows( @@ -188,7 +194,7 @@ public void testIgnoreNoneRemoteFiles() throws Exception { MetadataProvider metadataProvider = new StaticMetadataProvider(ImmutableMap.of(a, f)); RemoteCache remoteCache = newCache(options, digestUtil, new HashMap<>()); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); // act wait(actionInputFetcher.prefetchFiles(ImmutableList.of(a), metadataProvider)); @@ -206,7 +212,7 @@ public void testDownloadFile() throws Exception { Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries); RemoteCache remoteCache = newCache(options, digestUtil, cacheEntries); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); // act actionInputFetcher.downloadFile(a1.getPath(), metadata.get(a1)); @@ -227,23 +233,15 @@ public void testDownloadFile_onInterrupt_deletePartialDownloadedFile() throws Ex Map cacheEntries = new HashMap<>(); Artifact a1 = createRemoteArtifact("file1", "hello world", metadata, cacheEntries); RemoteCache remoteCache = mock(RemoteCache.class); - when(remoteCache.downloadFile(any(), any(), any())) - .thenAnswer( - invocation -> { - Path path = invocation.getArgument(1); - Digest digest = invocation.getArgument(2); - ByteString content = cacheEntries.get(digest); - if (content == null) { - return Futures.immediateFailedFuture(new IOException("Not found")); - } - content.writeTo(path.getOutputStream()); - - startSemaphore.release(); - return SettableFuture - .create(); // A future that never complete so we can interrupt later - }); + mockDownload( + remoteCache, + cacheEntries, + () -> { + startSemaphore.release(); + return SettableFuture.create(); // A future that never complete so we can interrupt later + }); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); AtomicBoolean interrupted = new AtomicBoolean(false); Thread t = @@ -265,6 +263,7 @@ public void testDownloadFile_onInterrupt_deletePartialDownloadedFile() throws Ex assertThat(interrupted.get()).isTrue(); assertThat(a1.getPath().exists()).isFalse(); + assertThat(tempPathGenerator.getTempDir().getDirectoryEntries()).isEmpty(); } @Test @@ -279,9 +278,9 @@ public void testPrefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThrea MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); SettableFuture download = SettableFuture.create(); RemoteCache remoteCache = mock(RemoteCache.class); - when(remoteCache.downloadFile(any(), any(), any())).thenAnswer(invocation -> download); + mockDownload(remoteCache, cacheEntries, () -> download); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); Thread cancelledThread = new Thread( () -> { @@ -326,6 +325,8 @@ public void testPrefetchFiles_multipleThreads_downloadIsNotCancelledByOtherThrea // assert assertThat(successful.get()).isTrue(); + assertThat(FileSystemUtils.readContent(artifact.getPath(), StandardCharsets.UTF_8)) + .isEqualTo("hello world"); } @Test @@ -340,9 +341,9 @@ public void testPrefetchFiles_multipleThreads_downloadIsCancelled() throws Excep SettableFuture download = SettableFuture.create(); RemoteCache remoteCache = mock(RemoteCache.class); - when(remoteCache.downloadFile(any(), any(), any())).thenAnswer(invocation -> download); + mockDownload(remoteCache, cacheEntries, () -> download); RemoteActionInputFetcher actionInputFetcher = - new RemoteActionInputFetcher("none", "none", remoteCache, execRoot); + new RemoteActionInputFetcher("none", "none", remoteCache, execRoot, tempPathGenerator); Thread cancelledThread1 = new Thread( @@ -376,6 +377,8 @@ public void testPrefetchFiles_multipleThreads_downloadIsCancelled() throws Excep // assert assertThat(download.isCancelled()).isTrue(); + assertThat(artifact.getPath().exists()).isFalse(); + assertThat(tempPathGenerator.getTempDir().getDirectoryEntries()).isEmpty(); } private Artifact createRemoteArtifact( @@ -420,4 +423,23 @@ private static void wait(ListenableFuture future) throws IOException, Inte throw e; } } + + private static void mockDownload( + RemoteCache remoteCache, + Map cacheEntries, + Supplier> resultSupplier) + throws IOException { + when(remoteCache.downloadFile(any(), any(), any())) + .thenAnswer( + invocation -> { + Path path = invocation.getArgument(1); + Digest digest = invocation.getArgument(2); + ByteString content = cacheEntries.get(digest); + if (content == null) { + return Futures.immediateFailedFuture(new IOException("Not found")); + } + FileSystemUtils.writeContent(path, content.toByteArray()); + return resultSupplier.get(); + }); + } }