vertical feedback path for Role replies

Change-Id: I31bdb85f90901ec79147adeea0df8ceae00ed1dc

@@ -61,12 +61,12 @@ public interface DeviceProviderService extends ProviderService<DeviceProvider> {
     void portStatusChanged(DeviceId deviceId, PortDescription portDescription);
 
     /**
-     * Notifies the core about the providers inability to assert the specified
-     * mastership role on the device.
+     * Notifies the core about the result of a RoleRequest sent to a device.
      *
      * @param deviceId identity of the device
-     * @param role mastership role that was asserted but failed
+     * @param requested mastership role that was requested by the node
+     * @param replied mastership role the switch accepted
      */
-    void unableToAssertRole(DeviceId deviceId, MastershipRole role);
+    void receivedRoleReply(DeviceId deviceId, MastershipRole requested, MastershipRole response);
 
 }

@@ -367,16 +367,48 @@ public class DeviceManager
         }
 
         @Override
-        public void unableToAssertRole(DeviceId deviceId, MastershipRole role) {
+        public void receivedRoleReply(
+                DeviceId deviceId, MastershipRole requested, MastershipRole response) {
+            // Several things can happen here:
+            // 1. request and response match
+            // 2. request and response don't match
+            // 3. MastershipRole and requested match (and 1 or 2 are true)
+            // 4. MastershipRole and requested don't match (and 1 or 2 are true)
+            //
+            // 2, 4, and 3 with case 2 are failure modes.
+
             // FIXME: implement response to this notification
-            log.warn("Failed to assert role [{}] onto Device {}", role,
-                     deviceId);
-            if (role == MastershipRole.MASTER) {
+
+            log.info("got reply to a role request for {}: asked for {}, and got {}",
+                    deviceId, requested, response);
+
+            if (requested == null && response == null) {
+                // something was off with DeviceProvider, maybe check channel too?
+                log.warn("Failed to assert role [{}] onto Device {}", requested, deviceId);
                 mastershipService.relinquishMastership(deviceId);
-                // TODO: Shouldn't we be triggering event?
-                //final Device device = getDevice(deviceId);
-                //post(new DeviceEvent(DEVICE_MASTERSHIP_CHANGED, device));
+                return;
+            }
+
+            if (requested.equals(response)) {
+                if (requested.equals(mastershipService.getLocalRole(deviceId))) {
+
+                    return;
+                } else {
+                    return;
+                    // FIXME roleManager got the device to comply, but doesn't agree with
+                    // the store; use the store's view, then try to reassert.
+                }
+            } else {
+                // we didn't get back what we asked for. Reelect someone else.
+                log.warn("Failed to assert role [{}] onto Device {}", response, deviceId);
+                if (response == MastershipRole.MASTER) {
+                    mastershipService.relinquishMastership(deviceId);
+                    // TODO: Shouldn't we be triggering event?
+                    //final Device device = getDevice(deviceId);
+                    //post(new DeviceEvent(DEVICE_MASTERSHIP_CHANGED, device));
+                }
             }
+
         }
     }
 

@@ -121,11 +121,12 @@ public interface OpenFlowSwitch {
     public void disconnectSwitch();
 
     /**
-     * Notifies the controller that role assertion has failed.
+     * Notifies the controller that the device has responded to a set-role request.
      *
-     * @param role the failed role
+     * @param requested the role requested by the controller
+     * @param response the role set at the device
      */
-    public void returnRoleAssertFailure(RoleState role);
+    public void returnRoleReply(RoleState requested, RoleState reponse);
 
     /**
      * Indicates if this switch is optical.

@@ -53,5 +53,5 @@ public interface OpenFlowSwitchListener {
      * @param dpid the switch that failed role assertion
      * @param role the role imposed by the controller
      */
-    public void roleAssertFailed(Dpid dpid, RoleState role);
+    public void receivedRoleReply(Dpid dpid, RoleState requested, RoleState response);
 }

@@ -217,8 +217,8 @@ public abstract class AbstractOpenFlowSwitch implements OpenFlowSwitchDriver {
     }
 
     @Override
-    public void returnRoleAssertFailure(RoleState role) {
-        this.agent.returnRoleAssertFailed(dpid, role);
+    public void returnRoleReply(RoleState requested, RoleState response) {
+        this.agent.returnRoleReply(dpid, requested, response);
     }
 
     @Override
@@ -300,6 +300,7 @@ public abstract class AbstractOpenFlowSwitch implements OpenFlowSwitchDriver {
                 this.transitionToEqualSwitch();
             }
         } else {
+            log.warn(">>> mismatch with expected role - got {} - Disconnecting", r);
             this.disconnectSwitch();
         }
     }

