[SPARK-28042][K8S] Support using volume mount as local storage #24879

chenjunjiedada · 2019-06-15T13:14:07Z

What changes were proposed in this pull request?

This pr is used to support using hostpath/PV volume mounts as local storage. In KubernetesExecutorBuilder.scala, the LocalDrisFeatureStep is built before MountVolumesFeatureStep which means we cannot use any volumes mount later. This pr adjust the order of feature building steps which moves localDirsFeature at last so that we can check if directories in SPARK_LOCAL_DIRS are set to volumes mounted such as hostPath, PV, or others.

How was this patch tested?

Unit tests

dongjoon-hyun · 2019-06-15T17:33:02Z

Thank you for making a PR, @chenjunjiedada !

dongjoon-hyun · 2019-06-15T17:33:06Z

ok to test

SparkQA · 2019-06-15T17:45:36Z

Test build #106542 has finished for PR 24879 at commit 6fca505.

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

SparkQA · 2019-06-15T17:50:27Z

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

...tes/core/src/test/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStepSuite.scala

SparkQA · 2019-06-15T18:04:15Z

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

felixcheung

could we add some documentation?

felixcheung · 2019-06-15T18:08:56Z

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

      new KerberosConfDriverFeatureStep(conf),
-      new PodTemplateConfigMapStep(conf))
+      new PodTemplateConfigMapStep(conf),
+      new LocalDirsFeatureStep(conf))


why not move this right after new MountVolumesFeatureStep(conf)?

I think some volume mounts maybe setup in PodTemplateConfigMapStep, so I move to last to ensure that.

Yep, since the intent is for users to provide custom local dir volumes via either mount volumes or pod templates it needs to appear after both

Either is fine, just wanted to point out that the pod template is initialized before any step is executed (i.e. PodTemplateConfigMapStep is not where the template is loaded).

felixcheung · 2019-06-15T18:11:37Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

