[SPARK-24137][K8s] Mount local directories as empty dir volumes. by mccheah · Pull Request #21238 · apache/spark

mccheah · 2018-05-04T21:43:04Z

What changes were proposed in this pull request?

Drastically improves performance and won't cause Spark applications to fail because they write too much data to the Docker image's specific file system. The file system's directories that back emptydir volumes are generally larger and more performant.

How was this patch tested?

Has been in use via the prototype version of Kubernetes support, but lost in the transition to here.

Dramatically improves performance and won't cause Spark applications to fail because they write too much data to the Docker image's specific file system. The file system's directories that back emptydir volumes are generally larger and more performant.

mccheah · 2018-05-04T21:43:29Z

@foxish @liyinan926 please take a look, thanks!

SparkQA · 2018-05-04T21:49:33Z

Test build #90217 has finished for PR 21238 at commit 736674b.

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

SparkQA · 2018-05-04T23:01:26Z

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

SparkQA · 2018-05-04T23:12:31Z

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

SparkQA · 2018-05-05T02:55:34Z

Test build #90225 has finished for PR 21238 at commit f68fcd8.

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

andrusha · 2018-05-07T13:53:48Z

Seems like it addresses similar problem to #21095. It might be worth investigating how to unify both.

mccheah · 2018-05-09T16:38:52Z

@andrusha I don't think it's entirely analogous - for the simple reason that the hostPath volumes PR doesn't take into account SPARK_LOCAL_DIRS. That environment variable is used to determine the right place for shuffle files. We want to strictly tie that setting to emptydir volumes - and moreover this should be done transparently for the user, i.e. the user shouldn't have to know to provision these volumes to get better performance.

mccheah · 2018-05-09T16:41:53Z

Also #21260 currently only supports hostPath and PVCs but you definitely want emptyDir for isolation (though that looks like a trivial enough change).

