[SPARK-4137] [EC2] Don't change working dir on user #2988

nchammas · 2014-10-29T03:14:40Z

This issue was uncovered after this discussion.

Don't change the working directory on the user. This breaks relative paths the user may pass in, e.g., for the SSH identity file.

./ec2/spark-ec2 -i ../my.pem

This patch will preserve the user's current working directory and allow calls like the one above to work.

SparkQA · 2014-10-29T03:20:00Z

Test build #22420 has started for PR 2988 at commit ce071fc.

This patch merges cleanly.

SparkQA · 2014-10-29T04:42:07Z

Test build #22420 has finished for PR 2988 at commit ce071fc.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class DeferredObjectAdapter(oi: ObjectInspector) extends DeferredObject

AmplabJenkins · 2014-10-29T04:42:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22420/
Test PASSed.

nchammas · 2014-10-29T05:03:19Z

Phantom new classes again? 😠 I'll look into this tomorrow. Thought it was a resolved issue...

SparkQA · 2014-10-29T20:02:30Z

Test build #22464 has started for PR 2988 at commit 77871a2.

This patch merges cleanly.

SparkQA · 2014-10-29T20:25:01Z

Test build #22465 has started for PR 2988 at commit bcdf6a5.

This patch merges cleanly.

SparkQA · 2014-10-29T21:14:40Z

Test build #22464 has finished for PR 2988 at commit 77871a2.

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

AmplabJenkins · 2014-10-29T21:14:43Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22464/
Test PASSed.

nchammas · 2014-10-29T21:21:55Z

cc @shivaram @JoshRosen

shivaram · 2014-10-29T21:28:45Z

@nchammas does the template replacement code still work correctly ? I am referring to the "deploy.generic" dir that we pass from https://github.com/apache/spark/blob/master/ec2/spark_ec2.py#L589 to https://github.com/apache/spark/blob/master/ec2/spark_ec2.py#L726

SparkQA · 2014-10-29T21:40:04Z

Test build #22465 has finished for PR 2988 at commit bcdf6a5.

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

AmplabJenkins · 2014-10-29T21:40:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22465/
Test PASSed.

nchammas · 2014-10-29T22:08:43Z

@shivaram Oh. Glad I pinged you. :)

In my brief testing I didn't allow launch to go all the way through. This is obviously broken.

Will update this PR with an alternate approach that lets the CWD be ec2/ as it was before.

nchammas · 2014-10-29T22:27:43Z

Actually, we have a few options here for spark-ec2 and the underlying spark_ec2.py script.

We leave the script as-is and just document the fact that relative paths won't work.
We update the script to preserve the user's working directory and qualify any paths relative to that directory.
We update the script so that all relative paths passed in are translated to absolute paths.

Comments:

Option 1 is the simplest but is a bit user-unfriendly.
Option 3 is tricky since we'd have to do the translation before the Python script is invoked, and this is annoying to do in a cross-platform manner; alternately, we can pass in both the user's CWD and the untranslated paths so that Python can do the translation in a cross-platform manner.
Option 2 seems feasible, but we'd have to track down all the places where we handle files.

So far, I see that any code that handles the following files would be affected if we go with either Option 2 or 3:

This turned out to be a bit more involved than I expected...

@shivaram What would you recommend?

shivaram · 2014-10-29T23:17:36Z

Thanks for taking a closer look ! I don't know much python, but can't we get the directory that the script is in using something like __file__ and prefix that to "deploy.generic" ?

From what I can see there are two kinds of paths we have -- the first are paths supplied by the user (like the SSH identity file) where we can preserve the working directory and not change the path

The second are files relative to spark_ec2.py 's location (like deploy.generic). Making these absolute paths seems like a good idea to me.

nchammas · 2014-10-29T23:34:38Z

So basically you're saying go with option 2, right?

From what I can see, deploy.generic may be the only file we need to fix the path for. Is that right? Maybe it's not such a big deal then.

In any case, I'll give it a shot and report back.

SparkQA · 2014-10-30T00:47:28Z

Test build #22501 has started for PR 2988 at commit 752f958.

This patch merges cleanly.

SparkQA · 2014-10-30T01:57:17Z

Test build #22501 has finished for PR 2988 at commit 752f958.

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

AmplabJenkins · 2014-10-30T01:57:20Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22501/
Test PASSed.

nchammas · 2014-10-30T03:30:16Z

@shivaram I took your suggestion and tested to make sure spark-ec2 still creates a functioning EC2 cluster.

This is ready for another review.

shivaram · 2014-10-30T04:04:02Z

ec2/spark_ec2.py

Should we change this style given that other functions in this file have comments on top ? Any thoughts @JoshRosen ?

