Bug 8746: Multi-threading improvements

Fix southbound Map-Register cache authentication update code

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

diff --git a/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/AuthenticationKeyDataListener.java b/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/AuthenticationKeyDataListener.java
index 036c85768a4612775a5dd9133c56cc441b8cfb81..95ae64eb60682428955c324e1bc7374afd8aed4a 100644 (file)
--- a/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/AuthenticationKeyDataListener.java
+++ b/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/AuthenticationKeyDataListener.java
@@ -8,6 +8,8 @@
 package org.opendaylight.lispflowmapping.southbound.lisp;
 
 import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
 import org.opendaylight.controller.md.sal.binding.api.ClusteredDataTreeChangeListener;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.DataObjectModification;
@@ -18,6 +20,7 @@ import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.lispflowmapping.lisp.util.LispAddressUtil;
 import org.opendaylight.lispflowmapping.mapcache.AuthKeyDb;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.eid.container.Eid;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.lisp.proto.rev151105.map.register.cache.metadata.container.map.register.cache.metadata.EidLispAddress;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.lfm.mappingservice.rev150906.MappingDatabase;
 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.AuthenticationKeyBuilder;
@@ -38,9 +41,7 @@ public class AuthenticationKeyDataListener implements ClusteredDataTreeChangeLis
     private final DataBroker broker;
     private final InstanceIdentifier<AuthenticationKey> path;
     private ListenerRegistration<ClusteredDataTreeChangeListener<AuthenticationKey>> registration;
-    private volatile boolean authKeyRefreshing = false;
-    private volatile long authKeyRefreshingDate;
-
+    private ConcurrentHashMap<Eid, Long> updatedEntries;
 
     public AuthenticationKeyDataListener(final DataBroker broker, final AuthKeyDb akdb) {
         this.broker = broker;
@@ -51,28 +52,15 @@ public class AuthenticationKeyDataListener implements ClusteredDataTreeChangeLis
         final DataTreeIdentifier<AuthenticationKey> dataTreeIdentifier = new DataTreeIdentifier<>(
                 LogicalDatastoreType.CONFIGURATION, path);
         registration = broker.registerDataTreeChangeListener(dataTreeIdentifier, this);
+        this.updatedEntries = new ConcurrentHashMap<>();
     }
 
     public void closeDataChangeListener() {
         registration.close();
     }
 
-    public boolean isAuthKeyRefreshing() {
-        return authKeyRefreshing;
-    }
-
-    public void setAuthKeyRefreshing(boolean authKeyRefreshing) {
-        this.authKeyRefreshing = authKeyRefreshing;
-    }
-
-    public long getAuthKeyRefreshingDate() {
-        return authKeyRefreshingDate;
-    }
-
     @Override
