For controller initiated connection, there is race condition
[ovsdb.git] / southbound / southbound-impl / src / main / java / org / opendaylight / ovsdb / southbound / OvsdbConnectionManager.java
index 54ff1e5a21b811fb66a797c1a00f21dc90100fd5..ecfedfe48f1b787f71331f454c1dc944ad72898e 100644 (file)
@@ -29,11 +29,12 @@ import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipC
 import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipListener;
 import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipListenerRegistration;
 import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipService;
+import org.opendaylight.controller.md.sal.common.api.clustering.EntityOwnershipState;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException;
 import org.opendaylight.ovsdb.lib.OvsdbClient;
+import org.opendaylight.ovsdb.lib.OvsdbConnection;
 import org.opendaylight.ovsdb.lib.OvsdbConnectionListener;
-import org.opendaylight.ovsdb.lib.impl.OvsdbConnectionService;
 import org.opendaylight.ovsdb.lib.operations.Operation;
 import org.opendaylight.ovsdb.lib.operations.OperationResult;
 import org.opendaylight.ovsdb.lib.operations.Select;
@@ -59,25 +60,28 @@ import com.google.common.util.concurrent.CheckedFuture;
 
 public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoCloseable {
     private Map<ConnectionInfo, OvsdbConnectionInstance> clients =
-            new ConcurrentHashMap<ConnectionInfo,OvsdbConnectionInstance>();
+            new ConcurrentHashMap<>();
     private static final Logger LOG = LoggerFactory.getLogger(OvsdbConnectionManager.class);
     private static final String ENTITY_TYPE = "ovsdb";
 
     private DataBroker db;
     private TransactionInvoker txInvoker;
     private Map<ConnectionInfo,InstanceIdentifier<Node>> instanceIdentifiers =
-            new ConcurrentHashMap<ConnectionInfo,InstanceIdentifier<Node>>();
+            new ConcurrentHashMap<>();
     private Map<Entity, OvsdbConnectionInstance> entityConnectionMap =
             new ConcurrentHashMap<>();
     private EntityOwnershipService entityOwnershipService;
     private OvsdbDeviceEntityOwnershipListener ovsdbDeviceEntityOwnershipListener;
+    private OvsdbConnection ovsdbConnection;
 
     public OvsdbConnectionManager(DataBroker db,TransactionInvoker txInvoker,
-                                  EntityOwnershipService entityOwnershipService) {
+                                  EntityOwnershipService entityOwnershipService,
+                                  OvsdbConnection ovsdbConnection) {
         this.db = db;
         this.txInvoker = txInvoker;
         this.entityOwnershipService = entityOwnershipService;
         this.ovsdbDeviceEntityOwnershipListener = new OvsdbDeviceEntityOwnershipListener(this, entityOwnershipService);
+        this.ovsdbConnection = ovsdbConnection;
     }
 
     @Override
@@ -103,12 +107,13 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
                 return ovsdbConnectionInstance;
             }
             LOG.warn("OVSDB Connection Instance {} being replaced with client {}", key, externalClient);
-            ovsdbConnectionInstance.disconnect();
 
             // Unregister Cluster Ownership for ConnectionInfo
             // Because the ovsdbConnectionInstance is about to be completely replaced!
             unregisterEntityForOwnership(ovsdbConnectionInstance);
 
+            ovsdbConnectionInstance.disconnect();
+
             removeConnectionInstance(key);
         }
 
