[GitHub] rabbah closed pull request #3177: Avoid repeated throttle check for sequences

[GitBox <gi...@apache.org>]

rabbah closed pull request #3177: Avoid repeated throttle check for sequences
URL: https://github.com/apache/incubator-openwhisk/pull/3177
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/core/controller/src/main/scala/whisk/core/controller/Actions.scala b/core/controller/src/main/scala/whisk/core/controller/Actions.scala
index 0f30d370d8..e08d0e44a1 100644
--- a/core/controller/src/main/scala/whisk/core/controller/Actions.scala
+++ b/core/controller/src/main/scala/whisk/core/controller/Actions.scala
@@ -385,7 +385,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
     exec match {
       case Some(seq: SequenceExec) =>
         logging.info(this, "checking if sequence components are accessible")
-        entitlementProvider.check(user, right, referencedEntities(seq))
+        entitlementProvider.check(user, right, referencedEntities(seq), noThrottle = true)
       case _ => Future.successful(true)
     }
   }
@@ -395,7 +395,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
     exec match {
       case Some(seq: SequenceExecMetaData) =>
         logging.info(this, "checking if sequence components are accessible")
-        entitlementProvider.check(user, right, referencedEntities(seq))
+        entitlementProvider.check(user, right, referencedEntities(seq), noThrottle = true)
       case _ => Future.successful(true)
     }
   }
diff --git a/core/controller/src/main/scala/whisk/core/entitlement/Entitlement.scala b/core/controller/src/main/scala/whisk/core/entitlement/Entitlement.scala
index 68d57c42dc..734fa09025 100644
--- a/core/controller/src/main/scala/whisk/core/entitlement/Entitlement.scala
+++ b/core/controller/src/main/scala/whisk/core/entitlement/Entitlement.scala
@@ -209,18 +209,23 @@ protected[core] abstract class EntitlementProvider(config: WhiskConfig, loadBala
    * @param user the subject identity to check rights for
    * @param right the privilege the subject is requesting (applies to the entire set of resources)
    * @param resources the set of resources the subject requests access to
+   * @param noThrottle ignore throttle limits
    * @return a promise that completes with success iff the subject is permitted to access all of the requested resources
    */
-  protected[core] def check(user: Identity, right: Privilege, resources: Set[Resource])(
+  protected[core] def check(user: Identity, right: Privilege, resources: Set[Resource], noThrottle: Boolean = false)(
     implicit transid: TransactionId): Future[Unit] = {
     val subject = user.subject
 
     val entitlementCheck: Future[Unit] = if (user.rights.contains(right)) {
       if (resources.nonEmpty) {
         logging.info(this, s"checking user '$subject' has privilege '$right' for '${resources.mkString(", ")}'")
-        checkSystemOverload(right)
-          .flatMap(_ => checkUserThrottle(user, right, resources))
-          .flatMap(_ => checkConcurrentUserThrottle(user, right, resources))
+        val throttleCheck =
+          if (noThrottle) Future.successful(())
+          else
+            checkSystemOverload(right)
+              .flatMap(_ => checkUserThrottle(user, right, resources))
+              .flatMap(_ => checkConcurrentUserThrottle(user, right, resources))
+        throttleCheck
           .flatMap(_ => checkPrivilege(user, right, resources))
           .flatMap(checkedResources => {
             val failedResources = checkedResources.filterNot(_._2)
diff --git a/tests/src/test/scala/limits/ThrottleTests.scala b/tests/src/test/scala/limits/ThrottleTests.scala
index 7c59aec150..dfee5c0a04 100644
--- a/tests/src/test/scala/limits/ThrottleTests.scala
+++ b/tests/src/test/scala/limits/ThrottleTests.scala
@@ -327,8 +327,12 @@ class NamespaceSpecificThrottleTests
   val oneProps = getAdditionalTestSubject("oneSubject")
   wskadmin.cli(Seq("limits", "set", oneProps.namespace, "--invocationsPerMinute", "1", "--firesPerMinute", "1"))
 
+  // Create a subject where the rate limits are set to 1 for testing sequences
+  val oneSequenceProps = getAdditionalTestSubject("oneSequenceSubject")
+  wskadmin.cli(Seq("limits", "set", oneSequenceProps.namespace, "--invocationsPerMinute", "1", "--firesPerMinute", "1"))
+
   override def afterAll() = {
-    Seq(zeroProps, zeroConcProps, oneProps).foreach { wp =>
+    Seq(zeroProps, zeroConcProps, oneProps, oneSequenceProps).foreach { wp =>
       val ns = wp.namespace
       disposeAdditionalTestSubject(ns)
       withClue(s"failed to delete temporary limits for $ns") {
@@ -400,6 +404,44 @@ class NamespaceSpecificThrottleTests
     }, 2, Some(1.second))
   }
 
+  // One sequence invocation should count as one invocation for rate throttling purposes.
+  // This is independent of the number of actions in the sequences.
+  it should "respect overridden rate-throttles of 1 for sequences" in withAssetCleaner(oneSequenceProps) {
+    (wp, assetHelper) =>
+      implicit val props = wp
+
+      val actionName = "oneAction"
+      val sequenceName = "oneSequence"
+
+      assetHelper.withCleaner(wsk.action, actionName) { (action, _) =>
+        action.create(actionName, defaultAction)
+      }
+
+      assetHelper.withCleaner(wsk.action, sequenceName) { (action, _) =>
+        action.create(sequenceName, Some(s"$actionName,$actionName"), kind = Some("sequence"))
+      }
+
+      val deployedControllers = WhiskProperties.getControllerHosts.split(",").length
+
+      // One invoke should be allowed.
+      wsk.action
+        .invoke(sequenceName, expectedExitCode = TestUtils.DONTCARE_EXIT)
+        .exitCode shouldBe TestUtils.SUCCESS_EXIT
+
+      // One invoke should be allowed, the second one throttled.
+      // Due to the current implementation of the rate throttling,
+      // it is possible that the counter gets deleted, because the minute switches.
+      retry({
+        val results = (1 to deployedControllers + 1).map { _ =>
+          wsk.action.invoke(sequenceName, expectedExitCode = TestUtils.DONTCARE_EXIT)
+        }
+        results.map(_.exitCode) should contain(TestUtils.THROTTLED)
+        results.map(_.stderr).mkString should {
+          include(prefix(tooManyRequests(0, 0))) and include("allowed: 1")
+        }
+      }, 2, Some(1.second))
+  }
+
   it should "respect overridden concurrent throttle of 0" in withAssetCleaner(zeroConcProps) { (wp, assetHelper) =>
     implicit val props = wp
     val actionName = "zeroConcurrentAction"


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services