For controller initiated connection, there is race condition

while disconnecting the device from controller, that does
not allow one of the non-owner southbound instance to disconnect
the device from the plugin. Race condition was occuring because
OvsdbConnectionInstance was not stored in the internal cache
at correct point in control flow.It gets added when EntityOwnershipChange
event happens, so when data change event for disconnection occured,
it requires OvsdbConnectionInstance to be in cache to disconnect
from the device, but OvsdbConnectionInstance was not in the cache
becuse EntityOwnershipService event was not recieved by that non-owner instance.

Conflicts:
	southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManager.java
	southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundProvider.java

Change-Id: I3861b5f2907d08709a3bd1aa4f4ecfbfd444351d
Signed-off-by: Anil Vishnoi <vishnoianil@gmail.com>

diff --git a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManager.java b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManager.java
index b1925261035d2877de9ba8ac930939b1cae42ee3..ecfedfe48f1b787f71331f454c1dc944ad72898e 100644 (file)
--- a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManager.java
+++ b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManager.java
@@ -107,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);
         }
 
@@ -130,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");
     }
@@ -159,7 +164,7 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             // 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;
     }
@@ -167,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());
         }
     }
 
@@ -302,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;
@@ -337,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,
@@ -360,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());
         }
     }
@@ -423,7 +427,7 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             iid = SouthboundMapper.getInstanceIdentifier(openvswitchRow);
             LOG.info("InstanceIdentifier {} generated for device "
                     + "connection {}",iid,ovsdbConnectionInstance.getConnectionInfo());
-
+            ovsdbConnectionInstance.setInstanceIdentifier(iid);
         }
         YangInstanceIdentifier entityId = SouthboundUtil.getInstanceIdentifierCodec().getYangInstanceIdentifier(iid);
         Entity deviceEntity = new Entity(ENTITY_TYPE, entityId);
@@ -445,7 +449,7 @@ 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.
@@ -454,16 +458,14 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             if (ownershipStateOpt.isPresent()) {
                 EntityOwnershipState ownershipState = ownershipStateOpt.get();
                 if (ownershipState.hasOwner() && !ownershipState.isOwner()) {
-                    if (getConnectionInstance(ovsdbConnectionInstance.getMDConnectionInfo()) != null) {
-                        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);
-                    }
+                    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);
         }
 
     }

diff --git a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundConstants.java b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundConstants.java
index 42c452ca5c70efa068e3a44c2170c6458105bab3..854d5b2febddd1fb03a980c7606d19bf793048e7 100755 (executable)
--- a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundConstants.java
+++ b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/SouthboundConstants.java
@@ -114,4 +114,23 @@ public class SouthboundConstants {
             return this.mode;
         }
     }
+
+    public enum OWNERSHIPSTATES {
+        OWNER("OWNER"),
+        NONOWNER("NON-OWNER");
+
+        private final String state;
+
+        OWNERSHIPSTATES(String state) {
+            this.state = state;
+        }
+        @Override
+        public String toString() {
+            return state;
+        }
+
+        public String getState() {
+            return this.state;
+        }
+    }
 }

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 189f5030eba576667d52f42f64af1254e94e36ac..d88074b9b3f0ee3228de54d6e49d2a6747a512eb 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
@@ -87,6 +87,7 @@ public class SouthboundProvider implements BindingAwareProvider, AutoCloseable {
                 if (ownershipState.hasOwner() && !ownershipState.isOwner()) {
                     ovsdbConnection.registerConnectionListener(cm);
                     ovsdbConnection.startOvsdbManager(SouthboundConstants.DEFAULT_OVSDB_PORT);
+                    LOG.info("*This* instance of OVSDB southbound provider is set as a SLAVE instance");
                 }
             }
         } catch (CandidateAlreadyRegisteredException e) {