-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve buildbarn documentation #179
Improve buildbarn documentation #179
Conversation
Hi @stagnation! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
||
In this document, we will go over the key configs used in this setup. | ||
Using a local docker-compose deployment from the [example deployment repo](https://github.com/buildbarn/bb-deployments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want some connective tissue here, to say that with a ready Buildbarn deployment the deploy section can be skipped. But for those who want to test remote execution, I think it's good to have instructions for that.
We can make it more succinct after we fix the instance-name prefix issue. But I would like to help people avoid that speed bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That landed, can you add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
@@ -2,13 +2,10 @@ | |||
root = . | |||
prelude = prelude | |||
|
|||
[buck2] | |||
digest_algorithms = SHA256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangentially related thought, shouldn't buck2 pick a suitable algorithm that matches one of the supported ones from the REV2 GetCapabilities call?
https://github.com/bazelbuild/remote-apis/blob/35aee1c4a4250d3df846f7ba3e4a4e66cb014ecd/build/bazel/remote/execution/v2/remote_execution.proto#L1960
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, though I think it's a bit unlikely that we'd make this a priority — we've got no use for it internally is a small reason, but the bigger reason would be be that this is a pretty substantial overhaul of how we start up things, since we need this config very early on, which is not true of the RE client, for ultimately fairly little payoff (at least at the moment, all RE providers we're aware of support SHA256, which is the default we use in the OSS build).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment that's true, there's not much gain in consuming GetCapabilities only to look at which digest function to use when SHA256 is pretty much universally supported.
But looking forward a little, the RE api recently adopted SHA256TREE as a digest function which is a precursor to add new functionality to the remote api ConcatenateBlobs and SplitBlobs. Those will allow partial transfer of larger blobs and ultimately become a more efficient replacement of bytestream.write.
There is of course no hurry, I just thought it might be worth poking the project on a trajectory that meshes well with how the protocol is moving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But looking forward a little, the RE api recently adopted bazelbuild/remote-apis#235 as a digest function which is a precursor to add new functionality to the remote api bazelbuild/remote-apis#233. Those will allow partial transfer of larger blobs and ultimately become a more efficient replacement of bytestream.write.
Sure, my point there is primarily that actually configuring the hashing scheme you use is rather the trivial bit, there's a lot more to actually implementing describing what you're doing than inferring the configuration.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
0b69e45
to
4b6363c
Compare
I wonder if this should be on the homepage on http://buck2.build, right? Then it can just point to this directory as an example. It was definitely non-obvious to me at first! |
action_cache_address = http://localhost:8980 | ||
engine_address = http://localhost:8980 | ||
cas_address = http://localhost:8980 | ||
action_cache_address = grpc://localhost:8980 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep this as http://
.
Neither of those are actually valid as per the GRPC spec for URIs, but grpc://
appears to be more used for when TLS is desired, so I intend to change #174 to respect that (at which point this would break)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, let's keep this as-is, and add a separate config field for TLS. I'll update #174 with that.
|
||
First, the Build Barn endpoint and certificate should be configured as the following: | ||
This is mostly a turn-key solution, with just one modification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe just remove this and wait for #181 to get merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Great point. I originally wrote this last week, when we had no timeline for a solution.
And then an in-review Pull Request would be a good location for other confused developers to find the information. I planned to remove this after the solution, so the final artifact would not have it.
I'm fine with wating for 181, so I can remove the mention from here.
``` | ||
|
||
### Relevant configs in `ExecutionPlatformInfo` | ||
No TLS is used in this example (for history see this [issue](https://github.com/facebook/buck2/issues/156)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we think the user should learn from this history? Can we either omit this entirely, or make that clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yepp, with your work to solve it we do not need any mention of the problems.
I will remove it.
|
||
The execution platform used in this project `root//platforms:platforms` do so. | ||
The Buildbarn workers take in a Docker image as their execution platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using "execution platform" here is a little confusing. To my knowledge, this is not a term Build Barn uses, but it is a term Buck2 uses, and that's not what it means in Buck2. I think "worker" did seem more appropriate (since that is terminology Build Barn uses)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
@@ -21,6 +21,8 @@ def _platforms(ctx): | |||
# Set those up based on what workers you've registered with Build Barn. | |||
remote_execution_properties = { | |||
"OSFamily": "Linux", | |||
# This could be any Docker image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can actually be any Docker image? It's got to be the one you did start your Build Barn cluster with, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you are right, and your base example did not have the comment. I just copy-pasted it from the Engflow example.
I'll remove it.
@@ -2,13 +2,10 @@ | |||
root = . | |||
prelude = prelude | |||
|
|||
[buck2] | |||
digest_algorithms = SHA256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, though I think it's a bit unlikely that we'd make this a priority — we've got no use for it internally is a small reason, but the bigger reason would be be that this is a pretty substantial overhaul of how we start up things, since we need this config very early on, which is not true of the RE client, for ultimately fairly little payoff (at least at the moment, all RE providers we're aware of support SHA256, which is the default we use in the OSS build).
4b6363c
to
36e1e37
Compare
cas_address = http://localhost:8980 | ||
action_cache_address = grpc://localhost:8980 | ||
engine_address = grpc://localhost:8980 | ||
cas_address = grpc://localhost:8980 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add tls = false
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though you already did in the commit that added it.
|
||
In this document, we will go over the key configs used in this setup. | ||
Using a local docker-compose deployment from the [example deployment repo](https://github.com/buildbarn/bb-deployments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That landed, can you add it?
Add instructions for a simple local Buildbarn deployment to test with.
36e1e37
to
5e23235
Compare
tls = false | ||
instance_name = fuse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record: inline toml comments are parsed as part of the string.
I presume this is a toml file, but you can disregard this if inline comments are not meant to work.
Internal error (stage: remote_call_error): Remote Execution Error (GRPC-SESSION-ID): RE: execution with digest 9a8db5f5c0ba0b07866989dafa06b9914c25483dfedba2b0f53d06251191bdfc:142: Failed to start remote execution: status: Unavailable, message: "No workers exist for instance name prefix \"fuse # or \\\"hardlinking\\\"\" platform {\"properties\
":[{\"name\":\"OSFamily\",\"value\":\"Linux\"},{\"name\":\"container-image\",\"value\":\"docker://ghcr.io/catthehacker/ubuntu:act22.04@sha256:5f9c35c25db1d51a8ddaae5c0ba8d3c163c5e9a4a6cc97acd409ac7eae239448\"}]}", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
"No workers exist for instance name prefix \"fuse # or \\\"hardlinking\\\"\"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really a toml file, no. I thin kit'd be good if we used a more robust format for this, but it's not the case currently
@krallin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Improve the example with instructions for how to deploy buildbarn locally with bb-deploymens.
And a workaround for instance name prefixes.
I will write an issue to track that, and we should refer to that issue in the example README.