Issues 364 and #365: destroyNode cleans up incidental resources

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);
-         }
       }
    }
 
@@ -180,26 +180,36 @@ public class EC2ComputeService extends BaseComputeService {
    }
 
    /**
-    * like {@link BaseComputeService#destroyNodesMatching} except that this will
+    * Cleans implicit keypairs and security groups.
-    * clean implicit keypairs and security groups.
     */
    @Override
-   public Set<? extends NodeMetadata> destroyNodesMatching(Predicate<NodeMetadata> filter) {
+   protected void cleanUpIncidentalResourcesOfDeadNodes(Set<? extends NodeMetadata> deadNodes) {
-      Set<? extends NodeMetadata> deadOnes = super.destroyNodesMatching(filter);
       Builder<String, String> regionGroups = ImmutableMultimap.<String, String> builder();
-      for (NodeMetadata nodeMetadata : deadOnes) {
+      for (NodeMetadata nodeMetadata : deadNodes) {
          if (nodeMetadata.getGroup() != null)
             regionGroups.put(AWSUtils.parseHandle(nodeMetadata.getId())[0], nodeMetadata.getGroup());
          }
       for (Entry<String, String> regionGroup : regionGroups.build().entries()) {
-         cleanUpIncidentalResources(regionGroup);
+         cleanUpIncidentalResources(regionGroup.getKey(), regionGroup.getValue());
       }
-      return deadOnes;
    }
 
-   protected void cleanUpIncidentalResources(Entry<String, String> regionGroup) {
+   protected void cleanUpIncidentalResources(String region, String group){
-      deleteKeyPair(regionGroup.getKey(), regionGroup.getValue());
+      // For issue #445, tries to delete security groups first: ec2 throws exception if in use, but
-      deleteSecurityGroup(regionGroup.getKey(), regionGroup.getValue());
+      // 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);
+      }
    }
 
    /**

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)));

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
+    * Cleans implicit keypairs.
-    * clean implicit keypairs.
     */
    @Override
-   public Set<? extends NodeMetadata> destroyNodesMatching(Predicate<NodeMetadata> filter) {
+   protected void cleanUpIncidentalResourcesOfDeadNodes(Set<? extends NodeMetadata> deadNodes) {
-      Set<? extends NodeMetadata> deadOnes = super.destroyNodesMatching(filter);
+      cleanupOrphanKeys.execute(deadNodes);
-      cleanupOrphanKeys.execute(deadOnes);
-      return deadOnes;
    }
 
    /**

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<? extends NodeMetadata> destroyNodesMatching(Predicate<NodeMetadata> filter) {
+      logger.debug(">> destroying nodes matching(%s)", filter);
+      Set<NodeMetadata> set = newLinkedHashSet(transformParallel(nodesMatchingFilterAndNotTerminated(filter),
+            new Function<NodeMetadata, Future<NodeMetadata>>() {
+
+               // TODO make an async interface instead of re-wrapping
+               @Override
+               public Future<NodeMetadata> apply(final NodeMetadata from) {
+                  return executor.submit(new Callable<NodeMetadata>() {
+
+                     @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<NodeMetadata> node = new AtomicReference<NodeMetadata>();
@@ -244,42 +284,16 @@ 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();
    }
 
-   /**
+   protected void cleanUpIncidentalResourcesOfDeadNodes(Set<? extends NodeMetadata> deadNodes) {
-    * {@inheritDoc}
+      // no-op; to be overridden
-    */
-   @Override
-   public Set<? extends NodeMetadata> destroyNodesMatching(Predicate<NodeMetadata> filter) {
-      logger.debug(">> destroying nodes matching(%s)", filter);
-      Set<NodeMetadata> set = newLinkedHashSet(transformParallel(nodesMatchingFilterAndNotTerminated(filter),
-            new Function<NodeMetadata, Future<NodeMetadata>>() {
-
-               // TODO make an async interface instead of re-wrapping
-               @Override
-               public Future<NodeMetadata> apply(final NodeMetadata from) {
-                  return executor.submit(new Callable<NodeMetadata>() {
-
-                     @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;
    }
    
    Iterable<? extends NodeMetadata> nodesMatchingFilterAndNotTerminated(Predicate<NodeMetadata> filter) {

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<String, String> regionTag) {
+   protected void cleanUpIncidentalResources(String region, String group) {
-      super.cleanUpIncidentalResources(regionTag);
+      super.cleanUpIncidentalResources(region, group);
-      deletePlacementGroup(regionTag.getKey(), regionTag.getValue());
+      deletePlacementGroup(region, group);
    }
 
    /**

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<? extends NodeMetadata> 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<SecurityGroup> securityGroups = securityGroupClient.describeSecurityGroupsInRegion(region, expectedSecurityGroupName);
+         Set<KeyPair> 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<SecurityGroup> securityGroupsAfterDestroyFirst = securityGroupClient.describeSecurityGroupsInRegion(region, expectedSecurityGroupName);
+         Set<KeyPair> 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<SecurityGroup> securityGroupsAfterDestroyAll;
+         Set<KeyPair> 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);
+      }
+   }
 }

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) {
+   protected void cleanUpIncidentalResourcesOfDeadNodes(Set<? extends NodeMetadata> deadNodes) {
-      super.destroyNode(id);
+      // TODO Was previously commented out in overridden destroyNode; refactored to here but left commented out
-      // eliminateDomain(id);
+//      for (NodeMetadata deadNode : deadNodes) {
+//         eliminateDomain(deadNode.getId());
+//      }
    }
 
    private void eliminateDomain(String id) {