Concurrency PR

[Tyson Norris <tn...@adobe.com.INVALID>]

Hi -
I have been noodling with a few tests and the akka http client and gotten the concurrency PR [1] to a good place, I think, so if anyone can help review that would be appreciated.

A couple of notes:
- akka http client has some different notion of connection reuse than the apache client, to address this I created a separate PR [2] which, instead of dissuading connection reuse, simple destroys the client (and connection pool) when the container is paused. (This change is not reflected in 2795 FWIW). AFAIK the connection reuse issue only comes up with container pauses, so I wanted to address this where it is relevant, and not impose additional performance costs for concurrency cases. This client is still not enabled by default.
- There was mention in the comments (for 2795) about need to handle a case where a container doesn’t support concurrency, but the action dev has enabled it at the action - this PR does NOT deal with that.

To summarize, enabling concurrency requires:
- all actions may signal that they support concurrency, so all images that might be used would need to support concurrency, if concurrency is enabled in your deployment
- log collection must be handled outside of invoker (since invoker does not deal with interleaved log parsing)
- wsk cli will require changes to allow action devs to set the concurrency limits on actions (current PR only exposes the OW api for doing this); I have a PR queued up for that [3]. (Will need another PR for the cli once the client-go lib is updated)

To better handle the case of images that don’t support concurrency, or don’t support log collection from invoker, I would suggest we change the container protocol to allow containers to broadcast their support either via the /init endpoint, or via a new /info endpoint. This of course would not give feedback until an action is executed (as opposed to when action is created), but I think this is ok. I will work on a separate PR for this, but want to mention some thoughts here about possible approaches to address these known concerns.

Thanks
Tyson


[1] https://github.com/apache/incubator-openwhisk/pull/2795
[2] https://github.com/apache/incubator-openwhisk/pull/3976
[3] https://github.com/apache/incubator-openwhisk-client-go/pull/94

Re: Concurrency PR

[Tyson Norris <tn...@adobe.com.INVALID>]

To better handle the case of images that don’t support concurrency, or
don’t support log collection from invoker, I would suggest we change the
container protocol to allow containers to broadcast their support either
via the /init endpoint, or via a new /info endpoint. This of course would
not give feedback until an action is executed (as opposed to when action is
created), but I think this is ok. I will work on a separate PR for this,
but want to mention some thoughts here about possible approaches to address
these known concerns.


Why not make this part of the runtimes manifest? Handling this as late as
actually invoking the action feels kinda weird if we can just as well know
ahead of time, that creating an action with a concurrency > 1 will not work
and should therefore forbid creation at all. Any strong reason not to
encode that information into the runtimes manifest?


One thing is blackbox containers - will only managed containers be allowed to support concurrency? If not, how will blackbox containers advertise support?

Separately, the only thing that action creation currently does is validate “action build time” issues, like "do action limits fit within the allowed ranges”, it does not mean the input value is appropriate for execution. If the runtime manifest had per-image settings for all action limits, it would make more sense to include concurrency there - but I would have the same suggestion, that allowing the image to dictate the entirety of its opinions on these allowed ranges would be nicer to do as part of the container protocol (/init, /run), and not the manifest - partly for blackbox support, and partly because it is simpler to maintain just the image and just the manifest (which just references images, not the metadata of what the image supports internally).

In any case, is this (per container metadata indicating concurrency support) really required for supporting concurrency at all? I agree it will be nice, but don’t think that it should block adding support for concurrency.

WDYT?

Thanks
Tyson

Re: Concurrency PR

[Markus Thömmes <ma...@apache.org>]

Hi Tyson, thanks for pushing forward on this! I'll try to get a review in
on it soon.

Am Fr., 17. Aug. 2018 um 19:04 Uhr schrieb Tyson Norris
<tn...@adobe.com.invalid>:

> Hi -
> I have been noodling with a few tests and the akka http client and gotten
> the concurrency PR [1] to a good place, I think, so if anyone can help
> review that would be appreciated.
>
> A couple of notes:
> - akka http client has some different notion of connection reuse than the
> apache client, to address this I created a separate PR [2] which, instead
> of dissuading connection reuse, simple destroys the client (and connection
> pool) when the container is paused. (This change is not reflected in 2795
> FWIW). AFAIK the connection reuse issue only comes up with container
> pauses, so I wanted to address this where it is relevant, and not impose
> additional performance costs for concurrency cases. This client is still
> not enabled by default.
> - There was mention in the comments (for 2795) about need to handle a case
> where a container doesn’t support concurrency, but the action dev has
> enabled it at the action - this PR does NOT deal with that.
>
> To summarize, enabling concurrency requires:
> - all actions may signal that they support concurrency, so all images that
> might be used would need to support concurrency, if concurrency is enabled
> in your deployment
> - log collection must be handled outside of invoker (since invoker does
> not deal with interleaved log parsing)
> - wsk cli will require changes to allow action devs to set the concurrency
> limits on actions (current PR only exposes the OW api for doing this); I
> have a PR queued up for that [3]. (Will need another PR for the cli once
> the client-go lib is updated)
>
> To better handle the case of images that don’t support concurrency, or
> don’t support log collection from invoker, I would suggest we change the
> container protocol to allow containers to broadcast their support either
> via the /init endpoint, or via a new /info endpoint. This of course would
> not give feedback until an action is executed (as opposed to when action is
> created), but I think this is ok. I will work on a separate PR for this,
> but want to mention some thoughts here about possible approaches to address
> these known concerns.
>

Why not make this part of the runtimes manifest? Handling this as late as
actually invoking the action feels kinda weird if we can just as well know
ahead of time, that creating an action with a concurrency > 1 will not work
and should therefore forbid creation at all. Any strong reason not to
encode that information into the runtimes manifest?


>
> Thanks
> Tyson
>
>
> [1] https://github.com/apache/incubator-openwhisk/pull/2795
> [2] https://github.com/apache/incubator-openwhisk/pull/3976
> [3] https://github.com/apache/incubator-openwhisk-client-go/pull/94
>
>