[JCLOUDS-1306] Fix SG cache invalidation when deleting

apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtension.java

@@ -191,14 +191,20 @@ public class NovaSecurityGroupExtension implements SecurityGroupExtension {
          return false;
       }
 
-      if (sgApi.get().get(groupId) == null) {
+      // Would be nice to delete the group and invalidate the cache atomically - i.e. use a mutex.
-         return false;
+      // Will make sure that a create operation in parallel won't see inconsistent state.
+
+      boolean deleted = sgApi.get().delete(groupId);
+
+      for (SecurityGroupInRegion cachedSg : groupCreator.asMap().values()) {
+         if (groupId.equals(cachedSg.getSecurityGroup().getId())) {
+            String groupName = cachedSg.getName();
+            groupCreator.invalidate(new RegionSecurityGroupNameAndPorts(region, groupName, ImmutableSet.<Integer>of()));
+            break;
+         }
       }
 
-      sgApi.get().delete(groupId);
+      return deleted;
-      // TODO: test this clear happens
-      groupCreator.invalidate(new RegionSecurityGroupNameAndPorts(region, groupId, ImmutableSet.<Integer> of()));
-      return true;
    }
 
    @Override

compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java

@@ -27,6 +27,7 @@ import javax.annotation.Resource;
 import javax.inject.Named;
 
 import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.ComputeServiceContext;
 import org.jclouds.compute.RunNodesException;
 import org.jclouds.compute.domain.SecurityGroup;
 import org.jclouds.compute.domain.Template;
@@ -60,6 +61,7 @@ public abstract class BaseSecurityGroupExtensionLiveTest extends BaseComputeServ
    protected Logger logger = Logger.NULL;
 
    protected final String secGroupName = "test-create-security-group";
+   protected final String secGroupNameToDelete = "test-create-security-group-to-delete";
    protected final String nodeGroup = "test-create-node-with-group";
 
    protected String groupId;
@@ -379,6 +381,70 @@ public abstract class BaseSecurityGroupExtensionLiveTest extends BaseComputeServ
       assertTrue(securityGroupExtension.get().removeSecurityGroup(group.getId()));
    }
 
+   @Test(groups = {"integration", "live"}, singleThreaded = true)
+   public void testSecurityGroupCacheInvalidated() throws Exception {
+      ComputeService computeService = view.getComputeService();
+      Optional<SecurityGroupExtension> securityGroupExtension = computeService.getSecurityGroupExtension();
+      assertTrue(securityGroupExtension.isPresent(), "security extension was not present");
+      final SecurityGroupExtension security = securityGroupExtension.get();
+      final SecurityGroup seedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation());
+      boolean deleted = security.removeSecurityGroup(seedGroup.getId());
+      assertTrue(deleted, "just created security group failed deletion");
+      final SecurityGroup recreatedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation());
+
+      // Makes sure the security group exists and is re-created and is not just returned from cache
+      security.addIpPermission(IpPermission.builder()
+                      .fromPort(1000)
+                      .toPort(1000)
+                      .cidrBlock("1.1.1.1/32")
+                      .ipProtocol(IpProtocol.TCP)
+                      .build(),
+              recreatedGroup);
+      boolean deleted2 = security.removeSecurityGroup(recreatedGroup.getId());
+      assertTrue(deleted2, "just created security group failed deletion");
+   }
+
+   @Test(groups = {"integration", "live"}, singleThreaded = true)
+   public void testSecurityGroupCacheInvalidatedWhenDeletedExternally() throws Exception {
+      ComputeService computeService = view.getComputeService();
+      Optional<SecurityGroupExtension> securityGroupExtension = computeService.getSecurityGroupExtension();
+      assertTrue(securityGroupExtension.isPresent(), "security extension was not present");
+      final SecurityGroupExtension security = securityGroupExtension.get();
+      final SecurityGroup seedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation());
+
+      deleteSecurityGroupFromAnotherView(seedGroup);
+
+      boolean deleted = security.removeSecurityGroup(seedGroup.getId());
+      assertFalse(deleted, "SG deleted externally so should've failed deletion");
+      final SecurityGroup recreatedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation());
+
+      // Makes sure the security group exists and is re-created and is not just returned from cache
+      security.addIpPermission(IpPermission.builder()
+                      .fromPort(1000)
+                      .toPort(1000)
+                      .cidrBlock("1.1.1.1/32")
+                      .ipProtocol(IpProtocol.TCP)
+                      .build(),
+              recreatedGroup);
+      boolean deleted2 = security.removeSecurityGroup(recreatedGroup.getId());
+      assertTrue(deleted2, "just created security group failed deletion");
+   }
+
+   private void deleteSecurityGroupFromAnotherView(SecurityGroup seedGroup) {
+      ComputeServiceContext localView = createView(setupProperties(), setupModules());
+      try {
+         ComputeService localComputeService = localView.getComputeService();
+         Optional<SecurityGroupExtension> securityGroupExtension = localComputeService.getSecurityGroupExtension();
+         assertTrue(securityGroupExtension.isPresent(), "security extension was not present");
+         final SecurityGroupExtension security = securityGroupExtension.get();
+
+         boolean deleted = security.removeSecurityGroup(seedGroup.getId());
+         assertTrue(deleted, "just created security group failed deletion");
+      } finally {
+         localView.close();
+      }
+   }
+
    private Multimap<String, String> emptyMultimap() {
       return LinkedHashMultimap.create();
    }