[SPARK-5242]: Add --private-ips flag to EC2 script

[mdagost]

The spark_ec2.py script currently references the ip_address and public_dns_name attributes of an instance. On private networks, these fields aren't set, so we have problems.

This PR introduces a --private-ips flag that instead refers to the private_ip_address attribute in both cases.

[AmplabJenkins]

Can one of the admins verify this patch?

[srowen]

You could take this all the way to implement what's described in SPARK-6220 as a more general version of this. And I think it would also be considered a resolution to SPARK-5242 and SPARK-5246. This overlaps with existing work at #4038 (see also mesos/spark-ec2#91 and mesos/spark-ec2#92) It'd be great to get a resolution to all of this since it keeps coming up in duplicate.

[mdagost]

Ooops. Didn't realize this was a duplicate. I don't think I'm going to have time in the immediate future to make a general version, but the private IP stuff is a total blocker for us.

[srowen]

@nchammas @voukka what do you think about this as a solution for SPARK-5242 and/or SPARK-5246?

[nchammas]

I'll look into this next week, but @mdagost do you mind sharing what you are using spark-ec2 for? It's always good to hear about real-life use cases from users.

[mdagost]

@nchammas At the moment, we're using Spark for large scale ALS. In the future probably other things too. And we use spark-ec2 to spin up and tear down clusters as we need them.

[mdagost]

@nchammas Did you get a chance to take a look?

[nchammas]

Nit: No possession or contraction, so "IPs" and not "IP's".

Use private instead of public IPs for instances if the VPC or subnet requires that.

[nchammas]

This is a well thought-out change. I prefer the explicit --private-ips option to the implicit address searching of #4038, but I must ask:

@mdagost Did you try the patch in #4038? Did it not meet your needs?

Also, while this change looks good, it touches a lot of code. Unfortunately, we don't have any automated test suites for spark-ec2 at this time.

In lieu of that, could you use coverage and post a report showing that you've tested most of the critical code paths?

Sorry to put this extra work on you, but that will help us merge this in with some confidence.

cc @shivaram

[voukka]

Hello @mdagost! There's a conditional logic being repeated more than 10 times. Probably you might want to extract it in a function. Maybe for that you could use something like I did in #4038. @nchammas

[voukka]

@srowen my 2 cents: I like https://issues.apache.org/jira/browse/SPARK-6220 as a general solution better as it is naturally extensible, but it is a lot less user-friendly. Probably they need to complement each other. For example, one can use --private-ips as parameter on cli, but for more options to pass one can use feature of https://issues.apache.org/jira/browse/SPARK-6220 and for cleaner cli feature of https://issues.apache.org/jira/browse/SPARK-925 could be used. I admit it looks like overkill. Still, I'd like to say that explicit parameters are easier to use.

[srowen]

@mdagost how about addressing the minor comment typo and considering wrapping up the conditional logic -- up to you whether that is worthwhile. Otherwise looking good to go.

[mdagost]

Okay. I fixed the IP's mis-spelling and refactored the conditional logic into a couple of helper functions, which should be more readable now.

@nchammas I wrote this code and submitted the PR before I realized it was a dupe, so I never tried #4038 .

In terms of testing, I've verified that this works with our AWS setup to launch clusters into private VPC's, and it works with get-master, login, and destroy. I'm not entirely sure what you want me to do with coverage. This is the output that I get:

[ mdagostino: ~/spark_mdagost/ec2 ]$ coverage report spark_ec2.py
Name        Stmts   Miss  Cover
-------------------------------
spark_ec2     677    509    25%

Do you actually want me to run the launch, get-master, login, and destroy commands and post the coverage reports for each of them?

[nchammas]

Oh, if you've tested out the relevant commands then that's great. I just wanted to know that we checked nothing was broken with this change. :)

[mdagost]

Yep. I've been using it for the last week :)

On Mon, Apr 6, 2015 at 10:45 AM, Nicholas Chammas notifications@github.com
wrote:

Oh, if you've tested out the relevant commands then that's great. I just
wanted to know that we checked nothing was broken with this change. :)

—
Reply to this email directly or view it on GitHub
#5244 (comment).

[shivaram]

Sorry I missed some of the earlier discussion, but the change looks pretty simple and seems like a useful addition. Does this also address #4038 ?

[nchammas]

Does this also address #4038 ?

Yes, I believe it does.

[srowen]

@mdagost Do you mind rebase this one? it looks good but doesn't merge at the moment.

[srowen]

Also, since this was a duplicate of SPARK-5242, edit the title please to point it at the non-duplicate JIRA.

[srowen]

ok to test

[SparkQA]

Test build #29853 has started for PR 5244 at commit a4a2eac.

[SparkQA]

Test build #29853 has finished for PR 5244 at commit a4a2eac.

• This patch fails Python style tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[AmplabJenkins]

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

[mdagost]

@srowen @nchammas Sorry about that. The lint checker should pass now.

One thing I forgot to mention yesterday: apparently someone got rid of the last place in the code where an instance was referred to by IP rather than DNS name. That means the helper function get_ip_address that I introduced isn't actually being called anywhere. I'd like to leave it in case someone needs IP in the future, but up to you guys.

[SparkQA]

Test build #29862 has started for PR 5244 at commit b684c67.

[SparkQA]

Test build #29862 has finished for PR 5244 at commit b684c67.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[AmplabJenkins]

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

[nchammas]

Looks like @voukka closed #4038 in favor of this PR?

In any case, this patch LGTM.

[voukka]

@nchammas that's correct, I closed #4038 in favor of this PR