[SPARK-5335] Fix deletion of security groups within a VPC #4122

voukka · 2015-01-20T15:35:48Z

Please see https://issues.apache.org/jira/browse/SPARK-5335.

The fix itself is in e58a8b01a8bedcbfbbc6d04b1c1489255865cf87 commit. Two earlier commits are fixes of another VPC related bug waiting to be merged. I should have created former bug fix in own branch then this fix would not have former fixes. :(

This code is released under the project's license.

AmplabJenkins · 2015-01-20T15:37:10Z

Can one of the admins verify this patch?

nchammas · 2015-01-22T04:51:18Z

I guess review on this can wait until #4038 is merged. You will probably need to rebase this PR at that time.

nchammas · 2015-02-05T22:21:33Z

ec2/spark_ec2.py

-                            conn.delete_security_group(group.name)
-                            print "Deleted security group " + group.name
+                            conn.delete_security_group(group_id=group.id)
+                            print "Deleted security group %s, id %s" % (group.name, group.id)


Is there any place where the user inputs a group by its ID instead of its name? If not, I don't think we should print this here.

nchammas · 2015-02-05T22:27:19Z

I just ran into this issue. Nice work @voukka on catching this and fixing it.

@voukka To make this easier to merge, could you do the following?

Rebase this PR so that only this fix is included.
Link to some reference (like this) in the PR body so that people in the future can understand why this change was made.
Edit the title to match our convention. Something like: "[SPARK-5335] Fix deletion of security groups within a VPC"

nchammas · 2015-02-05T22:31:36Z

ec2/spark_ec2.py

@@ -1082,11 +1109,11 @@ def real_main():
                    time.sleep(30)  # Yes, it does have to be this long :-(
                    for group in groups:
                        try:
-                            conn.delete_security_group(group.name)
-                            print "Deleted security group " + group.name
+                            conn.delete_security_group(group_id=group.id)


I would perhaps add a very short comment saying "we must use the group ID here" so that other developers don't change this back to group.name and think it works fine because they are testing outside VPCs.

srowen · 2015-02-08T12:20:44Z

ok to test

SparkQA · 2015-02-08T12:23:08Z

Test build #27041 has started for PR 4122 at commit e58a8b0.

This patch merges cleanly.

SparkQA · 2015-02-08T12:24:14Z

Test build #27041 has finished for PR 4122 at commit e58a8b0.

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

AmplabJenkins · 2015-02-08T12:24:15Z

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

…fails to remove security groups

SparkQA · 2015-02-12T12:28:00Z

Test build #27355 has started for PR 4122 at commit 730ec05.

This patch merges cleanly.

SparkQA · 2015-02-12T12:28:57Z

Test build #27355 has finished for PR 4122 at commit 730ec05.

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

AmplabJenkins · 2015-02-12T12:28:58Z

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

voukka · 2015-02-12T12:39:56Z

I just rebased my fix and made changes according to review suggestions.

SparkQA · 2015-02-12T12:42:56Z

Test build #27356 has started for PR 4122 at commit 090dca9.

This patch merges cleanly.

SparkQA · 2015-02-12T14:00:33Z

Test build #27356 has finished for PR 4122 at commit 090dca9.

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

AmplabJenkins · 2015-02-12T14:00:37Z

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

nchammas · 2015-02-12T18:48:24Z

Thanks for the update @voukka.

LGTM

Please see https://issues.apache.org/jira/browse/SPARK-5335. The fix itself is in e58a8b01a8bedcbfbbc6d04b1c1489255865cf87 commit. Two earlier commits are fixes of another VPC related bug waiting to be merged. I should have created former bug fix in own branch then this fix would not have former fixes. :( This code is released under the project's license. Author: Vladimir Grigor <vladimir@kiosked.com> Author: Vladimir Grigor <vladimir@voukka.com> Closes #4122 from voukka/SPARK-5335_delete_sg_vpc and squashes the following commits: 090dca9 [Vladimir Grigor] fixes as per review: removed printing of group_id and added comment 730ec05 [Vladimir Grigor] fix for SPARK-5335: Destroying cluster in VPC with "--delete-groups" fails to remove security groups (cherry picked from commit ada993e) Signed-off-by: Sean Owen <sowen@cloudera.com>

Please see https://issues.apache.org/jira/browse/SPARK-5335. The fix itself is in e58a8b01a8bedcbfbbc6d04b1c1489255865cf87 commit. Two earlier commits are fixes of another VPC related bug waiting to be merged. I should have created former bug fix in own branch then this fix would not have former fixes. :( This code is released under the project's license. Author: Vladimir Grigor <vladimir@kiosked.com> Author: Vladimir Grigor <vladimir@voukka.com> Closes apache#4122 from voukka/SPARK-5335_delete_sg_vpc and squashes the following commits: 090dca9 [Vladimir Grigor] fixes as per review: removed printing of group_id and added comment 730ec05 [Vladimir Grigor] fix for SPARK-5335: Destroying cluster in VPC with "--delete-groups" fails to remove security groups (cherry picked from commit ada993e) Signed-off-by: Sean Owen <sowen@cloudera.com>

nchammas reviewed Feb 5, 2015
View reviewed changes

fix for SPARK-5335: Destroying cluster in VPC with "--delete-groups" …

730ec05

…fails to remove security groups

voukka force-pushed the SPARK-5335_delete_sg_vpc branch from e58a8b0 to 730ec05 Compare February 12, 2015 12:26

voukka changed the title ~~fix for Spark-5335: Destroying cluster in VPC with "--delete-groups" fails to remove security groups~~ [SPARK-5335] Fix deletion of security groups within a VPC Feb 12, 2015

fixes as per review: removed printing of group_id and added comment

090dca9

asfgit closed this in ada993e Feb 12, 2015

[SPARK-5335] Fix deletion of security groups within a VPC #4122

[SPARK-5335] Fix deletion of security groups within a VPC #4122

Uh oh!

Conversation

voukka commented Jan 20, 2015

Uh oh!

AmplabJenkins commented Jan 20, 2015

Uh oh!

nchammas commented Jan 22, 2015

Uh oh!

nchammas Feb 5, 2015

Choose a reason for hiding this comment

Uh oh!

nchammas commented Feb 5, 2015

Uh oh!

nchammas Feb 5, 2015

Choose a reason for hiding this comment

Uh oh!

srowen commented Feb 8, 2015

Uh oh!

SparkQA commented Feb 8, 2015

Uh oh!

SparkQA commented Feb 8, 2015

Uh oh!

AmplabJenkins commented Feb 8, 2015

Uh oh!

SparkQA commented Feb 12, 2015

Uh oh!

SparkQA commented Feb 12, 2015

Uh oh!

AmplabJenkins commented Feb 12, 2015

Uh oh!

voukka commented Feb 12, 2015

Uh oh!

SparkQA commented Feb 12, 2015

Uh oh!

SparkQA commented Feb 12, 2015

Uh oh!

AmplabJenkins commented Feb 12, 2015

Uh oh!

nchammas commented Feb 12, 2015

Uh oh!

Uh oh!