From 68e1924cdab69ab92b8acf2f6e9324d11e00b267 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 27 Feb 2023 16:48:13 +0100 Subject: [PATCH] Also handle remote cache eviction for tree artifacts. (#17601) Related to #17366. PiperOrigin-RevId: 511849585 Change-Id: I1c4acd469280628b15441a8e7a97ff572c42c1ff Co-authored-by: Googler --- .../remote/AbstractActionInputPrefetcher.java | 11 ++++- .../build/lib/remote/ExecuteRetrier.java | 2 +- .../lib/remote/RemoteActionInputFetcher.java | 2 +- .../build/lib/remote/RemoteSpawnCache.java | 2 +- .../build/lib/remote/RemoteSpawnRunner.java | 7 ++- .../remote/common/BulkTransferException.java | 19 +++++-- .../BuildWithoutTheBytesIntegrationTest.java | 49 +++++++++++++++++++ 7 files changed, 80 insertions(+), 12 deletions(-) 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 c79ffd2162daef..478d30d5b4c44e 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 @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; @@ -252,7 +253,8 @@ private Completable prefetchInputTreeOrSymlink( PathFragment prefetchExecPath = treeMetadata.getMaterializationExecPath().orElse(execPath); Completable prefetch = - prefetchInputTree(context, provider, prefetchExecPath, treeFiles, treeMetadata, priority); + prefetchInputTree( + context, provider, prefetchExecPath, tree, treeFiles, treeMetadata, priority); // If prefetching to a different path, plant a symlink into it. if (!prefetchExecPath.equals(execPath)) { @@ -288,6 +290,7 @@ private Completable prefetchInputTree( Context context, MetadataProvider provider, PathFragment execPath, + SpecialArtifact tree, List treeFiles, FileArtifactValue treeMetadata, Priority priority) { @@ -354,6 +357,12 @@ private Completable prefetchInputTree( completed.set(true); }) + .doOnError( + error -> { + if (BulkTransferException.anyCausedByCacheNotFoundException(error)) { + missingActionInputs.add(tree); + } + }) .doFinally( () -> { if (!completed.get()) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/ExecuteRetrier.java b/src/main/java/com/google/devtools/build/lib/remote/ExecuteRetrier.java index 8bdeccdeeca2d1..ae4eab8737295a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ExecuteRetrier.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ExecuteRetrier.java @@ -89,7 +89,7 @@ public int getRetryAttempts() { } private static boolean shouldRetry(Exception e) { - if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) { + if (BulkTransferException.allCausedByCacheNotFoundException(e)) { return true; } Status status = StatusProto.fromThrowable(e); 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 20c739f4499bdd..bd482a2d9da823 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 @@ -104,7 +104,7 @@ protected ListenableFuture doDownloadFile( @Override protected Completable onErrorResumeNext(Throwable error) { if (error instanceof BulkTransferException) { - if (((BulkTransferException) error).onlyCausedByCacheNotFoundException()) { + if (((BulkTransferException) error).allCausedByCacheNotFoundException()) { var code = useNewExitCodeForLostInputs ? Code.REMOTE_CACHE_EVICTED : Code.REMOTE_CACHE_FAILED; error = 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 9d168d446736ef..fd4205bf2925c0 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 @@ -138,7 +138,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) } catch (CacheNotFoundException e) { // Intentionally left blank } catch (IOException e) { - if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) { + if (BulkTransferException.allCausedByCacheNotFoundException(e)) { // Intentionally left blank } else { String errorMessage; 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 49f2b6b73295ae..5b6622fbb0023d 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 @@ -225,7 +225,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) () -> action.getNetworkTime().getDuration(), spawnMetrics); } catch (BulkTransferException e) { - if (!e.onlyCausedByCacheNotFoundException()) { + if (!e.allCausedByCacheNotFoundException()) { throw e; } // No cache hit, so we fall through to local or remote execution. @@ -293,7 +293,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) () -> action.getNetworkTime().getDuration(), spawnMetrics); } catch (BulkTransferException e) { - if (e.onlyCausedByCacheNotFoundException()) { + if (e.allCausedByCacheNotFoundException()) { // No cache hit, so if we retry this execution, we must no longer accept // cached results, it must be reexecuted useCachedResult.set(false); @@ -517,8 +517,7 @@ private SpawnResult execLocallyAndUploadOrFail( private SpawnResult handleError( RemoteAction action, IOException exception, SpawnExecutionContext context) throws ExecException, InterruptedException, IOException { - boolean remoteCacheFailed = - BulkTransferException.isOnlyCausedByCacheNotFoundException(exception); + boolean remoteCacheFailed = BulkTransferException.allCausedByCacheNotFoundException(exception); if (exception.getCause() instanceof ExecutionStatusException) { ExecutionStatusException e = (ExecutionStatusException) exception.getCause(); if (e.getResponse() != null) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java b/src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java index 723151462912a6..03b3a448121025 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/BulkTransferException.java @@ -23,8 +23,9 @@ * with all constituent exceptions available for observation. */ public class BulkTransferException extends IOException { - // true since no empty BulkTransferException is ever thrown + // No empty BulkTransferException is ever thrown. private boolean allCacheNotFoundException = true; + private boolean anyCacheNotFoundException = false; public BulkTransferException() {} @@ -40,16 +41,26 @@ public BulkTransferException(IOException e) { */ public void add(IOException e) { allCacheNotFoundException &= e instanceof CacheNotFoundException; + anyCacheNotFoundException |= e instanceof CacheNotFoundException; super.addSuppressed(e); } - public boolean onlyCausedByCacheNotFoundException() { + public boolean anyCausedByCacheNotFoundException() { + return anyCacheNotFoundException; + } + + public static boolean anyCausedByCacheNotFoundException(Throwable e) { + return e instanceof BulkTransferException + && ((BulkTransferException) e).anyCausedByCacheNotFoundException(); + } + + public boolean allCausedByCacheNotFoundException() { return allCacheNotFoundException; } - public static boolean isOnlyCausedByCacheNotFoundException(Exception e) { + public static boolean allCausedByCacheNotFoundException(Throwable e) { return e instanceof BulkTransferException - && ((BulkTransferException) e).onlyCausedByCacheNotFoundException(); + && ((BulkTransferException) e).allCausedByCacheNotFoundException(); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index 2e086170e34e81..94ef104b381cef 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java @@ -513,4 +513,53 @@ public void remoteCacheEvictBlobs_incrementalBuildCanContinue() throws Exception // Assert: target was successfully built assertValidOutputFile("a/bar.out", "foo" + lineSeparator() + "updated bar" + lineSeparator()); } + + @Test + public void remoteCacheEvictBlobs_treeArtifact_incrementalBuildCanContinue() throws Exception { + // Arrange: Prepare workspace and populate remote cache + write("BUILD"); + writeOutputDirRule(); + write( + "a/BUILD", + "load('//:output_dir.bzl', 'output_dir')", + "output_dir(", + " name = 'foo.out',", + " manifest = ':manifest',", + ")", + "genrule(", + " name = 'bar',", + " srcs = ['foo.out', 'bar.in'],", + " outs = ['bar.out'],", + " cmd = '( ls $(location :foo.out); cat $(location :bar.in) ) > $@',", + " tags = ['no-remote-exec'],", + ")"); + write("a/manifest", "file-inside"); + write("a/bar.in", "bar"); + + // Populate remote cache + buildTarget("//a:bar"); + getOutputPath("a/foo.out").deleteTreesBelow(); + getOutputPath("a/bar.out").delete(); + getOutputBase().getRelative("action_cache").deleteTreesBelow(); + restartServer(); + + // Clean build, foo.out isn't downloaded + buildTarget("//a:bar"); + assertOutputDoesNotExist("a/foo.out/file-inside"); + + // Evict blobs from remote cache + worker.restart(); + + // trigger build error + write("a/bar.in", "updated bar"); + // Build failed because of remote cache eviction + assertThrows(BuildFailedException.class, () -> buildTarget("//a:bar")); + + // Act: Do an incremental build without "clean" or "shutdown" + buildTarget("//a:bar"); + + // Assert: target was successfully built + assertValidOutputFile( + "a/bar.out", "file-inside" + lineSeparator() + "updated bar" + lineSeparator()); + } }