[SPARK-4983]insert waiting time before tagging ec2 instances

[GenTang]

As the boto API doesn't support tag ec2 instances in the same call that launches them. We use exception handling code to wait that ec2 has enough time to propagate the information.

[AmplabJenkins]

Can one of the admins verify this patch?

[nchammas]

Would it make sense to insert a small wait here? Say, 0.1 seconds. Otherwise, if it takes even a second for the information about the new instance to propagate, we might spam EC2 with requests very quickly and get rate limited.

Also, can we catch the exception specific to an instance not existing? We don't want to catch other exceptions here, right?

[GenTang]

I think that it takes some time for EC2 to return an instance not existing exception . That's why I leave pass in the exception. However, Maybe we should add a small wait time to ensure that we don't submit too much requests to ec2

Yes, Here we just want to catch the exception of instance not existing. You are right, it is better to use specific exception. I will work on this.

[nchammas]

Thanks for working on this. I left a couple of comments inline.

cc @JoshRosen

[nchammas]

By the way, please also update the title of this PR to match the approach you are taking, since as you noted we can't actually use the same call to launch and tag instances. You can leave the JIRA tag at the beginning as-is.

[GenTang]

As all the exception about ec2 will throw out an EC2ResponseError exception, we use error_code to identify the specific error of instance not existing.
If EC2 returns instance not found exception, we wait a short time to relaunch the request. Otherwise, throw out the same error.

[nchammas]

Lower-case sleep() here?

[GenTang]

Sorry for the typo.

[nchammas]

@GenTang Did you test this on a few cluster launches to make sure it works?

[GenTang]

Yes, I reproduced the InvalidInstanceID.NotFound by change the instance ID before add_tag action and then re-change it to the correct id. However, it will print the error information as follow in the screen:
ERROR:boto:400 Bad Request
ERROR:boto:
InvalidInstanceID.NotFoundThe instance ID 'i-5aa99fb1' does not exist7bfc5342-1fc9-4431-b569-190b2f9c9e8c
It is printed by boto package.

[nchammas]

Are you saying that boto will print an error to screen even if we catch the exception?

[GenTang]

However, I met a really strange error a moment ago.
I launched a cluster containing 1 master and 1 slave with the script.
Add_tag to master succussed after two tries and add_tag to slave succussed without throwing out the error. However, EC2 threw out InvalidInstanceID.NotFound error for slave node at :

for i in cluster_instances:
    i.update()

in wait_for_cluster_state function. It seems that the information of instance has not been propagated for the update action. Meantime, information of instance has reached to certain point that add_tag action can be succussed.
I tried several time, it happened only once. I am not very clear why it happened. As wait_for_cluster_state is used for launch, start(these two need more than 1 minute to reach ssh-ready state), destroy(it needs about 1 second to reach terminated state) action, maybe the workaround this to add some more waiting time to launch update action by making following change:

while True:
    time.sleep(5 * num_attempts + 1) 

at line 724

[GenTang]

Yes, boto will print the error even we catch the exception but the script will continue and the cluster will be successfully launched.
It is just ugly to have such error information in the screen.
FYI, it used boto 2.34.0

[nchammas]

Hmm, sucks that boto still prints that error.

Getting errors on i.update() seems to be part of AWS's general flakiness when propagating metadata info. The tag operations probably got servers that knew about the new instances, but the update operation got one that didn't.

It would be better if we could avoid adding arbitrary waits everywhere just to work around this problem with AWS. Is there something more direct we can do? Though honestly, maybe just adding a second inside wait_for_cluster_state() like you suggested is sufficient.

cc @JoshRosen @shivaram

[GenTang]

I understand why it still print exception error information even we catch the exception. It is because that we use

logging.basicConfig()

in the script. So the exception information will be added in the log even being catched.
Maybe it is a stupid question, why do we use logging in this script since we don't add any logging information in the script ?

@nchammas

[nchammas]

Hmm, I'm not sure. I wondered about that myself. Maybe this solution will work for us without having to turn off logging completely, though if one of the maintainers chimes in maybe we can remove that basicConfig() line entirely.

[GenTang]

Yeah, Thanks for your solution. It works.
However, I prefer to remove the logging package entirely, if we don't really use it.

[nchammas]

Agreed. @shivaram / @JoshRosen?

[shivaram]

@nchammas @GenTang - The logging.basicConfig seems to have been around since the very beginning [1]. I don't know much about Python so I can't recommend keeping it or removing it. @JoshRosen can comment on that.

Other than that this solution looks fine to me. It is unfortunate that we have so many custom sleep calls across the file, but I don't think there is much else we can do given the EC2 API we have right now.

[1] https://github.com/mesos/spark/blob/08c50ad1fcf323f62c80dfeb8f1caaf164211e0b/ec2/spark_ec2.py#L538

[nchammas]

Revisiting this now, I wonder if it would just be simpler all around to insert a 5-second sleep right here and call it day. That should give AWS enough time to get its act together.

Having the wait/retry logic all over the place seems more a burden than a benefit, honestly. What do you think?

[GenTang]

I agree with you. Since we can meet the same problem about propagating information of AWS in the wait_for_cluster_state function, I think that adding waiting time is much better workaround.
The only disadvantage is that we need 5 more seconds to launch a cluster.

[nchammas]

Well, in this case I think the 5 seconds will just come out of the time we'll spend waiting anyway for SSH to become available.

[GenTang]

Yeah, It is true. I will make a new commit as soon as possible.
I think I will add 5 seconds sleep time before tagging master and make some modification in the comment.

[GenTang]

@nchammas
Maybe we should insert a print here to tell the client that we are waiting for the information to be propagated, since there will be 5 seconds idle between launch master in and wait for the cluster to enter.
How do you think?

[nchammas]

Sure. Also, in the comments, reference SPARK-4983.

[srowen]

This seems fairly harmless and fixes a problem. Is it ready to merge as far as you guys are concerned?

[shivaram]

The latest update is simple workaround and looks good to me

[nchammas]

Nit: Please rephrase this to "Waiting for AWS to propagate instance metadata..."

[nchammas]

Left 2 minor comments about phrasing, but otherwise this LGTM.

[nchammas]

Can someone trigger tests for this, by the way?

[nchammas]

Thank you @GenTang. This LGTM.

We could probably merge this right away, but the correct thing to do is wait for tests to run. @srowen can you trigger them please?

[shivaram]

Jenkins, test this please

[SparkQA]

Test build #26927 has started for PR 3986 at commit 13e257d.

• This patch merges cleanly.

[SparkQA]

Test build #26927 has finished for PR 3986 at commit 13e257d.

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

[AmplabJenkins]

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

[nchammas]

This PR is ready to merge. So much discussion and effort ended up getting boiled down to 2 lines of code. :)

Thanks @GenTang for sticking through it. heh

[GenTang]

@nchammas You are welcome.
I am more than happy that this PR could finally be done. ^^

[JoshRosen]

Thanks for the fix. LGTM, so I'm going to merge this to master (1.4.0), branch-1.3 (1.3.0), and branch-1.2 (1.2.2).