Bug 5259: Fix NPE in serializer when merge is on

Avoid null records in map-notify by ensuring that newly registered
mappings are actually stored before building the map-notify.

Change-Id: Ied01de30a9449b4dbf54dc6da0ee37066be2c164
Signed-off-by: Florin Coras <fcoras@cisco.com>

diff --git a/mappingservice/api/src/main/java/org/opendaylight/lispflowmapping/interfaces/mapcache/IMappingSystem.java b/mappingservice/api/src/main/java/org/opendaylight/lispflowmapping/interfaces/mapcache/IMappingSystem.java
index 57c5faa216a4bfd9c7a80fe5f2d0eec467e00363..95f93b4b0f22a82754363658e3e0a3b779aac738 100644 (file)
--- a/mappingservice/api/src/main/java/org/opendaylight/lispflowmapping/interfaces/mapcache/IMappingSystem.java
+++ b/mappingservice/api/src/main/java/org/opendaylight/lispflowmapping/interfaces/mapcache/IMappingSystem.java
@@ -10,6 +10,7 @@ package org.opendaylight.lispflowmapping.interfaces.mapcache;
 
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.eid.container.Eid;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.MappingOrigin;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.SiteId;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.mapping.authkey.container.MappingAuthkey;
 
 /**
@@ -32,6 +33,20 @@ public interface IMappingSystem {
      */
     void addMapping(MappingOrigin origin, Eid key, Object data);
 
