-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-25021][K8S] Add spark.executor.pyspark.memory limit for K8S #22298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b54a039
75742a3
46c30cc
467703b
3ad1324
7dc26ce
fe8cc5a
ea25b8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ package org.apache.spark.deploy.k8s.features.bindings | |
| import io.fabric8.kubernetes.api.model.{ContainerBuilder, HasMetadata} | ||
|
|
||
| import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverSpecificConf, SparkPod} | ||
| import org.apache.spark.deploy.k8s.Config.APP_RESOURCE_TYPE | ||
| import org.apache.spark.deploy.k8s.Constants.SPARK_CONF_PATH | ||
| import org.apache.spark.deploy.k8s.features.KubernetesFeatureConfigStep | ||
| import org.apache.spark.launcher.SparkLauncher | ||
|
|
@@ -38,7 +39,8 @@ private[spark] class JavaDriverFeatureStep( | |
| .build() | ||
| SparkPod(pod.pod, withDriverArgs) | ||
| } | ||
| override def getAdditionalPodSystemProperties(): Map[String, String] = Map.empty | ||
| override def getAdditionalPodSystemProperties(): Map[String, String] = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So is this going to override it if the user wants to set a different type even if they are ostensibly running a JVM pod (e.g. mixed language pipelines).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, not sure what our direction is going to be with respect to mixed pipelines throughout the cluster manager sections - if we should be supporting it in a first class way then perhaps we file a JIRA and we can discuss how the submission client should be refactored to support that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filed SPARK-25373 - Support mixed language pipelines on Spark on K8s |
||
| Map(APP_RESOURCE_TYPE.key -> "java") | ||
|
|
||
| override def getAdditionalKubernetesResources(): Seq[HasMetadata] = Seq.empty | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import scala.collection.JavaConverters._ | |
| import io.fabric8.kubernetes.api.model.{ContainerBuilder, EnvVarBuilder, HasMetadata} | ||
|
|
||
| import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverSpecificConf, KubernetesUtils, SparkPod} | ||
| import org.apache.spark.deploy.k8s.Config.APP_RESOURCE_TYPE | ||
| import org.apache.spark.deploy.k8s.Constants._ | ||
| import org.apache.spark.deploy.k8s.features.KubernetesFeatureConfigStep | ||
|
|
||
|
|
@@ -68,7 +69,8 @@ private[spark] class PythonDriverFeatureStep( | |
|
|
||
| SparkPod(pod.pod, withPythonPrimaryContainer) | ||
| } | ||
| override def getAdditionalPodSystemProperties(): Map[String, String] = Map.empty | ||
| override def getAdditionalPodSystemProperties(): Map[String, String] = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above (just curious if this overrides user params or precedence rules here). Also add a comment of the precedence rules. |
||
| Map(APP_RESOURCE_TYPE.key -> "python") | ||
|
|
||
| override def getAdditionalKubernetesResources(): Seq[HasMetadata] = Seq.empty | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import scala.collection.JavaConverters._ | |
| import io.fabric8.kubernetes.api.model.{ContainerBuilder, EnvVarBuilder, HasMetadata} | ||
|
|
||
| import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverSpecificConf, KubernetesUtils, SparkPod} | ||
| import org.apache.spark.deploy.k8s.Config.APP_RESOURCE_TYPE | ||
| import org.apache.spark.deploy.k8s.Constants._ | ||
| import org.apache.spark.deploy.k8s.features.KubernetesFeatureConfigStep | ||
|
|
||
|
|
@@ -54,7 +55,8 @@ private[spark] class RDriverFeatureStep( | |
|
|
||
| SparkPod(pod.pod, withRPrimaryContainer) | ||
| } | ||
| override def getAdditionalPodSystemProperties(): Map[String, String] = Map.empty | ||
| override def getAdditionalPodSystemProperties(): Map[String, String] = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
| Map(APP_RESOURCE_TYPE.key -> "r") | ||
|
|
||
| override def getAdditionalKubernetesResources(): Seq[HasMetadata] = Seq.empty | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,10 +19,10 @@ ARG base_img | |
| FROM $base_img | ||
| WORKDIR / | ||
| RUN mkdir ${SPARK_HOME}/R | ||
| COPY R ${SPARK_HOME}/R | ||
|
|
||
| RUN apk add --no-cache R R-dev | ||
|
|
||
| COPY R ${SPARK_HOME}/R | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change intentional?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes the docker build not run the |
||
| ENV R_HOME /usr/lib/R | ||
|
|
||
| WORKDIR /opt/spark/work-dir | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ ARG base_img | |
| FROM $base_img | ||
| WORKDIR / | ||
| RUN mkdir ${SPARK_HOME}/python | ||
| COPY python/lib ${SPARK_HOME}/python/lib | ||
| # TODO: Investigate running both pip and pip3 via virtualenvs | ||
| RUN apk add --no-cache python && \ | ||
| apk add --no-cache python3 && \ | ||
|
|
@@ -33,6 +32,7 @@ RUN apk add --no-cache python && \ | |
| # Removed the .cache to save space | ||
| rm -r /root/.cache | ||
|
|
||
| COPY python/lib ${SPARK_HOME}/python/lib | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same, is this change intentional
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the R change above ^^ |
||
| ENV PYTHONPATH ${SPARK_HOME}/python/lib/pyspark.zip:${SPARK_HOME}/python/lib/py4j-*.zip | ||
|
|
||
| WORKDIR /opt/spark/work-dir | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,7 @@ private[spark] trait SecretsTestsSuite { k8sSuite: KubernetesSuite => | |
| .delete() | ||
| } | ||
|
|
||
| // TODO: [SPARK-25291] This test is flaky with regards to memory of executors | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mccheah This test periodically fails on setting proper memory for executors on this specific test. I have filed a JIRA: SPARK-25291 |
||
| test("Run SparkPi with env and mount secrets.", k8sTestTag) { | ||
| createTestSecret() | ||
| sparkAppConf | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this instead of the bools? What about folks who want to make a pipeline which is both R and Python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is because we are already running binding steps that configure the driver based on the app resource. I thought it might as well pass the config down into the executors upon doing that binding bootstrap step.
Currently, we don't have any docker files that handle mixed pipelines so such configurations should be addressed in a followup-PR, imo. But I am open to suggestions (that are testable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah lets do something in a follow up after 2.4