Provide action limit configuration for each namespace #5229

upgle · 2022-05-03T08:08:54Z

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

API
Controller
Tests
Documentation

Types of changes

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

Checklist:

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

style95 · 2022-05-03T08:37:45Z

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 · 2022-05-03T10:16:11Z

Codecov Report

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

❗ 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

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

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ConcurrencyLimit.scala

jiangpengcheng · 2022-05-09T02:47:20Z

core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala

@@ -179,6 +178,8 @@ class InvokerReactive(
    WhiskAction
      .get(entityStore, actionid.id, actionid.rev, fromCache = actionid.rev != DocRevision.empty)
      .flatMap(action => {
+        // action that exceed the system limit cannot be executed.
+        action.limits.checkSystemLimits()


I think it already check the limit here? https://github.com/apache/openwhisk/pull/5229/files#diff-618ee3b5632d322d90e279346bd69b1d30f39f22b237e557f1a4f507414b61fdR216

@jiangpengcheng The code you shared is for checking the limit when creating an action.
In order to prevent execution by malicious modifications such as DB alteration, the invoker side verifies once more before invoking an action.

Does it apply the new scheduler? e.g. when FunctionPoolConatinerProxy fetches the activation from memoryQueue's activation

Yes, It supports new scheduler.

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

ningyougang · 2022-05-09T05:50:26Z

tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala

+        Messages.sizeExceedsAllowedThreshold(MemoryLimit.memoryLimitFieldName, is.toMB.toInt, allowed.toMB.toInt)
+      }
+    }
+  }


I am ok with test case, can provide the opposite test case? e.g. allow create when memory is greater than maximum allowed namespace limit?

bdoyle0182 · 2022-05-10T05:52:10Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

@@ -624,6 +626,30 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
    })
  }

