Port Controller from Spray to Akka #2218

dubee · 2017-05-05T19:08:15Z

Refactor controller to use Akka as Spray is no longer supported

dubee · 2017-05-05T19:09:21Z

Need to consider if we want to replace the existing controller with the port to Akka, or have tons of duplicate code hanging around for version 2 of the controller.

dubee · 2017-05-05T19:09:49Z

Still need to port over triggers, activations, rules, and packages. Opening the PR for comments though.

dubee · 2017-05-05T19:12:21Z

core/controller/src/main/scala/whisk/core/controller/v2/Actions.scala

+    private lazy val actionSequenceLimit = whiskConfig.actionSequenceLimit.toInt
+
+    /** Custom deserializer for timeout query parameter. */
+    private implicit val stringToTimeoutDeserializer = new FromStringDeserializer[FiniteDuration] {


Need to fix this to deserialize FiniteDuration with Akka.

rabbah · 2017-05-05T20:01:29Z

Need to consider if we want to replace the existing controller with the port to Akka, or have tons of duplicate code hanging around for version 2 of the controller.

I imagined - where necessary - to refactor the handlers from v1 so they are free of routing dependencies and so v1 vs v2 just become routing specific, with serializers for the corresponding http response types.

Definitely do not want to duplicate/clone-and-own all of v1 as v2.

markusthoemmes · 2017-05-08T05:49:14Z

Can we port v1 as is, without going to v2? Benefits include:

Move away from an unsupported spray.
Move away from an unsupported scala version (spray makes us stay on 2.11, while 2.12 is out)

markusthoemmes · 2017-05-10T21:46:16Z

Quick q on the procedure here: Do we want to copy all the code or do we want to migrate v1 to akka-http in-place? Asking specifically because I'd like to keep the git history of these files intact.

rabbah · 2017-05-11T04:55:57Z

+1 I do think there's a way to do the migration to Akka-http without all this copying.

dubee · 2017-05-11T20:46:02Z

POST requests currently require a payload. Otherwise, an error response is generated. Not sure if this is new in Akka, @rabbah. Will have to figure it out.

Another defect is that generated error responses are not currently JSON formatted.

dubee · 2017-05-11T20:47:17Z

For the port, I plan on overwriting the existing controller so that there isn't a lot of duplicate code. Will be able to reuse the existing test suites this way. For now, I'm just working with the duplicate code until I get everything converted over. Just need web actions now.

dubee · 2017-05-11T20:49:13Z

Side note: keep getting a consul test failure in Travis when running two controllers.

dubee · 2017-06-07T14:47:06Z

Currently I have replaced the Spray controller with the Akka controller. However, controller unit tests are failing as they are still using Spray, so these tests will have to be updated. Also the BasicHttpService will have to be updated to use Akka since the controller and invoker extend it.

dubee · 2017-06-19T21:24:45Z

I'm going to squash all the commits into one commit due to the nightmare that just resulted after rebasing against upstream.

rabbah · 2017-08-09T14:37:49Z

core/controller/src/main/scala/whisk/core/controller/Actions.scala

+        Unmarshaller.byteStringUnmarshaller.forContentTypes(`application/json`).mapWithCharset { (data, charset) =>
+            val decoded = data.decodeString(charset.nioCharset.name)
+
+            Try {


perhaps this should check decoded.length > 0 and decode else return JsObject and skip the try/catch in that case.

FWIW, here is the unmarshaller I wrote while debugging performance issues (it's the original one copied and added the data.size == 0 case)

implicit val entityToJsObject: FromEntityUnmarshaller[JsObject] = Unmarshaller.byteStringUnmarshaller.forContentTypes(`application/json`).mapWithCharset { (data, charset) => if (data.size == 0) { JsObject() } else { val input = if (charset == HttpCharsets.`UTF-8`) ParserInput(data.toArray) else ParserInput(data.decodeString(charset.nioCharset)) JsonParser(input).asJsObject } }

rabbah · 2017-08-12T00:52:46Z

common/scala/src/main/scala/whisk/http/BasicHttpService.scala

-            DebuggingDirectives.logRequest(logRequestInfo _) {
-                DebuggingDirectives.logRequestResponse(logResponseInfo _) {
-                    routes
+            handleRejections(customRejectionHandler) {


@dubee why did the directive order change?

rabbah · 2017-08-12T00:55:00Z

common/scala/src/main/scala/whisk/http/BasicHttpService.scala


    /** Assigns transaction id to every request. */
    protected val assignId = extract(_ => transid())

-    /** Rejection handler to terminate connection on a bad request. Delegates to Spray handler. */
-
-    protected def customRejectionHandler(implicit transid: TransactionId) = RejectionHandler {


is my understanding correct that the prioritization above lowers the priority on the auth rejection?

rabbah · 2017-08-12T08:39:56Z

common/scala/src/main/scala/whisk/http/BasicHttpService.scala

-
-    protected def customRejectionHandler(implicit transid: TransactionId) = RejectionHandler {
-        case rejections => {
-            logging.info(this, s"[REJECT] $rejections")


are we losing the rejection log message?

i'm ok dropping this - it's of nominal value.

rabbah · 2017-08-12T08:44:55Z

core/controller/src/main/scala/whisk/core/controller/Activations.scala

-                case Success(e) => Right(e)
-                case Failure(t) => Left(MalformedContent(t.getMessage))
+                case Success(e) => e
+                case Failure(t) => throw new IllegalArgumentException(s"Parameter is not a valid value for a entity name: $value")


i'd move all three throw here to messages to Messages.

pushed a commit to do this

rabbah · 2017-08-12T08:49:07Z

core/controller/src/main/scala/whisk/core/controller/AuthenticatedRoute.scala

-                    case t                        => Left(CustomRejection(ServiceUnavailable))
-                }
+    def basicAuth[A](verify: Option[BasicHttpCredentials] => Future[Option[A]]) = {
+        authenticateOrRejectWithChallenge[BasicHttpCredentials, A] { creds =>


the code above tries to return a more informative error when authentication fails because the database is not available. I think we are losing this feature now. I don't think there was a test for this (it will require mocking a db connection and failing it).

you can simulate this locally and verify this is a small change in behavior - if i'm right, then please open an issue to address it separately (this time with a test).

edit: confirmed that the new logic reports an internal server error if the database connection is severed instead of service unavailable.

opened #2607

rabbah · 2017-08-12T08:54:16Z

core/controller/src/main/scala/whisk/core/controller/RestAPIs.scala

 protected[controller] trait RespondWithHeaders extends Directives {
-    val allowOrigin = `Access-Control-Allow-Origin`(AllOrigins)
+    val allowOrigin = `Access-Control-Allow-Origin`.*


rabbah · 2017-08-12T08:56:04Z

core/controller/src/main/scala/whisk/core/controller/Triggers.scala

        complete(OK, trigger.withoutRules)
    }
+
+    implicit val entityToJsObject: FromEntityUnmarshaller[JsObject] =


this method is repeated twice?
occurs also in Actions.scala.

pushed a commit to address this.

rabbah · 2017-08-12T09:01:01Z

core/controller/src/main/scala/whisk/core/controller/WebActions.scala

-                    if (contentType.mediaType.binary) {
-                        Try(JsString(Base64.getEncoder.encodeToString(data.toByteArray))) match {
+                case HttpEntity.Strict(contentType, data) =>
+                    // application/json is not a binary type in Akka, but is binary in Spray


this will make a lot of users happy 🎉
opened #2606.

rabbah · 2017-08-12T09:03:22Z

tests/src/test/scala/whisk/core/controller/test/AuthenticateTests.scala

-
-        }
-    }
-


what are we losing by removing these tests?

rabbah · 2017-08-12T09:05:29Z

tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala

                        status should be(RequestEntityTooLarge)
                        val expectedErrorMsg = Messages.entityTooBig(SizeError(
                            fieldDescriptionForSizeError,
-                            (largeEntity.length + 13).B,
+                            (largeEntity.length + 8).B,


+5B again - guessing quotes are the difference.

rabbah · 2017-08-12T09:40:00Z

@dubeejw great job on getting all the issues sorted out in this migration!
a couple of small nits to address and I'll merge.

Fix logging of request.

rabbah · 2017-08-12T13:48:49Z

common/scala/src/main/scala/whisk/http/BasicHttpService.scala

-                    }
-                }
-        }
-        case CustomRejection(status, cause) :: _ => complete(status, ErrorResponse(cause, transid))


this in particular was used for the auth failure mode #2607.

we can delete CustomRejection since it will be unused now - assuming we keep this behavior.

satybald · 2017-08-13T08:32:36Z

tests/src/test/scala/whisk/core/controller/test/TriggersApiTests.scala

@@ -214,7 +218,7 @@ class TriggersApiTests extends ControllerTestCommon with WhiskTriggersApi {
            Parameters(key.toString, "a" * 10)
        } reduce (_ ++ _)
        val content = s"""{"parameters":$parameters}""".parseJson.asJsObject
-        Put(s"$collectionPath/${aname}", content) ~> sealRoute(routes(creds)) ~> check {
+        Put(s"$collectionPath/${aname}", content) ~> Route.seal(routes(creds)) ~> check {


just wondering why would want to use curly braces syntax instead of omitting them?

Around aname? Historically this may have been something else then copy paste. Should be removed.

sorry, yep around aname variable.

👍 if you want to remove them and send a pr

yup, I'll do that ;)

rabbah · 2017-08-13T13:30:50Z

core/controller/src/main/scala/whisk/core/controller/Triggers.scala

-                                        case Failure(usr: UnsuccessfulResponseException) if usr.response.status == StatusCodes.NotFound =>
+                                    Http().singleRequest(request).map {
+                                        case HttpResponse(StatusCodes.OK, headers, entity, _) =>
+                                            logging.info(this, s"successfully invoked ${rule.action} -> ")


looks like we missed this - was previously logging the activation id.

also looks like something is not quite right here - even after firing the trigger correctly, inspecting the controller logs shows:

[2017-08-13T13:48:32.233Z] [WARN] [#tid_7] [TriggersApi] action guest/a could not be invoked due to Source(SourceShape(StatefulMapConcat.out), CompositeModule [3c60a25e] Name: iterableSource Modules: (singleSource) GraphStage(SingleSource(List(ByteString(123, 34, 97, 99, 116, 105, 118, 97, 116, 105, 111, 110, 73, 100, 34, 58, 34, 98, 97, 53, 99, 57, 51, 55, 102, 51, 97, 57, 101, 52, 48, 56, 52, 56, 98, 49, 102, 100, 54, 99, 53, 54, 54, 52, 55, 48, 100, 50, 50, 34, 125)))) [41c5bf50] (unnamed) [703a0c97] copy of GraphStage(StatefulMapConcat) [50915aa5] Downstreams: single.out -> StatefulMapConcat.in Upstreams: StatefulMapConcat.in -> single.out MatValue: Atomic(singleSource[41c5bf50]))

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

dubee commented May 5, 2017

View reviewed changes

csantanapr added the wip label May 8, 2017

dubee force-pushed the v2 branch 4 times, most recently from 245b09b to 2d363da Compare May 10, 2017 19:06

dubee force-pushed the v2 branch from 6f280fc to c08be91 Compare May 11, 2017 20:40

dubee force-pushed the v2 branch 2 times, most recently from 65da9c0 to c5b7ef9 Compare May 15, 2017 19:57

dubee force-pushed the v2 branch from c5b7ef9 to c95a0fc Compare May 24, 2017 17:49

dubee force-pushed the v2 branch 5 times, most recently from 8620bed to 3add84e Compare June 6, 2017 19:57

dubee force-pushed the v2 branch 2 times, most recently from dd145eb to 4021c82 Compare June 13, 2017 14:39

dubee force-pushed the v2 branch 2 times, most recently from f213d4e to 0ec80f9 Compare June 20, 2017 20:31

dubee force-pushed the v2 branch 3 times, most recently from 508312c to 2af8d4b Compare August 8, 2017 16:02

rabbah reviewed Aug 9, 2017

View reviewed changes

dubee added 2 commits August 10, 2017 13:39

Port Controller from Spray to Akka

5d95f83

Increase max-connections and Update JSON Unmarshaller

fdcf8cd

dubee force-pushed the v2 branch from 2af8d4b to fdcf8cd Compare August 10, 2017 18:18

rabbah reviewed Aug 12, 2017

View reviewed changes

rabbah self-assigned this Aug 12, 2017

rabbah added the pgapproved Pipeline has approved this change. label Aug 12, 2017

rabbah approved these changes Aug 12, 2017

View reviewed changes

Move empty entity to JSON object serializer.

5e7c67c

Fix logging of request.

rabbah force-pushed the v2 branch from 18d4462 to 5e7c67c Compare August 12, 2017 13:26

rabbah added 2 commits August 12, 2017 09:33

Move error messages to canonical singleton.

e7f08f8

Remove deadcode.

ede5000

rabbah reviewed Aug 12, 2017

View reviewed changes

rabbah merged commit d564ea9 into apache:master Aug 12, 2017

satybald reviewed Aug 13, 2017

View reviewed changes

rabbah reviewed Aug 13, 2017

View reviewed changes

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Aug 17, 2017

Port Controller from Spray to Akka (apache#2218)

c3ea829

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Aug 17, 2017

Port Controller from Spray to Akka (apache#2218)

c5ed9ce

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Sep 18, 2017

Port Controller from Spray to Akka (apache#2218)

2153503

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017

Port Controller from Spray to Akka (apache#2218)

1d4a2b7

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017

Port Controller from Spray to Akka (apache#2218)

c1df65b

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017

Port Controller from Spray to Akka (apache#2218)

a313ac6

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 13, 2017

Port Controller from Spray to Akka (apache#2218)

2a9f6b4

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 14, 2017

Port Controller from Spray to Akka (apache#2218)

85d199e

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

houshengbo pushed a commit to houshengbo/openwhisk that referenced this pull request Nov 14, 2017

Port Controller from Spray to Akka (apache#2218)

5dad710

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Port Controller from Spray to Akka (apache#2218)

5972d95

* Port Controller from Spray to Akka * Increase max-connections and Update JSON Unmarshaller

Port Controller from Spray to Akka #2218

Port Controller from Spray to Akka #2218

Conversation

dubee commented May 5, 2017

dubee commented May 5, 2017

dubee commented May 5, 2017 • edited

Choose a reason for hiding this comment

rabbah commented May 5, 2017 • edited

markusthoemmes commented May 8, 2017

markusthoemmes commented May 10, 2017

rabbah commented May 11, 2017

dubee commented May 11, 2017

dubee commented May 11, 2017 • edited

dubee commented May 11, 2017 • edited

dubee commented Jun 7, 2017

dubee commented Jun 19, 2017

Choose a reason for hiding this comment

markusthoemmes Aug 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah commented Aug 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubee commented May 5, 2017 •

edited

rabbah commented May 5, 2017 •

edited

dubee commented May 11, 2017 •

edited

dubee commented May 11, 2017 •

edited

markusthoemmes Aug 9, 2017 •

edited

rabbah Aug 12, 2017 •

edited

rabbah Aug 12, 2017 •

edited