JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources #629

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@aledsage
Contributor
  • 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.
Contributor

This fix targets 1.8.x branch; once that is merged successfully, we can PR this against master as well.

Contributor

@aledsage code looks good to me, but the builder is not happy because of some checkstyle violations
+1 to PR against master as well, once you address the violations

@nacx nacx and 1 other commented on an outdated diff Jan 5, 2015
.../aws/ec2/compute/AWSEC2ComputeServiceApiMockTest.java
+ 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
nacx Jan 5, 2015 Owner

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?

aledsage
aledsage Jan 12, 2015 Contributor

@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.

nacx
nacx Jan 14, 2015 Owner

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.

Contributor

@nacx @andreaturli I've added support for reconfiguring the retry timeout for cleaning up incidental resources. Could you take another look please?

Owner
nacx commented Jan 28, 2015

Changes lgtm. Thanks @aledsage! Mind squashing and rebasing so I can cleanly merge it?

aledsage added some commits Dec 29, 2014
@aledsage aledsage JCLOUDS-529: fix aws-ec2 cleanupIncidentalResources
- 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.
- Adds Timeouts.cleanupIncidentalResources
- Use that timeout in EC2, when retrying the deletion of security group
  on VM deletion (previously hard-coded as 3 seconds).
- Configure that timeout in the tests, so deterministic number of retries
1feee81
@aledsage aledsage Fix/improve retry-predicate:
- If get timeout of 0 (or negative), then still try once.
- Remove (unlikely) race in retry’s apply(T) where context-switching
  delays could cause `before(end)` to return false the first time, even
  though the timeout was positive.
- Ensure retries at end of the timeout (e.g. if timeout is 30 secs
  and last sleep takes us up to the 30 secs mark, then test again
  rather than returning immediately after the sleep!)
- Use `long` for time, rather than `java.util.Date`, for internal
  calculations. Deprecates old protected methods that use Date.
7464af0
Contributor

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?

@aledsage aledsage deleted the aledsage:fix/jclouds-529 branch Jan 29, 2015
Owner
nacx commented Jan 29, 2015

Thx!

@aledsage aledsage referenced this pull request Jan 29, 2015
Closed

Fix jclouds 529 #658

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment