Bug 8764: Fix handling of old negative

Change-Id: Ia3295c955ecf7b364865e445cb7f75a0ec80a6bf
Signed-off-by: Lorand Jakab <lojakab@cisco.com>

diff --git a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingService.java b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingService.java
index 14acd328e95884e34745b7bc9eb945ae9db159a1..17673e8a12d854467715b3be010be50bb0038915 100644 (file)
--- a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingService.java
+++ b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingService.java
@@ -306,6 +306,9 @@ public class MappingService implements OdlMappingserviceService, IMappingService
 
     @Override
     public void removeMapping(MappingOrigin origin, Eid key) {
+        if (origin.equals(MappingOrigin.Southbound)) {
+            mappingSystem.removeMapping(origin, key);
+        }
         dsbe.removeMapping(DSBEInputUtil.toMapping(origin, key));
     }
 

diff --git a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingSystem.java b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingSystem.java
index 52d95e34b0c90ad07c7f552187558bd3ce71d458..b25e5986bb186b6bd1d1739f38111fcea1ec625b 100644 (file)
--- a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingSystem.java
+++ b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/MappingSystem.java
@@ -8,7 +8,6 @@
 
 package org.opendaylight.lispflowmapping.implementation;
 
-import java.util.AbstractMap.SimpleImmutableEntry;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.EnumMap;
@@ -417,13 +416,17 @@ public class MappingSystem implements IMappingSystem {
         dsbe.removeXtrIdMapping(DSBEInputUtil.toXtrIdMapping(mappingData));
     }
 
