Get rid of useless (Hwvtep)SouthboundProvider thread

The thread instantiated here is useless and the comment is misleading:
ListenerRegistration.close() is specified to be safe in any context
and idempotent. Just get rid of the thread and close the registration
immediately.

This flushes out an immediate problem, where the test would deadlock
due to base test using directExecutor() for DTCLs -- i.e. simulating
execution which does not happen in production. To deal with that we
switch the test to use asynchronous DTCLs and add a simple synchronization
loop to wait for DTCL to trigger.

JIRA: OVSDB-454
Change-Id: I2e86acb9eb30529fcd5d5675005d6c81bd804ba5
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/hwvtepsouthbound/hwvtepsouthbound-impl/src/main/java/org/opendaylight/ovsdb/hwvtepsouthbound/HwvtepSouthboundProvider.java b/hwvtepsouthbound/hwvtepsouthbound-impl/src/main/java/org/opendaylight/ovsdb/hwvtepsouthbound/HwvtepSouthboundProvider.java
index b8dd802751359b84fd2a57e36a7a4f3547358404..322fa4eb86ed988ba2a81ab82a2d790ed0a1e9a3 100644 (file)
--- a/hwvtepsouthbound/hwvtepsouthbound-impl/src/main/java/org/opendaylight/ovsdb/hwvtepsouthbound/HwvtepSouthboundProvider.java
+++ b/hwvtepsouthbound/hwvtepsouthbound-impl/src/main/java/org/opendaylight/ovsdb/hwvtepsouthbound/HwvtepSouthboundProvider.java
@@ -149,7 +149,7 @@ public class HwvtepSouthboundProvider implements ClusteredDataTreeChangeListener
         }
     }
 
-    private void initializeHwvtepTopology(LogicalDatastoreType type) {
+    private void initializeHwvtepTopology(final LogicalDatastoreType type) {
         InstanceIdentifier<Topology> path = InstanceIdentifier
                 .create(NetworkTopology.class)
                 .child(Topology.class, new TopologyKey(HwvtepSouthboundConstants.HWVTEP_TOPOLOGY_ID));
@@ -169,7 +169,7 @@ public class HwvtepSouthboundProvider implements ClusteredDataTreeChangeListener
         }
     }
 