+  private def checkNamespaceAndSystemLimits(user: Identity, content: WhiskActionPut)(


Should this be two separate functions? The context in which you check the set limits for a namespace are different from the context you check the system limits. You check against the system limits when creating the action or updating your namespace's configuration. When you check against the namespace's limit it would be on the execution path when an activation is occurring.

Because updating the namespace limit configuration directly inserts/updates into the Couch DB, I've updated the code to fallback to the system limit if the namespace limit that exceeds the system limit.

So, namespace limits that can not exceed the system limit are checked when an action is created/invoked.

proposals/POEM-3-action-limit-for-namespace.md

upgle · 2022-05-10T12:52:22Z

@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 · 2022-05-10T12:57:28Z

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 · 2022-05-12T01:06:14Z

If this pr: /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 · 2022-05-12T01:19:23Z

@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 · 2022-05-16T14:00:19Z

Removed the POEM document file from this PR as it created a separate PR for POEM #5236.

style95

It generally looks good to me.

common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/AkkaContainerClient.scala

style95 · 2022-06-15T11:32:35Z

...a/src/main/scala/org/apache/openwhisk/core/containerpool/ApacheBlockingContainerClient.scala

@@ -124,13 +129,13 @@ protected class ApacheBlockingContainerClient(hostname: String,

          // Negative contentLength means unknown or overflow. We don't want to consume in either case.
          if (contentLength >= 0) {
-            if (contentLength <= maxResponseBytes) {
+            if (contentLength <= maxResponse.toBytes) {


Do we need toBytes here?

Yes. Since maxResponse is a ByteSize type, it needs a cast.

maxResponse: ByteSize,

style95 · 2022-06-15T11:33:45Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ActivationEntityLimit.scala

+  } catch {
+    case _: Throwable =>
+      // Supports backwards compatibility for openwhisk that do not use the namespace default limit
+      ActivationEntityPayload(config.payload.max, config.payload.truncation)


style95 · 2022-06-15T11:49:27Z

tests/src/test/scala/org/apache/openwhisk/core/entity/test/SchemaTests.scala

        val int = the[DeserializationException] thrownBy s.read(JsNumber(2.5))
        int.getMessage should include("limit must be whole number")
      }
    }
  }

-  it should "reject bad limit values" in {


Don't we need this negative test case?

The scheme test based on the hardcoded limit value is no longer used and validation checks are performed based on user input at runtime.

style95 · 2022-06-22T01:52:38Z

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

style95

LGTM!

ningyougang · 2022-08-03T08:59:22Z

Need rebase

style95 · 2023-01-25T10:23:02Z

merged as it's been so long.

upgle changed the title ~~[Proposal] Provide different action limits for different namespaces~~ [Proposal] Provide action limit configuration for each namespace May 3, 2022

upgle added the wip label May 3, 2022

ningyougang reviewed May 9, 2022

View reviewed changes

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ConcurrencyLimit.scala Outdated Show resolved Hide resolved

jiangpengcheng reviewed May 9, 2022

View reviewed changes

ningyougang reviewed May 9, 2022

View reviewed changes

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala Outdated Show resolved Hide resolved

ningyougang reviewed May 9, 2022

View reviewed changes

bdoyle0182 reviewed May 10, 2022

View reviewed changes

proposals/POEM-3-action-limit-for-namespace.md Outdated Show resolved Hide resolved

upgle mentioned this pull request May 16, 2022

[Proposal] POEM: Providing action limits for each namespace #5236

Merged

8 tasks

upgle force-pushed the namespace-limit-config branch 2 times, most recently from 7de6e59 to ac42477 Compare May 26, 2022 14:56

upgle removed the wip label May 28, 2022

upgle changed the title ~~[Proposal] Provide action limit configuration for each namespace~~ Provide action limit configuration for each namespace May 28, 2022

style95 reviewed Jun 15, 2022

View reviewed changes

upgle force-pushed the namespace-limit-config branch from 8b343d9 to 4222ceb Compare July 12, 2022 10:15

upgle force-pushed the namespace-limit-config branch from 4222ceb to 5442831 Compare July 25, 2022 02:24

style95 approved these changes Jul 26, 2022

View reviewed changes

style95 requested a review from KeonHee August 2, 2022 06:40

upgle force-pushed the namespace-limit-config branch from ea4930b to fa46215 Compare August 2, 2022 10:46

ningyougang approved these changes Aug 3, 2022

View reviewed changes

upgle force-pushed the namespace-limit-config branch from 0a312a4 to 0fbc5e8 Compare August 3, 2022 16:01

upgle added 26 commits August 9, 2022 11:42

Change config key

3f9f983

Refactor code

2e19c9d

Add ansible config

7b81826

Update annotation for maxConcurrent

83b4258

Add test case for limit api

c3c0715

Rename limit config key

1e18a31

Update swagger

cab437b

Update document

f1446ad

Add parameter size limit

227d431

Refactor code

0670a80

Update limits API

e8ce495

Rename allowedDuration -> allowedActionDuration

cbddbb6

Add test case for parameter limit

e438be1

Refactor code

b88a8b9

Add request payload limit for namespace

f8ca8a0

Check activation result size with namespace payload limit

467ac5a

Provide truncation size option for namespace

4ad15fa

Support scheduler

689d10a

Supports backwards compatibility for new limit config

703f3e1

Update wskadmin

0dd5202

Add parameter annotation for truncation

9dc2037

Fix test code for KubernetesContainerTests

00dc2fb

Fix test code for DockerContainerTests

94a4634

Fix test cases

3c63ea8

Fix build error

66ba50b

Fix build error

28ade66

upgle force-pushed the namespace-limit-config branch from 8881b5e to 28ade66 Compare August 9, 2022 02:51

style95 merged commit 61ca4c8 into apache:master Jan 25, 2023

style95 mentioned this pull request Jan 25, 2023

Fix the bug that match does not exhaustive #5370

Merged

22 tasks

Provide action limit configuration for each namespace #5229

Provide action limit configuration for each namespace #5229

Uh oh!

Conversation

upgle commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

3 types of limits

Limit doc

New namespace limit config

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

style95 commented May 3, 2022

Uh oh!

codecov-commenter commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ningyougang May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

upgle commented May 10, 2022

Uh oh!

style95 commented May 10, 2022

Uh oh!

ningyougang commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upgle commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upgle commented May 16, 2022

Uh oh!

style95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

style95 commented Jun 22, 2022

Uh oh!

style95 left a comment

Choose a reason for hiding this comment

Uh oh!

ningyougang commented Aug 3, 2022

Uh oh!

style95 commented Jan 25, 2023

Uh oh!

Uh oh!

upgle commented May 3, 2022 •

edited

Loading

codecov-commenter commented May 3, 2022 •

edited

Loading

ningyougang May 9, 2022 •

edited

Loading

ningyougang commented May 12, 2022 •

edited

Loading

upgle commented May 12, 2022 •

edited

Loading