Skip to content

[POEM] Allow Users To Configure Max Action Container Concurrency Under Their Namespace Limit #5288

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

Merged

Conversation

bdoyle0182
Copy link
Contributor

Description

POEM for allowing users to configure fairness within their namespace across their actions. This allows users to control how much an action can autoscale within the confines of the system admin controlled namespace concurrency limit.

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Scheduler
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • 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.

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.42%. Comparing base (4016aa8) to head (4644eb1).
Report is 125 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5288      +/-   ##
==========================================
- Coverage   79.96%   75.42%   -4.54%     
==========================================
  Files         238      238              
  Lines       14122    14194      +72     
  Branches      599      606       +7     
==========================================
- Hits        11292    10706     -586     
- Misses       2830     3488     +658     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It generally looks good to me.

## Proposed changes: Architecture Diagram (optional), and Design

Add a optional `maxContainerConcurrency` limit field to action documents in the limits section. This limit will be used in the scheduler when deciding
if there is capacity for the action to scale up more containers. Previously, the scheduler was completely naive of functions across a namespace when provisioning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if there is capacity for the action to scale up more containers. Previously, the scheduler was completely naive of functions across a namespace when provisioning
if there is capacity for the action to scale out more containers. Previously, the scheduler was completely naive of functions across a namespace when provisioning

the namespace limit as the capacity limit. Therefore, there is no action required or side effects or coordination required from the system admin wanting to lower the namespace limit.
However, if the user wants to redeploy the same function with the same limit that is now over the namespace limit; the api will now reject the action upload until the action limit
is lowered below the new namespace limit.
- A user may want to update their action to go back to just relying on the namespace limit. Since updates to action documents copy over limits in the update even if not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be included in the PR too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will, just haven't gotten a chance to add it yet

the action will rely on the namespace limit again.
- When creating an action, the api will validate that your action container concurrency limit is less than or equal to the namespace concurrency limit. If it is greater,
the upload will fail with a BadRequest and error message that the limit must be less than the namespace limit with the namespace limit value included in the message.
- If the system admin lowers a namespace's concurrency limit below an amount that an existing action document has already configured, it will not break the action.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, if the sum of the maxContainerConcurrency of all actions exceeds the namespace limit, how does it guarantee fairness?
Do we leave it to the users?

Copy link
Contributor Author

@bdoyle0182 bdoyle0182 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's the user's responsibility. The idea is that users can configure high priority functions to have a high limit such that it can still potentially get more than another low priority function and that lower priority action may not get up to its lower limit. As an example of a namespace with a limit of 20 and 3 actions A, B, and C:

  • A configures limit of 5
  • B configures limit of 5
  • C configures limit of 15

C gets a burst of traffic that uses all 15 of its limit, there is now only 5 remaining for both A and B. That's fine as a user might want this level of scaling at C at the expense of the max A and B can now scale to being 2 or 3 each instead of 5.

Or the user can configure their limits perfectly up to their namespace limit to guarantee fairness such that A gets 5, B gets 5, and C gets 10. I think this level of flexibility to the user is a good thing.

I didn't want to refernce aws lambda explicitly, but in comparison to their concept of reserved concurrency for individual functions I think this provides more flexibility to the user. Reserved concurrency on lambda takes away from the total pool when configuring on a function so if I configure 5 to an action and the account limit is 20, there's now 15 capacity for other functions at all times. Well what if that function is not doing something most of the time? You've now taken away capacity permanently from your pool for a function that barely runs in exchange for a guarantee it will always be able to scale up to 5. In this proposal, you still have the ability to give yourself that guarantee if you provision evenly across your namespace as the user; but also have the flexibility to be smarter with what your traffic patterns look like in giving yourself additional overprovisioned / high priority capacity across your functions (which is sort of what serverless is all about.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so this is mainly to limit an action that may consume huge resources.
What is the relationship between actions with/without maxContainerConcurrency when they are invoked concurrently?
Even if action A, B, and C are configured like the above limits, any actions can still be invoked as long as there is enough capacity in the namespace, is that correct?

Copy link
Contributor Author

@bdoyle0182 bdoyle0182 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes every action except A, B, and C. A, B, and C can only have up to the max of whatever their configured limit is regardless of whether their is still additional capacity in the namespace. If action D came in with no limit configured for itself, its action limit is just inferred by the scheduler to be the namespace limit so it can have up to the full 20.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thus if no limits are configured for any action in a namespace, the behavior remains identical to the existing behavior of openwhisk; any action can have up to the full namespace limit making this feature completely opt-in / non-breaking to the existing paradigm

Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.
It is fully backward compatible.

@bdoyle0182 Do you want to update the Todo part or merge this as-is?

@bdoyle0182 bdoyle0182 merged commit a328792 into apache:master Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants