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 2020/10/02 14:40:57 UTC
[GitHub] [spark] stijndehaes opened a new pull request #29934: Make sure the pod template configmap has a unique name
stijndehaes opened a new pull request #29934:
URL: https://github.com/apache/spark/pull/29934
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
The pod template configmap always had the same name. This PR makes it unique.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
If you scheduled 2 spark jobs they will both use the same configmap name this will result in conflicts. This PR fixes that
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Unit tests
fixes: #SPARK-32067
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703255173
**[Test build #129389 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129389/testReport)** for PR 29934 at commit [`66609a2`](https://github.com/apache/spark/commit/66609a20439e3825ef17a4501bdebbbf7247a91f).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703182780
**[Test build #129385 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129385/testReport)** for PR 29934 at commit [`9993d13`](https://github.com/apache/spark/commit/9993d13a234d066d3caa11bb8a276a2bc54f23bc).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499845657
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
This makes the following status.
```
tpcds-py-eebc4474fa60b6f7-driver-conf-map 1 5s
tpcds-py-eebc4474fa60b6f7-podspec-configmap 1 5s
```
This is inconsistent because Driver is using `driver-conf-map` postfix and executor is using `podspec-configmap`.
```
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala: val configMapName = s"${conf.resourceNamePrefix}-driver-conf-map"
```
Let's use `-exec-conf-map` as a postfix.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704040821
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r500002157
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
Wel actually this configmap is not used by the executors but also by the driver. The driver needs this configmap to spawn executors with the right template. So I'll change the name to:`driver-podspec-configmap`. Let me know if that is fine.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703841103
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704025741
**[Test build #129433 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129433/testReport)** for PR 29934 at commit [`948e15a`](https://github.com/apache/spark/commit/948e15a48e68022ee39f715a883e62fbe2f15397).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499724333
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
By the way @stijndehaes I haven't been able to find the document that says the max name length of any configmap is 63 char long. From my quick command line test, I found it can be well over beyond that number. Could you confirm this limit is needed. Well, I do agree it is always good to be defensive but I found there is no such shortening logic for driver configmap either. So, could you confirm? Thanks.
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
Review comment:
By the way @stijndehaes I haven't been able to find the document that says the max name length of any configmap is 63 char long. From my quick command line test, I found it can be well over beyond that number. Could you confirm this limit is needed. Well, I do agree it is always good to be defensive but I found there is no such shortening logic for driver configmap either. So, could you confirm? 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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r500556445
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
Also, please update your PR description. I added **AFTER** paragraph at this PR description. You can fill in the final output there.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r500002157
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
Wel actually this configmap is not used by the executors but also by the driver. The driver needs this configmap to spawn executors with the right template. So I'll change the name to:`driver-podspec-conf-map`. Let me know if that is fine.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703256236
**[Test build #129390 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129390/testReport)** for PR 29934 at commit [`a4e423c`](https://github.com/apache/spark/commit/a4e423c78efdc78c95d80653e17934c343d83d17).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499791671
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
@yuj for me it is fine using the constant. Let's not change more than needed :)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703176535
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-705076235
Awesome. Thanks guys @stijndehaes @dongjoon-hyun
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499828167
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
@stijndehaes right.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703415211
**[Test build #129399 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129399/testReport)** for PR 29934 at commit [`09689f2`](https://github.com/apache/spark/commit/09689f2793828278428f97f7aad131b9b5fe02ec).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r500921364
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
During running the integration tests when watching the configmap it looks something like this:
```
aaece65ef82e4a30b7b7800aad600d4f spark-test-app-aac9f37502b2ca55-driver-conf-map 1 0s
aaece65ef82e4a30b7b7800aad600d4f spark-test-app-aac9f37502b2ca55-driver-podspec-conf-map 1 0
```
For me this looks nice because it's clear that the confimap is used by the driver. And when sorted alphabetically they are neatly together.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703410924
**[Test build #129399 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129399/testReport)** for PR 29934 at commit [`09689f2`](https://github.com/apache/spark/commit/09689f2793828278428f97f7aad131b9b5fe02ec).
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-705066894
Thank you again, @stijndehaes . I added you to the Apache Spark contributor group and assigned SPARK-32067 to you.
Welcome to the Apache Spark community again!
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499787259
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
Review comment:
@yuj you are right the name is max 253 long according to this document: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
I shall remove the logic, I don't think we need to be defensive when the max is this long.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703265007
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704040821
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703433464
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703182911
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499272899
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -85,3 +103,7 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
}
}
}
+
+private[spark] object PodTemplateConfigMapStep {
+ val PODSPEC_CONFIGMAP_POSTFIX = "-podspec-configmap"
+}
Review comment:
@stijndehaes Thanks for addressing the feedback and consolidating the max name length constant.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703415372
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703262467
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33997/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-705343221
@dongjoon-hyun @yuj Thanks for all the help guys :)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704029221
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: Make sure the pod template configmap has a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-702774206
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.
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 removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703257562
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703262865
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33996/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on pull request #29934: [SPARK-32067][K8S]Make sure the pod template configmap has a unique name
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703064501
@onursatici @vanzin I see you guys have changed parts of this code would you kindly review?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703410924
**[Test build #129399 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129399/testReport)** for PR 29934 at commit [`09689f2`](https://github.com/apache/spark/commit/09689f2793828278428f97f7aad131b9b5fe02ec).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499163210
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -85,3 +103,7 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
}
}
}
+
+private[spark] object PodTemplateConfigMapStep {
+ val PODSPEC_CONFIGMAP_POSTFIX = "-podspec-configmap"
+}
Review comment:
Please use the constant already defined in Constants.scala
`val POD_TEMPLATE_CONFIGMAP = "podspec-configmap"`
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStepSuite.scala
##########
@@ -56,8 +60,10 @@ class PodTemplateConfigMapStepSuite extends SparkFunSuite {
assert(configuredPod.pod.getSpec.getVolumes.size() === 1)
val volume = configuredPod.pod.getSpec.getVolumes.get(0)
+ val generatedResourceName = kubernetesConf.resourceNamePrefix +
+ PodTemplateConfigMapStep.PODSPEC_CONFIGMAP_POSTFIX
Review comment:
Same as the suggestion above, should use the constant already defined in Constants.scala.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on pull request #29934: [SPARK-32067][K8S]Make sure the pod template configmap has a unique name
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703064561
@HyukjinKwon Sorry forgot to add the existing JIRA ticket in the title. I also added the K8S component in the title now
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun edited a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun edited a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703850118
Of course, @yuj . I'm considering this for Apache Spark 3.1/3.0.2/2.4.8. I've been reviewing this PR since two days ago.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r501163662
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
Got it. Thank you, @stijndehaes .
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499731594
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
I'd vote for using the Constant.scala for store the value.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499245849
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -85,3 +103,7 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
}
}
}
+
+private[spark] object PodTemplateConfigMapStep {
+ val PODSPEC_CONFIGMAP_POSTFIX = "-podspec-configmap"
+}
Review comment:
@yuj Changed it to your suggestion. Also moved the constant max k8s name in service to constant since I reused it in this class.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499270226
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -85,3 +101,4 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
}
}
}
+
Review comment:
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.
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] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499356507
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
Review comment:
I think it is better now?
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: Make sure the pod template configmap has a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-702775614
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.
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 removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703841103
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703817742
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703817742
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499791144
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
Review comment:
Well this makes the PR way shorter :)
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499302260
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
Also, could you remove the following at line 80?
```scala
val POD_TEMPLATE_CONFIGMAP = "podspec-configmap"
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703257562
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703264999
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33997/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499724333
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
By the way @stijndehaes I haven't been able to find the document that says the max name length of any configmap is 63 char long. From my quick command line test, I found it can be well over beyond that number. Could you confirm this limit is needed. Well, I do agree it is always good to be defensive but I found there is no such shortening logic for driver configmap either. So, could you confirm? 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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704040803
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34040/
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703265007
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703817529
**[Test build #129419 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129419/testReport)** for PR 29934 at commit [`d48c8e2`](https://github.com/apache/spark/commit/d48c8e21147b078518d3115a8c7c891b0b644ee7).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703171143
**[Test build #129385 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129385/testReport)** for PR 29934 at commit [`9993d13`](https://github.com/apache/spark/commit/9993d13a234d066d3caa11bb8a276a2bc54f23bc).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703255365
Merged build finished. Test FAILed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703415372
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r501166683
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
Nice!
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703255357
**[Test build #129389 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129389/testReport)** for PR 29934 at commit [`66609a2`](https://github.com/apache/spark/commit/66609a20439e3825ef17a4501bdebbbf7247a91f).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703810863
**[Test build #129419 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129419/testReport)** for PR 29934 at commit [`d48c8e2`](https://github.com/apache/spark/commit/d48c8e21147b078518d3115a8c7c891b0b644ee7).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703255366
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129389/
Test FAILed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703256236
**[Test build #129390 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129390/testReport)** for PR 29934 at commit [`a4e423c`](https://github.com/apache/spark/commit/a4e423c78efdc78c95d80653e17934c343d83d17).
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499845657
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
This makes the following status.
```
tpcds-py-eebc4474fa60b6f7-driver-conf-map 1 5s
tpcds-py-eebc4474fa60b6f7-podspec-configmap 1 5s
```
This is inconsistent because Driver is using `driver-conf-map` postfix and executor is using `podspec-configmap`.
```
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala: val configMapName = s"${conf.resourceNamePrefix}-driver-conf-map"
```
Let's use `exec-conf-map` post fix.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #29934: Make sure the pod template configmap has a unique name
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703042759
Can you file a jira and link it to the PR title? See also https://spark.apache.org/contributing.html
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703255173
**[Test build #129389 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129389/testReport)** for PR 29934 at commit [`66609a2`](https://github.com/apache/spark/commit/66609a20439e3825ef17a4501bdebbbf7247a91f).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703257522
**[Test build #129390 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129390/testReport)** for PR 29934 at commit [`a4e423c`](https://github.com/apache/spark/commit/a4e423c78efdc78c95d80653e17934c343d83d17).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704029221
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703846263
Wow, it's much simpler, @stijndehaes . :)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499828738
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
Review comment:
It's way better, isn't!
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703841084
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34026/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703810863
**[Test build #129419 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129419/testReport)** for PR 29934 at commit [`d48c8e2`](https://github.com/apache/spark/commit/d48c8e21147b078518d3115a8c7c891b0b644ee7).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703433439
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34006/
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #29934:
URL: https://github.com/apache/spark/pull/29934
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703171143
**[Test build #129385 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129385/testReport)** for PR 29934 at commit [`9993d13`](https://github.com/apache/spark/commit/9993d13a234d066d3caa11bb8a276a2bc54f23bc).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703433464
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499731594
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
@stijndehaes I'd vote for using the Constant.scala for store the value since the value has been there, but it is up to whatever code style in place or your final choice.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stijndehaes commented on a change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499356599
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala
##########
@@ -86,6 +86,7 @@ private[spark] object Constants {
val DEFAULT_EXECUTOR_CONTAINER_NAME = "spark-kubernetes-executor"
val MEMORY_OVERHEAD_MIN_MIB = 384L
val NON_JVM_MEMORY_OVERHEAD_FACTOR = 0.4d
+ val KUBERNETES_MAX_NAME_LENGTH = 63
Review comment:
In my first version of the PR it was unused but I changed it back to use this constant.
Unless we want to have it more like `DriverServiceFeatureStep` where the suffix is stored in the companion object.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703262874
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703289132
Thank you for updates, @stijndehaes .
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703170767
ok to 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.
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] stijndehaes commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
stijndehaes commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-705343221
@dongjoon-hyun @yuj Thanks for all the help guys :)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703176532
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33993/
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r500555515
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
Could you share your result from `k get cm`? Does it looks reasonably consistent?
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499301593
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
+ } else {
+ val randomServiceId = KubernetesUtils.uniqueID(clock = clock)
Review comment:
`randomServiceId` -> `randomId`?
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703182911
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-702775614
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.
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] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703175300
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33993/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703262874
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703255365
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703850118
Of course, @yuj . I'm considering this for Apache Spark 3.1/3.0.2/2.4.8.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499185916
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStepSuite.scala
##########
@@ -84,4 +90,30 @@ class PodTemplateConfigMapStepSuite extends SparkFunSuite {
(Constants.EXECUTOR_POD_SPEC_TEMPLATE_MOUNTPATH + "/" +
Constants.EXECUTOR_POD_SPEC_TEMPLATE_FILE_NAME))
}
+
+ test("Long prefixes should switch to using a generated unique name.") {
Review comment:
Could you add `SPARK-32067:` prefix please?
```scala
- test("Long prefixes should switch to using a generated unique name.") {
+ test("SPARK-32067: Long prefixes should switch to using a generated unique name.") {
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: Make sure the pod template configmap has a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-702774206
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.
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 change in pull request #29934: [SPARK-32067][K8S] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499301593
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
+ } else {
+ val randomServiceId = KubernetesUtils.uniqueID(clock = clock)
Review comment:
`randomServiceId` -> `randomId` because this is irrelevant to a service.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499270167
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
+ extends KubernetesFeatureConfigStep with Logging {
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val preferredConfigmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
+ private val resolvedConfigmapName =
+ if (preferredConfigmapName.length <= KUBERNETES_MAX_NAME_LENGTH) {
+ preferredConfigmapName
Review comment:
indentation (line 39 ~ 40)?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yuj commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
yuj commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703848160
@dongjoon-hyun Could you please consider include this bug fix as part of any coming release. This is a much needed fix for probably many people.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704029124
**[Test build #129433 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129433/testReport)** for PR 29934 at commit [`948e15a`](https://github.com/apache/spark/commit/948e15a48e68022ee39f715a883e62fbe2f15397).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703832778
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34026/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703260514
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33996/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703426566
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34006/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704025741
**[Test build #129433 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129433/testReport)** for PR 29934 at commit [`948e15a`](https://github.com/apache/spark/commit/948e15a48e68022ee39f715a883e62fbe2f15397).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-704035641
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34040/
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][K8S] Use unique ConfigMap name for executor pod template
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499845657
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -31,14 +31,16 @@ private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
private val hasTemplate = conf.contains(KUBERNETES_EXECUTOR_PODTEMPLATE_FILE)
+ private val configmapName = s"${conf.resourceNamePrefix}-$POD_TEMPLATE_CONFIGMAP"
Review comment:
This makes the following status.
```
tpcds-py-eebc4474fa60b6f7-driver-conf-map 1 5s
tpcds-py-eebc4474fa60b6f7-podspec-configmap 1 5s
```
This is inconsistent because Driver is using `driver-conf-map` postfix and executor is using `podspec-configmap`.
```
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala: val configMapName = s"${conf.resourceNamePrefix}-driver-conf-map"
```
Let's use `exec-conf-map` as a postfix.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29934:
URL: https://github.com/apache/spark/pull/29934#issuecomment-703176535
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #29934: [SPARK-32067][k8s] Give pod template configmap a unique name
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29934:
URL: https://github.com/apache/spark/pull/29934#discussion_r499270134
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala
##########
@@ -22,23 +22,39 @@ import java.nio.charset.StandardCharsets
import com.google.common.io.Files
import io.fabric8.kubernetes.api.model.{ConfigMapBuilder, ContainerBuilder, HasMetadata, PodBuilder}
-import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesUtils, SparkPod}
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{Clock, SystemClock}
-private[spark] class PodTemplateConfigMapStep(conf: KubernetesConf)
- extends KubernetesFeatureConfigStep {
+private[spark] class PodTemplateConfigMapStep
+(conf: KubernetesConf, clock: Clock = new SystemClock())
Review comment:
indentation?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org