diff --git a/src/main/java/net/floodlightcontroller/core/IOFSwitch.java b/src/main/java/net/floodlightcontroller/core/IOFSwitch.java
index d63624cbd80848777e676e2aefa8bd2164e21534..60df97b40d5b858ac3e2cc8a8ac60a8100aced8a 100644
--- a/src/main/java/net/floodlightcontroller/core/IOFSwitch.java
+++ b/src/main/java/net/floodlightcontroller/core/IOFSwitch.java
@@ -250,7 +250,7 @@ public interface IOFSwitch {
* @return Future object wrapping OFStatisticsReply
* @throws IOException
*/
- public Future<OFFeaturesReply> getFeaturesReplyFromSwitch()
+ public Future<OFFeaturesReply> querySwitchFeaturesReply()
throws IOException;
/**
diff --git a/src/main/java/net/floodlightcontroller/core/internal/Controller.java b/src/main/java/net/floodlightcontroller/core/internal/Controller.java
index 7901b5c4f8a9333db84d94e1c264aed647eb8f13..881466ef3ae05b63f1f5151575f809bee690db3a 100644
--- a/src/main/java/net/floodlightcontroller/core/internal/Controller.java
+++ b/src/main/java/net/floodlightcontroller/core/internal/Controller.java
@@ -751,8 +751,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;
}
}
@@ -803,10 +802,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
@@ -829,7 +827,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()));
@@ -841,7 +839,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()));
@@ -1026,8 +1024,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);
}
}
}
@@ -1355,6 +1352,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.
@@ -1372,16 +1373,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 {
@@ -1418,10 +1423,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 {
@@ -1798,9 +1808,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,
@@ -1835,12 +1856,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/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java b/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java
index f0389a46cfeff34baded619aa907a7103c89d934..e93b627bdb279d162e65f0b98ba371422a4eab7a 100644
--- a/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java
+++ b/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java
@@ -803,9 +803,11 @@ public class OFSwitchImpl implements IOFSwitch {
}
@Override
- public Future<OFFeaturesReply> getFeaturesReplyFromSwitch()
+ public Future<OFFeaturesReply> querySwitchFeaturesReply()
throws IOException {
- OFMessage request = new OFFeaturesRequest();
+ OFMessage request =
+ floodlightProvider.getOFMessageFactory().
+ getMessage(OFType.FEATURES_REQUEST);
request.setXid(getNextTransactionId());
OFFeaturesReplyFuture future =
new OFFeaturesReplyFuture(threadPool, this, request.getXid());
diff --git a/src/main/java/net/floodlightcontroller/core/web/SwitchResourceBase.java b/src/main/java/net/floodlightcontroller/core/web/SwitchResourceBase.java
index d810024d188877d1642d6f38868196bbe0ff3982..81f07bee01ad87ad5f04734df644c658135c77be 100644
--- a/src/main/java/net/floodlightcontroller/core/web/SwitchResourceBase.java
+++ b/src/main/java/net/floodlightcontroller/core/web/SwitchResourceBase.java
@@ -140,7 +140,7 @@ public class SwitchResourceBase extends ServerResource {
OFFeaturesReply featuresReply = null;
if (sw != null) {
try {
- future = sw.getFeaturesReplyFromSwitch();
+ future = sw.querySwitchFeaturesReply();
featuresReply = future.get(10, TimeUnit.SECONDS);
} catch (Exception e) {
log.error("Failure getting features reply from switch" + sw, e);
diff --git a/src/test/java/net/floodlightcontroller/core/internal/ControllerTest.java b/src/test/java/net/floodlightcontroller/core/internal/ControllerTest.java
index 8c77b062d31278afbcb9c1800d1b13a9e45f0010..66b8709b04b3e4aae3fd0edeff6b104b2dbabdab 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;
@@ -479,7 +478,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);
@@ -511,9 +510,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);
}
@@ -1142,7 +1185,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);
@@ -1185,7 +1228,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);
diff --git a/src/test/java/net/floodlightcontroller/util/OFMessageDamperMockSwitch.java b/src/test/java/net/floodlightcontroller/util/OFMessageDamperMockSwitch.java
index 59e0a51b8b24e98449a8c7598c5cab019620eaea..616d4377c6058d8a79008b2c4dabc3cd9236efe3 100644
--- a/src/test/java/net/floodlightcontroller/util/OFMessageDamperMockSwitch.java
+++ b/src/test/java/net/floodlightcontroller/util/OFMessageDamperMockSwitch.java
@@ -304,7 +304,7 @@ public class OFMessageDamperMockSwitch implements IOFSwitch {
}
@Override
- public Future<OFFeaturesReply> getFeaturesReplyFromSwitch()
+ public Future<OFFeaturesReply> querySwitchFeaturesReply()
throws IOException {
// TODO Auto-generated method stub
return null;