[SPARK-24428][K8S] Fix unused code #21462

skonto · 2018-05-30T13:47:42Z

What changes were proposed in this pull request?

Remove code that is misleading and is a leftover from a previous implementation.

How was this patch tested?

Manually.

skonto · 2018-05-30T13:52:15Z

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

In general I would like to refactor the name here of SPARK_JAVA_OPTS, its a bit misleading as this property used to exist in Spark and was removed: https://issues.apache.org/jira/browse/SPARK-14453

it would be confusing -- this is local though right, not exported?

@felixcheung yes it is local, but for users not to make assumptions it would be good to rename it at some point.

SparkQA · 2018-05-30T14:08:58Z

Test build #91305 has finished for PR 21462 at commit 74e9011.

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

SparkQA · 2018-05-30T14:09:22Z

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

SparkQA · 2018-05-30T14:14:27Z

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

skonto · 2018-05-30T20:56:46Z

@foxish pls review.

foxish · 2018-05-30T21:06:45Z

jenkins, ok to test

SparkQA · 2018-05-30T21:22:08Z

Test build #91312 has finished for PR 21462 at commit 74e9011.

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

SparkQA · 2018-05-30T21:30:10Z

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

SparkQA · 2018-05-30T21:35:17Z

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

foxish · 2018-05-30T21:33:36Z

docs/running-on-kubernetes.md

Did we remove the ability to put jars somewhere in the image and add them to the classpath at runtime? We had this before with #20193.

cc @mccheah

We should confirm that the new init-container-less implementation retains the capability of doing that - maybe through an e2e test.

@foxish I can add that capability, but as far as I can tell this is not there now. I was looking for that but SPARK_EXTRA_CLASSPATH I am afraid is not used. In addition since we are using client mode --driver-class-path should be used here. In general all options in the common config doc should we functional.

Should be able to specify dependencies with the URI local://. However in order for the image itself to specify additional jars without having to list them in spark-submit, you have to put the jars in the same directory as the rest of the Spark distribution jars. It would be good to have an API or environment variable that can point to additional jars.

@mccheah yes putting under /opt/spark/jars is one option because its included by default in the classpath. I will add then SPARK_EXTRA_CLASSPATH back and will let the jars there added to the spark-submit in the container, sounds good?
I opened another ticker for the driver's extra java options here.

skonto · 2018-06-08T08:09:39Z

...es/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterManager.scala

It does not create any side effects so it can be removed.

Do we not have secrets -> mountpaths support right now? @mccheah @liyinan926

@foxish you are right we do have, but this statement has no effect. For example ti does not modify sparkConf.
It was used in the past by the following statement that was removed:

- val mountSecretBootstrap = if (executorSecretNamesToMountPaths.nonEmpty) { - Some(new MountSecretsBootstrap(executorSecretNamesToMountPaths)) - } else { - None - }

Here is the relevant PR:
a83ae0d#diff-7d9979c0153744eafa24348ecbfa1671

@mccheah @liyinan926 any more to this?

This might be a bug, shouldn't we just be mounting the secrets here?

@mccheah @felixcheung I think the logic has changed otherwise the tests I have in this PR(#21652) would have failed when removed that part and re-run them. Also they should have failed anyway if there was a bug. Tests there cover mounted secrets.

SparkQA · 2018-06-08T08:21:58Z

Test build #91560 has finished for PR 21462 at commit f8635fd.

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

skonto · 2018-06-08T08:23:13Z

@foxish pls review. It is quite short :)

skonto · 2018-06-08T08:23:55Z

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

-  cp -R "$SPARK_MOUNTED_FILES_DIR/." .
+readarray -t SPARK_EXECUTOR_JAVA_OPTS < /tmp/java_opts.txt
+
+if [ -n "$SPARK_EXTRA_CLASSPATH" ]; then


This var exists in docs but not in code.

skonto · 2018-06-08T08:24:13Z

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

-fi
-if [ -n "$SPARK_MOUNTED_FILES_DIR" ]; then
-  cp -R "$SPARK_MOUNTED_FILES_DIR/." .
+readarray -t SPARK_EXECUTOR_JAVA_OPTS < /tmp/java_opts.txt


Rename for historical reasons.

this is local right? shouldn't matter what the name is. also this might be an image running the driver, not an executor?

@felixcheung I believe this is because we are running spark-submit from driver. And so the JAVA_OPTS are now only applicable to the executor.
I +1 this change, but would like weigh-in from @foxish

Yeah spark-submit detects whatever it needs via the spark-launcher.

+1, that makes sense. Those env-vars are set by launcher at submission time.

SparkQA · 2018-06-08T08:32:08Z

Test build #91562 has finished for PR 21462 at commit 9f1ddf3.

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

SparkQA · 2018-06-08T08:45:44Z

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

SparkQA · 2018-06-08T08:51:19Z

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

