[SPARK-25295][K8S]Fix executor names collision #22405

skonto · 2018-09-12T19:57:40Z

What changes were proposed in this pull request?

Fixes the collision issue with spark executor names in client mode, see SPARK-25295 for the details.
It follows the cluster name convention as app-name will be used as the prefix and if that is not defined we use "spark" as the default prefix. Eg. spark-pi-1536781360723-exec-1 where spark-pi is the name of the app passed at the config side or transformed if it contains illegal characters.

Also fixes the issue with spark app name having spaces in cluster mode.
If you run the Spark Pi test in client mode it passes.
The tricky part is the user may set the app name:

spark/examples/src/main/scala/org/apache/spark/examples/SparkPi.scala

Line 30 in 3030b82

.appName("Spark Pi")

If i do:

./bin/spark-submit 
...
 --deploy-mode cluster --name "spark pi"
...

it will fail as the app name is used for the prefix of driver's pod name and it cannot have spaces (according to k8s conventions).

How was this patch tested?

Manually by running spark job in client mode.
To reproduce do:

kubectl create -f service.yaml
kubectl create -f pod.yaml

service.yaml :

kind: Service
apiVersion: v1
metadata:
  name: spark-test-app-1-svc
spec:
  clusterIP: None
  selector:
    spark-app-selector: spark-test-app-1
  ports:
  - protocol: TCP
    name: driver-port
    port: 7077
    targetPort: 7077
  - protocol: TCP
    name: block-manager
    port: 10000
    targetPort: 10000

pod.yaml:

apiVersion: v1
kind: Pod
metadata:
  name: spark-test-app-1
  labels:
    spark-app-selector: spark-test-app-1
spec:
  containers:
  - name: spark-test
    image: skonto/spark:k8s-client-fix
    imagePullPolicy: Always
    command:
      - 'sh'
      - '-c'
      -  "/opt/spark/bin/spark-submit
              --verbose
              --master k8s://https://kubernetes.default.svc
              --deploy-mode client
              --class org.apache.spark.examples.SparkPi
              --conf spark.app.name=spark
              --conf spark.executor.instances=1
              --conf spark.kubernetes.container.image=skonto/spark:k8s-client-fix
              --conf spark.kubernetes.container.image.pullPolicy=Always
              --conf spark.kubernetes.authenticate.oauthTokenFile=/var/run/secrets/kubernetes.io/serviceaccount/token
              --conf spark.kubernetes.authenticate.caCertFile=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt
              --conf spark.executor.memory=500m
              --conf spark.executor.cores=1
              --conf spark.executor.instances=1
              --conf spark.driver.host=spark-test-app-1-svc.default.svc
              --conf spark.driver.port=7077
              --conf spark.driver.blockManager.port=10000
              local:///opt/spark/examples/jars/spark-examples_2.11-2.4.0-SNAPSHOT.jar 1000000"

skonto · 2018-09-12T20:01:20Z

@liyinan926 @mccheah pls review.

SparkQA · 2018-09-12T20:04:57Z

Test build #96003 has finished for PR 22405 at commit d6b4a9f.

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

skonto · 2018-09-12T20:05:12Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+      .toLowerCase
+      .replaceAll(" +", " ")
+      .replaceAll("\\s", "-")
+      .replaceAll("[^A-Za-z0-9\\-]", "")


Might be a bit strict but if people want weird names then they should know k8s does not accept it and we use the appname for their convenience when they list pods. If they want a weird appname then they should set the pod name explicitly.

K8s resource names by convention are all lower cases, and can have - and .. So I would suggest adding . here and add another call to toLowerCase() after. See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names.

Correct will update.

SparkQA · 2018-09-12T20:12:44Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3057/

SparkQA · 2018-09-12T20:22:52Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3057/

liyinan926 · 2018-09-12T20:31:02Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+    s"$appName-$launchTime"
+      .toLowerCase
+      .replaceAll(" +", " ")
+      .replaceAll("\\s", "-")


Why not just do replaceAll("\\s+", "-") instead of the two above?