+    @SuppressWarnings("unchecked")
     private void removeSbMapping(Eid key, MappingData mappingData) {
         if (mappingData != null && mappingData.getXtrId() != null) {
             removeSbXtrIdSpecificMapping(key, mappingData.getXtrId(), mappingData);
         }
+
         removeFromSbTimeoutService(key);
+        Set<Subscriber> subscribers = (Set<Subscriber>) getData(MappingOrigin.Southbound, key, SubKeys.SUBSCRIBERS);
         smc.removeMapping(key);
         dsbe.removeMapping(DSBEInputUtil.toMapping(MappingOrigin.Southbound, key, mappingData));
+        notifyChange(mappingData, subscribers, null, MappingChange.Removed);
     }
 
     private void removeFromSbTimeoutService(Eid key) {
@@ -476,16 +479,17 @@ public class MappingSystem implements IMappingSystem {
         MappingData mapping = (MappingData) tableMap.get(origin).getMapping(null, key);
 
         if (mapping != null) {
+            MappingData notificationMapping = mapping;
             subscribers = (Set<Subscriber>) getData(MappingOrigin.Southbound, key, SubKeys.SUBSCRIBERS);
             // For SrcDst LCAF also send SMRs to Dst prefix
             if (key.getAddress() instanceof SourceDestKey) {
                 Eid dstAddr = SourceDestKeyHelper.getDstBinary(key);
                 dstSubscribers = (Set<Subscriber>) getData(MappingOrigin.Southbound, dstAddr, SubKeys.SUBSCRIBERS);
                 if (!(mapping.getRecord().getEid().getAddress() instanceof SourceDestKey)) {
-                    mapping = new MappingData(new MappingRecordBuilder().setEid(key).build());
+                    notificationMapping = new MappingData(new MappingRecordBuilder().setEid(key).build());
                 }
             }
-            notifyChange(mapping, subscribers, dstSubscribers, MappingChange.Removed);
+            notifyChange(notificationMapping, subscribers, dstSubscribers, MappingChange.Removed);
         }
 
         if (origin == MappingOrigin.Northbound) {
@@ -495,12 +499,7 @@ public class MappingSystem implements IMappingSystem {
         if (origin == MappingOrigin.Southbound) {
             removeFromSbTimeoutService(key);
             if (mapping != null && mapping.isPositive().or(false)) {
-                SimpleImmutableEntry<Eid, Set<Subscriber>> mergedNegativePrefix = computeMergedNegativePrefix(key);
-                if (mergedNegativePrefix != null) {
-                    addNegativeMapping(mergedNegativePrefix.getKey());
-                    subscribers = mergedNegativePrefix.getValue();
-                    notifyChange(mapping, subscribers, null, MappingChange.Created);
-                }
+                mergeNegativePrefixes(key);
             }
         }
         tableMap.get(origin).removeMapping(key);
@@ -518,21 +517,17 @@ public class MappingSystem implements IMappingSystem {
     }
 
 
-    @SuppressWarnings("unchecked")
     /*
-     * Returns the "merged" prefix and the subscribers of the prefixes that were merged.
+     * Merges adjacent negative prefixes and notifies their subscribers.
      */
-    private SimpleImmutableEntry<Eid, Set<Subscriber>> computeMergedNegativePrefix(Eid eid) {
-        // Variable to hold subscribers we collect along the way
-        Set<Subscriber> subscribers = null;
-
+    private void mergeNegativePrefixes(Eid eid) {
         // If prefix sibling has a negative mapping, save its subscribers
         Eid sibling = smc.getSiblingPrefix(eid);
         MappingData mapping = (MappingData) smc.getMapping(null, sibling);
         if (mapping != null && mapping.isNegative().or(false)) {
-            subscribers = (Set<Subscriber>) getData(MappingOrigin.Southbound, eid, SubKeys.SUBSCRIBERS);
+            removeSbMapping(sibling, mapping);
         } else {
-            return null;
+            return;
         }
 
         Eid currentNode = sibling;
@@ -540,15 +535,13 @@ public class MappingSystem implements IMappingSystem {
         while ((currentNode = smc.getVirtualParentSiblingPrefix(currentNode)) != null) {
             mapping = (MappingData) smc.getMapping(null, currentNode);
             if (mapping != null && mapping.isNegative().or(false)) {
-                subscribers.addAll((Set<Subscriber>)
-                        getData(MappingOrigin.Southbound, currentNode, SubKeys.SUBSCRIBERS));
                 removeSbMapping(currentNode, mapping);
             } else {
                 break;
             }
             previousNode = currentNode;
         }
-        return new SimpleImmutableEntry<>(getVirtualParent(previousNode), subscribers);
+        addNegativeMapping(getVirtualParent(previousNode));
     }
 
     private static Eid getVirtualParent(Eid eid) {
@@ -698,10 +691,6 @@ public class MappingSystem implements IMappingSystem {
         buildMapCaches();
     }
 
-    /*
-     * XXX  Mappings and keys should be separated for this to work properly, as is it will remove northbound originated
-     * authentication keys too, since they are currently stored in smc.
-     */
     public void cleanSBMappings() {
         smc = new SimpleMapCache(sdao);
     }

diff --git a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServer.java b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServer.java
index eb6d666159213fcc3a46063a4d9ad089a21c79f3..f281bd896ce118a819520ab2f1dcdb6c823170eb 100644 (file)
--- a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServer.java
+++ b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServer.java
@@ -10,6 +10,7 @@ package org.opendaylight.lispflowmapping.implementation.lisp;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import java.net.InetAddress;
 import java.net.NetworkInterface;
@@ -104,9 +105,8 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener, IS
     @SuppressWarnings("unchecked")
     public void handleMapRegister(MapRegister mapRegister) {
         boolean mappingUpdated = false;
-        boolean oldMappingRemoved = false;
         boolean merge = ConfigIni.getInstance().mappingMergeIsSet() && mapRegister.isMergeEnabled();
-        Set<Subscriber> subscribers = null;
+        Set<Subscriber> subscribers = Sets.newConcurrentHashSet();
         MappingRecord oldMapping;
 
         if (merge) {
@@ -127,9 +127,21 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener, IS
 
             oldMapping = getMappingRecord(mapService.getMapping(MappingOrigin.Southbound, eid));
             mapService.addMapping(MappingOrigin.Southbound, eid, getSiteId(mapRegister), mappingData);
-            if (oldMapping != null && MappingRecordUtil.isNegativeMapping(oldMapping)) {
+            if (oldMapping != null
+                    && MappingRecordUtil.isNegativeMapping(oldMapping)
+                    && !oldMapping.getEid().equals(eid)) {
+                if (subscriptionService) {
+                    // Here we save the subscribers of the OLD mapping before removing. We will add to this set the
+                    // subscribers of the NEW mapping below (since the EIDs are different, the result of
+                    // mappingChanged() will be true, and then send an SMR to all subscribers with the EID of the NEW
+                    // mapping only.
+                    Set<Subscriber> oldMappingSubscribers = getSubscribers(oldMapping.getEid());
+                    if (oldMappingSubscribers != null) {
+                        subscribers.addAll(oldMappingSubscribers);
+                        LoggingUtil.logSubscribers(LOG, oldMapping.getEid(), subscribers);
+                    }
+                }
                 mapService.removeMapping(MappingOrigin.Southbound, oldMapping.getEid());
-                oldMappingRemoved = true;
             }
 
             if (subscriptionService) {
@@ -139,19 +151,17 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener, IS
                 if (mappingChanged(oldMapping, newMapping)) {
                     if (LOG.isDebugEnabled()) {
                         LOG.debug("Mapping update occured for {} SMRs will be sent for its subscribers.",
-                                LispAddressStringifier.getString(mapping.getEid()));
+                                LispAddressStringifier.getString(eid));
                     }
-                    subscribers = getSubscribers(eid);
+                    Set<Subscriber> newMappingSubscribers = getSubscribers(eid);
                     if (oldMapping != null && !oldMapping.getEid().equals(eid)) {
-                        subscribers = addParentSubscribers(eid, subscribers);
+                        newMappingSubscribers = addParentSubscribers(eid, newMappingSubscribers);
                     }
-                    LoggingUtil.logSubscribers(LOG, eid, subscribers);
-                    handleSmr(eid, subscribers);
-                    if (oldMapping != null && oldMappingRemoved && !oldMapping.getEid().equals(eid)) {
-                        subscribers = getSubscribers(oldMapping.getEid());
-                        LoggingUtil.logSubscribers(LOG, oldMapping.getEid(), subscribers);
-                        handleSmr(oldMapping.getEid(), subscribers);
+                    if (newMappingSubscribers != null) {
+                        subscribers.addAll(newMappingSubscribers);
+                        LoggingUtil.logSubscribers(LOG, eid, subscribers);
                     }
+                    handleSmr(eid, subscribers);
                     mappingUpdated = true;
                 }
             }

diff --git a/mappingservice/implementation/src/test/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServerTest.java b/mappingservice/implementation/src/test/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServerTest.java
index b41beb2339fc0ea7dfc360327a284f852d529aea..6659ecae8f296717afda5f52a1ae19ae2aed493d 100644 (file)
--- a/mappingservice/implementation/src/test/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServerTest.java
+++ b/mappingservice/implementation/src/test/java/org/opendaylight/lispflowmapping/implementation/lisp/MapServerTest.java
@@ -180,7 +180,7 @@ public class MapServerTest {
                 .setAuthenticationData(null).build(), null);
 
         // only 1 subscriber has timed out.
-        assertEquals(1, subscriberSetMock_1.size());
+        assertEquals(2, subscriberSetMock_1.size());
     }
 
     @Test
@@ -329,7 +329,7 @@ public class MapServerTest {
 
         // for Ipv4 mapping
         final ArgumentCaptor<MapRequest> captor_3 = ArgumentCaptor.forClass(MapRequest.class);
-        Mockito.verify(notifyHandler, Mockito.times(1)).handleSMR(captor_3.capture(), Mockito.eq(RLOC_6));
+        Mockito.verify(notifyHandler, Mockito.times(2)).handleSMR(captor_3.capture(), Mockito.eq(RLOC_6));
         final Eid resultEid_3 = captor_3.getValue().getEidItem().iterator().next().getEid();
         assertEquals(IPV4_SOURCE_EID_6, resultEid_3);
     }