Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
voukka wants to merge 2 commits into from
Closed

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

voukka wants to merge 2 commits into from

Conversation

@voukka
Copy link

@voukka voukka commented Jan 20, 2015

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
Copy link

AmplabJenkins commented Jan 20, 2015

Can one of the admins verify this patch?

@nchammas
Copy link
Contributor

nchammas commented Jan 22, 2015

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

nchammas
nchammas reviewed Feb 5, 2015
View reviewed changes
ec2/spark_ec2.py Outdated
Copy link
Contributor

@nchammas nchammas Feb 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

nchammas commented Feb 5, 2015

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
nchammas reviewed Feb 5, 2015
View reviewed changes
ec2/spark_ec2.py Outdated
Copy link
Contributor

@nchammas nchammas Feb 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

srowen commented Feb 8, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Feb 8, 2015

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

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 8, 2015

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
Copy link

AmplabJenkins commented Feb 8, 2015

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.

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

SparkQA commented Feb 12, 2015

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

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 12, 2015

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
Copy link

AmplabJenkins commented Feb 12, 2015

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 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
@voukka
Copy link
Author

voukka commented Feb 12, 2015

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

@SparkQA
Copy link

SparkQA commented Feb 12, 2015

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

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 12, 2015

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
Copy link

AmplabJenkins commented Feb 12, 2015

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
Copy link
Contributor

nchammas commented Feb 12, 2015

Thanks for the update @voukka.

LGTM

asfgit pushed a commit that referenced this pull request Feb 12, 2015
@srowen
[SPARK-5335] Fix deletion of security groups within a VPC
5c9db4e 
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 <[email protected]>
Author: Vladimir Grigor <[email protected]>

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 <[email protected]>
asfgit pushed a commit that referenced this pull request Feb 12, 2015
@srowen
[SPARK-5335] Fix deletion of security groups within a VPC
d24971a 
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 <[email protected]>
Author: Vladimir Grigor <[email protected]>

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 <[email protected]>
@asfgit asfgit closed this in ada993e Feb 12, 2015
markhamstra pushed a commit to markhamstra/spark that referenced this pull request Feb 24, 2015
@markhamstra
[SPARK-5335] Fix deletion of security groups within a VPC
30265fd 
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 <[email protected]>
Author: Vladimir Grigor <[email protected]>

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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@voukka @AmplabJenkins @nchammas @srowen @SparkQA