+        hasVolumeMount(pod, localDirVolume.getName, localDirPath) match {
+          case true =>
+            pod.container.getVolumeMounts().asScala
+            .find(m => m.getName.equals(localDirVolume.getName)


please see other lines for indentation

ok, will check and run scalafmt.

felixcheung · 2019-06-15T18:13:36Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

-          .endEmptyDir()
-          .build()
+        val name = s"spark-local-dir-${index + 1}"
+        hasVolume(pod, name) match {


not a super big deal, but hasVolume is basically the same logic pod.pod.getSpec().getVolumes().asScala.exists as pod.pod.getSpec.getVolumes().asScala.find

I think you can refactor this to avoid scanning multiple times.

Updated in the latest commit.

SparkQA · 2019-06-16T01:51:20Z

Test build #106543 has finished for PR 24879 at commit ef66c87.

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

SparkQA · 2019-06-16T01:56:06Z

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

SparkQA · 2019-06-16T02:11:25Z

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

SparkQA · 2019-06-16T02:41:51Z

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

SparkQA · 2019-06-16T02:44:17Z

Test build #106544 has finished for PR 24879 at commit 5610fe4.

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

SparkQA · 2019-06-16T02:57:37Z

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

chenjunjiedada · 2019-06-18T06:26:15Z

Hi @dongjoon-hyun and @felixcheung,

I updated the code in the latest commits, could you please take a look? Thanks.

felixcheung · 2019-06-22T08:16:17Z

@mccheah

liyinan926

LGTM

erikerlandson · 2019-06-24T19:35:16Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

-          .build()
+        val name = s"spark-local-dir-${index + 1}"
+        findVolume(pod, name) match {
+          case Some(volume) => volume


integration tests should exercise both cases (found the volume or created it)

I agree with you, there is an existing unit test for creating a local directory via emptyDir, this PR adds a unit test for the case of found volume. Both cases should already be covered. Is that ok?

mccheah · 2019-06-25T00:19:58Z

So my impression was that hostPath volumes are unsafe because they break the boundary of isolation between the container and the underlying machine, and the isolation between containers. I also was under the impression that containers have to be run as root to support hostPath volumes. Because of these factors I would be hesitant to advertise supporting this as a first-class feature in the Spark on Kubernetes integration. If a user so desired they can use the existing pod template feature to break glass, and this seems more like a break glass feature in my estimation.

chenjunjiedada · 2019-06-25T02:34:53Z

Good point @mccheah,

First, I'd like to explain that this patch is not only for hostPath volume but also it suits for other volumes such as PV, etc. it tries to adjust feature build order to adopt volumes which already be supported as first-class features.

Second, the hostPath volume can be written by a non-root user if we change the file permission according to the description here.

Lastly, I think users that care about the performance should want this since even they can define volumes inside podTemplate, the volumes are not utilized, I have run spark-sql-perf on spark on Kubernetes with 1T scale, with the patch, most of the shuffle bounded queries can improve a lot, especially for q17, q25, q29, when using 4 disks as local storage instead of emptyDir, the query time improve more than 10X.

In summary, this patch is just to help users easily leverage what they have to improve performance.

chenjunjiedada · 2019-06-25T02:40:35Z

@vanzin , could you please help to have a look?

francoisfernando · 2019-07-02T22:58:36Z

@mccheah
I have hit a roadblock due to this issue trying to set the spark.local.dir to a something other than emptyDir volumes. For example following configuration throws an exception when executor pods are provisioned.

--conf spark.local.dir=/spark_tmp \
--conf spark.kubernetes.executor.volumes.hostPath.spaklocal.mount.path=/spark_tmp \
--conf spark.kubernetes.executor.volumes.hostPath.spaklocal.mount.readOnly=false \
--conf spark.kubernetes.executor.volumes.hostPath.spaklocal.options.path=/tmp \

chenjunjiedada · 2019-07-03T06:13:34Z

Hi @felixcheung @mccheah , Do you think it is ready? thanks.

dongjoon-hyun · 2019-07-04T02:51:34Z

Retest this please.

shaneknapp · 2019-07-24T20:22:35Z

test this please

SparkQA · 2019-07-24T20:31:38Z

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

SparkQA · 2019-07-24T20:31:49Z

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

SparkQA · 2019-07-24T20:39:46Z

Test build #108131 has finished for PR 24879 at commit 9392dad.

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

SparkQA · 2019-07-24T20:43:52Z

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

SparkQA · 2019-07-24T20:58:01Z

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

SparkQA · 2019-07-25T03:39:05Z

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

SparkQA · 2019-07-25T03:43:22Z

Test build #108149 has finished for PR 24879 at commit 6e5fcf6.

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

SparkQA · 2019-07-25T03:53:14Z

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

vanzin · 2019-07-25T16:39:54Z

docs/running-on-kubernetes.md

+```
+
+
+If none volume is set as local storage, Spark uses temporary scratch space to spill data to disk during shuffles and other operations. When using Kubernetes as the resource manager the pods will be created with an [emptyDir](https://kubernetes.io/docs/concepts/storage/volumes/#emptydir) volume mounted for each directory listed in `SPARK_LOCAL_DIRS`.  If no directories are explicitly specified then a default directory is created and configured appropriately.


"If no volume is set..."

Should mention the config (spark.local.dir) which is preferred over the env variable.

vanzin · 2019-07-25T16:40:23Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

 import java.util.UUID

-import io.fabric8.kubernetes.api.model.{ContainerBuilder, HasMetadata, PodBuilder, VolumeBuilder, VolumeMountBuilder}
+import collection.JavaConverters._


import scala....

vanzin · 2019-07-25T16:45:21Z

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

      new KerberosConfDriverFeatureStep(conf),
-      new PodTemplateConfigMapStep(conf))
+      new PodTemplateConfigMapStep(conf),
+      new LocalDirsFeatureStep(conf))


Either is fine, just wanted to point out that the pod template is initialized before any step is executed (i.e. PodTemplateConfigMapStep is not where the template is loaded).

vanzin · 2019-07-25T16:50:04Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

+    var localDirVolumeMounts : Seq[VolumeMount] = Seq()
+
+    if (localDirs.isEmpty) {
+      localDirs = resolvedLocalDirs.toSeq


Get rid of resolvedLocalDirs and move that statement here instead.

vanzin · 2019-07-25T16:51:47Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

+
+  def findLocalDirVolumeMount(pod: SparkPod): Seq[String] = {
+    val localDirVolumes = pod.pod.getSpec.getVolumes.asScala
+      .filter(v => v.getName.startsWith("spark-local-dir-"))


nit: either:

.filter { v => ... }

or

.filter(_.getName()...)

Also in a few places below.

vanzin · 2019-07-25T16:52:23Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

+    val localDirVolumes = pod.pod.getSpec.getVolumes.asScala
+      .filter(v => v.getName.startsWith("spark-local-dir-"))
+
+    localDirVolumes.map { volume => pod.container.getVolumeMounts.asScala


Move pod.container... to the next line.

vanzin · 2019-07-25T16:52:50Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

+      .filter(v => v.getName.startsWith("spark-local-dir-"))
+
+    localDirVolumes.map { volume => pod.container.getVolumeMounts.asScala
+      .find(m => m.getName.equals(volume.getName)) match {


You can use == for strings in scala.

vanzin · 2019-07-25T16:53:32Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

+          case Some(m) => m.getMountPath
+          case _ => ""
+      }
+    }.filter(s => s.length > 0)


You can use localDirVolumes.flatMap and avoid this filter.

vanzin · 2019-07-25T16:54:57Z

...tes/core/src/test/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStepSuite.scala

+    val configuredPod = mountVolumeStep.configurePod(SparkPod.initialPod())
+
+    val sparkConf = new SparkConfWithEnv(Map())
+    val localDirConf = KubernetesTestConf.createDriverConf(sparkConf)


You can use the same KubernetesConf for all steps right? That's how it works when spark-submit is run.

That is the idea, to keep building out one conf. (There were some early proposals around making these stages "more functional" but those were considered less broadly familiar and less spark idiomatic)

vanzin · 2019-07-25T16:56:26Z

...tes/core/src/test/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStepSuite.scala


 import org.apache.spark.{SparkConf, SparkFunSuite}
-import org.apache.spark.deploy.k8s.{KubernetesTestConf, SparkPod}
+import org.apache.spark.deploy.k8s.{KubernetesHostPathVolumeConf, KubernetesTestConf, KubernetesVolumeSpec, SparkPod}


Use the wildcard when the import line gets too long.

erikerlandson · 2019-07-25T18:37:33Z

Just to make sure I understand the rationale - we want the option to automatically create local directories "spark-local-dir-xxx" as a simplified UX (why mess around with a second channel of config when a working dir can be created automatically from the information in the first?)

vanzin · 2019-07-25T18:43:26Z

we want the option to automatically create local directories "spark-local-dir-xxx" as a simplified UX

Right. That's also more similar to other backends, where the local dirs are defined by the cluster manager themselves and users don't have to mess with the Spark configuration.

(Except here they have to, but it can be encapsulated in the pod template.)

erikerlandson · 2019-07-25T20:30:58Z

@vanzin thanks, I agree that makes sense

SparkQA · 2019-07-26T03:24:19Z

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

SparkQA · 2019-07-26T03:26:32Z

Test build #108190 has finished for PR 24879 at commit 2abb8e9.

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

SparkQA · 2019-07-26T03:41:22Z

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

vanzin

Just some small nits.

vanzin · 2019-07-26T20:22:56Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala


-import io.fabric8.kubernetes.api.model.{ContainerBuilder, HasMetadata, PodBuilder, VolumeBuilder, VolumeMountBuilder}
+import io.fabric8.kubernetes.api.model._
+import scala.collection.JavaConverters._


Hmm, style checker should have complained about this (scala imports should be separate from others).

vanzin · 2019-07-26T20:26:05Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

  }
+
+  def findLocalDirVolumeMount(pod: SparkPod): Seq[String] = {
+    val localDirVolumes = pod.pod.getSpec.getVolumes.asScala


Hmm... do you need to list the volumes at all? Can't you just look at the mounts (since they already have the volume name)?

vanzin · 2019-07-26T20:26:15Z

...bernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStep.scala

    SparkPod(podWithLocalDirVolumes, containerWithLocalDirVolumeMounts)
  }
+
+  def findLocalDirVolumeMount(pod: SparkPod): Seq[String] = {


Mount => Mounts

SparkQA · 2019-07-28T11:20:27Z

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

SparkQA · 2019-07-28T11:24:17Z

Test build #108273 has finished for PR 24879 at commit c29dd7a.

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

SparkQA · 2019-07-28T11:37:25Z

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

vanzin · 2019-07-29T17:43:52Z

Merging to master.

This pr is used to support using hostpath/PV volume mounts as local storage. In KubernetesExecutorBuilder.scala, the LocalDrisFeatureStep is built before MountVolumesFeatureStep which means we cannot use any volumes mount later. This pr adjust the order of feature building steps which moves localDirsFeature at last so that we can check if directories in SPARK_LOCAL_DIRS are set to volumes mounted such as hostPath, PV, or others. Unit tests Closes apache#24879 from chenjunjiedada/SPARK-28042. Lead-authored-by: Junjie Chen <[email protected]> Co-authored-by: Junjie Chen <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]>

[SPARK-28042][K8S] Support using hostpath volume mount as local storage

6fca505

dongjoon-hyun added the KUBERNETES label Jun 15, 2019

dongjoon-hyun reviewed Jun 15, 2019

View reviewed changes

...tes/core/src/test/scala/org/apache/spark/deploy/k8s/features/LocalDirsFeatureStepSuite.scala Show resolved Hide resolved

felixcheung reviewed Jun 15, 2019

View reviewed changes

SPARK-28042: fix unit test and address comments

ef66c87

SPARK-28042: refactor checks according to comments

5610fe4

felixcheung requested review from erikerlandson, felixcheung and vanzin June 22, 2019 08:12

liyinan926 approved these changes Jun 24, 2019

View reviewed changes

erikerlandson reviewed Jun 24, 2019

View reviewed changes

Don't set configuration when specific volume is set

6e5fcf6

vanzin reviewed Jul 25, 2019

View reviewed changes

address comments

2abb8e9

vanzin reviewed Jul 26, 2019

View reviewed changes

fix coding style

c29dd7a

vanzin closed this in 780d176 Jul 29, 2019

Jeffwan mentioned this pull request Mar 4, 2020

Duplicate value: "spark-local-dir-1" kubeflow/spark-operator#828

Closed

hopper-signifyd mentioned this pull request Jul 2, 2020

[SPARK-25262][K8S][2.4] Allow SPARK_LOCAL_DIRS to be tmpfs backed on K8S #28985

Closed

		```


		If none volume is set as local storage, Spark uses temporary scratch space to spill data to disk during shuffles and other operations. When using Kubernetes as the resource manager the pods will be created with an [emptyDir](https://kubernetes.io/docs/concepts/storage/volumes/#emptydir) volume mounted for each directory listed in `SPARK_LOCAL_DIRS`. If no directories are explicitly specified then a default directory is created and configured appropriately.

[SPARK-28042][K8S] Support using volume mount as local storage #24879

[SPARK-28042][K8S] Support using volume mount as local storage #24879

Uh oh!

Conversation

chenjunjiedada commented Jun 15, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Jun 15, 2019

Uh oh!

dongjoon-hyun commented Jun 15, 2019

Uh oh!

SparkQA commented Jun 15, 2019

Uh oh!

SparkQA commented Jun 15, 2019

Uh oh!

Uh oh!

SparkQA commented Jun 15, 2019

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 16, 2019

Uh oh!

SparkQA commented Jun 16, 2019

Uh oh!

SparkQA commented Jun 16, 2019

Uh oh!

SparkQA commented Jun 16, 2019

Uh oh!

SparkQA commented Jun 16, 2019

Uh oh!

SparkQA commented Jun 16, 2019

Uh oh!

chenjunjiedada commented Jun 18, 2019

Uh oh!

felixcheung commented Jun 22, 2019

Uh oh!

liyinan926 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah commented Jun 25, 2019

Uh oh!

chenjunjiedada commented Jun 25, 2019

Uh oh!

chenjunjiedada commented Jun 25, 2019

Uh oh!

francoisfernando commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenjunjiedada commented Jul 3, 2019

Uh oh!

dongjoon-hyun commented Jul 4, 2019

Uh oh!

shaneknapp commented Jul 24, 2019

Uh oh!

SparkQA commented Jul 24, 2019

Uh oh!

SparkQA commented Jul 24, 2019

Uh oh!

francoisfernando commented Jul 2, 2019 •

edited

Loading