Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.1.0] Also handle remote cache eviction for tree artifacts. #17601

Merged
merged 1 commit into from
Feb 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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)) {
Expand Down Expand Up @@ -288,6 +290,7 @@ private Completable prefetchInputTree(
Context context,
MetadataProvider provider,
PathFragment execPath,
SpecialArtifact tree,
List<TreeFileArtifact> treeFiles,
FileArtifactValue treeMetadata,
Priority priority) {
Expand Down Expand Up @@ -354,6 +357,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 @@ -104,7 +104,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 @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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 @@ -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) {
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 @@ -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());
}
}