[SPARK-2718] [yarn] Handle quotes and other characters in user args. #1724

vanzin · 2014-08-01T23:06:59Z

Due to the way Yarn runs things through bash, normal quoting doesn't
work as expected. This change applies the necessary voodoo to the user
args to avoid issues with bash and special characters.

The change also uncovered an issue with the event logger app name
sanitizing code; it wasn't cleaning up all "bad" characters, so
sometimes it would fail to create the log dirs. I just added some
more bad character replacements.

Due to the way Yarn runs things through bash, normal quoting doesn't work as expected. This change applies the necessary voodoo to the user args to avoid issues with bash and special characters. The change also uncovered an issue with the event logger app name sanitizing code; it wasn't cleaning up all "bad" characters, so sometimes it would fail to create the log dirs. I just added some more bad character replacements.

SparkQA · 2014-08-01T23:09:46Z

QA tests have started for PR 1724. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17713/consoleFull

vanzin · 2014-08-01T23:09:52Z

Screenshot of RM app list:

Mental note: always test both.

SparkQA · 2014-08-02T00:21:54Z

QA results for PR 1724:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17713/consoleFull

andrewor14 · 2014-08-04T22:00:06Z

Does this work with backslashes as well?

vanzin · 2014-08-04T23:05:39Z

Backslashes seem to work fine. Added test to be sure.

SparkQA · 2014-08-04T23:09:17Z

QA tests have started for PR 1724. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17881/consoleFull

SparkQA · 2014-08-05T00:08:19Z

QA results for PR 1724:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17881/consoleFull

andrewor14 · 2014-08-06T03:36:52Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala

+        arg.charAt(i) match {
+          case '$' => escaped.append("\\$")
+          case '"' => escaped.append("\\\"")
+          case '\'' => escaped.append("'\\\"'\\\"'")


I'm trying to parse this in my head, looks like a single quote becomes this: '\"'\"'. Could you explain this? It looks like we're wrapping a single quote in a pair of double quotes in a pair of single quotes.

Yeah, this is the tricky one. Escaping single quotes inside a single-quoted string does not work.

So what you have to do is close the previous string (remember the whole thing is wrapped in single quotes), and start a new string, delimited by double quotes, with a single single quote in it. So basically, for a string containing a single single quote, you're concatenating three different strings:

An empty string at the start: ''

The single quote, wrapped in double quotes: "'"

An empty string at the end: ''

And since all this is already inside double quotes in the bash script itself, you need to also escape the double quotes. Fun.

I guess a different approach could be:

'\\''

(i.e., same string-concatenating trick, but escaping the single quote instead of wrapping it in double quotes) A little more readable, maybe.

Ah I see. Yeah that looks simpler

andrewor14 · 2014-08-06T03:40:03Z

test this please

SparkQA · 2014-08-06T03:44:35Z

QA tests have started for PR 1724. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17987/consoleFull

SparkQA · 2014-08-06T04:37:12Z

QA results for PR 1724:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17987/consoleFull

SparkQA · 2014-08-06T23:09:23Z

QA tests have started for PR 1724. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18073/consoleFull

SparkQA · 2014-08-06T23:53:58Z

QA results for PR 1724:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18073/consoleFull

andrewor14 · 2014-08-06T23:56:37Z

failed tests are not related, test this please

andrewor14 · 2014-08-08T21:09:39Z

test this please...

pwendell · 2014-08-09T06:13:42Z

Jenkins, test this please.

SparkQA · 2014-08-09T06:19:36Z

QA tests have started for PR 1724. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18245/consoleFull

SparkQA · 2014-08-09T07:02:46Z

QA results for PR 1724:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18245/consoleFull

andrewor14 · 2014-08-09T07:38:07Z

Failed test is a flaky test that has been failing in many other PRs. This LGTM.

vanzin · 2014-08-14T16:24:04Z

Jenkins retest this please.

SparkQA · 2014-08-14T16:29:58Z

QA tests have started for PR 1724. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18544/consoleFull

SparkQA · 2014-08-14T17:21:52Z

QA results for PR 1724:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18544/consoleFull

vanzin · 2014-08-18T16:50:32Z

Ping.

andrewor14 · 2014-08-18T16:55:22Z

@vanzin I'm going to quickly test this on a yarn cluster first, and then I'll merge it.

andrewor14 · 2014-08-18T21:11:19Z

Thanks, I've merged this into master and 1.1

Due to the way Yarn runs things through bash, normal quoting doesn't work as expected. This change applies the necessary voodoo to the user args to avoid issues with bash and special characters. The change also uncovered an issue with the event logger app name sanitizing code; it wasn't cleaning up all "bad" characters, so sometimes it would fail to create the log dirs. I just added some more bad character replacements. Author: Marcelo Vanzin <vanzin@cloudera.com> Closes #1724 from vanzin/SPARK-2718 and squashes the following commits: cc84b89 [Marcelo Vanzin] Review feedback. c1a257a [Marcelo Vanzin] Add test for backslashes. 55571d4 [Marcelo Vanzin] Unbreak yarn-client. 515613d [Marcelo Vanzin] [SPARK-2718] [yarn] Handle quotes and other characters in user args. (cherry picked from commit 6201b27) Signed-off-by: Andrew Or <andrewor14@gmail.com>

Due to the way Yarn runs things through bash, normal quoting doesn't work as expected. This change applies the necessary voodoo to the user args to avoid issues with bash and special characters. The change also uncovered an issue with the event logger app name sanitizing code; it wasn't cleaning up all "bad" characters, so sometimes it would fail to create the log dirs. I just added some more bad character replacements. Author: Marcelo Vanzin <vanzin@cloudera.com> Closes apache#1724 from vanzin/SPARK-2718 and squashes the following commits: cc84b89 [Marcelo Vanzin] Review feedback. c1a257a [Marcelo Vanzin] Add test for backslashes. 55571d4 [Marcelo Vanzin] Unbreak yarn-client. 515613d [Marcelo Vanzin] [SPARK-2718] [yarn] Handle quotes and other characters in user args.

Unbreak yarn-client.

55571d4

Mental note: always test both.

Add test for backslashes.

c1a257a

andrewor14 reviewed Aug 6, 2014
View reviewed changes

Review feedback.

cc84b89

vanzin mentioned this pull request Aug 7, 2014

[SPARK-2889] Create Hadoop config objects consistently. #1843

Closed

asfgit closed this in 6201b27 Aug 18, 2014

vanzin deleted the SPARK-2718 branch August 18, 2014 21:19

[SPARK-2718] [yarn] Handle quotes and other characters in user args. #1724

[SPARK-2718] [yarn] Handle quotes and other characters in user args. #1724

Uh oh!

Conversation

vanzin commented Aug 1, 2014

Uh oh!

SparkQA commented Aug 1, 2014

Uh oh!

vanzin commented Aug 1, 2014

Uh oh!

SparkQA commented Aug 2, 2014

Uh oh!

andrewor14 commented Aug 4, 2014

Uh oh!

vanzin commented Aug 4, 2014

Uh oh!

SparkQA commented Aug 4, 2014

Uh oh!

SparkQA commented Aug 5, 2014

Uh oh!

andrewor14 Aug 6, 2014

Choose a reason for hiding this comment

Uh oh!

vanzin Aug 6, 2014

Choose a reason for hiding this comment

Uh oh!

vanzin Aug 6, 2014

Choose a reason for hiding this comment

Uh oh!

andrewor14 Aug 6, 2014

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

andrewor14 commented Aug 6, 2014

Uh oh!

andrewor14 commented Aug 8, 2014

Uh oh!

pwendell commented Aug 9, 2014

Uh oh!

SparkQA commented Aug 9, 2014

Uh oh!

SparkQA commented Aug 9, 2014

Uh oh!

andrewor14 commented Aug 9, 2014

Uh oh!

vanzin commented Aug 14, 2014

Uh oh!

SparkQA commented Aug 14, 2014

Uh oh!

SparkQA commented Aug 14, 2014

Uh oh!

vanzin commented Aug 18, 2014

Uh oh!

andrewor14 commented Aug 18, 2014

Uh oh!

andrewor14 commented Aug 18, 2014

Uh oh!

Uh oh!