liyinan926 · 2018-09-12T20:33:15Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+      .toLowerCase
+      .replaceAll(" +", " ")
+      .replaceAll("\\s", "-")
+      .replaceAll("[^A-Za-z0-9\\-]", "")


K8s resource names by convention are all lower cases, and can have - and .. So I would suggest adding . here and add another call to toLowerCase() after. See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names.

SparkQA · 2018-09-12T20:33:34Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3058/

SparkQA · 2018-09-12T20:40:16Z

Test build #96004 has finished for PR 22405 at commit 245d1ec.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-09-12T20:42:27Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3058/

skonto · 2018-09-12T21:14:07Z

@liyinan926 the prefix before this PR is defined as: val kubernetesResourceNamePrefix = { s"$appName-$launchTime".toLowerCase.replaceAll("\\.", "-") }
So it looks better to replace . with - will do that.

skonto · 2018-09-12T22:50:26Z

scalastyle &tests fixed...

skonto · 2018-09-12T22:50:50Z

jenkins, test this please.

liyinan926 · 2018-09-12T22:55:38Z

.../core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocatorSuite.scala

+          // string for the prefix, based on the app name, and this comparison here will fail.
+          val k8sConfCopy = k8sConf
+            .copy(appResourceNamePrefix = "")
+              .copy(sparkConf = conf)


Indention here seems off.

skonto · 2018-09-12T22:56:36Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+      .replaceAll("\\.", "-")
+      .replaceAll("[^a-z0-9\\-]", "")
+      .replaceAll("-+", "-")
+  }


reduce dashes. for example:

scala> " Spark #$ d #.Pi" .trim .toLowerCase .replaceAll("\\s+", "-") .replaceAll("\\.", "-") .replaceAll("[^a-z0-9\\-]", "") .replaceAll("-+", "-") res15: String = spark-d-pi

SparkQA · 2018-09-12T23:02:31Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3063/

SparkQA · 2018-09-12T23:10:03Z

Test build #96010 has finished for PR 22405 at commit 62729b6.

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

SparkQA · 2018-09-12T23:11:30Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3063/

liyinan926 · 2018-09-12T23:16:39Z

Will merge later today if there's no more comments.

SparkQA · 2018-09-12T23:16:50Z

Test build #96011 has finished for PR 22405 at commit 689f09b.

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

SparkQA · 2018-09-12T23:22:31Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3064/

SparkQA · 2018-09-12T23:22:44Z

Test build #96012 has finished for PR 22405 at commit 6975d69.

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

SparkQA · 2018-09-12T23:26:57Z

Test build #96013 has finished for PR 22405 at commit e28c2da.

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

skonto · 2018-09-12T23:28:43Z

Thanks @liyinan926!

SparkQA · 2018-09-12T23:31:54Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3064/

SparkQA · 2018-09-12T23:42:51Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3065/

SparkQA · 2018-09-12T23:52:22Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3065/

SparkQA · 2018-09-13T00:02:47Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3066/

SparkQA · 2018-09-13T00:12:32Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/3066/

