SPARK-2333 - spark_ec2 script should allow option for existing security group

[vidaha]

- Uses the name tag to identify machines in a cluster.
- Allows overriding the security group name so it doesn't need to coincide with the cluster name.
- Outputs the request id's of up to 10 pending spot instance requests.

[AmplabJenkins]

Can one of the admins verify this patch?

[vidaha]

Just FYI - I tested this and it worked - not sure if it's controversial to use the name tag rather than the group for destroy a cluster.

[JoshRosen]

Why is it useful to have the cluster name be different from the security group prefix? If I want to re-use an existing security group, I can just name my cluster after that security group. The cluster name only seems to be used to name the security groups, instance requests, and instances.

What would happen if I launched a cluster with a name that's different from its security group, then attempted to run spark-ec2 commands by passing only the actual security group name as the cluster name? In this case, I think the instances would still be named with the cluster name, which might cause problems since the script would be expecting to find ones named after the security group.

[vidaha]

Hi Josh,

IMHO, it's best not to require a Spark cluster name and the security group to be the same. While you can reuse an existing security group to launch another cluster, you can't launch more than one cluster with the same security group. Perhaps a company wants to have an internal-applications or dev security group and reuse that for launch multiple Spark clusters. In addition, AWS has a strict limit of 100 on the number of security groups on a VPC, and since two security groups are required (one of the masters and one for the workers), this means, that only 50 Spark clusters can be launched on a VPC. While that might seem like a reasonable limit, I can easily see companies having a use case to exceed that.

Do you mind illustrating the problem about name conflicts? If I understand what you are saying, you are mentioned this scenario:

% ./spark-ec2 … —security-group my-security-group launch my-cluster-name

And then later, you also run:

% ./spark-ec2 … launch my-security-group

This works fine - I tested it - there will be two clusters with the same security group, but different names. These are some error cases that I thought might offer and tested for manually and they worked out fine:

• Can you then run ./spark-ec2 … —delete-groups destroy my-security-group and delete the security group when another cluster is using that security group?

I tried this, and amazon has the correct controls to prevent deleting a security group still in use by another cluster.

• Can you forget to include the security group override on a launched cluster and create problems?

I tried this as well, and since the get_existing_cluster code was modified to use the name rather than the security group to identify the instance, this works right. You can’t run:

% ./spark-ec2 … launch my-cluster-name

if there is already a cluster with my-cluster-name launched.

Are there some other possible conflicts that you can think of? If you can write out the commands to illustrate the use cases you are thinking of - I can run them and see what happens.

-Vida

[JoshRosen]

After a closer look, it looks like the existing spark-ec2 already names instances after cluster_name, so it's safe and backwards-compatible to use that to identify existing clusters.

The code in this PR looks good; a couple of TODOs:

• Update the pull request title to reference a new or existing JIRA. I think SPARK-2333 fits the bill, so I'd name it something like "[SPARK-2333] Allow multiple spark-ec2 clusters to be launched in the same security group".
• Maybe update the intro of the Running Spark on EC2 guide guide to explain that the clusters are identified by their instances' names instead of security groups.

[JoshRosen]

Jenkins, this is ok to test. Test this please.

[SparkQA]

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

[SparkQA]

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

[SparkQA]

QA results for PR 1899:
- 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/18376/consoleFull

[SparkQA]

QA results for PR 1899:
- 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/18379/consoleFull

[JoshRosen]

This looks good to me. @pwendell or @shivaram, do you want to take a look at this?

[shivaram]

Typo: teh -> the

[shivaram]

Some minor comments, but otherwise LGTM.

One thing that might be worth noting is that we did use tags while launching clusters for AMPCamp[1] and one issue I ran into was that sometimes the create tag command failed because of eventual consistency between the instance being allocated and the instance being tag-able.

We might want to retry the tag operation a few times since it is the only way to identify instances now.

[1] https://github.com/amplab/training-scripts/blob/ampcamp4/spark_ec2.py#L342

[SparkQA]

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

[vidaha]

Great - I made the small edits, and added a loop for retrying the tagging logic. I tested to see if I can bring up a cluster, and it went up fine - it didn't try tagging more than once. I have no way of testing the tagging retry logic though in failure case.

By the way, I'm amending my commits rather than create a new one per edit - is that standard Spark etiquette? Let me know if not.

[vanzin]

@vidaha please update the PR title and description to say what the change does. Those actually become the final commit message in git.

[SparkQA]

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

[SparkQA]

QA results for PR 1899:
- 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/18505/consoleFull

[SparkQA]

QA results for PR 1899:
- 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/18510/consoleFull

[JoshRosen]

The committers use the merge_spark_pr.py script for merging pull requests. This script will squash together all of your commits into a single commit that uses the title of the PR plus the PR's description as the commit message, so you no longer need to worry about rebasing your patch into a single commit.

I'd just copy-paste the description from your last commit into the description above so it becomes the commit message

[JoshRosen]

What happens if an add_tag call fails? My bet is that it throws an exception rather than silently failing, in which case this re-try logic won't run. Rather than using this "set-and-test" logic, maybe we can just wrap the call in a try-except block?

@shivaram Did the eventual-consistency issue that you saw result in exceptions from add_tag?

[shivaram]

Yes - I am pretty sure it throws an exception. I don't remember what the type is -- All I see in my notes is that the exception says 'Instance not found'

[SparkQA]

QA tests have started for PR 1899 at commit c80d5c3.

• This patch merges cleanly.

[vidaha]

Okay, I made the retry a try catch, and edited the title again.

[SparkQA]

QA tests have finished for PR 1899 at commit c80d5c3.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[JoshRosen]

@shivaram@pwendell This looks good to me. Does one of you want to take a final look + signoff?

[shivaram]

Minor nit: You can remove this TODO now.

[shivaram]

LGTM

[JoshRosen]

Alright, great. I'm going to merge this into master and branch-1.1.

[douglaz]

Opened an issue related to this PR: https://issues.apache.org/jira/browse/SPARK-3332

[rxin]

note that we need a break here