From: Shashidhar Raja Date: Wed, 20 Feb 2019 15:21:01 +0000 (+0530) Subject: ACL: VM IP address failures and ID Pool issues X-Git-Tag: release/sodium~22 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=0d87e1f4598ecff9b37f987632eee86f487b6afc;p=netvirt.git 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 --- 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 3b78577f1c..59927b45ec 100644 --- 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 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 75985301db..d939e25275 100644 --- 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 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 aceList = acl.getAccessListEntries().getAce(); Collection aclInterfaces = @@ -118,11 +115,6 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase 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 interfacesBefore = ImmutableSet.copyOf(aclDataUtil.getInterfaceList(new Uuid(aclName))); @@ -163,15 +155,10 @@ public class AclEventListener extends AsyncDataTreeChangeListenerBase 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 c9ac31dbb3..3bad9ac6e9 100644 --- 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 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 dda987e04c..2ba6eae7df 100644 --- 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 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 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 704b68e5dc..9620850f04 100644 --- 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 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 5e50483f91..f9eabbf061 100644 --- 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);