Config for hard cpu limit on pods; default unlimited #356
Conversation
| @@ -438,7 +443,7 @@ private[spark] class KubernetesClusterSchedulerBackend( | |||
| .addToRequests("memory", executorMemoryQuantity) | |||
| .addToLimits("memory", executorMemoryLimitQuantity) | |||
| .addToRequests("cpu", executorCpuQuantity) | |||
| .addToLimits("cpu", executorCpuQuantity) | |||
| .addToLimits("cpu", executorCpuLimitQuantity) | |||
There was a problem hiding this comment.
@sandflee can you please change this PR so that if spark.kubernetes.executor.limit.cores is not set then there is no .addToLimits("cpu", executorCpuLimitQuantity) call here?
From discussion on the linked issue I think we want the default to be no cpu limit, and only add a limit if the user specifies one
|
Linking to #38 which is the umbrella issue for fully-generic customization of things like this via user-provided yaml files instead of incrementally adding additional options for every k8s feature. |
|
patch updated, add spark.kubernetes.driver.limit.cores besides spark.kubernetes.executor.limit.cores |
|
|
||
| private[spark] val KUBERNETES_EXECUTOR_LIMIT_CORES = | ||
| ConfigBuilder("spark.kubernetes.executor.limit.cores") | ||
| .doc("cpu limit for executor") |
There was a problem hiding this comment.
maybe should emphasize that this is a single executor, like hard cpu limit for a single executor
| @@ -64,6 +64,8 @@ private[spark] class Client( | |||
|
|
|||
| // CPU settings | |||
| private val driverCpuCores = sparkConf.getOption("spark.driver.cores").getOrElse("1") | |||
| private val driverLimitCores = sparkConf. | |||
| getOption(KUBERNETES_DRIVER_LIMIT_CORES.key).getOrElse("") | |||
There was a problem hiding this comment.
drop the .getOrElse("") and leave this value as Option[String]
| @@ -108,6 +108,8 @@ private[spark] class KubernetesClusterSchedulerBackend( | |||
| private val executorMemoryWithOverhead = executorMemoryMb + memoryOverheadMb | |||
|
|
|||
| private val executorCores = conf.getOption("spark.executor.cores").getOrElse("1") | |||
| private val executorLimitCores = conf. | |||
| getOption(KUBERNETES_EXECUTOR_LIMIT_CORES.key).getOrElse("") | |||
| .editResources | ||
| .addToLimits("cpu", executorCpuLimitQuantity) | ||
| .endResources() | ||
| .endContainer() |
ash211
changed the title
docs/running-on-kubernetes.md
Outdated
| <td><code>spark.kubernetes.driver.limit.cores</code></td> | ||
| <td>(none)</td> | ||
| <td> | ||
| specify the cpu limit for the driver pod |
There was a problem hiding this comment.
Looks rest of the docs for config keys all use capitalized full sentences. Can you do the same to be consistent?
docs/running-on-kubernetes.md
Outdated
| <td><code>spark.kubernetes.executor.limit.cores</code></td> | ||
| <td>(none)</td> | ||
| <td> | ||
| specify the cpu limit for the executor pod |
| @@ -485,6 +485,18 @@ package object config extends Logging { | |||
| .stringConf | |||
| .createOptional | |||
|
|
|||
| private[spark] val KUBERNETES_DRIVER_LIMIT_CORES = | |||
| ConfigBuilder("spark.kubernetes.driver.limit.cores") | |||
| .doc("cpu limit for driver") | |||
|
|
||
| private[spark] val KUBERNETES_EXECUTOR_LIMIT_CORES = | ||
| ConfigBuilder("spark.kubernetes.executor.limit.cores") | ||
| .doc("cpu limit for executor") |
|
rerun integration test please |
|
Hi @sandflee it looks like a recent commit caused a merge conflict in this PR. Can you please fix that and address the outstanding comments? With those minor changes I think this PR is ready to merge |
|
Thank a bunch @sandflee for the contribution! Looking forward to jobs feeling faster for everyone |
#352