From 162da091eb6080acbf0128c27124eea1389c687c Mon Sep 17 00:00:00 2001 From: Alessandro Boch Date: Tue, 2 Jul 2013 14:13:06 -0700 Subject: [PATCH] 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 --- .../FlowEntryInstall.java | 2 +- .../internal/Activator.java | 22 +++---- .../internal/ForwardingRulesManagerImpl.java | 58 ++++++++++++------- 3 files changed, 48 insertions(+), 34 deletions(-) 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 ebb5a7bb24..ee2113db82 100644 --- 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 2b161e9be7..f6fc0012ad 100644 --- 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> props = new Hashtable>(); Set propSet = new HashSet(); - 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 2c5144f284..7fae181ba6 100644 --- 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 toReInstall = new HashSet(); + // First remove all installed entries for (ConcurrentMap.Entry 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) 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) 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>) 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>) 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) 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) 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) 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) 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>) 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) 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; -- 2.36.6