@@ -97,5 +97,5 @@ public interface OpenFlowAgent {
      * @param dpid the switch that failed role assertion
      * @param role the failed role
      */
-    public void returnRoleAssertFailed(Dpid dpid, RoleState role);
+    public void returnRoleReply(Dpid dpid, RoleState requested, RoleState response);
 }

@@ -374,9 +374,9 @@ public class OpenFlowControllerImpl implements OpenFlowController {
         }
 
         @Override
-        public void returnRoleAssertFailed(Dpid dpid, RoleState role) {
+        public void returnRoleReply(Dpid dpid, RoleState requested, RoleState response) {
             for (OpenFlowSwitchListener l : ofSwitchListener) {
-                l.roleAssertFailed(dpid, role);
+                l.receivedRoleReply(dpid, requested, response);
             }
         }
     }

@@ -224,8 +224,8 @@ class RoleManager implements RoleHandler {
         }
 
         int xid = (int) rri.getXid();
-        RoleState role = rri.getRole();
-        // XXX S should check generation id meaningfully and other cases of expectations
+        RoleState receivedRole = rri.getRole();
+        // XXX Should check generation id meaningfully and other cases of expectations
 
         if (pendingXid != xid) {
             log.debug("Received older role reply from " +
@@ -236,10 +236,12 @@ class RoleManager implements RoleHandler {
             return RoleRecvStatus.OLD_REPLY;
         }
 
-        if (pendingRole == role) {
+        sw.returnRoleReply(pendingRole, receivedRole);
+
+        if (pendingRole == receivedRole) {
             log.debug("Received role reply message from {} that matched "
                     + "expected role-reply {} with expectations {}",
-                    new Object[] {sw.getStringId(), role, expectation});
+                    new Object[] {sw.getStringId(), receivedRole, expectation});
 
             if (expectation == RoleRecvStatus.MATCHED_CURRENT_ROLE ||
                     expectation == RoleRecvStatus.MATCHED_SET_ROLE) {
@@ -247,8 +249,6 @@ class RoleManager implements RoleHandler {
             } else {
                 return RoleRecvStatus.OTHER_EXPECTATION;
             }
-        } else {
-            sw.returnRoleAssertFailure(pendingRole);
         }
 
         // if xids match but role's don't, perhaps its a query (OF1.3)

@@ -175,11 +175,6 @@ public class RoleManagerTest {
         }
 
         @Override
-        public void returnRoleAssertFailure(RoleState role) {
-            failed = role;
-        }
-
-        @Override
         public boolean isOptical() {
             return false;
         }
@@ -300,5 +295,10 @@ public class RoleManagerTest {
         public void write(List<OFMessage> msgs) {
         }
 
+        @Override
+        public void returnRoleReply(RoleState requested, RoleState response) {
+            failed = requested;
+        }
+
     }
 }

@@ -226,23 +226,31 @@ public class OpenFlowDeviceProvider extends AbstractProvider implements DevicePr
         }
 
         @Override
-        public void roleAssertFailed(Dpid dpid, RoleState role) {
-            MastershipRole failed;
-            switch (role) {
+        public void receivedRoleReply(Dpid dpid, RoleState requested, RoleState response) {
+            MastershipRole request = roleOf(requested);
+            MastershipRole reply = roleOf(response);
+
+            providerService.receivedRoleReply(deviceId(uri(dpid)), request, reply);
+        }
+
+        /**
+         * Translates a RoleState to the corresponding MastershipRole.
+         *
+         * @param response
+         * @return a MastershipRole
+         */
+        private MastershipRole roleOf(RoleState response) {
+            switch (response) {
                 case MASTER:
-                    failed = MastershipRole.MASTER;
-                    break;
+                    return MastershipRole.MASTER;
                 case EQUAL:
-                    failed = MastershipRole.STANDBY;
-                    break;
+                    return MastershipRole.STANDBY;
                 case SLAVE:
-                    failed = MastershipRole.NONE;
-                    break;
+                    return MastershipRole.NONE;
                 default:
-                    LOG.warn("unknown role {}", role);
-                    return;
+                    LOG.warn("unknown role {}", response);
+                    return null;
             }
-            providerService.unableToAssertRole(deviceId(uri(dpid)), failed);
         }
 
         /**

@@ -136,12 +136,13 @@ public class OpenFlowDeviceProviderTest {
     }
 
     @Test
-    public void roleAssertFailed() {
-        controller.listener.roleAssertFailed(DPID1, RoleState.MASTER);
+    public void receivedRoleReply() {
+        // check translation capabilities
+        controller.listener.receivedRoleReply(DPID1, RoleState.MASTER, RoleState.MASTER);
         assertEquals("wrong role reported", DPID1, registry.roles.get(MASTER));
-        controller.listener.roleAssertFailed(DPID1, RoleState.EQUAL);
+        controller.listener.receivedRoleReply(DPID1, RoleState.EQUAL, RoleState.MASTER);
         assertEquals("wrong role reported", DPID1, registry.roles.get(STANDBY));
-        controller.listener.roleAssertFailed(DPID1, RoleState.SLAVE);
+        controller.listener.receivedRoleReply(DPID1, RoleState.SLAVE, RoleState.MASTER);
         assertEquals("wrong role reported", DPID1, registry.roles.get(NONE));
     }
 
@@ -210,8 +211,9 @@ public class OpenFlowDeviceProviderTest {
             }
 
             @Override
-            public void unableToAssertRole(DeviceId deviceId, MastershipRole role) {
-                roles.put(role, Dpid.dpid(deviceId.uri()));
+            public void receivedRoleReply(DeviceId deviceId,
+                    MastershipRole requested, MastershipRole response) {
+                roles.put(requested, Dpid.dpid(deviceId.uri()));
             }
 
         }
@@ -372,12 +374,12 @@ public class OpenFlowDeviceProviderTest {
         }
 
         @Override
-        public void returnRoleAssertFailure(RoleState role) {
+        public boolean isOptical() {
+            return false;
         }
 
         @Override
-        public boolean isOptical() {
-            return false;
+        public void returnRoleReply(RoleState requested, RoleState reponse) {
         }
 
     }

@@ -302,7 +302,10 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
         }
 
         @Override
-        public void roleAssertFailed(Dpid dpid, RoleState role) {}
+        public void receivedRoleReply(Dpid dpid, RoleState requested,
+                RoleState response) {
+            // Do nothing here for now.
+        }
 
         private synchronized void pushFlowMetrics(Dpid dpid, OFStatsReply stats) {
             if (stats.getStatsType() != OFStatsType.FLOW) {

@@ -160,7 +160,8 @@ public class OpenFlowLinkProvider extends AbstractProvider implements LinkProvid
         }
 
         @Override
-        public void roleAssertFailed(Dpid dpid, RoleState role) {
+        public void receivedRoleReply(Dpid dpid, RoleState requested,
+                RoleState response) {
             // do nothing for this.
         }
 

@@ -479,12 +479,12 @@ public class OpenFlowLinkProviderTest {
         }
 
         @Override
-        public void returnRoleAssertFailure(RoleState role) {
+        public boolean isOptical() {
+            return false;
         }
 
         @Override
-        public boolean isOptical() {
-            return false;
+        public void returnRoleReply(RoleState requested, RoleState reponse) {
         }
 
     }

@@ -410,12 +410,12 @@ public class OpenFlowPacketProviderTest {
         }
 
         @Override
-        public void returnRoleAssertFailure(RoleState role) {
+        public boolean isOptical() {
+            return false;
         }
 
         @Override
-        public boolean isOptical() {
-            return false;
+        public void returnRoleReply(RoleState requested, RoleState reponse) {
         }
 
     }