[SPARK-24446][yarn] Properly quote library path for YARN. #21476
Conversation
Because the way YARN executes commands via bash -c, everything needs to be quoted so that the whole command is fully contained inside a bash string and is interpreted correctly when the string is read by bash. This is a bit different than the quoting done when executing things as if typing in a bash shell. Tweaked unit tests to exercise the bad behavior, which would cause existing tests to time out without the fix. Also tested on a real cluster, verifying the shell script created by YARN to run the container.
|
Test build #91370 has finished for PR 21476 at commit
|
| * Create a properly quoted library path string to be added as a prefix to the command executed by | ||
| * YARN. This is different from plain quoting due to YARN executing the command through "bash -c". | ||
| */ | ||
| def createLibraryPathPrefix(libpath: String, conf: SparkConf): String = { |
There was a problem hiding this comment.
maybe pull this into a util class and have unit tests?
There was a problem hiding this comment.
This is so specific to the way YARN runs things that I don't think it would be useful anywhere else. If at some point it becomes useful, the code can be moved.
I think the tests I added are better than just unit testing this function, since that way the code is actually being run through YARN and bash.
| val envName = Utils.libraryPathEnvName | ||
| // For quotes, escape both the quote and the escape character when encoding in the command | ||
| // string. | ||
| val quoted = libpath.replace("\"", "\\\\\\\"") |
There was a problem hiding this comment.
Dumb question i think escaping """ => "\"". Not sure why we have so many escapes otherwise. Trying to understand, else PR looks good
There was a problem hiding this comment.
This needs to be double escaped because it goes through two rounds of bash interpreting the value.
|
Test build #91454 has finished for PR 21476 at commit
|
|
If there are no more comments I plan to push this next week. |
|
retest this please |
|
Test build #92388 has finished for PR 21476 at commit
|
|
Merging to master. |
Because the way YARN executes commands via bash -c, everything needs
to be quoted so that the whole command is fully contained inside a
bash string and is interpreted correctly when the string is read by
bash. This is a bit different than the quoting done when executing
things as if typing in a bash shell.
Tweaked unit tests to exercise the bad behavior, which would cause
existing tests to time out without the fix. Also tested on a real
cluster, verifying the shell script created by YARN to run the
container.