Implement renameTo and createDirectory[AndParents] for RemoteActionFi…

…leSystem Fixes bazelbuild#16351.
coeuvre · Oct 17, 2022 · 7b42fc6 · 7b42fc6
1 parent 2876569
commit 7b42fc6
Show file tree

Hide file tree

Showing 3 changed files with 188 additions and 0 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -19,6 +19,7 @@
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static com.google.common.collect.Streams.stream;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -86,6 +87,16 @@ public class RemoteActionFileSystem extends DelegateFileSystem {
     this.remoteOutputTree = new RemoteInMemoryFileSystem(getDigestFunction());
   }
 
+  @VisibleForTesting
+  protected RemoteInMemoryFileSystem getRemoteOutputTree() {
+    return remoteOutputTree;
+  }
+
+  @VisibleForTesting
+  protected FileSystem getLocalFileSystem() {
+    return delegateFs;
+  }
+
   /** Returns true if {@code path} is a file that's stored remotely. */
   boolean isRemote(Path path) {
     return getRemoteInputMetadata(path.asFragment()) != null;
@@ -438,6 +449,47 @@ private void downloadFileIfRemote(PathFragment path) throws IOException {
     }
   }
 
+  @Override
+  public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IOException {
+    FileNotFoundException remoteException = null;
+    try {
+      remoteOutputTree.renameTo(sourcePath, targetPath);
+    } catch (FileNotFoundException e) {
+      remoteException = e;
+    }
+
+    FileNotFoundException localException = null;
+    try {
+      delegateFs.renameTo(sourcePath, targetPath);
+    } catch (FileNotFoundException e) {
+      localException = e;
+    }
+
+    if (remoteException == null || localException == null) {
+      return;
+    }
+
+    localException.addSuppressed(remoteException);
+    throw localException;
+  }
+
+  @Override
+  public void createDirectoryAndParents(PathFragment path) throws IOException {
+    super.createDirectoryAndParents(path);
+    if (path.startsWith(outputBase)) {
+      remoteOutputTree.createDirectoryAndParents(path);
+    }
+  }
+
+  @Override
+  public boolean createDirectory(PathFragment path) throws IOException {
+    boolean created = delegateFs.createDirectory(path);
+    if (path.startsWith(outputBase)) {
+      created = remoteOutputTree.createDirectory(path) || created;
+    }
+    return created;
+  }
+
   /*
    * -------------------- TODO(buchgr): Not yet implemented --------------------
    *

diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.remote;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
@@ -34,9 +35,11 @@
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.lib.vfs.SyscallCache;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import org.junit.Before;
@@ -159,6 +162,107 @@ public void testDeleteLocalFile() throws Exception {
     assertThat(actionFs.exists(path.asFragment())).isFalse();
   }
 
+  @Test
+  public void renameTo_fileDoesNotExist_throwError() throws Exception {
+    // arrange
+    RemoteActionFileSystem actionFs = newRemoteActionFileSystem();
+    PathFragment path = outputRoot.getRoot().asPath().getRelative("file").asFragment();
+    PathFragment newPath = outputRoot.getRoot().asPath().getRelative("file-new").asFragment();
+
+    assertThrows(FileNotFoundException.class, () -> actionFs.renameTo(path, newPath));
+  }
+
+  @Test
+  public void renameTo_onlyRemoteFile_renameRemoteFile() throws Exception {
+    // arrange
+    RemoteActionFileSystem actionFs = newRemoteActionFileSystem();
+    PathFragment path = outputRoot.getRoot().asPath().getRelative("file").asFragment();
+    injectRemoteFile(actionFs, path, "remote-content");
+    PathFragment newPath = outputRoot.getRoot().asPath().getRelative("file-new").asFragment();
+
+    actionFs.renameTo(path, newPath);
+
+    assertThat(actionFs.exists(path)).isFalse();
+    assertThat(actionFs.exists(newPath)).isTrue();
+    assertThat(actionFs.getRemoteOutputTree().exists(path)).isFalse();
+    assertThat(actionFs.getRemoteOutputTree().exists(newPath)).isTrue();
+  }
+
+  @Test
+  public void renameTo_onlyLocalFile_renameLocalFile() throws Exception {
+    // arrange
+    RemoteActionFileSystem actionFs = newRemoteActionFileSystem();
+    PathFragment path = outputRoot.getRoot().asPath().getRelative("file").asFragment();
+    writeLocalFile(actionFs, path, "local-content");
+    PathFragment newPath = outputRoot.getRoot().asPath().getRelative("file-new").asFragment();
+
+    actionFs.renameTo(path, newPath);
+
+    assertThat(actionFs.exists(path)).isFalse();
+    assertThat(actionFs.exists(newPath)).isTrue();
+    assertThat(actionFs.getLocalFileSystem().exists(path)).isFalse();
+    assertThat(actionFs.getLocalFileSystem().exists(newPath)).isTrue();
+  }
+
+  @Test
+  public void renameTo_localAndRemoteFile_renameBoth() throws Exception {
+    // arrange
+    RemoteActionFileSystem actionFs = newRemoteActionFileSystem();
+    PathFragment path = outputRoot.getRoot().asPath().getRelative("file").asFragment();
+    injectRemoteFile(actionFs, path, "remote-content");
+    writeLocalFile(actionFs, path, "local-content");
+    PathFragment newPath = outputRoot.getRoot().asPath().getRelative("file-new").asFragment();
+
+    actionFs.renameTo(path, newPath);
+
+    assertThat(actionFs.exists(path)).isFalse();
+    assertThat(actionFs.exists(newPath)).isTrue();
+    assertThat(actionFs.getRemoteOutputTree().exists(path)).isFalse();
+    assertThat(actionFs.getRemoteOutputTree().exists(newPath)).isTrue();
+    assertThat(actionFs.getLocalFileSystem().exists(path)).isFalse();
+    assertThat(actionFs.getLocalFileSystem().exists(newPath)).isTrue();
+  }
+
+  @Test
+  public void createDirectoryAndParents_createLocallyAndRemotely() throws Exception {
+    RemoteActionFileSystem actionFs = newRemoteActionFileSystem();
+    PathFragment path = outputRoot.getRoot().asPath().getRelative("dir").asFragment();
+
+    actionFs.createDirectoryAndParents(path);
+
+    assertThat(actionFs.getRemoteOutputTree().getPath(path).isDirectory()).isTrue();
+    assertThat(actionFs.getLocalFileSystem().getPath(path).isDirectory()).isTrue();
+  }
+
+  @Test
+  public void createDirectory_createLocallyAndRemotely() throws Exception {
+    RemoteActionFileSystem actionFs = newRemoteActionFileSystem();
+    actionFs.createDirectoryAndParents(outputRoot.getRoot().asPath().asFragment());
+    PathFragment path = outputRoot.getRoot().asPath().getRelative("dir").asFragment();
+
+    actionFs.createDirectory(path);
+
+    assertThat(actionFs.getRemoteOutputTree().getPath(path).isDirectory()).isTrue();
+    assertThat(actionFs.getLocalFileSystem().getPath(path).isDirectory()).isTrue();
+  }
+
+  private void injectRemoteFile(RemoteActionFileSystem actionFs, PathFragment path, String content)
+      throws IOException {
+    byte[] contentBytes = content.getBytes(StandardCharsets.UTF_8);
+    HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentBytes);
+    actionFs.injectRemoteFile(path, hashCode.asBytes(), contentBytes.length, "action-id");
+  }
+
+  private void writeLocalFile(RemoteActionFileSystem actionFs, PathFragment path, String content)
+      throws IOException {
+    FileSystemUtils.writeContent(actionFs.getPath(path), StandardCharsets.UTF_8, content);
+  }
+
+  private RemoteActionFileSystem newRemoteActionFileSystem() {
+    ActionInputMap inputs = new ActionInputMap(0);
+    return newRemoteActionFileSystem(inputs);
+  }
+
   private RemoteActionFileSystem newRemoteActionFileSystem(ActionInputMap inputs) {
     return newRemoteActionFileSystem(inputs, ImmutableList.of());
   }

diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -572,6 +572,38 @@ EOF
   expect_log "test.outputs_manifest__MANIFEST"
 }
 
+function test_multiple_test_attempts() {
+  # Test that test logs of multiple test attempts can be renamed and reported by
+  # BEP.
+  mkdir -p a
+  cat > a/BUILD <<EOF
+sh_test(
+  name = "foo",
+  srcs = ["foo.sh"],
+)
+EOF
+  cat > a/foo.sh <<'EOF'
+exit 1
+EOF
+  chmod a+x a/foo.sh
+  rm -rf $TEST_TMPDIR/bep.json
+
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    --build_event_json_file=$TEST_TMPDIR/bep.json \
+    --flaky_test_attempts=2 \
+    //a:foo >& $TEST_log && fail "Test //a:foo should fail"
+
+  expect_log "FAILED in 2 out of 2"
+  expect_log "a/foo/test.log"
+  expect_log "a/foo/test_attempts/attempt_1.log"
+  cat $TEST_TMPDIR/bep.json > $TEST_log
+  rm -rf $TEST_TMPDIR/bep.json
+  expect_log "attempt\":1.*test.log.*bytestream.*test.xml.*bytestream"
+  expect_log "attempt\":2.*test.log.*bytestream.*test.xml.*bytestream"
+}
+
 # This test is derivative of test_bep_output_groups in
 # build_event_stream_test.sh, which verifies that successful output groups'
 # artifacts appear in BEP when a top-level target fails to build.