From 71dfb09aa4f6ef41a434e963e0b3a41e8d0aac01 Mon Sep 17 00:00:00 2001
From: Gregor Maier <gregor.maier@bigswitch.com>
Date: Sun, 21 Oct 2012 23:16:49 -0700
Subject: [PATCH] Switch handshake massaging.
* Must not call clearAllFlowMods() on slave -> master transition.
* Hack to solve immediate perf problem on switch handshake: only call
readSwitchPortStateFromStorage() on role change. Switch handshake
is now fast again but role change is slow (which is has been before)
BSC-2695
BSC-2727
---
.../core/internal/Controller.java | 53 ++++++++++++-------
.../core/internal/ControllerTest.java | 53 +++++++++++++++++--
2 files changed, 82 insertions(+), 24 deletions(-)
diff --git a/src/main/java/net/floodlightcontroller/core/internal/Controller.java b/src/main/java/net/floodlightcontroller/core/internal/Controller.java
index 16cbd7ef1..e00eca572 100644
--- a/src/main/java/net/floodlightcontroller/core/internal/Controller.java
+++ b/src/main/java/net/floodlightcontroller/core/internal/Controller.java
@@ -750,8 +750,7 @@ public class Controller implements IFloodlightProviderService,
role, sw);
// Need to clear FlowMods before we add the switch
// and dispatch updates otherwise we have a race condition.
- sw.clearAllFlowMods();
- addSwitch(sw);
+ addSwitch(sw, true, false);
state.firstRoleReplyReceived = true;
}
}
@@ -802,10 +801,9 @@ public class Controller implements IFloodlightProviderService,
sw.deliverRoleReply(vendorMessage.getXid(), role);
- boolean isActive = activeSwitches.containsKey(sw.getId());
if (sw.isActive()) {
// Transition from SLAVE to MASTER.
-
+ boolean shouldClearFlowMods = false;
if (!state.firstRoleReplyReceived ||
getAlwaysClearFlowsOnSwAdd()) {
// This is the first role-reply message we receive from
@@ -828,7 +826,7 @@ public class Controller implements IFloodlightProviderService,
// flow-table. The end result would be that the flow
// table for a newly connected switch is never
// flushed. Not sure how to handle that case though...
- sw.clearAllFlowMods();
+ shouldClearFlowMods = true;
log.debug("First role reply from master switch {}, " +
"clear FlowTable to active switch list",
HexString.toHexString(sw.getId()));
@@ -840,7 +838,7 @@ public class Controller implements IFloodlightProviderService,
// doesn't support the role request messages, so in that
// case we're effectively in the EQUAL role and the
// switch should be included in the active switch list.
- addSwitch(sw);
+ addSwitch(sw, shouldClearFlowMods, true);
log.debug("Added master switch {} to active switch list",
HexString.toHexString(sw.getId()));
@@ -1025,8 +1023,7 @@ public class Controller implements IFloodlightProviderService,
// and dispatch updates otherwise we have a race condition.
// TODO: switch update is async. Won't we still have a potential
// race condition?
- sw.clearAllFlowMods();
- addSwitch(sw);
+ addSwitch(sw, true, false);
}
}
}
@@ -1354,6 +1351,10 @@ public class Controller implements IFloodlightProviderService,
* This happens either when a switch first connects (and the controller is
* not in the slave role) or when the role of the controller changes from
* slave to master.
+ *
+ * FIXME: remove shouldReadSwitchPortStateFromStorage argument once
+ * performance problems are solved. We should call it always or never.
+ *
* @param sw the switch that has been added
*/
// TODO: need to rethink locking and the synchronous switch update.
@@ -1371,16 +1372,20 @@ public class Controller implements IFloodlightProviderService,
"very rarely, then it is likely this is a transient " +
"network problem that can be ignored."
)
- protected void addSwitch(IOFSwitch sw) {
+ protected void addSwitch(IOFSwitch sw,
+ boolean shouldClearFlowMods,
+ boolean shouldReadSwitchPortStateFromStorage) {
// TODO: is it safe to modify the HashMap without holding
// the old switch's lock?
OFSwitchImpl oldSw = (OFSwitchImpl) this.activeSwitches.put(sw.getId(), sw);
if (sw == oldSw) {
// Note == for object equality, not .equals for value
- log.debug("New add switch for pre-existing switch {}", sw);
+ log.info("New add switch for pre-existing switch {}", sw);
return;
}
+
+
if (oldSw != null) {
oldSw.getListenerWriteLock().lock();
try {
@@ -1417,10 +1422,15 @@ public class Controller implements IFloodlightProviderService,
oldSw.getListenerWriteLock().unlock();
}
}
- else {// Some switches don't seem to update us with port
- // status messages while in slave role.
+
+ // Some switches don't seem to update us with port
+ // status messages while in slave role.
+ if (shouldReadSwitchPortStateFromStorage)
readSwitchPortStateFromStorage(sw);
- }
+
+ if (shouldClearFlowMods)
+ sw.clearAllFlowMods();
+
updateActiveSwitchInfo(sw);
SwitchUpdate update = new SwitchUpdate(sw, SwitchUpdateType.ADDED);
try {
@@ -1797,9 +1807,20 @@ public class Controller implements IFloodlightProviderService,
/**
* Read switch port data from storage and write it into a switch object
+ * This is a hack since some switches don't seem to update us with
+ * port status messages while in slave mode, so we read the port
+ * list from storage and /add/ them to the current port list.
+ * NOTE: this method /will not send/ a PORTCHANGED switch update since
+ * we usually call it from addSwitch.
+ *
* @param sw the switch to update
*/
protected void readSwitchPortStateFromStorage(IOFSwitch sw) {
+ /*
+ * FIXME: This method causes performance degradation if many switches
+ * are connected updated. CHANGE IT. Once done remove the
+ * shouldReadSwitchPortStateFromStorage() argument to addSwitch
+ */
OperatorPredicate op =
new OperatorPredicate(PORT_SWITCH,
OperatorPredicate.Operator.EQ,
@@ -1834,12 +1855,6 @@ public class Controller implements IFloodlightProviderService,
// ignore
}
}
- SwitchUpdate update = new SwitchUpdate(sw, SwitchUpdateType.PORTCHANGED);
- try {
- this.updates.put(update);
- } catch (InterruptedException e) {
- log.error("Failure adding update to queue", e);
- }
}
protected void removePortInfo(IOFSwitch sw, short portNumber) {
diff --git a/src/test/java/net/floodlightcontroller/core/internal/ControllerTest.java b/src/test/java/net/floodlightcontroller/core/internal/ControllerTest.java
index 02695737b..ca0dc4f6e 100644
--- a/src/test/java/net/floodlightcontroller/core/internal/ControllerTest.java
+++ b/src/test/java/net/floodlightcontroller/core/internal/ControllerTest.java
@@ -64,7 +64,6 @@ import net.floodlightcontroller.test.FloodlightTestCase;
import net.floodlightcontroller.threadpool.IThreadPoolService;
import org.easymock.Capture;
-import org.easymock.EasyMock;
import org.jboss.netty.channel.Channel;
import org.junit.Test;
import org.openflow.protocol.OFError;
@@ -478,7 +477,7 @@ public class ControllerTest extends FloodlightTestCase {
}
@Test
- public void testAddSwitch() throws Exception {
+ public void testAddSwitchNoClearFM() throws Exception {
controller.activeSwitches = new ConcurrentHashMap<Long, IOFSwitch>();
//OFSwitchImpl oldsw = createMock(OFSwitchImpl.class);
@@ -510,9 +509,53 @@ public class ControllerTest extends FloodlightTestCase {
expect(newsw.getTables()).andReturn((byte)0).anyTimes();
expect(newsw.getActions()).andReturn(0).anyTimes();
controller.activeSwitches.put(0L, oldsw);
+
+ replay(newsw, channel, channel2);
+
+ controller.addSwitch(newsw, false, false);
+
+ verify(newsw, channel, channel2);
+ }
+
+ @Test
+ public void testAddSwitchClearFM() throws Exception {
+ controller.activeSwitches = new ConcurrentHashMap<Long, IOFSwitch>();
+
+ //OFSwitchImpl oldsw = createMock(OFSwitchImpl.class);
+ OFSwitchImpl oldsw = new OFSwitchImpl();
+ OFFeaturesReply featuresReply = new OFFeaturesReply();
+ featuresReply.setDatapathId(0L);
+ featuresReply.setPorts(new ArrayList<OFPhysicalPort>());
+ oldsw.setFeaturesReply(featuresReply);
+ //expect(oldsw.getId()).andReturn(0L).anyTimes();
+ //expect(oldsw.asyncRemoveSwitchLock()).andReturn(rwlock.writeLock()).anyTimes();
+ //oldsw.setConnected(false);
+ //expect(oldsw.getStringId()).andReturn("00:00:00:00:00:00:00").anyTimes();
+
+ Channel channel = createNiceMock(Channel.class);
+ //expect(oldsw.getChannel()).andReturn(channel);
+ oldsw.setChannel(channel);
+ expect(channel.close()).andReturn(null);
+
+ IOFSwitch newsw = createMock(IOFSwitch.class);
+ expect(newsw.getId()).andReturn(0L).anyTimes();
+ expect(newsw.getStringId()).andReturn("00:00:00:00:00:00:00").anyTimes();
+ expect(newsw.getConnectedSince()).andReturn(new Date());
+ Channel channel2 = createMock(Channel.class);
+ expect(newsw.getChannel()).andReturn(channel2);
+ expect(channel2.getRemoteAddress()).andReturn(null);
+ expect(newsw.getPorts()).andReturn(new ArrayList<OFPhysicalPort>());
+ expect(newsw.getCapabilities()).andReturn(0).anyTimes();
+ expect(newsw.getBuffers()).andReturn(0).anyTimes();
+ expect(newsw.getTables()).andReturn((byte)0).anyTimes();
+ expect(newsw.getActions()).andReturn(0).anyTimes();
+ newsw.clearAllFlowMods();
+ expectLastCall().once();
+ controller.activeSwitches.put(0L, oldsw);
+
replay(newsw, channel, channel2);
- controller.addSwitch(newsw);
+ controller.addSwitch(newsw, true, false);
verify(newsw, channel, channel2);
}
@@ -1141,7 +1184,7 @@ public class ControllerTest extends FloodlightTestCase {
expect(chdlr.sw.isActive()).andReturn(true);
controller.activeSwitches.put(1L, chdlr.sw);
chdlr.state.firstRoleReplyReceived = false;
- chdlr.sw.clearAllFlowMods();
+ // Must not clear flow mods
replay(chdlr.sw);
chdlr.processOFMessage(msg);
verify(chdlr.sw);
@@ -1184,7 +1227,7 @@ public class ControllerTest extends FloodlightTestCase {
*/
@Test
public void testRemoveActiveSwitch() {
- IOFSwitch sw = EasyMock.createNiceMock(IOFSwitch.class);
+ IOFSwitch sw = createNiceMock(IOFSwitch.class);
boolean exceptionThrown = false;
expect(sw.getId()).andReturn(1L).anyTimes();
replay(sw);
--
GitLab