[SPARK-32067][K8S] Use unique ConfigMap name for executor pod template #29934

stijndehaes · 2020-10-02T14:40:47Z

What changes were proposed in this pull request?

The pod template configmap always had the same name. This PR makes it unique.

Why are the changes needed?

If you scheduled 2 spark jobs they will both use the same configmap name this will result in conflicts. This PR fixes that

BEFORE

$ kubectl get cm --all-namespaces -w | grep podspec
podspec-configmap                              1      65s

AFTER

$ kubectl get cm --all-namespaces -w | grep podspec
aaece65ef82e4a30b7b7800aad600d4f   spark-test-app-aac9f37502b2ca55-driver-podspec-conf-map   1      0s

This can be seen when running the integration tests

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests and the integration tests test if this works

HyukjinKwon · 2020-10-03T04:09:26Z

Can you file a jira and link it to the PR title? See also https://spark.apache.org/contributing.html

stijndehaes · 2020-10-03T07:57:34Z

@onursatici @vanzin I see you guys have changed parts of this code would you kindly review?

stijndehaes · 2020-10-03T07:58:02Z

@HyukjinKwon Sorry forgot to add the existing JIRA ticket in the title. I also added the K8S component in the title now

yuj

Please consider using the already-defined podspec-configmap constant.

...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala

...core/src/test/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStepSuite.scala

dongjoon-hyun · 2020-10-03T22:10:05Z

ok to test

SparkQA · 2020-10-03T23:05:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33993/

SparkQA · 2020-10-03T23:22:27Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33993/

SparkQA · 2020-10-04T00:48:49Z

Test build #129385 has finished for PR 29934 at commit 9993d13.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-04T13:24:01Z

Test build #129389 has finished for PR 29934 at commit 66609a2.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-04T13:40:54Z

Test build #129390 has finished for PR 29934 at commit a4e423c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-04T14:03:52Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33996/

SparkQA · 2020-10-04T14:18:08Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33997/

SparkQA · 2020-10-04T14:20:54Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33996/

SparkQA · 2020-10-04T14:38:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33997/

dongjoon-hyun · 2020-10-04T17:35:51Z

Thank you for updates, @stijndehaes .

...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Constants.scala

SparkQA · 2020-10-05T06:00:05Z

Test build #129399 has finished for PR 29934 at commit 09689f2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-05T06:29:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34006/

SparkQA · 2020-10-05T06:45:10Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34006/

SparkQA · 2020-10-05T18:43:42Z

Test build #129419 has finished for PR 29934 at commit d48c8e2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-05T19:12:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34026/

SparkQA · 2020-10-05T19:29:05Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34026/

dongjoon-hyun · 2020-10-05T19:39:33Z

Wow, it's much simpler, @stijndehaes . :)

yuj · 2020-10-05T19:43:29Z

@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.

dongjoon-hyun · 2020-10-05T19:47:43Z

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.

dongjoon-hyun · 2020-10-05T20:13:55Z

...etes/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"


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.

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.

Could you share your result from k get cm? Does it looks reasonably consistent?

Also, please update your PR description. I added AFTER paragraph at this PR description. You can fill in the final output there.

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.

Got it. Thank you, @stijndehaes .

dongjoon-hyun

@stijndehaes . This PR looks good except the consistency between driver and executor. Please update it.

https://github.com/apache/spark/pull/29934/files#r499845657

The pod template configmap always had the same name for each job submitted. This means if you schedule 2 spark jobs in the same namespace there will be conflicts.

SparkQA · 2020-10-06T05:04:20Z

Test build #129433 has finished for PR 29934 at commit 948e15a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-06T05:26:26Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34040/

SparkQA · 2020-10-06T05:42:36Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34040/

dongjoon-hyun

+1, LGTM. Merged to master/3.0 for Apache Spark 3.1.0/3.0.2.

### What changes were proposed in this pull request? The pod template configmap always had the same name. This PR makes it unique. ### Why are the changes needed? If you scheduled 2 spark jobs they will both use the same configmap name this will result in conflicts. This PR fixes that **BEFORE** ``` $ kubectl get cm --all-namespaces -w | grep podspec podspec-configmap 1 65s ``` **AFTER** ``` $ kubectl get cm --all-namespaces -w | grep podspec aaece65ef82e4a30b7b7800aad600d4f spark-test-app-aac9f37502b2ca55-driver-podspec-conf-map 1 0s ``` This can be seen when running the integration tests ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit tests and the integration tests test if this works Closes #29934 from stijndehaes/bugfix/SPARK-32067-unique-name-for-template-configmap. Authored-by: Stijn De Haes <stijndehaes@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 3099fd9) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2020-10-07T16:56:52Z

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!

yuj · 2020-10-07T17:13:35Z

Awesome. Thanks guys @stijndehaes @dongjoon-hyun

stijndehaes · 2020-10-08T05:43:18Z

@dongjoon-hyun @yuj Thanks for all the help guys :)

### What changes were proposed in this pull request? The pod template configmap always had the same name. This PR makes it unique. ### Why are the changes needed? If you scheduled 2 spark jobs they will both use the same configmap name this will result in conflicts. This PR fixes that **BEFORE** ``` $ kubectl get cm --all-namespaces -w | grep podspec podspec-configmap 1 65s ``` **AFTER** ``` $ kubectl get cm --all-namespaces -w | grep podspec aaece65ef82e4a30b7b7800aad600d4f spark-test-app-aac9f37502b2ca55-driver-podspec-conf-map 1 0s ``` This can be seen when running the integration tests ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Unit tests and the integration tests test if this works Closes apache#29934 from stijndehaes/bugfix/SPARK-32067-unique-name-for-template-configmap. Authored-by: Stijn De Haes <stijndehaes@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 3099fd9) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

stijndehaes changed the title ~~Make sure the pod template configmap has a unique name~~ [SPARK-32067][K8S]Make sure the pod template configmap has a unique name Oct 3, 2020

stijndehaes changed the title ~~[SPARK-32067][K8S]Make sure the pod template configmap has a unique name~~ [SPARK-32067][k8s] Give pod template configmap a unique name Oct 3, 2020

yuj reviewed Oct 3, 2020

View reviewed changes

...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStep.scala Outdated Show resolved Hide resolved

...core/src/test/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStepSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Oct 3, 2020

View reviewed changes

...core/src/test/scala/org/apache/spark/deploy/k8s/features/PodTemplateConfigMapStepSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun added the KUBERNETES label Oct 4, 2020