Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources (#629)
Thx! --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/629#issuecomment-72017329
Re: [jclouds] JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources (#629)
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)
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)
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)
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)
@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)
> + 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)
> + 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)
> + 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)
@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)
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
[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)
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
