Skip to content

Commit

Permalink
Fixes #6219. Don't rethrow any remote cache failures on either downlo…
Browse files Browse the repository at this point in the history 
…ad or upload, only warn. Added more tests.

TESTED=new unit tests
RELNOTES: Fix regression #6219, remote cache failures
PiperOrigin-RevId: 214614941
  • Loading branch information
olaola authored and Copybara-Service committed Sep 26, 2018
1 parent f469574 commit bf6a63d
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 13 deletions.
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.UserExecException;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.remote.Retrier.RetryException;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.Utils;
Expand Down Expand Up @@ -69,7 +68,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable {
((SettableFuture<byte[]>) EMPTY_BYTES).set(new byte[0]);
}

public static boolean causedByCacheMiss(RetryException t) {
public static boolean causedByCacheMiss(IOException t) {
return t.getCause() instanceof CacheNotFoundException;
}

Expand Down
18 changes: 7 additions & 11 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import com.google.devtools.build.lib.exec.SpawnCache;
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.remote.Retrier.RetryException;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
Expand Down Expand Up @@ -138,18 +137,15 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
.build();
return SpawnCache.success(spawnResult);
}
} catch (RetryException e) {
if (!AbstractRemoteActionCache.causedByCacheMiss(e)) {
throw e;
}
// There's a cache miss. Fall back to local execution.
} catch (IOException e) {
String errorMsg = e.getMessage();
if (isNullOrEmpty(errorMsg)) {
errorMsg = e.getClass().getSimpleName();
if (!AbstractRemoteActionCache.causedByCacheMiss(e)) {
String errorMsg = e.getMessage();
if (isNullOrEmpty(errorMsg)) {
errorMsg = e.getClass().getSimpleName();
}
errorMsg = "Error reading from the remote cache:\n" + errorMsg;
report(Event.warn(errorMsg));
}
errorMsg = "Error reading from the remote cache:\n" + errorMsg;
report(Event.warn(errorMsg));
} finally {
withMetadata.detach(previous);
}
Expand Down
125 changes: 125 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import build.bazel.remote.execution.v2.Action;
import build.bazel.remote.execution.v2.ActionResult;
import build.bazel.remote.execution.v2.Command;
import build.bazel.remote.execution.v2.Digest;
import build.bazel.remote.execution.v2.OutputFile;
import build.bazel.remote.execution.v2.RequestMetadata;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand All @@ -49,6 +51,7 @@
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.exec.util.FakeOwner;
import com.google.devtools.build.lib.remote.Retrier.RetryException;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
Expand Down Expand Up @@ -409,4 +412,126 @@ public void printWarningIfUploadFails() throws Exception {
assertThat(progressUpdates)
.containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
}

@Test
public void printWarningIfDownloadFails() throws Exception {
doThrow(new RetryException("reason", 0, io.grpc.Status.UNAVAILABLE.asRuntimeException()))
.when(remoteCache)
.getCachedActionResult(any(ActionKey.class));

CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy);
assertThat(entry.hasResult()).isFalse();
SpawnResult result =
new SpawnResult.Builder()
.setExitCode(0)
.setStatus(Status.SUCCESS)
.setRunnerName("test")
.build();
ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));

Mockito.doAnswer(
new Answer<Void>() {
@Override
public Void answer(InvocationOnMock invocation) {
RequestMetadata meta = TracingMetadataUtils.fromCurrentContext();
assertThat(meta.getCorrelatedInvocationsId()).isEqualTo("build-req-id");
assertThat(meta.getToolInvocationId()).isEqualTo("command-id");
return null;
}
})
.when(remoteCache)
.upload(
any(ActionKey.class),
any(Action.class),
any(Command.class),
any(Path.class),
eq(outputFiles),
eq(outErr),
eq(true));
entry.store(result);
verify(remoteCache)
.upload(
any(ActionKey.class),
any(Action.class),
any(Command.class),
any(Path.class),
eq(outputFiles),
eq(outErr),
eq(true));

assertThat(eventHandler.getEvents()).hasSize(1);
Event evt = eventHandler.getEvents().get(0);
assertThat(evt.getKind()).isEqualTo(EventKind.WARNING);
assertThat(evt.getMessage()).contains("Error");
assertThat(evt.getMessage()).contains("reading");
assertThat(evt.getMessage()).contains("reason");
assertThat(progressUpdates)
.containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
}

@Test
public void orphanedCachedResultIgnored() throws Exception {
Digest digest = digestUtil.computeAsUtf8("bla");
ActionResult actionResult =
ActionResult.newBuilder()
.addOutputFiles(OutputFile.newBuilder().setPath("/random/file").setDigest(digest))
.build();
when(remoteCache.getCachedActionResult(any(ActionKey.class)))
.thenAnswer(
new Answer<ActionResult>() {
@Override
public ActionResult answer(InvocationOnMock invocation) {
RequestMetadata meta = TracingMetadataUtils.fromCurrentContext();
assertThat(meta.getCorrelatedInvocationsId()).isEqualTo("build-req-id");
assertThat(meta.getToolInvocationId()).isEqualTo("command-id");
return actionResult;
}
});
doThrow(new RetryException("cache miss", 0, new CacheNotFoundException(digest, digestUtil)))
.when(remoteCache)
.download(actionResult, execRoot, outErr);

CacheHandle entry = cache.lookup(simpleSpawn, simplePolicy);
assertThat(entry.hasResult()).isFalse();
SpawnResult result =
new SpawnResult.Builder()
.setExitCode(0)
.setStatus(Status.SUCCESS)
.setRunnerName("test")
.build();
ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));

Mockito.doAnswer(
new Answer<Void>() {
@Override
public Void answer(InvocationOnMock invocation) {
RequestMetadata meta = TracingMetadataUtils.fromCurrentContext();
assertThat(meta.getCorrelatedInvocationsId()).isEqualTo("build-req-id");
assertThat(meta.getToolInvocationId()).isEqualTo("command-id");
return null;
}
})
.when(remoteCache)
.upload(
any(ActionKey.class),
any(Action.class),
any(Command.class),
any(Path.class),
eq(outputFiles),
eq(outErr),
eq(true));
entry.store(result);
verify(remoteCache)
.upload(
any(ActionKey.class),
any(Action.class),
any(Command.class),
any(Path.class),
eq(outputFiles),
eq(outErr),
eq(true));
assertThat(progressUpdates)
.containsExactly(Pair.of(ProgressStatus.CHECKING_CACHE, "remote-cache"));
assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed.
}
}

0 comments on commit bf6a63d

Please sign in to comment.