+    /**
+     * Add mapping and persist to datastore
+     *
+     * @param origin
+     *            Table where mapping should be added
+     * @param siteId
+     *            SiteID of SB device doing the registration
+     * @param key
+     *            Key of the mapping
+     * @param data
+     *            Value to be stored
+     */
+    void addMapping(MappingOrigin origin, SiteId siteId, Eid key, Object data);
+
     /**
      * Retrieves mapping for the provided src and dst key.
      *

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 855ffff1180136e17217a12636b9e63cbf6c5811..9d677b1269f1e440bbe25653406f6941218bc23d 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
@@ -356,8 +356,12 @@ public class MappingService implements OdlMappingserviceService, IMappingService
 
     @Override
     public void addMapping(MappingOrigin origin, Eid key, SiteId siteId, Object data) {
-        dsbe.addMapping(DSBEInputUtil.toMapping(origin, key, siteId, (MappingRecord) data));
-        mappingSystem.updateMappingRegistration(origin, key);
+        // SB registrations are first written to the MappingSystem and only afterwards are persisted to the datastore
+        if (origin.equals(MappingOrigin.Southbound)) {
+            mappingSystem.addMapping(origin, siteId, key, data);
+        } else {
+            dsbe.addMapping(DSBEInputUtil.toMapping(origin, key, siteId, (MappingRecord) data));
+        }
     }
 
     @Override

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 3f5fd1c8fa88dbd8452d2075874093e67b38306b..fbb63dd72b52094b16c46933d26d21236b0af39f 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
@@ -18,6 +18,7 @@ import org.opendaylight.lispflowmapping.implementation.mapcache.FlatMapCache;
 import org.opendaylight.lispflowmapping.implementation.mapcache.MultiTableMapCache;
 import org.opendaylight.lispflowmapping.implementation.mapcache.SimpleMapCache;
 import org.opendaylight.lispflowmapping.implementation.mdsal.DataStoreBackEnd;
+import org.opendaylight.lispflowmapping.implementation.util.DSBEInputUtil;
 import org.opendaylight.lispflowmapping.implementation.util.MappingMergeUtil;
 import org.opendaylight.lispflowmapping.interfaces.dao.ILispDAO;
 import org.opendaylight.lispflowmapping.interfaces.mapcache.IMapCache;
@@ -37,6 +38,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.ma
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.mapping.record.container.MappingRecordBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.rloc.container.Rloc;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.MappingOrigin;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.SiteId;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.db.instance.AuthenticationKey;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.db.instance.Mapping;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.mapping.authkey.container.MappingAuthkey;
@@ -107,6 +109,13 @@ public class MappingSystem implements IMappingSystem {
         tableMap.put(MappingOrigin.Southbound, smc);
     }
 
+    public void addMapping(MappingOrigin origin, SiteId siteId, Eid key, Object data) {
+        // Store data first in MapCache and only afterwards persist to datastore. This should be used only for SB
+        // registrations
+        addMapping(origin, key, data);
+        dsbe.addMapping(DSBEInputUtil.toMapping(origin, key, siteId, (MappingRecord) data));
+    }
+
     public void addMapping(MappingOrigin origin, Eid key, Object value) {
         tableMap.get(origin).addMapping(key, value, origin == MappingOrigin.Southbound ? overwrite : true);
     }

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 abae8ae6ba71fb4e9397ebaf97713c0be3d34ebd..a8f5ba0ad80106f09c42f56119775b0463c8c101 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
@@ -100,8 +100,7 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener {
 
     @SuppressWarnings("unchecked")
     public void handleMapRegister(MapRegister mapRegister) {
-        boolean failed = false;
-        boolean updated = false;
+        boolean authFailed = false;
         String password = null;
         Set<SubscriberRLOC> subscribers = null;
         MappingRecord oldMapping;
@@ -115,28 +114,27 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener {
                 }
                 if (!LispAuthenticationUtil.validate(mapRegister, password)) {
                     LOG.warn("Authentication failed");
-                    failed = true;
+                    authFailed = true;
                     break;
                 }
             }
             oldMapping = (MappingRecord) mapService.getMapping(MappingOrigin.Southbound, mapping.getEid());
+            mapService.addMapping(MappingOrigin.Southbound, mapping.getEid(), getSiteId(mapRegister), mapping);
 
-            if (subscriptionService && mappingChanged(oldMapping, mapping)) {
-                if (LOG.isDebugEnabled()){
-                    LOG.debug("Mapping update occured for {} SMRs will be sent for its subscribers.",
-                            LispAddressStringifier.getString(mapping.getEid()));
+            if (subscriptionService) {
+                MappingRecord newMapping = ConfigIni.getInstance().mappingMergeIsSet()
+                        ? (MappingRecord) mapService.getMapping(MappingOrigin.Southbound, mapping.getEid()) : mapping;
+                if (mappingChanged(oldMapping, newMapping)) {
+                    if (LOG.isDebugEnabled()) {
+                        LOG.debug("Mapping update occured for {} SMRs will be sent for its subscribers.",
+                                LispAddressStringifier.getString(mapping.getEid()));
+                    }
+                    subscribers = getSubscribers(mapping.getEid());
+                    sendSmrs(mapping, subscribers);
                 }
-                subscribers = getSubscribers(mapping.getEid());
-                updated = true;
-            }
-            // Must update the record before sending SMRs
-            mapService.addMapping(MappingOrigin.Southbound, mapping.getEid(), getSiteId(mapRegister), mapping);
-            if (updated) {
-                updated = false;
-                sendSmrs(mapping, subscribers);
             }
         }
-        if (!failed && BooleanUtils.isTrue(mapRegister.isWantMapNotify())) {
+        if (!authFailed && BooleanUtils.isTrue(mapRegister.isWantMapNotify())) {
             LOG.trace("MapRegister wants MapNotify");
             MapNotifyBuilder builder = new MapNotifyBuilder();
             List<TransportAddress> rlocs = null;
@@ -226,25 +224,22 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener {
 
     private void sendSmrs(MappingRecord record, Set<SubscriberRLOC> subscribers) {
         Eid eid = record.getEid();
-        handleSmr(record, subscribers, notifyHandler);
+        handleSmr(eid, subscribers, notifyHandler);
 
         // For SrcDst LCAF also send SMRs to Dst prefix
         if (eid.getAddress() instanceof SourceDestKey) {
             Eid dstAddr = SourceDestKeyHelper.getDst(eid);
             Set<SubscriberRLOC> dstSubs = getSubscribers(dstAddr);
-            MappingRecord newRecord = new MappingRecordBuilder().setAction(record.getAction())
-                    .setAuthoritative(record.isAuthoritative()).setLocatorRecord(record.getLocatorRecord())
-                    .setMapVersion(record.getMapVersion()).setRecordTtl(record.getRecordTtl())
-                    .setEid(dstAddr).build();
-            handleSmr(newRecord, dstSubs, notifyHandler);
+            MappingRecord newRecord = new MappingRecordBuilder(record).setEid(dstAddr).build();
+            handleSmr(newRecord.getEid(), dstSubs, notifyHandler);
         }
     }
 
-    private void handleSmr(MappingRecord record, Set<SubscriberRLOC> subscribers, IMapNotifyHandler callback) {
+    private void handleSmr(Eid eid, Set<SubscriberRLOC> subscribers, IMapNotifyHandler callback) {
         if (subscribers == null) {
             return;
         }
-        MapRequestBuilder mrb = MapRequestUtil.prepareSMR(record.getEid(), LispAddressUtil.toRloc(getLocalAddress()));
+        MapRequestBuilder mrb = MapRequestUtil.prepareSMR(eid, LispAddressUtil.toRloc(getLocalAddress()));
         LOG.trace("Built SMR packet: " + mrb.build().toString());
         for (SubscriberRLOC subscriber : subscribers) {
             if (subscriber.timedOut()) {
@@ -262,7 +257,7 @@ public class MapServer implements IMapServerAsync, OdlMappingserviceListener {
                 }
             }
         }
-        addSubscribers(record.getEid(), subscribers);
+        addSubscribers(eid, subscribers);
     }
 
     @SuppressWarnings("unchecked")

diff --git a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/DataStoreBackEnd.java b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/DataStoreBackEnd.java
index bf955351a17d80e4e9929438814541a0de69233d..22a5948e651cae271d73f8dcaf8563c3e4d360ca 100644 (file)
--- a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/DataStoreBackEnd.java
+++ b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/DataStoreBackEnd.java
@@ -42,12 +42,9 @@ import com.google.common.util.concurrent.Futures;
  */
 public class DataStoreBackEnd implements TransactionChainListener {
     protected static final Logger LOG = LoggerFactory.getLogger(DataStoreBackEnd.class);
-
-    private DataBroker broker;
     private BindingTransactionChain txChain;
 
     public DataStoreBackEnd(DataBroker broker) {
-        this.broker = broker;
         this.txChain = broker.createTransactionChain(this);
     }
 

diff --git a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/MappingDataListener.java b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/MappingDataListener.java
index 37bb69a53ce2949e088b8691c875ef07a751b2aa..bec12db39782578cd85109124e1bf44338aa15d6 100644 (file)
--- a/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/MappingDataListener.java
+++ b/mappingservice/implementation/src/main/java/org/opendaylight/lispflowmapping/implementation/mdsal/MappingDataListener.java
@@ -61,6 +61,12 @@ public class MappingDataListener extends AbstractDataListener {
             if (entry.getValue() instanceof Mapping) {
                 Mapping mapping = (Mapping)entry.getValue();
 
+                // Only treat mapping changes caused by Northbound, since Southbound changes are already handled
+                // before being persisted. XXX separate NB and SB to avoid ignoring SB notifications
+                if (mapping.getOrigin() == MappingOrigin.Southbound) {
+                    continue;
+                }
+
                 LOG.trace("Received created data");
                 LOG.trace("Key: {}", entry.getKey());
                 LOG.trace("Value: {}", mapping);
@@ -68,15 +74,12 @@ public class MappingDataListener extends AbstractDataListener {
                 mapSystem.addMapping(mapping.getOrigin(), mapping.getMappingRecord().getEid(),
                         mapping.getMappingRecord());
 
-                if (mapping.getOrigin() == MappingOrigin.Northbound) {
-                    // Only publish notifications for mapping changes caused by Northbound, since Southbound has a
-                    // dedicated code path for detecting mapping updates. The notifications are used for sending SMR.
-                    try {
-                        notificationPublishService.putNotification(MSNotificationInputUtil.toMappingChanged(mapping,
-                                MappingChange.Created));
-                    } catch (InterruptedException e) {
-                        LOG.warn("Notification publication interrupted!");
-                    }
+                try {
+                    // The notifications are used for sending SMR.
+                    notificationPublishService.putNotification(MSNotificationInputUtil.toMappingChanged(mapping,
+                            MappingChange.Created));
+                } catch (InterruptedException e) {
+                    LOG.warn("Notification publication interrupted!");
                 }
             }
         }
@@ -87,6 +90,12 @@ public class MappingDataListener extends AbstractDataListener {
             if (entry.getValue() instanceof Mapping) {
                 Mapping mapping = (Mapping)entry.getValue();
 
+                // Only treat mapping changes caused by Northbound, since Southbound changes are already handled
+                // before being persisted.
+                if (mapping.getOrigin() == MappingOrigin.Southbound) {
+                    continue;
+                }
+
                 LOG.trace("Received changed data");
                 LOG.trace("Key: {}", entry.getKey());
                 LOG.trace("Value: {}", entry.getValue());
@@ -94,15 +103,12 @@ public class MappingDataListener extends AbstractDataListener {
                 mapSystem.addMapping(mapping.getOrigin(), mapping.getMappingRecord().getEid(),
                         mapping.getMappingRecord());
 
-                if (mapping.getOrigin() == MappingOrigin.Northbound) {
-                    // Only publish notifications for mapping changes caused by Northbound, since Southbound has a
-                    // dedicated code path for detecting mapping updates. The notifications are used for sending SMR.
-                    try {
-                        notificationPublishService.putNotification(MSNotificationInputUtil.toMappingChanged(mapping,
-                                MappingChange.Updated));
-                    } catch (InterruptedException e) {
-                        LOG.warn("Notification publication interrupted!");
-                    }
+                // The notifications are used for sending SMR.
+                try {
+                    notificationPublishService.putNotification(MSNotificationInputUtil.toMappingChanged(mapping,
+                            MappingChange.Updated));
+                } catch (InterruptedException e) {
+                    LOG.warn("Notification publication interrupted!");
                 }
             }
         }
@@ -114,6 +120,12 @@ public class MappingDataListener extends AbstractDataListener {
             if (dataObject instanceof Mapping) {
                 Mapping mapping = (Mapping)dataObject;
 
+                // Only treat mapping changes caused by Northbound, since Southbound changes are already handled
+                // before being persisted.
+                if (mapping.getOrigin() == MappingOrigin.Southbound) {
+                    continue;
+                }
+
                 LOG.trace("Received deleted data");
                 LOG.trace("Key: {}", entry);
                 LOG.trace("Value: {}", dataObject);