[SPARK-19740][MESOS]Add support in Spark to pass arbitrary parameters into docker when running on mesos with docker containerizer #17109

yanji84 · 2017-03-01T02:04:51Z

What changes were proposed in this pull request?

Allow passing in arbitrary parameters into docker when launching spark executors on mesos with docker containerizer @tnachen

How was this patch tested?

Manually built and tested with passed in parameter

…tainerizer

tnachen · 2017-03-01T02:35:25Z

ok to test

tnachen · 2017-03-01T02:35:35Z

@yanji84 can you add a test for this?

SparkQA · 2017-03-01T02:48:41Z

Test build #73645 has finished for PR 17109 at commit ecb7a8e.

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

yanji84 · 2017-03-01T18:15:15Z

@tnachen I will add test

SparkQA · 2017-03-05T18:27:44Z

Test build #73937 has finished for PR 17109 at commit 4087936.

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

SparkQA · 2017-03-05T18:44:12Z

Test build #73938 has finished for PR 17109 at commit 54f9ec8.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class MesosSchedulerBackendUtilSuite extends SparkFunSuite with Matchers with MockitoSugar

yanji84 · 2017-03-05T22:37:11Z

@tnachen test ready

SparkQA · 2017-03-05T22:39:22Z

Test build #73942 has finished for PR 17109 at commit 423dfa8.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-05T23:02:50Z

Test build #73944 has finished for PR 17109 at commit 0696d4f.

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

yanji84 · 2017-03-10T02:52:39Z

@tnachen does the PR look good? Thanks!

tnachen · 2017-03-10T02:58:53Z

Hey sorry for the late response, code looks good to me, however we need to add documentation about the new flag. Can you modify the Mesos configuration docs in the docs folder?

SparkQA · 2017-03-10T06:07:55Z

Test build #74299 has finished for PR 17109 at commit 03e89eb.

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

tnachen · 2017-03-10T06:42:41Z

@srowen @mgummelt PTAL

mgummelt · 2017-03-10T18:22:27Z

docs/running-on-mesos.md

+<tr>
+  <td><code>spark.mesos.executor.docker.params</code></td>
+  <td>(none)</td>
+  <td>


s/mesos/Mesos

s/docker containerizer/the docker containerizer

mgummelt · 2017-03-10T18:22:56Z

docs/running-on-mesos.md

+  <td><code>spark.mesos.executor.docker.params</code></td>
+  <td>(none)</td>
+  <td>
+    Set the list of custom parameters which will be passed into the <code>docker run</code> command when launching the Spark executor on mesos using docker containerizer. The format of this property is a comma-separated list of


s/That is they take the form:/Example:

mgummelt · 2017-03-10T18:23:32Z

docs/running-on-mesos.md

    By default Mesos agents will not pull images they already have cached.
  </td>
 </tr>
+<tr>


s/params/parameters

to be consistent with the Mesos protobuf terminology

