Bug 963 - OSGi error in Topology manager component - After exiting

mininet of13 simulation

* added check in onLinkRemoved() to check if link exists before
atempting to delete it (without check, lots of datastore transaction
errors were displayed on mininet shutdown because links were already
removed with nodes)
* added checks to check if node, connector or link exists before trying
to remove it (reads are cheap and delete exception can break the
transaction chain)
* made reads synchronous
* added illegalState exception handling to operation processor
* made affected links removal to be in the same transaction as
Node/NodeConnector removal(creating and applying snapshot for each link
is expensive)
* updated tests

Change-Id: I83b5f91569e64082ce13a4f379cd6261e63fbde6
Signed-off-by: Jan Hajnar <jhajnar@cisco.com>

diff --git a/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporter.java b/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporter.java
index d8602c2ddded47d2708bd89bff5b6cd7a8d42bf7..9b36f9f4973581187e2bf0f45b376c72cdd8da4a 100644 (file)
--- a/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporter.java
+++ b/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporter.java
@@ -68,20 +68,11 @@ class FlowCapableTopologyExporter implements FlowTopologyDiscoveryListener, Open
         final NodeId nodeId = toTopologyNodeId(getNodeKey(notification.getNodeRef()).getId());
         final InstanceIdentifier<Node> nodeInstance = toNodeIdentifier(notification.getNodeRef());
 
