WIP: custom platform properties #1432
Conversation
BuildStream-Migration-Bot
requested review from
BenjaminSchubert,
cs-shadow,
gtristan and
juergbi
as code owners
|
In GitLab by [Gitlab user @tpollard] on Nov 6, 2020, 16:56 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 13:06 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 13:13 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 13:14 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 15:21 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 15:42 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 15:50 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 16:01 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 16:02 changed the description |
1 similar comment
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 16:02 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 9, 2020, 16:04 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 12:54 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 13:44 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 13:55 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 14:07 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 16:49 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 16:50 marked the task Add tests (expand remote_services fixture?) as completed |
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 16:50 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 10, 2020, 17:36 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 11, 2020, 11:04 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 11, 2020, 11:15 added 14 commits
|
|
In GitLab by [Gitlab user @tpollard] on Nov 11, 2020, 11:26 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 11, 2020, 11:36 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 11, 2020, 11:50 added 22 commits
|
|
In GitLab by [Gitlab user @tpollard] on Nov 12, 2020, 15:38 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 12, 2020, 15:52 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 12, 2020, 15:55 The RE tests should now be working with the default properties, with the addition of a custom property and with a default property not set (ISA). |
|
In GitLab by [Gitlab user @tpollard] on Nov 12, 2020, 16:02 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 12, 2020, 16:03 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 12, 2020, 16:08 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 13, 2020, 15:56 mentioned in merge request remote-apis-testing/remote-apis-testing!192 |
|
In GitLab by [Gitlab user @tpollard] on Nov 16, 2020, 10:45 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 16, 2020, 13:37 This is branch now passing the buildstream-buildbarn integration test at https://gitlab.com/remote-apis-testing/remote-apis-testing/-/merge_requests/192 |
|
In GitLab by [Gitlab user @tpollard] on Nov 18, 2020, 10:30 I feel like this implementation needs initial review before working on the remaining hardening |
|
In GitLab by [Gitlab user @tpollard] on Nov 18, 2020, 10:45 changed the description |
|
In GitLab by [Gitlab user @tristanvb] on Nov 20, 2020, 05:12 First pass highlevel API considerations:
|
|
In GitLab by [Gitlab user @tpollard] on Nov 23, 2020, 12:05 Hi [Gitlab user @tristanvb] thanks for the initial comments, I'll try and answer where I can:
In terms of Buildgrid, the server also needs to be configured to allow arbitrary/custom properties for jobs before they're assigned to a worker, this MR currently has an example of that using the required config from https://gitlab.com/BuildGrid/buildgrid/-/blob/master/buildgrid/_app/settings/reference.yml#L150 Without BuildStream being able to query the capabilities of a server (which even if it could I don't think it should) I think the remotespec is the correct place for this config.
I agree, there's definitely usecases where a project could have elements that need to be built on differing workers (for instance, with gpu acceleration present). I suppose one approach to that could be to have specific junctions for specific environments?
I wouldn't be against this, but I also can see the view that the current sourcing of 'default properties' is a nice to have. However if a user wants to specify 'custom properties', then I'd be happy if that then superseeded any local decisions bst would normally make (i.e, the user needs to declare all the properties they require if they want to stray from the local defaults). This would be more explicit, and would only require a single new config option. It is worth considering the complication of cachekeys being a product of these local defaults from the host sandboxconfig (uname etc), if not set by the user/project/element. For instance, this could lead to hashing linux and x86-64 for the key but then ignoring this input data and building remotely with darwin and arm64 as the platform properties with this explicit superseeding without some form of validation against the sanboxconfig values. Not just cachekeys either, this could lead to issues when trying to launch a local sandbox on an artifact that was built remotely without handling the mismatch. |
|
In GitLab by [Gitlab user @juergbi] on Nov 25, 2020, 08:13 It's overall pretty tricky because the Remote Execution API allows the server implementation a lot of flexibility. I.e., it's underspecified and the servers are not required to support any particular platform property. The reason for this is that RE clients and their (sandboxing) requirements are also quite diverse. Platform properties are used for a variety of use cases. From OS/ISA selection to sandboxing restrictions, Docker images, hardware requirements and project tagging. The effect of this is that at least in some cases BuildStream as a client will need server-specific 'platform properties'. I think our aim should be that anything that is not specific to a particular RE server (and thus, is typically also applicable to local builds) is declared in the project or element-level However, configuration that is truly server-specific should clearly not take part in the cache key calculation and thus belongs in the A concern are aspects that are not specific to a particular RE server, however, where the RE server requires non-standard configuration (or standard configuration does not even exist). I tend towards something like the following:
¹ A more radical variant would be to simply make |
|
In GitLab by [Gitlab user @tpollard] on Nov 25, 2020, 15:54 changed the description |
|
In GitLab by [Gitlab user @tpollard] on Nov 26, 2020, 10:24 added 11 commits
|
|
In GitLab by [Gitlab user @tpollard] on Nov 26, 2020, 14:52 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 26, 2020, 16:45 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 09:46 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 10:23 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 12:25 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 13:20 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 14:19 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 14:43 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 15:12 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 15:27 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 15:46 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 15:47 added 1 commit
|
|
In GitLab by [Gitlab user @tpollard] on Nov 27, 2020, 16:35 I've added a commit (will squash and tidy etc after agreement/code review ofc) which drops the If one of the 'default' properties is passed via this config option, providing a value which is anything other than What do you think? (There's obviously a few things left to implement, like allowing properties to have list values and preventing nested dicts in the yaml, etc) |
|
Note this is related with #1313 |
See original merge request on GitLab
In GitLab by [Gitlab user @tpollard] on Nov 6, 2020, 16:55
This is related to https://gitlab.com/BuildStream/buildstream/-/issues/1313
In short, there are remote-exection usecases where server/worker implementations may incorporate custom/additional platform properties, or choose not to implement all/any of the 'optional' standard properties, as defined by the platform message lexicon:
https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/platform.md
get_unique_key()for the sandbox config)This branch is being exercised with this remote-apis-testing MR https://gitlab.com/remote-apis-testing/remote-apis-testing/-/merge_requests/192