Bug 960 : Avoid adding redundant Openflow Controller entries

* Added IT for reproducing the problem;
* Enhanced ConfigurationService.setOFController() to detect duplicate
  targets and update rows instead of adding new ones.

Log: https://gist.github.com/anonymous/4ce9b2abdecda3c5e7fa

Change-Id: Ifedc351e849a18feb66b6025189495cb991c38f2
Signed-off-by: Flavio Fernandes <ffernand@redhat.com>

diff --git a/integrationtest/src/test/java/org/opendaylight/ovsdb/integrationtest/plugin/OvsdbPluginIT.java b/integrationtest/src/test/java/org/opendaylight/ovsdb/integrationtest/plugin/OvsdbPluginIT.java
index 2557be552effda0eae2e0ca299080df62275907b..38f7d6a918938827c547153f9e0799f89aed98a5 100644 (file)
--- a/integrationtest/src/test/java/org/opendaylight/ovsdb/integrationtest/plugin/OvsdbPluginIT.java
+++ b/integrationtest/src/test/java/org/opendaylight/ovsdb/integrationtest/plugin/OvsdbPluginIT.java
@@ -20,6 +20,7 @@ import static org.ops4j.pax.exam.CoreOptions.options;
 import static org.ops4j.pax.exam.CoreOptions.propagateSystemProperty;
 import static org.ops4j.pax.exam.CoreOptions.systemProperty;
 
+import org.junit.After;
 import org.opendaylight.controller.sal.core.Node;
 import org.opendaylight.controller.sal.utils.ServiceHelper;
 import org.opendaylight.controller.sal.utils.Status;
@@ -57,6 +58,7 @@ import org.slf4j.LoggerFactory;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.ConcurrentMap;
 
 import javax.inject.Inject;
@@ -206,6 +208,85 @@ public class OvsdbPluginIT extends OvsdbIntegrationTestBase {
 
     }
 
+    @Test
+    public void testSetOFControllers() throws Exception {
+        Thread.sleep(5000);
+        OvsdbConnectionService
+                connectionService = (OvsdbConnectionService)ServiceHelper.getGlobalInstance(OvsdbConnectionService.class, this);
+
+        // 1. Check for the ovsdb Connection as seen by the Plugin layer
+        assertNotNull(connectionService.getNodes());
+        assertTrue(connectionService.getNodes().size() > 0);
+        Node node = connectionService.getNodes().get(0);
+        Connection connection = connectionService.getConnection(node);
+        assertNotNull(connection);
+
+        // 2. Create a bridge with a valid parent_uuid & Assert to make sure the return status is success.
+        final StatusWithUuid status = insertBridge(connection, getOpenVSwitchTableUUID(connection));
+        assertTrue(status.isSuccess());
+
+        // Thread.sleep(3000);  // wait for _real_ controller to be added to bridge... or not (see below **)
+
+        // 3. Test against bug 960: Add same controller multiple times and make sure we do not end up with duplicates.
+        ovsdbConfigurationService.setOFController(node, status.getUuid().toString());
+        ovsdbConfigurationService.setOFController(node, status.getUuid().toString());
+        ovsdbConfigurationService.setOFController(node, status.getUuid().toString());
+        ovsdbConfigurationService.setOFController(node, status.getUuid().toString());
+
+        Row bridgeRow = ovsdbConfigurationService.getRow(node,
+                                                  ovsdbConfigurationService.getTableName(node, Bridge.class),
+                                                  status.getUuid().toString());
+        assertNotNull(bridgeRow);
+        Bridge bridge = ovsdbConfigurationService.getTypedRow(node, Bridge.class, bridgeRow);
+        assertTrue(bridge.getUuid().equals(status.getUuid()));
+
+        final int currControllersSize = bridge.getControllerColumn().getData().size();
+
+        log.debug("Bridge has " + bridge.getControllerColumn().getData().size() + " controllers");
+
+        // ** Note: we assert against 2 or less -- instead of 1 -- to account for the _real_ controller's connection
+        assertTrue( "Too few controllers added to bridge object. Is this bug 960?", currControllersSize >= 1 );
+        assertTrue( "Too many controllers added to bridge object. Is this bug 960?", currControllersSize <= 2 );
+
+        // Removal of bridge created in this test is done via tearDown(). It is done that way, so cleanup is ran
+        // even if test fails.
+    }
+
+    @After
+    public void tearDown() throws InterruptedException {
+        Thread.sleep(5000);
+        OvsdbConnectionService
+                connectionService = (OvsdbConnectionService)ServiceHelper.getGlobalInstance(OvsdbConnectionService.class, this);
+
+        if (connectionService.getNodes() == null) {
+            return;  // no nodes: noop
+        }
+
+        int bridgesRemoved = 0;
+        List<Node> nodes = connectionService.getNodes();
+        for (Node node : nodes) {
+            Map<String, Row> bridgeRows =
+                    ovsdbConfigurationService.getRows(node, ovsdbConfigurationService.getTableName(node, Bridge.class));
+            if (bridgeRows == null) {
+                continue;
+            }
+            for (Row bridgeRow : bridgeRows.values()) {
+                Bridge bridge = ovsdbConfigurationService.getTypedRow(node, Bridge.class, bridgeRow);
+                log.trace("Test clean up removing Bridge " + bridge.getUuid());
+                Status delStatus = ovsdbConfigurationService.deleteRow(node,
+                                                                bridge.getSchema().getName(),
+                                                                bridge.getUuid().toString());
+                assertTrue(delStatus.isSuccess());
+                bridgesRemoved++;
+            }
+        }
+
+        if (bridgesRemoved > 0) {
+            log.debug("Test clean up removed " + bridgesRemoved + " bridges");
+            Thread.sleep(2000); // TODO : Remove this Sleep once the Select operation is resolved.
+        }
+    }
+
     public void endToEndApiTest(Connection connection, String parentUuid) throws Exception {
         // 1. Print Cache and Assert to make sure the bridge is not created yet.
         printCache();

diff --git a/plugin/src/main/java/org/opendaylight/ovsdb/plugin/impl/ConfigurationServiceImpl.java b/plugin/src/main/java/org/opendaylight/ovsdb/plugin/impl/ConfigurationServiceImpl.java
index de1463805419619860050bbc3faabec5d6162f97..7e5273bfb69e6ef373eaefc53e40ea2b6a2bcbf9 100644 (file)
--- a/plugin/src/main/java/org/opendaylight/ovsdb/plugin/impl/ConfigurationServiceImpl.java
+++ b/plugin/src/main/java/org/opendaylight/ovsdb/plugin/impl/ConfigurationServiceImpl.java
@@ -444,6 +444,23 @@ public class ConfigurationServiceImpl implements IPluginInBridgeDomainConfigServ
         return openFlowPort;
     }
 
