diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/compute/EC2ComputeService.java b/apis/ec2/src/main/java/org/jclouds/ec2/compute/EC2ComputeService.java index 75941d3472..7c40c79eeb 100644 --- a/apis/ec2/src/main/java/org/jclouds/ec2/compute/EC2ComputeService.java +++ b/apis/ec2/src/main/java/org/jclouds/ec2/compute/EC2ComputeService.java @@ -109,20 +109,20 @@ public class EC2ComputeService extends BaseComputeService { this.securityGroupMap = securityGroupMap; } + /** + * @throws IllegalStateException If the security group was in use + */ @VisibleForTesting void deleteSecurityGroup(String region, String group) { + checkNotEmpty(region, "region"); checkNotEmpty(group, "group"); String groupName = String.format("jclouds#%s#%s", group, region); if (ec2Client.getSecurityGroupServices().describeSecurityGroupsInRegion(region, groupName).size() > 0) { logger.debug(">> deleting securityGroup(%s)", groupName); - try { - ec2Client.getSecurityGroupServices().deleteSecurityGroupInRegion(region, groupName); - // TODO: test this clear happens - securityGroupMap.invalidate(new RegionNameAndIngressRules(region, groupName, null, false)); - logger.debug("<< deleted securityGroup(%s)", groupName); - } catch (IllegalStateException e) { - logger.debug("<< inUse securityGroup(%s)", groupName); - } + ec2Client.getSecurityGroupServices().deleteSecurityGroupInRegion(region, groupName); + // TODO: test this clear happens + securityGroupMap.invalidate(new RegionNameAndIngressRules(region, groupName, null, false)); + logger.debug("<< deleted securityGroup(%s)", groupName); } } @@ -180,26 +180,36 @@ public class EC2ComputeService extends BaseComputeService { } /** - * like {@link BaseComputeService#destroyNodesMatching} except that this will - * clean implicit keypairs and security groups. + * Cleans implicit keypairs and security groups. */ @Override - public Set destroyNodesMatching(Predicate filter) { - Set deadOnes = super.destroyNodesMatching(filter); + protected void cleanUpIncidentalResourcesOfDeadNodes(Set deadNodes) { Builder regionGroups = ImmutableMultimap. builder(); - for (NodeMetadata nodeMetadata : deadOnes) { + for (NodeMetadata nodeMetadata : deadNodes) { if (nodeMetadata.getGroup() != null) regionGroups.put(AWSUtils.parseHandle(nodeMetadata.getId())[0], nodeMetadata.getGroup()); - } + } for (Entry regionGroup : regionGroups.build().entries()) { - cleanUpIncidentalResources(regionGroup); + cleanUpIncidentalResources(regionGroup.getKey(), regionGroup.getValue()); } - return deadOnes; } - protected void cleanUpIncidentalResources(Entry regionGroup) { - deleteKeyPair(regionGroup.getKey(), regionGroup.getValue()); - deleteSecurityGroup(regionGroup.getKey(), regionGroup.getValue()); + protected void cleanUpIncidentalResources(String region, String group){ + // For issue #445, tries to delete security groups first: ec2 throws exception if in use, but + // deleting a key pair does not. + // This is "belt-and-braces" because deleteKeyPair also does extractIdsFromInstances & usingKeyPairAndNotDead + // for us to check if any instances are using the key-pair before we delete it. + // There is (probably?) still a race if someone is creating instances at the same time as deleting them: + // we may delete the key-pair just when the node-being-created was about to rely on the incidental + // resources existing. + try { + logger.debug(">> deleting incidentalResources(%s @ %s)", region, group); + deleteSecurityGroup(region, group); + deleteKeyPair(region, group); // not executed if securityGroup was in use + logger.debug("<< deleted incidentalResources(%s @ %s)", region, group); + } catch (IllegalStateException e) { + logger.debug("<< inUse incidentalResources(%s @ %s)", region, group); + } } /** diff --git a/apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2ComputeServiceLiveTest.java b/apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2ComputeServiceLiveTest.java index 48cf8c5567..a11fa1dcdf 100644 --- a/apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2ComputeServiceLiveTest.java +++ b/apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2ComputeServiceLiveTest.java @@ -302,6 +302,11 @@ public class EC2ComputeServiceLiveTest extends BaseComputeServiceLiveTest { } } + /** + * Gets the instance with the given ID from the default region + * + * @throws NoSuchElementException If no instance with that id exists, or the instance is in a different region + */ protected RunningInstance getInstance(InstanceClient instanceClient, String id) { RunningInstance instance = Iterables.getOnlyElement(Iterables.getOnlyElement(instanceClient .describeInstancesInRegion(null, id))); diff --git a/common/trmk/src/main/java/org/jclouds/trmk/vcloud_0_8/compute/TerremarkVCloudComputeService.java b/common/trmk/src/main/java/org/jclouds/trmk/vcloud_0_8/compute/TerremarkVCloudComputeService.java index 590c02a995..fd46d6a75f 100644 --- a/common/trmk/src/main/java/org/jclouds/trmk/vcloud_0_8/compute/TerremarkVCloudComputeService.java +++ b/common/trmk/src/main/java/org/jclouds/trmk/vcloud_0_8/compute/TerremarkVCloudComputeService.java @@ -87,14 +87,11 @@ public class TerremarkVCloudComputeService extends BaseComputeService { } /** - * like {@link BaseComputeService#destroyNodesMatching} except that this will - * clean implicit keypairs. + * Cleans implicit keypairs. */ @Override - public Set destroyNodesMatching(Predicate filter) { - Set deadOnes = super.destroyNodesMatching(filter); - cleanupOrphanKeys.execute(deadOnes); - return deadOnes; + protected void cleanUpIncidentalResourcesOfDeadNodes(Set deadNodes) { + cleanupOrphanKeys.execute(deadNodes); } /** diff --git a/compute/src/main/java/org/jclouds/compute/internal/BaseComputeService.java b/compute/src/main/java/org/jclouds/compute/internal/BaseComputeService.java index 4c2d15d64e..98ec74a125 100644 --- a/compute/src/main/java/org/jclouds/compute/internal/BaseComputeService.java +++ b/compute/src/main/java/org/jclouds/compute/internal/BaseComputeService.java @@ -32,6 +32,7 @@ import static org.jclouds.compute.predicates.NodePredicates.all; import static org.jclouds.concurrent.FutureIterables.awaitCompletion; import static org.jclouds.concurrent.FutureIterables.transformParallel; +import java.util.Collections; import java.util.Map; import java.util.NoSuchElementException; import java.util.Set; @@ -225,6 +226,45 @@ public class BaseComputeService implements ComputeService { */ @Override public void destroyNode(final String id) { + NodeMetadata destroyedNode = doDestroyNode(id); + cleanUpIncidentalResourcesOfDeadNodes(Collections.singleton(destroyedNode)); + } + + /** + * {@inheritDoc} + */ + @Override + public Set destroyNodesMatching(Predicate filter) { + logger.debug(">> destroying nodes matching(%s)", filter); + Set set = newLinkedHashSet(transformParallel(nodesMatchingFilterAndNotTerminated(filter), + new Function>() { + + // TODO make an async interface instead of re-wrapping + @Override + public Future apply(final NodeMetadata from) { + return executor.submit(new Callable() { + + @Override + public NodeMetadata call() throws Exception { + doDestroyNode(from.getId()); + return from; + } + + @Override + public String toString() { + return "destroyNode(" + from.getId() + ")"; + } + }); + } + + }, executor, null, logger, "destroyNodesMatching(" + filter + ")")); + logger.debug("<< destroyed(%d)", set.size()); + + cleanUpIncidentalResourcesOfDeadNodes(set); + return set; + } + + protected NodeMetadata doDestroyNode(final String id) { checkNotNull(id, "id"); logger.debug(">> destroying node(%s)", id); final AtomicReference node = new AtomicReference(); @@ -244,44 +284,18 @@ public class BaseComputeService implements ComputeService { } }, timeouts.nodeRunning, 1000, TimeUnit.MILLISECONDS); + boolean successful = tester.apply(id) && (node.get() == null || nodeTerminated.apply(node.get())); if (successful) credentialStore.remove("node#" + id); logger.debug("<< destroyed node(%s) success(%s)", id, successful); + return node.get(); } - /** - * {@inheritDoc} - */ - @Override - public Set destroyNodesMatching(Predicate filter) { - logger.debug(">> destroying nodes matching(%s)", filter); - Set set = newLinkedHashSet(transformParallel(nodesMatchingFilterAndNotTerminated(filter), - new Function>() { - - // TODO make an async interface instead of re-wrapping - @Override - public Future apply(final NodeMetadata from) { - return executor.submit(new Callable() { - - @Override - public NodeMetadata call() throws Exception { - destroyNode(from.getId()); - return from; - } - - @Override - public String toString() { - return "destroyNode(" + from.getId() + ")"; - } - }); - } - - }, executor, null, logger, "destroyNodesMatching(" + filter + ")")); - logger.debug("<< destroyed(%d)", set.size()); - return set; + protected void cleanUpIncidentalResourcesOfDeadNodes(Set deadNodes) { + // no-op; to be overridden } - + Iterable nodesMatchingFilterAndNotTerminated(Predicate filter) { return filter(detailsOnAllNodes(), and(checkNotNull(filter, "filter"), not(TERMINATED))); } diff --git a/providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeService.java b/providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeService.java index 9d7b138573..d8c3ac4dbe 100644 --- a/providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeService.java +++ b/providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeService.java @@ -204,9 +204,9 @@ public class AWSEC2ComputeService extends EC2ComputeService { } @Override - protected void cleanUpIncidentalResources(Entry regionTag) { - super.cleanUpIncidentalResources(regionTag); - deletePlacementGroup(regionTag.getKey(), regionTag.getValue()); + protected void cleanUpIncidentalResources(String region, String group) { + super.cleanUpIncidentalResources(region, group); + deletePlacementGroup(region, group); } /** diff --git a/providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeServiceLiveTest.java b/providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeServiceLiveTest.java index 9636a78c41..737a20e986 100644 --- a/providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeServiceLiveTest.java +++ b/providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeServiceLiveTest.java @@ -24,7 +24,9 @@ import static org.jclouds.compute.domain.OsFamily.AMZN_LINUX; import static org.jclouds.compute.options.RunScriptOptions.Builder.runAsRoot; import static org.jclouds.ec2.util.IpPermissions.permit; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotNull; +import java.util.Collections; import java.util.Date; import java.util.Set; import java.util.concurrent.TimeUnit; @@ -52,14 +54,18 @@ import org.jclouds.ec2.domain.SecurityGroup; import org.jclouds.ec2.services.InstanceClient; import org.jclouds.ec2.services.KeyPairClient; import org.jclouds.logging.log4j.config.Log4JLoggingModule; +import org.jclouds.predicates.RetryablePredicate; import org.jclouds.rest.RestContext; import org.jclouds.rest.RestContextFactory; import org.jclouds.scriptbuilder.domain.Statements; import org.testng.annotations.Test; +import com.google.common.base.Predicate; +import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Iterables; import com.google.common.util.concurrent.ListenableFuture; import com.google.inject.Module; @@ -198,4 +204,90 @@ public class AWSEC2ComputeServiceLiveTest extends EC2ComputeServiceLiveTest { } } + + @Test + public void testIncidentalResourcesGetCleanedUpAfterInstanceIsDestroyed() throws Exception { + AWSSecurityGroupClient securityGroupClient = AWSEC2Client.class.cast(context.getProviderSpecificContext().getApi()) + .getSecurityGroupServices(); + + KeyPairClient keyPairClient = EC2Client.class.cast(context.getProviderSpecificContext().getApi()) + .getKeyPairServices(); + + InstanceClient instanceClient = EC2Client.class.cast(context.getProviderSpecificContext().getApi()) + .getInstanceServices(); + + String group = this.group + "incidental"; + String region = null; + + try { + // Create two instances + // TODO set spotPrice(0.3f) ? + Template template = client.templateBuilder().build(); + Set nodes = client.createNodesInGroup(group, 2, template); + NodeMetadata first = Iterables.get(nodes, 0); + NodeMetadata second = Iterables.get(nodes, 1); + + String instanceId1 = Iterables.get(nodes, 0).getProviderId(); + String instanceId2 = Iterables.get(nodes, 1).getProviderId(); + + AWSRunningInstance instance1 = AWSRunningInstance.class.cast(getInstance(instanceClient, instanceId1)); + AWSRunningInstance instance2 = AWSRunningInstance.class.cast(getInstance(instanceClient, instanceId2)); + + // Assert the two instances are in the same groups + region = instance1.getRegion(); + String expectedSecurityGroupName = "jclouds#" + group + "#" + region; + + assertEquals(instance1.getRegion(), region); + assertNotNull(instance1.getKeyName()); + assertEquals(instance1.getRegion(), instance2.getRegion(), "Nodes are not in the same region"); + assertEquals(instance1.getKeyName(), instance2.getKeyName(), "Nodes do not have same key-pair name"); + assertEquals(instance1.getGroupIds(), instance2.getGroupIds(), "Nodes are not in the same group"); + assertEquals(instance1.getGroupIds(), ImmutableSet.of(expectedSecurityGroupName), "Nodes are not in the expected security group"); + + // Assert a single key-pair and security group has been created + String expectedKeyPairName = instance1.getKeyName(); + Set securityGroups = securityGroupClient.describeSecurityGroupsInRegion(region, expectedSecurityGroupName); + Set keyPairs = keyPairClient.describeKeyPairsInRegion(region, expectedKeyPairName); + assertEquals(securityGroups.size(), 1); + assertEquals(Iterables.get(securityGroups, 0).getName(), expectedSecurityGroupName); + assertEquals(keyPairs.size(), 1); + assertEquals(Iterables.get(keyPairs, 0).getKeyName(), expectedKeyPairName); + + // Destroy the first node; the key-pair and security-group should still remain + client.destroyNode(first.getId()); + + Set securityGroupsAfterDestroyFirst = securityGroupClient.describeSecurityGroupsInRegion(region, expectedSecurityGroupName); + Set keyPairsAfterDestroyFirst = keyPairClient.describeKeyPairsInRegion(region, expectedKeyPairName); + assertEquals(securityGroupsAfterDestroyFirst, securityGroups); + assertEquals(keyPairsAfterDestroyFirst, keyPairs); + + // Destroy the second node; the key-pair and security-group should be automatically deleted + // It can take some time after destroyNode returns for the securityGroup and keyPair to be completely removed. + // Therefore try repeatedly. + client.destroyNode(second.getId()); + + final int TIMEOUT_MS = 30*1000; + boolean firstAttempt = true; + boolean done; + Set securityGroupsAfterDestroyAll; + Set keyPairsAfterDestroyAll; + Stopwatch stopwatch = new Stopwatch(); + stopwatch.start(); + do { + if (!firstAttempt) Thread.sleep(1000); + firstAttempt = false; + securityGroupsAfterDestroyAll = securityGroupClient.describeSecurityGroupsInRegion(region, expectedSecurityGroupName); + keyPairsAfterDestroyAll = keyPairClient.describeKeyPairsInRegion(region, expectedKeyPairName); + done = securityGroupsAfterDestroyAll.isEmpty() && keyPairsAfterDestroyAll.isEmpty(); + } while (!done && stopwatch.elapsedMillis() < TIMEOUT_MS); + + assertEquals(securityGroupsAfterDestroyAll, Collections.emptySet()); + assertEquals(keyPairsAfterDestroyAll, Collections.emptySet()); + + } finally { + client.destroyNodesMatching(NodePredicates.inGroup(group)); + + if (region != null) cleanupExtendedStuffInRegion(region, securityGroupClient, keyPairClient, group); + } + } } diff --git a/sandbox-apis/libvirt/src/main/java/org/jclouds/libvirt/compute/LibvirtComputeService.java b/sandbox-apis/libvirt/src/main/java/org/jclouds/libvirt/compute/LibvirtComputeService.java index 4e5de1af88..93a4c97e3f 100644 --- a/sandbox-apis/libvirt/src/main/java/org/jclouds/libvirt/compute/LibvirtComputeService.java +++ b/sandbox-apis/libvirt/src/main/java/org/jclouds/libvirt/compute/LibvirtComputeService.java @@ -95,9 +95,11 @@ public class LibvirtComputeService extends BaseComputeService { } @Override - public void destroyNode(String id) { - super.destroyNode(id); - // eliminateDomain(id); + protected void cleanUpIncidentalResourcesOfDeadNodes(Set deadNodes) { + // TODO Was previously commented out in overridden destroyNode; refactored to here but left commented out +// for (NodeMetadata deadNode : deadNodes) { +// eliminateDomain(deadNode.getId()); +// } } private void eliminateDomain(String id) {