You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ifilonenko <gi...@git.apache.org> on 2018/02/25 00:12:23 UTC
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
GitHub user ifilonenko opened a pull request:
https://github.com/apache/spark/pull/20669
[SPARK-22839][K8S] Remove the use of init-container for downloading remote dependencies
## What changes were proposed in this pull request?
Removal of the init-container for downloading remote dependencies. Built off of the work done by @vanzin in an attempt to refactor driver/executor configuration elaborated in [this](https://issues.apache.org/jira/browse/SPARK-22839) ticket.
## How was this patch tested?
This patch was tested with unit and integration tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ifilonenko/spark remove-init-container
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20669.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20669
----
commit 2fefd0edf2f15ba66620fd507bd0cd7ce01bcd1e
Author: Ilan Filonenko <if...@...>
Date: 2018-02-24T23:25:45Z
Removed the use of init-container for downloading remote dependencies
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174345265
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +179,26 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties file and the java options file
--- End diff --
Forgot to add it. Will resolve this
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1061/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174902939
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
Yes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r172677514
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
it's risky to pass the driver spark configuration this way. The way this is built, this step must be specifically placed last in the list of steps returned by the orchestrator. Ideally, the orchestrators should be able to return monads that can be applied in any order. But this is done in such a way that the driver spark configuration must be completely built in the spec before this step is run.
I think this code shouldn't necessarily even live in a config step, but instead are run directly in the outer submission client. This is what we were doing before, when we just passed all of the driver Spark configurations as system properties to the main class. We can replace that instead with passing these values as `--conf` options. Thoughts?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1034/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
Hm, noted that we're making this tradeoff. We have an internal use case where we're pushing a custom logging properties file into the container using `spark.files`. Logging properties files need to be in the container before the JVM starts to configure the appenders from the get-go, but logging properties are more dynamic and probably don't belong in a statically built Docker image. We use YARN cluster mode primarily and rely on its file distribution, and we migrated to the fork's implementation of Kubernetes without having to change our internal setup.
I think we can adapt to this change, but I don't think the use case I've described is as uncommon as one may think. There's plenty of lower-level tooling out there that requires the JVM to load files in static initializations.
> Oh, btw, if you think that is a really, really important feature, you still don't need an init container for that. You can just run the dependency download tool before you run spark-submit in the driver container. Problem solved.
Agreed. Init-containers are but one option to support this. The question was more if running spark-submit in client mode is completely sufficient, which it seems like it isn't in this specific case.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174564815
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterManager.scala ---
@@ -33,7 +33,9 @@ private[spark] class KubernetesClusterManager extends ExternalClusterManager wit
override def canCreate(masterURL: String): Boolean = masterURL.startsWith("k8s")
override def createTaskScheduler(sc: SparkContext, masterURL: String): TaskScheduler = {
- if (masterURL.startsWith("k8s") && sc.deployMode == "client") {
+ if (masterURL.startsWith("k8s") &&
+ sc.deployMode == "client" &&
+ !sc.conf.contains(KUBERNETES_EXECUTOR_POD_NAME_PREFIX)) {
--- End diff --
Is this sufficient to prevent use the client mode from end users? What about adding a special key when calling `spark-submit` in the driver and test that key here instead?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
BTW I wonder why are we still relying on integration tests that do not live in the Spark repo...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1105/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175237354
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala ---
@@ -79,6 +79,12 @@ private[spark] object Config extends Logging {
.stringConf
.createOptional
+ val KUBERNETES_DRIVER_SUBMIT_CHECK =
+ ConfigBuilder("spark.kubernetes.submitInDriver")
+ .internal()
--- End diff --
Nit: The indentation looks slightly off here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/20669
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174638065
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
Ah I think I was misreading the code here - just putting `spark.driver.extraJavaOptions` in the properties works. See https://github.com/apache/spark/blob/cfcd746689c2b84824745fa6d327ffb584c7a17d/launcher/src/main/java/org/apache/spark/launcher/SparkSubmitCommandBuilder.java#L268
Sorry about that - @ifilonenko think just adding `spark.driver.extraJavaOptions` as-is to the properties file will suffice.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1032/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
I'm not sure there was a consensus at the end of that discussion, and @mccheah has been working on some internal refactoring here also. @foxish
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1495/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
Oh, btw, if you think that is a really, really important feature, you still don't need an init container for that. You can just run the dependency download tool before you run spark-submit in the driver container. Problem solved.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174562210
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -202,6 +221,10 @@ private[spark] class KubernetesClientApplication extends SparkApplication {
val launchTime = System.currentTimeMillis()
val waitForAppCompletion = sparkConf.get(WAIT_FOR_APP_COMPLETION)
val appName = sparkConf.getOption("spark.app.name").getOrElse("spark")
+ val kubernetesResourceNamePrefix = {
+ val uuid = UUID.nameUUIDFromBytes(Longs.toByteArray(launchTime)).toString.replaceAll("-", "")
--- End diff --
Using a uuid with `-` removed is what we already do.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175268465
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
> If one is going to override the docker image and provide their own implementation
There is no documentation about that, so how about defining what that contract is supposed to be first? Otherwise we're all just talking about hypotheticals.
My current understanding is that regardless of what's in the image, `entrypoint.sh` defines the entry point's contract, so this is an internal decision between spark-submit code and that shell script.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
@vanzin anything other feedback before merging this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174626276
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
+1 for this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174606620
--- Diff: examples/src/main/scala/org/apache/spark/examples/SparkRemoteFileTest.scala ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// scalastyle:off println
+package org.apache.spark.examples
+
+import java.io.File
+
+import org.apache.spark.SparkFiles
+import org.apache.spark.sql.SparkSession
+
+/** Usage: SparkRemoteFileTest [file] */
+object SparkRemoteFileTest {
--- End diff --
Hm, what is the purpose of this specific example?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1067/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
If that's the consensus, then great! I thought some people still wanted to keep the init container around for some reason.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on the issue:
https://github.com/apache/spark/pull/20669
@vanzin @mccheah @foxish This is ready for your review as it passes unit tests and integration tests. At the moment, it is required that we modify the integration tests to call `SparkFiles.get()`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/20669
It's just because we haven't PR'd them yet. There's some cleanups that I wanted to do before doing that.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174636337
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
Why does it have to be on the command line and not in the properties file?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1459/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
I probably won't have time to review this carefully, so if you're happy with it, don't wait for me.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174635977
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
This doesn't put `spark.driver.extraJavaOptions` on the launch of the spark-submit command in client mode, I think.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174596786
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
}
}
- private def resolveFileUri(
- uri: String,
- fileDownloadPath: String,
- assumesDownloaded: Boolean): String = {
+ private def resolveFileUri(uri: String): String = {
val fileUri = Utils.resolveURI(uri)
- val fileScheme = Option(fileUri.getScheme).getOrElse("file")
+ val fileScheme = Option(fileUri.getScheme).getOrElse("non-local")
--- End diff --
Should just stick with `file` I think.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1558/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/20669
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
@mccheah you should have gotten an e-mail from Matei explaining the basics of how to merge PRs.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174626627
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
You want to write two config map data items:
- one data item with the properties file name,
- One data item with the JVM options.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1066/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174630216
--- Diff: examples/src/main/scala/org/apache/spark/examples/SparkRemoteFileTest.scala ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// scalastyle:off println
+package org.apache.spark.examples
+
+import java.io.File
+
+import org.apache.spark.SparkFiles
+import org.apache.spark.sql.SparkSession
+
+/** Usage: SparkRemoteFileTest [file] */
+object SparkRemoteFileTest {
--- End diff --
To test the presence of a remote file being mounted on the executors via the spark-submit being run by the driver. Should I add a Javadoc (`HDFSTest.scala` didn't include one) but I can if necessary
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r173866657
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
Not sure I understand why being able to declare things in any order is important.
Not doing so means you'll have to pass properties as command line arguments. Which, in a way, is already enforcing ordering - it assumes that the code that sets those command line arguments runs last, so it can see all parameters.
This approach also means you don't need to think about things such as java options. They're in the config file, which is processed by spark-submit. The whole point of this exercise was to avoid duplicating stuff that spark-submit already does.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174564001
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
I have one concern on using a different path for `SPARK_CONF_DIR` than `/opt/spark/conf` which is the one under the installation path. Because the environment variable `SPARK_CONF_DIR` is set, people who want to customize the image, e.g., using a custom `spark-env.sh` has to know to put the file into this custom path instead of `/opt/spark/conf`. Why don't we use `/opt/spark/conf` instead?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
My impression was that we did reach a consensus that this was the right idea from the mailing list discussion. Specifically http://apache-spark-developers-list.1001551.n3.nabble.com/Kubernetes-why-use-init-containers-tp23113p23140.html.
We want to get this in before we start refactoring because it reduces the size of the code that actually needs to be refactored by a sizable amount.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174596718
--- Diff: bin/docker-image-tool.sh ---
@@ -63,9 +63,11 @@ function build {
error "Cannot find docker image. This script must be run from a runnable distribution of Apache Spark."
fi
+ local DOCKERFILE=${DOCKERFILE:-"$IMG_PATH/spark/Dockerfile"}
--- End diff --
Can we separate this out?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174350660
--- Diff: bin/docker-image-tool.sh ---
@@ -63,9 +63,11 @@ function build {
error "Cannot find docker image. This script must be run from a runnable distribution of Apache Spark."
fi
+ local DOCKERFILE=${DOCKERFILE:-"$IMG_PATH/spark/Dockerfile"}
--- End diff --
Uhm, this technically could be a separate PR. I was initially piggy-backing off the work of Marcelo in being able to build Dockerfiles via -f.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1511/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174597583
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
I'm unsure of what the behavior is if we try to mount to a directory that already exists on the image itself.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on the issue:
https://github.com/apache/spark/pull/20669
@vanzin you are definitely right and that is what needs to happen. However, one of the functions of the init container was resolving the file location which atm isn’t supported unless the spark-application that is testing this service uses
`SparkFiles.get()`. This is something I am looking into right now either on the integration testing side or maybe there could be some way to bring back this resolution (as the file is currently being stored in /tmp)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174626720
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
In other words I'd expect two calls to `addToData` here with the appropriate item names corresponding to the file names.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174627764
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
> One data item with the JVM options.
Why? spark-submit handles that already.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1475/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175522981
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
Think that makes sense re: defining specifically what a custom implementation has to do. We can follow up on that separately. Also agree that we haven't been too precise about what a custom implementation would look like. There's simple things like adding or modifying the existing content, but in terms of modifying logic we haven't given that enough thought.
For now I think the code we have here will suffice for the immediate need of removing the init-container, and we can follow up later on which path we want to take in this part of the discussion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
Thanks - merging shortly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
@vanzin I have a question regarding how this will interact with files that need to exist before the JVM starts.
When we used the init-container approach, we were specifically fetching the user's files before the user's JVM launches. This meant that all the user's dependencies were present at JVM boot time. Now, however, we are having spark-submit do the localization. But the spark-submit JVM is the same JVM that runs the user's code here.
Let's take a concrete hypothetical example where the user would like to load a YourKit agent binary into the driver container for debugging. The user may not want to build an entirely separate docker image for this, or perhaps they're porting over a debugging workflow from YARN mode where they used `--files` to distribute this binary that they hosted in some remote location. In YARN mode, adding the yourkit agent to `spark.files` works because SparkSubmit distributes the files via `spark.yarn.dist.files` and using HDFS-backed localization. The files are localized before the driver JVM starts, and the yourkit agent is loaded correctly. However, in Kubernetes mode, without an init-container, the yourkit agent binary will always be localized after the driver JVM starts, which is too late.
In other words, without an init-container, it's impossible for applications to depend on localizing files that must be present _before_ the JVM even launches. This is mitigated by docker being essentially a built-in localization mechanism in and of itself, and the fact that one can use secrets/config maps as an alternate mounting mechanism. But I'm curious as to our thoughts for those porting over YARN applications with this specific use case. I'm also curious as to how Mesos handles this, since they use a similar scheme too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on the issue:
https://github.com/apache/spark/pull/20669
Newest push passes all tests (with this merged I will then merge in [this](https://github.com/apache-spark-on-k8s/spark-integration/pull/42/files))
```
KubernetesSuite:
- Run SparkPi with no resources
- Run SparkPi with a very long application name.
- Run SparkPi with a master URL without a scheme.
- Run SparkPi with an argument.
- Run SparkPi with custom labels, annotations, and environment variables.
- Run SparkPi with a test secret mounted into the driver and executor pods
- Run FileCheck using a Remote Data File
Run completed in 2 minutes, 37 seconds.
Total number of tests run: 7
Suites: completed 2, aborted 0
Tests: succeeded 7, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
```
I welcome the opinion of the community on the strategy for passing spark.driver.extraJavaOptions to the driver as I am currently specifying the `SPARK_CONF_DIR` to be pointed at the JAVA_PROPERTIES file. Open to any better suggestions.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174607871
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
Then I would suggest removing `COPY conf /opt/spark/conf` from the Dockerfile as it is no effect anyway if `SPARK_CONF_DIR` points to somewhere else. Then set `SPARK_CONF_DIR` to point to `/opt/spark/conf`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1160/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1167/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1459/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1028/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
@ifilonenko you can't just remove the init container without replacing its functionality. In my patch that was done by using spark-submit to run the driver. But here you have nothing. That's why the tests are failing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174328774
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +179,26 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties file and the java options file
--- End diff --
Maybe an incorrect javadoc, or else this is erroneously not putting in the java options file? Also where do we handle `spark.driver.extraJavaOptions` here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174628963
--- Diff: bin/docker-image-tool.sh ---
@@ -63,9 +63,11 @@ function build {
error "Cannot find docker image. This script must be run from a runnable distribution of Apache Spark."
fi
+ local DOCKERFILE=${DOCKERFILE:-"$IMG_PATH/spark/Dockerfile"}
--- End diff --
Not a problem.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1028/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1026/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174608662
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterManager.scala ---
@@ -33,7 +33,9 @@ private[spark] class KubernetesClusterManager extends ExternalClusterManager wit
override def canCreate(masterURL: String): Boolean = masterURL.startsWith("k8s")
override def createTaskScheduler(sc: SparkContext, masterURL: String): TaskScheduler = {
- if (masterURL.startsWith("k8s") && sc.deployMode == "client") {
+ if (masterURL.startsWith("k8s") &&
+ sc.deployMode == "client" &&
+ !sc.conf.contains(KUBERNETES_EXECUTOR_POD_NAME_PREFIX)) {
--- End diff --
I think it's safer to have a new internal config key that is only used for this purpose. Checking the presence of `KUBERNETES_EXECUTOR_POD_NAME_PREFIX ` isn't sufficient as it is set by the submission client code that will be called in client mode also.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174561587
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
--- End diff --
Actually is this function still needed?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174329806
--- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala ---
@@ -108,62 +111,42 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
SecondTestConfigurationStep.containerName)
}
- test("The client should create the secondary Kubernetes resources.") {
+ test("The client should create Kubernetes resources") {
val submissionClient = new Client(
submissionSteps,
new SparkConf(false),
kubernetesClient,
false,
"spark",
- loggingPodStatusWatcher)
+ loggingPodStatusWatcher,
+ KUBERNETES_RESOURCE_PREFIX)
submissionClient.run()
val createdPod = createdPodArgumentCaptor.getValue
val otherCreatedResources = createdResourcesArgumentCaptor.getAllValues
- assert(otherCreatedResources.size === 1)
- val createdResource = Iterables.getOnlyElement(otherCreatedResources).asInstanceOf[Secret]
- assert(createdResource.getMetadata.getName === FirstTestConfigurationStep.secretName)
- assert(createdResource.getData.asScala ===
- Map(FirstTestConfigurationStep.secretKey -> FirstTestConfigurationStep.secretData))
- val ownerReference = Iterables.getOnlyElement(createdResource.getMetadata.getOwnerReferences)
- assert(ownerReference.getName === createdPod.getMetadata.getName)
- assert(ownerReference.getKind === DRIVER_POD_KIND)
- assert(ownerReference.getUid === DRIVER_POD_UID)
- assert(ownerReference.getApiVersion === DRIVER_POD_API_VERSION)
- }
-
- test("The client should attach the driver container with the appropriate JVM options.") {
- val sparkConf = new SparkConf(false)
- .set("spark.logConf", "true")
- .set(
- org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS,
- "-XX:+HeapDumpOnOutOfMemoryError -XX:+PrintGCDetails")
- val submissionClient = new Client(
- submissionSteps,
- sparkConf,
- kubernetesClient,
- false,
- "spark",
- loggingPodStatusWatcher)
- submissionClient.run()
- val createdPod = createdPodArgumentCaptor.getValue
+ assert(otherCreatedResources.size === 2)
+ otherCreatedResources.toArray.foreach{
--- End diff --
Think for this test, it'll be more streamlined if you
- check for the existence of a secret,
- extract the secret and check its contents,
- check for the existence of a config map,
- extract the config map and check its contents.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1160/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20669
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by erikerlandson <gi...@git.apache.org>.
Github user erikerlandson commented on the issue:
https://github.com/apache/spark/pull/20669
@mccheah workflow is to use `dev/merge_spark_pr.py`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/20669
@vanzin I just opened this PR for putting the integration tests in the Spark repo: https://github.com/apache/spark/pull/20697
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1072/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174602662
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
--- End diff --
Remove this for now.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r172678246
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
One other question - how are we passing `spark.driver.extraJavaOptions` properties to the driver now?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174636792
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
You really should try this out to see that it works the way I've been saying since the beginning. Just let spark-submit do its thing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
That's one scenario where an init-container /might/ help. But be aware that your YARN example only works in a single one scenario - YARN cluster mode, and maybe even then may need some config customization. And it may not even work in some cases (e.g. YARN data directory mounted with `noexec`). YARN client, nor any other supported cluster manager, is able to do what you're saying.
Personally I feel it's perfectly ok to require a custom docker image in these cases, since they're so uncommon (I've never seen one of our users use the yarn-cluster feature for this purpose). People can have a "main" Spark image and a "debug" one that can be easily chosen from when submitting the app.
During this discussion I think someone mentioned that it might be possible to side-load init containers into Spark without this. I'm not that familiar with kubernetes, but if that's possible, it's another way you could achieve this without Spark having its own init container.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by foxish <gi...@git.apache.org>.
Github user foxish commented on the issue:
https://github.com/apache/spark/pull/20669
There's a section explaining it at the bottom of https://spark.apache.org/committers.html
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175238547
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
What's the problem of passing arguments as arguments? I really dislike using environment variables in general, but this would be an especially nasty use for them. And I'm not sure how they make the contract more obvious.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175520457
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
> The point is, even if entrypoint.sh defines the contract, what arguments would a custom implementation pass it?
`entrypoint.sh` doesn't really define the contract, it implements the contract. Basically the entry point has to respect whatever the Spark submit code tells it to do.
So define the contract first. If you want users to be able to write their own custom entry points, then document that contract in a public doc, so that it cannot change in future versions of Spark. That was the main reason (well, one of them) why I asked this whole thing to be marked "experimental" in 2.3 - nothing is defined, but there seems to be a lot of tribal knowledge about what people want to do.
That doesn't really work in the long run.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
Merge button doesn't appear for me in the UI =( will need to look into that.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175518159
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
We've seen some users override the entry point to do a bunch of extra bootstrapping before calling into the actual command to run the driver in custom images. I vaguely recall something related to Openshift doing this - @erikerlandson.
The point is, even if `entrypoint.sh` defines the contract, what arguments would a custom implementation pass it? Something like this would fail because we didn't pass the appropriate arguments to the entrypoint:
```
#!/bin/bash
# my-entrypoint.sh
source /opt/my-custom.script.sh
/opt/spark/entrypoint.sh # Notice we don't pass arguments
```
So the contract may be, that whatever arguments the user's entrypoint is passed, they must eventually call `entrypoint.sh` with all of those arguments forwarded through. That might be an acceptable contract. With environment variables we can say that it doesn't matter what arguments the downstream entrypoint is passed, just that you eventually need to run that code and it knows what to do if the application submission code set up the container's environment properly.
Having defined these ideas more precisely, I think either contract is acceptable here, with a slight preference towards having Spark interpret the environment rather than the arguments.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on the issue:
https://github.com/apache/spark/pull/20669
Results from integration testing:
```
Discovery starting.
Discovery completed in 123 milliseconds.
Run starting. Expected test count is: 8
KubernetesSuite:
- Run SparkPi with no resources
- Run SparkPi with a very long application name.
- Run SparkPi with a master URL without a scheme.
- Run SparkPi with an argument.
- Run SparkPi with custom labels, annotations, and environment variables.
- Run SparkPi with a test secret mounted into the driver and executor pods
- Test extraJVMProprties being present on Driver
- Run FileCheck using a Remote Data File
Run completed in 3 minutes, 22 seconds.
Total number of tests run: 8
Suites: completed 2, aborted 0
Tests: succeeded 8, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174582921
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
I am flexible to that, anyone else have thoughts?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1026/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174597008
--- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala ---
@@ -108,62 +111,42 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
SecondTestConfigurationStep.containerName)
}
- test("The client should create the secondary Kubernetes resources.") {
+ test("The client should create Kubernetes resources") {
val submissionClient = new Client(
submissionSteps,
new SparkConf(false),
kubernetesClient,
false,
"spark",
- loggingPodStatusWatcher)
+ loggingPodStatusWatcher,
+ KUBERNETES_RESOURCE_PREFIX)
submissionClient.run()
val createdPod = createdPodArgumentCaptor.getValue
val otherCreatedResources = createdResourcesArgumentCaptor.getAllValues
- assert(otherCreatedResources.size === 1)
- val createdResource = Iterables.getOnlyElement(otherCreatedResources).asInstanceOf[Secret]
- assert(createdResource.getMetadata.getName === FirstTestConfigurationStep.secretName)
- assert(createdResource.getData.asScala ===
- Map(FirstTestConfigurationStep.secretKey -> FirstTestConfigurationStep.secretData))
- val ownerReference = Iterables.getOnlyElement(createdResource.getMetadata.getOwnerReferences)
- assert(ownerReference.getName === createdPod.getMetadata.getName)
- assert(ownerReference.getKind === DRIVER_POD_KIND)
- assert(ownerReference.getUid === DRIVER_POD_UID)
- assert(ownerReference.getApiVersion === DRIVER_POD_API_VERSION)
- }
-
- test("The client should attach the driver container with the appropriate JVM options.") {
- val sparkConf = new SparkConf(false)
- .set("spark.logConf", "true")
- .set(
- org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS,
- "-XX:+HeapDumpOnOutOfMemoryError -XX:+PrintGCDetails")
- val submissionClient = new Client(
- submissionSteps,
- sparkConf,
- kubernetesClient,
- false,
- "spark",
- loggingPodStatusWatcher)
- submissionClient.run()
- val createdPod = createdPodArgumentCaptor.getValue
+ assert(otherCreatedResources.size === 2)
+ otherCreatedResources.toArray.foreach{
--- End diff --
This test wouldn't catch if we returned 2 secrets or 2 config maps, instead of one of each, for example.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174636507
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
My understanding is for `spark-submit` to pick it up you want a `jvm-opts` file in the Spark configuration directory. This is specifically for JVM options that aren't `spark.*` options, e.g. GC, out of memory settings, yourkit agent loading.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
I think tests are failing because you're missing changes to run the driver through spark-submit in the entry point script:
https://github.com/vanzin/spark/commit/7e1dd8f0823993a9c1872c203f7bf4e0d7da3b09#diff-c12f609c87a8093c2902284459fbe84b
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174583140
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
--- End diff --
At the moment is not being used. Should it be removed then?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1574/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r173912287
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
> Just to double check, will this logic need to be re-used anywhere? I don't think the executor pods have to mount the same properties config map.
Currently, I don't think so, since executors aren't started with spark-submit. That would be nice to do at some point, to simplify how executors are started, but that's a little beyond what this PR is doing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174629033
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
}
}
- private def resolveFileUri(
- uri: String,
- fileDownloadPath: String,
- assumesDownloaded: Boolean): String = {
+ private def resolveFileUri(uri: String): String = {
val fileUri = Utils.resolveURI(uri)
- val fileScheme = Option(fileUri.getScheme).getOrElse("file")
+ val fileScheme = Option(fileUri.getScheme).getOrElse("non-local")
--- End diff --
Done.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1558/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174345430
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
}
}
- private def resolveFileUri(
- uri: String,
- fileDownloadPath: String,
- assumesDownloaded: Boolean): String = {
+ private def resolveFileUri(uri: String): String = {
val fileUri = Utils.resolveURI(uri)
- val fileScheme = Option(fileUri.getScheme).getOrElse("file")
+ val fileScheme = Option(fileUri.getScheme).getOrElse("non-local")
--- End diff --
Valid, `match` statement is on `_` so it didn't matter what I put in. I can change this if need be
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r173871228
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
I see - using the config map is a good idea then, but it still can't be in a config step because it has to assume that all the properties its given are the absolute final ones. We should keep the driver properties code in the same _place_ as it was before, which was separate from the config steps and after all the steps were processed. The implementation can still be with the config map which allows spark-submit to pick up these properties.
Just to double check, will this logic need to be re-used anywhere? I don't think the executor pods have to mount the same properties config map.
> I will also include --conf spark.driver.extraJavaOptions= in the spark-submit launched in the driver
That won't work because by the time spark-submit is running, you'll want to _already_ have these JVM options applied since spark-submit will be running the driver JVM itself. You'll need to find a way to get the spark-submit JVM to run with these JVM options.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
`SparkFiles.get()` is the official way of retrieving anything that is distributed using `--files`. So if the application is not using that, it's relying on undocumented behavior.
In the error logs from jenkins, the path of the file ("/var/spark-data/spark-files/pagerank_data.txt") seems to be provided by the test code as part of running spark-submit. That seems wrong, since the test code doesn't really have control of whether files will show up. If the file is already in the image somehow, then it's probably a case of the test and the image not agreeing about what the path is. If the file is uploaded using `--files`, then providing an absolute path to the app is wrong (or, at worst, the app should be using just the file name and using `SparkFiles.get()` to find its actual location).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1066/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174348342
--- Diff: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala ---
@@ -108,62 +111,42 @@ class ClientSuite extends SparkFunSuite with BeforeAndAfter {
SecondTestConfigurationStep.containerName)
}
- test("The client should create the secondary Kubernetes resources.") {
+ test("The client should create Kubernetes resources") {
val submissionClient = new Client(
submissionSteps,
new SparkConf(false),
kubernetesClient,
false,
"spark",
- loggingPodStatusWatcher)
+ loggingPodStatusWatcher,
+ KUBERNETES_RESOURCE_PREFIX)
submissionClient.run()
val createdPod = createdPodArgumentCaptor.getValue
val otherCreatedResources = createdResourcesArgumentCaptor.getAllValues
- assert(otherCreatedResources.size === 1)
- val createdResource = Iterables.getOnlyElement(otherCreatedResources).asInstanceOf[Secret]
- assert(createdResource.getMetadata.getName === FirstTestConfigurationStep.secretName)
- assert(createdResource.getData.asScala ===
- Map(FirstTestConfigurationStep.secretKey -> FirstTestConfigurationStep.secretData))
- val ownerReference = Iterables.getOnlyElement(createdResource.getMetadata.getOwnerReferences)
- assert(ownerReference.getName === createdPod.getMetadata.getName)
- assert(ownerReference.getKind === DRIVER_POD_KIND)
- assert(ownerReference.getUid === DRIVER_POD_UID)
- assert(ownerReference.getApiVersion === DRIVER_POD_API_VERSION)
- }
-
- test("The client should attach the driver container with the appropriate JVM options.") {
- val sparkConf = new SparkConf(false)
- .set("spark.logConf", "true")
- .set(
- org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS,
- "-XX:+HeapDumpOnOutOfMemoryError -XX:+PrintGCDetails")
- val submissionClient = new Client(
- submissionSteps,
- sparkConf,
- kubernetesClient,
- false,
- "spark",
- loggingPodStatusWatcher)
- submissionClient.run()
- val createdPod = createdPodArgumentCaptor.getValue
+ assert(otherCreatedResources.size === 2)
+ otherCreatedResources.toArray.foreach{
--- End diff --
I thought it would be easier to just go through all the resources in a single loop, do you think this is better?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174345374
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
--- End diff --
Unnecessary, but it should not be allowed in our Utils as we can't resolve the location without `SparkFiles.get()`which makes that logic useless. No?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174582150
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterManager.scala ---
@@ -33,7 +33,9 @@ private[spark] class KubernetesClusterManager extends ExternalClusterManager wit
override def canCreate(masterURL: String): Boolean = masterURL.startsWith("k8s")
override def createTaskScheduler(sc: SparkContext, masterURL: String): TaskScheduler = {
- if (masterURL.startsWith("k8s") && sc.deployMode == "client") {
+ if (masterURL.startsWith("k8s") &&
+ sc.deployMode == "client" &&
+ !sc.conf.contains(KUBERNETES_EXECUTOR_POD_NAME_PREFIX)) {
--- End diff --
I believe that logic might be beyond the scope of this PR. But I could add that if it seems appropriate.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174902563
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -110,30 +109,29 @@ private[spark] class Client(
for (nextStep <- submissionSteps) {
currentDriverSpec = nextStep.configureDriver(currentDriverSpec)
}
-
- val resolvedDriverJavaOpts = currentDriverSpec
- .driverSparkConf
- // Remove this as the options are instead extracted and set individually below using
- // environment variables with prefix SPARK_JAVA_OPT_.
- .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
- .getAll
- .map {
- 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()
- .withName(s"$ENV_JAVA_OPT_PREFIX$index")
- .withValue(option)
- .build()
- }
-
+ val configMapName = s"$kubernetesResourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, currentDriverSpec.driverSparkConf)
+ // The include of the ENV_VAR for "SPARK_CONF_DIR" is to allow for the
+ // Spark command builder to pickup on the Java Options present in the ConfigMap
val resolvedDriverContainer = new ContainerBuilder(currentDriverSpec.driverContainer)
- .addAllToEnv(driverJavaOptsEnvs.asJava)
+ .addNewEnv()
+ .withName(SPARK_CONF_DIR_ENV)
+ .withValue(SPARK_CONF_PATH)
--- End diff --
Do the executors require a SPARK_CONF_DIR directory to be defined as well?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20669
@mccheah you're a committer...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r173665102
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
I will also include `--conf spark.driver.extraJavaOptions=` in the spark-submit launched in the driver
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1105/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174628986
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
--- End diff --
Done.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by foxish <gi...@git.apache.org>.
Github user foxish commented on the issue:
https://github.com/apache/spark/pull/20669
+1, the benefits outweigh the desire to be k8s-like in the approach. We should have tests that validate the behavior changes though. I'm wondering if we should move the integration tests into the repo first, before indulging in any large scale refactors.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174328623
--- Diff: bin/docker-image-tool.sh ---
@@ -63,9 +63,11 @@ function build {
error "Cannot find docker image. This script must be run from a runnable distribution of Apache Spark."
fi
+ local DOCKERFILE=${DOCKERFILE:-"$IMG_PATH/spark/Dockerfile"}
--- End diff --
Any particular reason why this is tied to this change?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/20669
Think someone with permissions to merge has to do so here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174329568
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
}
}
- private def resolveFileUri(
- uri: String,
- fileDownloadPath: String,
- assumesDownloaded: Boolean): String = {
+ private def resolveFileUri(uri: String): String = {
val fileUri = Utils.resolveURI(uri)
- val fileScheme = Option(fileUri.getScheme).getOrElse("file")
+ val fileScheme = Option(fileUri.getScheme).getOrElse("non-local")
--- End diff --
Don't think we should be putting an arbitrary scheme here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r173663339
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/DriverConfigPropertiesStep.scala ---
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.deploy.k8s.submit.steps
+
+import java.io.StringWriter
+import java.util.Properties
+
+import io.fabric8.kubernetes.api.model._
+
+import org.apache.spark.SparkConf
+import org.apache.spark.deploy.k8s.Config._
+import org.apache.spark.deploy.k8s.Constants._
+import org.apache.spark.deploy.k8s.submit.KubernetesDriverSpec
+
+/**
+ * Create a config map with the driver configuration and attach it to the pod. This needs to
+ * come at the end of the driver configuration so that all modifications to the Spark config
+ * are reflected in the generated config map.
+ */
+private[spark] class DriverConfigPropertiesStep(resourceNamePrefix: String)
+ extends DriverConfigurationStep {
+
+ override def configureDriver(spec: KubernetesDriverSpec): KubernetesDriverSpec = {
+ val configMapName = s"$resourceNamePrefix-driver-conf-map"
+ val configMap = buildConfigMap(configMapName, spec.driverSparkConf)
--- End diff --
I agree in that it should be in any order. I will restructure it as such.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174329513
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -89,26 +56,16 @@ private[spark] object KubernetesUtils {
def getOnlyRemoteFiles(uris: Iterable[String]): Iterable[String] = {
uris.filter { uri =>
val scheme = Utils.resolveURI(uri).getScheme
- scheme != "file" && scheme != "local"
+ scheme != "local"
--- End diff --
Change seems unnecessary for now, since `file` is still forbidden by our submission code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1061/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175238936
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
If one is going to override the docker image and provide their own implementation, the customizing developer would like to know all of the parameters that they are being passed up-front. Receiving "$@" makes it less obvious that we should be expecting a properties file and the `SparkLauncher.NO_RESOURCE` from spark-submit.
We could do argument parsing here to verify that `--properties-file` etc. are explicitly provided.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S][WIP] Remove the use of init-container...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1112/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1495/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174626914
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -180,6 +184,31 @@ private[spark] class Client(
originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
}
}
+
+ // Build a Config Map that will house both the properties and the java options in a single file
+ private def buildConfigMap(
+ configMapName: String,
+ conf: SparkConf,
+ driverJavaOps: String): ConfigMap = {
+ val properties = new Properties()
+ conf
+ .remove(org.apache.spark.internal.config.DRIVER_JAVA_OPTIONS)
+ .getAll.foreach { case (k, v) =>
+ properties.setProperty(k, v)
+ }
+ val propertiesWriter = new StringWriter()
+ properties.store(propertiesWriter,
+ s"Java properties built from Kubernetes config map with name: $configMapName")
+
+ val namespace = conf.get(KUBERNETES_NAMESPACE)
+ new ConfigMapBuilder()
+ .withNewMetadata()
+ .withName(configMapName)
+ .withNamespace(namespace)
+ .endMetadata()
+ .addToData(SPARK_CONF_FILE_NAME, propertiesWriter.toString + driverJavaOps)
--- End diff --
We should probably also capture this with an integration test. Some test that has a call to `System.getProperty(property)` with a property that isn't a Spark-specific option should suffice.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r174328964
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala ---
@@ -202,6 +221,10 @@ private[spark] class KubernetesClientApplication extends SparkApplication {
val launchTime = System.currentTimeMillis()
val waitForAppCompletion = sparkConf.get(WAIT_FOR_APP_COMPLETION)
val appName = sparkConf.getOption("spark.app.name").getOrElse("spark")
+ val kubernetesResourceNamePrefix = {
+ val uuid = UUID.nameUUIDFromBytes(Longs.toByteArray(launchTime)).toString.replaceAll("-", "")
--- End diff --
Heh, UUIDs aren't great here because they tend to be very long. There's lots of inconvenient name length restrictions in Kubernetes. That's why we were just using the launch time before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by erikerlandson <gi...@git.apache.org>.
Github user erikerlandson commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175532438
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
as @mccheah mentioned, I included some logic on the current entrypoint.sh to allow Spark to work in cases such as an anonymous uid (another way to look at it is managing Spark's long-standing quirk of failing when it can't find a passwd entry). Putting it in entrypoint.sh was a good way to make sure this happened regardless of how the actual CMD evolved. A kind of defensive future-proofing, which is important for reference Dockerfiles. It also provides execution via `tini` as "pid 1", which is considered good standard practice.
All of this is done in part with the expectation that _most_ users are liable to just want to customize their images by building the reference dockerfiles and using those as base images for their own, without modifying the CMD or entrypoint.sh
That said, I think that in terms of formally documenting a container API, entrypoint.sh may be a red herring. In theory, a user _should_ be able to build their own custom container from the ground up, up to and including a different entrypoint, or default entrypoint, etc.
Part of the reason we went with an externalized CMD (instead of creating one in the backend code) was to allow maximum flexibility in how images were constructed. The back-end provides certain information to the pod. The "API" is a catalogue of this information, combined with any behaviors that the user's container _must_ implement. API doc shouldn't assume the existence of entrypoint.sh
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1027/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20669
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-spark-integration/1027/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20669: [SPARK-22839][K8S] Remove the use of init-container for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1033/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175237767
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
There's an argument that can be made for enforcing what arguments can be passed here. For example, we can instead have the following command:
```
"$SPARK_HOME/bin/spark-submit"
--conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
--deploy-mode client
--properties-file $SPARK_DRIVER_PROPERTIES_FILE
--class $SPARK_DRIVER_MAIN_CLASS
spark-internal
```
And then instead of passing args to the Kubernetes container command, we pass everything as environment variables. I think this is clearer and makes the contract more obvious, in terms of what this driver container is expecting as input. Thoughts @vanzin @ifilonenko?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20669: [SPARK-22839][K8S] Remove the use of init-contain...
Posted by ifilonenko <gi...@git.apache.org>.
Github user ifilonenko commented on a diff in the pull request:
https://github.com/apache/spark/pull/20669#discussion_r175262319
--- Diff: resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -53,14 +53,10 @@ fi
case "$SPARK_K8S_CMD" in
driver)
CMD=(
- ${JAVA_HOME}/bin/java
- "${SPARK_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
+ "$SPARK_HOME/bin/spark-submit"
+ --conf "spark.driver.bindAddress=$SPARK_DRIVER_BIND_ADDRESS"
+ --deploy-mode client
+ "$@"
--- End diff --
+1 on explicit envs.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org