Provide action limit configuration for each namespace

[upgle]

Description

Currently, openwhisk has only the system limit shared by all namespaces. I have received requirements to set different limits for each namespace/user. So I propose a new feature for namespace limit.

3 types of limits

• There are (1) system limits, (2) namespace default limits, (3) namespace limits.
  • (1) system limits: under no circumstances may this limit be exceeded.
  • (2) namespace default limits: use this limit value unless a limit has been set in the namespace.
  • (3) namespace limit: It can be set by the system administrator. This value cannot exceed the range of the system limit.

case1) Using default namespace limit

case1) Using namespace limit (for foo namespace)

Limit doc

You can set namespace limits with {namespace}/limits document just like any other existing setting.

{
  "concurrentInvocations": 100,
  "invocationsPerMinute": 100,
  "firesPerMinute": 100,
  "maxActionMemory": 128,
  "maxActionConcurrency": 400
  "maxActionLogs": 128,
  "maxParameterSize": "1 kb"
}

New namespace limit config

config key	description
minActionMemory	minimum action memory size for namespace
maxActionMemory	maximum action memory size for namespace
minActionLogs	minimum activation log size for namespace
maxActionLogs	maximum activation log size for namespace
minActionTimeout	minimum action time limit for namespace
maxActionTimeout	maximum action time limit for namespace
minActionConcurrency	minimum action concurrency limit for namespace
maxActionConcurrency	maximum action concurrency limit for namespace
maxParameterSize	maximum parameter size for namespace
maxPayloadSize	maximum activation payload size for namespace
truncationSize	activation truncation size for namespace

Related issue and scope

I didn't open new issue, but described the changes here.

My changes affect the following components

• [x] API
• [x] Controller
• [x] Tests
• [ ] Documentation

Types of changes

• [ ] Bug fix (generally a non-breaking change which closes an issue).
• [x] Enhancement or new feature (adds new functionality).
• [ ] Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

• [x] I signed an Apache CLA.
• [x] I reviewed the style guides and followed the recommendations (Travis CI will check :).
• [x] I added tests to cover my changes.
• [x] My changes require further changes to the documentation.
• [x] I updated the documentation where necessary.

[style95]

Nice! Could you open another PR to add a POEM for this feature as well? https://github.com/apache/openwhisk/tree/master/proposals

[codecov-commenter]

Codecov Report

Merging #5229 (9005a08) into master (9005a08) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 9005a08 differs from pull request most recent head 28ade66. Consider uploading reports for the commit 28ade66 to get more accurate results

@@           Coverage Diff           @@
##           master    #5229   +/-   ##
=======================================
  Coverage   81.12%   81.12%           
=======================================
  Files         239      239           
  Lines       14192    14192           
  Branches      580      580           
=======================================
  Hits        11513    11513           
  Misses       2679     2679           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[upgle]

@style95 As the implementation continues to change, POEM has been added to this PR for consistency. I will continue to update the PR content as well.

Nice! Could you open another PR to add a POEM for this feature as well? https://github.com/apache/openwhisk/tree/master/proposals

[style95]

According to this process, we need a separate PR to add a POEM along with this PR. https://github.com/apache/openwhisk/blob/master/proposals/POEM-1-proposal-for-openwhisk-enhancements.md#procedures

[ningyougang]

If this pr: apache/openwhisk/pull/5229 merged.

Let's assume, user may want to use 3GB of memory for her action, but the system limit maybe 2GB currently.

• Does openwhisk admin need to reconfigure his system limit (e.g. make system limit to a high value)?

  So openwhisk admin needs to adjust the system limit value to >=3GB?

• As common user, he just changes the $namespace/limit document is enough?

  Has any check mechanism that when user changes the $namespace/limit document, namespace limit value can't be greater than system limit?

• Has any extra work on wskadmin? e.g. Admin can use wskadmin can change the $namespace/limit document as well.

Have any influence for old action? e.g. user or admin may need some migration works.

[upgle]

@ningyougang

• Does openwhisk admin need to reconfigure his system limit (e.g. make system limit to a high value)? So openwhisk admin needs to adjust the system limit value to >=3GB?

  • If the existing system limit was 2GB and you want to allocate up to 3GB, you need to raise the system limit and redeploy it. And because the namespace default limit is 2GB, existing users without a limit setting will use this limit.

• Has any check mechanism that when user changes the $namespace/limit document, namespace limit value can't be greater than system limit?

  • If the namespace limit is greater than the system limit, it is lowered to the system limit.

• Has any extra work on wskadmin? e.g. Admin can use wskadmin can change the $namespace/limit document as well.

  • Not yet. There is a plan to add it, but it seems to have to be created as a separate PR because there are a lot of changes in the PR.

@style95

According to this process, we need a separate PR to add a POEM along with this PR. https://github.com/apache/openwhisk/blob/master/proposals/POEM-1-proposal-for-openwhisk-enhancements.md#procedures

I'm working on integrating with the new scheduler and improving backward compatibility. I'ill create a separate POEM PR as soon as it is completed.

[upgle]

Removed the POEM document file from this PR as it created a separate PR for POEM https://github.com/apache/openwhisk/pull/5236.

[style95]

