[SPARK-33063][K8S] Improve error message for insufficient K8s volume confs by Gschiavon · Pull Request #29941 · apache/spark

Gschiavon · 2020-10-04T09:06:21Z

What changes were proposed in this pull request?

Provide error handling when creating kubernetes volumes. Right now they keys are expected to be there and if not it fails with a key not found error, but not knowing why do you need that key.

Also I renamed some tests that didn't indicate the kind of kubernetes volume

Why are the changes needed?

Easier for the users to understand why spark-submit command is failing if not providing they right kubernetes volumes properties.

Does this PR introduce any user-facing change?

No

How was this patch tested?

It was tested with the current tests plus added one more.

Jira ticket

dongjoon-hyun · 2020-10-04T17:31:51Z

ok to test

dongjoon-hyun · 2020-10-04T17:32:07Z

Thank you for your contribution, @Gschiavon .

SparkQA · 2020-10-04T17:43:41Z

Test build #129391 has finished for PR 29941 at commit f54a7a5.

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

SparkQA · 2020-10-04T18:14:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33998/

SparkQA · 2020-10-04T18:30:51Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33998/

dongjoon-hyun · 2020-10-04T21:39:58Z

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

+


Could you remove this addition? The existing one line looks okay to me. (https://github.com/databricks/scala-style-guide#blank-lines-vertical-whitespace)

Use one or two blank line(s) to separate class or object definitions.

yes! just waiting for the checks to pass. Removing now

dongjoon-hyun · 2020-10-04T21:55:08Z

+    val sparkConf = new SparkConf(false)
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.path", "/path")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.readOnly", "true")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.options.clamName", "claimeName")


Although I understand your intention to use clamName as a typo, it would be great if you use more explicit test case. This is easily misleading the reviewer. Please remove this line completely. That is more clear.

I agree! I did it to follow the "style" of the other tests, but I'll remove it.

dongjoon-hyun · 2020-10-04T22:07:13Z

+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +


Can we use pathKey instead of this $KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY?

I didnt use path key because it includes the volume name given by the user and I wasn't sure about this.

I've added it.

dongjoon-hyun

@Gschiavon . This PR has some repeated pattern to provide an informative error message. I'd like to recommend you to have a helper function.

  private def verifyOptionKey(options: Map[String, String], key: String, msg: String): Unit = {
    if (!options.isDefinedAt(key)) {
      throw new NoSuchElementException(key + s"is required for $msg")
    }
  }

   private def parseVolumeSpecificConf(
       options: Map[String, String],
       volumeType: String,
@@ -67,6 +73,7 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
+        verifyOptionKey(options, pathKey, KUBERNETES_VOLUMES_HOSTPATH_TYPE)
         KubernetesHostPathVolumeConf(options(pathKey))
 
       case KUBERNETES_VOLUMES_PVC_TYPE =>
@@ -74,6 +81,7 @@ private[spark] object KubernetesVolumeUtils {
         val storageClassKey =
           s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_STORAGE_CLASS_KEY"
         val sizeLimitKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SIZE_LIMIT_KEY"
+        verifyOptionKey(options, claimNameKey, KUBERNETES_VOLUMES_PVC_TYPE)
         KubernetesPVCVolumeConf(
           options(claimNameKey),
           options.get(storageClassKey),
@@ -87,6 +95,8 @@ private[spark] object KubernetesVolumeUtils {
       case KUBERNETES_VOLUMES_NFS_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
         val serverKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SERVER_KEY"
+        verifyOptionKey(options, pathKey, KUBERNETES_VOLUMES_NFS_TYPE)
+        verifyOptionKey(options, serverKey, KUBERNETES_VOLUMES_NFS_TYPE)
         KubernetesNFSVolumeConf(
           options(pathKey),
           options(serverKey))

SparkQA · 2020-10-05T05:29:48Z

Test build #129398 has finished for PR 29941 at commit 8be9381.

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

SparkQA · 2020-10-05T05:57:41Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34005/

SparkQA · 2020-10-05T06:14:11Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34005/

dongjoon-hyun

+1, LGTM. Thank you so much, @Gschiavon .
Merged to master.

dongjoon-hyun · 2020-10-05T16:06:26Z

Thank you, @Gschiavon . SPARK-33063 is assigned to you.

Gschiavon · 2020-10-05T17:05:34Z

Thanks @dongjoon-hyun !

provide error handling when creating k8s volumes

f54a7a5

dongjoon-hyun changed the title ~~[SPARK-33063] [KUBERNETES] provide error handling when creating k8s volumes~~ [SPARK-33063][K8S] provide error handling when creating k8s volumes Oct 4, 2020