Add parameter for driver pod name #258

hustcat · 2017-05-02T06:21:50Z

Add parameter for driver pod name. #252

lins05 · 2017-05-03T15:34:01Z

This could be handy. One question: IIRC the reason of appending timestamps to driver pod name (as well as the driver service name, we use the same for them) is because pod/service names must be unique within the same k8s namespace and we want to avoid conflictions. So should we check if there is already a pod/service with the same name before launching the driver pod?

If so, maybe it's also worth adding an integration test?

hustcat · 2017-05-04T02:32:38Z

If pod name is exist, k8s API server will return error for spark-submit, such as:

Exception in thread "main" io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: POST at: https://test.k8s:6443/api/v1/namespaces/dbyin/pods. Message: pods "spark-pi-001" already exists. Received status: Status(apiVersion=v1, code=409, details=StatusDetails(causes=[], group=null, kind=pods, name=spark-pi-001, retryAfterSeconds=null, additionalProperties={}), kind=Status, message=pods "spark-pi-001" already exists, metadata=ListMeta(resourceVersion=null, selfLink=null, additionalProperties={}), reason=AlreadyExists, status=Failure, additionalProperties={}).

I think this is what we expect. Of course, we can check it before create driver pod, but I think it's unnecessary, k8s API server do this is better when creating pod.

foxish · 2017-05-04T02:40:22Z

The parameter is currently marked internal in config.scala.
We should probably mark that as not being internal and add documentation if we want to expose it to the user.

  private[spark] val KUBERNETES_DRIVER_POD_NAME =
    ConfigBuilder("spark.kubernetes.driver.pod.name")
      .doc("Name of the driver pod.")
      .internal()
      .stringConf
      .createOptional

foxish · 2017-05-04T02:41:06Z

Also would need to be added here: https://github.com/apache-spark-on-k8s/spark/blob/branch-2.1-kubernetes/docs/running-on-kubernetes.md#spark-properties

hustcat · 2017-05-08T04:12:37Z

@foxish @lins05 Some updates, PTAL.

foxish · 2017-05-09T21:17:46Z

...ts/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesV1Suite.scala

      "Unexpected value for annotation2")
  }

+  test("Run with driver pod name") {


I wish we did not add more e2es and instead used unit tests better. This one is fine for now, but it's something to consider in the future.

foxish · 2017-05-09T21:18:18Z

LGTM. @lins05 @ash211 any comments?

ash211 · 2017-05-10T05:32:30Z

...ers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/submit/v1/Client.scala

  private val appName = sparkConf.getOption("spark.app.name")
    .getOrElse("spark")
-  private val kubernetesAppId = s"$appName-$launchTime".toLowerCase.replaceAll("\\.", "-")
+  private val kubernetesAppId = sparkConf.getOption("spark.kubernetes.driver.pod.name")


the pod name spark.kubernetes.driver.pod.name and the app ID spark.app.id are slightly different -- I think the change on this line should be pulling from spark.app.id if we want to make the app ID configurable.

Given that this PR is about the pod name and not the app ID, maybe drop the change on this line and leave just the pod name configurable?

It's reasonable:)

ash211 · 2017-05-11T08:07:24Z

...ers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/submit/v1/Client.scala

  private val appName = sparkConf.getOption("spark.app.name")
    .getOrElse("spark")
  private val kubernetesAppId = s"$appName-$launchTime".toLowerCase.replaceAll("\\.", "-")
+  private val kubernetesDriverPodName = sparkConf.getOption("spark.kubernetes.driver.pod.name")


can you use KUBERNETES_DRIVER_POD_NAME here?

ash211 · 2017-05-11T08:07:31Z

...ers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/submit/v2/Client.scala

  private val appName = sparkConf.getOption("spark.app.name")
    .getOrElse("spark")
  private val kubernetesAppId = s"$appName-$launchTime".toLowerCase.replaceAll("\\.", "-")
+  private val kubernetesDriverPodName = sparkConf.getOption("spark.kubernetes.driver.pod.name")


…_NAME

ash211

LGTM let's merge if tests pass

lins05 · 2017-05-12T15:13:39Z

docs/running-on-kubernetes.md

+  <td><code>spark.kubernetes.driver.pod.name</code></td>
+  <td><code>(none)</code></td>
+  <td>
+    Name of the driver pod


Maybe add this: If not set, the driver pod name would be "spark.app.name" suffixed by a timestamp to avoid confliction with other spark apps.

Done. Thanks @lins05

* Add parameter for driver pod name * Mark KUBERNETES_DRIVER_POD_NAME not being internal. Update docment. * Add test case for driver pod name * Diff driver pod name with appid * replace 'spark.kubernetes.driver.pod.name` with KUBERNETES_DRIVER_POD_NAME * Update readme to complete item

[NOSQUASH] Resync kube

* Add parameter for driver pod name * Mark KUBERNETES_DRIVER_POD_NAME not being internal. Update docment. * Add test case for driver pod name * Diff driver pod name with appid * replace 'spark.kubernetes.driver.pod.name` with KUBERNETES_DRIVER_POD_NAME * Update readme to complete item

hustcat added 2 commits May 4, 2017 12:36

Add parameter for driver pod name

dad36fd

Mark KUBERNETES_DRIVER_POD_NAME not being internal. Update docment.

85a7c53

hustcat force-pushed the driver_podname branch from b863889 to 85a7c53 Compare May 4, 2017 06:23

Add test case for driver pod name

5d3e4c2

hustcat force-pushed the driver_podname branch from 7b052a3 to 5d3e4c2 Compare May 8, 2017 03:34

foxish reviewed May 9, 2017

View reviewed changes

Merge branch 'branch-2.1-kubernetes' into driver_podname

85c76d3

ash211 reviewed May 10, 2017

View reviewed changes

Diff driver pod name with appid

e37e78b

ash211 reviewed May 11, 2017

View reviewed changes

ash211 and others added 2 commits May 11, 2017 01:07

Merge branch 'branch-2.1-kubernetes' into driver_podname

90985cf

replace 'spark.kubernetes.driver.pod.name` with KUBERNETES_DRIVER_POD…

f55ade9

…_NAME

ash211 approved these changes May 11, 2017

View reviewed changes

lins05 reviewed May 12, 2017

View reviewed changes

Update readme to complete item

aa7de0d

foxish merged commit eb45ae5 into apache-spark-on-k8s:branch-2.1-kubernetes May 16, 2017

mccheah mentioned this pull request Jun 8, 2017

Discuss naming schemes for pods, config maps, and secrets that are created for the job #335

Open

ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 26, 2019

Merge pull request apache-spark-on-k8s#258 from palantir/resync-kube

a72151f

[NOSQUASH] Resync kube

Add parameter for driver pod name #258

Add parameter for driver pod name #258

Uh oh!

Conversation

hustcat commented May 2, 2017

Uh oh!

lins05 commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hustcat commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

foxish commented May 4, 2017

Uh oh!

foxish commented May 4, 2017

Uh oh!

hustcat commented May 8, 2017

Uh oh!

foxish May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxish commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ash211 May 10, 2017

Choose a reason for hiding this comment

Uh oh!

hustcat May 11, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 May 11, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 May 11, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

lins05 May 12, 2017

Choose a reason for hiding this comment

Uh oh!

foxish May 16, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lins05 commented May 3, 2017 •

edited

Loading

hustcat commented May 4, 2017 •

edited

Loading

foxish May 9, 2017 •

edited

Loading

foxish commented May 9, 2017 •

edited

Loading