You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/09/05 10:59:30 UTC
[GitHub] [spark] fanyilun opened a new pull request, #37803: [SPARK-39546][Kubernetes] Executor pod template should support port definitions
fanyilun opened a new pull request, #37803:
URL: https://github.com/apache/spark/pull/37803
### What changes were proposed in this pull request?
This pr changes the fabric8 k8s method used in BasicExecutorFeatureStep about setting blockmanager ports. Because It will override port definitions in executor pod template file.
### Why are the changes needed?
We use spark.kubernetes.executor.podTemplateFile to specify executor pod definition. However, we cannot set port definition to pod. For example, this is my executorPodTemplate.yaml
`
apiversion: v1
kind: Pod
spec:
containers:
- name: spark-kubernetes-executor
ports:
- containerPort: 8090
name: jmx-exporter
protocol: TCP
`
When executors start, the pod definitions only show default blockmanager port.
`
ports:
- containerPort: 7079
name: blockmanager
protocol: TCP
`
It looks like a bug, because the BasicExecutorFeatureStep uses withPorts to override all port definitions.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Add unit test
--
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 #37803: [SPARK-39546][Kubernetes] Executor pod template should support port definitions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #37803:
URL: https://github.com/apache/spark/pull/37803#discussion_r965609555
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala:
##########
@@ -510,6 +511,20 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter {
assert(mem === s"${expected}Mi")
}
+ test("SPARK-39546: executor pod template should support port definitions") {
+ val baseDriverPod = SparkPod.initialPod()
+ val ports = new ContainerPortBuilder()
+ .withName("port-from-template")
+ .withContainerPort(1000)
+ .build()
+ baseDriverPod.container.setPorts(JavaConverters.seqAsJavaListConverter(Seq(ports)).asJava)
+ val step1 = new BasicExecutorFeatureStep(newExecutorConf(), new SecurityManager(baseConf),
+ defaultProfile)
+ val podConfigured1 = step1.configurePod(baseDriverPod)
+ // port-from-template should exist after step1
+ assert(podConfigured1.container.getPorts.contains(ports))
+
Review Comment:
Shalll we remove this redundant empty line?
--
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 #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37803:
URL: https://github.com/apache/spark/pull/37803#issuecomment-1241047879
Welcome to the Apache Spark community, @fanyilun . I added you to the Apache Spark contributor group and assigned SPARK-39546 to you. Do we need this patch for driver pod template? If then, please feel free to make another JIRA and continue.
--
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 #37803: [SPARK-39546][Kubernetes] Executor pod template should support port definitions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37803:
URL: https://github.com/apache/spark/pull/37803#issuecomment-1240341865
Thank you for pinging me, @fanyilun .
--
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] AmplabJenkins commented on pull request #37803: [SPARK-39546][Kubernetes] Executor pod template should support port definitions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37803:
URL: https://github.com/apache/spark/pull/37803#issuecomment-1237518955
Can one of the admins verify this patch?
--
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] fanyilun commented on pull request #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
Posted by GitBox <gi...@apache.org>.
fanyilun commented on PR #37803:
URL: https://github.com/apache/spark/pull/37803#issuecomment-1240569383
Hi @dongjoon-hyun , please take another look
--
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 closed pull request #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
URL: https://github.com/apache/spark/pull/37803
--
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 #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #37803:
URL: https://github.com/apache/spark/pull/37803#discussion_r965636530
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala:
##########
@@ -510,6 +511,20 @@ class BasicExecutorFeatureStepSuite extends SparkFunSuite with BeforeAndAfter {
assert(mem === s"${expected}Mi")
}
+ test("SPARK-39546: executor pod template should support port definitions") {
+ val baseDriverPod = SparkPod.initialPod()
+ val ports = new ContainerPortBuilder()
+ .withName("port-from-template")
+ .withContainerPort(1000)
+ .build()
+ baseDriverPod.container.setPorts(JavaConverters.seqAsJavaListConverter(Seq(ports)).asJava)
Review Comment:
```scala
- baseDriverPod.container.setPorts(JavaConverters.seqAsJavaListConverter(Seq(ports)).asJava)
+ baseDriverPod.container.setPorts(Seq(ports).asJava)
```
--
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] fanyilun commented on pull request #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
Posted by GitBox <gi...@apache.org>.
fanyilun commented on PR #37803:
URL: https://github.com/apache/spark/pull/37803#issuecomment-1242604703
Thanks, driver pod template already supports ports definition. It works for me.
--
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 #37803: [SPARK-39546][K8S] Support `ports` definition in executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #37803:
URL: https://github.com/apache/spark/pull/37803#discussion_r965636918
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala:
##########
@@ -16,6 +16,7 @@
*/
package org.apache.spark.deploy.k8s.features
+import scala.collection.JavaConverters
Review Comment:
Let's remove this after applying https://github.com/apache/spark/pull/37803/files#r965636530.
--
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] fanyilun commented on pull request #37803: [SPARK-39546][Kubernetes] Executor pod template should support port definitions
Posted by GitBox <gi...@apache.org>.
fanyilun commented on PR #37803:
URL: https://github.com/apache/spark/pull/37803#issuecomment-1237818158
cc @dongjoon-hyun could u take a look? 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