[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping #20641

krcz · 2018-02-20T15:17:17Z

What changes were proposed in this pull request?

Don't enclose --conf option value with "", as they are already escaped by shellEscape
and additional wrapping cancels out this escaping, making the driver fail to start.

This reverts commit 9b377aa [SPARK-18114].

How was this patch tested?

Manual test with driver command logging added.

Author: Marcin Kurczych [email protected]

Don't enclose --conf option value with "", as they are already escaped by shellEscape and additional wrapping cancels out this escaping, causing driver fail to start. This reverts commit 9b377aa [SPARK-18114]. Manual test with driver command logging added. Author: Marcin Kurczych <[email protected]>

susanxhuynh · 2018-02-24T22:14:04Z

Thanks for the PR! It seems that the previous attempt to fix this (SPARK-18114) was wrong -- I'm not sure why we didn't catch the problem before, maybe lack of testing? @krcz My suggestion for this patch is to add a test, in order to prevent another regression in the future. I've written a unit test for this -- you could do something similar: d2iq-archive@4812ba3 I will also do more testing with my own integration tests. cc @skonto

krcz · 2018-03-01T23:32:46Z

@susanxhuynh Thanks for the example! I have added similar test, covering more cases. After you or someone else reviews it, I'll rebase the pull request.

susanxhuynh

Thanks, @krcz . Overall looks good ... I left one question about one of the test parameters. Also, have you verified in the driver / executors that they receive the correct Java system properties (with the "-D" options)? -- I was going to test that myself.

susanxhuynh · 2018-03-05T15:24:48Z

...sos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala

+            "-XX+PrintGC -Dparam1=val1 -Dparam2=val2",
+          // special characters, to be escaped
+          "spark.executor.extraJavaOptions" ->
+            """-Dparam1="value 1" -Dparam2=value\ 2 -Dpath=$PATH"""),


I'm not sure the second param (param2) would be parsed correctly if you actually ran the command. Doesn't there need to be quotes around the space? Have you tested it and checked if the executor gets the correct value for param2?

@susanxhuynh
I have checked it and it worked. There don't need to be quotes, as the space has been escaped. Backslash stops it from being interpreted as a boundary between arguments and makes it being understood as simple space in the value. This is standard behaviour of bash (and sh) syntax.

skonto · 2018-03-20T15:13:58Z

...sos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala

@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite with LocalSparkContext wi
    })
  }

+  test("properly wraps and escapes parameters passed to driver command") {


Does this test fail with the old code?

It does.

[info] - properly wraps and escapes parameters passed to driver command *** FAILED *** (154 milliseconds) [info] "test/./bin/spark-submit --name test --master mesos://mesos://localhost:5050 --driver-cores 1.0 --driver-memory 1000M --class mainClass --conf "spark.app.name=test" --conf "spark.mesos.executor.home=tes t" --conf "spark.executor.extraJavaOptions="-Dparam1=\"value 1\" -Dparam2=value\\ 2 -Dpath=\$PATH"" --conf "spark.driver.extraJavaOptions="-XX+PrintGC -Dparam1=val1 -Dparam2=val2"" ./jar arg" did not contain "-- conf spark.driver.extraJavaOptions="-XX+PrintGC -Dparam1=val1 -Dparam2=val2"" (MesosClusterSchedulerSuite.scala:227) [info] org.scalatest.exceptions.TestFailedException: [info] at org.scalatest.Assertions$class.newAssertionFailedException(Assertions.scala:528) [info] at org.scalatest.FunSuite.newAssertionFailedException(FunSuite.scala:1560) [info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:501) [info] at org.apache.spark.scheduler.cluster.mesos.MesosClusterSchedulerSuite$$anonfun$16.apply(MesosClusterSchedulerSuite.scala:227) [info] at org.apache.spark.scheduler.cluster.mesos.MesosClusterSchedulerSuite$$anonfun$16.apply(MesosClusterSchedulerSuite.scala:202)

This looks ready to merge; anything else needed @skonto or @susanxhuynh ?

That would be great to merge it if you think it's ready.

Sorry for the delay. I was going to test this in DC/OS and haven't gotten a chance to do so.

@susanxhuynh I can do a test for this.

skonto · 2018-06-12T15:49:07Z

@susanxhuynh I tested this. Let me know if I need to test anything else. Here is what you get:

 ./bin/spark-submit --deploy-mode cluster --name test --master mesos://spark.marathon.mesos:13830 --driver-cores 1.0 --driver-memory 1000M --class org.apache.spark.examples.SparkPi --conf spark.executor.extraJavaOptions="-Dparam1=\"value 1\" -Dparam2=value\\ 2 -Dpath=\$PATH" --conf spark.mesos.containerizer=mesos  --conf spark.mesos.executor.docker.image=skonto/spark-test:test https://...jar 10000

"sparkProperties" : {
"spark.executor.extraJavaOptions" : "-Dparam1="value 1" -Dparam2=value\ 2 -Dpath=$PATH",
"spark.mesos.containerizer" : "mesos",
"spark.jars" : "....jar",
"spark.driver.supervise" : "false",
"spark.app.name" : "test",
"spark.driver.memory" : "1000M",
"spark.mesos.executor.docker.image" : "skonto/spark-test:test",
"spark.driver.cores" : "1.0",
"spark.submit.deployMode" : "cluster",
"spark.master" : "mesos://spark.marathon.mesos:13830"
}
}