-    public void handleOwnershipChange(EntityOwnershipChange ownershipChange) {
+    public void handleOwnershipChange(final EntityOwnershipChange ownershipChange) {
         if (ownershipChange.getState().isOwner()) {
             LOG.info("*This* instance of HWVTEP southbound provider is set as a MASTER instance");
             LOG.info("Initialize HWVTEP topology {} in operational and config data store if not already present",
@@ -183,27 +183,25 @@ public class HwvtepSouthboundProvider implements ClusteredDataTreeChangeListener
 
 
     @Override
-    public void onDataTreeChanged(Collection<DataTreeModification<Topology>> collection) {
+    public void onDataTreeChanged(final Collection<DataTreeModification<Topology>> collection) {
         if (!registered.getAndSet(true)) {
             LOG.info("Starting the ovsdb port");
             ovsdbConnection.registerConnectionListener(cm);
             ovsdbConnection.startOvsdbManager();
         }
-        //mdsal registration/deregistration in mdsal update callback should be avoided
-        new Thread(() -> {
-            if (operTopologyRegistration != null) {
-                operTopologyRegistration.close();
-                operTopologyRegistration = null;
-            }
-        }).start();
+
+        if (operTopologyRegistration != null) {
+            operTopologyRegistration.close();
+            operTopologyRegistration = null;
+        }
     }
 
     private static class HwvtepsbPluginInstanceEntityOwnershipListener implements EntityOwnershipListener {
         private final HwvtepSouthboundProvider hsp;
         private final EntityOwnershipListenerRegistration listenerRegistration;
 
-        HwvtepsbPluginInstanceEntityOwnershipListener(HwvtepSouthboundProvider hsp,
-                EntityOwnershipService entityOwnershipService) {
+        HwvtepsbPluginInstanceEntityOwnershipListener(final HwvtepSouthboundProvider hsp,
+                final EntityOwnershipService entityOwnershipService) {
             this.hsp = hsp;
             listenerRegistration = entityOwnershipService.registerListener(ENTITY_TYPE, this);
         }
@@ -213,7 +211,7 @@ public class HwvtepSouthboundProvider implements ClusteredDataTreeChangeListener
         }
 
         @Override
-        public void ownershipChanged(EntityOwnershipChange ownershipChange) {
+        public void ownershipChanged(final EntityOwnershipChange ownershipChange) {
             hsp.handleOwnershipChange(ownershipChange);
         }
     }

diff --git a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundProvider.java b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundProvider.java
index 3cc833b08f82fd4802c116e8a5f80509d194f6e2..066b1870eaa4b35418f18dfe7372fae57e159122 100644 (file)
--- a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundProvider.java
+++ b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundProvider.java
@@ -7,6 +7,7 @@
  */
 package org.opendaylight.ovsdb.southbound;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
 import com.google.common.util.concurrent.CheckedFuture;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -89,7 +90,7 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
                               @Reference final BindingNormalizedNodeSerializer bindingNormalizedNodeSerializer,
                               @Reference final SystemReadyMonitor systemReadyMonitor,
                               @Reference final DiagStatusService diagStatusService) {
-        this.db = dataBroker;
+        SouthboundProvider.db = dataBroker;
         this.entityOwnershipService = entityOwnershipServiceDependency;
         registration = null;
         this.ovsdbConnection = ovsdbConnection;
@@ -155,7 +156,7 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
         ovsdbStatusProvider.reportStatus(ServiceState.UNREGISTERED, "OVSDB Service stopped");
     }
 
-    private void initializeOvsdbTopology(LogicalDatastoreType type) {
+    private void initializeOvsdbTopology(final LogicalDatastoreType type) {
         InstanceIdentifier<Topology> path = InstanceIdentifier
                 .create(NetworkTopology.class)
                 .child(Topology.class, new TopologyKey(SouthboundConstants.OVSDB_TOPOLOGY_ID));
@@ -175,7 +176,7 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
         }
     }
 
-    public void handleOwnershipChange(EntityOwnershipChange ownershipChange) {
+    public void handleOwnershipChange(final EntityOwnershipChange ownershipChange) {
         if (ownershipChange.getState().isOwner()) {
             LOG.info("*This* instance of OVSDB southbound provider is set as a MASTER instance");
             LOG.info("Initialize OVSDB topology {} in operational and config data store if not already present",
@@ -188,7 +189,7 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
     }
 
     @Override
-    public void onDataTreeChanged(Collection<DataTreeModification<Topology>> collection) {
+    public void onDataTreeChanged(final Collection<DataTreeModification<Topology>> collection) {
         if (!registered.getAndSet(true)) {
             LOG.info("Starting the ovsdb port");
             ovsdbConnection.registerConnectionListener(cm);
@@ -197,13 +198,11 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
                 ovsdbConnection.startOvsdbManager();
                 LOG.info("Started OVSDB Manager (in system ready listener)");
             });
-            //mdsal registration/deregistration in mdsal update callback should be avoided
-            new Thread(() -> {
-                if (operTopologyRegistration != null) {
-                    operTopologyRegistration.close();
-                    operTopologyRegistration = null;
-                }
-            }).start();
+
+            if (operTopologyRegistration != null) {
+                operTopologyRegistration.close();
+                operTopologyRegistration = null;
+            }
             ovsdbStatusProvider.reportStatus(ServiceState.OPERATIONAL, "OVSDB initialization complete");
         }
     }
@@ -212,8 +211,8 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
         private final SouthboundProvider sp;
         private final EntityOwnershipListenerRegistration listenerRegistration;
 
-        SouthboundPluginInstanceEntityOwnershipListener(SouthboundProvider sp,
-                EntityOwnershipService entityOwnershipService) {
+        SouthboundPluginInstanceEntityOwnershipListener(final SouthboundProvider sp,
+                final EntityOwnershipService entityOwnershipService) {
             this.sp = sp;
             listenerRegistration = entityOwnershipService.registerListener(ENTITY_TYPE, this);
         }
@@ -223,12 +222,12 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
         }
 
         @Override
-        public void ownershipChanged(EntityOwnershipChange ownershipChange) {
+        public void ownershipChanged(final EntityOwnershipChange ownershipChange) {
             sp.handleOwnershipChange(ownershipChange);
         }
     }
 
-    public void setSkipMonitoringManagerStatus(boolean flag) {
+    public void setSkipMonitoringManagerStatus(final boolean flag) {
         LOG.debug("skipManagerStatus set to {}", flag);
         if (flag) {
             SouthboundConstants.SKIP_COLUMN_FROM_TABLE.get("Manager").add("status");
@@ -237,11 +236,11 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
         }
     }
 
-    public static void setBridgesReconciliationInclusionList(List<String> bridgeList) {
+    public static void setBridgesReconciliationInclusionList(final List<String> bridgeList) {
         reconcileBridgeInclusionList = bridgeList;
     }
 
-    public static void setBridgesReconciliationExclusionList(List<String> bridgeList) {
+    public static void setBridgesReconciliationExclusionList(final List<String> bridgeList) {
         reconcileBridgeExclusionList = bridgeList;
     }
 
@@ -252,4 +251,9 @@ public class SouthboundProvider implements ClusteredDataTreeChangeListener<Topol
     public static List<String> getBridgesReconciliationExclusionList() {
         return reconcileBridgeExclusionList;
     }
+
+    @VisibleForTesting
+    boolean isRegistered() {
+        return registered.get();
+    }
 }

diff --git a/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/SouthboundProviderTest.java b/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/SouthboundProviderTest.java
index 09ae00851b142a7e8e4434d3d2ba139a7b680632..ba5dce98292137610992047bcd135efe4309322e 100644 (file)
--- a/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/SouthboundProviderTest.java
+++ b/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/SouthboundProviderTest.java
@@ -5,24 +5,26 @@
  * terms of the Eclipse Public License v1.0 which accompanies this distribution,
  * and is available at http://www.eclipse.org/legal/epl-v10.html
  */
-
 package org.opendaylight.ovsdb.southbound;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anyString;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.opendaylight.infrautils.ready.testutils.TestSystemReadyMonitor.Behaviour.IMMEDIATE;
 
+import com.google.common.base.Stopwatch;
+import com.google.common.util.concurrent.Uninterruptibles;
+import java.util.concurrent.TimeUnit;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mockito;
-import org.opendaylight.controller.md.sal.binding.test.AbstractDataBrokerTest;
+import org.opendaylight.controller.md.sal.binding.test.AbstractConcurrentDataBrokerTest;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException;
 import org.opendaylight.infrautils.diagstatus.DiagStatusService;
@@ -45,10 +47,14 @@ import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 
-public class SouthboundProviderTest extends AbstractDataBrokerTest {
+public class SouthboundProviderTest extends AbstractConcurrentDataBrokerTest {
 
     private EntityOwnershipService entityOwnershipService;
 
+    public SouthboundProviderTest() {
+        super(true);
+    }
+
     @Before
     public void setUp() throws CandidateAlreadyRegisteredException {
         entityOwnershipService = mock(EntityOwnershipService.class);
@@ -165,6 +171,13 @@ public class SouthboundProviderTest extends AbstractDataBrokerTest {
             southboundProvider.handleOwnershipChange(new EntityOwnershipChange(entity,
                     EntityOwnershipChangeState.from(false, true, true)));
 
+            // Up to 3 seconds for DTCL to settle
+            final Stopwatch sw = Stopwatch.createStarted();
+            while (!southboundProvider.isRegistered() && sw.elapsed(TimeUnit.SECONDS) < 3) {
+                Uninterruptibles.sleepUninterruptibly(100, TimeUnit.MILLISECONDS);
+            }
+            assertTrue(southboundProvider.isRegistered());
+
             // Now the OVSDB topology must be present in both trees
             assertTrue(getDataBroker().newReadOnlyTransaction().read(LogicalDatastoreType.CONFIGURATION,
                     topologyIid).checkedGet().isPresent());