foxish · 2018-05-09T17:32:00Z

 if (contains("spark.local.dir")) {
 val msg = "In Spark 1.0 and later spark.local.dir will be overridden by the value set by " +
- "the cluster manager (via SPARK_LOCAL_DIRS in mesos/standalone and LOCAL_DIRS in YARN)."
+ "the cluster manager (via SPARK_LOCAL_DIRS in mesos/standalone/kubernetes and LOCAL_DIRS" +


oops, I deleted a comment here accidentally. @rxin said that we could remove this warning about Spark 1.0.

erikerlandson · 2018-05-09T17:45:01Z

I agree with @mcheah that the potential code reuse is small. Keeping this as a separate pod construction step, decoupled from the user-exposed step, is cleaner.

liyinan926 · 2018-05-09T17:48:09Z

+ val localDirVolumes = resolvedLocalDirs
+ .zipWithIndex
+ .map {
+ case (localDir, index) =>


I think the convention is to put case on the same line as map {.

liyinan926 · 2018-05-09T17:49:22Z

+ .map {
+ case (localDir, index) =>
+ new VolumeBuilder()
+ .withName(s"spark-local-dir-${index + 1}-${Paths.get(localDir).getFileName.toString}")


Do you really need to include the actual path in the volume name? I think spark-local-dir-${index + 1} is sufficient.

liyinan926 · 2018-05-09T17:49:46Z

+ val localDirVolumeMounts = localDirVolumes
+ .zip(resolvedLocalDirs)
+ .map {
+ case (localDirVolume, localDirPath) =>


mccheah · 2018-05-10T00:56:17Z

@rxin @liyinan926 @foxish addressed comments.

SparkQA · 2018-05-10T01:17:24Z

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

SparkQA · 2018-05-10T01:22:29Z

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

liyinan926

LGTM.

SparkQA · 2018-05-10T04:29:03Z

Test build #90431 has finished for PR 21238 at commit aebdb68.

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

SparkQA · 2018-05-10T05:24:56Z

Test build #90432 has finished for PR 21238 at commit fa095cd.

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

mccheah · 2018-05-10T17:58:56Z

Requesting sign off and merge from @foxish

foxish · 2018-05-10T18:32:05Z

LGTM. Merging to master.
Thanks @mccheah

liyinan926 · 2018-05-10T18:41:02Z

@foxish @mccheah should we also merge this to branch-2.3? I think we should target this for 2.3.1.

foxish · 2018-05-10T18:48:43Z

Maintenance releases most often have fixes for stability. We could maybe backport this since it's not a new feature but an omission from before. If it is going to be some effort, thanks to all the refactors that went in so far, we should think twice about whether we need to.

foxish · 2018-05-10T18:59:31Z

@mccheah, wdyt? I just haven't heard from any users here of 2.3 - if you think it's useful for 2.3.1 and low risk, then please feel free to propose a cherrypick.

mccheah · 2018-05-10T19:40:19Z

I think we can afford to hold off here.

mccheah · 2018-05-10T19:44:14Z

What would make this difficult to backport is the fact that this patch was built on top of the big refactor PR that only went in after 2.3. So we'd need to rewrite this with the old architecture which is a non-trivial effort.

foxish · 2018-05-10T19:44:28Z

SG. @liyinan926, let's revisit this if we hear from 2.3 users.

liyinan926 · 2018-05-10T19:51:02Z

Makes sense to me.

Drastically improves performance and won't cause Spark applications to fail because they write too much data to the Docker image's specific file system. The file system's directories that back emptydir volumes are generally larger and more performant. Has been in use via the prototype version of Kubernetes support, but lost in the transition to here. Author: mcheah <mcheah@palantir.com> Closesapache#21238 from mccheah/mount-local-dirs.

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. ---- - [x] emptyDir support - [x] Testing - [x] Documentation - [x] KubernetesVolumeUtils tests Author: Andrew Korzhuev <andrew.korzhuev@klarna.com> Author: madanadit <adit@alluxio.com> Closes#21260 from andrusha/k8s-vol.

mccheah changed the title ~~[SPARK-24137] Mount local directories as empty dir volumes.~~[SPARK-24137][K8s] Mount local directories as empty dir volumes.May 4, 2018

Fix scalastyle
f68fcd8

andrusha mentioned this pull request May 7, 2018
[SPARK-23529][K8s] Support mounting volumes #21260
Closed
4 tasks

liyinan926 mentioned this pull request May 7, 2018
[SPARK-23529][K8s] Support mounting hostPath volumes #21095
Closed

apache deleted a comment from rxin May 9, 2018

foxish reviewed May 9, 2018
View reviewed changes

liyinan926 reviewed May 9, 2018
View reviewed changes

mccheah added 2 commits May 9, 2018 17:53

Fix style
904435c

Fix test
aebdb68

FIx indentation
fa095cd

liyinan926 approved these changes May 10, 2018
View reviewed changes

asfgit closed this in 6282fc6May 10, 2018

mccheah mentioned this pull request May 10, 2018
[SPARK-24137][K8S] Mount local directories as empty dir volumes. palantir/spark#372
Merged

robert3005 deleted the mount-local-dirs branch August 11, 2018 05:10

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

mccheah commented May 4, 2018

Uh oh!

SparkQA commented May 4, 2018

Uh oh!

SparkQA commented May 4, 2018

Uh oh!

SparkQA commented May 4, 2018

Uh oh!

SparkQA commented May 5, 2018

Uh oh!

andrusha commented May 7, 2018

Uh oh!

mccheah commented May 9, 2018

Uh oh!

mccheah commented May 9, 2018

Uh oh!

foxishMay 9, 2018

Choose a reason for hiding this comment

Uh oh!

erikerlandson commented May 9, 2018

Uh oh!

liyinan926May 9, 2018

Choose a reason for hiding this comment

Uh oh!

liyinan926May 9, 2018

Choose a reason for hiding this comment

Uh oh!

liyinan926May 9, 2018

Choose a reason for hiding this comment

Uh oh!

mccheah commented May 10, 2018

Uh oh!

SparkQA commented May 10, 2018

Uh oh!

SparkQA commented May 10, 2018

Uh oh!

liyinan926 left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 10, 2018

Uh oh!

SparkQA commented May 10, 2018

Uh oh!

mccheah commented May 10, 2018

Uh oh!

foxish commented May 10, 2018

Uh oh!

liyinan926 commented May 10, 2018

Uh oh!

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

Uh oh!

Uh oh!

foxish commented May 10, 2018

Uh oh!

mccheah commented May 10, 2018

Uh oh!

mccheah commented May 10, 2018

Uh oh!

foxish commented May 10, 2018

Uh oh!

liyinan926 commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mccheah commented May 4, 2018•
edited
Loading

foxish commented May 10, 2018•
edited
Loading