recreate http client on resume() #4185

tysonnorris · 2018-12-17T17:28:18Z

In testing concurrent actions (multiple activations in same container) we noticed that when a container is used after Container.suspend(), the http connection may be re-created multiple times (and may have side affects of losing connections each time a new one is created.

Description

This PR addresses this by:

using a separate function for creating the connection - this will be invoked first during /init
same function will be invoked again during Container.resume() (which means that subclasses must invoke super.resume())

This way the subsequent + multiple calls to /run will arrive after connection has already been recreated.
I extended the ContainerProxyTests to assert connection state (exists or not) after suspend() and after resume().

Related issue and scope

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

My changes affect the following components

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-io · 2018-12-18T02:05:48Z

Codecov Report

Merging #4185 into master will decrease coverage by 4.85%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #4185      +/-   ##
==========================================
- Coverage   85.93%   81.08%   -4.86%     
==========================================
  Files         152      152              
  Lines        7304     7312       +8     
  Branches      484      480       -4     
==========================================
- Hits         6277     5929     -348     
- Misses       1027     1383     +356

Impacted Files	Coverage Δ
...la/org/apache/openwhisk/core/mesos/MesosTask.scala	`73.8% <0%> (ø)`	⬆️
...sk/core/containerpool/docker/DockerContainer.scala	`96.2% <100%> (ø)`	⬆️
...containerpool/kubernetes/KubernetesContainer.scala	`94.73% <100%> (ø)`	⬆️
...pache/openwhisk/core/containerpool/Container.scala	`88.46% <91.66%> (+6.77%)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.54%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-83.34%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`0% <0%> (-62.5%)`	⬇️
...in/scala/org/apache/openwhisk/common/Counter.scala	`40% <0%> (-20%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb96c21...80c3519. Read the comment docs.

markusthoemmes

LGTM in general

Left 2 nits. We should explicitly document that this trait is not thread-safe.

markusthoemmes · 2018-12-18T07:11:25Z

tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerProxyTests.scala

@@ -1078,12 +1089,20 @@ class ContainerProxyTests

    def runCount = atomicRunCount.get()
    override def suspend()(implicit transid: TransactionId): Future[Unit] = {
+      println("suspending!!!")


Please remove that extranous logline.

markusthoemmes · 2018-12-18T07:11:30Z

tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerProxyTests.scala

    }
-    def resume()(implicit transid: TransactionId): Future[Unit] = {
+    override def resume()(implicit transid: TransactionId): Future[Unit] = {
+      println("resuming!!!")


markusthoemmes · 2018-12-18T07:14:05Z

common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/Container.scala

@@ -73,6 +73,10 @@ trait Container {
  /** HTTP connection to the container, will be lazily established by callContainer */
  protected var httpConnection: Option[ContainerClient] = None

+  /** maxConcurrent+timeout are cached during first init, so that resuming connections can reference */
+  protected var containerHttpMaxConcurrent: Int = 1
+  protected var containerHttpTimeout: FiniteDuration = 60.seconds


I know this was the case before, but we should add a comment to the trait stating that it's not thread-safe and the caller MUST ensure synchronization (which we do by using an actor)

@markusthoemmes Hello. I'm a newbie at scala and having a hard time reading the source code. Can you please explain what thread-safe means above, and why resume() (which I'm guessing that it somehow calls "docker container resume") is not thread safe??

tysonnorris · 2019-01-07T22:42:18Z

@markusthoemmes any other comments?

markusthoemmes

LGTM modulo some nits.

markusthoemmes · 2019-01-08T07:56:36Z

common/scala/src/main/scala/org/apache/openwhisk/core/mesos/MesosTask.scala

@@ -207,6 +207,7 @@ class MesosTask(override protected val id: ContainerId,
  override def resume()(implicit transid: TransactionId): Future[Unit] = {
    // resume not supported
    Future.successful(Unit)


This is dead code, you can just omit the Future.successful call.

markusthoemmes · 2019-01-08T07:58:38Z

tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerProxyTests.scala

@@ -170,6 +173,7 @@ class ContainerProxyTests
      activation.annotations.get("limits") shouldBe Some(a.limits.toJson)
      activation.annotations.get("path") shouldBe Some(a.fullyQualifiedName(false).toString.toJson)
      activation.annotations.get("kind") shouldBe Some(a.exec.kind.toJson)
+      system.log.info(s"acking ${activation.activationId}")


Are these loglines needed? (Same above).

tysonnorris · 2019-01-14T22:51:23Z

@markusthoemmes I think this is ready now

markusthoemmes

One last comment on the test, then it's LGTM.

markusthoemmes · 2019-01-15T07:03:36Z

tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerProxyTests.scala

-      Future.successful(())
+      val r = super.resume()
+      //verify that httpconn is recreated
+      httpConnection should be('defined)


Please await the futures of either resume and suspend before checking that the client is there. Right now this is not an issue but once the implementation of the trait changes we're prone to heisenbugs.

markusthoemmes

I'm sorry I only caught this now... If I get time I'll throw a commit on there myself changing these bits to flatMaps.

markusthoemmes · 2019-01-16T06:36:34Z

.../src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesContainer.scala

@@ -104,7 +104,8 @@ class KubernetesContainer(protected[core] val id: ContainerId,
    super.suspend().flatMap(_ => kubernetes.suspend(this))
  }

-  def resume()(implicit transid: TransactionId): Future[Unit] = kubernetes.resume(this)
+  override def resume()(implicit transid: TransactionId): Future[Unit] =
+    kubernetes.resume(this).map(_ => super.resume())


This should be a flatMap to also "wait" for the future returned by super.resume.

markusthoemmes · 2019-01-16T06:36:41Z

.../invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainer.scala

-  def resume()(implicit transid: TransactionId): Future[Unit] =
-    if (useRunc) { runc.resume(id) } else { docker.unpause(id) }
+  override def resume()(implicit transid: TransactionId): Future[Unit] = {
+    if (useRunc) { runc.resume(id) } else { docker.unpause(id) }.map(_ => super.resume())


Does the map() really run on the full if expression or just the else block?

when in doubt, refactor...

good question @sven-lange-last. Better safe than sorry and make an interim value or put parentheses around the if expression.

I don't think it's needed, but agree it will at least be more clear. Also changed to flatMap

tysonnorris · 2019-01-16T14:49:55Z

Thanks for the feedback, it is always welcome.

ddragosd · 2019-01-18T19:39:24Z

common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/Container.scala

@@ -204,6 +209,17 @@ trait Container {
        RunResult(Interval(started, finished), response)
      }
  }
+  private def openConnections(timeout: FiniteDuration, maxConcurrent: Int) = {
+    if (Container.config.akkaClient) {


I forgot why are we still maintaining the Akka Client and the Apache Client, since the latter was found in #3812 to perform poorly ?

are we ready to move to the akka client exclusively? there were some reservations about making sure we didn't get bit by some akka issues as in the past.

I'm ready 😄
FWIW, we have been using akka client exclusively in prod, with concurrency enabled, since December.
Tests are already using it as well.
I'm not sure if there are others that are interested in testing it more, but it would be great to retire the apache client IMHO.

Let's confirm that via dev-list.

ddragosd

LGTM. I'm approving based on previous reviews and successful tests.

reopen connections only once, during Container.resume()

markusthoemmes reviewed Dec 18, 2018

View reviewed changes

rabbah added invoker review Review for this PR has been requested and yet needs to be done. and removed invoker labels Jan 5, 2019

markusthoemmes reviewed Jan 8, 2019

View reviewed changes

markusthoemmes added the reviewed Review for this PR is finished. It is mergeable from a review's perspective. label Jan 8, 2019

reopen connections only once, during Container.resume()

1552099

tysonnorris force-pushed the resume-http-client branch from f9521ba to 1552099 Compare January 14, 2019 21:55

markusthoemmes reviewed Jan 15, 2019

View reviewed changes

review feedback - add Await before suspend/resume assertions

b311911

markusthoemmes reviewed Jan 16, 2019

View reviewed changes

review feedback - flatMaps!

80c3519

tysonnorris requested a review from chetanmeh January 18, 2019 16:49

ddragosd reviewed Jan 18, 2019

View reviewed changes

ddragosd approved these changes Jan 18, 2019

View reviewed changes

markusthoemmes merged commit 2312b7d into apache:master Jan 21, 2019

ddragosd deleted the resume-http-client branch January 22, 2019 07:04

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

Recreate HTTP client on Container.resume(). (apache#4185)

5ba6e19

reopen connections only once, during Container.resume()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

recreate http client on resume() #4185

recreate http client on resume() #4185

tysonnorris commented Dec 17, 2018

codecov-io commented Dec 18, 2018 •

edited

markusthoemmes left a comment

markusthoemmes Dec 18, 2018

markusthoemmes Dec 18, 2018

markusthoemmes Dec 18, 2018 •

edited by rabbah

joonho3020 Jan 27, 2021

tysonnorris commented Jan 7, 2019

markusthoemmes left a comment

markusthoemmes Jan 8, 2019

markusthoemmes Jan 8, 2019

tysonnorris commented Jan 14, 2019

markusthoemmes left a comment

markusthoemmes Jan 15, 2019

markusthoemmes left a comment

markusthoemmes Jan 16, 2019

markusthoemmes Jan 16, 2019

sven-lange-last Jan 16, 2019

rabbah Jan 16, 2019 •

edited

markusthoemmes Jan 16, 2019

tysonnorris Jan 16, 2019

tysonnorris commented Jan 16, 2019

ddragosd Jan 18, 2019 •

edited

rabbah Jan 18, 2019

tysonnorris Jan 18, 2019

markusthoemmes Jan 21, 2019

ddragosd left a comment

recreate http client on resume() #4185

recreate http client on resume() #4185

Conversation

tysonnorris commented Dec 17, 2018

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

codecov-io commented Dec 18, 2018 • edited

Codecov Report

markusthoemmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusthoemmes Dec 18, 2018 • edited by rabbah

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tysonnorris commented Jan 7, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tysonnorris commented Jan 14, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusthoemmes left a 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 Jan 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tysonnorris commented Jan 16, 2019

ddragosd Jan 18, 2019 • 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

ddragosd left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 18, 2018 •

edited

markusthoemmes Dec 18, 2018 •

edited by rabbah

rabbah Jan 16, 2019 •

edited

ddragosd Jan 18, 2019 •

edited