-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Proposal] POEM: Providing action limits for each namespace #5236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I'm still not clear about the differences between
looks like the |
|
It seems many pull requests are failing because of the following reason and this PR is also one of them.
Need to see if there is any issue in the dependency resolution. |
``` | ||
|
||
#### Using wskadmin command (tool) | ||
- In general, it is recommended to use a wskadmin rather than modify the DB directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think it would be nice to have admin routes in the controller to update this data and you give just one namespace that the operators have access to authorization to hit the admin route path. wskadmin
isn't really any different from editing the db directly security / authorization wise as it's just a cli tool that makes the db api calls.
That doesn't really preclude this proposal since this is just adding operator limits that can be configured that way, which is already done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now if it's the owners of an individual namespace that can tune these min / max values, then that's definitely a problem to hand them the keys to wskadmin. I don't think that's what's being proposed here though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referred to the previous implementation for namespace limits, and I agree it is necessary to introduce a namespace with administrator privileges and an administrator API. I think it can be achieved through another proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting with wskadmin
or modifications to couch is a fair transition. I also see the value of making this part of an OW Admin API. Regardless, I believe the GET /api/v1/namespaces[_/limits]
should return not just the namespace name but the limits for that namespace as shown below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the wskadmin tool is the same as modifying the db directly, so I will delete the ambiguous phrase.
maxActionLogs | integer (unit: MB) | maximum activation log size for namespace | ||
minActionTimeout | integer (unit: milliseconds) | minimum action time limit for namespace | ||
maxActionTimeout | integer (unit: milliseconds) | maximum action time limit for namespace | ||
minActionConcurrency | integer | minimum action concurrency limit for namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be renamed. This is representing container activation concurrency. If we are ever to add action concurrency like namespace concurrency but more fine grained, this will then be very confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comments. I've been thinking a lot too but I can't come up with a good config name.
And a lot of code already calls it the action concurrency limit. I think using a new name can also be more confusing.
openwhisk/tests/src/test/scala/org/apache/openwhisk/core/limits/ConcurrencyTests.scala
Line 53 in 59b67fe
behavior of "Action concurrency limits" |
"CONFIG_whisk_concurrencyLimit_min": "{{ limit_action_concurrency_min | default() }}" |
- concurrentInvocations: Max allowed concurrent in-flight invocations for a namespace level.
- minActionConcurrency: Minimum allowed concurrent invocations for an action level.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5236 +/- ##
===========================================
+ Coverage 44.53% 74.80% +30.26%
===========================================
Files 238 238
Lines 13957 13965 +8
Branches 570 582 +12
===========================================
+ Hits 6216 10446 +4230
+ Misses 7741 3519 -4222 ☔ View full report in Codecov by Sentry. |
@bdoyle0182 @rabbah |
``` | ||
|
||
#### Using wskadmin command (tool) | ||
- In general, it is recommended to use a wskadmin rather than modify the DB directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting with wskadmin
or modifications to couch is a fair transition. I also see the value of making this part of an OW Admin API. Regardless, I believe the GET /api/v1/namespaces[_/limits]
should return not just the namespace name but the limits for that namespace as shown below.
- In general, it is recommended to use a wskadmin rather than modify the DB directly. | ||
- There is plan to provide the feauture to change namespace limits in wskadmin. | ||
|
||
### Namespace Limit API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you intend to extend the limits document/attachment to include the namespace limits, is that right? I didn't see it called out explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rabbah Yes right.
The the limits document is described here:
https://github.com/apache/openwhisk/pull/5236/files#diff-bf63bbe4a2d067875e015af1374ab7ad40416cdb02d0f23b2c739f063e402848R82
Co-authored-by: rodric rabbah <rodric@gmail.com>
I will merge this as it has been open for a while. |
Description
This is the POEM documentation for new feature providing action limits for each namespace.
I refer to this guide document for POEM: https://github.com/apache/openwhisk/blob/master/proposals/README.md
Related issue and scope
My changes affect the following components
Types of changes
Checklist:
Title
Providing action limits for each namespace
Status
Summary and Motivation
This POEM proposes a new feature that allows administrators to set action limits (memory, timeout, log, and concurrency) for each namespace.
Sometimes some users want to make an action with more memory and longer duration. But, Openwhisk only has a system limit for action shared by all namespaces.
There is no way to adjust the action limit for a few users, and changing the action limit setting will affect all users.
In some private environments, you can operate Openwhisk more flexibly by providing different action limits.
(For example, providing high memory only to some users.)
Proposed changes
3 types of action limits
There was only a system limit shared by all namespaces, but two more concepts for namespace limits are added.
Limit configs for namespace
ByteSize
string. (e.g. 1 MB, 512 KB...)The following settings are new:
Limit document for CouchDB
You can set namespace limits with
{namespace}/limits
document just like any other existing settings (invocationsPerMinute, concurrentInvocations..).Using wskadmin command (tool)
Namespace Limit API
User can get the applied action limits of the namespace by the namespace limit API.
If the namespace's action limit is not set, the default namespace limit value will be returned.
System API (URI path: /)
A namespace default limit information is additionally provided separately from the previously provided system limit information.
Preview
Backward compatibility
For backward compatibility, if there is no namespace default limit setting, it is replaced with a system limit.
As the namespace default limit is the same as the system limit, so the administrator cannot set the namespace limit, and the user can create actions with resources (memory, logs, timeout...) up to the system limit as before.
Namespace limit validation
Previously, system limits were validated when unmarshalls the ActionLimits object from the user request.
However, at the time of unmarshalls the user requests, the namespace's action limit cannot be known and the limit value cannot be included in an error message, so the validation must be performed after unmarshalling.
Therefore, the code to perform this validation has been added to the controller, scheduler, and invoker.
1. Validate action limits when the action is created in the controller
When an action is created in the controller, make sure that the action limits do not exceed the system limits and namespace limits.
If the namespace limits or system limits are exceeded, the namespace limit value must be returned as an error message in the response body.
2. Validate action limits when the action is executed in the invoker
When the action is executed, the invoker must checks whether the action limit exceeds the system limit and namespace limits.
If the limit of the action to be executed exceeds the limit, an application error with
Messages.actionLimitExceeded
message is returned and invocation is aborted.3. Validate action limits when the action is executed in the invoker with the scheduler
The invoker that works with the scheduler should check namespace limits when creating containers and handling activations.
Handling invalid namespace limits
Because there is no admin API to handle namespace limits, the CouchDB document may have namespace limit values that exceed the system limits.
But, If there is a namespace limit that exceeds the system limit, the namespace limit is lowered to the system limit.