From: Flavio Fernandes Date: Sat, 2 Aug 2014 04:20:58 +0000 (-0400) Subject: Bug 960 : Avoid adding redundant Openflow Controller entries X-Git-Tag: release/helium~45 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=1f0ec4191953f7852894e2313d21d3317741db9f;p=ovsdb.git 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 --- 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 2557be552..38f7d6a91 100644 --- 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 nodes = connectionService.getNodes(); + for (Node node : nodes) { + Map 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 de1463805..7e5273bfb 100644 --- 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 getCurrentControllerTargets(Node node, final String controllerTableName) { + Set currentControllerTargets = new HashSet<>(); + ConcurrentMap rows = this.getRows(node, controllerTableName); + + if (rows != null) { + for (Map.Entry entry : rows.entrySet()) { + Controller currController = this.getTypedRow(node, Controller.class, entry.getValue()); + Column 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 currControllerTargets = null; List 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(cache.keySet()); } -} \ No newline at end of file +}