+    private Set<String> getCurrentControllerTargets(Node node, final String controllerTableName) {
+        Set<String> currentControllerTargets = new HashSet<>();
+        ConcurrentMap<String, Row> rows = this.getRows(node, controllerTableName);
+
+        if (rows != null) {
+            for (Map.Entry<String, Row> entry : rows.entrySet()) {
+                Controller currController = this.getTypedRow(node, Controller.class, entry.getValue());
+                Column<GenericTableSchema, String> column = currController.getTargetColumn();
+                String currTarget = column.getData();
+                if (currTarget == null) continue;
+                currentControllerTargets.add(currTarget);
+            }
+        }
+
+        return currentControllerTargets;
+    }
+
     @Override
     public Boolean setOFController(Node node, String bridgeUUID) throws InterruptedException, ExecutionException {
         Connection connection = this.getConnection(node);
@@ -484,14 +501,28 @@ public class ConfigurationServiceImpl implements IPluginInBridgeDomainConfigServ
         }
 
         Status status = null;
+        Set<String> currControllerTargets = null;
         List<InetAddress> ofControllerAddrs = this.getControllerIPAddresses(connection);
         short ofControllerPort = getControllerOFPort();
         for (InetAddress ofControllerAddress : ofControllerAddrs) {
-            String newController = "tcp:"+ofControllerAddress.getHostAddress()+":"+ofControllerPort;
-            Controller controllerRow = connection.getClient().createTypedRowWrapper(Controller.class);
-            controllerRow.setTarget(newController);
-            //ToDo: Status gets overwritten on each iteration. If any operation other than the last fails it's ignored.
-            status = this.insertRow(node, controllerRow.getSchema().getName(), bridgeUUID, controllerRow.getRow());
+            String newControllerTarget = "tcp:"+ofControllerAddress.getHostAddress()+":"+ofControllerPort;
+            Controller newController = connection.getClient().createTypedRowWrapper(Controller.class);
+            newController.setTarget(newControllerTarget);
+            final String controllerTableName = newController.getSchema().getName();
+
+            // get current controller targets, iff this is the first iteration
+            if (currControllerTargets == null) {
+                currControllerTargets = getCurrentControllerTargets(node, controllerTableName);
+            }
+
+            if (currControllerTargets.contains(newControllerTarget)) {
+                //ToDo: Status gets overwritten on each iteration. If any operation other than the last fails it's ignored.
+                status = this.updateRow(node, controllerTableName, null, bridgeUUID, newController.getRow());
+            } else {
+                //ToDo: Status gets overwritten on each iteration. If any operation other than the last fails it's ignored.
+                status = this.insertRow(node, controllerTableName, bridgeUUID, newController.getRow());
+                if (status.isSuccess()) currControllerTargets.add(newControllerTarget);
+            }
         }
 
         if (status != null) {
@@ -1586,4 +1617,4 @@ public class ConfigurationServiceImpl implements IPluginInBridgeDomainConfigServ
         if (cache == null) return null;
         return new ArrayList<String>(cache.keySet());
     }
-}
\ No newline at end of file
+}