You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by "quintenp01 (via GitHub)" <gi...@apache.org> on 2023/09/26 20:55:50 UTC

[GitHub] [openwhisk] quintenp01 opened a new pull request, #5443: Add optional cpu limit to spawned action containers

quintenp01 opened a new pull request, #5443:
URL: https://github.com/apache/openwhisk/pull/5443

   <!--- Provide a concise summary of your changes in the Title -->
   
   ## Description
   <!--- Provide a detailed description of your changes. -->
   <!--- Include details of what problem you are solving and how your changes are tested. -->
   This change provides the option to limit action container cpu usage proportional to the allocated memory. This helps to prevent an invoker from being overloaded by action cpu usage, and also provides more predictable performance for actions.
   
   ## Related issue and scope
   <!--- Please include a link to a related issue if there is one. -->
   
   ## My changes affect the following components
   <!--- Select below all system components are affected by your change. -->
   <!--- Enter an `x` in all applicable boxes. -->
   - [ ] API
   - [ ] Controller
   - [ ] Message Bus (e.g., Kafka)
   - [ ] Loadbalancer
   - [ ] Scheduler
   - [x] Invoker
   - [ ] Intrinsic actions (e.g., sequences, conductors)
   - [ ] Data stores (e.g., CouchDB)
   - [ ] Tests
   - [ ] Deployment
   - [ ] CLI
   - [ ] General tooling
   - [ ] Documentation
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
   - [ ] Bug fix (generally a non-breaking change which closes an issue).
   - [x] Enhancement or new feature (adds new functionality).
   - [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
   
   ## Checklist:
   <!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
   
   - [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
   - [x] I reviewed the [style guides](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md#coding-standards) and followed the recommendations (Travis CI will check :).
   - [x] I added tests to cover my changes.
   - [ ] My changes require further changes to the documentation.
   - [ ] I updated the documentation where necessary.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#issuecomment-1736439964

   It seems the code is wrongly formatted.
   
   ```
   > Task :tests:checkTestScalafmt FAILED
   FAILURE: Build failed with an exception.
   
   * What went wrong:
   
   Execution failed for task ':tests:checkTestScalafmt'.
   Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
   > Files incorrectly formatted: /home/runner/work/openwhisk/openwhisk/tests/src/test/scala/common/LoggedFunction.scala,/home/runner/work/openwhisk/openwhisk/tests/src/test/scala/org/apache/openwhisk/core/containerpool/v2/test/FunctionPullingContainerProxyTests.scala,/home/runner/work/openwhisk/openwhisk/tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerProxyTests.scala
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] quintenp01 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "quintenp01 (via GitHub)" <gi...@apache.org>.
quintenp01 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337871898


##########
core/invoker/src/main/resources/application.conf:
##########
@@ -71,6 +71,7 @@ whisk {
     prewarm-promotion: false # if true, action can take prewarm container which has bigger memory
     memory-sync-interval: 1 second # period to sync memory info to etcd
     batch-deletion-size: 10 # batch size for removing containers when disable invoker, too big value may cause docker/k8s overload
+    # user-cpus: 1 optional setting to limit total allocated cpus for all action containers, each container will get a fraction of this proportional to its allocated memory

Review Comment:
   For most cases yes, but you could set this lower than the actual number of cores the invoker machine has if you wanted to reserve some amount of cpu for the invoker/docker processes.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] quintenp01 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "quintenp01 (via GitHub)" <gi...@apache.org>.
quintenp01 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337870081


##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala:
##########
@@ -73,6 +75,15 @@ case class ContainerPoolConfig(userMemory: ByteSize,
   // Grant more CPU to a container if it allocates more memory.
   def cpuShare(reservedMemory: ByteSize) =
     max((totalShare / (userMemory.toBytes / reservedMemory.toBytes)).toInt, 2) // The minimum allowed cpu-shares is 2
+
+  private val minContainerCpus = 0.01 // The minimum cpus allowed by docker is 0.01
+  def cpuLimit(reservedMemory: ByteSize): Option[Double] = {
+    userCpus.map(c => {
+      val containerCpus = c / (userMemory.toBytes / reservedMemory.toBytes)
+      val roundedContainerCpus = round(containerCpus * 100).toDouble / 100 // Docker only allows decimal precision of 2
+      max(roundedContainerCpus, minContainerCpus)

Review Comment:
   It depends on how many total cores are configured. Looks like the default userMemory is 1024MB, so with 1 total core configured the limit would be 0.25 cores for 256MB. With 2 cores configured, the limit would be 0.5 cores for 256MB. I left the total cores not configured by default though so no limit is applied unless that is set.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] quintenp01 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "quintenp01 (via GitHub)" <gi...@apache.org>.
quintenp01 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337974612


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainer.scala:
##########
@@ -101,6 +102,7 @@ object DockerContainer {
       dnsSearch.flatMap(d => Seq("--dns-search", d)) ++
       dnsOptions.flatMap(d => Seq(dnsOptString, d)) ++
       name.map(n => Seq("--name", n)).getOrElse(Seq.empty) ++
+      cpuLimit.map(c => Seq("--cpus", c.toString)).getOrElse(Seq.empty) ++

Review Comment:
   `--cpu-shares` still provides the weight/priority to cpu cycles for the container, `--cpus` just provides the cap.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337875042


##########
core/invoker/src/main/resources/application.conf:
##########
@@ -71,6 +71,7 @@ whisk {
     prewarm-promotion: false # if true, action can take prewarm container which has bigger memory
     memory-sync-interval: 1 second # period to sync memory info to etcd
     batch-deletion-size: 10 # batch size for removing containers when disable invoker, too big value may cause docker/k8s overload
+    # user-cpus: 1 optional setting to limit total allocated cpus for all action containers, each container will get a fraction of this proportional to its allocated memory

Review Comment:
   ```suggestion
       # optional setting to specify the total allocatable cpus for all action containers, each container will get a fraction of this proportional to its allocated memory to limit the cpu
       # user-cpus: 1 
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337875042


##########
core/invoker/src/main/resources/application.conf:
##########
@@ -71,6 +71,7 @@ whisk {
     prewarm-promotion: false # if true, action can take prewarm container which has bigger memory
     memory-sync-interval: 1 second # period to sync memory info to etcd
     batch-deletion-size: 10 # batch size for removing containers when disable invoker, too big value may cause docker/k8s overload
+    # user-cpus: 1 optional setting to limit total allocated cpus for all action containers, each container will get a fraction of this proportional to its allocated memory

Review Comment:
   ```suggestion
       # user-cpus: 1 # optional setting to specify the total allocatable cpus for all action containers, each container will get a fraction of this proportional to its allocated memory to limit the cpu
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337947388


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainer.scala:
##########
@@ -101,6 +102,7 @@ object DockerContainer {
       dnsSearch.flatMap(d => Seq("--dns-search", d)) ++
       dnsOptions.flatMap(d => Seq(dnsOptString, d)) ++
       name.map(n => Seq("--name", n)).getOrElse(Seq.empty) ++
+      cpuLimit.map(c => Seq("--cpus", c.toString)).getOrElse(Seq.empty) ++

Review Comment:
   i wasn't aware that you could do both `--cpus` and `--cpu-shares` on the docker run. how does this look in practice with how it behaves setting both?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1340751065


##########
ansible/roles/invoker/tasks/deploy.yml:
##########
@@ -258,6 +258,7 @@
       "CONFIG_whisk_containerFactory_containerArgs_network": "{{ invoker_container_network_name | default('bridge') }}"
       "INVOKER_CONTAINER_POLICY": "{{ invoker_container_policy_name | default()}}"
       "CONFIG_whisk_containerPool_userMemory": "{{ hostvars[groups['invokers'][invoker_index | int]].user_memory | default(invoker.userMemory) }}"
+      "CONFIG_whisk_containerPool_userCpus": "{{ invoker.userCpus | default() }}"

Review Comment:
   I could deploy this branch w/ and w/o this configuration using ansible.
   I confirmed that NanoCpus is properly configured in containers according to this config.
   
   ```json
   ...
               "CpuShares": 25,
               "Memory": 268435456,
               "NanoCpus": 0,
   ...
   ```
   
   ```json
   ...
               "CpuShares": 25,
               "Memory": 268435456,
               "NanoCpus": 150000000,
   ...
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on pull request #5443: Add optional cpu limit to spawned action containers

Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#issuecomment-1736562290

   the tradeoff here should be obvious. this will prevent user functions from bursting to take an entire node's cpu if capacity is available, but that is entirely dependent on what else is running on the node resulting in inconsistent performance for the user. this also results in an uncontrollable noisy neighbor issue where a high cpu function could impact the performance of the overall node and other functions executing on it. it's likely preferable to the user to have a reliable and consistent performance to each execution over cpu bursting unless in a highly controlled cluster where you have control over all functions running. I think this should be considered to be defaulted to be on for the 2.0 major version though I'm sure that would be controversial as that will be extremely hard to apply to public clouds where there's no guarantee this change wouldn't break a user's function with lowered cpu. It would require a secondary deployment for 2.0 at that point where a function user
  consciously upgrades their function to the 2.0 cluster. (which may already have to be the upgrade model for public clouds to 2.0 anyways with the new scheduler)
   
   also the way this was implemented is really clean. I've wanted to do this in the past and my thought didn't include having the config be the number of available cpu cores and rationing that in the exact same way the user-memory config is set. makes it very easy for an operator to understand and modify.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] quintenp01 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "quintenp01 (via GitHub)" <gi...@apache.org>.
quintenp01 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1339118485


##########
core/invoker/src/main/resources/application.conf:
##########
@@ -71,6 +71,8 @@ whisk {
     prewarm-promotion: false # if true, action can take prewarm container which has bigger memory
     memory-sync-interval: 1 second # period to sync memory info to etcd
     batch-deletion-size: 10 # batch size for removing containers when disable invoker, too big value may cause docker/k8s overload
+    # optional setting to specify the total allocatable cpus for all action containers, each container will get a fraction of this proportional to its allocated memory to limit the cpu
+    # user-cpus: 1

Review Comment:
   @style95 I just pushed some changes to `ansible/roles/invoker/tasks/deploy.yml` and `ansible/group_vars/all`. I'm not super familiar with ansible and was trying to keep it optional. Can you let me know how that looks?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 merged pull request #5443: Add optional cpu limit to spawned action containers

Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 merged PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337872778


##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala:
##########
@@ -73,6 +75,15 @@ case class ContainerPoolConfig(userMemory: ByteSize,
   // Grant more CPU to a container if it allocates more memory.
   def cpuShare(reservedMemory: ByteSize) =
     max((totalShare / (userMemory.toBytes / reservedMemory.toBytes)).toInt, 2) // The minimum allowed cpu-shares is 2
+
+  private val minContainerCpus = 0.01 // The minimum cpus allowed by docker is 0.01
+  def cpuLimit(reservedMemory: ByteSize): Option[Double] = {
+    userCpus.map(c => {
+      val containerCpus = c / (userMemory.toBytes / reservedMemory.toBytes)
+      val roundedContainerCpus = round(containerCpus * 100).toDouble / 100 // Docker only allows decimal precision of 2
+      max(roundedContainerCpus, minContainerCpus)

Review Comment:
   Got it, the below `user-cpus` is commented.
   I think the default userMemory is 2048MB.
   https://github.com/apache/openwhisk/blob/master/ansible/group_vars/all#L211
   
   So when the default `userMemory` is 2048 and the `user-cpus` is 1, then it would be 0.125 core.
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337888402


##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala:
##########
@@ -73,6 +75,16 @@ case class ContainerPoolConfig(userMemory: ByteSize,
   // Grant more CPU to a container if it allocates more memory.
   def cpuShare(reservedMemory: ByteSize) =
     max((totalShare / (userMemory.toBytes / reservedMemory.toBytes)).toInt, 2) // The minimum allowed cpu-shares is 2
+
+  private val minContainerCpus = 0.01 // The minimum cpus allowed by docker is 0.01
+  private val roundingMultiplier = 100000
+  def cpuLimit(reservedMemory: ByteSize): Option[Double] = {
+    userCpus.map(c => {
+      val containerCpus = c / (userMemory.toBytes / reservedMemory.toBytes)
+      val roundedContainerCpus = round(containerCpus * roundingMultiplier).toDouble / roundingMultiplier // Only use decimal precision of 5

Review Comment:
   👍 



##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala:
##########
@@ -73,6 +75,16 @@ case class ContainerPoolConfig(userMemory: ByteSize,
   // Grant more CPU to a container if it allocates more memory.
   def cpuShare(reservedMemory: ByteSize) =
     max((totalShare / (userMemory.toBytes / reservedMemory.toBytes)).toInt, 2) // The minimum allowed cpu-shares is 2
+
+  private val minContainerCpus = 0.01 // The minimum cpus allowed by docker is 0.01
+  private val roundingMultiplier = 100000
+  def cpuLimit(reservedMemory: ByteSize): Option[Double] = {
+    userCpus.map(c => {
+      val containerCpus = c / (userMemory.toBytes / reservedMemory.toBytes)
+      val roundedContainerCpus = round(containerCpus * roundingMultiplier).toDouble / roundingMultiplier // Only use decimal precision of 5

Review Comment:
   👍 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337852659


##########
tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerPoolConfigTests.scala:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.openwhisk.core.containerpool.test
+
+import org.apache.openwhisk.core.containerpool.ContainerPoolConfig
+import org.apache.openwhisk.core.entity.ByteSize
+import org.apache.openwhisk.core.entity.size.SizeInt
+import org.junit.runner.RunWith
+import org.scalatest.{FlatSpec, Matchers}
+import org.scalatest.junit.JUnitRunner
+
+import scala.concurrent.duration.DurationInt
+
+@RunWith(classOf[JUnitRunner])
+class ContainerPoolConfigTests extends FlatSpec with Matchers {

Review Comment:
   👍 



##########
core/invoker/src/main/resources/application.conf:
##########
@@ -71,6 +71,7 @@ whisk {
     prewarm-promotion: false # if true, action can take prewarm container which has bigger memory
     memory-sync-interval: 1 second # period to sync memory info to etcd
     batch-deletion-size: 10 # batch size for removing containers when disable invoker, too big value may cause docker/k8s overload
+    # user-cpus: 1 optional setting to limit total allocated cpus for all action containers, each container will get a fraction of this proportional to its allocated memory

Review Comment:
   Is this supposed to be the cpu core of an invoker machine?
   



##########
tests/src/test/scala/org/apache/openwhisk/core/containerpool/test/ContainerPoolConfigTests.scala:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.openwhisk.core.containerpool.test
+
+import org.apache.openwhisk.core.containerpool.ContainerPoolConfig
+import org.apache.openwhisk.core.entity.ByteSize
+import org.apache.openwhisk.core.entity.size.SizeInt
+import org.junit.runner.RunWith
+import org.scalatest.{FlatSpec, Matchers}
+import org.scalatest.junit.JUnitRunner
+
+import scala.concurrent.duration.DurationInt
+
+@RunWith(classOf[JUnitRunner])
+class ContainerPoolConfigTests extends FlatSpec with Matchers {

Review Comment:
   👍 



##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala:
##########
@@ -73,6 +75,15 @@ case class ContainerPoolConfig(userMemory: ByteSize,
   // Grant more CPU to a container if it allocates more memory.
   def cpuShare(reservedMemory: ByteSize) =
     max((totalShare / (userMemory.toBytes / reservedMemory.toBytes)).toInt, 2) // The minimum allowed cpu-shares is 2
+
+  private val minContainerCpus = 0.01 // The minimum cpus allowed by docker is 0.01
+  def cpuLimit(reservedMemory: ByteSize): Option[Double] = {
+    userCpus.map(c => {
+      val containerCpus = c / (userMemory.toBytes / reservedMemory.toBytes)
+      val roundedContainerCpus = round(containerCpus * 100).toDouble / 100 // Docker only allows decimal precision of 2
+      max(roundedContainerCpus, minContainerCpus)

Review Comment:
   So the expected cpu limit with the default configuration would be 0.125 core for 256MB?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] quintenp01 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "quintenp01 (via GitHub)" <gi...@apache.org>.
quintenp01 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337882263


##########
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerFactory.scala:
##########
@@ -73,6 +75,15 @@ case class ContainerPoolConfig(userMemory: ByteSize,
   // Grant more CPU to a container if it allocates more memory.
   def cpuShare(reservedMemory: ByteSize) =
     max((totalShare / (userMemory.toBytes / reservedMemory.toBytes)).toInt, 2) // The minimum allowed cpu-shares is 2
+
+  private val minContainerCpus = 0.01 // The minimum cpus allowed by docker is 0.01
+  def cpuLimit(reservedMemory: ByteSize): Option[Double] = {
+    userCpus.map(c => {
+      val containerCpus = c / (userMemory.toBytes / reservedMemory.toBytes)
+      val roundedContainerCpus = round(containerCpus * 100).toDouble / 100 // Docker only allows decimal precision of 2
+      max(roundedContainerCpus, minContainerCpus)

Review Comment:
   Ah, I was looking at the invoker application.conf. So yeah with those settings 0.125 core is correct.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] codecov-commenter commented on pull request #5443: Add optional cpu limit to spawned action containers

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#issuecomment-1736566373

   ## [Codecov](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#5443](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8c8a649) into [master](https://app.codecov.io/gh/apache/openwhisk/commit/6375c96066ca0e24cb81996a877bdd7caecdb72b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6375c96) will **decrease** coverage by `72.27%`.
   > The diff coverage is `21.42%`.
   
   > :exclamation: Current head 8c8a649 differs from pull request most recent head 6a6da58. Consider uploading reports for the commit 6a6da58 to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #5443       +/-   ##
   ==========================================
   - Coverage   76.79%   4.52%   -72.27%     
   ==========================================
     Files         241     241               
     Lines       14634   14646       +12     
     Branches      607     617       +10     
   ==========================================
   - Hits        11238     663    -10575     
   - Misses       3396   13983    +10587     
   ```
   
   
   | [Files](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...che/openwhisk/core/yarn/YARNContainerFactory.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUveWFybi9ZQVJOQ29udGFpbmVyRmFjdG9yeS5zY2FsYQ==) | `0.00% <ø> (-87.37%)` | :arrow_down: |
   | [.../containerpool/docker/DockerContainerFactory.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9kb2NrZXIvRG9ja2VyQ29udGFpbmVyRmFjdG9yeS5zY2FsYQ==) | `0.00% <ø> (-69.70%)` | :arrow_down: |
   | [...erpool/kubernetes/KubernetesContainerFactory.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9rdWJlcm5ldGVzL0t1YmVybmV0ZXNDb250YWluZXJGYWN0b3J5LnNjYWxh) | `0.00% <ø> (ø)` | |
   | [...sk/core/containerpool/docker/DockerContainer.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9kb2NrZXIvRG9ja2VyQ29udGFpbmVyLnNjYWxh) | `0.00% <0.00%> (-98.80%)` | :arrow_down: |
   | [...pool/docker/StandaloneDockerContainerFactory.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9kb2NrZXIvU3RhbmRhbG9uZURvY2tlckNvbnRhaW5lckZhY3Rvcnkuc2NhbGE=) | `0.00% <0.00%> (ø)` | |
   | [.../openwhisk/core/containerpool/ContainerProxy.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9Db250YWluZXJQcm94eS5zY2FsYQ==) | `0.00% <0.00%> (-95.21%)` | :arrow_down: |
   | [...ntainerpool/v2/FunctionPullingContainerProxy.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC92Mi9GdW5jdGlvblB1bGxpbmdDb250YWluZXJQcm94eS5zY2FsYQ==) | `0.00% <0.00%> (-78.66%)` | :arrow_down: |
   | [...penwhisk/core/containerpool/ContainerFactory.scala](https://app.codecov.io/gh/apache/openwhisk/pull/5443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9Db250YWluZXJGYWN0b3J5LnNjYWxh) | `30.76% <37.50%> (-58.71%)` | :arrow_down: |
   
   ... and [200 files with indirect coverage changes](https://app.codecov.io/gh/apache/openwhisk/pull/5443/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on a diff in pull request #5443: Add optional cpu limit to spawned action containers

Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on code in PR #5443:
URL: https://github.com/apache/openwhisk/pull/5443#discussion_r1337997683


##########
core/invoker/src/main/resources/application.conf:
##########
@@ -71,6 +71,8 @@ whisk {
     prewarm-promotion: false # if true, action can take prewarm container which has bigger memory
     memory-sync-interval: 1 second # period to sync memory info to etcd
     batch-deletion-size: 10 # batch size for removing containers when disable invoker, too big value may cause docker/k8s overload
+    # optional setting to specify the total allocatable cpus for all action containers, each container will get a fraction of this proportional to its allocated memory to limit the cpu
+    # user-cpus: 1

Review Comment:
   BTW, could you add a configuration like this?
   https://github.com/apache/openwhisk/blob/master/ansible/roles/invoker/tasks/deploy.yml#L304
   
   It would be easier to configure it with ansible.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org