Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[New Scheduler]Implement FPCEntitlementProvider #5029

Merged
merged 2 commits into from
Jan 29, 2021
Merged

[New Scheduler]Implement FPCEntitlementProvider #5029

merged 2 commits into from
Jan 29, 2021

Conversation

jiangpengcheng
Copy link
Contributor

Implement a FPCEntitlementProvider

Description

A new FPCEntitlementProvider for scheduler:
Since the new scheduler is using different throttler logic, so this PR is created

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
  • 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.

style95
style95 reviewed Nov 18, 2020
View reviewed changes
core/controller/src/main/scala/org/apache/openwhisk/core/loadBalancer/LoadBalancer.scala
@@ -75,6 +75,9 @@ trait LoadBalancer {

/** Gets the size of the cluster all loadbalancers are acting in */
def clusterSize: Int = 1

/** Gets the throttling for given action. */
def checkThrottle(namespace: EntityPath, action: String): Boolean = false
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if you share how this will work in a new load balancer.

bdoyle0182
bdoyle0182 reviewed Nov 18, 2020
View reviewed changes
core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/FPCEntitlement.scala
loadBalancer.checkThrottle(user.namespace.name.toPath, res.fqname)
}
if (checks.contains(true)) {
Future.failed(RejectRequest(TooManyRequests, "Too many requests"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be consistent with the other throttling rejections where it differentiates between per minute and concurrent?

Copy link
Member

Choose a reason for hiding this comment

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

You may find this useful.
https://cwiki.apache.org/confluence/display/OPENWHISK/Design+consideration#Designconsideration-2.4Newconceptoftheresource

Copy link
Member

Choose a reason for hiding this comment

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

So now we only have one throttling limit that is the number of concurrent containers.

bdoyle0182
bdoyle0182 reviewed Nov 18, 2020
View reviewed changes
core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
@@ -153,7 +153,7 @@ protected[core] abstract class EntitlementProvider(
activationThrottleCalculator(config.actionInvokeConcurrentLimit.toInt, _.limits.concurrentInvocations))

private val messagingProvider = SpiLoader.get[MessagingProvider]
private val eventProducer = messagingProvider.getProducer(this.config)
protected val eventProducer = messagingProvider.getProducer(this.config)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this get used later? I don't see where this is needed in the child

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, there was one metric for throttling in FPCEntitlementProvider, I will add that

@bdoyle0182
Copy link
Contributor

LGTM

@jiangpengcheng
Copy link
Contributor Author

@style95 @bdoyle0182 below is a brief introducation for throttling in new scheduler:

new scheduler is using a different throttling logic, it doesn't consider the per minute and concurrent limitations :

  • for a namespace, it limit how many containers a namespace can have, if exceed, it enables throttle for the namespace

  • for an action, it limit how many activations can wait in a queue, if exceed, it enables throttle for the action

when any throttle is enabled, scheduler will save it into ETCD, and the FPCLoadBalancer will listen to such throttling data and save/update them in a map

And for an incoming activation, the FPCLoadBalancer will use below logic to decide whether reject or accpet it

     * Action Throttle true      -> 429
     *                 false     -> Pass
     *                 not exist -> Namespace Throttle true      -> 429
     *                                                 false     -> Pass
     *                                                 not exist -> Pass

@bdoyle0182
Copy link
Contributor

Very happy to hear about the new throttling mechanism. Will the throttles remain configurable per namespace?

@jiangpengcheng
Copy link
Contributor Author

yes, we can configure that for each namespace

style95
style95 approved these changes Jan 28, 2021
View reviewed changes
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.

LGTM

@bdoyle0182 bdoyle0182 merged commit efdbd60 into apache:master Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdoyle0182 bdoyle0182 bdoyle0182 left review comments

@style95 style95 style95 approved these changes

Assignees
No one assigned
Labels
scheduler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jiangpengcheng @bdoyle0182 @style95