Add build info across all requests #502

vaskomitanov · 2023-11-19T07:40:44Z

No description provided.

facebook-github-bot · 2023-11-19T07:40:50Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot · 2023-11-19T09:54:33Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

cormacrelf · 2023-11-19T13:04:06Z

There is already a type defined by RBE for this, which #310 sends. You could pick up that PR and add the config flag that was requested.

thoughtpolice · 2023-11-19T17:46:21Z

Yes, sorry for not getting around to it lately. I think #310 is the right way forward, but note that PR is very incomplete. Most of the plumbing for the RBEv2 metadata isn't there, so I think a lot of refactorings need to go in, first.

vaskomitanov · 2023-11-19T18:36:02Z

#310 looks exactly what I needed, thanks for starting this effort @thoughtpolice. I will rebase my changes of yours and use the tool_invocation_id for this purpose.

ndmitchell · 2023-11-20T19:50:43Z

@vaskomitanov - are all your rebasings done now?

vaskomitanov · 2023-11-21T08:53:43Z

@vaskomitanov - are all your rebasings done now?

Yes. There are few fields not yet populated but even in this state we have all of the needed information for efficient scheduling for action execution and caching.
I can add more information in a subsequent PR.

thoughtpolice · 2023-11-21T21:01:15Z

Thanks for taking this over the finish line! Please note that @krallin did I have one important adjustment he wanted me to make in #310 (comment)

Could we preferably expose this via a config option rather than a build flag? We do use our internal RE in order to test the OSS build, and that doesn't build with fbcode_build, so this would break that testing.

That is, they do a build of the OSS version of Buck internally and test that. So, we need to refactor this part of the diff in particular to just use a buckconfig value instead of conditional compilation thru #[cfg]:

2eaa19c#diff-0bb786b03f4f35aef10055270239e2afbf4f4b0ba0fa214b38ffa858c4bc9e95R1257-R1323

Maybe we should call it:

[buck2_re_client]
use_fbcode_metadata = true

And then default it to false so OSS users don't have to even know about it.

vaskomitanov · 2023-11-27T22:00:00Z

[buck2_re_client]
use_fbcode_metadata = true
And then default it to false so OSS users don't have to even know about it.

OK, sorry for the wait (thanksgiving), I've made requested changes.
As a side note, to keep the current behavior, I've made use_fbcode_metadata= true. Can make it false by default if everybody thinks that should be the way to go.

Yashshiva75 · 2023-11-28T11:32:06Z

THIS IS A SAMPLE COMMENT

thoughtpolice · 2023-11-30T03:15:11Z

As a side note, to keep the current behavior, I've made use_fbcode_metadata= true. Can make it false by default if everybody thinks that should be the way to go.

IMO it should be turned off by default; after all, the code path in question is located inside the RPC client for the OSS Bazel API, so it's the exact path all OSS users rely on. This codepath should work as expected in OSS builds, i.e. with OSS metadata.

We could #[cfg] that choice, but I'm sure Meta can override it with a simple -c buck2_re_client.use_fbcode_metadata=true flag to buck2 in their test harness.

vaskomitanov · 2023-11-30T06:39:42Z

As a side note, to keep the current behavior, I've made use_fbcode_metadata= true. Can make it false by default if everybody thinks that should be the way to go.

IMO it should be turned off by default; after all, the code path in question is located inside the RPC client for the OSS Bazel API, so it's the exact path all OSS users rely on. This codepath should work as expected in OSS builds, i.e. with OSS metadata.

We could #[cfg] that choice, but I'm sure Meta can override it with a simple -c buck2_re_client.use_fbcode_metadata=true flag to buck2 in their test harness.

OK, done, default is "false"

vaskomitanov · 2023-12-02T18:45:03Z

@ndmitchell any additional steps I need to do to get this PR merged?

ndmitchell · 2023-12-15T15:39:51Z

Sorry for the delay - was travelling then got a bit ill. Just running through the code now.

JakobDegen

Sorry that this took so long to get to. Overall this looks right, just a couple of thoughts

app/buck2_execute_impl/src/executors/action_cache.rs

JakobDegen · 2024-02-08T10:35:55Z

app/buck2_execute/src/re/uploader.rs

+            let mut mtd = use_case.metadata();
+            if let Some(identity) = identity {
+                apply_identity(identity, &mut mtd);
+            }


Hm, so I'm not the expert on this code, but I'm not super clear on the difference between the cases where this is applied vs the cases where it isn't? Is there a reason that some of these are more important than others?

I would've expected the Option<&ReActionIdentity> to instead become a mandatory parameter to use_case.metadata(), so that call sites have to explicit ask for the None behavior instead of letting this go forgotten

Great question!
This PR covers the RE GRPC request sequence: find missing blobs -> upload missing blobs -> execute action -> download produced artifacts.
This way on the server side we can be smart when replicating CAS artifacts and dispatching actions (we will be able to correlateaction execution with preceeding artifact uploads).
Without this change we are simply receiving 100s of parallel find missing blobs requests, 100s of parallel upload missing blobs and execute action without any correlation between them (even though they are correlated).
Other cases are not covered (can be covered in the future) because a) are not relevant regarding replication or request dispatching, b) will unnecessarily increase the grpc requests payload (in the header section).

Other cases are not covered (can be covered in the future) because a) are not relevant regarding replication or request dispatching

Leaving things not completely finished in this PR is fine, but just for clarity: What are the other cases? I guess I see action cache fetches and maybe cas_artifact actions. Anything I'm missing?

will unnecessarily increase the grpc requests payload (in the header section).

I think even (and maybe especially) if this is intentionally not being filled out in some cases we should make that explicit in the code by requiring the parameter on use_case.metadata() and then writing a comment to explain why we pass None in some cases. Otherwise this will be forgotten about in the future

That makes sense. I’ve updated the code to address this concern.

Summary: When doing requests to a RBE service, it's useful to attach the 'RequestMetadata' type defined by Bazel onto gRPC requests, using a specific header. This is actually already done, more or less, but only for the internal fbcode build service which uses a different and simpler Metadata structure, and a different header. Therefore attaching a new header isn't so complicated. This also ensures that non-fbcode builds of Buck2 don't bother attaching fbcode Metadata, since it's not useful and can only be tested internally there, anyway. And likewise, we don't attach OSS Bazel RequestMetadata to fbcode_build configurations, either. Test Plan: NIH. Signed-off-by: Austin Seipp <aseipp@pobox.com> Change-Id: Irrsqpmptqmzzoukzuzrqmkxsoqtxvuvx

facebook-github-bot · 2024-02-21T22:18:59Z

@JakobDegen has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-02-29T11:45:10Z

This pull request has been reverted by 1d39ef8.

JakobDegen · 2024-03-04T21:18:43Z

The label was technically right that this was reverted, but I relanded it a day later so all good :)

vaskomitanov mentioned this pull request Nov 19, 2023

There is a need to correlate all requests to CAS,AC and Execution server in order to improve scheduling #503

Open

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 19, 2023

vaskomitanov force-pushed the user/vaskomitanov/add-build-id branch 2 times, most recently from c05ebb0 to 9950c18 Compare November 20, 2023 15:25

vaskomitanov force-pushed the user/vaskomitanov/add-build-id branch 2 times, most recently from 31ebaaf to dbfcc1c Compare November 21, 2023 06:33

vaskomitanov force-pushed the user/vaskomitanov/add-build-id branch from fde1407 to 7205dcd Compare November 27, 2023 21:57

vaskomitanov force-pushed the user/vaskomitanov/add-build-id branch from 121f713 to 35f08e8 Compare November 30, 2023 16:05

JakobDegen reviewed Feb 8, 2024

View reviewed changes

thoughtpolice and others added 3 commits February 12, 2024 09:38

Add tool_invocation_id in exec metadata

13ee3ba

Propagation of more information into metadata.

d59c19a

vaskomitanov force-pushed the user/vaskomitanov/add-build-id branch from 35f08e8 to ba485da Compare February 12, 2024 23:19

Replaced fbcode_metadata as compile time option and made it runtime one

0122a0f

vaskomitanov force-pushed the user/vaskomitanov/add-build-id branch from ba485da to 0122a0f Compare February 14, 2024 17:25

facebook-github-bot closed this in ee69c80 Feb 28, 2024

JakobDegen mentioned this pull request Feb 29, 2024

feat(remote_execution): attach RequestMetadata for non-fbcode builds #310

Closed

facebook-github-bot added the Reverted label Feb 29, 2024

JakobDegen removed the Reverted label Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build info across all requests #502

Add build info across all requests #502

vaskomitanov commented Nov 19, 2023

facebook-github-bot commented Nov 19, 2023

facebook-github-bot commented Nov 19, 2023

cormacrelf commented Nov 19, 2023

thoughtpolice commented Nov 19, 2023

vaskomitanov commented Nov 19, 2023 •

edited

Loading

ndmitchell commented Nov 20, 2023

vaskomitanov commented Nov 21, 2023

thoughtpolice commented Nov 21, 2023 •

edited

Loading

vaskomitanov commented Nov 27, 2023 •

edited

Loading

Yashshiva75 commented Nov 28, 2023

thoughtpolice commented Nov 30, 2023 •

edited

Loading

vaskomitanov commented Nov 30, 2023

vaskomitanov commented Dec 2, 2023

ndmitchell commented Dec 15, 2023

JakobDegen left a comment

JakobDegen Feb 8, 2024

vaskomitanov Feb 13, 2024

JakobDegen Feb 14, 2024

vaskomitanov Feb 14, 2024

facebook-github-bot commented Feb 21, 2024

facebook-github-bot commented Feb 29, 2024

JakobDegen commented Mar 4, 2024

Add build info across all requests #502

Add build info across all requests #502

Conversation

vaskomitanov commented Nov 19, 2023

facebook-github-bot commented Nov 19, 2023

Action Required

Process

facebook-github-bot commented Nov 19, 2023

cormacrelf commented Nov 19, 2023

thoughtpolice commented Nov 19, 2023

vaskomitanov commented Nov 19, 2023 • edited Loading

ndmitchell commented Nov 20, 2023

vaskomitanov commented Nov 21, 2023

thoughtpolice commented Nov 21, 2023 • edited Loading

vaskomitanov commented Nov 27, 2023 • edited Loading

Yashshiva75 commented Nov 28, 2023

thoughtpolice commented Nov 30, 2023 • edited Loading

vaskomitanov commented Nov 30, 2023

vaskomitanov commented Dec 2, 2023

ndmitchell commented Dec 15, 2023

JakobDegen left a comment

Choose a reason for hiding this comment

JakobDegen Feb 8, 2024

Choose a reason for hiding this comment

vaskomitanov Feb 13, 2024

Choose a reason for hiding this comment

JakobDegen Feb 14, 2024

Choose a reason for hiding this comment

vaskomitanov Feb 14, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Feb 21, 2024

facebook-github-bot commented Feb 29, 2024

JakobDegen commented Mar 4, 2024

vaskomitanov commented Nov 19, 2023 •

edited

Loading

thoughtpolice commented Nov 21, 2023 •

edited

Loading

vaskomitanov commented Nov 27, 2023 •

edited

Loading

thoughtpolice commented Nov 30, 2023 •

edited

Loading