Implement missing parts of OSS RE caching #477

cormacrelf · 2023-11-05T10:32:04Z

This enables action caching on OSS builds. There were two missing pieces:

write_action_result, for writing to the action cache. This was the major piece; without it, all you have is a bunch of uploaded CAS blobs for various things, but no way to find them again from the action digest. Now that the action results are uploaded as well, buck can find them when it's querying for cached actions based on the calculated digest alone.
upload_blob was a less-exercised piece of code, it was only used for uploading stdout and stderr, and even then, only if they were more than 50KiB. I have added a test for it, but not sure if internal is the right one for it to be in, or when these tests get run.

I've successfully used this implementation with BuildBuddy, both from Linux (using remote_enabled = True), and from macOS, with remote_enabled = False but caching still enabled. I have only tested it with RBE v2.3+ output_paths mode.

As an implementation note, I found it very difficult to even figure out that these parts weren't implemented. Buck2 was just saying "0% cache hits mate" and reporting that every re_action_cache (= attempts to fetch a cached action, not attempts to cache an executed action!) had failed. None of these failures were logged anywhere. I don't know what the appropriate mechanism to log this stuff is, but I whacked a tracing::warn for failed action result cache writes in there.

…ebuggable

cormacrelf · 2023-11-05T11:23:05Z

The TODOs around symlinking are related to #222

ndmitchell

Thanks so much for this, and the great testing you have done3. Two things I'm still figuring out:

Why didn't you get a warn - I've noted where I expected that to pop out. Is that line not being hit for you?
Still figuring out if there are internal implications for upload blob taking a digest, or if it should have done so all along.

ndmitchell · 2023-11-20T22:12:54Z

remote_execution/oss/re_grpc/src/client.rs

