refactored RoleManager to handle multiple pending requests

Change-Id: I669e0527107c5bd29c1600f4667424bc4e6f6b7e

refactored RoleManager to handle multiple pending requests
Change-Id: I669e0527107c5bd29c1600f4667424bc4e6f6b7e
Ayaka Koshibe
Commit 00ae86304fbb6e4d043feaad9653e45c26f4a778 00ae8630 1 parent d0e32d74
Showing 2 changed files with 44 additions and 43 deletions
openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java
openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java
--- a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java
View file @00ae863
+++ b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java
View file @00ae863
@@ -300,7 +300,6 @@ public abstract class AbstractOpenFlowSwitch implements OpenFlowSwitchDriver {
                 this.transitionToEqualSwitch();
             }
         } else {
-            log.warn(">>> mismatch with expected role - got {} - Disconnecting", r);
             this.disconnectSwitch();
         }
     }
--- a/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java
View file @00ae863
+++ b/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java
View file @00ae863
@@ -17,6 +17,7 @@ package org.onlab.onos.openflow.controller.impl;
 import java.io.IOException;
 import java.util.Collections;
+import java.util.concurrent.TimeUnit;
 import org.onlab.onos.openflow.controller.RoleState;
 import org.onlab.onos.openflow.controller.driver.OpenFlowSwitchDriver;
