[SPARK-25876][k8s] Simplify kubernetes configuration types. #22959

vanzin · 2018-11-06T18:26:20Z

There are a few issues with the current configuration types used in
the kubernetes backend:

they use type parameters for role-specific specialization, which makes
type signatures really noisy throughout the code base.
they break encapsulation by forcing the code that creates the config
object to remove the configuration from SparkConf before creating the
k8s-specific wrapper.
they don't provide an easy way for tests to have default values for
fields they do not use.

This change fixes those problems by:

creating a base config type with role-specific specialization using
inheritance
encapsulating the logic of parsing SparkConf into k8s-specific views
inside the k8s config classes
providing some helper code for tests to easily override just the part
of the configs they want.

Most of the change relates to the above, especially cleaning up the
tests. While doing that, I also made some smaller changes elsewhere:

removed unnecessary type parameters in KubernetesVolumeSpec
simplified the error detection logic in KubernetesVolumeUtils; all
the call sites would just throw the first exception collected by
that class, since they all called "get" on the "Try" object. Now
the unnecessary wrapping is gone and the exception is just thrown
where it occurs.
removed a lot of unnecessary mocking from tests.
changed the kerberos-related code so that less logic needs to live
in the driver builder. In spirit it should be part of the upcoming
work in this series of cleanups, but it made parts of this change
simpler.

Tested with existing unit tests and integration tests.

There are a few issues with the current configuration types used in the kubernetes backend: - they use type parameters for role-specific specialization, which makes type signatures really noisy throughout the code base. - they break encapsulation by forcing the code that creates the config object to remove the configuration from SparkConf before creating the k8s-specific wrapper. - they don't provide an easy way for tests to have default values for fields they do not use. This change fixes those problems by: - creating a base config type with role-specific specialization using inheritance - encapsulating the logic of parsing SparkConf into k8s-specific views inside the k8s config classes - providing some helper code for tests to easily override just the part of the configs they want. Most of the change relates to the above, especially cleaning up the tests. While doing that, I also madke some smaller changes elsewhere: - removed unnecessary type parameters in KubernetesVolumeSpec - simplified the error detection logic in KubernetesVolumeUtils; all the call sites would just throw the first exception collected by that class, since they all called "get" on the "Try" object. Now the unnecessary wrapping is gone and the exception is just thrown where it occurs. - removed a lot of unnecessary mocking from tests. - changed the kerberos-related code so that less logic needs to live in the driver builder. In spirit it should be part of the upcoming work in this series of cleanups, but it made parts of this change simpler. Tested with existing unit tests and integration tests.

SparkQA · 2018-11-06T18:40:30Z

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

SparkQA · 2018-11-06T18:47:07Z

Test build #98526 has finished for PR 22959 at commit ea0f8bc.

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

SparkQA · 2018-11-06T18:51:07Z

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

rvesse · 2018-11-07T18:01:24Z

First glance this looks like a lot of nice simplification, will take a proper look over this tomorrow

rvesse

LGTM

vanzin · 2018-11-08T20:25:45Z

Thanks Rob. @mccheah @liyinan926

liyinan926 · 2018-11-19T20:40:37Z

...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverCommandFeatureStep.scala

@@ -85,7 +83,7 @@ private[spark] class DriverCommandFeatureStep(conf: KubernetesConf[KubernetesDri
    val pythonEnvs =
      Seq(new EnvVarBuilder()
          .withName(ENV_PYSPARK_MAJOR_PYTHON_VERSION)
-          .withValue(conf.sparkConf.get(PYSPARK_MAJOR_PYTHON_VERSION))
+          .withValue(driverConf.sparkConf.get(PYSPARK_MAJOR_PYTHON_VERSION))


Looks like you can simply do driverConf.get.

liyinan926 · 2018-11-19T20:40:44Z

...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverCommandFeatureStep.scala

@@ -124,7 +122,7 @@ private[spark] class DriverCommandFeatureStep(conf: KubernetesConf[KubernetesDri
  }

  private def mergeFileList(key: String, filesToAdd: Seq[String]): Map[String, String] = {
-    val existing = Utils.stringToSeq(conf.sparkConf.get(key, ""))
+    val existing = Utils.stringToSeq(driverConf.sparkConf.get(key, ""))


liyinan926 · 2018-11-19T20:42:46Z

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

  extends KubernetesFeatureConfigStep with Logging {

  override def configurePod(pod: SparkPod): SparkPod = {
-    val sparkConf = kubernetesConf.sparkConf
-    val hadoopConfDirCMapName = sparkConf.getOption(HADOOP_CONFIG_MAP_NAME)
+    val hadoopConfDirCMapName = conf.sparkConf.getOption(HADOOP_CONFIG_MAP_NAME)


Looks like you can do conf. getOption.

liyinan926 · 2018-11-19T20:42:58Z

...src/main/scala/org/apache/spark/deploy/k8s/features/HadoopSparkUserExecutorFeatureStep.scala


  override def configurePod(pod: SparkPod): SparkPod = {
-    val sparkUserName = kubernetesConf.sparkConf.get(KERBEROS_SPARK_USER_NAME)
+    val sparkUserName = conf.sparkConf.get(KERBEROS_SPARK_USER_NAME)


liyinan926 · 2018-11-19T20:43:16Z

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

  private val conf = kubernetesConf.sparkConf
+
+  private val hadoopConfDir = Option(kubernetesConf.sparkConf.getenv(ENV_HADOOP_CONF_DIR))
+  private val hadoopConfigMapName = kubernetesConf.sparkConf.get(KUBERNETES_HADOOP_CONF_CONFIG_MAP)


liyinan926 · 2018-11-19T20:45:47Z

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


-  require(kubernetesConf.hadoopConfSpec.isDefined,
-     "Ensure that HADOOP_CONF_DIR is defined either via env or a pre-defined ConfigMap")
-  private val hadoopConfDirSpec = kubernetesConf.hadoopConfSpec.get
  private val conf = kubernetesConf.sparkConf


Is conf still needed?

liyinan926

LGTM

SparkQA · 2018-11-26T23:20:30Z

Test build #99297 has finished for PR 22959 at commit 516ae68.

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

SparkQA · 2018-11-26T23:21:08Z

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

SparkQA · 2018-11-26T23:32:42Z

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

liyinan926 · 2018-11-27T18:12:56Z

@mccheah do you have any comment?

mccheah

Looks fine, think we can merge soon! Only want some feedback on a small point as follows.

mccheah · 2018-11-29T00:27:06Z

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala

+  extends KubernetesConf(sparkConf) {
+
+  override val resourceNamePrefix: String = {
+    val custom = if (Utils.isTesting) get(KUBERNETES_DRIVER_POD_NAME_PREFIX) else None


Possibly inject this in the test so that we don't have to use Utils.isTesting? Preference against using test flags to override behavior.

I'm trying to avoid creating custom test classes here (that's what I understand by "inject", since there's no way to "inject" this otherwise). There's really a single test that needs this functionality, IIRC, and this pattern is way more common in Spark than what you're suggesting.

mccheah · 2018-11-29T22:17:36Z

Given that Utils.isTesting is used elsewhere, I'm fine with merging this. Going to in a few hours if there are no further comments.

mccheah · 2018-12-01T00:23:11Z

Ah I forgot to merge, sorry! Merging into master.

There are a few issues with the current configuration types used in the kubernetes backend: - they use type parameters for role-specific specialization, which makes type signatures really noisy throughout the code base. - they break encapsulation by forcing the code that creates the config object to remove the configuration from SparkConf before creating the k8s-specific wrapper. - they don't provide an easy way for tests to have default values for fields they do not use. This change fixes those problems by: - creating a base config type with role-specific specialization using inheritance - encapsulating the logic of parsing SparkConf into k8s-specific views inside the k8s config classes - providing some helper code for tests to easily override just the part of the configs they want. Most of the change relates to the above, especially cleaning up the tests. While doing that, I also made some smaller changes elsewhere: - removed unnecessary type parameters in KubernetesVolumeSpec - simplified the error detection logic in KubernetesVolumeUtils; all the call sites would just throw the first exception collected by that class, since they all called "get" on the "Try" object. Now the unnecessary wrapping is gone and the exception is just thrown where it occurs. - removed a lot of unnecessary mocking from tests. - changed the kerberos-related code so that less logic needs to live in the driver builder. In spirit it should be part of the upcoming work in this series of cleanups, but it made parts of this change simpler. Tested with existing unit tests and integration tests. Author: Marcelo Vanzin <vanzin@cloudera.com> Closes apache#22959 from vanzin/SPARK-25876.

…riverFeatureStep` ### What changes were proposed in this pull request? This PR removes a variable `hadoopConf` from `KerberosConfDriverFeatureStep`. ### Why are the changes needed? #22959 added a variable `hadoopConf` to generate `tokenManager`. And, #22911 removed `tokenManager` and `buildKerberosSpec`, so `hadoopConf` is no-use. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the GA. Closes #36283 from dcoliversun/SPARK-38968. Authored-by: Qian.Sun <qian.sun2020@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com>

### What changes were proposed in this pull request? This PR aims to promote `KubernetesVolumeUtils` to `DeveloperApi` from Apache Spark 4.0.0 for Apache Spark Kubernetes Operator. ### Why are the changes needed? This API was added by the following at `Apache Spark 3.0.0` and has been stable. - #22959 Since `Apache Spark Kubernetes Operator` requires this, we had better maintain it as a developer API officially from `Apache Spark 4.0.0`. - apache/spark-kubernetes-operator#10 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46326 from dongjoon-hyun/SPARK-48076. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

### What changes were proposed in this pull request? This PR aims to promote `KubernetesVolumeUtils` to `DeveloperApi` from Apache Spark 4.0.0 for Apache Spark Kubernetes Operator. ### Why are the changes needed? This API was added by the following at `Apache Spark 3.0.0` and has been stable. - apache#22959 Since `Apache Spark Kubernetes Operator` requires this, we had better maintain it as a developer API officially from `Apache Spark 4.0.0`. - apache/spark-kubernetes-operator#10 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46326 from dongjoon-hyun/SPARK-48076. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

rvesse approved these changes Nov 8, 2018

View reviewed changes

liyinan926 reviewed Nov 19, 2018

View reviewed changes

Marcelo Vanzin added 4 commits November 26, 2018 14:50

Merge branch 'master' into SPARK-25876

00d7a48

Fix code after merge.

396fcc8

Feedback.

2bed04b

Another test fix.

516ae68

liyinan926 approved these changes Nov 26, 2018

View reviewed changes

mccheah approved these changes Nov 29, 2018

View reviewed changes

asfgit closed this in 6be272b Dec 1, 2018

vanzin deleted the SPARK-25876 branch December 3, 2018 21:55

dcoliversun mentioned this pull request Apr 20, 2022

[SPARK-38968][K8S] Remove a variable hadoopConf from KerberosConfDriverFeatureStep #36283

Closed

dongjoon-hyun mentioned this pull request May 1, 2024

[SPARK-48076][K8S] Promote KubernetesVolumeUtils to DeveloperApi #46326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25876][k8s] Simplify kubernetes configuration types. #22959

[SPARK-25876][k8s] Simplify kubernetes configuration types. #22959

vanzin commented Nov 6, 2018

SparkQA commented Nov 6, 2018

SparkQA commented Nov 6, 2018

SparkQA commented Nov 6, 2018

rvesse commented Nov 7, 2018

rvesse left a comment

vanzin commented Nov 8, 2018

liyinan926 Nov 19, 2018

liyinan926 Nov 19, 2018

liyinan926 Nov 19, 2018

liyinan926 Nov 19, 2018

liyinan926 Nov 19, 2018

liyinan926 Nov 19, 2018

liyinan926 left a comment

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

liyinan926 commented Nov 27, 2018

mccheah left a comment

mccheah Nov 29, 2018

vanzin Nov 29, 2018

mccheah commented Nov 29, 2018

mccheah commented Dec 1, 2018

[SPARK-25876][k8s] Simplify kubernetes configuration types. #22959

[SPARK-25876][k8s] Simplify kubernetes configuration types. #22959

Conversation

vanzin commented Nov 6, 2018

SparkQA commented Nov 6, 2018

SparkQA commented Nov 6, 2018

SparkQA commented Nov 6, 2018

rvesse commented Nov 7, 2018

rvesse left a comment

Choose a reason for hiding this comment

vanzin commented Nov 8, 2018

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

liyinan926 left a comment

Choose a reason for hiding this comment

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

liyinan926 commented Nov 27, 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

mccheah commented Nov 29, 2018

mccheah commented Dec 1, 2018