@upgle It seems it does not pass the unit test. I kept rerunning the test but could not make it work.

[ningyougang]

Need rebase

[bdoyle0182]

Before you merge I just have one last comment to discuss on naming with my separate poem on giving users the ability to configure max container / "server" concurrency, which I currently have to be maxContainerConcurrency as a field in the limits section of an action document. I've named it this way because there is already concurrency in the limits section for max concurrent requests within an individual container / "server".

I suspect we will add a similar configuration to this feature for a namespace to allow a min and max container concurrency setting to be allowed to be configured per action within a namespace. Since in this pr the configuration to control max and min of concurrent requests per container is maxActionConcurrency

So I think we've gotten to the point where we have many concurrency knobs and it's getting confusing in naming.

1. concurrentInvocations on the namespace global limit which represents the total containers that can be up at once, which isn't really representative of the semantics of that limit anymore since you can have more concurrent invocations than the set value.
2. concurrency as a limit on the action document which represents max requests that a single container for an action can handle at once.
3. maxActionConcurrency defined in this pr which represents the min and max defined as namespace limits of a window of what values can be configured for 2.
4. maxContainerConcurrency defined in my poem which is also a limit on the action document like 2. but represents the max containers that can be running for that action at once such that max throughput for an action is then 2. * 4.

If we were to add lower and upper limits in the same vein for my new maxContainerConcurrency value here, what would we name it. maxContainerConcurrency and minContainerConcurrency? Should I then rename the limit defined in the action document to then just be containerConcurrency rather than include max?

Don't want to hang up this pr much more as I know it's been sitting for awhile just want to make sure we get the naming right as the concurrency knobs are the most important for controlling the system and I don't want it to be confusing for new users or even existing users that see these new configurations.

Does maxActionConcurrency and maxContainerConcurrency feel adequately different in name and actually representative of what these limits do?

[bdoyle0182]

Also one other thing since I don't have time to comb through the code again right now, but you mention in the description that you can update the default limit of a namespace but don't see in the description any limit options to set default for namespace so was wondering if it's in the pr.

i.e. if the system memory limit default is 256mb, can you change the default value for a specific namespace to 128mb?

[style95]

It's worth discussing.

I feel there are mainly two big concurrencies.

1. Concurrent containers. (concurrent in-flight activations in the old scheduler)
2. Concurrent activations in one container.

Currently, we call 1. as concurrentInvocations and for 2., just concurrency. I feel we need to clean up these names first.

Since the name, concurrentInvocations best fits with ShardingContainerPoolBalancer and it is still being used, it might not be easy to change the name before we drop supporting it.

So we can think about concurrency. The concurrency is only referring to the intra-concurrency in one container until now. Since it is related to the action container, I feel maxActionConcurrency is sort of aligned with it.

But for maxContainerConcurrency, I am unsure if this best describes what we intended. Container may not best fit in case OW is running on K8S as it is using Pod. I feel it should not include a certain implementation in the name. We might introduce containerless runtime in the future.

This configuration is basically to allow overprovision of containers beyond the default concurrentInvocations. We may consider names like the below until we keep using concurrentInvocations for container-level concurrency.

• maxConcurrentInvocations.
• Some names related to overprovision.

[upgle]

@bdoyle0182 Thanks for your comments.

i.e. if the system memory limit default is 256mb, can you change the default value for a specific namespace to 128mb?

If the system memory limit is 256mb, and default memory limit is 128mb, you can create action with 128mb memory, higher memory is not available. If administrator gives you a higher memory limit (called namespace limit), you can create an action with 256mb memory.

This namespace limit can be set in a document with the key {namespace}/limits by system administrator.

And the default limit of a namespace is a value set as an environment variable during deployment. If not set, the same value is set as the current system limit for backward compatibility.

---

I also feel it's not easy to explain different types of concurrency. The name "action concurrency" is already used a lot in code, and configs. I used the same name because using a different name from the existing one is rather confusing.

behavior of "Action concurrency limits"

val actionConcurrency = 5

   * @param concurrency the action concurrency limit to be set in test
   * @param ec the expected exit code when creating the action
  val concurrencyEnabled = Option(WhiskProperties.getProperty("whisk.action.concurrency")).exists(_.toBoolean)

And the difference is that the "maxActionConcurrency" config added in this PR are not exposed to the user.

Because the name is similar to the existing "concurrency" config, users may need to read the document to understand the concept of new container concurrency.

• maxActionConcurrency
  • The config is not exposed to the user. (It's admin setting)
• maxContainerConcurrency
  • The config is exposed to the user by action limit.

concurrency: ConcurrencyLimit = ConcurrencyLimit(),
maxContainerConcurrency: Option[ContainerConcurrencyLimit] = None)

[bdoyle0182]

Do you all think instanceConcurrency would make more sense as the identifier of the limit on the action document rather than maxContainerConcurrency in my pr?

I think including max doesn't make sense on the action document to follow the same nomenclature of the existing concurrency field and that works in a similar matter where it's the max requests the container can serve at once but isn't guaranteed to be using at once, same would apply to total number of containers. So that a maxInstanceConcurrency and minInstanceConcurrency can then be defined as new namespace default limits that this pr is adding

[style95]

merged as it's been so long.