You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "DHKold (via GitHub)" <gi...@apache.org> on 2023/03/20 13:38:53 UTC
[GitHub] [spark] DHKold opened a new pull request, #40491: [SPARK-41006] Generate new ConfigMap names for each run
DHKold opened a new pull request, #40491:
URL: https://github.com/apache/spark/pull/40491
### What changes were proposed in this pull request?
When using the Spark Kubernetes library to launch multiple jobs, the name for the config maps is only generated once, meaning all jobs will try to use the same one. This results in an error.
Relates issue: https://github.com/apache/spark/pull/38574
This PR makes the name of the config-maps dynamic so that each run gets its own. Relate Unit Tests have been updated to take the change into account.
### Why are the changes needed?
The change is needed to allow launching multiple jobs using the Spark Kubernetes library.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
The fixed library has been tested on a private K8S cluster to ensure we can launch multiple jobs with the config maps getting different names.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] DHKold commented on a diff in pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "DHKold (via GitHub)" <gi...@apache.org>.
DHKold commented on code in PR #40491:
URL: https://github.com/apache/spark/pull/40491#discussion_r1156980552
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala:
##########
@@ -43,7 +43,7 @@ private[spark] object KubernetesClientUtils extends Logging {
val configMapNameExecutor: String = configMapName(s"spark-exec-${KubernetesUtils.uniqueID()}")
- val configMapNameDriver: String = configMapName(s"spark-drv-${KubernetesUtils.uniqueID()}")
+ def configMapNameDriver: String = configMapName(s"spark-drv-${KubernetesUtils.uniqueID()}")
Review Comment:
We did not encounter issue with this one. But I can dig in the code to try figuring out if it could also be changed
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1539248438
Thank you for sharing, @ejblanco .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41006][K8S] Generate new ConfigMap names for each run [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
URL: https://github.com/apache/spark/pull/40491
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40491:
URL: https://github.com/apache/spark/pull/40491#discussion_r1150524833
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala:
##########
@@ -43,7 +43,7 @@ private[spark] object KubernetesClientUtils extends Logging {
val configMapNameExecutor: String = configMapName(s"spark-exec-${KubernetesUtils.uniqueID()}")
- val configMapNameDriver: String = configMapName(s"spark-drv-${KubernetesUtils.uniqueID()}")
+ def configMapNameDriver: String = configMapName(s"spark-drv-${KubernetesUtils.uniqueID()}")
Review Comment:
It seems that you have a different usage for this `KubernetesClientUtils` class. Actually, this is not supposed to be used like that initially.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1486827716
Got it. Thank you.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ejblanco commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "ejblanco (via GitHub)" <gi...@apache.org>.
ejblanco commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1653468670
Gentle ping. Any news?
Thx
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40491:
URL: https://github.com/apache/spark/pull/40491#discussion_r1150526906
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala:
##########
@@ -43,7 +43,7 @@ private[spark] object KubernetesClientUtils extends Logging {
val configMapNameExecutor: String = configMapName(s"spark-exec-${KubernetesUtils.uniqueID()}")
- val configMapNameDriver: String = configMapName(s"spark-drv-${KubernetesUtils.uniqueID()}")
+ def configMapNameDriver: String = configMapName(s"spark-drv-${KubernetesUtils.uniqueID()}")
Review Comment:
If we need to change this, I'm wondering `val configMapNameExecutor` is safe or not in the same reason.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] DHKold commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "DHKold (via GitHub)" <gi...@apache.org>.
DHKold commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1486822133
Hi, thanks for looking at it.
Our use case: we have an application which is using the `InProcessLauncher` class several times (to launch multiple spark jobs).
The first one is launched correctly but the next ones fail, trying to use the same configMaps created for the previous job.
I think the call chain is `InProcessLauncher` -> `InProcessSparkSubmit` -> `SparkSubmit` -> `KubernetesClientApplication` -> [Client](https://github.com/apache/spark/blob/ba1badd2adfd175c6680dff90e14b8aaa6cecd78/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala#L105) -> `KubernetesClientUtils`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] DHKold commented on a diff in pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "DHKold (via GitHub)" <gi...@apache.org>.
DHKold commented on code in PR #40491:
URL: https://github.com/apache/spark/pull/40491#discussion_r1150554912
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala:
##########
@@ -214,8 +221,6 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
assert(secrets.nonEmpty)
assert(configMaps.nonEmpty)
val configMap = configMaps.head
- assert(configMap.getMetadata.getName ===
- KubernetesClientUtils.configMapNameDriver)
Review Comment:
I wasn't sure what to test. Should I maybe ensure it is not empty?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ejblanco commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "ejblanco (via GitHub)" <gi...@apache.org>.
ejblanco commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1537865489
> Any updates, @DHKold ?
Hello. We have the same issue (i'm the one who opened the JIRA issue). We currently have a fork with the change from val to def.
If there is no update maybe we can help. Thx
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] DHKold commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "DHKold (via GitHub)" <gi...@apache.org>.
DHKold commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1495644811
Hi, sorry for the delay.
I will add some Unit Testing in order to validate the changes as you mention.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jakubhava commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "jakubhava (via GitHub)" <gi...@apache.org>.
jakubhava commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1542112169
To collect more feedback, we are in the same situation as @ejblanco . Can also offer helping hand in case of need.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41006][K8S] Generate new ConfigMap names for each run [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1847997646
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.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40491:
URL: https://github.com/apache/spark/pull/40491#discussion_r1150528380
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala:
##########
@@ -214,8 +221,6 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
assert(secrets.nonEmpty)
assert(configMaps.nonEmpty)
val configMap = configMaps.head
- assert(configMap.getMetadata.getName ===
- KubernetesClientUtils.configMapNameDriver)
Review Comment:
So, we simply remove all existing test coverages because there is no way to verify that?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jakubhava commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "jakubhava (via GitHub)" <gi...@apache.org>.
jakubhava commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1547714727
On our side we duplicated the code also because of driver config map being hard-coded as well. We are using same launcher to start multiple spark sessions hence it would lead to conflict
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] DHKold commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "DHKold (via GitHub)" <gi...@apache.org>.
DHKold commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1547710656
Hi, sorry, Iw was away for some time. What remains to do:
- add some Unit Tests for the propose changes (I'll try to do that this week)
- check if the configmap for the driver should also be generated dynamically (I was not able to determine it, on our scenario it does not seem to produce any error)
If someone can investigate the second point, that would help.
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40491: [SPARK-41006][K8S] Generate new ConfigMap names for each run
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40491:
URL: https://github.com/apache/spark/pull/40491#issuecomment-1501405550
Any updates, @DHKold ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org