Fix container flow logic in forwarding rules manager

- In Activator.java:
   + All instances of FRM must be IContainerListener
   + Pass the right cache name in order to have the save config event working in the cluster
- In ForwardingRulesManaerImpl.java:
   + Add container check filtering in the IContainerListener methods
   + In updateFlowsContainerFlow() first uninstall all current flows, then reinstall instead
     of interleave remove/add which could cause conflict failures during installation phase
     as the new entries could conflict with existing old container flow merged flows present
     on the netwrok node
   + minor logs and style changes
- In FlowEntryInstall.java:
   + Fixed a bug where container flow merging was altering the original flow entry

Signed-off-by: Alessandro Boch <aboch@cisco.com>

diff --git a/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntryInstall.java b/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntryInstall.java
index ebb5a7bb2457125d8c3c27ea795a820377354eef..ee2113db8287066f7337a147ade4e7d9048bf2ea 100644 (file)
--- a/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntryInstall.java
+++ b/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntryInstall.java
@@ -34,7 +34,7 @@ public class FlowEntryInstall implements Serializable {
     public FlowEntryInstall(FlowEntry original, ContainerFlow cFlow) {
         this.original = original;
         this.cFlow = cFlow;
-        this.install = (cFlow == null) ? original.clone() : original.mergeWith(cFlow);
+        this.install = (cFlow == null) ? original.clone() : original.clone().mergeWith(cFlow);
         deletePending = false;
         requestId = 0;
     }

diff --git a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/Activator.java b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/Activator.java
index 2b161e9be7018365f6fee3ca7dcdb989e6257da4..f6fc0012ad4219f368373cb10284e1c401e96a0f 100644 (file)
--- a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/Activator.java
+++ b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/Activator.java
@@ -22,7 +22,6 @@ import org.opendaylight.controller.sal.core.IContainer;
 import org.opendaylight.controller.sal.core.IContainerListener;
 import org.opendaylight.controller.sal.flowprogrammer.IFlowProgrammerListener;
 import org.opendaylight.controller.sal.flowprogrammer.IFlowProgrammerService;
-import org.opendaylight.controller.sal.utils.GlobalConstants;
 import org.opendaylight.controller.switchmanager.IInventoryListener;
 import org.opendaylight.controller.switchmanager.ISwitchManager;
 import org.opendaylight.controller.switchmanager.ISwitchManagerAware;
@@ -41,6 +40,7 @@ public class Activator extends ComponentActivatorAbstractBase {
      * are done by the ComponentActivatorAbstractBase.
      *
      */
+    @Override
     public void init() {
 
     }
@@ -50,6 +50,7 @@ public class Activator extends ComponentActivatorAbstractBase {
      * ComponentActivatorAbstractBase
      *
      */
+    @Override
     public void destroy() {
 
     }
@@ -63,6 +64,7 @@ public class Activator extends ComponentActivatorAbstractBase {
      *         instantiated in order to get an fully working implementation
      *         Object
      */
+    @Override
     public Object[] getImplementations() {
         Object[] res = { ForwardingRulesManagerImpl.class };
         return res;
@@ -83,26 +85,20 @@ public class Activator extends ComponentActivatorAbstractBase {
      *            per-container different behavior if needed, usually should not
      *            be the case though.
      */
+    @Override
     public void configureInstance(Component c, Object imp, String containerName) {
         if (imp.equals(ForwardingRulesManagerImpl.class)) {
             String interfaces[] = null;
             Dictionary<String, Set<String>> props = new Hashtable<String, Set<String>>();
             Set<String> propSet = new HashSet<String>();
-            propSet.add("staticFlows");
+            propSet.add("frm.flowsSaveEvent");
             props.put("cachenames", propSet);
 
             // export the service
-            if (containerName.equals(GlobalConstants.DEFAULT.toString())) {
-                interfaces = new String[] { IContainerListener.class.getName(), ISwitchManagerAware.class.getName(),
-                        IForwardingRulesManager.class.getName(), IInventoryListener.class.getName(),
-                        ICacheUpdateAware.class.getName(), IConfigurationContainerAware.class.getName(),
-                        IFlowProgrammerListener.class.getName() };
-            } else {
-                interfaces = new String[] { ISwitchManagerAware.class.getName(),
-                        IForwardingRulesManager.class.getName(), IInventoryListener.class.getName(),
-                        ICacheUpdateAware.class.getName(), IConfigurationContainerAware.class.getName(),
-                        IFlowProgrammerListener.class.getName() };
-            }
+            interfaces = new String[] { IContainerListener.class.getName(), ISwitchManagerAware.class.getName(),
+                    IForwardingRulesManager.class.getName(), IInventoryListener.class.getName(),
+                    ICacheUpdateAware.class.getName(), IConfigurationContainerAware.class.getName(),
+                    IFlowProgrammerListener.class.getName() };
 
             c.setInterface(interfaces, props);
 

diff --git a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java
index 2c5144f284b908f46d2a114da4acf2f2c3a14a58..7fae181ba632c0aa51a26d96dccd93698d313eab 100644 (file)
--- a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java
+++ b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManagerImpl.java
@@ -929,16 +929,21 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager, Port
      * on the network node
      */
     private void updateFlowsContainerFlow() {
+        Set<FlowEntry> toReInstall = new HashSet<FlowEntry>();
+        // First remove all installed entries
         for (ConcurrentMap.Entry<FlowEntryInstall, FlowEntryInstall> entry : installedSwView.entrySet()) {
             FlowEntryInstall current = entry.getValue();
-            FlowEntry reInstall = current.getOriginal();
+            // Store the original entry
+            toReInstall.add(current.getOriginal());
             // Remove the old couples. No validity checks to be run, use the
             // internal remove
             this.removeEntryInternal(current, false);
-
+        }
+        // Then reinstall the original entries
+        for (FlowEntry entry : toReInstall) {
             // Reinstall the original flow entries, via the regular path: new
             // cFlow merge + validations
-            this.installFlowEntry(reInstall);
+            this.installFlowEntry(entry);
         }
     }
 
@@ -1131,7 +1136,7 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager, Port
             return;
         }
 
-        log.debug("FRM allocateCaches for Container {}", container);
+        log.debug("Allocating caches for Container {}", container.getName());
 
         try {
             clusterContainerService.createCache("frm.originalSwView",
@@ -1165,9 +1170,9 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager, Port
                     EnumSet.of(IClusterServices.cacheMode.NON_TRANSACTIONAL));
 
         } catch (CacheConfigException cce) {
-            log.error("FRM CacheConfigException");
+            log.error("CacheConfigException");
         } catch (CacheExistException cce) {
-            log.error("FRM CacheExistException");
+            log.error("CacheExistException");
         }
     }
 
@@ -1180,76 +1185,76 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager, Port
             return;
         }
 
-        log.debug("FRM retrieveCaches for Container {}", container);
+        log.debug("Retrieving Caches for Container {}", container.getName());
 
         map = clusterContainerService.getCache("frm.originalSwView");
         if (map != null) {
             originalSwView = (ConcurrentMap<FlowEntry, FlowEntry>) map;
         } else {
-            log.error("FRM Cache frm.originalSwView allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.originalSwView cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.installedSwView");
         if (map != null) {
             installedSwView = (ConcurrentMap<FlowEntryInstall, FlowEntryInstall>) map;
         } else {
-            log.error("FRM Cache frm.installedSwView allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.installedSwView cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.nodeFlows");
         if (map != null) {
             nodeFlows = (ConcurrentMap<Node, List<FlowEntryInstall>>) map;
         } else {
-            log.error("FRM Cache frm.nodeFlows allocation failed for Container {}", container.getName());
+            log.error("Retrieval of cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.groupFlows");
         if (map != null) {
             groupFlows = (ConcurrentMap<String, List<FlowEntryInstall>>) map;
         } else {
-            log.error("FRM Cache frm.groupFlows allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.groupFlows cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.staticFlows");
         if (map != null) {
             staticFlows = (ConcurrentMap<Integer, FlowConfig>) map;
         } else {
-            log.error("FRM Cache frm.staticFlows allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.staticFlows cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.flowsSaveEvent");
         if (map != null) {
             flowsSaveEvent = (ConcurrentMap<Long, String>) map;
         } else {
-            log.error("FRM Cache frm.flowsSaveEvent allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.flowsSaveEvent cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.staticFlowsOrdinal");
         if (map != null) {
             staticFlowsOrdinal = (ConcurrentMap<Integer, Integer>) map;
         } else {
-            log.error("FRM Cache frm.staticFlowsOrdinal allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.staticFlowsOrdinal cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.portGroupConfigs");
         if (map != null) {
             portGroupConfigs = (ConcurrentMap<String, PortGroupConfig>) map;
         } else {
-            log.error("FRM Cache frm.portGroupConfigs allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.portGroupConfigs cache failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.portGroupData");
         if (map != null) {
             portGroupData = (ConcurrentMap<PortGroupConfig, Map<Node, PortGroup>>) map;
         } else {
-            log.error("FRM Cache frm.portGroupData allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.portGroupData allocation failed for Container {}", container.getName());
         }
 
         map = clusterContainerService.getCache("frm.TSPolicies");
         if (map != null) {
             TSPolicies = (ConcurrentMap<String, Object>) map;
         } else {
-            log.error("FRM Cache frm.TSPolicies allocation failed for Container {}", container.getName());
+            log.error("Retrieval of frm.TSPolicies cache failed for Container {}", container.getName());
         }
 
     }
@@ -2217,12 +2222,19 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager, Port
 
     @Override
     public void tagUpdated(String containerName, Node n, short oldTag, short newTag, UpdateType t) {
-
+        if (!container.getName().equals(containerName)) {
+            return;
+        }
     }
 
     @Override
-    public void containerFlowUpdated(String containerName, ContainerFlow previousFlow, ContainerFlow currentFlow,
+    public void containerFlowUpdated(String containerName, ContainerFlow previous, ContainerFlow current,
             UpdateType t) {
+        if (!container.getName().equals(containerName)) {
+            return;
+        }
+        log.trace("Container {}: Updating installed flows because of container flow change: {} {}",
+                container.getName(), t, current);
         /*
          * Whether it is an addition or removal, we have to recompute the merged
          * flows entries taking into account all the current container flows
@@ -2233,11 +2245,17 @@ public class ForwardingRulesManagerImpl implements IForwardingRulesManager, Port
 
     @Override
     public void nodeConnectorUpdated(String containerName, NodeConnector p, UpdateType t) {
-        // No action
+        if (!container.getName().equals(containerName)) {
+            return;
+        }
     }
 
     @Override
     public void containerModeUpdated(UpdateType update) {
+        // Only default container instance reacts on this event
+        if (!container.getName().equals(GlobalConstants.DEFAULT.toString())) {
+            return;
+        }
         switch (update) {
         case ADDED:
             this.inContainerMode = true;