Don't create prewarm container when used memory reaches the limit #5048

ningyougang · 2021-01-20T10:47:20Z

Don't create prewarm container when used memory reaches the limit
have no need to judge whether memory when take container from busy/free pool due to don't create new container

codecov-io · 2021-01-20T16:58:38Z

Codecov Report

Merging #5048 (0c91e52) into master (6686820) will decrease coverage by 10.75%.
The diff coverage is 95.65%.

@@             Coverage Diff             @@
##           master    #5048       +/-   ##
===========================================
- Coverage   82.52%   71.76%   -10.76%     
===========================================
  Files         206      206               
  Lines       10006    10007        +1     
  Branches      445      451        +6     
===========================================
- Hits         8257     7182     -1075     
- Misses       1749     2825     +1076

Impacted Files	Coverage Δ
...e/openwhisk/core/containerpool/ContainerPool.scala	`88.44% <95.65%> (-8.76%)`	⬇️
...che/openwhisk/core/invoker/LogStoreCollector.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...core/database/cosmosdb/RxObservableImplicits.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...e/database/cosmosdb/cache/ChangeFeedConsumer.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0.00% <0.00%> (-95.85%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0.00% <0.00%> (-93.90%)`	⬇️
...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala	`0.00% <0.00%> (-92.31%)`	⬇️
...a/org/apache/openwhisk/http/BasicHttpService.scala	`3.33% <0.00%> (-86.67%)`	⬇️
...la/org/apache/openwhisk/http/BasicRasService.scala	`16.66% <0.00%> (-83.34%)`	⬇️
... and 44 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 6686820...0c91e52. Read the comment docs.

ningyougang · 2021-01-21T00:20:13Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

-    val newContainer = childFactory(context)
-    prewarmStartingPool = prewarmStartingPool + (newContainer -> (exec.kind, memoryLimit))
-    newContainer ! Start(exec, memoryLimit, ttl)
+    if (hasPoolSpaceFor(busyPool ++ freePool ++ prewarmedPool, memoryLimit)) {


When create prewarm container, judge whether exist enough memory.

ningyougang · 2021-01-21T03:33:28Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

+  def hasPoolSpaceFor[A](pool: Map[A, ContainerData],
+                         prewarmStartingPool: Map[A, (String, ByteSize)],
+                         memory: ByteSize): Boolean = {
+    memoryConsumptionOf(pool) + +prewarmStartingPool.map(_._2._2.toMB).sum + memory.toMB <= poolConfig.userMemory.toMB


When calculate whether has idle space, it is better to calculate prewarmStartingPool's memory as well.

Is this a typo?
+ +

Ah, yes, you are right.
Already fixed to memoryConsumptionOf(pool) + prewarmStartingPool.map(_._2._2.toMB).sum + memory.toMB <= poolConfig.userMemory.toMB

ningyougang · 2021-01-21T03:34:57Z

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

@@ -829,7 +842,7 @@ class ContainerPoolTests
    stream.reset()
    val prewarmExpirationCheckIntervel = 2.seconds
    val poolConfig =
-      ContainerPoolConfig(MemoryLimit.STD_MEMORY * 8, 0.5, false, prewarmExpirationCheckIntervel, None, 100)
+      ContainerPoolConfig(MemoryLimit.STD_MEMORY * 12, 0.5, false, prewarmExpirationCheckIntervel, None, 100)


Due to add logic of whether exist enough memory, so need to adjust the user memory here.

ningyougang · 2021-01-21T03:36:04Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

@@ -129,44 +129,40 @@ class ContainerPool(childFactory: ActorRefFactory => ActorRef,
        val memory = r.action.limits.memory.megabytes.MB

        val createdContainer =
-          // Is there enough space on the invoker for this action to be executed.
-          if (hasPoolSpaceFor(busyPool ++ prewarmedPool, memory)) {


Due to didn't create any container here, so it is better to remove below logic here.

// Is there enough space on the invoker for this action to be executed. if (hasPoolSpaceFor(busyPool ++ prewarmedPool, memory))

ningyougang · 2021-01-28T04:54:21Z

Have any comment?

style95 · 2021-01-28T07:11:37Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

+  def hasPoolSpaceFor[A](pool: Map[A, ContainerData],
+                         prewarmStartingPool: Map[A, (String, ByteSize)],
+                         memory: ByteSize): Boolean = {
+    memoryConsumptionOf(pool) + +prewarmStartingPool.map(_._2._2.toMB).sum + memory.toMB <= poolConfig.userMemory.toMB


Is this a typo?
+ +

style95 · 2021-01-28T07:16:25Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

+              // There was no warm/warming/warmingCold container. Try to take a prewarm container or a cold container.
+
+              // Is there enough space to create a new container or do other containers have to be removed?
+              if (hasPoolSpaceFor(busyPool ++ freePool ++ prewarmedPool, prewarmStartingPool, memory)) {


If we take a container from the prewarmPool as there is a matched container, do we still need to concern about the total memory?
Since one container is moved from prewarmPool to busyPool and there would be no difference in the sum of memory consumption.

hm..i think we need to concern about the total memory when take a prewarm conainer from prewarmPool,

e.g. let's assume invoker's user memory is 1024 MB, currently, busyPool is 512MB, prewarmPool is 512MB,
if user's activation's corresponding action can match the warmedPool, have no need to add condition: if (hasPoolSpaceFor(busyPool...) due to didn't create new container

but if doesn't match warmedPool and matches a prewarm container(256MB), then, busyPool will be 512MB+256MB, prewarmedPool is 512MB-256MB+256MB, so the total used memory will be 1024MB +256MB, so it is better to add judge condition before take a prewarmed container.

But if we take the current approach, even under the condition that there are 512MB of prewarm containers, they can't be used.

I'd rather let it take prewarm containers but prevent the subsequent creation of a new prewarm container.

Updated accordingly.

ningyougang · 2021-01-29T00:51:17Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

+                .map(container => (container, "prewarmed"))
+                .orElse {
+                  // Is there enough space to create a new container or do other containers have to be removed?
+                  if (hasPoolSpaceFor(busyPool ++ freePool ++ prewarmedPool, prewarmStartingPool, memory)) {


Add memory check condition for coldStart

ningyougang · 2021-01-29T00:53:02Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

-                      val container = Some(createContainer(memory), "cold")
+                    .map(container => (container, "recreatedPrewarm"))
+                    .getOrElse {
+                      val container = (createContainer(memory), "recreated")


For here's coldStart, i think has no need to do memory check. because it will delete a container from freePool in advance.

style95

LGTM

style95 · 2021-01-29T01:55:47Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala

+  def hasPoolSpaceFor[A](pool: Map[A, ContainerData],
+                         prewarmStartingPool: Map[A, (String, ByteSize)],
+                         memory: ByteSize): Boolean = {
+    memoryConsumptionOf(pool) + prewarmStartingPool.map(_._2._2.toMB).sum + memory.toMB <= poolConfig.userMemory.toMB


BTW, when the prewarm pool are backfilled again?

https://github.com/apache/openwhisk/blob/master/core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerPool.scala#L88
every 1 minute by default

ningyougang force-pushed the not-create-prewarm-when-memory-reach-limit branch from f07708f to c27800e Compare January 21, 2021 00:18

ningyougang commented Jan 21, 2021

View reviewed changes

ningyougang force-pushed the not-create-prewarm-when-memory-reach-limit branch from c27800e to c9cba12 Compare January 21, 2021 03:31

ningyougang commented Jan 21, 2021

View reviewed changes

ningyougang closed this Jan 25, 2021

ningyougang reopened this Jan 25, 2021

Don't create prewarm container when used memory reaches the limit

743070e

ningyougang force-pushed the not-create-prewarm-when-memory-reach-limit branch from c9cba12 to 743070e Compare January 28, 2021 04:47

style95 reviewed Jan 28, 2021

View reviewed changes

Fix review points

0c91e52

ningyougang force-pushed the not-create-prewarm-when-memory-reach-limit branch from 1f270a1 to 0c91e52 Compare January 29, 2021 00:49

ningyougang commented Jan 29, 2021

View reviewed changes

style95 approved these changes Jan 29, 2021

View reviewed changes

style95 reviewed Jan 29, 2021

View reviewed changes

ningyougang merged commit c6e32b1 into apache:master Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create prewarm container when used memory reaches the limit #5048

Don't create prewarm container when used memory reaches the limit #5048

ningyougang commented Jan 20, 2021

codecov-io commented Jan 20, 2021 •

edited

Loading

ningyougang Jan 21, 2021

ningyougang Jan 21, 2021 •

edited

Loading

style95 Jan 28, 2021

ningyougang Jan 28, 2021

ningyougang Jan 21, 2021 •

edited

Loading

ningyougang Jan 21, 2021 •

edited

Loading

ningyougang commented Jan 28, 2021

style95 Jan 28, 2021

style95 Jan 28, 2021

ningyougang Jan 28, 2021 •

edited

Loading

style95 Jan 28, 2021

ningyougang Jan 29, 2021

ningyougang Jan 29, 2021

ningyougang Jan 29, 2021 •

edited

Loading

style95 left a comment

style95 Jan 29, 2021

ningyougang Jan 29, 2021

Don't create prewarm container when used memory reaches the limit #5048

Don't create prewarm container when used memory reaches the limit #5048

Conversation

ningyougang commented Jan 20, 2021

codecov-io commented Jan 20, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ningyougang Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningyougang Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

ningyougang Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

ningyougang commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningyougang Jan 28, 2021 • edited Loading

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

ningyougang Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

style95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 20, 2021 •

edited

Loading

ningyougang Jan 21, 2021 •

edited

Loading

ningyougang Jan 21, 2021 •

edited

Loading

ningyougang Jan 21, 2021 •

edited

Loading

ningyougang Jan 28, 2021 •

edited

Loading

ningyougang Jan 29, 2021 •

edited

Loading