## What changes were proposed in this pull request? Fixes the collision issue with spark executor names in client mode, see SPARK-25295 for the details. It follows the cluster name convention as app-name will be used as the prefix and if that is not defined we use "spark" as the default prefix. Eg. `spark-pi-1536781360723-exec-1` where spark-pi is the name of the app passed at the config side or transformed if it contains illegal characters. Also fixes the issue with spark app name having spaces in cluster mode. If you run the Spark Pi test in client mode it passes. The tricky part is the user may set the app name: https://github.com/apache/spark/blob/3030b82c89d3e45a2e361c469fbc667a1e43b854/examples/src/main/scala/org/apache/spark/examples/SparkPi.scala#L30 If i do: ``` ./bin/spark-submit ... --deploy-mode cluster --name "spark pi" ... ``` it will fail as the app name is used for the prefix of driver's pod name and it cannot have spaces (according to k8s conventions). ## How was this patch tested? Manually by running spark job in client mode. To reproduce do: ``` kubectl create -f service.yaml kubectl create -f pod.yaml ``` service.yaml : ``` kind: Service apiVersion: v1 metadata: name: spark-test-app-1-svc spec: clusterIP: None selector: spark-app-selector: spark-test-app-1 ports: - protocol: TCP name: driver-port port: 7077 targetPort: 7077 - protocol: TCP name: block-manager port: 10000 targetPort: 10000 ``` pod.yaml: ``` apiVersion: v1 kind: Pod metadata: name: spark-test-app-1 labels: spark-app-selector: spark-test-app-1 spec: containers: - name: spark-test image: skonto/spark:k8s-client-fix imagePullPolicy: Always command: - 'sh' - '-c' - "/opt/spark/bin/spark-submit --verbose --master k8s://https://kubernetes.default.svc --deploy-mode client --class org.apache.spark.examples.SparkPi --conf spark.app.name=spark --conf spark.executor.instances=1 --conf spark.kubernetes.container.image=skonto/spark:k8s-client-fix --conf spark.kubernetes.container.image.pullPolicy=Always --conf spark.kubernetes.authenticate.oauthTokenFile=/var/run/secrets/kubernetes.io/serviceaccount/token --conf spark.kubernetes.authenticate.caCertFile=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt --conf spark.executor.memory=500m --conf spark.executor.cores=1 --conf spark.executor.instances=1 --conf spark.driver.host=spark-test-app-1-svc.default.svc --conf spark.driver.port=7077 --conf spark.driver.blockManager.port=10000 local:///opt/spark/examples/jars/spark-examples_2.11-2.4.0-SNAPSHOT.jar 1000000" ``` Closes #22405 from skonto/fix-k8s-client-mode-executor-names. Authored-by: Stavros Kontopoulos <stavros.kontopoulos@lightbend.com> Signed-off-by: Yinan Li <ynli@google.com> (cherry picked from commit 3e75a9f) Signed-off-by: Yinan Li <ynli@google.com>

## What changes were proposed in this pull request? Fixes the collision issue with spark executor names in client mode, see SPARK-25295 for the details. It follows the cluster name convention as app-name will be used as the prefix and if that is not defined we use "spark" as the default prefix. Eg. `spark-pi-1536781360723-exec-1` where spark-pi is the name of the app passed at the config side or transformed if it contains illegal characters. Also fixes the issue with spark app name having spaces in cluster mode. If you run the Spark Pi test in client mode it passes. The tricky part is the user may set the app name: https://github.com/apache/spark/blob/3030b82c89d3e45a2e361c469fbc667a1e43b854/examples/src/main/scala/org/apache/spark/examples/SparkPi.scala#L30 If i do: ``` ./bin/spark-submit ... --deploy-mode cluster --name "spark pi" ... ``` it will fail as the app name is used for the prefix of driver's pod name and it cannot have spaces (according to k8s conventions). ## How was this patch tested? Manually by running spark job in client mode. To reproduce do: ``` kubectl create -f service.yaml kubectl create -f pod.yaml ``` service.yaml : ``` kind: Service apiVersion: v1 metadata: name: spark-test-app-1-svc spec: clusterIP: None selector: spark-app-selector: spark-test-app-1 ports: - protocol: TCP name: driver-port port: 7077 targetPort: 7077 - protocol: TCP name: block-manager port: 10000 targetPort: 10000 ``` pod.yaml: ``` apiVersion: v1 kind: Pod metadata: name: spark-test-app-1 labels: spark-app-selector: spark-test-app-1 spec: containers: - name: spark-test image: skonto/spark:k8s-client-fix imagePullPolicy: Always command: - 'sh' - '-c' - "/opt/spark/bin/spark-submit --verbose --master k8s://https://kubernetes.default.svc --deploy-mode client --class org.apache.spark.examples.SparkPi --conf spark.app.name=spark --conf spark.executor.instances=1 --conf spark.kubernetes.container.image=skonto/spark:k8s-client-fix --conf spark.kubernetes.container.image.pullPolicy=Always --conf spark.kubernetes.authenticate.oauthTokenFile=/var/run/secrets/kubernetes.io/serviceaccount/token --conf spark.kubernetes.authenticate.caCertFile=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt --conf spark.executor.memory=500m --conf spark.executor.cores=1 --conf spark.executor.instances=1 --conf spark.driver.host=spark-test-app-1-svc.default.svc --conf spark.driver.port=7077 --conf spark.driver.blockManager.port=10000 local:///opt/spark/examples/jars/spark-examples_2.11-2.4.0-SNAPSHOT.jar 1000000" ``` Closes apache#22405 from skonto/fix-k8s-client-mode-executor-names. Authored-by: Stavros Kontopoulos <stavros.kontopoulos@lightbend.com> Signed-off-by: Yinan Li <ynli@google.com>

