Skip to content

Commit

Permalink
Also handle remote cache eviction for tree artifacts.
Browse files Browse the repository at this point in the history
Related to #17366.

PiperOrigin-RevId: 511849585
Change-Id: I1c4acd469280628b15441a8e7a97ff572c42c1ff
  • Loading branch information
tjgq authored and copybara-github committed Feb 23, 2023
1 parent c65860b commit 32e4f23
Show file tree
Hide file tree
Showing 7 changed files with 80 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -264,7 +265,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)) {
Expand Down Expand Up @@ -300,6 +302,7 @@ private Completable prefetchInputTree(
Context context,
MetadataProvider provider,
PathFragment execPath,
SpecialArtifact tree,
List<TreeFileArtifact> treeFiles,
FileArtifactValue treeMetadata,
Priority priority) {
Expand Down Expand Up @@ -366,6 +369,12 @@ private Completable prefetchInputTree(

completed.set(true);
})
.doOnError(
error -> {
if (BulkTransferException.anyCausedByCacheNotFoundException(error)) {
missingActionInputs.add(tree);
}
})
.doFinally(
() -> {
if (!completed.get()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ protected ListenableFuture<Void> 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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,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 = Utils.grpcAwareErrorMessage(e, verboseFailures);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -516,8 +516,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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -512,4 +512,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());
}
}

0 comments on commit 32e4f23

Please sign in to comment.