skonto · 2018-06-09T19:06:53Z

@felixcheung @mccheah pls review.

skonto · 2018-06-12T09:06:11Z

@foxish gentle ping.

skonto · 2018-06-14T12:46:34Z

@felixcheung gentle ping.

felixcheung

I think ok, but someone else should review entrypoint.sh

felixcheung · 2018-06-15T05:57:25Z

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

-fi
-if [ -n "$SPARK_MOUNTED_FILES_DIR" ]; then
-  cp -R "$SPARK_MOUNTED_FILES_DIR/." .
+readarray -t SPARK_EXECUTOR_JAVA_OPTS < /tmp/java_opts.txt


this is local right? shouldn't matter what the name is. also this might be an image running the driver, not an executor?

skonto · 2018-06-19T12:04:26Z

@felixcheung its local.

skonto · 2018-06-20T22:27:06Z

@foxish gentle ping.

foxish

Just a couple of questions.
Change LGTM, thanks.

foxish · 2018-06-25T19:08:49Z

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

-fi
-if [ -n "$SPARK_MOUNTED_FILES_DIR" ]; then
-  cp -R "$SPARK_MOUNTED_FILES_DIR/." .
+readarray -t SPARK_EXECUTOR_JAVA_OPTS < /tmp/java_opts.txt


+1, that makes sense. Those env-vars are set by launcher at submission time.

foxish · 2018-06-25T19:09:43Z

...es/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterManager.scala

Do we not have secrets -> mountpaths support right now? @mccheah @liyinan926

skonto · 2018-06-26T20:22:07Z

Thank you @foxish I was also thinking of adding the following:

 if [ -n "$HADOOP_CONFIG_URL" ]; then
     echo "Setting up hadoop config files...."
     mkdir -p /etc/hadoop/conf
     wget $HADOOP_CONFIG_URL/core-site.xml -P /etc/hadoop/conf 
     wget $HADOOP_CONFIG_URL/hdfs-site.xml -P /etc/hadoop/conf
     export HADOOP_CONF_DIR=/etc/hadoop/conf
 fi

I have already tested it with: org.apache.spark.examples.DFSReadWriteTest
At least we get some basic support for integrating with an external hdfs cluster. Are you ok with adding that small ode snippet (I know that hdfs/kerberos hdfs support will come, still this might be useful since it allows to fetch the config files from any place)? Something similar we use for Spark on DC/OS here.

ifilonenko · 2018-06-27T00:04:23Z

@skonto can we hold off on that until I merge Kerberos support? It is in the works :)

skonto · 2018-06-28T20:04:56Z

@foxish @felixcheung gentle ping for merge. I replied for mount paths.

felixcheung · 2018-06-29T05:55:24Z

ping @mccheah @liyinan926 on #21462 (comment)

foxish · 2018-07-02T20:05:35Z

LGTM, we can file a separate task to track what happened to secrets mounting without blocking on removing dead code.

Thanks for the fix @skonto.

foxish · 2018-07-02T20:05:42Z

Merging to master

skonto commented May 30, 2018

View reviewed changes

foxish reviewed May 30, 2018

View reviewed changes

skonto force-pushed the fix-k8s-docs branch from 74e9011 to f8635fd Compare June 8, 2018 08:07

skonto commented Jun 8, 2018

View reviewed changes

fix unused code

9f1ddf3

skonto force-pushed the fix-k8s-docs branch from f8635fd to 9f1ddf3 Compare June 8, 2018 08:15

skonto commented Jun 8, 2018

View reviewed changes

felixcheung reviewed Jun 15, 2018

View reviewed changes

foxish reviewed Jun 25, 2018

View reviewed changes

asfgit closed this in 85fe129 Jul 2, 2018

echarles mentioned this pull request Jul 10, 2018

[SPARK-23146][WIP] Support client mode for Kubernetes in Out-Cluster mode #20451

Closed

[SPARK-24428][K8S] Fix unused code #21462

[SPARK-24428][K8S] Fix unused code #21462

Uh oh!

Conversation

skonto commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 30, 2018

Uh oh!

SparkQA commented May 30, 2018

Uh oh!

SparkQA commented May 30, 2018

Uh oh!

skonto commented May 30, 2018

Uh oh!

foxish commented May 30, 2018

Uh oh!

SparkQA commented May 30, 2018

Uh oh!

SparkQA commented May 30, 2018

Uh oh!

SparkQA commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 8, 2018

Uh oh!

skonto commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 8, 2018

Uh oh!

SparkQA commented Jun 8, 2018

skonto commented May 30, 2018 •

edited

Loading

skonto May 30, 2018 •

edited

Loading

skonto May 31, 2018 •

edited

Loading

skonto Jun 28, 2018 •

edited

Loading

skonto Jun 30, 2018 •

edited

Loading

skonto commented Jun 26, 2018 •

edited

Loading