vertical feedback path for Role replies
Change-Id: I31bdb85f90901ec79147adeea0df8ceae00ed1dc
diff --git a/core/api/src/main/java/org/onlab/onos/net/device/DeviceProviderService.java b/core/api/src/main/java/org/onlab/onos/net/device/DeviceProviderService.java
index f1667db..6f71495 100644
--- a/core/api/src/main/java/org/onlab/onos/net/device/DeviceProviderService.java
+++ b/core/api/src/main/java/org/onlab/onos/net/device/DeviceProviderService.java
@@ -61,12 +61,12 @@
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);
}
diff --git a/core/net/src/main/java/org/onlab/onos/net/device/impl/DeviceManager.java b/core/net/src/main/java/org/onlab/onos/net/device/impl/DeviceManager.java
index dfff752..740a110 100644
--- a/core/net/src/main/java/org/onlab/onos/net/device/impl/DeviceManager.java
+++ b/core/net/src/main/java/org/onlab/onos/net/device/impl/DeviceManager.java
@@ -367,16 +367,48 @@
}
@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));
+ }
+ }
+
}
}
diff --git a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitch.java b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitch.java
index 17f168f..77eb437 100644
--- a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitch.java
+++ b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitch.java
@@ -121,11 +121,12 @@
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.
diff --git a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitchListener.java b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitchListener.java
index a96c56f..192f045 100644
--- a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitchListener.java
+++ b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/OpenFlowSwitchListener.java
@@ -53,5 +53,5 @@
* @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);
}
diff --git a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java
index 9950515..49943b8 100644
--- a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java
+++ b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/AbstractOpenFlowSwitch.java
@@ -217,8 +217,8 @@
}
@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 @@
this.transitionToEqualSwitch();
}
} else {
+ log.warn(">>> mismatch with expected role - got {} - Disconnecting", r);
this.disconnectSwitch();
}
}
diff --git a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/OpenFlowAgent.java b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/OpenFlowAgent.java
index fa2823f..6b73efc 100644
--- a/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/OpenFlowAgent.java
+++ b/openflow/api/src/main/java/org/onlab/onos/openflow/controller/driver/OpenFlowAgent.java
@@ -97,5 +97,5 @@
* @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);
}
diff --git a/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/OpenFlowControllerImpl.java b/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/OpenFlowControllerImpl.java
index 70c9b1b..098771d 100644
--- a/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/OpenFlowControllerImpl.java
+++ b/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/OpenFlowControllerImpl.java
@@ -374,9 +374,9 @@
}
@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);
}
}
}
diff --git a/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java b/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java
index c9f71b7..a22aac0 100644
--- a/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java
+++ b/openflow/ctl/src/main/java/org/onlab/onos/openflow/controller/impl/RoleManager.java
@@ -224,8 +224,8 @@
}
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 @@
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 @@
} else {
return RoleRecvStatus.OTHER_EXPECTATION;
}
- } else {
- sw.returnRoleAssertFailure(pendingRole);
}
// if xids match but role's don't, perhaps its a query (OF1.3)
diff --git a/openflow/ctl/src/test/java/org/onlab/onos/openflow/controller/impl/RoleManagerTest.java b/openflow/ctl/src/test/java/org/onlab/onos/openflow/controller/impl/RoleManagerTest.java
index aa23995..7260ddd 100644
--- a/openflow/ctl/src/test/java/org/onlab/onos/openflow/controller/impl/RoleManagerTest.java
+++ b/openflow/ctl/src/test/java/org/onlab/onos/openflow/controller/impl/RoleManagerTest.java
@@ -175,11 +175,6 @@
}
@Override
- public void returnRoleAssertFailure(RoleState role) {
- failed = role;
- }
-
- @Override
public boolean isOptical() {
return false;
}
@@ -300,5 +295,10 @@
public void write(List<OFMessage> msgs) {
}
+ @Override
+ public void returnRoleReply(RoleState requested, RoleState response) {
+ failed = requested;
+ }
+
}
}
diff --git a/providers/openflow/device/src/main/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProvider.java b/providers/openflow/device/src/main/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProvider.java
index 8aa3d77..1a96fca 100644
--- a/providers/openflow/device/src/main/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProvider.java
+++ b/providers/openflow/device/src/main/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProvider.java
@@ -226,23 +226,31 @@
}
@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);
}
/**
diff --git a/providers/openflow/device/src/test/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProviderTest.java b/providers/openflow/device/src/test/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProviderTest.java
index e7e57b2..76d32e4 100644
--- a/providers/openflow/device/src/test/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProviderTest.java
+++ b/providers/openflow/device/src/test/java/org/onlab/onos/provider/of/device/impl/OpenFlowDeviceProviderTest.java
@@ -136,12 +136,13 @@
}
@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 @@
}
@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 @@
}
@Override
- public void returnRoleAssertFailure(RoleState role) {
+ public boolean isOptical() {
+ return false;
}
@Override
- public boolean isOptical() {
- return false;
+ public void returnRoleReply(RoleState requested, RoleState reponse) {
}
}
diff --git a/providers/openflow/flow/src/main/java/org/onlab/onos/provider/of/flow/impl/OpenFlowRuleProvider.java b/providers/openflow/flow/src/main/java/org/onlab/onos/provider/of/flow/impl/OpenFlowRuleProvider.java
index 53a49b6..018d6f3 100644
--- a/providers/openflow/flow/src/main/java/org/onlab/onos/provider/of/flow/impl/OpenFlowRuleProvider.java
+++ b/providers/openflow/flow/src/main/java/org/onlab/onos/provider/of/flow/impl/OpenFlowRuleProvider.java
@@ -302,7 +302,10 @@
}
@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) {
diff --git a/providers/openflow/link/src/main/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProvider.java b/providers/openflow/link/src/main/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProvider.java
index 02ba907..f32ca55 100644
--- a/providers/openflow/link/src/main/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProvider.java
+++ b/providers/openflow/link/src/main/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProvider.java
@@ -160,7 +160,8 @@
}
@Override
- public void roleAssertFailed(Dpid dpid, RoleState role) {
+ public void receivedRoleReply(Dpid dpid, RoleState requested,
+ RoleState response) {
// do nothing for this.
}
diff --git a/providers/openflow/link/src/test/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProviderTest.java b/providers/openflow/link/src/test/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProviderTest.java
index 32339f2..4109ba0 100644
--- a/providers/openflow/link/src/test/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProviderTest.java
+++ b/providers/openflow/link/src/test/java/org/onlab/onos/provider/of/link/impl/OpenFlowLinkProviderTest.java
@@ -479,12 +479,12 @@
}
@Override
- public void returnRoleAssertFailure(RoleState role) {
+ public boolean isOptical() {
+ return false;
}
@Override
- public boolean isOptical() {
- return false;
+ public void returnRoleReply(RoleState requested, RoleState reponse) {
}
}
diff --git a/providers/openflow/packet/src/test/java/org/onlab/onos/provider/of/packet/impl/OpenFlowPacketProviderTest.java b/providers/openflow/packet/src/test/java/org/onlab/onos/provider/of/packet/impl/OpenFlowPacketProviderTest.java
index ed682d6..f547bbe 100644
--- a/providers/openflow/packet/src/test/java/org/onlab/onos/provider/of/packet/impl/OpenFlowPacketProviderTest.java
+++ b/providers/openflow/packet/src/test/java/org/onlab/onos/provider/of/packet/impl/OpenFlowPacketProviderTest.java
@@ -410,12 +410,12 @@
}
@Override
- public void returnRoleAssertFailure(RoleState role) {
+ public boolean isOptical() {
+ return false;
}
@Override
- public boolean isOptical() {
- return false;
+ public void returnRoleReply(RoleState requested, RoleState reponse) {
}
}