BUG: Fix missing creation of initial openflow rules upon node creation

While using the registerDataChangeListener for createFlowCapableNodePath, we began to
see cases where onDataChanged provided no entries in getCreatedData() when it should.
To deal with that, the code will now pay attention to updates in getUpdatedData() and
treat it as create if this is the first time the listener hears about the nodeId.

Patch 2:
  - listen to operational instead of configuration mdsal tree;
  - check for create/update inside change loop
  - remove redundant [removed/added] words in log
Patch 3:
  - fix notifyNodeUpdated log
Patch 4:
  - change log level for notifyNodeUpdated
Patch 5:
  - undo patch 4. Lots of logs [1]

[1]: https://gist.github.com/67daada34dd91aff3190

Change-Id: Id1b445ec4892f9c8848b6ca5a6739948d87432c9
Signed-off-by: Flavio Fernandes <ffernand@redhat.com>

diff --git a/openstack/net-virt-providers/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/providers/openflow13/FlowCapableNodeDataChangeListener.java b/openstack/net-virt-providers/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/providers/openflow13/FlowCapableNodeDataChangeListener.java
index 3193d9743636161c53dad2e18c412a976448a5f4..6efd26e99465f98daaa8cb0bae157d2927c6e035 100644 (file)
--- a/openstack/net-virt-providers/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/providers/openflow13/FlowCapableNodeDataChangeListener.java
+++ b/openstack/net-virt-providers/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/providers/openflow13/FlowCapableNodeDataChangeListener.java
@@ -12,7 +12,7 @@ package org.opendaylight.ovsdb.openstack.netvirt.providers.openflow13;
 import com.google.common.collect.Lists;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
+
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.DataChangeListener;
 import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker;
@@ -36,6 +36,7 @@ public class FlowCapableNodeDataChangeListener implements DataChangeListener, Au
     private static final Logger LOG = LoggerFactory.getLogger(FlowCapableNodeDataChangeListener.class);
     private ListenerRegistration<DataChangeListener> registration;
     private List<Node> nodeCache = Lists.newArrayList();
+    private NetworkingProviderManager networkingProviderManager = null;
     private PipelineOrchestrator pipelineOrchestrator = null;
     private NodeCacheManager nodeCacheManager = null;
 
@@ -48,7 +49,7 @@ public class FlowCapableNodeDataChangeListener implements DataChangeListener, Au
 
     public FlowCapableNodeDataChangeListener (DataBroker dataBroker) {
         LOG.info("Registering FlowCapableNodeChangeListener");
-        registration = dataBroker.registerDataChangeListener(LogicalDatastoreType.CONFIGURATION,
+        registration = dataBroker.registerDataChangeListener(LogicalDatastoreType.OPERATIONAL,
                 createFlowCapableNodePath(), this, AsyncDataBroker.DataChangeScope.ONE);
     }
 
@@ -58,63 +59,73 @@ public class FlowCapableNodeDataChangeListener implements DataChangeListener, Au
     }
 
     @Override