-    public void onDataTreeChanged(Collection<DataTreeModification<AuthenticationKey>> changes) {
-        authKeyRefreshing = true;
-        authKeyRefreshingDate = System.currentTimeMillis();
+    public synchronized void onDataTreeChanged(Collection<DataTreeModification<AuthenticationKey>> changes) {
         for (DataTreeModification<AuthenticationKey> change : changes) {
             final DataObjectModification<AuthenticationKey> mod = change.getRootNode();
 
@@ -86,6 +74,7 @@ public class AuthenticationKeyDataListener implements ClusteredDataTreeChangeLis
                 final AuthenticationKey convertedAuthKey = convertToBinaryIfNecessary(authKey);
 
                 akdb.removeAuthenticationKey(convertedAuthKey.getEid());
+                updatedEntries.put(convertedAuthKey.getEid(), System.currentTimeMillis());
             } else if (ModificationType.WRITE == mod.getModificationType() || ModificationType.SUBTREE_MODIFIED == mod
                     .getModificationType()) {
                 if (ModificationType.WRITE == mod.getModificationType()) {
@@ -102,12 +91,41 @@ public class AuthenticationKeyDataListener implements ClusteredDataTreeChangeLis
                 final AuthenticationKey convertedAuthKey = convertToBinaryIfNecessary(authKey);
 
                 akdb.addAuthenticationKey(convertedAuthKey.getEid(), convertedAuthKey.getMappingAuthkey());
+                updatedEntries.put(convertedAuthKey.getEid(), System.currentTimeMillis());
             } else {
                 LOG.warn("Ignoring unhandled modification type {}", mod.getModificationType());
             }
         }
     }
 
+    /**
+     * We maintain a HashMap with the update times of AuthenticationKey objects in the updatedEntries field. We keep
+     * entries in the HashMap for the Map-Register cache timeout interval, and lazy remove them afterwards. As a result
+     * the same EID will be considered updated during that interval, even on subsequent queries. This is necessary
+     * because more than one xTR may register the same EID, and to avoid complexity we don't store origin information.
+     * The performance trade-off is not significant, because during a typical cache timeout the same xTR will send only
+     * a few registration packets (2 for the default value of 90s, when UDP Map-Registers are sent at 1 minute
+     * intervals).
+     *
+     * @param eids List of EIDs to check
+     * @param timeout MapRegister cache timeout value
+     * @return false if any of the EIDs in the eids list was updated in the last timout period, true otherwise
+     */
+    public synchronized boolean authKeysForEidsUnchanged(List<EidLispAddress> eids, long timeout) {
+        boolean result = true;
+        Long currentTime = System.currentTimeMillis();
+        for (EidLispAddress eidLispAddress : eids) {
+            Long updateTime = updatedEntries.get(eidLispAddress.getEid());
+            if (updateTime != null) {
+                result = false;
+                if (currentTime - updateTime > timeout) {
+                    updatedEntries.remove(eidLispAddress.getEid());
+                }
+            }
+        }
+        return result;
+    }
+
     private static AuthenticationKey convertToBinaryIfNecessary(AuthenticationKey authKey) {
         Eid originalEid = authKey.getEid();
         if (LispAddressUtil.addressNeedsConversionToBinary(originalEid.getAddress())) {

diff --git a/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundHandler.java b/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundHandler.java
index c12f5500b3f01548cb1f9d6de54b31004f73eda6..239852ea2fca3f0e457310fc650460a93597d4d3 100644 (file)
--- a/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundHandler.java
+++ b/mappingservice/southbound/src/main/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundHandler.java
@@ -263,28 +263,21 @@ public class LispSouthboundHandler extends SimpleChannelInboundHandler<DatagramP
     }
 
     private MapRegisterCacheValue refreshAuthKeyIfNecessary(MapRegisterCacheValue mapRegisterCacheValue) {
-        if (authenticationKeyDataListener.isAuthKeyRefreshing()) {
-            final boolean shouldAuthKeyRefreshingStop = System.currentTimeMillis() - authenticationKeyDataListener
-                    .getAuthKeyRefreshingDate() > mapRegisterCacheTimeout;
-            if (shouldAuthKeyRefreshingStop) {
-                authenticationKeyDataListener.setAuthKeyRefreshing(false);
-            } else {
-                final MappingAuthkey mappingAuthkey = provideAuthenticateKey(mapRegisterCacheValue
-                        .getMapRegisterCacheMetadata().getEidLispAddress());
-
-                final MapRegisterCacheValueBuilder newMapRegisterCacheValueBuilder = new MapRegisterCacheValueBuilder(
-                        mapRegisterCacheValue);
-                final MapRegisterCacheMetadataBuilder newMapRegisterCacheMetadataBuilder =
-                        new MapRegisterCacheMetadataBuilder(mapRegisterCacheValue.getMapRegisterCacheMetadata());
+        final List<EidLispAddress> eids = mapRegisterCacheValue.getMapRegisterCacheMetadata().getEidLispAddress();
 
-                newMapRegisterCacheValueBuilder.setMappingAuthkey(mappingAuthkey);
-                newMapRegisterCacheValueBuilder.setMapRegisterCacheMetadata(newMapRegisterCacheMetadataBuilder.build());
-                return newMapRegisterCacheValueBuilder.build();
-            }
+        if (authenticationKeyDataListener.authKeysForEidsUnchanged(eids, mapRegisterCacheTimeout)) {
+            return mapRegisterCacheValue;
         }
 
-        return mapRegisterCacheValue;
+        final MappingAuthkey mappingAuthkey = provideAuthenticateKey(eids);
+        final MapRegisterCacheValueBuilder newMapRegisterCacheValueBuilder = new MapRegisterCacheValueBuilder(
+                mapRegisterCacheValue);
+        final MapRegisterCacheMetadataBuilder newMapRegisterCacheMetadataBuilder =
+                new MapRegisterCacheMetadataBuilder(mapRegisterCacheValue.getMapRegisterCacheMetadata());
 
+        newMapRegisterCacheValueBuilder.setMappingAuthkey(mappingAuthkey);
+        newMapRegisterCacheValueBuilder.setMapRegisterCacheMetadata(newMapRegisterCacheMetadataBuilder.build());
+        return newMapRegisterCacheValueBuilder.build();
     }
 
     private MapRegisterCacheValue resolveCacheValue(Map.Entry<MapRegisterCacheKey, byte[]> entry) {

diff --git a/mappingservice/southbound/src/test/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundServiceTest.java b/mappingservice/southbound/src/test/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundServiceTest.java
index 71e5a14e34a020666a1068decd7d5163ada10be5..6dff007db8be23e07be8a2f46ee2ffda09993c4d 100644 (file)
--- a/mappingservice/southbound/src/test/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundServiceTest.java
+++ b/mappingservice/southbound/src/test/java/org/opendaylight/lispflowmapping/southbound/lisp/LispSouthboundServiceTest.java
@@ -102,6 +102,7 @@ public class LispSouthboundServiceTest extends BaseTestCase {
     private static final long CACHE_RECORD_TIMEOUT = 90000;
 
     private static AuthKeyDb akdb;
+    private static AuthenticationKeyDataListener akdl;
 
     private interface MapReplyIpv4SingleLocatorPos {
         int RECORD_COUNT = 3;
@@ -142,6 +143,9 @@ public class LispSouthboundServiceTest extends BaseTestCase {
                 .thenReturn(new MappingAuthkeyBuilder().setKeyType(1).setKeyString("password").build());
         Mockito.when(akdb.getAuthenticationKey(Matchers.eq(LispAddressUtil.asIpv4PrefixBinaryEid("172.1.1.2/32"))))
                 .thenReturn(new MappingAuthkeyBuilder().setKeyType(1).setKeyString("password").build());
+
+        akdl = Mockito.mock(AuthenticationKeyDataListener.class);
+        Mockito.when(akdl.authKeysForEidsUnchanged(Mockito.anyList(), Mockito.anyLong())).thenReturn(true);
     }
 
     @Override
@@ -158,7 +162,7 @@ public class LispSouthboundServiceTest extends BaseTestCase {
         testedLispService.setMapRegisterCache(mapRegisterCache);
         testedLispService.setDataBroker(Mockito.mock(DataBroker.class));
         testedLispService.setAuthKeyDb(akdb);
-        testedLispService.setAuthenticationKeyDataListener(Mockito.mock(AuthenticationKeyDataListener.class));
+        testedLispService.setAuthenticationKeyDataListener(akdl);
         nps = context.mock(NotificationPublishService.class);
         testedLispService.setNotificationProvider(nps);
         lispSouthboundStats = new ConcurrentLispSouthboundStats();