You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/10/21 18:38:47 UTC
[GitHub] [openwhisk] mcdan opened a new pull request #5008: Add ability to scale Ephemeral storage along with memory, similar to CPU
mcdan opened a new pull request #5008:
URL: https://github.com/apache/openwhisk/pull/5008
Building on setting a simple limit for k8s ephemeral storage this introduces another config setting to also allow scaling the storage based on the memory.
<!--- 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. -->
Along with CPU it seems reasonable action developers may need more disk as their actions use more and more memory. Ideally, we'd start to tease all this apart to allow developers to request a variety of resources but this simply creates a similar setting as CPU to allow the disk capacity to stretch along with memory.
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [ ] I opened an issue to propose and discuss this change (#????)
## 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
- [x] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [ ] CLI
- [ ] General tooling
- [x] 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/wiki/Contributing:-Git-guidelines#code-readiness) and followed the recommendations (Travis CI will check :).
- [x] I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- [x] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] mcdan commented on a change in pull request #5008: Add ability to scale Ephemeral storage along with memory, similar to CPU
Posted by GitBox <gi...@apache.org>.
mcdan commented on a change in pull request #5008:
URL: https://github.com/apache/openwhisk/pull/5008#discussion_r509662488
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/WhiskPodBuilder.scala
##########
@@ -108,7 +108,12 @@ class WhiskPodBuilder(client: NamespacedKubernetesClient, config: KubernetesClie
.getOrElse(Map.empty)
val diskLimit = config.ephemeralStorage
- .map(diskConfig => Map("ephemeral-storage" -> new Quantity(diskConfig.limit.toMB + "Mi")))
+ .map(diskConfig =>
+ if (diskConfig.scaleFactor > 0) {
+ Map("ephemeral-storage" -> new Quantity(diskConfig.scaleFactor * memory.toMB + "Mi"))
Review comment:
That's a fair critique. Will do that and adjust the docs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #5008: Add ability to scale Ephemeral storage along with memory, similar to CPU
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #5008:
URL: https://github.com/apache/openwhisk/pull/5008#discussion_r509633515
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/WhiskPodBuilder.scala
##########
@@ -108,7 +108,12 @@ class WhiskPodBuilder(client: NamespacedKubernetesClient, config: KubernetesClie
.getOrElse(Map.empty)
val diskLimit = config.ephemeralStorage
- .map(diskConfig => Map("ephemeral-storage" -> new Quantity(diskConfig.limit.toMB + "Mi")))
+ .map(diskConfig =>
+ if (diskConfig.scaleFactor > 0) {
Review comment:
based on `KubernetesEphemeralStorageConfig`sig it should not be null, but technically it might be possible; If null was allowed, it should be `Option[Doube]`; I would leave it as is
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] mcdan commented on a change in pull request #5008: Add ability to scale Ephemeral storage along with memory, similar to CPU
Posted by GitBox <gi...@apache.org>.
mcdan commented on a change in pull request #5008:
URL: https://github.com/apache/openwhisk/pull/5008#discussion_r509615332
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/WhiskPodBuilder.scala
##########
@@ -108,7 +108,12 @@ class WhiskPodBuilder(client: NamespacedKubernetesClient, config: KubernetesClie
.getOrElse(Map.empty)
val diskLimit = config.ephemeralStorage
- .map(diskConfig => Map("ephemeral-storage" -> new Quantity(diskConfig.limit.toMB + "Mi")))
+ .map(diskConfig =>
+ if (diskConfig.scaleFactor > 0) {
Review comment:
Didn't think null was a valid value for a Double but, maybe I'm mixing up languages. Will test thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] selfxp commented on a change in pull request #5008: Add ability to scale Ephemeral storage along with memory, similar to CPU
Posted by GitBox <gi...@apache.org>.
selfxp commented on a change in pull request #5008:
URL: https://github.com/apache/openwhisk/pull/5008#discussion_r509592629
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/WhiskPodBuilder.scala
##########
@@ -108,7 +108,12 @@ class WhiskPodBuilder(client: NamespacedKubernetesClient, config: KubernetesClie
.getOrElse(Map.empty)
val diskLimit = config.ephemeralStorage
- .map(diskConfig => Map("ephemeral-storage" -> new Quantity(diskConfig.limit.toMB + "Mi")))
+ .map(diskConfig =>
+ if (diskConfig.scaleFactor > 0) {
Review comment:
Can `diskConfig.scaleFactor` be `null`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #5008: Add ability to scale Ephemeral storage along with memory, similar to CPU
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #5008:
URL: https://github.com/apache/openwhisk/pull/5008#discussion_r509634615
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/WhiskPodBuilder.scala
##########
@@ -108,7 +108,12 @@ class WhiskPodBuilder(client: NamespacedKubernetesClient, config: KubernetesClie
.getOrElse(Map.empty)
val diskLimit = config.ephemeralStorage
- .map(diskConfig => Map("ephemeral-storage" -> new Quantity(diskConfig.limit.toMB + "Mi")))
+ .map(diskConfig =>
+ if (diskConfig.scaleFactor > 0) {
+ Map("ephemeral-storage" -> new Quantity(diskConfig.scaleFactor * memory.toMB + "Mi"))
Review comment:
I think there should still be a limit - i.e. use the scale factor unless the result is > some limit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org