At the driver side in DC/OS:

/usr/lib/jvm/jre1.8.0_152/bin/java -cp /opt/spark/dist/conf/:/opt/spark/dist/jars/*:/etc/hadoop/ -Dspark.mesos.driver.frameworkId=bddf96ce-45f6-45f7-ac83-5a3622cafc41-0030-driver-20180612154509-0007 -Xmx1000M org.apache.spark.deploy.SparkSubmit --master mesos://zk://master.mesos:2181/mesos --conf spark.driver.memory=1000M --conf spark.driver.cores=1.0 --conf spark.mesos.executor.docker.image=skonto/spark-test:test --conf spark.app.name=test --conf spark.mesos.containerizer=mesos --conf spark.executor.extraJavaOptions=-Dparam1="value 1" -Dparam2=value\ 2 -Dpath=$PATH --conf spark.driver.supervise=false --class org.apache.spark.examples.SparkPi --name test --driver-cores 1.0 /mnt/mesos/sandbox/....jar 10000

Also tried:
--conf spark.executor.extraJavaOptions="'--verbose:gc -XX:+PrintGCDetails -XX:+PrintGCTimeStamps'"

Got:

/usr/lib/jvm/jre1.8.0_152/bin/java -cp /opt/spark/dist/conf/:/opt/spark/dist/jars/*:/etc/hadoop/ -Dspark.mesos.driver.frameworkId=bddf96ce-45f6-45f7-ac83-5a3622cafc41-0030-driver-20180612155144-0008 -Xmx1000M org.apache.spark.deploy.SparkSubmit --master mesos://zk://master.mesos:2181/mesos --conf spark.driver.memory=1000M --conf spark.driver.cores=1.0 --conf spark.mesos.executor.docker.image=skonto/spark-test:test --conf spark.app.name=test --conf spark.mesos.containerizer=mesos --conf spark.executor.extraJavaOptions=--verbose:gc -XX:+PrintGCDetails -XX:+PrintGCTimeStamps --conf spark.driver.supervise=false --class org.apache.spark.examples.SparkPi --name test --driver-cores 1.0 /mnt/mesos/sandbox/....jar 10000

"sparkProperties" : {
"spark.executor.extraJavaOptions" : "'--verbose:gc -XX:+PrintGCDetails -XX:+PrintGCTimeStamps'",
"spark.mesos.containerizer" : "mesos",
"spark.jars" : ".....jar",
"spark.driver.supervise" : "false",
"spark.app.name" : "test",
"spark.driver.memory" : "1000M",
"spark.mesos.executor.docker.image" : "skonto/spark-test:test",
"spark.driver.cores" : "1.0",
"spark.submit.deployMode" : "cluster",
"spark.master" : "mesos://spark.marathon.mesos:13830"
}
@krcz Could you update the PR to remove conflicts?
@felixcheung could you help merge this one once updated?

krcz · 2018-06-12T16:58:45Z

@skonto
The command you have pasted as "At the driver side in DC/OS:" concerns me. It doesn't seem escaped properly. Is it the command to be run by shell or is it just space-joined arguments list (as in Spark Dispatcher web UI), which would explain why it is not escaped?

skonto · 2018-06-12T18:41:20Z

@krcz that is the output of ps, probably not the best tool here. Btw both jobs run successfully although might not have been escaped properly. I will check with some other tool like printing the exact commands or something plus what the process get as java properties. Spark ui shows everything as expected btw.

krcz · 2018-06-13T01:08:44Z

@skonto The one directly below "At the driver side in DC/OS:" in your comment. But if it's output of ps that's ok, as ps wouldn't be able to discern between command "foo bar" and command foo bar.

skonto · 2018-06-13T10:42:25Z

@krcz yes it is ok, yes ps has limitations. So I verified it just to be on the safe side:
Dispatcher passes the following command to mesos:

18/06/13 10:30:45 INFO MesosClusterScheduler: Command for the driver (test):./bin/spark-submit --name test --master mesos://zk://master.mesos:2181/mesos --driver-cores 1.0 --driver-memory 1000M --class org.apache.spark.examples.SparkPi --conf spark.executor.extraJavaOptions="-Dexecutor.test.param1=\"value 1\" -Dexecutor.test.param2=value\\ 2 -Dexecutor.test.path=\$PATH" --conf spark.mesos.containerizer=mesos --conf spark.driver.supervise=false --conf spark.app.name=test --conf spark.driver.memory=1000M --conf spark.mesos.executor.docker.image=skonto/spark-test:test --conf spark.driver.cores=1.0 --conf spark.driver.extraJavaOptions="-Dspark.test.param1=\"value 1\" -Dspark.test.param2=value\\ 2 -Dspark.test.path=\$PATH" $MESOS_SANDBOX/spark-examples_2.11-2.3.0.jar 10000

Spark-class will run this at the executor side:

exec /usr/lib/jvm/jre1.8.0_152/bin/java -cp '/opt/spark/dist/conf/:/opt/spark/dist/jars/*:/etc/hadoop/' '-Dexecutor.test.param1=value 1' '-Dexecutor.test.param2=value 2' '-Dexecutor.test.path=$PATH' -Xmx1024m org.apache.spark.executor.CoarseGrainedExecutorBackend --driver-url spark://[email protected]:33887 --executor-id 9 --hostname 10.0.0.77 --cores 3 --app-id bddf96ce-45f6-45f7-ac83-5a3622cafc41-0032-driver-20180613103044-0001

@susanxhuynh if there is no issue we can move on with this fix. @krcz it would be good to add more tests like having a scenario where pure GC properties are passed to make it more realistic. This is pretty common in the field: d2iq-archive/spark-build#263

For example: "'--verbose:gc -XX:+PrintGCDetails -XX:+PrintGCTimeStamps'". Btw I tested and that passes but it would be good to have such a test to avoid any regressions.

susanxhuynh · 2018-06-13T16:25:29Z

@skonto Thanks for testing it. Tests results look good.

felixcheung · 2018-06-14T06:14:44Z

ok, could you rebase this PR

felixcheung · 2018-06-14T06:14:52Z

ok to test

felixcheung · 2018-06-14T06:15:17Z

@mgummelt too

SparkQA · 2018-06-14T07:05:02Z

Test build #91820 has finished for PR 20641 at commit 22c6739.

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

krcz · 2018-06-18T09:30:08Z

@felixb @skonto
I've triend rebasing, but it looks that the issue has been meanwhile fixed independently in other pull request while someone was working on SPARK-23941 issue. It looks that there is no unit test there though, so I can modify his PR to incorporate just the test. Do you want that or should I just close the pull request?

felixb · 2018-06-18T10:46:31Z

@krcz
@felixcheung <- different felix ;)

felixcheung · 2018-06-26T06:20:47Z

@krcz how is your PR different from 21014?

krcz · 2018-06-26T11:54:24Z

@felixcheung It fixes the same problem so in terms of implementation it is not very different. But when I created this one 21014 didn't exist.

The only difference is that this PR adds an unit test, which 21014 had not. That's why I'm asking if I should modify this PR to just add the test.

felixcheung · 2018-06-26T16:43:18Z

yap, that sounds like a good idea

AmplabJenkins · 2018-11-09T17:53:30Z

Can one of the admins verify this patch?

vanzin · 2018-12-13T00:41:35Z

I'm closing this since there's been no activity. If you plan on adding the test, update your branch, and tag this with the other bug number (I duped this one to the bug with the fix attached to it).

[SPARK-23464][MESOS] Add unit test [to be squashed]

22c6739

susanxhuynh reviewed Mar 5, 2018

View reviewed changes

skonto reviewed Mar 20, 2018

View reviewed changes

samvantran mentioned this pull request Jun 22, 2018

Fix mesos cluster scheduler options double-escaping d2iq-archive/spark#32

Closed

vanzin closed this Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping #20641

[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping #20641

krcz commented Feb 20, 2018

susanxhuynh commented Feb 24, 2018

krcz commented Mar 1, 2018

susanxhuynh left a comment

susanxhuynh Mar 5, 2018

krcz Mar 12, 2018 •

edited

Loading

skonto Mar 20, 2018

krcz Mar 20, 2018

blue666man Mar 22, 2018

adyatlov Apr 17, 2018

susanxhuynh Apr 17, 2018

skonto Jun 6, 2018

skonto commented Jun 12, 2018 •

edited

Loading

krcz commented Jun 12, 2018

skonto commented Jun 12, 2018 •

edited

Loading

krcz commented Jun 13, 2018

skonto commented Jun 13, 2018 •

edited

Loading

susanxhuynh commented Jun 13, 2018

felixcheung commented Jun 14, 2018

felixcheung commented Jun 14, 2018

felixcheung commented Jun 14, 2018

SparkQA commented Jun 14, 2018

krcz commented Jun 18, 2018

felixb commented Jun 18, 2018

felixcheung commented Jun 26, 2018

krcz commented Jun 26, 2018

felixcheung commented Jun 26, 2018

AmplabJenkins commented Nov 9, 2018

vanzin commented Dec 13, 2018

[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping #20641

[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping #20641

Conversation

krcz commented Feb 20, 2018

What changes were proposed in this pull request?

How was this patch tested?

susanxhuynh commented Feb 24, 2018

krcz commented Mar 1, 2018

susanxhuynh left a comment

Choose a reason for hiding this comment

susanxhuynh Mar 5, 2018

Choose a reason for hiding this comment

krcz Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

skonto Mar 20, 2018

Choose a reason for hiding this comment

krcz Mar 20, 2018

Choose a reason for hiding this comment

blue666man Mar 22, 2018

Choose a reason for hiding this comment

adyatlov Apr 17, 2018

Choose a reason for hiding this comment

susanxhuynh Apr 17, 2018

Choose a reason for hiding this comment

skonto Jun 6, 2018

Choose a reason for hiding this comment

skonto commented Jun 12, 2018 • edited Loading

krcz commented Jun 12, 2018

skonto commented Jun 12, 2018 • edited Loading

krcz commented Jun 13, 2018

skonto commented Jun 13, 2018 • edited Loading

susanxhuynh commented Jun 13, 2018

felixcheung commented Jun 14, 2018

felixcheung commented Jun 14, 2018

felixcheung commented Jun 14, 2018

SparkQA commented Jun 14, 2018

krcz commented Jun 18, 2018

felixb commented Jun 18, 2018

felixcheung commented Jun 26, 2018

krcz commented Jun 26, 2018

felixcheung commented Jun 26, 2018

AmplabJenkins commented Nov 9, 2018

vanzin commented Dec 13, 2018

krcz Mar 12, 2018 •

edited

Loading

skonto commented Jun 12, 2018 •

edited

Loading

skonto commented Jun 12, 2018 •

edited

Loading

skonto commented Jun 13, 2018 •

edited

Loading