OVSDB-462 Bridge randomly missing in operds 48/73348/4
authorK.V Suneelu Verma <k.v.suneelu.verma@ericsson.com>
Fri, 22 Jun 2018 11:43:02 +0000 (17:13 +0530)
committerK.V Suneelu Verma <k.v.suneelu.verma@ericsson.com>
Thu, 5 Jul 2018 05:58:39 +0000 (11:28 +0530)
After registering for eos check if eos state is set
if eos state is set go ahead and register for callbacks which evenutually
puts the bridge in operational datastore.

Added error log statement for transaction chain failures

Addressed connection flap race conditions.
If the bridge disconnects from one odl and connects to other odl.
Before disowning ownership, delete the bridge from oper store.
The second odl after getting the ownership will put it back in oper store.

Change-Id: I0fafa1c1d3a73523bc102e2d720fdcf65dba1e3f
Signed-off-by: K.V Suneelu Verma <k.v.suneelu.verma@ericsson.com>
12 files changed:
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionInstance.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManager.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/ovsdb/transact/BridgeUpdateCommand.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/ovsdb/transact/TransactInvokerImpl.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/AbstractTransactionCommand.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/OvsdbBridgeUpdateCommand.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/OvsdbNodeRemoveCommand.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/OvsdbOperationalCommandAggregator.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionCommand.java
southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImpl.java
southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/OvsdbConnectionManagerTest.java
southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/transactions/md/OvsdbBridgeUpdateCommandTest.java

