Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Proposal] POEM: Providing action limits for each namespace #5236
Changes from 1 commit
6519747
4f044a0
5c2d8b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
openwhisk/ansible/roles/controller/tasks/deploy.yml
Line 223 in b0a88b5
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 theGET /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.
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