[SPARK-23529][K8s] Support mounting volumes #21260

andrusha · 2018-05-07T17:15:48Z

This PR continues #21095 and intersects with #21238. I've added volume mounts as a separate step and added PersistantVolumeClaim support.

There is a fundamental problem with how we pass the options through spark conf to fabric8. For each volume type and all possible volume options we would have to implement some custom code to map config values to fabric8 calls. This will result in big body of code we would have to support and means that Spark will always be somehow out of sync with k8s.

I think there needs to be a discussion on how to proceed correctly (eg use PodPreset instead)

Due to the complications of provisioning and managing actual resources this PR addresses only volume mounting of already present resources.

emptyDir support
Testing
Documentation
KubernetesVolumeUtils tests

foxish · 2018-05-08T08:00:42Z

jenkins, ok to test

SparkQA · 2018-05-08T08:04:42Z

Test build #90363 has finished for PR 21260 at commit f1ada4c.

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

SparkQA · 2018-05-08T08:15:53Z

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

SparkQA · 2018-05-08T08:21:02Z

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

liyinan926 · 2018-05-09T18:07:31Z

There is a fundamental problem with how we pass the options through spark conf to fabric8. For each volume type and all possible volume options we would have to implement some custom code to map config values to fabric8 calls. This will result in big body of code we would have to support and means that Spark will always be somehow out of sync with k8s.

This is indeed concerning, given that we don't yet support a lot of pod customization options yet, e.g., affinity and anti-affinity, security context, etc. Ideally, pod specs should be specified declaratively like in Deployment and StatefulSet, but Spark is configuration property based. The Spark Operator attempted to address this using initializers. But initializers are alpha and dangerous. Admission webhooks are an option, but again, they pose risks.

liyinan926 · 2018-05-11T16:57:02Z

@andrusha Can you remove coverage of #21238 and update the title? I will take a look once it's ready. You also need to rebase the PR.

andrusha · 2018-05-14T11:52:56Z

@liyinan926 sounds fair, it also pending tests, I'll add those in todo list for this PR and ping you once its done

er0sin · 2018-05-15T02:49:22Z

How does one configure a PV/PVC with this change?
spark.kubernetes.executor.volumes=pvName:containerPath ?

liyinan926 · 2018-05-15T05:16:37Z

@er0sin PVCs can be mounted similarly to the example below:

spark.kubernetes.driver.volumes.persistentVolumeClaim.pv1.mount.path=/mnt
spark.kubernetes.driver.volumes.persistentVolumeClaim.pv1.mount.readOnly=false
spark.kubernetes.driver.volumes.persistentVolumeClaim.pv1.options.claimName=clm-1

SparkQA · 2018-05-22T12:24:47Z

Test build #90956 has finished for PR 21260 at commit 0134239.

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

SparkQA · 2018-05-22T12:38:16Z

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

SparkQA · 2018-05-22T12:43:53Z

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

andrusha · 2018-05-22T12:58:40Z

@liyinan926 now hostPath, emptyDir and peristentVolumeClaim are supported. That's enough to cover most of the use-cases as PVC volumes can be cloud-specific, say AWS EFS. I've also added tests and documentation.

Would appreciate feedback on documentation, as we have to make clear which features are supported.

Following refactorings might be done:

Introduce KubernetesConf.empty so it would be simpler to add new specs and will results in less merge conflicts.
Encode KubernetesVolumeSpec.volumeType as ADT sealed trait KubernetesVolumeType so compiler exhaustiveness checks can be done.

SparkQA · 2018-05-22T12:59:55Z

Test build #90960 has finished for PR 21260 at commit dde6cf9.

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

andrusha · 2018-05-22T13:28:45Z

@AmplabJenkins failed due to github flakiness. Please retest.

SparkQA · 2018-05-22T13:34:33Z

Test build #90962 has finished for PR 21260 at commit 95d9312.

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

liyinan926 · 2018-05-22T22:53:51Z

@andrusha Is this ready for review?

andrusha · 2018-05-24T08:35:03Z

@liyinan926 yeap, ready for review. If you think it makes sense to make refactors I'm speaking of then I can add those too.

andrusha · 2018-05-24T13:05:59Z

...agers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala

+   * @param prefix the given property name prefix
+   * @return a Map storing with volume name as key and spec as value
+   */
+  def parseVolumesWithPrefix(


Tests are missing

SparkQA · 2018-05-24T16:29:37Z

Test build #91112 has finished for PR 21260 at commit de148e3.

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

SparkQA · 2018-05-24T16:44:42Z

Test build #91114 has finished for PR 21260 at commit f218d8a.

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

SparkQA · 2018-05-24T17:00:31Z

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

SparkQA · 2018-05-24T17:06:04Z

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

SparkQA · 2018-06-11T15:18:20Z

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

liyinan926 · 2018-06-11T16:17:06Z

...netes/core/src/main/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStep.scala

+import org.apache.spark.deploy.k8s._
+
+private[spark] class MountVolumesFeatureStep(
+  kubernetesConf: KubernetesConf[_ <: KubernetesRoleSpecificConf])


The class parameter line should be indented 4 spaces.

liyinan926 · 2018-06-11T16:26:33Z

...agers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala

+        val sizeLimitKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SIZE_LIMIT_KEY"
+        for {
+          medium <- options.getTry(mediumKey)
+          sizeLimit <- options.getTry(sizeLimitKey)


Both Medium and SizeLimit are optional for EmptyDirVolumeSource so users are not required to set them. What happens with options.getTry if users do not set the key?

Fixed.
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#emptydirvolumesource-v1-core I don't quite like that you have to pass "" and null to k8s, but it works.

liyinan926 · 2018-06-11T16:35:45Z

...agers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala

+
+      for {
+        path <- properties.getTry(pathKey)
+        readOnly <- properties.getTry(readOnlyKey)


ReadOnly is optional and defaults to false so users are not required to set it explicitly. Is this semantics captured by properties.getTry?

Made it optional with default false value. We basically ended up with two ways to parse spark options now, one is custom for volumes as there is just so many combinations and one which the rest of spark is using. It must be possible to refactor this somehow, maybe improving spark options parser itself.

SparkQA · 2018-06-11T18:25:33Z

Test build #91663 has finished for PR 21260 at commit d960e34.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-06-13T14:47:34Z

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

SparkQA · 2018-06-13T14:53:07Z

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

SparkQA · 2018-06-13T14:53:43Z

Test build #91775 has finished for PR 21260 at commit f714b8e.

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

SparkQA · 2018-06-15T09:37:57Z

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

SparkQA · 2018-06-15T09:42:58Z

Test build #91904 has finished for PR 21260 at commit 7433244.

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

SparkQA · 2018-06-15T09:43:53Z

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

andrusha · 2018-06-18T10:15:05Z

retest please

felixcheung · 2018-06-26T16:52:26Z

Jenkins, retest this please

SparkQA · 2018-06-26T17:08:37Z

Test build #92346 has finished for PR 21260 at commit 7433244.

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

SparkQA · 2018-07-03T10:43:03Z

Test build #92560 has finished for PR 21260 at commit 45eb477.

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

liyinan926

LGTM.

liyinan926 · 2018-07-06T05:49:32Z

@felixcheung @mccheah Can you take a look and merge this?

skonto · 2018-07-06T13:05:40Z

@felixcheung gentle ping, this is pretty useful.

felixcheung · 2018-07-06T15:34:47Z

Will review today.

baluchicken · 2018-07-07T08:07:04Z

Nice work, only one thing I would consider including a StorageClass name option for the PersistentVolumeClaim volume type which defaults to an empty string. Without that the PVC will always use the default StorageClass which may not exists in all scenarios. Thus the pod will remain in pending state indefinitely.

felixcheung · 2018-07-08T03:35:36Z

@skonto is it better to generalize the approach to match the one in https://issues.apache.org/jira/browse/SPARK-24434?

not sure if @mccheah @foxish @erikerlandson have any last thought

liyinan926 · 2018-07-08T04:15:42Z

@felixcheung This feature was discussed and this PR was started before https://issues.apache.org/jira/browse/SPARK-24434 was even brought up. Being able to mount commonly used types of volumes seems super useful for some users, so it might make sense to accept it while https://issues.apache.org/jira/browse/SPARK-24434 is still going through design review.

felixcheung · 2018-07-09T07:07:52Z

ok that makes sense. could we get some clear guidance in SPARK-24434 on how to decide what should be a conf and what should be in an external template file?

felixcheung

LGTM, would be great to add a complete example in running-on-kubernetes.md as a follow up
(will wait for 24hr, then merge)

liyinan926 · 2018-07-09T18:21:49Z

ok that makes sense. could we get some clear guidance in SPARK-24434 on how to decide what should be a conf and what should be in an external template file?

We talked about stop accepting any new config options for customizing the driver/executor pods. Moving forward, all new customization needs will be fulfilled by the solution to SPARK-24434.

felixcheung · 2018-07-11T05:58:56Z

merged to master

skonto · 2018-07-11T11:17:40Z

@felixcheung volumes will be supported by the pod template and so you will be able to do it without the conf options defined here. If both spark conf and template properties exist we have defined a precedence order in the design doc.

andrusha mentioned this pull request May 7, 2018

[SPARK-23529][K8s] Support mounting hostPath volumes #21095

Closed

liyinan926 mentioned this pull request May 9, 2018

Support for PVs, PVCs, and other K8s persistent volume support apache-spark-on-k8s/spark#627

Closed

mccheah mentioned this pull request May 9, 2018

[SPARK-24137][K8s] Mount local directories as empty dir volumes. #21238

Closed

andrusha force-pushed the k8s-vol branch from f1ada4c to 0134239 Compare May 22, 2018 12:19

andrusha changed the title ~~[SPARK-23529][K8s] Support mounting hostPath volumes~~ [SPARK-23529][K8s] Support mounting volumes May 22, 2018

andrusha commented May 24, 2018

View reviewed changes

andrusha force-pushed the k8s-vol branch 2 times, most recently from f87bf3f to f218d8a Compare May 24, 2018 16:40

liyinan926 reviewed Jun 11, 2018

View reviewed changes

Make mount.readOnly and emptyDir params optional

f714b8e

Fix indentation for class parameters

7433244

Merge remote-tracking branch 'apache/master' into k8s-vol

45eb477

liyinan926 approved these changes Jul 6, 2018

View reviewed changes

skonto mentioned this pull request Jul 6, 2018

[SPARK-23980][K8S] Resilient Spark driver on Kubernetes #21067

Closed

felixcheung approved these changes Jul 9, 2018

View reviewed changes

asfgit closed this in 5ff1b9b Jul 11, 2018

[SPARK-23529][K8s] Support mounting volumes #21260

[SPARK-23529][K8s] Support mounting volumes #21260

Conversation

andrusha commented May 7, 2018 • edited Loading

foxish commented May 8, 2018

SparkQA commented May 8, 2018

SparkQA commented May 8, 2018

SparkQA commented May 8, 2018

liyinan926 commented May 9, 2018

liyinan926 commented May 11, 2018

andrusha commented May 14, 2018

er0sin commented May 15, 2018

liyinan926 commented May 15, 2018

SparkQA commented May 22, 2018

SparkQA commented May 22, 2018

SparkQA commented May 22, 2018

andrusha commented May 22, 2018

SparkQA commented May 22, 2018

andrusha commented May 22, 2018

SparkQA commented May 22, 2018

liyinan926 commented May 22, 2018

andrusha commented May 24, 2018

andrusha May 24, 2018

Choose a reason for hiding this comment

SparkQA commented May 24, 2018

SparkQA commented May 24, 2018

SparkQA commented May 24, 2018

SparkQA commented May 24, 2018

SparkQA commented Jun 11, 2018

liyinan926 Jun 11, 2018

Choose a reason for hiding this comment

liyinan926 Jun 11, 2018

Choose a reason for hiding this comment

andrusha Jun 15, 2018

Choose a reason for hiding this comment

liyinan926 Jun 11, 2018

Choose a reason for hiding this comment

andrusha Jun 15, 2018

Choose a reason for hiding this comment

SparkQA commented Jun 11, 2018

SparkQA commented Jun 13, 2018

SparkQA commented Jun 13, 2018

SparkQA commented Jun 13, 2018

SparkQA commented Jun 15, 2018

SparkQA commented Jun 15, 2018

SparkQA commented Jun 15, 2018

andrusha commented Jun 18, 2018

felixcheung commented Jun 26, 2018

SparkQA commented Jun 26, 2018

SparkQA commented Jul 3, 2018

liyinan926 left a comment

Choose a reason for hiding this comment

liyinan926 commented Jul 6, 2018

skonto commented Jul 6, 2018

felixcheung commented Jul 6, 2018 via email • edited Loading

baluchicken commented Jul 7, 2018

felixcheung commented Jul 8, 2018 • edited Loading

liyinan926 commented Jul 8, 2018

felixcheung commented Jul 9, 2018

felixcheung left a comment

Choose a reason for hiding this comment

liyinan926 commented Jul 9, 2018

felixcheung commented Jul 11, 2018

skonto commented Jul 11, 2018 • edited Loading

andrusha commented May 7, 2018 •

edited

Loading

felixcheung commented Jul 6, 2018 via email •

edited

Loading

felixcheung commented Jul 8, 2018 •

edited

Loading

skonto commented Jul 11, 2018 •

edited

Loading