+            is_topologically_sorted: false,
+        })
+    })?;
+    anyhow::Ok(ActionResult {


Any reason to not just Ok?

It's just splatter from copy pasting the function below and inverting every operation, instead of coding it from scratch. I'll tidy this up, a few of these don't even need to be Result.

ndmitchell · 2023-11-20T22:13:57Z

remote_execution/oss/re_grpc/src/client.rs

+        || !action_result.output_file_symlinks.is_empty()
+        || !action_result.output_directory_symlinks.is_empty()
+    {
+        anyhow::bail!(


We tend to avoid bail because it can be hard to reason about - easier to see the explicit return

ndmitchell · 2023-11-20T22:14:32Z

remote_execution/oss/re_grpc/src/response.rs

@@ -91,6 +91,9 @@ pub struct TSubsysPerfCount {
 pub struct TActionResult2 {
    pub output_files: Vec<TFile>,
    pub output_directories: Vec<TDirectory2>,
+    // TODO: output_symlinks (use in preference when output_paths mode is used the execution side)
+    // TODO: output_file_symlinks (deprecated)


Why mark this as a todo? Is it something we should add? Or is this just for when servers return deprecated stuff?

ndmitchell · 2023-11-20T22:16:58Z

app/buck2_execute/src/re/client.rs

@@ -1141,6 +1143,9 @@ impl RemoteExecutionClientImpl {
                    ..Default::default()
                },
            )
+            .inspect_err(|err| {
+                tracing::warn!("write_action_result failed: {err}");


Does this not appear in https://github.com/facebook/buck2/blob/main/app/buck2_execute_impl/src/executors/caching.rs#L108C25-L108C25 as a warning? It certainly should

That line may have run, but I definitely did not see it in the terminal. Thanks for the pointer, I will have a look at why.

ndmitchell · 2023-11-21T13:21:32Z

Having upload blob take a digest shouldn't be a big deal since I think the amount of data flowing through this API isn't big enough to matter. I can see why this is more convenient and matches the upload API better.

However, once we are going through that path, maybe we should just delete upload_blob entirely and reuse upload? That already takes a InlinedBlobWithDigest so then we could have fewer functions?

cormacrelf · 2023-11-22T02:06:30Z

However, once we are going through that path, maybe we should just delete upload_blob entirely and reuse upload? That already takes a InlinedBlobWithDigest so then we could have fewer functions?

There is a lot of "code overhead" for each piece of GRPC client API you want to expose. There are about 5 layers of nesting that, for fn upload_blob, add exactly nothing at each layer. Each layer basically does "return this.field.upload_blob(...)". So it would certainly be economical to delete one of those function stacks. However I imagine there are opportunities to collapse the nesting and make it less costly to maintain in the first place. I might report back on that.

ndmitchell · 2023-11-22T13:20:52Z

Some of these layers of forwarding are so we can inject the internal closed source RE client (closed source because it uses a lot of systems that aren't open source, rather than because it has magical properties). They might need to stay (or might not).

benbrittain · 2024-02-07T12:47:34Z

Is there anything I can do here to help push this to being landed? I'm happy to finish the PR if you'd like @cormacrelf

cormacrelf · 2024-02-11T04:17:04Z

Hi @benbrittain, I haven't had any time to work on this recently. I've given you push access to my fork cormacrelf/buck2, so you can keep working on the rbe-writes branch.

miaoyipu · 2024-05-01T20:28:20Z

Is there anything I can do here to help push this to being landed? I'm happy to finish the PR if you'd like @cormacrelf

@cormacrelf @benbrittain any update on this PR? This might help our RE execution efficiency, and I can help if no one is working on it.

benbrittain · 2024-05-01T20:32:22Z

Hey @miaoyipu! It's on my near term TODO but I'm certainly not against you picking it up. If you do so though, let me know :)

avdv · 2024-05-10T06:43:57Z

Hi @cormacrelf

I've successfully used this implementation with BuildBuddy, both from Linux (using remote_enabled = True), and from macOS, with remote_enabled = False but caching still enabled. I have only tested it with RBE v2.3+ output_paths mode.

I am also trying to use this with Buildbuddy and disabled remote_enabled on Linux but I cannot get it to work. Could you share your config so I can compare that to mine? Thank you!

\edit: Never mind, it's working now. Thank you for the implementation @cormacrelf!

cormacrelf added 5 commits November 5, 2023 20:50

log on write_action_result failure, because these were completely und…

3c96e77

…ebuggable

implement write_action_result

966ded2

bail if ActionResult has symlinks in it

ca99697

Implement upload_blob

4fc77c1

Add a large_stdout test, exercising upload_blob

00dafa8

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 5, 2023

cormacrelf mentioned this pull request Nov 5, 2023

Should allow_cache_upload be available on more rules? #478

Open

ndmitchell reviewed Nov 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement missing parts of OSS RE caching #477

Implement missing parts of OSS RE caching #477

cormacrelf commented Nov 5, 2023

cormacrelf commented Nov 5, 2023

ndmitchell left a comment

ndmitchell Nov 20, 2023

cormacrelf Nov 21, 2023

ndmitchell Nov 20, 2023

ndmitchell Nov 20, 2023

ndmitchell Nov 20, 2023

cormacrelf Nov 21, 2023

ndmitchell commented Nov 21, 2023

cormacrelf commented Nov 22, 2023

ndmitchell commented Nov 22, 2023

benbrittain commented Feb 7, 2024

cormacrelf commented Feb 11, 2024

miaoyipu commented May 1, 2024

benbrittain commented May 1, 2024

avdv commented May 10, 2024 •

edited

Implement missing parts of OSS RE caching #477

Are you sure you want to change the base?

Implement missing parts of OSS RE caching #477

Conversation

cormacrelf commented Nov 5, 2023

cormacrelf commented Nov 5, 2023

ndmitchell left a comment

Choose a reason for hiding this comment

ndmitchell Nov 20, 2023

Choose a reason for hiding this comment

cormacrelf Nov 21, 2023

Choose a reason for hiding this comment

ndmitchell Nov 20, 2023

Choose a reason for hiding this comment

ndmitchell Nov 20, 2023

Choose a reason for hiding this comment

ndmitchell Nov 20, 2023

Choose a reason for hiding this comment

cormacrelf Nov 21, 2023

Choose a reason for hiding this comment

ndmitchell commented Nov 21, 2023

cormacrelf commented Nov 22, 2023

ndmitchell commented Nov 22, 2023

benbrittain commented Feb 7, 2024

cormacrelf commented Feb 11, 2024

miaoyipu commented May 1, 2024

benbrittain commented May 1, 2024

avdv commented May 10, 2024 • edited

avdv commented May 10, 2024 •

edited