[SPARK-24434][K8S] pod template files #22146

onursatici · 2018-08-20T01:23:02Z

What changes were proposed in this pull request?

New feature to pass podspec files for driver and executor pods.

How was this patch tested?

new unit and integration tests

more overwrites in integration tests
invalid template integration test, documentation

mccheah · 2018-08-20T18:58:29Z

@skonto @erikerlandson

erikerlandson · 2018-08-20T19:03:30Z

Jenkins, OK to test

erikerlandson · 2018-08-20T19:05:23Z

Thanks @onursatici!
Can you please resolve merge conflicts (rebase?), so CI can build it?

yifeih · 2018-08-22T00:48:50Z

@onursatici is also away this week, so I discussed with him and have taken over the PR

This v0 implementation simply starts with the pod template instead of an empty pod, applying any sparkConf overrides, either user-specified or default values, on top of the template. It currently doesn't warn upon overriding anything that's also specified in the pod template

mccheah

Thanks for working on this. Some comments. Also might want an integration test in KubernetesSuite?

mccheah · 2018-08-22T00:25:56Z

...ce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala

+    try {
+      val pod = kubernetesClient.pods().load(templateFile).get()
+      val container = pod.getSpec.getContainers.asScala
+        .filter(_.getName == containerName)


Can use require(...exists)

mccheah · 2018-08-22T00:32:53Z

...kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/TemplateVolumeStep.scala

+    require(conf.get(Config.KUBERNETES_EXECUTOR_PODTEMPLATE_FILE).isDefined)
+    val podTemplateFile = conf.get(Config.KUBERNETES_EXECUTOR_PODTEMPLATE_FILE).get
+    val podWithVolume = new PodBuilder(pod.pod)
+      .editSpec()


Match the indentation here with the indentation style down below.

mccheah · 2018-08-22T00:33:19Z

...kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/TemplateVolumeStep.scala

+      .build()
+
+    val containerWithVolume = new ContainerBuilder(pod.container)
+        .withVolumeMounts(new VolumeMountBuilder()


addNewVolumeMount()

mccheah · 2018-08-22T00:34:33Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilder.scala

+          val sparkPod = KubernetesUtils.loadPodFromTemplate(
+            kubernetesClient,
+            file,
+            Constants.DRIVER_CONTAINER_NAME)


Unclear if these container names should be configurable.

You may import org.apache.spark.deploy.k8s.Constants._ at the top of the file and then not need the Constants prefix here.

They're not configurable currently. We should probably make them configurable since I'd imagine people would want to rely on these names being consistent

I think we should also make the driver container name configurable.

mccheah · 2018-08-22T00:35:21Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilder.scala

+
+import org.apache.spark.{SparkConf, SparkException}
+import org.apache.spark.deploy.k8s._
+import org.apache.spark.deploy.k8s.features._


Undo these import changes. Keep the ordering correct, but import each class individually.

mccheah · 2018-08-22T00:38:55Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilder.scala

+    provideTemplateVolumeStep: (KubernetesConf[_ <: KubernetesRoleSpecificConf]
+      => TemplateVolumeStep) =
+    new TemplateVolumeStep(_),
+    provideInitialSpec: KubernetesConf[KubernetesDriverSpecificConf]


Why not provideInitialPod to be consistent with the executor builder?

I think it's because we need the KubernetesDriverSpec object instead of just the pod? which includes things like the entire sparkConf map

You can make the initial pod and wrap it in the KubernetesDriverSpec object.

mccheah · 2018-08-22T00:48:07Z

...kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/TemplateVolumeStep.scala

+      .editSpec()
+      .addNewVolume()
+      .withName(Constants.POD_TEMPLATE_VOLUME)
+      .withHostPath(new HostPathVolumeSource(podTemplateFile))


hostPath is not the correct volume type here. Instead, do the following:

Override getAdditionalKubernetesResources() with the following:

Load the contents of the template file from this process's local disk into a UTF-8 String

Create and return a ConfigMap object containing the contents of that config map, with some given key

In configurePod, add the config map as a volume in the pod spec, and add a volume mount pointing to that volume as done here.

Can we also change the name of this class to something like PodTemplateConfigMapStep to make it clear that is uses a ConfigMap to ship the template file?

mccheah · 2018-08-22T00:51:26Z

...kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/TemplateVolumeStep.scala

+
+import org.apache.spark.deploy.k8s._
+
+private[spark] class TemplateVolumeStep(


So this pushes the pod spec yml from the spark-submit process's local disk up to the driver pod. It may be worthwhile to support specifying the file as a location in the driver pod that hasn't been mounted by spark-submit, but I think doing it this way is fine for now.

mccheah · 2018-08-22T17:04:06Z

Jenkins, ok to test

mccheah · 2018-08-22T17:07:53Z

@yifeih can we also modify docs/running-on-kubernetes.md and have a section specific to using this feature? Mark it as experimental also.

SparkQA · 2018-08-22T17:17:32Z

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

SparkQA · 2018-08-22T17:25:32Z

Test build #95112 has finished for PR 22146 at commit 8ef756e.

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

shaneknapp · 2018-10-29T20:45:26Z

test this please

SparkQA · 2018-10-29T21:00:14Z

Test build #98231 has finished for PR 22146 at commit 80b56c1.

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

SparkQA · 2018-10-29T21:04:50Z

Test build #98233 has finished for PR 22146 at commit 80b56c1.

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

erikerlandson · 2018-10-29T21:07:21Z

thanks @shaneknapp !

SparkQA · 2018-10-29T21:23:38Z

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

SparkQA · 2018-10-29T21:41:08Z

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

mccheah · 2018-10-29T22:07:42Z

retest this please

SparkQA · 2018-10-29T22:22:40Z

Test build #98237 has finished for PR 22146 at commit 80b56c1.

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

SparkQA · 2018-10-29T22:36:38Z

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

SparkQA · 2018-10-29T22:48:11Z

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

erikerlandson · 2018-10-29T23:39:39Z

@mccheah integration testing is passing with the latest container selection policy, good to merge?

mccheah

Only two more nits and then will merge.

@skonto @aditanase FYI. Are any of your unresolved comments blocking?

If we don't get other feedback here we will probably merge in short order.

mccheah · 2018-10-30T15:53:34Z

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

+  // Pod spec templates
+  val EXECUTOR_POD_SPEC_TEMPLATE_FILE_NAME = "pod-spec-template.yml"
+  val EXECUTOR_POD_SPEC_TEMPLATE_MOUNTHPATH = "/opt/spark/pod-template"
+  val POD_TEMPLATE_VOLUME = "podspec-volume"


mccheah · 2018-10-30T15:54:06Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilder.scala

+
+import org.apache.spark.{SparkConf, SparkException}
+import org.apache.spark.deploy.k8s._
+import org.apache.spark.deploy.k8s.features._


SparkQA · 2018-10-30T16:38:26Z

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

SparkQA · 2018-10-30T16:42:28Z

Test build #98273 has finished for PR 22146 at commit 3707e6a.

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

SparkQA · 2018-10-30T16:48:49Z

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

mccheah · 2018-10-30T20:52:02Z

Ok I'm going to merge this into master. Thanks everyone for the feedback. Follow up discussions around validation can be addressed in follow up patches.

aditanase · 2018-11-02T09:21:34Z

@mccheah agreed on all points, sorry for taking this long to respond. Happy to see this merged in, can't wait to take it for a spin!

## What changes were proposed in this pull request? Adding docs for an enhancement that came in late in this PR: #22146 Currently the docs state that we're going to use the first container in a pod template, which was the implementation for some time, until it was improved with 2 new properties. ## How was this patch tested? I tested that the properties work by combining pod templates with client-mode and a simple pod template. Please review http://spark.apache.org/contributing.html before opening a pull request. Closes #23155 from aditanase/k8s-readme. Authored-by: Adrian Tanase <atanase@adobe.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

## What changes were proposed in this pull request? New feature to pass podspec files for driver and executor pods. ## How was this patch tested? new unit and integration tests - [x] more overwrites in integration tests - [ ] invalid template integration test, documentation Author: Onur Satici <osatici@palantir.com> Author: Yifei Huang <yifeih@palantir.com> Author: onursatici <onursatici@gmail.com> Closes apache#22146 from onursatici/pod-template.

## What changes were proposed in this pull request? Adding docs for an enhancement that came in late in this PR: apache#22146 Currently the docs state that we're going to use the first container in a pod template, which was the implementation for some time, until it was improved with 2 new properties. ## How was this patch tested? I tested that the properties work by combining pod templates with client-mode and a simple pod template. Please review http://spark.apache.org/contributing.html before opening a pull request. Closes apache#23155 from aditanase/k8s-readme. Authored-by: Adrian Tanase <atanase@adobe.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

New feature to pass podspec files for driver and executor pods. new unit and integration tests - [x] more overwrites in integration tests - [ ] invalid template integration test, documentation Author: Onur Satici <osatici@palantir.com> Author: Yifei Huang <yifeih@palantir.com> Author: onursatici <onursatici@gmail.com> Closes apache#22146 from onursatici/pod-template.

## What changes were proposed in this pull request? New feature to pass podspec files for driver and executor pods. ## How was this patch tested? new unit and integration tests - [x] more overwrites in integration tests - [ ] invalid template integration test, documentation Author: Onur Satici <osatici@palantir.com> Author: Yifei Huang <yifeih@palantir.com> Author: onursatici <onursatici@gmail.com> Closes apache#22146 from onursatici/pod-template.

onursatici added 9 commits August 17, 2018 13:16

add spec template configs

e2e7223

start from template for driver

ea4dde6

start from template for executor

4f088db

wip

f2f9a44

volume executor podspec template

368d0a4

move logic to apply functions

0005ea5

find containers

dda5cc9

style

d0f41aa

remove import

c4c1231

yifeih added 10 commits August 21, 2018 16:50

compiles

74de0e5

tests pass

205ddd3

adding TemplateVolumeStepSuite

4ae6fc6

DriverBuilder test

c0bcfea

WIP trying to write tests for KubernetesDriverBuilder constructor

b9e4263

fix test

c5e1ea0

fix test, and move loading logic to util method

56a6b32

validate that the executor pod template is good in the driver

7d0d928

cleaning

1da79a8

Merge branch 'apache/master' into yh/pod-template

8ef756e

mccheah suggested changes Aug 22, 2018

View reviewed changes

mccheah reviewed Aug 22, 2018

View reviewed changes

onursatici changed the title ~~[WIP][SPARK-24434][K8S] pod template files~~ [SPARK-24434][K8S] pod template files Aug 22, 2018

mccheah reviewed Oct 30, 2018

View reviewed changes

onursatici added 2 commits October 30, 2018 16:24

rename pod template volume name

8f7f571

imports

3707e6a

asfgit closed this in f6cc354 Oct 30, 2018

aditanase mentioned this pull request Nov 27, 2018

[MINOR][K8S] add missing docs for podTemplateContainerName properties #23155

Closed

erikerlandson mentioned this pull request Jul 22, 2019

[SPARK-28149][K8S] Added variable to disable negative DNS caching #24962

Closed


		import org.apache.spark.deploy.k8s._

		private[spark] class TemplateVolumeStep(

[SPARK-24434][K8S] pod template files #22146

[SPARK-24434][K8S] pod template files #22146

Conversation

onursatici commented Aug 20, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

mccheah commented Aug 20, 2018

erikerlandson commented Aug 20, 2018

erikerlandson commented Aug 20, 2018

yifeih commented Aug 22, 2018

mccheah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented Aug 22, 2018

mccheah commented Aug 22, 2018

SparkQA commented Aug 22, 2018

SparkQA commented Aug 22, 2018

shaneknapp commented Oct 29, 2018

SparkQA commented Oct 29, 2018

SparkQA commented Oct 29, 2018

erikerlandson commented Oct 29, 2018

SparkQA commented Oct 29, 2018

SparkQA commented Oct 29, 2018

mccheah commented Oct 29, 2018

SparkQA commented Oct 29, 2018

SparkQA commented Oct 29, 2018

SparkQA commented Oct 29, 2018

erikerlandson commented Oct 29, 2018

mccheah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 30, 2018

SparkQA commented Oct 30, 2018

SparkQA commented Oct 30, 2018

mccheah commented Oct 30, 2018

aditanase commented Nov 2, 2018

onursatici commented Aug 20, 2018 •

edited

Loading