tysonnorris closed pull request #3941: to address #3918, reuse a container on 
applicationError
URL: https://github.com/apache/incubator-openwhisk/pull/3941
 
 
   

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/common/scala/src/main/scala/whisk/core/containerpool/Container.scala 
b/common/scala/src/main/scala/whisk/core/containerpool/Container.scala
index 7c46615d77..41d210de93 100644
--- a/common/scala/src/main/scala/whisk/core/containerpool/Container.scala
+++ b/common/scala/src/main/scala/whisk/core/containerpool/Container.scala
@@ -111,7 +111,7 @@ trait Container {
           Future.failed(
             InitializationError(
               result.interval,
-              
ActivationResponse.applicationError(Messages.timedoutActivation(timeout, 
true))))
+              
ActivationResponse.containerError(Messages.timedoutActivation(timeout, true))))
         } else {
           Future.failed(
             InitializationError(
@@ -148,7 +148,7 @@ trait Container {
       }
       .map { result =>
         val response = if (result.interval.duration >= timeout) {
-          
ActivationResponse.applicationError(Messages.timedoutActivation(timeout, false))
+          
ActivationResponse.containerError(Messages.timedoutActivation(timeout, false))
         } else {
           ActivationResponse.processRunResponseContent(result.response, 
logging)
         }
diff --git 
a/core/invoker/src/main/scala/whisk/core/containerpool/ContainerProxy.scala 
b/core/invoker/src/main/scala/whisk/core/containerpool/ContainerProxy.scala
index d04dfb3547..566207610b 100644
--- a/core/invoker/src/main/scala/whisk/core/containerpool/ContainerProxy.scala
+++ b/core/invoker/src/main/scala/whisk/core/containerpool/ContainerProxy.scala
@@ -153,7 +153,7 @@ class ContainerProxy(
             // the failure is either the system fault, or for docker actions, 
the application/developer fault
             val response = t match {
               case WhiskContainerStartupError(msg) => 
ActivationResponse.whiskError(msg)
-              case BlackboxStartupError(msg)       => 
ActivationResponse.applicationError(msg)
+              case BlackboxStartupError(msg)       => 
ActivationResponse.containerError(msg)
               case _                               => 
ActivationResponse.whiskError(Messages.resourceProvisionError)
             }
             // construct an appropriate activation and record it in the 
datastore,
@@ -419,9 +419,10 @@ class ContainerProxy(
 
     // Disambiguate activation errors and transform the Either into a 
failed/successful Future respectively.
     activationWithLogs.flatMap {
-      case Right(act) if !act.response.isSuccess => 
Future.failed(ActivationUnsuccessfulError(act))
-      case Left(error)                           => Future.failed(error)
-      case Right(act)                            => Future.successful(act)
+      case Right(act) if !act.response.isSuccess && 
!act.response.isApplicationError =>
+        Future.failed(ActivationUnsuccessfulError(act))
+      case Left(error) => Future.failed(error)
+      case Right(act)  => Future.successful(act)
     }
   }
 }
diff --git 
a/tests/src/test/scala/whisk/core/cli/test/WskRestBasicUsageTests.scala 
b/tests/src/test/scala/whisk/core/cli/test/WskRestBasicUsageTests.scala
index 23209087c6..c82445e53c 100644
--- a/tests/src/test/scala/whisk/core/cli/test/WskRestBasicUsageTests.scala
+++ b/tests/src/test/scala/whisk/core/cli/test/WskRestBasicUsageTests.scala
@@ -199,7 +199,7 @@ class WskRestBasicUsageTests extends TestHelpers with 
WskTestHelpers with WskAct
       withActivation(wsk.activation, wsk.action.invoke(name)) { activation =>
         val response = activation.response
         response.result.get.fields("error") shouldBe 
Messages.timedoutActivation(3 seconds, true).toJson
-        response.status shouldBe 
ActivationResponse.messageForCode(ActivationResponse.ApplicationError)
+        response.status shouldBe 
ActivationResponse.messageForCode(ActivationResponse.ContainerError)
       }
   }
 
@@ -285,7 +285,7 @@ class WskRestBasicUsageTests extends TestHelpers with 
WskTestHelpers with WskAct
 
       val run = wsk.action.invoke(name)
       withActivation(wsk.activation, run) { activation =>
-        activation.response.status shouldBe 
ActivationResponse.messageForCode(ActivationResponse.ApplicationError)
+        activation.response.status shouldBe 
ActivationResponse.messageForCode(ActivationResponse.ContainerError)
         activation.response.result.get
           .fields("error") shouldBe s"Failed to pull container image 
'$containerName'.".toJson
         activation.annotations shouldBe defined
diff --git 
a/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerTests.scala
 
b/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerTests.scala
index a7080af784..077afa9369 100644
--- 
a/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerTests.scala
+++ 
b/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerTests.scala
@@ -426,7 +426,7 @@ class DockerContainerTests
 
     val error = the[InitializationError] thrownBy await(init, initTimeout)
     error.interval shouldBe interval
-    error.response.statusCode shouldBe ActivationResponse.ApplicationError
+    error.response.statusCode shouldBe ActivationResponse.ContainerError
 
     // assert the finish log is there
     val end = LogMarker.parse(logLines.last)
@@ -474,7 +474,7 @@ class DockerContainerTests
     }
 
     val runResult = container.run(JsObject.empty, JsObject.empty, runTimeout)
-    await(runResult) shouldBe (interval, ActivationResponse.applicationError(
+    await(runResult) shouldBe (interval, ActivationResponse.containerError(
       Messages.timedoutActivation(runTimeout, false)))
 
     // assert the finish log is there
diff --git 
a/tests/src/test/scala/whisk/core/containerpool/kubernetes/test/KubernetesContainerTests.scala
 
b/tests/src/test/scala/whisk/core/containerpool/kubernetes/test/KubernetesContainerTests.scala
index bd3c948410..2221e91e28 100644
--- 
a/tests/src/test/scala/whisk/core/containerpool/kubernetes/test/KubernetesContainerTests.scala
+++ 
b/tests/src/test/scala/whisk/core/containerpool/kubernetes/test/KubernetesContainerTests.scala
@@ -242,7 +242,7 @@ class KubernetesContainerTests
 
     val error = the[InitializationError] thrownBy await(init, initTimeout)
     error.interval shouldBe interval
-    error.response.statusCode shouldBe ActivationResponse.ApplicationError
+    error.response.statusCode shouldBe ActivationResponse.ContainerError
 
     // assert the finish log is there
     val end = LogMarker.parse(logLines.last)
diff --git 
a/tests/src/test/scala/whisk/core/containerpool/test/ContainerProxyTests.scala 
b/tests/src/test/scala/whisk/core/containerpool/test/ContainerProxyTests.scala
index 25b0303271..4228c3dd5c 100644
--- 
a/tests/src/test/scala/whisk/core/containerpool/test/ContainerProxyTests.scala
+++ 
b/tests/src/test/scala/whisk/core/containerpool/test/ContainerProxyTests.scala
@@ -79,6 +79,11 @@ class ContainerProxyTests
     Interval(now, now.plusMillis(200))
   }
 
+  val errorInterval = {
+    val now = initInterval.end.plusMillis(75) // delay between init and run
+    Interval(now, now.plusMillis(150))
+  }
+
   val uuid = UUID()
 
   val message = ActivationMessage(
@@ -385,6 +390,73 @@ class ContainerProxyTests
     }
   }
 
+  it should "complete the transaction and reuse the container on a failed run 
IFF failure was applicationError" in within(
+    timeout) {
+    val container = new TestContainer {
+      override def run(parameters: JsObject, environment: JsObject, timeout: 
FiniteDuration)(
+        implicit transid: TransactionId): Future[(Interval, 
ActivationResponse)] = {
+        runCount += 1
+        //every other run fails
+        if (runCount % 2 == 0) {
+          Future.successful((runInterval, ActivationResponse.success()))
+        } else {
+          Future.successful((errorInterval, 
ActivationResponse.applicationError(("boom"))))
+        }
+      }
+    }
+    val factory = createFactory(Future.successful(container))
+    val acker = createAcker()
+    val store = createStore
+    val collector = createCollector()
+
+    val machine =
+      childActorOf(
+        ContainerProxy
+          .props(factory, acker, store, collector, InvokerInstanceId(0), 
poolConfig, pauseGrace = timeout))
+    registerCallback(machine)
+    preWarm(machine)
+
+    //first one will fail
+    run(machine, Started)
+
+    // Note that there are no intermediate state changes
+    //second one will succeed
+    run(machine, Ready)
+
+    //With exception of the error on first run, the assertions should be the 
same as in
+    //         `run an action and continue with a next run without pausing the 
container`
+    awaitAssert {
+      factory.calls should have size 1
+      container.initializeCount shouldBe 1
+      container.runCount shouldBe 2
+      collector.calls should have size 2
+      container.suspendCount shouldBe 0
+      container.destroyCount shouldBe 0
+      acker.calls should have size 2
+      store.calls should have size 2
+
+      val initErrorActivation = acker.calls(0)._2
+      initErrorActivation.duration shouldBe Some((initInterval.duration + 
errorInterval.duration).toMillis)
+      initErrorActivation.annotations
+        .get(WhiskActivation.initTimeAnnotation)
+        .get
+        .convertTo[Int] shouldBe initInterval.duration.toMillis
+      initErrorActivation.annotations
+        .get(WhiskActivation.waitTimeAnnotation)
+        .get
+        .convertTo[Int] shouldBe
+        Interval(message.transid.meta.start, 
initInterval.start).duration.toMillis
+
+      val runOnlyActivation = acker.calls(1)._2
+      runOnlyActivation.duration shouldBe Some(runInterval.duration.toMillis)
+      runOnlyActivation.annotations.get(WhiskActivation.initTimeAnnotation) 
shouldBe empty
+      
runOnlyActivation.annotations.get(WhiskActivation.waitTimeAnnotation).get.convertTo[Int]
 shouldBe {
+        Interval(message.transid.meta.start, 
runInterval.start).duration.toMillis
+      }
+    }
+
+  }
+
   /*
    * ERROR CASES
    */
@@ -423,7 +495,7 @@ class ContainerProxyTests
       override def initialize(initializer: JsObject,
                               timeout: FiniteDuration)(implicit transid: 
TransactionId): Future[Interval] = {
         initializeCount += 1
-        Future.failed(InitializationError(initInterval, 
ActivationResponse.applicationError("boom")))
+        Future.failed(InitializationError(initInterval, 
ActivationResponse.containerError("boom")))
       }
     }
     val factory = createFactory(Future.successful(container))
@@ -448,7 +520,7 @@ class ContainerProxyTests
       collector.calls should have size 1
       container.destroyCount shouldBe 1
       val activation = acker.calls(0)._2
-      activation.response shouldBe ActivationResponse.applicationError("boom")
+      activation.response shouldBe ActivationResponse.containerError("boom")
       activation.annotations
         .get(WhiskActivation.initTimeAnnotation)
         .get
@@ -458,12 +530,13 @@ class ContainerProxyTests
     }
   }
 
-  it should "complete the transaction and destroy the container on a failed 
run" in within(timeout) {
+  it should "complete the transaction and destroy the container on a failed 
run IFF failure was containerError" in within(
+    timeout) {
     val container = new TestContainer {
       override def run(parameters: JsObject, environment: JsObject, timeout: 
FiniteDuration)(
         implicit transid: TransactionId): Future[(Interval, 
ActivationResponse)] = {
         runCount += 1
-        Future.successful((initInterval, 
ActivationResponse.applicationError("boom")))
+        Future.successful((initInterval, 
ActivationResponse.containerError(("boom"))))
       }
     }
     val factory = createFactory(Future.successful(container))
@@ -487,7 +560,7 @@ class ContainerProxyTests
       container.runCount shouldBe 1
       collector.calls should have size 1
       container.destroyCount shouldBe 1
-      acker.calls(0)._2.response shouldBe 
ActivationResponse.applicationError("boom")
+      acker.calls(0)._2.response shouldBe 
ActivationResponse.containerError("boom")
       store.calls should have size 1
     }
   }
diff --git 
a/tests/src/test/scala/whisk/core/limits/MaxActionDurationTests.scala 
b/tests/src/test/scala/whisk/core/limits/MaxActionDurationTests.scala
index 1a854f1684..efd431cb2b 100644
--- a/tests/src/test/scala/whisk/core/limits/MaxActionDurationTests.scala
+++ b/tests/src/test/scala/whisk/core/limits/MaxActionDurationTests.scala
@@ -86,7 +86,7 @@ class MaxActionDurationTests extends TestHelpers with 
WskTestHelpers with WskAct
           pollPeriod = 1.minute,
           totalWait = TimeLimit.MAX_DURATION + 2.minutes) { activation =>
           withClue("Activation result not as expected:") {
-            activation.response.status shouldBe 
ActivationResponse.messageForCode(ActivationResponse.ApplicationError)
+            activation.response.status shouldBe 
ActivationResponse.messageForCode(ActivationResponse.ContainerError)
             activation.response.result shouldBe Some(
               JsObject("error" -> 
Messages.timedoutActivation(TimeLimit.MAX_DURATION, init = false).toJson))
             activation.duration.toInt should be >= 
TimeLimit.MAX_DURATION.toMillis.toInt


 

----------------------------------------------------------------
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:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to