skonto commented Sep 12, 2018

View reviewed changes

skonto force-pushed the fix-k8s-client-mode-executor-names branch from d6b4a9f to 245d1ec Compare September 12, 2018 20:19

liyinan926 reviewed Sep 12, 2018

View reviewed changes

skonto force-pushed the fix-k8s-client-mode-executor-names branch from 245d1ec to 62729b6 Compare September 12, 2018 22:49

skonto force-pushed the fix-k8s-client-mode-executor-names branch from 62729b6 to 689f09b Compare September 12, 2018 22:54

liyinan926 reviewed Sep 12, 2018

View reviewed changes

skonto commented Sep 12, 2018

View reviewed changes

skonto force-pushed the fix-k8s-client-mode-executor-names branch from 689f09b to 6975d69 Compare September 12, 2018 22:57

fix executor names collision

e28c2da

skonto force-pushed the fix-k8s-client-mode-executor-names branch from 6975d69 to e28c2da Compare September 12, 2018 23:05

liyinan926 approved these changes Sep 12, 2018

View reviewed changes

asfgit closed this in 3e75a9f Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25295][K8S]Fix executor names collision #22405

[SPARK-25295][K8S]Fix executor names collision #22405

skonto commented Sep 12, 2018 •

edited

skonto commented Sep 12, 2018

SparkQA commented Sep 12, 2018

skonto Sep 12, 2018 •

edited

liyinan926 Sep 12, 2018 •

edited

skonto Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

liyinan926 Sep 12, 2018

skonto Sep 12, 2018

liyinan926 Sep 12, 2018 •

edited

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

skonto commented Sep 12, 2018 •

edited

skonto commented Sep 12, 2018 •

edited

skonto commented Sep 12, 2018

liyinan926 Sep 12, 2018

skonto Sep 12, 2018

skonto Sep 12, 2018 •

edited

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

liyinan926 commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

skonto commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 13, 2018

SparkQA commented Sep 13, 2018

[SPARK-25295][K8S]Fix executor names collision #22405

[SPARK-25295][K8S]Fix executor names collision #22405

Conversation

skonto commented Sep 12, 2018 • edited

What changes were proposed in this pull request?

How was this patch tested?

skonto commented Sep 12, 2018

SparkQA commented Sep 12, 2018

skonto Sep 12, 2018 • edited

Choose a reason for hiding this comment

liyinan926 Sep 12, 2018 • edited

Choose a reason for hiding this comment

skonto Sep 12, 2018

Choose a reason for hiding this comment

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

liyinan926 Sep 12, 2018

Choose a reason for hiding this comment

skonto Sep 12, 2018

Choose a reason for hiding this comment

liyinan926 Sep 12, 2018 • edited

Choose a reason for hiding this comment

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

skonto commented Sep 12, 2018 • edited

skonto commented Sep 12, 2018 • edited

skonto commented Sep 12, 2018

liyinan926 Sep 12, 2018

Choose a reason for hiding this comment

skonto Sep 12, 2018

Choose a reason for hiding this comment

skonto Sep 12, 2018 • edited

Choose a reason for hiding this comment

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

liyinan926 commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

skonto commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 12, 2018

SparkQA commented Sep 13, 2018

SparkQA commented Sep 13, 2018

skonto commented Sep 12, 2018 •

edited

skonto Sep 12, 2018 •

edited

liyinan926 Sep 12, 2018 •

edited

liyinan926 Sep 12, 2018 •

edited

skonto commented Sep 12, 2018 •

edited

skonto commented Sep 12, 2018 •

edited

skonto Sep 12, 2018 •

edited