From f642b98efcb65a1d198b080f891b63b02033d771 Mon Sep 17 00:00:00 2001 From: Jakob Buchgraber Date: Mon, 17 Jun 2019 14:03:59 +0200 Subject: [PATCH] remote: make the dynamic spawn scheduler work. Fixes #8646 This change fixes the correctness issue of dynamic spawn scheduler when being used with remote execution. See #8646 for more details. There's a performance issue remaining: #8467 --- .../build/lib/remote/RemoteSpawnRunner.java | 4 ++++ .../lib/remote/RemoteSpawnRunnerTest.java | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) 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 3e57b5c989c693..297d61ae55a568 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 @@ -304,6 +304,10 @@ private SpawnResult downloadAndFinalizeSpawnResult( SpawnExecutionContext context, RemoteOutputsMode remoteOutputsMode) throws ExecException, IOException, InterruptedException { + // Ensure that when using dynamic spawn strategy that we are the only ones writing to the + // output files. See https://github.com/bazelbuild/bazel/issues/8646 for more details. + context.lockOutputFiles(); + boolean downloadOutputs = shouldDownloadAllSpawnOutputs( remoteOutputsMode, diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index d1fb3de3ce1505..8744e9a3d27f23 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -521,7 +521,7 @@ public void testNonHumanReadableServerLogsNotSaved() throws Exception { .build()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.NON_ZERO_EXIT); @@ -529,6 +529,7 @@ public void testNonHumanReadableServerLogsNotSaved() throws Exception { verify(executor).executeRemotely(any(ExecuteRequest.class)); verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class)); verify(cache, never()).downloadFile(any(Path.class), any(Digest.class)); + assertThat(policy.lockOutputFilesCalled).isTrue(); } @Test @@ -547,7 +548,7 @@ public void testServerLogsNotSavedForSuccessfulAction() throws Exception { .build()); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); SpawnResult res = runner.exec(spawn, policy); assertThat(res.status()).isEqualTo(Status.SUCCESS); @@ -555,6 +556,7 @@ public void testServerLogsNotSavedForSuccessfulAction() throws Exception { verify(executor).executeRemotely(any(ExecuteRequest.class)); verify(cache).download(eq(result), eq(execRoot), any(FileOutErr.class)); verify(cache, never()).downloadFile(any(Path.class), any(Digest.class)); + assertThat(policy.lockOutputFilesCalled).isTrue(); } @Test @@ -866,7 +868,7 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); // act SpawnResult result = runner.exec(spawn, policy); @@ -877,6 +879,7 @@ public void testDownloadMinimalOnCacheMiss() throws Exception { verify(executor).executeRemotely(any()); verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr)); + assertThat(policy.lockOutputFilesCalled).isTrue(); } @Test @@ -893,7 +896,7 @@ public void testDownloadMinimalIoError() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(); Spawn spawn = newSimpleSpawn(); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); // act SpawnExecException e = assertThrows(SpawnExecException.class, () -> runner.exec(spawn, policy)); @@ -902,6 +905,7 @@ public void testDownloadMinimalIoError() throws Exception { // assert verify(cache).downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); verify(cache, never()).download(any(ActionResult.class), any(Path.class), eq(outErr)); + assertThat(policy.lockOutputFilesCalled).isTrue(); } @Test @@ -920,7 +924,7 @@ public void testDownloadTopLevel() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput)); Spawn spawn = newSimpleSpawn(topLevelOutput); - SpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); + FakeSpawnExecutionContext policy = new FakeSpawnExecutionContext(spawn); // act SpawnResult result = runner.exec(spawn, policy); @@ -931,6 +935,7 @@ public void testDownloadTopLevel() throws Exception { verify(cache).download(eq(succeededAction), any(Path.class), eq(outErr)); verify(cache, never()) .downloadMinimal(eq(succeededAction), anyCollection(), any(), any(), any(), any()); + assertThat(policy.lockOutputFilesCalled).isTrue(); } private static Spawn newSimpleSpawn(Artifact... outputs) { @@ -998,6 +1003,8 @@ private RemoteSpawnRunner newSpawnRunnerWithoutExecutor(Reporter reporter) { // TODO(buchgr): Extract a common class to be used for testing. class FakeSpawnExecutionContext implements SpawnExecutionContext { + private boolean lockOutputFilesCalled; + private final ArtifactExpander artifactExpander = (artifact, output) -> output.add(artifact); private final Spawn spawn; @@ -1018,7 +1025,7 @@ public void prefetchInputs() { @Override public void lockOutputFiles() { - throw new UnsupportedOperationException(); + lockOutputFilesCalled = true; } @Override