Yeah, I thought I'd make this the first change toward having all the function descriptions be in docstrings, but for consistency's sake you're right--it should be a comment on top.

@JoshRosen Any comments on this? I'd be more than happy to convert it back to being a comment.

@pwendell Perhaps you can comment on this. It's a minor issue, and I think we should be able to get this PR merged in for 1.2.

My suggestion would be to revert to the existing style for now and open a separate patch for the style fix (similar to the PEP-8 patches ?).

No problem. Will do.

@shivaram Done.

shivaram · 2014-10-30T04:04:23Z

Functionality LGTM. I left a minor style question for @JoshRosen

SparkQA · 2014-11-06T02:49:50Z

Test build #22977 has started for PR 2988 at commit fbc20c7.

This patch merges cleanly.

SparkQA · 2014-11-06T02:50:59Z

Test build #22977 has finished for PR 2988 at commit fbc20c7.

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

AmplabJenkins · 2014-11-06T02:51:00Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22977/
Test FAILed.

SparkQA · 2014-11-06T03:04:50Z

Test build #22981 has started for PR 2988 at commit f3850b5.

This patch merges cleanly.

SparkQA · 2014-11-06T04:29:34Z

Test build #22981 has finished for PR 2988 at commit f3850b5.

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

AmplabJenkins · 2014-11-06T04:29:37Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22981/
Test PASSed.

shivaram · 2014-11-06T04:46:18Z

Thanks @nchammas -- Merged this and backported to 1.2

This issue was uncovered after [this discussion](https://issues.apache.org/jira/browse/SPARK-3398?focusedCommentId=14187471&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14187471). Don't change the working directory on the user. This breaks relative paths the user may pass in, e.g., for the SSH identity file. ``` ./ec2/spark-ec2 -i ../my.pem ``` This patch will preserve the user's current working directory and allow calls like the one above to work. Author: Nicholas Chammas <[email protected]> Closes #2988 from nchammas/spark-ec2-cwd and squashes the following commits: f3850b5 [Nicholas Chammas] pep8 fix fbc20c7 [Nicholas Chammas] revert to old commenting style 752f958 [Nicholas Chammas] specify deploy.generic path absolutely bcdf6a5 [Nicholas Chammas] fix typo 77871a2 [Nicholas Chammas] add clarifying comment ce071fc [Nicholas Chammas] don't change working dir (cherry picked from commit db45f5a) Signed-off-by: Shivaram Venkataraman <[email protected]>

don't change working dir

ce071fc

add clarifying comment

77871a2

fix typo

bcdf6a5

nchammas changed the title ~~[EC2] Don't change working dir on user~~ [SPARK-4137] [EC2] Don't change working dir on user Oct 29, 2014

specify deploy.generic path absolutely

752f958

shivaram reviewed Oct 30, 2014
View reviewed changes

revert to old commenting style

fbc20c7

pep8 fix

f3850b5

asfgit closed this in db45f5a Nov 6, 2014

nchammas deleted the spark-ec2-cwd branch November 6, 2014 21:05

nchammas mentioned this pull request Dec 26, 2014

[SPARK-4501][Core] - Create build/mvn to automatically download maven/zinc/scalac #3707

Closed

[SPARK-4137] [EC2] Don't change working dir on user #2988

[SPARK-4137] [EC2] Don't change working dir on user #2988

Uh oh!

Conversation

nchammas commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 29, 2014

Uh oh!

AmplabJenkins commented Oct 29, 2014

Uh oh!

nchammas commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 29, 2014

Uh oh!

AmplabJenkins commented Oct 29, 2014

Uh oh!

nchammas commented Oct 29, 2014

Uh oh!

shivaram commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 29, 2014

Uh oh!

AmplabJenkins commented Oct 29, 2014

Uh oh!

nchammas commented Oct 29, 2014

Uh oh!

nchammas commented Oct 29, 2014

Uh oh!

shivaram commented Oct 29, 2014

Uh oh!

nchammas commented Oct 29, 2014

Uh oh!

SparkQA commented Oct 30, 2014

Uh oh!

SparkQA commented Oct 30, 2014

Uh oh!

AmplabJenkins commented Oct 30, 2014

Uh oh!

nchammas commented Oct 30, 2014

Uh oh!

shivaram Oct 30, 2014

Choose a reason for hiding this comment

Uh oh!

nchammas Oct 30, 2014

Choose a reason for hiding this comment

Uh oh!

nchammas Nov 3, 2014

Choose a reason for hiding this comment

Uh oh!

nchammas Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

shivaram Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

nchammas Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

nchammas Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

shivaram commented Oct 30, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014