add directory support for remote caching and execution #4011

hchauvin · 2017-11-02T14:45:22Z

Add support for directory trees as artifacts

bazel-io · 2017-11-02T14:45:24Z

Can one of the admins verify this patch?

hchauvin · 2017-11-02T14:56:56Z

This commit makes artifact directories usable in a remote context both for caching and execution. I realise this is quite an unwieldy commit, but it resolves all the TODOs I found in the code revolving around directories in com.google.devtools.build.lib.remote. I also took this as an opportunity to add some unit tests for the SimpleBlobStoreActionCache, which this commit modifies.

Basically, this is simply an implementation hinted at by @buchgr augmented by some changes in TreeNodeRepository.

One thing I am not happy with if the necessity of stating all the input files to check whether they are directories or not, this information, to my knowledge, not being provided anywhere else (however, some TODOs algorithms point at some improvements to the diffing algorithm, but this is out of the scope of this commit). I also don't have enough knowledge of the code base to know whether everything is optimized as it could be.

buchgr · 2017-11-03T22:44:50Z

wooohoooo 🎆 ... let me review this 😀

buchgr · 2017-11-03T22:45:10Z

OK to test

hchauvin · 2017-11-04T11:13:34Z

Test //src/test/shell/bazel:empty_package_test failed on Darwin because: "Xcode version must be specified to use an Apple CROSSTOOL". Test //scripts/release:release_test timed out. Both pass locally.

jgavris · 2017-11-06T23:22:32Z

src/main/java/com/google/devtools/build/lib/remote/RemoteActionCacheBase.java

+        }
+      } else {
+        downloadBlob(digest, path);
+        System.err.println("DOWNLOADED path " + path + ": " +


Was testing this because I need it too :) I think you want to pull this out.

thanks I completely forgot this one!

rahul-malik · 2017-11-28T15:34:21Z

@buchgr @hchauvin - Whats the current status of this PR?

kamalmarhubi · 2017-11-28T15:46:58Z

Also curious. It'd be great if this could land in 0.9 or 0.10!

@ulfjack

@ulfjack @buchgr - I'm resubmitting #3984 on behalf of @mterring to get past CLA issues that are holding it up from merging. This is a temporary fix for the issue #3891 while we wait for #4011 to be reviewed and tested Closes #4188. PiperOrigin-RevId: 177276751

@ulfjack

@ulfjack @buchgr - I'm resubmitting bazelbuild#3984 on behalf of @mterring to get past CLA issues that are holding it up from merging. This is a temporary fix for the issue bazelbuild#3891 while we wait for bazelbuild#4011 to be reviewed and tested Closes bazelbuild#4188. PiperOrigin-RevId: 177276751

buchgr · 2017-12-01T15:50:42Z

I apologize I lost track of it. I ll review it Monday.

@rahul-malik @kamalmarhubi did you test it on your iOS project with remote caching? Does it work?

hchauvin · 2017-12-02T09:17:19Z

Hi, thanks for the update @buchgr.

@rahul-malik @kamalmarhubi thanks for the interest. Currently we're using it in production for a workspace that uses lots of target directories, both for remote caching and remote execution. Using directories is the only sane way we found for dealing with external npm dependencies (JavaScript) for our frontend (whether it is a good idea or not can be discussed, but it is another subject :)), so we cache whole node_modules folders without problem.

I followed Jakob's blueprint on how this should be implemented using Merkle trees and the remote API. I took this as an opportunity to increase test coverage and to deduplicate a bit the code for remote caching and execution in order to not have the logic for dealing with directories in multiple places.

Things that might need some input:

Symlinks. Currently, I don't follow them when caching directories. I isolated the policy in a constant, TreeNodeRepository.symlinkPolicy, to make that clear. In practice, this is not likely to be a problem because if your project has symlinks going out of target directories, you should do something concerning the way the project is organized. I don't know anyway whether doing something other than Symlinks.NOFOLLOW makes sense for directory caching. It's also worth pointing out that if the symlinks are "local" to the cached directory, nothing is broken. If the symlinks exit the cached directory... it depends :)
Speed. Testing whether each input file is a directory or not has a cost. I don't see any noticeable slowdown however, and when doing remote caching checking the stats of local files is probably not your biggest concern.

hchauvin · 2017-12-02T09:37:34Z

Thinking about it the change might be "big" enough to warrant writing an integration test in src/test/shell/bazel/remote_execution_test.sh, what do you think @buchgr ?

buchgr · 2017-12-03T16:00:40Z

@hchauvin Yes please ... an integration test would be great :-)!