@@ -41,6 +42,9 @@ import org.projectfloodlight.openflow.types.U64;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+
 /**
  * A utility class to handle role requests and replies for this channel.
@@ -59,13 +63,15 @@ class RoleManager implements RoleHandler {
     protected static final long NICIRA_EXPERIMENTER = 0x2320;
     private static Logger log = LoggerFactory.getLogger(RoleManager.class);
-    // indicates that a request is currently pending
+
-    // needs to be volatile to allow correct double-check idiom
+    // The time until cached XID is evicted. Arbitray for now.
-    private volatile boolean requestPending;
+    private final int pendingXidTimeoutSeconds = 60;
-    // the transaction Id of the pending request
+
-    private int pendingXid;
+    // The cache for pending expected RoleReplies keyed on expected XID
-    // the role that's pending
+    private Cache<Integer, RoleState> pendingReplies =
-    private RoleState pendingRole;
+            CacheBuilder.newBuilder()
+                .expireAfterWrite(pendingXidTimeoutSeconds, TimeUnit.SECONDS)
+                .build();
     // the expectation set by the caller for the returned role
     private RoleRecvStatus expectation;
@@ -73,9 +79,6 @@ class RoleManager implements RoleHandler {
     public RoleManager(OpenFlowSwitchDriver sw) {
-        this.requestPending = false;
-        this.pendingXid = -1;
-        this.pendingRole = null;
         this.expectation = RoleRecvStatus.MATCHED_CURRENT_ROLE;
         this.sw = sw;
     }
@@ -157,15 +160,11 @@ class RoleManager implements RoleHandler {
             }
             // OF1.0 switch with support for NX_ROLE_REQUEST vendor extn.
             // make Role.EQUAL become Role.SLAVE
-            pendingRole = role;
+            RoleState roleToSend = (role == RoleState.EQUAL) ? RoleState.SLAVE : role;
-            role = (role == RoleState.EQUAL) ? RoleState.SLAVE : role;
+            pendingReplies.put(sendNxRoleRequest(roleToSend), role);
-            pendingXid = sendNxRoleRequest(role);
-            requestPending = true;
         } else {
             // OF1.3 switch, use OFPT_ROLE_REQUEST message
-            pendingXid = sendOF13RoleRequest(role);
+            pendingReplies.put(sendOF13RoleRequest(role), role);
-            pendingRole = role;
-            requestPending = true;
         }
         return true;
     }
@@ -192,12 +191,17 @@ class RoleManager implements RoleHandler {
     @Override
     public synchronized RoleRecvStatus deliverRoleReply(RoleReplyInfo rri)
             throws SwitchStateException {
-        if (!requestPending) {
+        int xid = (int) rri.getXid();
+        RoleState receivedRole = rri.getRole();
+        RoleState expectedRole = pendingReplies.getIfPresent(xid);
+
+        if (expectedRole == null) {
             RoleState currentRole = (sw != null) ? sw.getRole() : null;
             if (currentRole != null) {
                 if (currentRole == rri.getRole()) {
                     // Don't disconnect if the role reply we received is
                     // for the same role we are already in.
+                    // FIXME: but we do from the caller anyways.
                     log.debug("Received unexpected RoleReply from "
                             + "Switch: {}. "
                             + "Role in reply is same as current role of this "
@@ -223,26 +227,24 @@ class RoleManager implements RoleHandler {
             return RoleRecvStatus.OTHER_EXPECTATION;
         }
-        int xid = (int) rri.getXid();
-        RoleState receivedRole = rri.getRole();
         // XXX Should check generation id meaningfully and other cases of expectations
-
+        //if (pendingXid != xid) {
-        if (pendingXid != xid) {
+        //    log.info("Received older role reply from " +
-            log.debug("Received older role reply from " +
+        //            "switch {} ({}). Ignoring. " +
-                    "switch {} ({}). Ignoring. " +
+        //            "Waiting for {}, xid={}",
-                    "Waiting for {}, xid={}",
+        //            new Object[] {sw.getStringId(), rri,
-                    new Object[] {sw.getStringId(), rri,
+        //            pendingRole, pendingXid });
-                    pendingRole, pendingXid });
+        //    return RoleRecvStatus.OLD_REPLY;
-            return RoleRecvStatus.OLD_REPLY;
+        //}
-        }
+        sw.returnRoleReply(expectedRole, receivedRole);
-
+
-        sw.returnRoleReply(pendingRole, receivedRole);
+        if (expectedRole == receivedRole) {
-
-        if (pendingRole == receivedRole) {
             log.debug("Received role reply message from {} that matched "
                     + "expected role-reply {} with expectations {}",
                     new Object[] {sw.getStringId(), receivedRole, expectation});
+            // Done with this RoleReply; Invalidate
+            pendingReplies.invalidate(xid);
             if (expectation == RoleRecvStatus.MATCHED_CURRENT_ROLE ||
                     expectation == RoleRecvStatus.MATCHED_SET_ROLE) {
                 return expectation;
@@ -251,6 +253,7 @@ class RoleManager implements RoleHandler {
             }
         }
+        pendingReplies.invalidate(xid);
         // if xids match but role's don't, perhaps its a query (OF1.3)
         if (expectation == RoleRecvStatus.REPLY_QUERY) {
             return expectation;
@@ -270,18 +273,17 @@ class RoleManager implements RoleHandler {
     @Override
     public synchronized RoleRecvStatus deliverError(OFErrorMsg error)
             throws SwitchStateException {
-        if (!requestPending) {
+        RoleState errorRole = pendingReplies.getIfPresent(error.getXid());
-            log.debug("Received an error msg from sw {}, but no pending "
+        if (errorRole == null) {
-                    + "requests in role-changer; not handling ...",
-                    sw.getStringId());
-            return RoleRecvStatus.OTHER_EXPECTATION;
-        }
-        if (pendingXid != error.getXid()) {
             if (error.getErrType() == OFErrorType.ROLE_REQUEST_FAILED) {
                 log.debug("Received an error msg from sw {} for a role request,"
                         + " but not for pending request in role-changer; "
                         + " ignoring error {} ...",
                         sw.getStringId(), error);
+            } else {
+                log.debug("Received an error msg from sw {}, but no pending "
+                        + "requests in role-changer; not handling ...",
+                        sw.getStringId());
             }
             return RoleRecvStatus.OTHER_EXPECTATION;
         }
@@ -292,7 +294,7 @@ class RoleManager implements RoleHandler {
                     + "role-messaging is supported. Possible issues in "
                     + "switch driver configuration?", new Object[] {
                             ((OFBadRequestErrorMsg) error).toString(),
-                            sw.getStringId(), pendingRole
+                            sw.getStringId(), errorRole
                     });
             return RoleRecvStatus.UNSUPPORTED;
         }
@@ -316,7 +318,7 @@ class RoleManager implements RoleHandler {
                         + "received Error to for pending role request [%s]. "
                         + "Error:[%s]. Disconnecting switch ... ",
                         sw.getStringId(),
-                        pendingRole, rrerr);
+                        errorRole, rrerr);
                 throw new SwitchStateException(msgx);
             default:
                 break;