Adding unit test for Role Changing code in Controller and OFSwitchImpl.

Now with all relevant files added!

[#26756797]

build.xml

@@ -43,7 +43,6 @@
         <include name="logback-core-1.0.0.jar"/>
         <include name="jackson-core-asl-1.8.6.jar"/>
         <include name="jackson-mapper-asl-1.8.6.jar"/>
-        <include name="org.easymock_2.5.2.jar"/>
         <include name="slf4j-api-1.6.4.jar"/>
         <include name="org.restlet-2.1-RC1.jar"/>
         <include name="org.restlet.ext.jackson-2.1-RC1.jar"/>
@@ -84,7 +83,9 @@
 
     <patternset id="lib-test">
         <include name="junit-4.8.2.jar"/>
-        <include name="org.easymock_2.5.2.jar"/>
+        <include name="org.easymock-3.1.jar"/>
+		<include name="objenesis-1.2.jar"/>  <!-- required by easymock to mock classes -->
+		<include name="cglib-nodep-2.2.2.jar"/>    <!-- required by easymock to mock classes -->
     </patternset>
     <path id="classpath-test">
         <fileset dir="lib">

src/main/java/net/floodlightcontroller/core/IOFSwitch.java

@@ -51,6 +51,7 @@ public interface IOFSwitch {
     /**
      * Writes to the OFMessage to the output stream.
      * The message will be handed to the floodlightProvider for possible filtering
+     * and processing by message listeners
      * @param m   
      * @param bc  
      * @throws IOException  
@@ -60,6 +61,7 @@ public interface IOFSwitch {
     /**
      * Writes the list of messages to the output stream
      * The message will be handed to the floodlightProvider for possible filtering
+     * and processing by message listeners.
      * @param msglist
      * @param bc
      * @throws IOException
@@ -73,7 +75,8 @@ public interface IOFSwitch {
     public void disconnectOutputStream();
 
     /**
-     *
+     * FIXME: remove getChannel(). All access to the channel should be through
+     *        wrapper functions in IOFSwitch
      * @return
      */
     public Channel getChannel();
@@ -214,12 +217,6 @@ public interface IOFSwitch {
      */
     public Role getRole();
     
-    /**
-     * Set the role of the controller for the switch
-     * @param role controller role
-     */
-    public void setRole(Role role);
-    
     /**
      * Check if the controller is an active controller for the switch.
      * The controller is active if its role is MASTER or EQUAL.

src/main/java/net/floodlightcontroller/core/internal/OFChannelState.java

@@ -39,6 +39,7 @@ class OFChannelState {
 
         /**
          * We've received the features reply
+         * Waiting for Config and Description reply
          */
         FEATURES_REPLY,
 
@@ -53,11 +54,11 @@ class OFChannelState {
     protected boolean hasGetConfigReply = false;
     protected boolean hasDescription = false;
     
-    // The hasNxRoleReply flag doesn't mean that the switch supports the NX
-    // role messages, just that we've received an answer back from the 
-    // switch (possibly a bad vendor error) in response to our initial
-    // role request. It's used as a flag to indicate that we've met one
-    // of the conditions necessary to advance the handshake state to READY.
-    protected boolean hasNxRoleReply = false;
-    protected int nxRoleRequestXid;
+    // The firstRoleReplyRecevied flag indicates if we have received the
+    // first role reply message on this connection (in response to the 
+    // role request sent after the handshake). If role support is disabled
+    // on the controller we also set this flag to true. 
+    // The flag is used to decide if the flow table should be wiped
+    // @see Controller.handleRoleReplyMessage()
+    protected boolean firstRoleReplyReceived = false;
 }
\ No newline at end of file

src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java

@@ -21,6 +21,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
@@ -47,6 +48,7 @@ import org.openflow.protocol.OFMessage;
 import org.openflow.protocol.OFPhysicalPort;
 import org.openflow.protocol.OFPort;
 import org.openflow.protocol.OFType;
+import org.openflow.protocol.OFVendor;
 import org.openflow.protocol.OFPhysicalPort.OFPortConfig;
 import org.openflow.protocol.OFPhysicalPort.OFPortState;
 import org.openflow.protocol.OFStatisticsRequest;
@@ -54,6 +56,9 @@ import org.openflow.protocol.statistics.OFDescriptionStatistics;
 import org.openflow.protocol.statistics.OFStatistics;
 import org.openflow.util.HexString;
 import org.openflow.util.U16;
+import org.openflow.vendor.nicira.OFNiciraVendorData;
+import org.openflow.vendor.nicira.OFRoleRequestVendorData;
+import org.openflow.vendor.nicira.OFRoleVendorData;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -62,6 +67,8 @@ import org.slf4j.LoggerFactory;
  * @author David Erickson (daviderickson@cs.stanford.edu)
  */
 public class OFSwitchImpl implements IOFSwitch {
+    // TODO: should we really do logging in the class or should we throw
+    // exception that can then be handled by callers?
     protected static Logger log = LoggerFactory.getLogger(OFSwitchImpl.class);
 
     protected ConcurrentMap<Object, Object> attributes;
@@ -82,6 +89,17 @@ public class OFSwitchImpl implements IOFSwitch {
     protected TimedCache<Long> timedCache;
     protected ReentrantReadWriteLock listenerLock;
     protected ConcurrentMap<Short, Long> portBroadcastCacheHitMap;
+    /**
+     * When sending a role request message, the role request is added
+     * to this queue. If a role reply is received this queue is checked to 
+     * verify that the reply matches the expected reply. We require in order
+     * delivery of replies. That's why we use a Queue. 
+     * The RoleChanger uses a timeout to ensure we receive a timely reply.
+     * 
+     * Need to synchronize on this instance if a request is sent, received, 
+     * checked. 
+     */
+    protected LinkedList<PendingRoleRequestEntry> pendingRoleRequests;
     
     public static IOFSwitchFeatures switchFeatures;
     protected static final ThreadLocal<Map<OFSwitchImpl,List<OFMessage>>> local_msg_buffer =
@@ -95,6 +113,18 @@ public class OFSwitchImpl implements IOFSwitch {
     // for managing our map sizes
     protected static final int MAX_MACS_PER_SWITCH  = 1000;
     
+    protected static class PendingRoleRequestEntry {
+        protected int xid;
+        protected Role role;
+        // cookie is used to identify the role "generation". roleChanger uses
+        protected long cookie;
+        public PendingRoleRequestEntry(int xid, Role role, long cookie) {
+            this.xid = xid;
+            this.role = role;
+            this.cookie = cookie;
+        }
+    }
+    
     public OFSwitchImpl() {
         this.attributes = new ConcurrentHashMap<Object, Object>();
         this.connectedSince = new Date();
@@ -108,6 +138,7 @@ public class OFSwitchImpl implements IOFSwitch {
         this.timedCache = new TimedCache<Long>(100, 5*1000 );  // 5 seconds interval
         this.listenerLock = new ReentrantReadWriteLock();
         this.portBroadcastCacheHitMap = new ConcurrentHashMap<Short, Long>();
+        this.pendingRoleRequests = new LinkedList<OFSwitchImpl.PendingRoleRequestEntry>();
         
         // Defaults properties for an ideal switch
         this.setAttribute(PROP_FASTWILDCARDS, (Integer) OFMatch.OFPFW_ALL);
@@ -148,6 +179,7 @@ public class OFSwitchImpl implements IOFSwitch {
         this.channel = channel;
     }
     
+    // TODO: document the difference between the different write functions
     public void write(OFMessage m, FloodlightContext bc) throws IOException {
         Map<OFSwitchImpl,List<OFMessage>> msg_buffer_map = local_msg_buffer.get();
         List<OFMessage> msg_buffer = msg_buffer_map.get(this);
@@ -373,17 +405,12 @@ public class OFSwitchImpl implements IOFSwitch {
     }
     
     @Override
-    public synchronized Role getRole() {
+    public Role getRole() {
         return role;
     }
     
     @Override
-    public synchronized void setRole(Role role) {
-        this.role = role;
-    }
-    
-    @Override
-    public synchronized boolean isActive() {
+    public boolean isActive() {
         return (role != Role.SLAVE);
     }
     
@@ -473,4 +500,166 @@ public class OFSwitchImpl implements IOFSwitch {
     public Lock getListenerWriteLock() {
         return listenerLock.writeLock();
     }
+    
+    /**
+     * Send NX role request message to the switch requesting the specified role.
+     * 
+     * This method should ONLY be called by @see RoleChanger.submitRequest(). 
+     * 
+     * After sending the request add it to the queue of pending request. We
+     * use the queue to later verify that we indeed receive the correct reply.
+     * @param sw switch to send the role request message to
+     * @param role role to request
+     * @param cookie an opaque value that will be stored in the pending queue so
+     *        RoleChanger can check for timeouts.
+     * @return transaction id of the role request message that was sent
+     */
+    protected int sendNxRoleRequest(Role role, long cookie)
+            throws IOException {
+        synchronized(pendingRoleRequests) {
+            // Convert the role enum to the appropriate integer constant used
+            // in the NX role request message
+            int nxRole = 0;
+            switch (role) {
+                case EQUAL:
+                    nxRole = OFRoleVendorData.NX_ROLE_OTHER;
+                    break;
+                case MASTER:
+                    nxRole = OFRoleVendorData.NX_ROLE_MASTER;
+                    break;
+                case SLAVE:
+                    nxRole = OFRoleVendorData.NX_ROLE_SLAVE;
+                    break;
+                default:
+                    log.error("Invalid Role specified for switch {}."
+                              + " Disconnecting.", this);
+                    // TODO: should throw an error
+                    return 0;
+            }
+            
+            // Construct the role request message
+            OFVendor roleRequest = (OFVendor)floodlightProvider.
+                    getOFMessageFactory().getMessage(OFType.VENDOR);
+            int xid = this.getNextTransactionId();
+            roleRequest.setXid(xid);
+            roleRequest.setVendor(OFNiciraVendorData.NX_VENDOR_ID);
+            OFRoleRequestVendorData roleRequestData = new OFRoleRequestVendorData();
+            roleRequestData.setRole(nxRole);
+            roleRequest.setVendorData(roleRequestData);
+            roleRequest.setLengthU(OFVendor.MINIMUM_LENGTH + 
+                                   roleRequestData.getLength());
+            
+            // Send it to the switch
+            List<OFMessage> msglist = new ArrayList<OFMessage>(1);
+            msglist.add(roleRequest);
+            // FIXME: should this use this.write() in order for messages to
+            // be processed by handleOutgoingMessage()
+            this.channel.write(msglist);
+            
+            pendingRoleRequests.add(new PendingRoleRequestEntry(xid, role, cookie));
+            return xid;
+        }
+    }
+    
+    /** 
+     * Deliver a RoleReply message to this switch. Checks if the reply 
+     * message matches the expected reply (head of the pending request queue). 
+     * We require in-order delivery of replies. If there's any deviation from
+     * our expectations we disconnect the switch. 
+     * 
+     * We must not check the received role against the controller's current
+     * role because there's no synchronization but that's fine @see RoleChanger
+     * 
+     * Will be called by the OFChannelHandler's receive loop
+     * 
+     * @param xid Xid of the reply message
+     * @param role The Role in the the reply message
+     */
+    protected void deliverRoleReply(int xid, Role role) {
+        synchronized(pendingRoleRequests) {
+            PendingRoleRequestEntry head = pendingRoleRequests.poll();
+            if (head == null) {
+                // Maybe don't disconnect if the role reply we received is 
+                // for the same role we are already in. 
+                log.error("Switch {}: received unexpected role reply for Role {}" + 
+                          " Disconnecting switch", this, role );
+                this.channel.close();
+            }
+            else if (head.xid != xid) {
+                // check xid before role!!
+                log.error("Switch {}: expected role reply with " +
+                       "Xid {}, got {}. Disconnecting switch",
+                       new Object[] { this, head.xid, xid } );
+                this.channel.close();
+            }
+            else if (head.role != role) {
+                log.error("Switch {}: expected role reply with " +
+                       "Role {}, got {}. Disconnecting switch",
+                       new Object[] { this, head.role, role } );
+                this.channel.close();
+            }
+            else {
+                log.debug("Received role reply message from {}, setting role to {}",
+                          this, role);
+                if (this.role == null && getAttribute(SWITCH_SUPPORTS_NX_ROLE) == null) {
+                    // The first role reply we received. Set the attribute
+                    // that the switch supports roles
+                    setAttribute(SWITCH_SUPPORTS_NX_ROLE, true);
+                }
+                this.role = role;
+            }
+        }
+    }
+    
+    /** 
+     * Checks whether the given xid matches the xid of the first pending
+     * role request. 
+     * @param xid
+     * @return 
+     */
+    protected boolean checkFirstPendingRoleRequestXid (int xid) {
+        synchronized(pendingRoleRequests) {
+            PendingRoleRequestEntry head = pendingRoleRequests.peek();
+            if (head == null)
+                return false;
+            else 
+                return head.xid == xid;
+        }
+    }
+    
+    /**
+     * Checks whether the given request cookie matches the cookie of the first 
+     * pending request
+     * @param cookie
+     * @return
+     */
+    protected boolean checkFirstPendingRoleRequestCookie(long cookie) {
+        synchronized(pendingRoleRequests) {
+            PendingRoleRequestEntry head = pendingRoleRequests.peek();
+            if (head == null)
+                return false;
+            else 
+                return head.cookie == cookie;
+        }
+    }
+    
+    /**
+     * Called if we receive a vendor error message indicating that roles
+     * are not supported by the switch. If the xid matches the first pending
+     * one, we'll mark the switch as not supporting roles and remove the head.
+     * Otherwise we ignore it.
+     * @param xid
+     */
+    protected void deliverRoleRequestNotSupported(int xid) {
+        synchronized(pendingRoleRequests) {
+            PendingRoleRequestEntry head = pendingRoleRequests.poll();
+            this.role = null;
+            if (head!=null && head.xid == xid) {
+                setAttribute(SWITCH_SUPPORTS_NX_ROLE, false);
+            }
+            else {
+                this.channel.close();
+            }
+        }
+    }
 }

src/main/java/net/floodlightcontroller/core/internal/RoleChanger.java

@@ -293,7 +293,7 @@ public class RoleChanger {
     protected void verifyRoleReplyReceived(Collection<OFSwitchImpl> switches,
                                    long cookie) {
         for (OFSwitchImpl sw: switches) {
-            if (sw.checkFirstPendingRoleReqeustCookie(cookie)) {
+            if (sw.checkFirstPendingRoleRequestCookie(cookie)) {
                 sw.getChannel().close();
                 log.warn("Timeout while waiting for role reply from switch {}."
                          + " Disconnecting", sw);

src/test/java/net/floodlightcontroller/core/internal/RoleChangerTest.java

@@ -140,13 +140,13 @@ public class RoleChangerTest {
         
         // Add a switch that has received a role reply
         OFSwitchImpl sw1 = EasyMock.createMock(OFSwitchImpl.class);
-        expect(sw1.checkFirstPendingRoleReqeustCookie(123456))
+        expect(sw1.checkFirstPendingRoleRequestCookie(123456))
                         .andReturn(false).once();
         switches.add(sw1);
         
         // Add a switch that has not yet received a role reply
         OFSwitchImpl sw2 = EasyMock.createMock(OFSwitchImpl.class);
-        expect(sw2.checkFirstPendingRoleReqeustCookie(123456))
+        expect(sw2.checkFirstPendingRoleRequestCookie(123456))
                         .andReturn(true).once();
         Channel channel2 = createMock(Channel.class);
         expect(sw2.getChannel()).andReturn(channel2);
@@ -201,9 +201,9 @@ public class RoleChangerTest {
         expect(sw1.sendNxRoleRequest(EasyMock.same(Role.SLAVE), EasyMock.anyLong()))
                        .andReturn(1);
         // The following calls happen for timeout handling:
-        expect(sw1.checkFirstPendingRoleReqeustCookie(EasyMock.anyLong()))
+        expect(sw1.checkFirstPendingRoleRequestCookie(EasyMock.anyLong()))
                         .andReturn(false);
-        expect(sw1.checkFirstPendingRoleReqeustCookie(EasyMock.anyLong()))
+        expect(sw1.checkFirstPendingRoleRequestCookie(EasyMock.anyLong()))
                         .andReturn(false);
         switches.add(sw1);