ACL: VM IP address failures and ID Pool issues

Below ACL issues are fixed:

(a) Issue: Tenant VM IP address failures

    Fix: Cache was not updated properly in AclInterfaceStateListener. This was
           resulting in wrong lport and dpnid in cache. Updated to rectify this
           problem in AclInterfaceStateListener.

(b) Issue: Ids exhausted for pool : ACL-TAG-POOL

    Fix: In Event listener there was wrong check for SecurityRule in
         add(), remove() and update() methods. The check in remove() some times
         was resulting in releaseId of ACL-TAG-POOL not being called. This was
         resulting in Ids exhausted when large number of ACLs were created and
         deleted. Check for SecurityRule in add(), remove() and update() is
         removed to fix the problem.

(b) Issue: ACL-TAG-POOL error with releaseId

    Fix: In the current code, releaseId of ACL-TAG-POOL is called on all
         the nodes of cluster. On one node releaseId would have succeeded and
         on other two nodes, error reported would be observed. Code is updated
         now to invoke releaseId with Entirty check so that it gets executed on
         only one node instead of all the nodes.

Change-Id: Iba0e6486795cfd867db03487b24c069ac06f35eb
Signed-off-by: Shashidhar Raja <shashidharr@altencalsoftlabs.com>

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java
index 3b78577f1c6f8cc046c461b53478508d0d3cdd2d..59927b45ecf4e7f8a0bb75643c712c3ae974901f 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/AbstractAclServiceImpl.java
@@ -380,6 +380,11 @@ public abstract class AbstractAclServiceImpl implements AclServiceListener {
     protected void programAceRule(List<FlowEntity> flowEntries, AclInterface port, String aclName, Ace ace,
             int addOrRemove) {
         SecurityRuleAttr aceAttr = AclServiceUtils.getAccessListAttributes(ace);
+        if (aceAttr == null) {
+            LOG.error("Ace {} of Acl {} is either null or not having SecurityRuleAttr",
+                    ((ace == null) ? null : ace.getRuleName()), aclName);
+            return;
+        }
         if (addOrRemove == NwConstants.ADD_FLOW && aceAttr.isDeleted()) {
             LOG.trace("Ignoring {} rule which is already deleted", ace.getRuleName());
             return;

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java
index 75985301db4968bdb32e8a26b09ee4b37c1bc232..d939e25275a0bdc2ca636ce11b1e756e5151c34a 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclEventListener.java
@@ -93,21 +93,18 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase<Acl, AclEv
 
     @Override
     protected void remove(InstanceIdentifier<Acl> key, Acl acl) {
-        String aclName = acl.getAclName();
-        if (!AclServiceUtils.isOfAclInterest(acl)) {
-            LOG.trace("{} does not have SecurityRuleAttr augmentation", aclName);
-            return;
-        }
-
         LOG.trace("On remove event, remove ACL: {}", acl);
+        String aclName = acl.getAclName();
         this.aclDataUtil.removeAcl(aclName);
         Integer aclTag = this.aclDataUtil.getAclTag(aclName);
         if (aclTag != null) {
             this.aclDataUtil.removeAclTag(aclName);
-            this.aclServiceUtils.releaseAclTag(aclName);
         }
         updateRemoteAclCache(acl.getAccessListEntries().getAce(), aclName, AclServiceManager.Action.REMOVE);
         if (aclClusterUtil.isEntityOwner()) {
+            if (aclTag != null) {
+                this.aclServiceUtils.releaseAclTag(aclName);
+            }
             // Handle Rule deletion If SG Remove event is received before SG Rule delete event
             List<Ace> aceList = acl.getAccessListEntries().getAce();
             Collection<AclInterface> aclInterfaces =
@@ -118,11 +115,6 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase<Acl, AclEv
 
     @Override
     protected void update(InstanceIdentifier<Acl> key, Acl aclBefore, Acl aclAfter) {
-        if (!AclServiceUtils.isOfAclInterest(aclAfter) && !AclServiceUtils.isOfAclInterest(aclBefore)) {
-            LOG.trace("before {} and after {} does not have SecurityRuleAttr augmentation",
-                    aclBefore.getAclName(), aclAfter.getAclName());
-            return;
-        }
         String aclName = aclAfter.getAclName();
         Collection<AclInterface> interfacesBefore =
                 ImmutableSet.copyOf(aclDataUtil.getInterfaceList(new Uuid(aclName)));
@@ -163,15 +155,10 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase<Acl, AclEv
 
     @Override
     protected void add(InstanceIdentifier<Acl> key, Acl acl) {
-        String aclName = acl.getAclName();
-        if (!AclServiceUtils.isOfAclInterest(acl)) {
-            LOG.trace("{} does not have SecurityRuleAttr augmentation", aclName);
-            return;
-        }
-
         LOG.trace("On add event, add ACL: {}", acl);
         this.aclDataUtil.addAcl(acl);
 
+        String aclName = acl.getAclName();
         Integer aclTag = this.aclServiceUtils.allocateAclTag(aclName);
         if (aclTag != null && aclTag != AclConstants.INVALID_ACL_TAG) {
             this.aclDataUtil.addAclTag(aclName, aclTag);

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceStateListener.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceStateListener.java
index c9ac31dbb366967c7b636bd5c966c4c8ecf43585..3bad9ac6e93d145fe6713a32f2b54da19566a27b 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceStateListener.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/listeners/AclInterfaceStateListener.java
@@ -174,7 +174,7 @@ public class AclInterfaceStateListener extends AsyncDataTreeChangeListenerBase<I
         if (AclServiceUtils.isOfInterest(aclInterface)) {
             List<Uuid> aclList = aclInterface.getSecurityGroups();
             if (aclList != null) {
-                aclDataUtil.addAclInterfaceMap(aclList, aclInterface);
+                aclDataUtil.addOrUpdateAclInterfaceMap(aclList, aclInterface);
             }
             if (aclInterface.getElanId() == null) {
                 LOG.debug("On Add event, skip ADD since ElanId is not updated");

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java
index dda987e04c928bb4660f943126738a2e9039ee68..2ba6eae7df7434b0196e7df4f62756dcba06047e 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtil.java
@@ -70,17 +70,6 @@ public class AclDataUtil implements AclDataCache {
         return this.aclMap.get(aclName);
     }
 
-    public void addAclInterfaceMap(List<Uuid> aclList, AclInterface port) {
-        for (Uuid acl : aclList) {
-            addAclInterface(acl, port);
-        }
-    }
-
-    private void addAclInterface(Uuid acl, AclInterface port) {
-        aclInterfaceMap.computeIfAbsent(acl, key -> new ConcurrentHashMap<>())
-                .putIfAbsent(port.getInterfaceId(), port);
-    }
-
     public void addOrUpdateAclInterfaceMap(List<Uuid> aclList, AclInterface port) {
         for (Uuid acl : aclList) {
             aclInterfaceMap.computeIfAbsent(acl, key -> new ConcurrentHashMap<>()).put(port.getInterfaceId(), port);

diff --git a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java
index 704b68e5dce96ecb67a72cba8a0aa8d22c23813a..9620850f0403d2dc746947b91acb0a80a411210d 100644 (file)
--- a/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java
+++ b/aclservice/impl/src/main/java/org/opendaylight/netvirt/aclservice/utils/AclServiceUtils.java
@@ -245,12 +245,10 @@ public final class AclServiceUtils {
     @Nullable
     public static SecurityRuleAttr getAccessListAttributes(Ace ace) {
         if (ace == null) {
-            LOG.error("Ace is Null");
             return null;
         }
         SecurityRuleAttr aceAttributes = ace.augmentation(SecurityRuleAttr.class);
         if (aceAttributes == null) {
-            LOG.error("Ace is null");
             return null;
         }
         return aceAttributes;
@@ -954,16 +952,6 @@ public final class AclServiceUtils {
         return flowMatches;
     }
 
-    public static boolean isOfAclInterest(Acl acl) {
-        if (acl.getAccessListEntries() != null) {
-            List<Ace> aceList = acl.getAccessListEntries().getAce();
-            if (aceList != null && !aceList.isEmpty()) {
-                return aceList.get(0).augmentation(SecurityRuleAttr.class) != null;
-            }
-        }
-        return false;
-    }
-
     /**
      * Builds the ip protocol matches.
      *
@@ -1020,7 +1008,8 @@ public final class AclServiceUtils {
         if (accessListEntries != null && accessListEntries.getAce() != null) {
             for (Ace ace : accessListEntries.getAce()) {
                 SecurityRuleAttr aceAttr = AclServiceUtils.getAccessListAttributes(ace);
-                if (Objects.equals(aceAttr.getDirection(), direction) && doesAceHaveRemoteGroupId(aceAttr)) {
+                if (aceAttr != null && Objects.equals(aceAttr.getDirection(), direction)
+                        && doesAceHaveRemoteGroupId(aceAttr)) {
                     remoteAclIds.add(aceAttr.getRemoteGroupId());
                 }
             }

diff --git a/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtilTest.java b/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtilTest.java
index 5e50483f91e67e1a6f5908f03d6a1e759fa507da..f9eabbf061008ac7fb2e5811f4ca975b2106dbac 100644 (file)
--- a/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtilTest.java
+++ b/aclservice/impl/src/test/java/org/opendaylight/netvirt/aclservice/utils/AclDataUtilTest.java
@@ -54,17 +54,17 @@ public class AclDataUtilTest {
         final BigInteger dpId = new BigInteger("123");
         assertFalse(aclDataUtil.doesDpnHaveAclInterface(dpId));
 
-        aclDataUtil.addAclInterfaceMap(Arrays.asList(ACL1, ACL2), PORT1);
+        aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL1, ACL2), PORT1);
         assertAclInterfaces(ACL1, PORT1);
         assertAclInterfaces(ACL2, PORT1);
 
-        aclDataUtil.addAclInterfaceMap(Arrays.asList(ACL1), PORT2);
+        aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL1), PORT2);
         assertAclInterfaces(ACL1, PORT1, PORT2);
         assertAclInterfaces(ACL2, PORT1);
 
         assertFalse(aclDataUtil.doesDpnHaveAclInterface(dpId));
 
-        aclDataUtil.addAclInterfaceMap(Arrays.asList(ACL1), PORT2);
+        aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL1), PORT2);
         assertAclInterfaces(ACL1, PORT1, PORT2);
 
         aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL1), PORT3);
@@ -118,13 +118,13 @@ public class AclDataUtilTest {
         assertNotNull(map);
         assertEquals(0, map.size());
 
-        aclDataUtil.addAclInterfaceMap(Arrays.asList(ACL2), PORT1);
-        aclDataUtil.addAclInterfaceMap(Arrays.asList(ACL2), PORT2);
+        aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL2), PORT1);
+        aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL2), PORT2);
         map = aclDataUtil.getRemoteAclInterfaces(ACL1, direction);
         assertEquals(1, map.size());
         assertAclInterfaces(map.get(ACL2.getValue()), PORT1, PORT2);
 
-        aclDataUtil.addAclInterfaceMap(Arrays.asList(ACL3), PORT3);
+        aclDataUtil.addOrUpdateAclInterfaceMap(Arrays.asList(ACL3), PORT3);
         map = aclDataUtil.getRemoteAclInterfaces(ACL1, direction);
         assertEquals(2, map.size());
         assertAclInterfaces(map.get(ACL2.getValue()), PORT1, PORT2);