Skip to content

RemoteEx 2.2: Promote platform properties from command to action#167

Merged
sstriker merged 1 commit intomasterfrom
edbaunton/inline-platform-properties
Oct 17, 2020
Merged

RemoteEx 2.2: Promote platform properties from command to action#167
sstriker merged 1 commit intomasterfrom
edbaunton/inline-platform-properties

Conversation

@edbaunton
Copy link
Copy Markdown
Contributor

@edbaunton edbaunton commented Sep 1, 2020

Platform properties are currently a member of the command message which is
referred to in the action by digest. This requires the Execution Service to
make a call to the CAS to retrieve the contents of the command if it wishes to
inspect it. The platform properties are commonly used for making routing
decision about the action. Therefore, in order to route an action common
execution service implementations must introduce an additional call to the CAS
to fully hydrate the action and determine where it should be routed.

This commit promotes the platform properties from the command to the action. We
deprecate the platform properties contained within the action and bump the
minor version to version 2.2, following the model for output_paths.

Fixes #166

Signed-off-by: Ed Baunton [email protected]

@googlebot googlebot added the cla: yes Pull requests whose authors are covered by a CLA with Google. label Sep 1, 2020
@edbaunton edbaunton mentioned this pull request Sep 1, 2020
Closed
@ulfjack
Copy link
Copy Markdown
Collaborator

ulfjack commented Sep 2, 2020

This seems reasonable.

EdSchouten
EdSchouten approved these changes Sep 5, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Question: should we make it a bit more explicit what a transition path looks like? Should clients provide both for the time being? Should the one in Action be preferred over the one in Command?

At the same time, some time ago I got a request on the Buildbarn side whether it's required to specify platform properties at all. Is it valid to send requests where both Action.platform and Command.platform are null/nil/.../None, or is it required that at least one of them contains a message?

@edbaunton edbaunton force-pushed the edbaunton/inline-platform-properties branch from 307ac57 to 6eec24a Compare September 9, 2020 20:27
@edbaunton
Copy link
Copy Markdown
Contributor Author

edbaunton commented Sep 9, 2020

I am not 100% familiar with the right way to perform a proto field migration but I tried to update the docs as I would myself have expected to do it. Basically if you use 2.2 you must use the new field, if you are not using 2.2 you must populate the old and can populate the new if you want. From 2.2 you shouldn't populate the old.

From your comment @EdSchouten I also added in an 'optional' to the new field to denote that they can be empty. I didn't change that for the deprecated field.

EdSchouten
EdSchouten reviewed Sep 11, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving this some more thought, one 'downside' of this approach is that clusters can no longer use the command digest as a key for correlating equivalent actions. For example, inspecting historical build actions to see whether their resource usage increased over time, etc..

That said, clusters are still capable of achieving this by introducing their own composite type:

message Key {
  Digest command_digest = 1;
  Platform platform = 2;
}

And hashing that to obtain a proper key for stuff like that. There's no need to account for that in the client <-> cluster protocol.

Comment thread build/bazel/remote/execution/v2/remote_execution.proto Outdated
@edbaunton
Copy link
Copy Markdown
Contributor Author

edbaunton commented Sep 22, 2020

How do others feel about how strict we should be with version migrations?

@edbaunton edbaunton force-pushed the edbaunton/inline-platform-properties branch from 6eec24a to 469859e Compare September 29, 2020 11:53
@edbaunton
Copy link
Copy Markdown
Contributor Author

edbaunton commented Sep 29, 2020

I have simplified the documentation to keep this simple change moving.

