You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Aled Sage <no...@github.com> on 2014/12/29 22:06:20 UTC
[jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources (#629)
- Some users get a `DependencyVioloation`, rather than `InvalidGroup.InUse`, when attempting to delete the security group. This caused `cleanupIncidentalResources` to propagate an exception.
- Fixes it by converting this to an `IllegalStateException` (in same way as is done for `InUse`)
- Adds tests (using `MockWebServer`) for happy-path and for failing to delete the security group with each of `InUse` and `DependencyViolation` responses.
You can merge this Pull Request by running:
git pull https://github.com/aledsage/jclouds fix/jclouds-529
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/629
-- Commit Summary --
* JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
-- File Changes --
A apis/ec2/src/test/resources/delete_keypair.xml (4)
A apis/ec2/src/test/resources/delete_placementgroup.xml (4)
A apis/ec2/src/test/resources/describe_instances_empty.xml (15)
A apis/ec2/src/test/resources/describe_keypairs_jcloudssingle.xml (8)
M apis/sts/src/main/java/org/jclouds/aws/handlers/ParseAWSErrorFromXmlContent.java (4)
M providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeServiceApiMockTest.java (104)
M providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/internal/BaseAWSEC2ApiMockTest.java (11)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/629.patch
https://github.com/jclouds/jclouds/pull/629.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Aled Sage <no...@github.com>.
This fix targets 1.8.x branch; once that is merged successfully, we can PR this against master as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-68303007
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Andrea Turli <no...@github.com>.
@aledsage code looks good to me, but the builder is not happy because of some [checkstyle violations](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1475/org.apache.jclouds.provider$aws-ec2/violations/)
+1 to PR against master as well, once you address the violations
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-68400307
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Aled Sage <no...@github.com>.
Also see comments in https://issues.apache.org/jira/browse/JCLOUDS-529
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-68305954
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Aled Sage <no...@github.com>.
> + assertPosted(DEFAULT_REGION, "Action=DescribePlacementGroups&GroupName.1=jclouds%23sg-3c6ef654%23us-east-1");
> + assertPosted(DEFAULT_REGION, "Action=DeletePlacementGroup&GroupName=jclouds%23sg-3c6ef654%23us-east-1");
> + assertPosted(DEFAULT_REGION, "Action=DescribePlacementGroups&GroupName.1=jclouds%23sg-3c6ef654%23us-east-1");
> + }
> +
> + public void deleteIncidentalResourcesGivingDependencyViolationForSecurityGroup() throws Exception {
> + runDeleteIncidentalResourcesGivingErrForSecurityGroup("DependencyViolation");
> + }
> +
> + public void deleteIncidentalResourcesGivingInUseForSecurityGroup() throws Exception {
> + runDeleteIncidentalResourcesGivingErrForSecurityGroup("InvalidGroup.InUse");
> + }
> +
> + protected void runDeleteIncidentalResourcesGivingErrForSecurityGroup(String errCode) throws Exception {
> + // Complicated dispatcher is needed because cleanUpIncidentalResources will retry an unpredictable
> + // number of times (because it is time-based, for 3 seconds - not configurable).
@nacx agreed that making this configurable is sensible. I'd prefer that work to be done in multiple stages if possible - with this PR being merged first, as it fixes the issue of delete-VM always failing for a sub-set of AWS account holders.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629/files#r22782327
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Andrea Turli <no...@github.com>.
Thanks @aledsage
Merged at https://git1-us-west.apache.org/repos/asf?p=jclouds.git;a=commit;h=ee42fb1a687d3f8de0db9ff870cb7e9ad00d53aa
and
https://git1-us-west.apache.org/repos/asf?p=jclouds.git;a=commit;h=7866612a2eca6e3ee32bc7928e692ec325ac7d30
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-72011507
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Aled Sage <no...@github.com>.
@nacx @andreaturli I've added support for reconfiguring the retry timeout for cleaning up incidental resources. Could you take another look please?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-71786070
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Ignasi Barrera <no...@github.com>.
> + assertPosted(DEFAULT_REGION, "Action=DescribePlacementGroups&GroupName.1=jclouds%23sg-3c6ef654%23us-east-1");
> + assertPosted(DEFAULT_REGION, "Action=DeletePlacementGroup&GroupName=jclouds%23sg-3c6ef654%23us-east-1");
> + assertPosted(DEFAULT_REGION, "Action=DescribePlacementGroups&GroupName.1=jclouds%23sg-3c6ef654%23us-east-1");
> + }
> +
> + public void deleteIncidentalResourcesGivingDependencyViolationForSecurityGroup() throws Exception {
> + runDeleteIncidentalResourcesGivingErrForSecurityGroup("DependencyViolation");
> + }
> +
> + public void deleteIncidentalResourcesGivingInUseForSecurityGroup() throws Exception {
> + runDeleteIncidentalResourcesGivingErrForSecurityGroup("InvalidGroup.InUse");
> + }
> +
> + protected void runDeleteIncidentalResourcesGivingErrForSecurityGroup(String errCode) throws Exception {
> + // Complicated dispatcher is needed because cleanUpIncidentalResources will retry an unpredictable
> + // number of times (because it is time-based, for 3 seconds - not configurable).
If the hardcoded retry policy is an issue here, and it is demonstrated that might not be convenient when dealing with eventual consistency, should it be better be refactored to read those values from properties (or make the entire predicate injectable, or whatever) so users can customize it and we can add a better test?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629/files#r22459827
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Ignasi Barrera <no...@github.com>.
> + assertPosted(DEFAULT_REGION, "Action=DescribePlacementGroups&GroupName.1=jclouds%23sg-3c6ef654%23us-east-1");
> + assertPosted(DEFAULT_REGION, "Action=DeletePlacementGroup&GroupName=jclouds%23sg-3c6ef654%23us-east-1");
> + assertPosted(DEFAULT_REGION, "Action=DescribePlacementGroups&GroupName.1=jclouds%23sg-3c6ef654%23us-east-1");
> + }
> +
> + public void deleteIncidentalResourcesGivingDependencyViolationForSecurityGroup() throws Exception {
> + runDeleteIncidentalResourcesGivingErrForSecurityGroup("DependencyViolation");
> + }
> +
> + public void deleteIncidentalResourcesGivingInUseForSecurityGroup() throws Exception {
> + runDeleteIncidentalResourcesGivingErrForSecurityGroup("InvalidGroup.InUse");
> + }
> +
> + protected void runDeleteIncidentalResourcesGivingErrForSecurityGroup(String errCode) throws Exception {
> + // Complicated dispatcher is needed because cleanUpIncidentalResources will retry an unpredictable
> + // number of times (because it is time-based, for 3 seconds - not configurable).
I understand what you mean, but given that you've had to struggle with this test to make it work with the current implementation (because it is not a good one), I think it is reasonable to propose to fix and improve the code as soon as we find ourselves having to workaround it.
I know this might not be directly related to the change in the error parser, but if we just *fix stuff* without trying to make the related code better, we'll just be growing the codebase in a way that will make it harder to maintain and evolve in the future. In this case, there is just a test that calls code that is not test friendly. Instead of making the test complex, it is better to fix that code, as it should be pretty straightforward.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629/files#r22925462
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Aled Sage <no...@github.com>.
I've squashed that down to two commits. I've left it as two because the changes in the retry-predicate are logically separate from the cleanupIncidentalResources changes. It could have been its own pull request, but that felt unnecessary given it is being reviewed in the context of this PR.
@nacx @andreaturli will one of you merge this please, and then I'll submit PR against master with same changes?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-72009360
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Andrea Turli <no...@github.com>.
Closed #629.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#event-226766595
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Ignasi Barrera <no...@github.com>.
Changes lgtm. Thanks @aledsage! Mind squashing and rebasing so I can cleanly merge it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-71911666
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
(#629)
Posted by Ignasi Barrera <no...@github.com>.
Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/629#issuecomment-72017329