Skip to content
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

Promoting or inlining platform properties #166

Closed
edbaunton opened this issue Aug 28, 2020 · 4 comments · Fixed by #167
Closed

Promoting or inlining platform properties #166

edbaunton opened this issue Aug 28, 2020 · 4 comments · Fixed by #167

Comments

@edbaunton
Copy link
Collaborator

edbaunton commented Aug 28, 2020

We discussed this during a monthly meeting (I seem to recall that @ulfjack initially raised it), adding here for tracking.

The current design of platform properties is that they are indirectly embedded in the Command property of the Action. The Action does not send the Command directly but rather sends a Command digest.

The upshot of this is that additional blob uploads are required from the client before submitting and action as well as additional CAS interactions on the server side if any data is required from the Command. For example, platform properties.

I think the initial discussion specifically mentioned that if the server wanted to making routing decisions for an action based on the platform properties it would require and additional hit to the CAS to determine those for the action.

It seems to me that some of this extra CAS interaction overhead could be avoided if we inlined the command or platform properties into the action.

I can see that we could probably:

  1. Extend the action message to have both command_digest or the actual command
  2. Inline the platform properties
  3. Something else?
@bergsieker
Copy link
Collaborator

bergsieker commented Aug 28, 2020 via email

@ulfjack
Copy link
Collaborator

ulfjack commented Aug 28, 2020

I am concerned about the schedulers having to fetch multiple blobs from the CAS sequentially, and having to parse them into local memory (especially because we expect the Command proto to be large). Both because this is awkward for our implementation, and also because it may enable various denial-of-service vectors. Especially for small clusters, having to overprovision schedulers to safeguard against this can incur significant compute / memory costs. (I'd also be slightly concerned about the Platform proto growing significantly in size.)

I don't think this is over-designing for a specific scheduler implementation. The current protocol requires that the scheduler reads the Action & Command protos, which clearly imposes restrictions on the scheduler design.

I think this may also tie into the proposal by @EdSchouten about making the scheduler untrusted - the more CAS reads are required in the scheduler for routing, the more holes you need to poke into the security model. Personally, I think untrusted schedulers are only viable from a security perspective if either a) the platform proto being sent to the scheduler explicitly, i.e., no direct CAS reads from the scheduler, or b) the Platform proto (and every proto between the execute proto and the platform proto) are stored in a separate CAS service (public CAS / private CAS distinction). Requiring the Command to be public seems like a fairly large information leak.

I'm not sure what the best place to put the Platform proto is. I have a strong preference for moving it out of Command. Ideally, it would not be stored in the CAS at all: that would allow a scheduler design that does routing with the Platform proto only and is also untrusted, without requiring a separate public CAS (or complicating the CAS protocol to allow a public / private distinction). However, I'd be concerned about the platform proto growing to be significantly larger. Over time, I can see us define hundreds of settings in the platform proto. Maybe a compromise would be to allow the platform proto to be inlined in the execute request or referenced via digest? Too much flexibility?

@bergsieker
Copy link
Collaborator

bergsieker commented Aug 28, 2020 via email

edbaunton added a commit that referenced this issue 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]>
@edbaunton
Copy link
Collaborator Author

I think #167 satisfies the above discussion except for the case of a large set of platform properties that @ulfjack mentions. I think we would need to do something much more sophisticated with platform properties to support this case: those that are placed in-line and those that reside in the command.

edbaunton added a commit that referenced this issue Sep 9, 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]>
edbaunton added a commit that referenced this issue Sep 29, 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]>
edbaunton added a commit that referenced this issue Oct 14, 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]>
edbaunton added a commit that referenced this issue Oct 14, 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]>
sstriker pushed a commit that referenced this issue Oct 17, 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants