[SPARK-22646] [Submission] Spark on Kubernetes - basic submission client #19717

liyinan926 · 2017-11-10T17:32:54Z

What changes were proposed in this pull request?

This PR contains implementation of the basic submission client for the cluster mode of Spark on Kubernetes. It's step 2 from the step-wise plan documented here.
This addition is covered by the SPIP vote which passed on Aug 31.

This PR and #19468 together form a MVP of Spark on Kubernetes that allows users to run Spark applications that use resources locally within the driver and executor containers on Kubernetes 1.6 and up. Some changes on pom and build/test setup are copied over from #19468 to make this PR self contained and testable.

The submission client is mainly responsible for creating the Kubernetes pod that runs the Spark driver. It follows a step-based approach to construct the driver pod, as the code under the submit.steps package shows. The steps are orchestrated by DriverConfigurationStepsOrchestrator. Client creates the driver pod and waits for the application to complete if it's configured to do so, which is the case by default.

This PR also contains Dockerfiles of the driver and executor images. They are included because some of the environment variables set in the code would not make sense without referring to the Dockerfiles.

How was this patch tested?

The patch contains unit tests which are passing.
Manual testing: ./build/mvn -Pkubernetes clean package succeeded.
It is a subset of the entire changelist hosted at http://github.com/apache-spark-on-k8s/spark which is in active use in several organizations.
There is integration testing enabled in the fork currently hosted by PepperData which is being moved over to RiseLAB CI.
Detailed documentation on trying out the patch in its entirety is in: https://apache-spark-on-k8s.github.io/userdocs/running-on-kubernetes.html

cc @rxin @felixcheung @mateiz (shepherd)
k8s-big-data SIG members & contributors: @mccheah @foxish @ash211 @ssuchter @varunkatta @kimoonkim @erikerlandson @tnachen @ifilonenko @liyinan926

SparkQA · 2017-11-10T20:54:08Z

Test build #83692 has finished for PR 19717 at commit 37c7ad6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
require(mainClass.isDefined, \"Main class must be specified via --main-class\")

SparkQA · 2017-11-10T21:16:08Z

Test build #83694 has finished for PR 19717 at commit 6d50369.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
require(mainClass.isDefined, \"Main class must be specified via --main-class\")

liyinan926 · 2017-11-17T17:19:46Z

@vanzin @jiangxb1987 @mridulm PTAL. Thanks!

SparkQA · 2017-11-22T22:22:12Z

Test build #84120 has finished for PR 19717 at commit f38144b.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
require(mainClass.isDefined, \"Main class must be specified via --main-class\")

SparkQA · 2017-11-23T01:46:52Z

Test build #84121 has finished for PR 19717 at commit 60234a2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
require(mainClass.isDefined, \"Main class must be specified via --main-class\")

mridulm · 2017-11-27T01:48:36Z

+CC @srowen

mridulm

It has been a while since I looked at spark-submit and related infra. I added a few comments.
I will defer to others more involved with the code for review though.

mridulm · 2017-11-27T01:52:03Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -702,6 +715,18 @@ object SparkSubmit extends CommandLineUtils with Logging {
      }
    }

+    if (isKubernetesCluster) {
+      childMainClass = "org.apache.spark.deploy.k8s.submit.Client"
+      childArgs ++= Array("--primary-java-resource", args.primaryResource)


Handle primary resource == SparkLauncher.NO_RESOURCE ?

mridulm · 2017-11-27T01:57:15Z

resource-managers/kubernetes/core/pom.xml

+  ~ See the License for the specific language governing permissions and
+  ~ limitations under the License.
+  -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">


Hopefully the duplicated files/diffs between #19468 and #19717 are in sync.

We will rebase this PR once #19468 gets merged.

mridulm · 2017-11-27T01:59:50Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+        .withName(s"$ENV_JAVA_OPT_PREFIX$index")
+        .withValue(option)
+        .build()
+    }


As with #19468 I am not sure how this is being used in driver.

It's used in the driver Docker file included in this PR. See https://github.com/apache-spark-on-k8s/spark/blob/60234a29846955b8a6e8cb6fbb1dc35da3c3b4f2/resource-managers/kubernetes/docker/src/main/dockerfiles/driver/Dockerfile.

Thanks ! Not sure how I missed this one when I searched ... something messed up in Intellij

mridulm · 2017-11-27T03:02:45Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+
+    require(mainAppResource.isDefined,
+      "Main app resource must be defined by either --primary-py-file or --primary-java-resource.")
+    require(mainClass.isDefined, "Main class must be specified via --main-class")


mainAppResource need not be valid here

Wondering why?

For case of spark examples, spark thrift server, etc - there is no main app resource - the bits are bundled as part of spark itself.
You can take a look at how yarn or mesos handles this case : I would assume something similar should suffice (look for usage of SparkLauncher.NO_RESOURCE)

Got it, removed the check.

mridulm · 2017-11-27T07:40:16Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+      val resolvedURL = s"https://$masterWithoutK8sPrefix"
+      logInfo("No scheme specified for kubernetes master URL, so defaulting to https. Resolved" +
+        s" URL is $resolvedURL")
+      resolvedURL


Is it required that this should be a https binding ?

Communication with the Kubernetes API server (aka master) from outside the cluster is secured by default, so we expect https is always used.

I see http related check above.
If default is expected to be https, this should be fine.

mridulm · 2017-11-27T07:44:56Z

...src/main/scala/org/apache/spark/deploy/k8s/submit/DriverConfigurationStepsOrchestrator.scala

+    val driverCustomLabels = ConfigurationUtils.parsePrefixedKeyValuePairs(
+      submissionSparkConf,
+      KUBERNETES_DRIVER_LABEL_PREFIX)
+    require(!driverCustomLabels.contains(SPARK_APP_ID_LABEL), "Label with key " +


nit: SPARK_ROLE_LABEL also ?

mridulm · 2017-11-27T08:09:41Z

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

+        closeWatch()
+
+      case Action.ERROR =>
+        closeWatch()


No need for hasCompleted ? Are all error's non-recoverable ?

hasCompleted is checked below. Refactored the cases.

It is checked only for case of action != DELETED and action != ERROR right ?

It's because in both DELETE and ERROR cases, the application is considered being terminated. In other cases, we need to check the phase of the driver pod to determine if the application terminated.

mridulm · 2017-11-27T08:38:43Z

...re/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/BaseDriverConfigurationStep.scala

+        .endEnv()
+      .addNewEnv()
+        .withName(ENV_DRIVER_ARGS)
+        .withValue(appArgs.mkString(" "))


How does this handle escaping, etc ?
If user specified arguments which have whitespace within, this will break ?
Example: appArgs = Array("a 1", "a\t2") and so on

Good catch. Added \"\" around each argument.

mridulm · 2017-11-27T08:42:08Z

...rc/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverKubernetesCredentialsStep.scala

+        "Driver client key file provided at %s does not exist or is not a file.")
+    val clientCertDataBase64 = safeFileConfToBase64(
+        s"$KUBERNETES_AUTH_DRIVER_CONF_PREFIX.$CLIENT_CERT_FILE_CONF_SUFFIX",
+        "Driver client cert file provided at %s does not exist or is not a file.")


I will need to read up on how k8s does security before I can comment more here ... would be great if @vanzin and @tgravescs took a look.

jiangxb1987 · 2017-11-27T03:40:22Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

+      childArgs ++= Array("--main-class", args.mainClass)
+      if (args.childArgs != null) {
+        args.childArgs.foreach { arg =>
+          childArgs += "--arg"


Why not childArgs += ("--arg", arg)?

jiangxb1987 · 2017-11-27T14:31:59Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+
+  val KUBERNETES_NAMESPACE =
+    ConfigBuilder("spark.kubernetes.namespace")
+      .doc("The namespace that will be used for running the driver and executor pods. When using" +


nit: for all the concat strings, always place the space into the end of one line, instead of the beginning of the next line.

Maybe we should do it in #19468 as well.

Sure, will update #19468.

jiangxb1987 · 2017-11-27T14:36:22Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+      .stringConf
+      .createWithDefault("spark")
+
+  val KUBERNETES_ALLOCATION_BATCH_SIZE =


This config is not used in the current PR, so please remove it.

jiangxb1987 · 2017-11-27T14:47:58Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+  val KUBERNETES_DRIVER_ENV_KEY = "spark.kubernetes.driverEnv."
+
+  def getK8sMasterUrl(rawMasterString: String): String = {
+    if (!rawMasterString.startsWith("k8s://")) {


Why not use require here?

jiangxb1987 · 2017-11-27T14:49:00Z

...managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/ConfigurationUtils.scala

+    sparkConf.getAllWithPrefix(prefix).toMap
+  }
+
+  def requireSecondIfFirstIsDefined(


This function is never used.

jiangxb1987 · 2017-11-27T15:02:00Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+      currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
+    }
+
+    val resolvedDriverJavaOpts = currentDriverSpec


Is this resolvedDriverJavaOpts or resolvedDriverOpts?

resolvedDriverJavaOpts.

jiangxb1987 · 2017-11-27T15:10:13Z

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

+ *                             number.
+ */
+private[k8s] class LoggingPodStatusWatcherImpl(
+      appId: String, maybeLoggingInterval: Option[Long])


nit: each param for a single line.

jiangxb1987 · 2017-11-27T15:16:37Z

...re/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/BaseDriverConfigurationStep.scala

+        " Spark bookkeeping operations.")
+
+    val driverCustomEnvs = submissionSparkConf.getAllWithPrefix(KUBERNETES_DRIVER_ENV_KEY).toSeq
+      .map(env => new EnvVarBuilder()


nit: new EnvVarBuilder() in the next line

jiangxb1987 · 2017-11-27T15:25:28Z

...rc/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverKubernetesCredentialsStep.scala

+
+private class OptionSettableSparkConf(sparkConf: SparkConf) {
+  def setOption(configEntry: String, option: Option[String]): SparkConf = {
+    option.map( opt => {


nit:

option.map { opt => sparkConf.set(configEntry, opt) }.getOrElse(sparkConf)

jiangxb1987 · 2017-11-27T15:28:26Z

...ore/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverServiceBootstrapStep.scala

+      val randomServiceId = clock.getTimeMillis()
+      val shorterServiceName = s"spark-$randomServiceId$DRIVER_SVC_POSTFIX"
+      logWarning(s"Driver's hostname would preferably be $preferredServiceName, but this is" +
+        s" too long (must be <= 63 characters). Falling back to use $shorterServiceName" +


nit: must be <= $MAX_SERVICE_NAME_LENGTH characters

jiangxb1987 · 2017-11-27T15:30:10Z

Also cc @cloud-fan @ueshin

jerryshao · 2017-11-28T02:26:45Z

I think we'd better to honor newly added org.apache.spark.deploy.SparkApplication to implement k8s client, like #19631 .

SparkQA · 2017-11-28T03:27:22Z

Test build #84235 has finished for PR 19717 at commit 0f90746.

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

liyinan926 · 2017-11-28T05:18:43Z

@jerryshao Made Client implement SparkApplication in 4164ba5.

jerryshao · 2017-11-28T06:55:49Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+      .createWithDefault(s"spark-executor:$sparkVersion")
+
+  val DOCKER_IMAGE_PULL_POLICY =
+    ConfigBuilder("spark.kubernetes.docker.image.pullPolicy")


This configuration seems like a set of String options, we should check all the legal options like this SQL conf:

val CATALOG_IMPLEMENTATION = buildStaticConf("spark.sql.catalogImplementation") .internal() .stringConf .checkValues(Set("hive", "in-memory")) .createWithDefault("in-memory")

Beside we should add checkValue for the ConfigEntry if possible.

jerryshao · 2017-11-28T06:56:57Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -702,6 +715,19 @@ object SparkSubmit extends CommandLineUtils with Logging {
      }
    }

+    if (isKubernetesCluster) {
+      childMainClass = "org.apache.spark.deploy.k8s.submit.Client"


Here the style should be unified after #19631 is merged.

let's wait for that and rebase this PR

jerryshao · 2017-11-28T07:03:12Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+  val KUBERNETES_DRIVER_ENV_KEY = "spark.kubernetes.driverEnv."
+
+  def getK8sMasterUrl(rawMasterString: String): String = {
+    require(rawMasterString.startsWith("k8s://"),


Shall we add some checks in SparkSubmitArguments for k8s master to fail fast?

What's the convention for mesos and yarn? By the time this method is called, it's already verified that the URL starts with k8s. So this check here just makes sure that the double slashes are also present. I don't see this check in mesos mode and yarn mode.

I see, I was just wondering if we can fail fast for illegal master url.

could we also check the slashes and "k8s://http" sequence up front?
we might want to explore re-using this check/logic by exposing a helper func

Added a util method in Utils and used it in SparkSubmit to check the k8s master URL and set args.master to the resolved URL. This method is no longer needed and is removed as the check is redundant.

jerryshao · 2017-11-28T07:05:16Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala

+        | Kubernetes only:
+        |  --kubernetes-namespace NS   The namespace in the Kubernetes cluster within which the
+        |                              application must be launched. The namespace must already
+        |                              exist in the cluster. (Default: default).


We should also add check for validateKillArguments and validateStatusRequstArguments.

now that docker image is without a default value (spark.kubernetes.*.docker.image), (yes that discussion is ongoing) I wonder if it makes sense to bubble that up as a --param for visibility/convenient.

tbh I'm generally against adding --param to submit because of the potential confusion it can cause, but since we are here and there's a --kubernetes-namespace

I think if we eventually decide to not have default docker images, we should make the options --param ones. I'm not sure if we want to make a call and do that in this PR though. Can we defer this to a later time when we are clearer on how we publish and maintain the images?

jerryshao · 2017-11-28T07:18:50Z

...re/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/BaseDriverConfigurationStep.scala

+        .withName(ENV_SUBMIT_EXTRA_CLASSPATH)
+        .withValue(classPath)
+        .build()
+    }


Did you add support of this configuration "spark.driver.userClassPathFirst" and "spark.driver.userClassPathFirst"? Sorry I cannot find it.

No, I don't think so. @mccheah to confirm.

We don't. It can be supported in the future. However, we also expect many of the use cases that normally would be handled by these properties to be manageable by building custom Docker images and letting those define the artifacts that are on the classpath.

custom docker images ftw !

ueshin

I left some comments from my first round.

ueshin · 2017-11-28T06:14:46Z

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala

+  def getK8sMasterUrl(rawMasterString: String): String = {
+    require(rawMasterString.startsWith("k8s://"),
+      "Master URL should start with k8s:// in Kubernetes mode.")
+    val masterWithoutK8sPrefix = rawMasterString.replaceFirst("k8s://", "")


nit: rawMasterString.substring("k8s://".length)?

ueshin · 2017-11-28T06:21:27Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+    var mainClass: Option[String] = None
+    val driverArgs = mutable.Buffer.empty[String]
+
+    args.sliding(2, 2).toList.collect {


nit: foreach instead of collect?

ueshin · 2017-11-28T06:22:37Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+  def fromCommandLineArgs(args: Array[String]): ClientArguments = {
+    var mainAppResource: Option[MainAppResource] = None
+    var mainClass: Option[String] = None
+    val driverArgs = mutable.Buffer.empty[String]


nit: can we use ArrayBuffer explicitly?

ueshin · 2017-11-28T06:34:45Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+    // client arguments that are passed in, created by orchestrator
+    for (nextStep <- submissionSteps) {
+      currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
+    }


Maybe we can use foldLeft here?
I'm not sure which is preferable for other reviewers, though.

val currentDriverSpec = submissionSteps.foldLeft(KubernetesDriverSpec.initialSpec(submissionSparkConf)) { (spec, step) => step.configureDriver(spec) }

I feel that the current syntax is more readable and intuitive.

Other reviews have stated that foldLeft is to be avoided.

Oh, I see, thanks.

ueshin · 2017-11-28T06:42:37Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+        case (confKey, confValue) => s"-D$confKey=$confValue"
+      } ++ driverJavaOptions.map(Utils.splitCommandString).getOrElse(Seq.empty)
+    val driverJavaOptsEnvs: Seq[EnvVar] = resolvedDriverJavaOpts.zipWithIndex.map {
+      case (option, index) => new EnvVarBuilder()


nit: new EnvVarBuilder() in the next line.

ueshin · 2017-11-28T06:59:16Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+      sparkConf,
+      None,
+      None)) {
+      kubernetesClient =>


nit: indent, or we can put kubernetesClient => at the end of the previous line.

ueshin · 2017-11-28T07:37:03Z

...ore/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverServiceBootstrapStep.scala

+      " address is managed and set to the driver pod's IP address.")
+    require(submissionSparkConf.getOption(DRIVER_HOST_KEY).isEmpty,
+      s"$DRIVER_HOST_KEY is not supported in Kubernetes mode, as the driver's hostname will be" +
+      " managed via a Kubernetes service.")


nit: ditto about concat strings.

ueshin · 2017-11-28T07:46:21Z

resource-managers/kubernetes/docker/src/main/dockerfiles/driver/Dockerfile

+    readarray -t SPARK_DRIVER_JAVA_OPTS < /tmp/java_opts.txt && \
+    if ! [ -z ${SPARK_MOUNTED_CLASSPATH+x} ]; then SPARK_CLASSPATH="$SPARK_MOUNTED_CLASSPATH:$SPARK_CLASSPATH"; fi && \
+    if ! [ -z ${SPARK_SUBMIT_EXTRA_CLASSPATH+x} ]; then SPARK_CLASSPATH="$SPARK_SUBMIT_EXTRA_CLASSPATH:$SPARK_CLASSPATH"; fi && \
+    ${JAVA_HOME}/bin/java "${SPARK_DRIVER_JAVA_OPTS[@]}" -cp "$SPARK_CLASSPATH" -Xms$SPARK_DRIVER_MEMORY -Xmx$SPARK_DRIVER_MEMORY -Dspark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS $SPARK_DRIVER_CLASS $SPARK_DRIVER_ARGS


nit: could you add a line break at the end of the file?

ueshin · 2017-11-28T07:46:37Z

resource-managers/kubernetes/docker/src/main/dockerfiles/spark-base/Dockerfile

+
+WORKDIR /opt/spark/work-dir
+
+ENTRYPOINT [ "/opt/entrypoint.sh" ]


nit: ditto.

ueshin · 2017-11-28T07:46:55Z

resource-managers/kubernetes/docker/src/main/dockerfiles/spark-base/entrypoint.sh

+fi
+
+# Execute the container CMD under tini for better hygiene
+/sbin/tini -s -- "$@"


nit: ditto.

SparkQA · 2017-11-28T08:05:01Z

Test build #84243 has finished for PR 19717 at commit 4164ba5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-11-28T20:08:29Z

Test build #84268 has finished for PR 19717 at commit 350932b.

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

foxish · 2017-11-29T12:19:05Z

@liyinan926, please change PR title to match sub-task: https://issues.apache.org/jira/browse/SPARK-22646

liyinan926 · 2017-11-29T16:36:42Z

Updated the title and rebased onto the latest master.

SparkQA · 2017-11-29T19:49:21Z

Test build #84306 has finished for PR 19717 at commit 2d02e39.

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

ueshin · 2017-11-30T09:00:09Z

...ers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesClientFactory.scala

+ * parse configuration keys, similar to the manner in which Spark's SecurityManager parses SSL
+ * options for different components.
+ */
+private[spark] object KubernetesClientFactory {


What's the difference between this and SparkKubernetesClientFactory?

Ah, it's duplicated. Removed this class.

ueshin · 2017-11-30T09:27:30Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/Client.scala

+    require(mainClass.isDefined, "Main class must be specified via --main-class")
+
+    ClientArguments(
+      mainAppResource.get,


Don't we need to check if mainAppResource is defined or not before here? Otherwise some exception will be thrown if it is not defined.

As @mridulm pointed above, mainAppResource may be empty in cases of spark examples, Thrift server, etc. --primary-java-resource is set only if mainAppResource != SparkLauncher.NO_RESOURCE in SparkSubmit. So it could be empty here. I changed ClientArguments to take an Option[MainAppResource] instead.

Also added the missing step DependencyResolutionStep that is essential for supporting local:// dependencies (jars and files).

ueshin · 2017-11-30T09:57:34Z

...rc/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverKubernetesCredentialsStep.scala

+      mountedUserSpecified: Option[String],
+      valueMountedFromSubmitter: Option[String],
+      mountedCanonicalLocation: String): Option[String] = {
+    mountedUserSpecified.orElse(valueMountedFromSubmitter.map( _ => {


nit: we can remove { and corresponding }.

ueshin · 2017-11-30T09:58:16Z

...rc/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverKubernetesCredentialsStep.scala

+      mountedUserSpecified: Option[String],
+      valueMountedFromSubmitter: Option[String],
+      secretName: String): Map[String, String] = {
+    mountedUserSpecified.map { _ => Map.empty[String, String]}


nit: add a space before }.

liyinan926 · 2017-12-09T01:34:48Z

@vanzin Created https://issues.apache.org/jira/browse/SPARK-22743 to track the work on consolidating the common logic for handling driver and executor memory overhead. Addressed other comments in caf2206. Thanks!

SparkQA · 2017-12-09T05:00:07Z

Test build #84672 has finished for PR 19717 at commit caf2206.

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

felixcheung · 2017-12-09T23:20:56Z

sounds like a good to go from @vanzin and a couple of others too.
any other comment?

felixcheung

minor comment, LGTM

felixcheung · 2017-12-10T19:01:59Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -294,6 +298,16 @@ object SparkSubmit extends CommandLineUtils with Logging {
      }
    }

+    if (clusterManager == KUBERNETES) {
+      args.master = Utils.checkAndGetK8sMasterUrl(args.master)
+      // Make sure YARN is included in our build if we're trying to use it


this should say KUBERNETES

felixcheung · 2017-12-10T19:04:36Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+    val masterWithoutK8sPrefix = rawMasterURL.substring("k8s://".length)
+
+    // To handle master URLs, e.g., k8s://host:port.
+    if (!masterWithoutK8sPrefix.contains("://")) {


startsWith instead of contains?

This is used to differentiate https://host:port vs. host:port. So contains is the right method here.

felixcheung · 2017-12-10T19:13:45Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+        }
+      } catch {
+        case NonFatal(e) =>
+          kubernetesClient.pods().delete(createdDriverPod)


not sure if we have discussed this, should it delete the pod even when the exception is Fatal?

@mccheah commented above that we probably don't want to take action in case of OutOfMemoryError for example. See #19717 (comment).

SparkQA · 2017-12-11T07:18:22Z

Test build #84701 has finished for PR 19717 at commit 2e7810b.

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

vanzin

Couple of nits left.

vanzin · 2017-12-11T19:18:33Z

docs/configuration.md

+  <td>
+    The amount of off-heap memory (in megabytes) to be allocated per driver in cluster mode. This is
+    memory that accounts for things like VM overheads, interned strings, other native overheads, etc.
+    This tends to grow with the container size (typically 6-10%). This option is supported in Yarn


YARN. I'd also simplify:

"This option is currently supported on YARN and Kubernetes."

Same below.

vanzin · 2017-12-11T19:19:21Z

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

@@ -1146,6 +1146,27 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
    }
  }

+  test("check Kubernetes master URL") {
+    val k8sMasterURLHttps = Utils.checkAndGetK8sMasterUrl("k8s://https://host:port")
+    assert(k8sMasterURLHttps == "k8s:https://host:port")


=== (also in other places)

SparkQA · 2017-12-11T23:08:42Z

Test build #84726 has finished for PR 19717 at commit cbcd30e.

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

vanzin · 2017-12-11T23:14:11Z

The SparkR tests seem flaky all of a sudden; other PRs are also failing. So I'm just going to ignore that failure.

Merging to master.

What changes were proposed in this pull request? This PR contains documentation on the usage of Kubernetes scheduler in Spark 2.3, and a shell script to make it easier to build docker images required to use the integration. The changes detailed here are covered by #19717 and #19468 which have merged already. How was this patch tested? The script has been in use for releases on our fork. Rest is documentation. cc rxin mateiz (shepherd) k8s-big-data SIG members & contributors: foxish ash211 mccheah liyinan926 erikerlandson ssuchter varunkatta kimoonkim tnachen ifilonenko reviewers: vanzin felixcheung jiangxb1987 mridulm TODO: - [x] Add dockerfiles directory to built distribution. (#20007) - [x] Change references to docker to instead say "container" (#19995) - [x] Update configuration table. - [x] Modify spark.kubernetes.allocation.batch.delay to take time instead of int (#20032) Author: foxish <[email protected]> Closes #19946 from foxish/update-k8s-docs.

…res` when deploying on K8s ### What changes were proposed in this pull request? Remove Kubernetes from the support list of `--total-executor-cores` in SparkSubmit ### Why are the changes needed? `--total-executor-cores` does not take effect in Spark on K8s, [the comments from original PR](#19717 (comment)) also proves that ### Does this PR introduce _any_ user-facing change? The output of `spark-submit --help` changed ```patch ... - Spark standalone and Kubernetes only: + Spark standalone only: --total-executor-cores NUM Total cores for all executors. ... ``` ### How was this patch tested? Pass GA and review. ### Was this patch authored or co-authored using generative AI tooling? No Closes #43536 from pan3793/tec. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…res` when deploying on K8s Remove Kubernetes from the support list of `--total-executor-cores` in SparkSubmit `--total-executor-cores` does not take effect in Spark on K8s, [the comments from original PR](apache#19717 (comment)) also proves that The output of `spark-submit --help` changed ```patch ... - Spark standalone, Mesos and Kubernetes only: + Spark standalone and Mesos only: --total-executor-cores NUM Total cores for all executors. ... ``` Pass GA and review. No Closes apache#43536 from pan3793/tec. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…or-cores` when deploying on K8s This is the cherry-pick of #43536 for branch-3.3 ### What changes were proposed in this pull request? Remove Kubernetes from the support list of `--total-executor-cores` in SparkSubmit ### Why are the changes needed? `--total-executor-cores` does not take effect in Spark on K8s, [the comments from original PR](#19717 (comment)) also proves that ### Does this PR introduce _any_ user-facing change? The output of `spark-submit --help` changed ```patch ... - Spark standalone, Mesos and Kubernetes only: + Spark standalone and Mesos only: --total-executor-cores NUM Total cores for all executors. ... ``` ### How was this patch tested? Pass GA and review. ### Was this patch authored or co-authored using generative AI tooling? No Closes #43548 from pan3793/SPARK-45670-3.3. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

…or-cores` when deploying on K8s This is the cherry-pick of #43536 for branch-3.4 ### What changes were proposed in this pull request? Remove Kubernetes from the support list of `--total-executor-cores` in SparkSubmit ### Why are the changes needed? `--total-executor-cores` does not take effect in Spark on K8s, [the comments from original PR](#19717 (comment)) also proves that ### Does this PR introduce _any_ user-facing change? The output of `spark-submit --help` changed ```patch ... - Spark standalone, Mesos and Kubernetes only: + Spark standalone and Mesos only: --total-executor-cores NUM Total cores for all executors. ... ``` ### How was this patch tested? Pass GA and review. ### Was this patch authored or co-authored using generative AI tooling? No Closes #43549 from pan3793/SPARK-45670-3.4. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

…or-cores` when deploying on K8s This is the cherry-pick of #43536 for branch-3.5 ### What changes were proposed in this pull request? Remove Kubernetes from the support list of `--total-executor-cores` in SparkSubmit ### Why are the changes needed? `--total-executor-cores` does not take effect in Spark on K8s, [the comments from original PR](#19717 (comment)) also proves that ### Does this PR introduce _any_ user-facing change? The output of `spark-submit --help` changed ```patch ... - Spark standalone, Mesos and Kubernetes only: + Spark standalone and Mesos only: --total-executor-cores NUM Total cores for all executors. ... ``` ### How was this patch tested? Pass GA and review. ### Was this patch authored or co-authored using generative AI tooling? No Closes #43550 from pan3793/SPARK-45670-3.5. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

…or-cores` when deploying on K8s This is the cherry-pick of apache#43536 for branch-3.4 ### What changes were proposed in this pull request? Remove Kubernetes from the support list of `--total-executor-cores` in SparkSubmit ### Why are the changes needed? `--total-executor-cores` does not take effect in Spark on K8s, [the comments from original PR](apache#19717 (comment)) also proves that ### Does this PR introduce _any_ user-facing change? The output of `spark-submit --help` changed ```patch ... - Spark standalone, Mesos and Kubernetes only: + Spark standalone and Mesos only: --total-executor-cores NUM Total cores for all executors. ... ``` ### How was this patch tested? Pass GA and review. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#43549 from pan3793/SPARK-45670-3.4. Authored-by: Cheng Pan <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

foxish mentioned this pull request Nov 10, 2017

Upstreaming and pull request strategy for Spark on Kubernetes apache-spark-on-k8s/spark#441

Open

liyinan926 mentioned this pull request Nov 10, 2017

Spark on Kubernetes - basic submission client apache-spark-on-k8s/spark#545

Closed

foxish mentioned this pull request Nov 10, 2017

[SPARK-18278] [Scheduler] Spark on Kubernetes - Basic Scheduler Backend #19468

Closed

liyinan926 force-pushed the spark-kubernetes-4 branch from 37c7ad6 to 6d50369 Compare November 10, 2017 17:44

liyinan926 force-pushed the spark-kubernetes-4 branch from 6d50369 to f38144b Compare November 22, 2017 22:07

liyinan926 force-pushed the spark-kubernetes-4 branch from f38144b to 60234a2 Compare November 22, 2017 22:26

mridulm reviewed Nov 27, 2017

View reviewed changes

jiangxb1987 reviewed Nov 27, 2017

View reviewed changes

jerryshao reviewed Nov 28, 2017

View reviewed changes

ueshin reviewed Nov 28, 2017

View reviewed changes

mccheah mentioned this pull request Nov 28, 2017

Spark on Kubernetes - basic scheduler backend apache-spark-on-k8s/spark#498

Closed

2 tasks

liyinan926 changed the title ~~[SPARK-18278] [Submission] Spark on Kubernetes - basic submission client~~ [SPARK-22646] [Submission] Spark on Kubernetes - basic submission client Nov 29, 2017

liyinan926 force-pushed the spark-kubernetes-4 branch from 350932b to 2d02e39 Compare November 29, 2017 16:35

ueshin reviewed Nov 30, 2017

View reviewed changes

liyinan926 force-pushed the spark-kubernetes-4 branch from 78e5940 to 563e9f4 Compare November 30, 2017 18:32

Addressed docs comments

caf2206

felixcheung approved these changes Dec 10, 2017

View reviewed changes

Fixed a comment

2e7810b

vanzin reviewed Dec 11, 2017

View reviewed changes

Addressed latest comments

cbcd30e

asfgit closed this in 3f4060c Dec 11, 2017

foxish mentioned this pull request Dec 12, 2017

[SPARK-22648] [K8S] Spark on Kubernetes - Documentation #19946

Closed

4 tasks

foxish deleted the spark-kubernetes-4 branch December 13, 2017 16:48

felixcheung mentioned this pull request Dec 29, 2017

Add libc6-compat to base spark docker image to fix issue with parquet… apache-spark-on-k8s/spark#550

Open

pan3793 mentioned this pull request Oct 26, 2023

[SPARK-45670][CORE] SparkSubmit does not support --total-executor-cores when deploying on K8s #43536

Closed


		WORKDIR /opt/spark/work-dir

		ENTRYPOINT [ "/opt/entrypoint.sh" ]

[SPARK-22646] [Submission] Spark on Kubernetes - basic submission client #19717

[SPARK-22646] [Submission] Spark on Kubernetes - basic submission client #19717

Conversation

liyinan926 commented Nov 10, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 10, 2017

SparkQA commented Nov 10, 2017

liyinan926 commented Nov 17, 2017

SparkQA commented Nov 22, 2017

SparkQA commented Nov 23, 2017

mridulm commented Nov 27, 2017

mridulm left a 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

Choose a reason for hiding this comment

liyinan926 Nov 27, 2017 • edited Loading

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

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

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 Nov 30, 2017 • edited Loading

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

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

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

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

jiangxb1987 commented Nov 27, 2017

jerryshao commented Nov 28, 2017

SparkQA commented Nov 28, 2017

liyinan926 commented Nov 28, 2017

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

jerryshao Nov 28, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyinan926 commented Nov 10, 2017 •

edited

Loading

liyinan926 Nov 27, 2017 •

edited

Loading

liyinan926 Nov 30, 2017 •

edited

Loading

jerryshao Nov 28, 2017 •

edited

Loading