index ad86945f8e52a7a708ce5bf600989df31ec386cb..35047115ae22972c713b38bf9e47829f99d63615 100644 (file)
@@ -368,6 +368,8 @@ public class OvsdbConnectionInstance {
 
     public void setHasDeviceOwnership(Boolean hasDeviceOwnership) {
         if (hasDeviceOwnership != null) {
+            LOG.debug("Ownership status for {} old {} new {}",
+                    instanceIdentifier, this.hasDeviceOwnership, hasDeviceOwnership);
             this.hasDeviceOwnership = hasDeviceOwnership;
         }
     }
index ac2fee0212a531084d225a0b4a35be06825de5b6..3362e239090c937a5a48d7744e16b34967d9ff35 100644 (file)
@@ -75,6 +75,8 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
     private final TransactionInvoker txInvoker;
     private final Map<ConnectionInfo,InstanceIdentifier<Node>> instanceIdentifiers =
             new ConcurrentHashMap<>();
+    private final Map<InstanceIdentifier<Node>, OvsdbConnectionInstance> nodeIdVsConnectionInstance =
+            new ConcurrentHashMap<>();
     private final Map<Entity, OvsdbConnectionInstance> entityConnectionMap =
             new ConcurrentHashMap<>();
     private final EntityOwnershipService entityOwnershipService;
@@ -166,15 +168,20 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             // 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));
-
+            if (ovsdbConnectionInstance.getHasDeviceOwnership()) {
+                LOG.info("Library disconnected {} this controller instance has ownership", key);
+                deleteOperNodeAndReleaseOwnership(ovsdbConnectionInstance);
+            } else {
+                LOG.info("Library disconnected {} this controller does not have ownership", key);
+                unregisterEntityForOwnership(ovsdbConnectionInstance);
+            }
             removeConnectionInstance(key);
 
             //Controller initiated connection can be terminated from switch side.
             //So cleanup the instance identifier cache.
             removeInstanceIdentifier(key);
+            nodeIdVsConnectionInstance.remove(ovsdbConnectionInstance.getInstanceIdentifier(),
+                    ovsdbConnectionInstance);
             stopBridgeConfigReconciliationIfActive(ovsdbConnectionInstance.getInstanceIdentifier());
             retryConnection(ovsdbConnectionInstance.getInstanceIdentifier(),
                     ovsdbConnectionInstance.getOvsdbNodeAugmentation(),
@@ -185,6 +192,29 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         LOG.trace("OvsdbConnectionManager: exit disconnected client: {}", client);
     }
 
+    private void deleteOperNodeAndReleaseOwnership(final OvsdbConnectionInstance ovsdbConnectionInstance) {
+        ovsdbConnectionInstance.setHasDeviceOwnership(false);
+        final InstanceIdentifier nodeIid = ovsdbConnectionInstance.getInstanceIdentifier();
+        //remove the node from oper only if it has ownership
+        txInvoker.invoke(new OvsdbNodeRemoveCommand(ovsdbConnectionInstance, null, null) {
+
+            @Override
+            public void onSuccess() {
+                super.onSuccess();
+                LOG.debug("Successfully removed node {} from oper", nodeIid);
+                //Giveup the ownership only after cleanup is done
+                unregisterEntityForOwnership(ovsdbConnectionInstance);
+            }
+
+            @Override
+            public void onFailure(Throwable throwable) {
+                LOG.debug("Failed to remove node {} from oper", nodeIid);
+                super.onFailure(throwable);
+                unregisterEntityForOwnership(ovsdbConnectionInstance);
+            }
+        });
+    }
+
     public OvsdbClient connect(InstanceIdentifier<Node> iid,
             OvsdbNodeAugmentation ovsdbNode) throws UnknownHostException, ConnectException {
         LOG.info("Connecting to {}", SouthboundUtil.connectionInfoToString(ovsdbNode.getConnectionInfo()));
@@ -215,7 +245,7 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
         OvsdbConnectionInstance client = getConnectionInstance(ovsdbNode.getConnectionInfo());
         if (client != null) {
             // Unregister Cluster Onwership for ConnectionInfo
-            unregisterEntityForOwnership(client);
+            deleteOperNodeAndReleaseOwnership(client);
 
             client.disconnect();
 
@@ -306,6 +336,9 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
     }
 
     public OvsdbConnectionInstance getConnectionInstance(InstanceIdentifier<Node> nodePath) {
+        if (nodeIdVsConnectionInstance.get(nodePath) != null) {
+            return nodeIdVsConnectionInstance.get(nodePath);
+        }
         try {
             ReadOnlyTransaction transaction = db.newReadOnlyTransaction();
             CheckedFuture<Optional<Node>, ReadFailedException> nodeFuture = transaction.read(
@@ -523,8 +556,16 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
     }
 
     private void registerEntityForOwnership(OvsdbConnectionInstance ovsdbConnectionInstance) {
+        putConnectionInstance(ovsdbConnectionInstance.getMDConnectionInfo(), ovsdbConnectionInstance);
 
         Entity candidateEntity = getEntityFromConnectionInstance(ovsdbConnectionInstance);
+        if (entityConnectionMap.containsKey(candidateEntity)) {
+            LOG.error("Old connection still hanging for {}", candidateEntity);
+            disconnected(ovsdbConnectionInstance.getOvsdbClient());
+            //TODO do cleanup for old connection or stale check
+        }
+        nodeIdVsConnectionInstance.put((InstanceIdentifier<Node>) candidateEntity.getIdentifier(),
+                ovsdbConnectionInstance);
         entityConnectionMap.put(candidateEntity, ovsdbConnectionInstance);
         ovsdbConnectionInstance.setConnectedEntity(candidateEntity);
         try {
@@ -533,28 +574,26 @@ public class OvsdbConnectionManager implements OvsdbConnectionListener, AutoClos
             ovsdbConnectionInstance.setDeviceOwnershipCandidateRegistration(registration);
             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 == EntityOwnershipState.OWNED_BY_OTHER) {
-                    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);
         }
-
+        //If entity already has owner, it won't get notification from EntityOwnershipService
+        Optional<EntityOwnershipState> ownershipStateOpt =
+                entityOwnershipService.getOwnershipState(candidateEntity);
+        if (ownershipStateOpt.isPresent()) {
+            EntityOwnershipState ownershipState = ownershipStateOpt.get();
+            if (ownershipState == EntityOwnershipState.OWNED_BY_OTHER) {
+                ovsdbConnectionInstance.setHasDeviceOwnership(false);
+            } else if (ownershipState == EntityOwnershipState.IS_OWNER) {
+                ovsdbConnectionInstance.setHasDeviceOwnership(true);
+                ovsdbConnectionInstance.registerCallbacks(instanceIdentifierCodec);
+            }
+        }
     }
 
     private void unregisterEntityForOwnership(OvsdbConnectionInstance ovsdbConnectionInstance) {
         ovsdbConnectionInstance.closeDeviceOwnershipCandidateRegistration();
-        entityConnectionMap.remove(ovsdbConnectionInstance.getConnectedEntity());
+        entityConnectionMap.remove(ovsdbConnectionInstance.getConnectedEntity(), ovsdbConnectionInstance);
     }
 
     private void retryConnection(final InstanceIdentifier<Node> iid, final OvsdbNodeAugmentation ovsdbNode,
index 4de54077be0fb044d3ce93f97787249278f4bc32..0e33ad231ad4fd9962e5d015d630baed301cff24 100644 (file)
@@ -91,6 +91,7 @@ public class BridgeUpdateCommand implements TransactCommand {
                     ovsdbManagedNode.getBridgeUuid());
         } else {
             String existingBridgeName = operationalBridgeOptional.get().getBridgeName().getValue();
+            LOG.debug("Bridge {} already exists in device updating {}", existingBridgeName, iid);
             // Name is immutable, and so we *can't* update it.  So we use extraBridge for the schema stuff
             Bridge extraBridge = TyperUtils.getTypedRowWrapper(transaction.getDatabaseSchema(), Bridge.class);
             extraBridge.setName("");
index d1152c4774e81ba4a9675db3b80f2139709a2a5c..dbef7467671fd7aa81ae207db447c76658f11f29 100644 (file)
@@ -12,6 +12,8 @@ import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.CancellationException;
 import java.util.concurrent.ExecutionException;
+
+import org.apache.commons.lang3.StringUtils;
 import org.opendaylight.controller.md.sal.binding.api.DataTreeModification;
 import org.opendaylight.ovsdb.lib.operations.OperationResult;
 import org.opendaylight.ovsdb.lib.operations.TransactionBuilder;
@@ -55,6 +57,11 @@ public class TransactInvokerImpl implements TransactInvoker {
             try {
                 if (!result.isCancelled()) {
                     List<OperationResult> got = result.get();
+                    if (got != null) {
+                        got.stream()
+                                .filter(response -> !StringUtils.isEmpty(response.getError()))
+                                .peek(response -> LOG.error("Failed to transact to device {}", response.getError()));
+                    }
                     LOG.debug("OVSDB transaction result: {}", got);
                 } else {
                     LOG.debug("Operation task cancelled for transaction : {}", tb);
index 0c7145923aa97b9a361ed291f876f308d14d9b60..432fd0602871c122dbb47b1060149c6588448369 100644 (file)
@@ -45,4 +45,9 @@ public abstract class AbstractTransactionCommand implements TransactionCommand {
         this.key = key;
     }
 
+    public void onSuccess() {
+    }
+
+    public void onFailure(Throwable throwable) {
+    }
 }
\ No newline at end of file
index a7ef18e2eaffae434c108884b0276dc01abfbb96..8a2e48373296dc33cd50bb38023c3b3e69e2f587 100644 (file)
@@ -73,6 +73,7 @@ public class OvsdbBridgeUpdateCommand extends AbstractTransactionCommand {
     private final InstanceIdentifierCodec instanceIdentifierCodec;
     private final Map<UUID,Bridge> updatedBridgeRows;
     private final Map<UUID, Bridge> oldBridgeRows;
+    private final List<InstanceIdentifier<Node>> updatedBridges = new ArrayList<>();
 
     public OvsdbBridgeUpdateCommand(InstanceIdentifierCodec instanceIdentifierCodec, OvsdbConnectionInstance key,
             TableUpdates updates, DatabaseSchema dbSchema) {
@@ -109,7 +110,8 @@ public class OvsdbBridgeUpdateCommand extends AbstractTransactionCommand {
         InstanceIdentifier<Node> bridgeIid = getInstanceIdentifier(bridge);
         Node bridgeNode = buildBridgeNode(bridge);
         transaction.merge(LogicalDatastoreType.OPERATIONAL, bridgeIid, bridgeNode);
-        deleteEntries(transaction, protocolEntriesToRemove(bridgeIid,bridge));
+        updatedBridges.add(bridgeIid);
+        deleteEntries(transaction, protocolEntriesToRemove(bridgeIid, bridge));
         deleteEntries(transaction, externalIdsToRemove(bridgeIid,bridge));
         deleteEntries(transaction, bridgeOtherConfigsToRemove(bridgeIid,bridge));
     }
@@ -400,4 +402,16 @@ public class OvsdbBridgeUpdateCommand extends AbstractTransactionCommand {
         NodeKey nodeKey = getInstanceIdentifier(bridge).firstKeyOf(Node.class);
         return nodeKey.getNodeId();
     }
+
+    public void onSuccess() {
+        for (InstanceIdentifier<Node> updatedBridge : updatedBridges) {
+            LOG.debug("Updated bridge {} in operational datastore", updatedBridge);
+        }
+    }
+
+    public void onFailure(Throwable throwable) {
+        for (InstanceIdentifier<Node> updatedBridge : updatedBridges) {
+            LOG.error("Failed to update bridge {} in operational datastore", updatedBridge);
+        }
+    }
 }
index 8234871a47775a7e7fb900a90f2b4c64b811ba32..64e0712afeaaff9a40c1f583ddf337724dd25a0a 100644 (file)
@@ -56,6 +56,7 @@ public class OvsdbNodeRemoveCommand extends AbstractTransactionCommand {
                     } else {
                         LOG.warn("{} had no OvsdbNodeAugmentation", ovsdbNode.getNodeId().getValue());
                     }
+                    LOG.debug("Deleting node {} from oper", getOvsdbConnectionInstance().getInstanceIdentifier());
                     transaction.delete(LogicalDatastoreType.OPERATIONAL,
                             getOvsdbConnectionInstance().getInstanceIdentifier());
                 } else {
index 1843b6faf87cb7c81e92e6676a5477b476267096..8ee27c5344f4c0d9858b4352ac655ca620c7e2e0 100644 (file)
@@ -62,4 +62,18 @@ public class OvsdbOperationalCommandAggregator implements TransactionCommand {
             }
         }
     }
+
+    @Override
+    public void onSuccess() {
+        for (TransactionCommand command: commands) {
+            command.onSuccess();
+        }
+    }
+
+    @Override
+    public void onFailure(Throwable throwable) {
+        for (TransactionCommand command: commands) {
+            command.onFailure(throwable);
+        }
+    }
 }
index f504ff754d38f40d0d53a5bf058733a09ece5326..037bcbb1c588f3b6736b3b2e00a0d4e05f9a9f3f 100644 (file)
@@ -14,4 +14,9 @@ public interface TransactionCommand {
 
     void execute(ReadWriteTransaction transaction);
 
+    default void onSuccess() {
+    }
+
+    default void onFailure(Throwable throwable) {
+    }
 }
index 8a547110fe7372c5c1fbb8a2b648ab1ccf1fe2f2..46b7934622a7023ac3c4d848c76186b2f89c4d05 100644 (file)
@@ -68,6 +68,7 @@ public class TransactionInvokerImpl implements TransactionInvoker,TransactionCha
     @Override
     public void onTransactionChainFailed(TransactionChain<?, ?> chainArg,
             AsyncTransaction<?, ?> transaction, Throwable cause) {
+        LOG.error("Failed to write operational topology", cause);
         offerFailedTransaction(transaction);
     }
 
@@ -103,10 +104,12 @@ public class TransactionInvokerImpl implements TransactionInvoker,TransactionCha
                                 LOG.error("successfulTransactionQueue is full (size: {}) - could not offer {}",
                                         successfulTransactionQueue.size(), transaction);
                             }
+                            command.onSuccess();
                         }
 
                         @Override
                         public void onFailure(final Throwable throwable) {
+                            command.onFailure(throwable);
                             // NOOP - handled by failure of transaction chain
                         }
                     }, MoreExecutors.directExecutor());