-    public void onDataChanged (AsyncDataChangeEvent<InstanceIdentifier<?>,
-            DataObject> changes) {
+    public void onDataChanged (AsyncDataChangeEvent<InstanceIdentifier<?>, DataObject> changes) {
+        checkMemberInitialization();
 
-        LOG.debug("onDataChanged: {}", changes);
+        LOG.debug(">>>> onDataChanged: {}", changes);
 
-        if (pipelineOrchestrator == null) {
-            pipelineOrchestrator =
-                    (PipelineOrchestrator) ServiceHelper.getGlobalInstance(PipelineOrchestrator.class, this);
-        }
-        if (nodeCacheManager == null) {
-            nodeCacheManager = (NodeCacheManager) ServiceHelper.getGlobalInstance(NodeCacheManager.class, this);
+        for (InstanceIdentifier instanceIdentifier : changes.getRemovedPaths()) {
+            DataObject originalDataObject = changes.getOriginalData().get(instanceIdentifier);
+            LOG.info(">>>>> removed iiD: {} - dataObject {}", instanceIdentifier, originalDataObject);
+            if (originalDataObject != null && originalDataObject instanceof Node){
+                Node node = (Node) originalDataObject;
+                notifyNodeRemoved(NodeUtils.getOpenFlowNode(node.getId().getValue()));
+            }
         }
 
-        for( Map.Entry<InstanceIdentifier<?>, DataObject> created : changes.getCreatedData().entrySet()) {
+        for (Map.Entry<InstanceIdentifier<?>, DataObject> created : changes.getCreatedData().entrySet()) {
             InstanceIdentifier<?> iID = created.getKey();
             String openflowId = iID.firstKeyOf(Node.class, NodeKey.class).getId().getValue();
             LOG.debug(">>>>> created iiD: {} - first: {} - NodeKey: {}",
-                    iID,
-                    iID.firstIdentifierOf(Node.class),
-                    openflowId);
-
-            if (pipelineOrchestrator != null) {
-                pipelineOrchestrator.enqueue(openflowId);
+                    iID, iID.firstIdentifierOf(Node.class), openflowId);
+            Node openFlowNode = NodeUtils.getOpenFlowNode(openflowId);
+            if (nodeCache.contains(openFlowNode)) {
+                notifyNodeUpdated(openFlowNode);
+            } else {
+                notifyNodeCreated(openFlowNode);
             }
-
-            notifyNodeUpdated(NodeUtils.getOpenFlowNode(openflowId));
         }
 
-        for (InstanceIdentifier instanceIdentifier : changes.getRemovedPaths()) {
-            DataObject originalDataObject = changes.getOriginalData().get(instanceIdentifier);
-            if (originalDataObject != null && originalDataObject instanceof Node){
-                Node node = (Node) originalDataObject;
-                notifyNodeRemoved(NodeUtils.getOpenFlowNode(node.getId().getValue()));
+        for (Map.Entry<InstanceIdentifier<?>, DataObject> updated : changes.getUpdatedData().entrySet()) {
+            InstanceIdentifier<?> iID = updated.getKey();
+            String openflowId = iID.firstKeyOf(Node.class, NodeKey.class).getId().getValue();
+            LOG.debug(">>>>> updated iiD: {} - first: {} - NodeKey: {}",
+                    iID, iID.firstIdentifierOf(Node.class), openflowId);
+            Node openFlowNode = NodeUtils.getOpenFlowNode(openflowId);
+            if (nodeCache.contains(openFlowNode)) {
+                notifyNodeUpdated(openFlowNode);
+            } else {
+                notifyNodeCreated(openFlowNode);
             }
         }
     }
 
-    public void notifyNodeUpdated (Node openFlowNode) {
-        LOG.debug("notifyNodeUpdated: Node {} update from Controller's inventory Service",
-                openFlowNode.getId().getValue());
+    private void notifyNodeUpdated (Node openFlowNode) {
+        final String openflowId = openFlowNode.getId().getValue();
+        LOG.trace("notifyNodeUpdated: Node {} from Controller's inventory Service", openflowId);
 
-        // Add the Node Type check back once the Consistency issue is resolved between MD-SAL and AD-SAL
-        if (!nodeCache.contains(openFlowNode)) {
-            nodeCache.add(openFlowNode);
-            NetworkingProviderManager networkingProviderManager =
-                    (NetworkingProviderManager) ServiceHelper.getGlobalInstance(NetworkingProviderManager.class,
-                            this);
-            networkingProviderManager.getProvider(openFlowNode).initializeOFFlowRules(openFlowNode);
+        // TODO: will do something amazing here, someday
+    }
 
-            if (nodeCacheManager != null) {
-                nodeCacheManager.nodeAdded(openFlowNode.getId().getValue());
-            }
+    private void notifyNodeCreated (Node openFlowNode) {
+        final String openflowId = openFlowNode.getId().getValue();
+        LOG.info("notifyNodeCreated: Node {} from Controller's inventory Service", openflowId);
+
+        nodeCache.add(openFlowNode);
+
+        if (networkingProviderManager != null) {
+            networkingProviderManager.getProvider(openFlowNode).initializeOFFlowRules(openFlowNode);
+        }
+        if (pipelineOrchestrator != null) {
+            pipelineOrchestrator.enqueue(openflowId);
+        }
+        if (nodeCacheManager != null) {
+            nodeCacheManager.nodeAdded(openflowId);
         }
     }
 
-    public void notifyNodeRemoved (Node openFlowNode) {
-        LOG.debug("notifyNodeRemoved: Node {} update from Controller's inventory Service",
+    private void notifyNodeRemoved (Node openFlowNode) {
+        LOG.info("notifyNodeRemoved: Node {} from Controller's inventory Service",
                 openFlowNode.getId().getValue());
 
         nodeCache.remove(openFlowNode);
@@ -123,4 +134,22 @@ public class FlowCapableNodeDataChangeListener implements DataChangeListener, Au
             nodeCacheManager.nodeRemoved(openFlowNode.getId().getValue());
         }
     }
+
+    private void checkMemberInitialization() {
+        /**
+         * Obtain local ref to members, if needed. Having these local saves us from calling getGlobalInstance
+         * upon every event.
+         */
+        if (networkingProviderManager == null) {
+            networkingProviderManager =
+                    (NetworkingProviderManager) ServiceHelper.getGlobalInstance(NetworkingProviderManager.class, this);
+        }
+        if (pipelineOrchestrator == null) {
+            pipelineOrchestrator =
+                    (PipelineOrchestrator) ServiceHelper.getGlobalInstance(PipelineOrchestrator.class, this);
+        }
+        if (nodeCacheManager == null) {
+            nodeCacheManager = (NodeCacheManager) ServiceHelper.getGlobalInstance(NodeCacheManager.class, this);
+        }
+    }
 }