buchgr

Looks good! This is great work @hchauvin! Thank you.

buchgr · 2017-12-04T10:50:36Z

src/main/java/com/google/devtools/build/lib/remote/RemoteActionCacheBase.java

+ * and {@link GrpcRemoteCache}.
+ */
+@ThreadSafety.ThreadSafe
+public abstract class RemoteActionCacheBase implements RemoteActionCache {


It seems to me that you could just remove the RemoteActionCache interface and rename RemoteActionCacheBase to AbstractRemoteActionCache (it's a naming convention used throughout Bazel).

Having an abstract class with abstract upload/download methods seems like a good abstraction, that renders the interface unnecessary.

buchgr · 2017-12-04T11:37:38Z

src/tools/remote_worker/src/main/java/com/google/devtools/build/remote/ExecutionServer.java

@@ -188,7 +188,14 @@ private ActionResult execute(Action action, Path execRoot)
      FileSystemUtils.createDirectoryAndParents(file.getParentDirectory());
      outputs.add(file);
    }
-    // TODO(olaola): support output directories.
+    for (String output : action.getOutputDirectoriesList()) {


An integration testing both the client and remote worker implementation would be great!

buchgr · 2017-12-04T11:42:33Z

src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java

+    if (digest == null) {
+      // If the artifact does not have a digest, it is because it is a directory.
+      // We get the digest from the set of Merkle hashes computed in this TreeNodeRepository.
+      return Preconditions.checkNotNull(inputDirectoryDigestCache.get(input));


can you please add a message stating what went wrong. makes errors easier to debug.

buchgr · 2017-12-04T11:44:45Z

src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java

@@ -105,15 +122,22 @@ public int hashCode() {
    }

    // Should only be called by the TreeNodeRepository.
-    private TreeNode(Iterable<ChildEntry> childEntries) {
-      this.actionInput = null;
+    private TreeNode(Iterable<ChildEntry> childEntries, ActionInput actionInput) {


nit: @Nullable

buchgr · 2017-12-04T11:47:29Z

src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java

+    List<Dirent> sortedDirent = new ArrayList<>(path.readdir(symlinkPolicy));
+    sortedDirent.sort(Comparator.comparing(Dirent::getName));
+
+    List<TreeNode.ChildEntry> entries = new ArrayList<>();


nit: initialize with sortedDirent.size().

hchauvin · 2017-12-05T11:04:40Z

@buchgr I took your comments, added 3 integration tests and rebased.

buchgr · 2017-12-06T12:00:26Z

src/test/shell/bazel/remote_execution_test.sh

+function test_directory_artifact() {
+  set_directory_artifact_testfixtures
+
+  bazel --host_jvm_args=-Dbazel.DigestFunction=SHA1 build \


this should no longer be needed after you rebase.

Add support for directory trees as artifacts

hchauvin · 2017-12-06T15:55:07Z

@buchgr got it. Rebased again and removed digest function.

buchgr · 2017-12-08T14:01:15Z

Thanks again! It's in the internal review! Should be exported soon!

kamalmarhubi · 2017-12-09T21:44:57Z

@hchauvin this is awesome!

@buchgr any chance this could be cherry picked into 0.9? Would make a great holiday gift... :-)

rahul-malik · 2017-12-18T17:40:55Z

@buchgr - Did this pass internal review?

buchgr · 2017-12-20T17:30:33Z

Sorry for the delay. It's merged finally! Will be in 0.10.0

Add support for directory trees as artifacts. Closes bazelbuild#4011. PiperOrigin-RevId: 179691001

@ulfjack

@ulfjack @buchgr - I'm resubmitting bazelbuild/bazel#3984 on behalf of @mterring to get past CLA issues that are holding it up from merging. This is a temporary fix for the issue bazelbuild/bazel#3891 while we wait for bazelbuild/bazel#4011 to be reviewed and tested Closes #4188. PiperOrigin-RevId: 177276751

googlebot added the cla: yes label Nov 2, 2017

katre added category: service APIs type: feature request P2 We'll consider working on this in future. (Assignee optional) labels Nov 2, 2017

buchgr self-requested a review November 3, 2017 22:43

buchgr self-assigned this Nov 3, 2017

jgavris reviewed Nov 6, 2017

View reviewed changes

rahul-malik mentioned this pull request Nov 29, 2017

Change error on adding a directory to the remote cache to be non-fatal #4188

Closed

buchgr suggested changes Dec 4, 2017

View reviewed changes

buchgr reviewed Dec 6, 2017

View reviewed changes

hchauvin added 5 commits December 6, 2017 16:31

add directory support for remote caching and execution

f9b8687

Add support for directory trees as artifacts

@buchgr comments

1e81a75

integration tests

60d8f3a

resolve conflicts and adapt to DigestUtil API change

08a57fe

remove -Dbazel.DigestFunction=SHA1

4762dde

bazel-io closed this in 3d0a04d Dec 20, 2017

buchgr mentioned this pull request Dec 20, 2017

Support Directories in SimpleBlobStoreActionCache #3891

Closed

werkt pushed a commit to werkt/bazel that referenced this pull request Mar 2, 2018

remote: add directory support for remote caching and execution

49948a2

Add support for directory trees as artifacts. Closes bazelbuild#4011. PiperOrigin-RevId: 179691001

werkt pushed a commit to werkt/bazel that referenced this pull request Mar 2, 2018

remote: add directory support for remote caching and execution

ee3363b

Add support for directory trees as artifacts. Closes bazelbuild#4011. PiperOrigin-RevId: 179691001

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add directory support for remote caching and execution #4011

add directory support for remote caching and execution #4011

hchauvin commented Nov 2, 2017

bazel-io commented Nov 2, 2017

hchauvin commented Nov 2, 2017

buchgr commented Nov 3, 2017

buchgr commented Nov 3, 2017

hchauvin commented Nov 4, 2017

jgavris Nov 6, 2017

hchauvin Nov 7, 2017

rahul-malik commented Nov 28, 2017

kamalmarhubi commented Nov 28, 2017

buchgr commented Dec 1, 2017

hchauvin commented Dec 2, 2017

hchauvin commented Dec 2, 2017

buchgr commented Dec 3, 2017

buchgr left a comment

buchgr Dec 4, 2017

buchgr Dec 4, 2017

buchgr Dec 4, 2017

buchgr Dec 4, 2017

buchgr Dec 4, 2017

hchauvin commented Dec 5, 2017

buchgr Dec 6, 2017

hchauvin commented Dec 6, 2017 •

edited

Loading

buchgr commented Dec 8, 2017

kamalmarhubi commented Dec 9, 2017

rahul-malik commented Dec 18, 2017

buchgr commented Dec 20, 2017

add directory support for remote caching and execution #4011

add directory support for remote caching and execution #4011

Conversation

hchauvin commented Nov 2, 2017

bazel-io commented Nov 2, 2017

hchauvin commented Nov 2, 2017

buchgr commented Nov 3, 2017

buchgr commented Nov 3, 2017

hchauvin commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul-malik commented Nov 28, 2017

kamalmarhubi commented Nov 28, 2017

buchgr commented Dec 1, 2017

hchauvin commented Dec 2, 2017

hchauvin commented Dec 2, 2017

buchgr commented Dec 3, 2017

buchgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hchauvin commented Dec 5, 2017

Choose a reason for hiding this comment

hchauvin commented Dec 6, 2017 • edited Loading

buchgr commented Dec 8, 2017

kamalmarhubi commented Dec 9, 2017

rahul-malik commented Dec 18, 2017

buchgr commented Dec 20, 2017

hchauvin commented Dec 6, 2017 •

edited

Loading