-
         processor.enqueueOperation(new TopologyOperation() {
             @Override
             public void applyOperation(ReadWriteTransaction transaction) {
-                Optional<Node> nodeOptional = Optional.absent();
-                try {
-                    nodeOptional = transaction.read(LogicalDatastoreType.OPERATIONAL, nodeInstance).checkedGet();
-                } catch (ReadFailedException e) {
-                    LOG.error("Error occured when trying to read Node ", e);
-                }
-                if (nodeOptional.isPresent()) {
                     removeAffectedLinks(nodeId, transaction);
                     transaction.delete(LogicalDatastoreType.OPERATIONAL, nodeInstance);
-                }
             }
 
             @Override
@@ -117,19 +108,21 @@ class FlowCapableTopologyExporter implements FlowTopologyDiscoveryListener, Open
         final InstanceIdentifier<TerminationPoint> tpInstance = toTerminationPointIdentifier(
                 notification.getNodeConnectorRef());
 
+        final InstanceIdentifier<Node> node = tpInstance.firstIdentifierOf(Node.class);
+
         final TpId tpId = toTerminationPointId(getNodeConnectorKey(
                 notification.getNodeConnectorRef()).getId());
 
         processor.enqueueOperation(new TopologyOperation() {
             @Override
             public void applyOperation(ReadWriteTransaction transaction) {
-                Optional<TerminationPoint> terminationPointOptional = Optional.absent();
+                Optional<Node> nodeOptional = Optional.absent();
                 try {
-                    terminationPointOptional = transaction.read(LogicalDatastoreType.OPERATIONAL, tpInstance).checkedGet();
+                    nodeOptional = transaction.read(LogicalDatastoreType.OPERATIONAL, node).checkedGet();
                 } catch (ReadFailedException e) {
                     LOG.error("Error occured when trying to read NodeConnector ", e);
                 }
-                if (terminationPointOptional.isPresent()) {
+                if (nodeOptional.isPresent()) {
                     removeAffectedLinks(tpId, transaction);
                     transaction.delete(LogicalDatastoreType.OPERATIONAL, tpInstance);
                 }
@@ -216,7 +209,6 @@ class FlowCapableTopologyExporter implements FlowTopologyDiscoveryListener, Open
         });
     }
 
-
     @Override
     public void onLinkUtilizationNormal(final LinkUtilizationNormal notification) {
         // NOOP

diff --git a/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/OperationProcessor.java b/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/OperationProcessor.java
index 41162d30463d54338d687dfd904ef1f7646dbec6..c00943339570dc67ee10ed8349f461132dc1e3e7 100644 (file)
--- a/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/OperationProcessor.java
+++ b/opendaylight/md-sal/topology-manager/src/main/java/org/opendaylight/md/controller/topology/manager/OperationProcessor.java
@@ -29,6 +29,7 @@ final class OperationProcessor implements AutoCloseable, Runnable, TransactionCh
     private final BlockingQueue<TopologyOperation> queue = new LinkedBlockingQueue<>(OPERATION_QUEUE_DEPTH);
     private final DataBroker dataBroker;
     private BindingTransactionChain transactionChain;
+    private volatile boolean finishing = false;
 
     OperationProcessor(final DataBroker dataBroker) {
         this.dataBroker = Preconditions.checkNotNull(dataBroker);
@@ -45,57 +46,56 @@ final class OperationProcessor implements AutoCloseable, Runnable, TransactionCh
 
     @Override
     public void run() {
-        try {
-            for (; ; ) {
-                TopologyOperation op = queue.take();
+            while (!finishing) {
+                try {
+                    TopologyOperation op = queue.take();
 
-                LOG.debug("New {} operation available, starting transaction", op);
+                    LOG.debug("New {} operation available, starting transaction", op);
 
-                final ReadWriteTransaction tx = transactionChain.newReadWriteTransaction();
+                    final ReadWriteTransaction tx = transactionChain.newReadWriteTransaction();
 
-                int ops = 0;
-                do {
-                    op.applyOperation(tx);
+                    int ops = 0;
+                    do {
+                        op.applyOperation(tx);
 
-                    ops++;
-                    if (ops < MAX_TRANSACTION_OPERATIONS) {
-                        op = queue.poll();
-                    } else {
-                        op = null;
-                    }
+                        ops++;
+                        if (ops < MAX_TRANSACTION_OPERATIONS) {
+                            op = queue.poll();
+                        } else {
+                            op = null;
+                        }
 
-                    LOG.debug("Next operation {}", op);
-                } while (op != null);
+                        LOG.debug("Next operation {}", op);
+                    } while (op != null);
 
-                LOG.debug("Processed {} operations, submitting transaction", ops);
+                    LOG.debug("Processed {} operations, submitting transaction", ops);
 
-                try {
-                    tx.submit().checkedGet();
-                } catch (final TransactionCommitFailedException e) {
+                    try {
+                        tx.submit().checkedGet();
+                    } catch (final TransactionCommitFailedException e) {
+                        LOG.warn("Stat DataStoreOperation unexpected State!", e);
+                        transactionChain.close();
+                        transactionChain = dataBroker.createTransactionChain(this);
+                        cleanDataStoreOperQueue();
+                    }
+
+                } catch (final IllegalStateException e) {
                     LOG.warn("Stat DataStoreOperation unexpected State!", e);
                     transactionChain.close();
                     transactionChain = dataBroker.createTransactionChain(this);
                     cleanDataStoreOperQueue();
+                } catch (final InterruptedException e) {
+                    LOG.warn("Stat Manager DS Operation thread interupted!", e);
+                    finishing = true;
+                } catch (final Exception e) {
+                    LOG.warn("Stat DataStore Operation executor fail!", e);
                 }
             }
-        } catch (final IllegalStateException e) {
-            LOG.warn("Stat DataStoreOperation unexpected State!", e);
-            transactionChain.close();
-            transactionChain = dataBroker.createTransactionChain(this);
-            cleanDataStoreOperQueue();
-        } catch (final InterruptedException e) {
-            LOG.warn("Stat Manager DS Operation thread interupted!", e);
-        } catch (final Exception e) {
-            LOG.warn("Stat DataStore Operation executor fail!", e);
-        }
-
         // Drain all events, making sure any blocked threads are unblocked
         cleanDataStoreOperQueue();
-
     }
 
     private void cleanDataStoreOperQueue() {
-        // Drain all events, making sure any blocked threads are unblocked
         while (!queue.isEmpty()) {
             queue.poll();
         }
@@ -104,6 +104,9 @@ final class OperationProcessor implements AutoCloseable, Runnable, TransactionCh
     @Override
     public void onTransactionChainFailed(TransactionChain<?, ?> chain, AsyncTransaction<?, ?> transaction, Throwable cause) {
         LOG.error("Failed to export Topology manager operations, Transaction {} failed.", transaction.getIdentifier(), cause);
+        transactionChain.close();
+        transactionChain = dataBroker.createTransactionChain(this);
+        cleanDataStoreOperQueue();
     }
 
     @Override

diff --git a/opendaylight/md-sal/topology-manager/src/test/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporterTest.java b/opendaylight/md-sal/topology-manager/src/test/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporterTest.java
index d07d42f2ec2d1d486d427d7e5dfa2762722d663f..7f8d021b3bc28963cc144ed8e347d11545431600 100644 (file)
--- a/opendaylight/md-sal/topology-manager/src/test/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporterTest.java
+++ b/opendaylight/md-sal/topology-manager/src/test/java/org/opendaylight/md/controller/topology/manager/FlowCapableTopologyExporterTest.java
@@ -67,7 +67,6 @@ import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.NodeBuilder;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.NodeKey;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.node.TerminationPoint;
-import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.node.TerminationPointBuilder;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.node.TerminationPointKey;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
@@ -243,9 +242,8 @@ public class FlowCapableTopologyExporterTest {
         NodeKey topoNodeKey = new NodeKey(new NodeId("node1"));
         TerminationPointKey terminationPointKey = new TerminationPointKey(new TpId("tp1"));
 
-        InstanceIdentifier<TerminationPoint> topoTermPointII = topologyIID.child(Node.class, topoNodeKey)
-                .child(TerminationPoint.class, terminationPointKey);
-        TerminationPoint topoTermPoint = new TerminationPointBuilder().setKey(terminationPointKey).build();
+        InstanceIdentifier<Node> topoNodeII = topologyIID.child(Node.class, topoNodeKey);
+        Node topoNode = new NodeBuilder().setKey(topoNodeKey).build();
 
         org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeKey
                                                                   nodeKey = newInvNodeKey(topoNodeKey.getNodeId().getValue());
@@ -274,10 +272,10 @@ public class FlowCapableTopologyExporterTest {
         doReturn(Futures.makeChecked(readFuture, ReadFailedException.MAPPER)).when(mockTx1)
                 .read(LogicalDatastoreType.OPERATIONAL, topologyIID);
 
-        SettableFuture<Optional<TerminationPoint>> readFutureNode = SettableFuture.create();
-        readFutureNode.set(Optional.of(topoTermPoint));
+        SettableFuture<Optional<Node>> readFutureNode = SettableFuture.create();
+        readFutureNode.set(Optional.of(topoNode));
         doReturn(Futures.makeChecked(readFutureNode, ReadFailedException.MAPPER)).when(mockTx1)
-                .read(LogicalDatastoreType.OPERATIONAL, topoTermPointII);
+                .read(LogicalDatastoreType.OPERATIONAL, topoNodeII);
 
         CountDownLatch submitLatch1 = setupStubbedSubmit(mockTx1);
 
@@ -287,7 +285,6 @@ public class FlowCapableTopologyExporterTest {
                 ArgumentCaptor.forClass(InstanceIdentifier.class);
         setupStubbedDeletes(mockTx1, deletedLinkIDs, deleteLatch);
 
-
         doReturn(mockTx1).when(mockTxChain).newReadWriteTransaction();
 
         exporter.onNodeConnectorRemoved(new NodeConnectorRemovedBuilder().setNodeConnectorRef(
@@ -311,9 +308,8 @@ public class FlowCapableTopologyExporterTest {
         NodeKey topoNodeKey = new NodeKey(new NodeId("node1"));
         TerminationPointKey terminationPointKey = new TerminationPointKey(new TpId("tp1"));
 
-        InstanceIdentifier<TerminationPoint> topoTermPointII = topologyIID.child(Node.class, topoNodeKey)
-                .child(TerminationPoint.class, terminationPointKey);
-        TerminationPoint topoTermPoint = new TerminationPointBuilder().setKey(terminationPointKey).build();
+        InstanceIdentifier<Node> topoNodeII = topologyIID.child(Node.class, topoNodeKey);
+        Node topoNode = new NodeBuilder().setKey(topoNodeKey).build();
 
         org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeKey
                 nodeKey = newInvNodeKey(topoNodeKey.getNodeId().getValue());
@@ -333,10 +329,10 @@ public class FlowCapableTopologyExporterTest {
                 .read(LogicalDatastoreType.OPERATIONAL, topologyIID);
         CountDownLatch submitLatch = setupStubbedSubmit(mockTx);
 
-        SettableFuture<Optional<TerminationPoint>> readFutureNode = SettableFuture.create();
-        readFutureNode.set(Optional.of(topoTermPoint));
+        SettableFuture<Optional<Node>> readFutureNode = SettableFuture.create();
+        readFutureNode.set(Optional.of(topoNode));
         doReturn(Futures.makeChecked(readFutureNode, ReadFailedException.MAPPER)).when(mockTx)
-                .read(LogicalDatastoreType.OPERATIONAL, topoTermPointII);
+                .read(LogicalDatastoreType.OPERATIONAL, topoNodeII);
 
         CountDownLatch deleteLatch = new CountDownLatch(1);
         ArgumentCaptor<InstanceIdentifier> deletedLinkIDs =
@@ -556,7 +552,7 @@ public class FlowCapableTopologyExporterTest {
     }
 
     @Test
-    public void testOnLinkRemovedLinkExists() {
+    public void testOnLinkRemoved() {
 
         org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeKey
                 sourceNodeKey = newInvNodeKey("sourceNode");