@@ -126,13 +131,17 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         ConnectionInfo key = SouthboundMapper.createConnectionInfo(client);
         OvsdbConnectionInstance ovsdbConnectionInstance = getConnectionInstance(key);
         if (ovsdbConnectionInstance != null) {
+            // Unregister Entity ownership as soon as possible ,so this instance should
+            // not be used as a candidate in Entity election (given that this instance is
+            // about to disconnect as well), if current owner get disconnected from
+            // OVSDB device.
+            unregisterEntityForOwnership(ovsdbConnectionInstance);
+
             txInvoker.invoke(new OvsdbNodeRemoveCommand(ovsdbConnectionInstance, null, null));
-            removeConnectionInstance(key);
 
-            // Unregister Cluster Onwership for ConnectionInfo
-            unregisterEntityForOwnership(ovsdbConnectionInstance);
+            removeConnectionInstance(key);
         } else {
-            LOG.warn("OVSDB disconnected event did not find connection instance for {}", key);
+            LOG.warn("disconnected : Connection instance not found for OVSDB Node {} ", key);
         }
         LOG.trace("OvsdbConnectionManager: disconnected exit");
     }
@@ -143,18 +152,19 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         // TODO use transaction chains to handle ordering issues between disconnected
         // TODO and connected when writing to the operational store
         InetAddress ip = SouthboundMapper.createInetAddress(ovsdbNode.getConnectionInfo().getRemoteIp());
-        OvsdbClient client = OvsdbConnectionService.getService().connect(ip,
+        OvsdbClient client = ovsdbConnection.connect(ip,
                 ovsdbNode.getConnectionInfo().getRemotePort().getValue());
         // For connections from the controller to the ovs instance, the library doesn't call
         // this method for us
         if (client != null) {
             putInstanceIdentifier(ovsdbNode.getConnectionInfo(), iid.firstIdentifierOf(Node.class));
             OvsdbConnectionInstance ovsdbConnectionInstance = connectedButCallBacksNotRegistered(client);
+            ovsdbConnectionInstance.setOvsdbNodeAugmentation(ovsdbNode);
 
             // Register Cluster Ownership for ConnectionInfo
             registerEntityForOwnership(ovsdbConnectionInstance);
         } else {
-            LOG.warn("Failed to connect to Ovsdb Node {}", ovsdbNode.getConnectionInfo());
+            LOG.warn("Failed to connect to OVSDB Node {}", ovsdbNode.getConnectionInfo());
         }
         return client;
     }
@@ -162,12 +172,14 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
     public void disconnect(OvsdbNodeAugmentation ovsdbNode) throws UnknownHostException {
         OvsdbConnectionInstance client = getConnectionInstance(ovsdbNode.getConnectionInfo());
         if (client != null) {
-            client.disconnect();
-
             // Unregister Cluster Onwership for ConnectionInfo
             unregisterEntityForOwnership(client);
 
+            client.disconnect();
+
             removeInstanceIdentifier(ovsdbNode.getConnectionInfo());
+        } else {
+            LOG.debug("disconnect : connection instance not found for {}",ovsdbNode.getConnectionInfo());
         }
     }
 
