[SPARK-43657][K8S]: reuse config map for executor when running on k8s-cluster mode#41257
[SPARK-43657][K8S]: reuse config map for executor when running on k8s-cluster mode#41257advancedxy wants to merge 3 commits intoapache:masterfrom
Conversation
| @@ -378,7 +388,9 @@ private[spark] object Config extends Logging { | |||
|
|
|||
| val KUBERNETES_EXECUTOR_DISABLE_CONFIGMAP = | |||
| ConfigBuilder("spark.kubernetes.executor.disableConfigMap") | |||
There was a problem hiding this comment.
Hi, @dongjoon-hyun If I understand your original intention correctly, this option is for spark on k8s-cluster mode?
For spark on k8s-client mode, the driver/client would have to create the config map as well.
If that's not your intention, please point it out, I will revert the related changes.
|
Hi @dongjoon-hyun and @ScrapCodes, would you mind to take a look at this pr, and let's discuss and sync on the same pages before I'm going any further. I wrote an email to the dev list but got no replies. Thought I should go ahead and create this PR first. |
|
Gently ping @dongjoon-hyun |
|
Gently ping @dongjoon-hyun and @ScrapCodes . |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for asking review.
We need to keep the existing behavior because this is designed to get a config-free executor pod. The executor pods receive Spark conf from the driver, @advancedxy and @holdenk .
So, could you propose the new feature independently from the existing one. Also, new config should not supersede the existing configuration. In other words, spark.kubernetes.executor.disableConfigMap should have the higher priority if the user gave both of them at the same time mistakenly.
I'd like some clarifications before going further:
|
|
What I meant was
|
Hi, @dongjoon-hyun updated, PTAL. |
|
Thank you for updates, @advancedxy . |
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
Show resolved
Hide resolved
| val additionalSystemProperties = Map(SPARK_CONF_DIR_CONFIG_MAP_NAME.key -> configMapName) | ||
| val confFilesMap = KubernetesClientUtils.buildSparkConfDirFilesMap(configMapName, | ||
| conf.sparkConf, resolvedDriverSpec.systemProperties) | ||
| conf.sparkConf, resolvedDriverSpec.systemProperties ++ additionalSystemProperties) |
There was a problem hiding this comment.
This looks like propagating the existing configMapName variable correctly.
...rc/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
Outdated
Show resolved
Hide resolved
...rc/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| def podHasConfigMapVolume(pod: Pod, volumeName: String, configMapName: String): Boolean = { |
There was a problem hiding this comment.
Yes, I didn't find any similar methods, so this one is created. Do you have any suggestions?
...managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala
Outdated
Show resolved
Hide resolved
| .createWithDefault(1048576) // 1.0 MiB | ||
|
|
||
| val SPARK_CONF_DIR_CONFIG_MAP_NAME = | ||
| ConfigBuilder("spark.kubernetes.sparkConfDir.configMapName") |
There was a problem hiding this comment.
I was confused initially because this configuration requires a user-provided config name.
Can we simply this new feature by simply controlled by a boolean configuration instead of a random string name?
There was a problem hiding this comment.
This configuration is marked as internal() and shall be used internally only to bookkeep the config map name used for driver when the spark-submit clients generates the SPARK_CONF config map for the driver pod.
Can we simply this new feature by simply controlled by a boolean configuration instead of a random string name?
For this case(k8s-cluster mode), I don't think executors should use a different config map for driver and executor, thus I would prefer reusing the config map is mandatory.
The reason why this configuration is defined is that we have to keep/record the config map name, so the driver pod could refer the name. If there's other ways to propagate such thing, I will change it accordingly.
|
Gently ping @dongjoon-hyun , PTAL. |
|
Gently ping @dongjoon-hyun |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
2 participants
What changes were proposed in this pull request?
spark.kubernetes.executor.disableConfigMapas it may not be needed any moreWhy are the changes needed?
Does this PR introduce any user-facing change?
Yes.
spark.kubernetes.executor.disableConfigMapis deprecated and may not be necessary.How was this patch tested?
Added UTs.