index a5c51861e0013d3a69ea634257430c0c187122ff..1307bc966008d6ad4ca9c97adc59e13916e08123 100644 (file)
@@ -23,6 +23,7 @@ import com.google.common.base.Optional;
 import com.google.common.util.concurrent.CheckedFuture;
 import java.net.InetAddress;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
@@ -175,6 +176,7 @@ public class OvsdbConnectionManagerTest {
                 OvsdbConnectionInstance.class));
         instanceIdentifiers = new ConcurrentHashMap<>();
         field(OvsdbConnectionManager.class, "instanceIdentifiers").set(ovsdbConnManager, instanceIdentifiers);
+        field(OvsdbConnectionManager.class, "nodeIdVsConnectionInstance").set(ovsdbConnManager, new HashMap<>());
 
         MemberModifier.suppress(MemberMatcher.method(OvsdbConnectionManager.class, "reconcileConnection",
                 InstanceIdentifier.class, OvsdbNodeAugmentation.class));
index 315e2bab3cd711ea86d1beb573fb60c421ed0bfc..3a10ad1de07ed199a2cf96ac49f58a2b9c466d8e 100644 (file)
@@ -96,6 +96,8 @@ public class OvsdbBridgeUpdateCommandTest {
     @Before
     public void setUp() throws Exception {
         ovsdbBridgeUpdateCommand = PowerMockito.mock(OvsdbBridgeUpdateCommand.class, Mockito.CALLS_REAL_METHODS);
+        MemberModifier.field(OvsdbBridgeUpdateCommand.class, "updatedBridges")
+                .set(ovsdbBridgeUpdateCommand, new ArrayList<>());
         MemberModifier.field(OvsdbBridgeUpdateCommand.class, "updatedBridgeRows").set(ovsdbBridgeUpdateCommand,
                 updatedBridgeRows);
     }