bergsieker
bergsieker approved these changes Oct 13, 2020
View reviewed changes
Comment thread build/bazel/remote/execution/v2/remote_execution.proto Outdated
Comment thread build/bazel/remote/execution/v2/remote_execution.proto Outdated
@edbaunton edbaunton force-pushed the edbaunton/inline-platform-properties branch from 469859e to 87b4f3d Compare October 14, 2020 19:46
@edbaunton
RemoteEx 2.2: Promote platform properties from command to action
b171c22 
Platform properties are currently a member of the `command` message which is
referred to in the action by digest. This requires the Execution Service to
make a call to the CAS to retrieve the contents of the command if it wishes to
inspect it. The platform properties are commonly used for making routing
decision about the action. Therefore, in order to route an action common
execution service implementations must introduce an additional call to the CAS
to fully hydrate the action and determine where it should be routed.

This commit promotes the platform properties from the command to the action. We
deprecate the platform properties contained within the action and bump the
minor version to version 2.2, following the model for `output_paths`.

Fixes #166

Signed-off-by: Ed Baunton <[email protected]>
@edbaunton edbaunton force-pushed the edbaunton/inline-platform-properties branch from 87b4f3d to b171c22 Compare October 14, 2020 19:47
@edbaunton
Copy link
Copy Markdown
Contributor Author

edbaunton commented Oct 15, 2020

Keen for one more approver @EdSchouten and @ulfjack if you have a chance

@sstriker sstriker merged commit ddca4b2 into master Oct 17, 2020
@edbaunton edbaunton deleted the edbaunton/inline-platform-properties branch October 22, 2020 19:15
illicitonion added a commit to illicitonion/bazel that referenced this pull request Mar 1, 2021
@illicitonion
Set Platform on Action not just Command
ad6f4da 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.
@illicitonion illicitonion mentioned this pull request Mar 1, 2021
Closed
illicitonion added a commit to illicitonion/bazel that referenced this pull request Mar 1, 2021
@illicitonion
Set Platform on Action not just Command
fafab77 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.
illicitonion added a commit to illicitonion/bazel that referenced this pull request Mar 1, 2021
@illicitonion
Set Platform on Action not just Command
82ef52e 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.
illicitonion added a commit to illicitonion/bazel that referenced this pull request Mar 1, 2021
@illicitonion
Set Platform on Action not just Command
928abb2 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.
illicitonion added a commit to illicitonion/bazel that referenced this pull request Mar 3, 2021
@illicitonion
Set Platform on Action not just Command
eaf6868 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.
bazel-io pushed a commit to bazelbuild/bazel that referenced this pull request Mar 3, 2021
@illicitonion @copybara-github
Set Platform on Action not just Command
dad9630 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.

Closes #13134.

PiperOrigin-RevId: 360647520
philwo pushed a commit to bazelbuild/bazel that referenced this pull request Mar 15, 2021
@illicitonion @philwo
Set Platform on Action not just Command
9563215 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.

Closes #13134.

PiperOrigin-RevId: 360647520
philwo pushed a commit to bazelbuild/bazel that referenced this pull request Mar 15, 2021
@illicitonion @philwo
Set Platform on Action not just Command
122581f 
bazelbuild/remote-apis#167 promoted the field,
but left the old one present for legacy fallback for remote execution
platforms which haven't updated yet.

This PR sets both.

Closes #13134.

PiperOrigin-RevId: 360647520
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this pull request Sep 4, 2022
Set Platform on Action not just Command
873d032 
    bazelbuild/remote-apis#167 promoted the field,
    but left the old one present for legacy fallback for remote execution
    platforms which haven't updated yet.

    This PR sets both.

    Closes #13134.

    PiperOrigin-RevId: 360647520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EdSchouten EdSchouten EdSchouten approved these changes

@sstriker sstriker sstriker approved these changes

@agoulti agoulti Awaiting requested review from agoulti

@buchgr buchgr Awaiting requested review from buchgr

@ola-rozenfeld ola-rozenfeld Awaiting requested review from ola-rozenfeld

+1 more reviewer

@bergsieker bergsieker bergsieker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Pull requests whose authors are covered by a CLA with Google.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Promoting or inlining platform properties

6 participants

@edbaunton @ulfjack @EdSchouten @sstriker @bergsieker @googlebot