mgummelt · 2017-03-10T18:26:10Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+   * takes the form key=value
+   */
+  def parseParamsSpec(params: String): List[Parameter] = {
+    params.split(",").map(_.split("=")).flatMap { kv: Array[String] =>


s/kv/parameter for naming consistency

mgummelt · 2017-03-10T18:26:15Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+   * Parse a comma-delimited list of arbitrary parameters, each of which
+   * takes the form key=value
+   */
+  def parseParamsSpec(params: String): List[Parameter] = {


mgummelt · 2017-03-10T18:26:24Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

  }

+  /**
+   * Parse a comma-delimited list of arbitrary parameters, each of which


"list of docker parameters"

mgummelt · 2017-03-10T18:35:52Z

...src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtilSuite.scala

+
+  test("Parse arbitrary parameter to pass into docker containerizer") {
+    val parsed = MesosSchedulerBackendUtil.parseParamsSpec("a=1,b=2,c=3")
+    parsed(0).getKey shouldBe "a"


check out the other test suites. We use assert, not Matchers

mgummelt · 2017-03-10T18:36:49Z

...src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtilSuite.scala

+    parsed(2).getKey shouldBe "c"
+    parsed(2).getValue shouldBe "3"
+
+    val invalid = MesosSchedulerBackendUtil.parseParamsSpec("a,b")


invalid entries should be in a separate test

mgummelt · 2017-03-10T18:37:14Z

...src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtilSuite.scala

+class MesosSchedulerBackendUtilSuite extends SparkFunSuite with Matchers with MockitoSugar {
+
+  test("Parse arbitrary parameter to pass into docker containerizer") {
+    val parsed = MesosSchedulerBackendUtil.parseParamsSpec("a=1,b=2,c=3")


This should be a private method, so let's not test it. Just test containerInfo

SparkQA · 2017-03-12T02:32:50Z

Test build #74392 has finished for PR 17109 at commit 737acf0.

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

yanji84 · 2017-03-12T03:36:20Z

@mgummelt comments addressed, please take another look

mgummelt · 2017-03-13T17:40:23Z

...esos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala

+   * takes the form key=value
+   */
+  private def parseParamsSpec(params: String): List[Parameter] = {
+    params.split(",").map(_.split("=")).flatMap { spec: Array[String] =>


s/spec/parameter, for consistency

to be consistent with other methods, i think we should stick with spec.

def parsePortMappingsSpec(portmaps: String): List[DockerInfo.PortMapping] = {
portmaps.split(",").map(_.split(":")).flatMap { spec: Array[String] =>
val portmap: DockerInfo.PortMapping.Builder = DockerInfo.PortMapping

it seems to be better to reserve the name parameter later for builder

val param: Parameter.Builder = Parameter.newBuilder()

I am not very familiar with docker parameters - but wont this method not fail if '=' exists in the value ?
Or is that prohibited by docker ?

hmm if a value contains a '=' we will have a parsing error.

= in values seems valid for env vars: moby/moby#12763

I see, so we should split with a limit instead.
@yanji84 can you fix this?

sorry missed out this comment earlier, Ii set limit to 2, pushed the new pull request

mgummelt · 2017-03-13T17:42:58Z

...src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtilSuite.scala

+
+import org.apache.spark.{SparkConf, SparkFunSuite}
+
+class MesosSchedulerBackendUtilSuite extends SparkFunSuite with Matchers with MockitoSugar {


Is Matchers used anymore?

i'll check, if not, will remove it

SparkQA · 2017-03-14T05:43:17Z

Test build #74488 has finished for PR 17109 at commit cbb784a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class MesosSchedulerBackendUtilSuite extends SparkFunSuite

mgummelt · 2017-03-14T20:26:34Z

LGTM @srowen ready to merge

yanji84 · 2017-03-18T02:13:07Z

hello, any update on this?

yanji84 · 2017-04-11T18:34:13Z

@srowen is there anything else holding this up? why does it take so long? thanks

srowen · 2017-04-11T20:04:38Z

@yanji84 I have no experience with Mesos and am not, in general, a reviewer for this code, and don't follow Mesos changes. That's why. I pitch in to help merge but would appreciate others stepping up to help.

mgummelt · 2017-04-11T20:19:41Z

@srowen We do appreciate your help with Mesos commits, and generally find you responsive. I have a habit of pinging you for merges because you seemed to have stepped in once @andrewor14 stepped away. There are really only two active Spark contributers with knowledge of Mesos code, being myself and @skonto. We would obviously love it if one of us could be made a committer so we could more effectively maintain the Mesos module. It's something @rxin emailed me about previously, but there was some concern about my total code volume. I'll be upstreaming a sizable feature this week (Kerberos support on Mesos). Maybe that, along with my 1.5 year history of responsive code reviews and contributions, would push me over the edge?

I'd be more than happy to take this maintenance burden away from you :)

tnachen · 2017-04-12T01:06:55Z

@srowen Appreciate the help you're doing, I think we're doing what we can to help review these patches and making sure Mesos support is still being maintained and improved over time.
If you trust our judgement and also us still around fixing issues when arises, then we really just need someone like you to help merge patches.
Ensuring someone else or if anyone that's been contributing to this area can become a committer definitely is a ever ongoing problem that we're still hoping one day can be addressed. Another parallel effort that I think is very worth investigating is to decouple the cluster manager intergation from Spark, which I believe is becoming more relevant now as we have more integration coming.

Long story short, if you can still help in the mean time will be greatly appreciated as we can still make sure improvements around Mesos integration can still happen

srowen · 2017-04-15T14:32:53Z

Is this one still good to go? I see a late comment above. I can merge it, no problem.

SparkQA · 2017-04-15T19:39:40Z

Test build #75828 has finished for PR 17109 at commit 2f3f8b2.

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

yanji84 · 2017-04-16T02:33:27Z

@srowen @tnachen confirm merge

srowen · 2017-04-16T13:34:36Z

Merged to master

…s into docker when running on mesos with docker containerizer ## What changes were proposed in this pull request? Allow passing in arbitrary parameters into docker when launching spark executors on mesos with docker containerizer tnachen ## How was this patch tested? Manually built and tested with passed in parameter Author: Ji Yan <jiyan@Jis-MacBook-Air.local> Closes apache#17109 from yanji84/ji/allow_set_docker_user.

Ji Yan added 4 commits February 25, 2017 14:36

allow setting docker user when running spark on mesos with docker con…

4f8368e

…tainerizer

allow arbitrary parameters to pass to docker through spark conf

bba57f9

add some debug prints

ae30e23

remove debug print

ecb7a8e

yanji84 changed the title ~~[SPARK-19740][MESOS]~~ [SPARK-19740][MESOS]Add support in Spark to pass arbitrary parameters into docker when running on mesos with docker containerizer Mar 1, 2017

add test

4087936

add MesosSchedulerBackendUtilSuite.scala

54f9ec8

add newline

423dfa8

add empty line

0696d4f

add documentation

03e89eb

mgummelt reviewed Mar 10, 2017

View reviewed changes

Ji Yan added 2 commits March 11, 2017 18:15

address comments

3417721

address comment/syntax fix

737acf0

mgummelt reviewed Mar 13, 2017

View reviewed changes

remove unnecessary interface from test

cbb784a

guard against having equality sign inside the parameter value

2f3f8b2

asfgit closed this in a888fed Apr 16, 2017


		import org.apache.spark.{SparkConf, SparkFunSuite}

		class MesosSchedulerBackendUtilSuite extends SparkFunSuite with Matchers with MockitoSugar {

[SPARK-19740][MESOS]Add support in Spark to pass arbitrary parameters into docker when running on mesos with docker containerizer #17109

[SPARK-19740][MESOS]Add support in Spark to pass arbitrary parameters into docker when running on mesos with docker containerizer #17109

Uh oh!

Conversation

yanji84 commented Mar 1, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tnachen commented Mar 1, 2017

Uh oh!

tnachen commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

yanji84 commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 5, 2017

Uh oh!

SparkQA commented Mar 5, 2017

Uh oh!

yanji84 commented Mar 5, 2017

Uh oh!

SparkQA commented Mar 5, 2017

Uh oh!

SparkQA commented Mar 5, 2017

Uh oh!

yanji84 commented Mar 10, 2017

Uh oh!

tnachen commented Mar 10, 2017

Uh oh!

SparkQA commented Mar 10, 2017

Uh oh!

tnachen commented Mar 10, 2017

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!

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 Mar 12, 2017

Uh oh!

yanji84 commented Mar 12, 2017

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!

skonto Apr 12, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 14, 2017

Uh oh!

skonto Apr 12, 2017 •

edited

Loading

mgummelt commented Apr 11, 2017 •

edited

Loading