@@ -223,8 +235,7 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
 
     public InstanceIdentifier<Node> getInstanceIdentifier(ConnectionInfo key) {
         ConnectionInfo connectionInfo = SouthboundMapper.suppressLocalIpPort(key);
-        InstanceIdentifier<Node> iid = instanceIdentifiers.get(connectionInfo);
-        return iid;
+        return instanceIdentifiers.get(connectionInfo);
     }
 
     public OvsdbConnectionInstance getConnectionInstance(OvsdbBridgeAttributes mn) {
@@ -298,33 +309,27 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
 
     private void handleOwnershipChanged(EntityOwnershipChange ownershipChange) {
         OvsdbConnectionInstance ovsdbConnectionInstance = getConnectionInstanceFromEntity(ownershipChange.getEntity());
-        LOG.info("handleOwnershipChanged: {} event received for device {}",
+        LOG.debug("handleOwnershipChanged: {} event received for device {}",
                 ownershipChange, ovsdbConnectionInstance != null ? ovsdbConnectionInstance.getConnectionInfo()
-                        : "THAT'S NOT REGISTERED BY THIS SOUTHBOUND PLUGIN INSTANCE");
+                        : "that's currently NOT registered by *this* southbound plugin instance");
 
         if (ovsdbConnectionInstance == null) {
             if (ownershipChange.isOwner()) {
-                LOG.warn("handleOwnershipChanged: found no connection instance for {}", ownershipChange.getEntity());
+                LOG.warn("handleOwnershipChanged: *this* instance is elected as an owner of the device {} but it "
+                        + "is NOT registered for ownership", ownershipChange.getEntity());
             } else {
                 // EntityOwnershipService sends notification to all the nodes, irrespective of whether
                 // that instance registered for the device ownership or not. It is to make sure that
                 // If all the controller instance that was connected to the device are down, so the
                 // running instance can clear up the operational data store even though it was not
                 // connected to the device.
-                LOG.debug("handleOwnershipChanged: found no connection instance for {}", ownershipChange.getEntity());
+                LOG.debug("handleOwnershipChanged: No connection instance found for {}", ownershipChange.getEntity());
             }
 
             // If entity has no owner, clean up the operational data store (it's possible because owner controller
             // might went down abruptly and didn't get a chance to clean up the operational data store.
             if (!ownershipChange.hasOwner()) {
-                LOG.debug("{} has no onwer, cleaning up the operational data store", ownershipChange.getEntity());
-                // Below code might look weird but it's required. We want to give first opportunity to the
-                // previous owner of the device to clean up the operational data store if there is no owner now.
-                // That way we will avoid lot of nasty md-sal exceptions because of concurrent delete.
-                if (ownershipChange.wasOwner()) {
-                    cleanEntityOperationalData(ownershipChange.getEntity());
-                }
-                // If first cleanEntityOperationalData() was called, this call will be no-op.
+                LOG.info("{} has no owner, cleaning up the operational data store", ownershipChange.getEntity());
                 cleanEntityOperationalData(ownershipChange.getEntity());
             }
             return;
@@ -333,15 +338,17 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         putConnectionInstance(ovsdbConnectionInstance.getMDConnectionInfo(),ovsdbConnectionInstance);
 
         if (ownershipChange.isOwner() == ovsdbConnectionInstance.getHasDeviceOwnership()) {
-            LOG.debug("handleOwnershipChanged: no change in ownership for {}. Ownership status is : {}",
-                    ovsdbConnectionInstance.getConnectionInfo(), ovsdbConnectionInstance.getHasDeviceOwnership());
+            LOG.info("handleOwnershipChanged: no change in ownership for {}. Ownership status is : {}",
+                    ovsdbConnectionInstance.getConnectionInfo(), ovsdbConnectionInstance.getHasDeviceOwnership()
+                            ? SouthboundConstants.OWNERSHIPSTATES.OWNER.getState()
+                            : SouthboundConstants.OWNERSHIPSTATES.NONOWNER.getState());
             return;
         }
 
         ovsdbConnectionInstance.setHasDeviceOwnership(ownershipChange.isOwner());
         // You were not an owner, but now you are
         if (ownershipChange.isOwner()) {
-            LOG.info("handleOwnershipChanged: *this* southbound plugin instance is owner of device {}",
+            LOG.info("handleOwnershipChanged: *this* southbound plugin instance is an OWNER of the device {}",
                     ovsdbConnectionInstance.getConnectionInfo());
 
             //*this* instance of southbound plugin is owner of the device,
@@ -356,7 +363,8 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             //when clustering service implement a ownership grant strategy which can revoke the
             //device ownership for load balancing the devices across the instances.
             //Once this condition occur, we should unregister the callback.
-            LOG.error("handleOwnershipChanged: *this* southbound plugin instance is no longer the owner of device {}",
+            LOG.error("handleOwnershipChanged: *this* southbound plugin instance is no longer the owner of device {}."
+                    + "This should NOT happen.",
                     ovsdbConnectionInstance.getNodeId().getValue());
         }
     }
@@ -385,17 +393,16 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         if (dbSchema != null) {
             GenericTableSchema openVSwitchSchema = TyperUtils.getTableSchema(dbSchema, OpenVSwitch.class);
 
-            List<String> openVSwitchTableColumn = new ArrayList<String>();
+            List<String> openVSwitchTableColumn = new ArrayList<>();
             openVSwitchTableColumn.addAll(openVSwitchSchema.getColumns());
             Select<GenericTableSchema> selectOperation = op.select(openVSwitchSchema);
-            selectOperation.setColumns(openVSwitchTableColumn);;
+            selectOperation.setColumns(openVSwitchTableColumn);
 
-            ArrayList<Operation> operations = new ArrayList<Operation>();
+            List<Operation> operations = new ArrayList<>();
             operations.add(selectOperation);
             operations.add(op.comment("Fetching Open_VSwitch table rows"));
-            List<OperationResult> results = null;
             try {
-                results = connectionInstance.transact(dbSchema, operations).get();
+                List<OperationResult> results = connectionInstance.transact(dbSchema, operations).get();
                 if (results != null ) {
                     OperationResult selectResult = results.get(0);
                     openVSwitchRow = TyperUtils.getTypedRowWrapper(
@@ -410,8 +417,7 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         return openVSwitchRow;
     }
     private Entity getEntityFromConnectionInstance(@Nonnull OvsdbConnectionInstance ovsdbConnectionInstance) {
-        YangInstanceIdentifier entityId = null;
-        InstanceIdentifier<Node> iid = ovsdbConnectionInstance.getInstanceIdentifier();;
+        InstanceIdentifier<Node> iid = ovsdbConnectionInstance.getInstanceIdentifier();
         if ( iid == null ) {
             /* Switch initiated connection won't have iid, till it gets OpenVSwitch
              * table update but update callback is always registered after ownership
@@ -421,9 +427,9 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             iid = SouthboundMapper.getInstanceIdentifier(openvswitchRow);
             LOG.info("InstanceIdentifier {} generated for device "
                     + "connection {}",iid,ovsdbConnectionInstance.getConnectionInfo());
-
+            ovsdbConnectionInstance.setInstanceIdentifier(iid);
         }
-        entityId = SouthboundUtil.getInstanceIdentifierCodec().getYangInstanceIdentifier(iid);
+        YangInstanceIdentifier entityId = SouthboundUtil.getInstanceIdentifierCodec().getYangInstanceIdentifier(iid);
         Entity deviceEntity = new Entity(ENTITY_TYPE, entityId);
         LOG.debug("Entity {} created for device connection {}",
                 deviceEntity, ovsdbConnectionInstance.getConnectionInfo());
@@ -443,9 +449,23 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             EntityOwnershipCandidateRegistration registration =
                     entityOwnershipService.registerCandidate(candidateEntity);
             ovsdbConnectionInstance.setDeviceOwnershipCandidateRegistration(registration);
-            LOG.info("OVSDB entity {} is registred for ownership.", candidateEntity);
+            LOG.info("OVSDB entity {} is registered for ownership.", candidateEntity);
+
+            //If entity already has owner, it won't get notification from EntityOwnershipService
+            //so cache the connection instances.
+            Optional<EntityOwnershipState> ownershipStateOpt =
+                    entityOwnershipService.getOwnershipState(candidateEntity);
+            if (ownershipStateOpt.isPresent()) {
+                EntityOwnershipState ownershipState = ownershipStateOpt.get();
+                if (ownershipState.hasOwner() && !ownershipState.isOwner()) {
+                    LOG.info("OVSDB entity {} is already owned by other southbound plugin "
+                                    + "instance, so *this* instance is NOT an OWNER of the device",
+                            ovsdbConnectionInstance.getConnectionInfo());
+                    putConnectionInstance(ovsdbConnectionInstance.getMDConnectionInfo(),ovsdbConnectionInstance);
+                }
+            }
         } catch (CandidateAlreadyRegisteredException e) {
-            LOG.warn("OVSDB entity {} was already registered for {} ownership", candidateEntity, e);
+            LOG.warn("OVSDB entity {} was already registered for ownership", candidateEntity, e);
         }
 
     }