fixes related to getRole() assumptions
Change-Id: Icf19d95714dc217200eed021a495d9a78440ca8e
diff --git a/core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java b/core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java
index dcb2d95..fdbb645 100644
--- a/core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java
+++ b/core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java
@@ -1,6 +1,5 @@
package org.onlab.onos.mastership;
-import org.onlab.onos.cluster.NodeId;
import org.onlab.onos.cluster.RoleInfo;
import org.onlab.onos.event.AbstractEvent;
import org.onlab.onos.net.DeviceId;
@@ -56,19 +55,6 @@
}
/**
- * Returns the NodeID of the node associated with the event.
- * For MASTER_CHANGED this is the newly elected master, and for
- * BACKUPS_CHANGED, this is the node that was newly added, removed, or
- * whose position was changed in the list.
- *
- * @return node ID as a subject
- */
- //XXX to-be removed - or keep for convenience?
- public NodeId node() {
- return roleInfo.master();
- }
-
- /**
* Returns the current role state for the subject.
*
* @return RoleInfo associated with Device ID subject
diff --git a/core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java b/core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java
index b2c172a..1a671f6 100644
--- a/core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java
+++ b/core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java
@@ -227,10 +227,14 @@
if (clusterService.getNodes().size() > (clusterSize.intValue() / 2)) {
return true;
}
- //else {
+// else {
//FIXME: break tie for equal-sized clusters, by number of
// connected switches, then masters, then nodeId hash
- // }
+ // problem is, how do we get at channel info cleanly here?
+ // Also, what's the time hit for a distributed store look-up
+ // versus channel re-negotiation? bet on the latter being worse.
+
+// }
return false;
}
diff --git a/core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java b/core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java
index 5e1457a..f03600d 100644
--- a/core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java
+++ b/core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java
@@ -86,7 +86,7 @@
final List<NodeId> standbyList = Collections.<NodeId>emptyList();
eventDispatcher.post(new ReplicaInfoEvent(MASTER_CHANGED,
event.subject(),
- new ReplicaInfo(event.node(), standbyList)));
+ new ReplicaInfo(event.roleInfo().master(), standbyList)));
}
}
diff --git a/core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java b/core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
index 74ca8cd..316a3b4 100644
--- a/core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
+++ b/core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
@@ -91,23 +91,14 @@
@Override
public MastershipRole getRole(NodeId nodeId, DeviceId deviceId) {
- NodeId current = getNode(MASTER, deviceId);
- if (current == null) {
- if (isRole(STANDBY, nodeId, deviceId)) {
- //was previously standby, or set to standby from master
- return MastershipRole.STANDBY;
- } else {
- return MastershipRole.NONE;
- }
- } else {
- if (current.equals(nodeId)) {
- //*should* be in unusable, not always
- return MastershipRole.MASTER;
- } else {
- //may be in backups or unusable from earlier retirement
- return MastershipRole.STANDBY;
- }
+ final RoleValue roleInfo = getRoleValue(deviceId);
+ if (roleInfo.contains(MASTER, nodeId)) {
+ return MASTER;
}
+ if (roleInfo.contains(STANDBY, nodeId)) {
+ return STANDBY;
+ }
+ return NONE;
}
@Override
@@ -124,10 +115,11 @@
roleMap.put(deviceId, rv);
return null;
case STANDBY:
+ case NONE:
NodeId current = rv.get(MASTER);
if (current != null) {
//backup and replace current master
- rv.reassign(nodeId, NONE, STANDBY);
+ rv.reassign(current, NONE, STANDBY);
rv.replace(current, nodeId, MASTER);
} else {
//no master before so just add.
@@ -137,12 +129,6 @@
roleMap.put(deviceId, rv);
updateTerm(deviceId);
return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
- case NONE:
- rv.add(MASTER, nodeId);
- rv.reassign(nodeId, STANDBY, NONE);
- roleMap.put(deviceId, rv);
- updateTerm(deviceId);
- return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
default:
log.warn("unknown Mastership Role {}", role);
return null;
@@ -193,21 +179,28 @@
switch (role) {
case MASTER:
rv.reassign(local, STANDBY, NONE);
+ terms.putIfAbsent(deviceId, INIT);
roleMap.put(deviceId, rv);
break;
case STANDBY:
rv.reassign(local, NONE, STANDBY);
roleMap.put(deviceId, rv);
terms.putIfAbsent(deviceId, INIT);
-
break;
case NONE:
- //claim mastership
- rv.add(MASTER, local);
- rv.reassign(local, STANDBY, NONE);
+ //either we're the first standby, or first to device.
+ //for latter, claim mastership.
+ if (rv.get(MASTER) == null) {
+ rv.add(MASTER, local);
+ rv.reassign(local, STANDBY, NONE);
+ updateTerm(deviceId);
+ role = MastershipRole.MASTER;
+ } else {
+ rv.add(STANDBY, local);
+ rv.reassign(local, NONE, STANDBY);
+ role = MastershipRole.STANDBY;
+ }
roleMap.put(deviceId, rv);
- updateTerm(deviceId);
- role = MastershipRole.MASTER;
break;
default:
log.warn("unknown Mastership Role {}", role);
@@ -315,7 +308,10 @@
RoleValue value = roleMap.get(deviceId);
if (value == null) {
value = new RoleValue();
- roleMap.put(deviceId, value);
+ RoleValue concurrentlyAdded = roleMap.putIfAbsent(deviceId, value);
+ if (concurrentlyAdded != null) {
+ return concurrentlyAdded;
+ }
}
return value;
}
@@ -329,16 +325,6 @@
return null;
}
- //check if node is a certain role given a device
- private boolean isRole(
- MastershipRole role, NodeId nodeId, DeviceId deviceId) {
- RoleValue value = roleMap.get(deviceId);
- if (value != null) {
- return value.contains(role, nodeId);
- }
- return false;
- }
-
//adds or updates term information.
private void updateTerm(DeviceId deviceId) {
terms.lock(deviceId);
diff --git a/core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java b/core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java
index 5c867f4..c5daf6c 100644
--- a/core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java
+++ b/core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java
@@ -97,6 +97,7 @@
assertEquals("wrong role:", NONE, dms.getRole(N1, DID1));
testStore.put(DID1, N1, true, false, true);
assertEquals("wrong role:", MASTER, dms.getRole(N1, DID1));
+ testStore.put(DID1, N2, false, true, false);
assertEquals("wrong role:", STANDBY, dms.getRole(N2, DID1));
}
@@ -155,6 +156,7 @@
//switch over to N2
assertEquals("wrong event:", Type.MASTER_CHANGED, dms.setMaster(N2, DID1).type());
+ System.out.println(dms.getTermFor(DID1).master() + ":" + dms.getTermFor(DID1).termNumber());
assertEquals("wrong term", MastershipTerm.of(N2, 1), dms.getTermFor(DID1));
//orphan switch - should be rare case
@@ -182,14 +184,9 @@
assertEquals("wrong event:", Type.MASTER_CHANGED, dms.relinquishRole(N1, DID1).type());
assertEquals("wrong master", N2, dms.getMaster(DID1));
- //STANDBY - nothing here, either
- assertNull("wrong event:", dms.relinquishRole(N1, DID1));
- assertEquals("wrong role for node:", STANDBY, dms.getRole(N1, DID1));
-
//all nodes "give up" on device, which goes back to NONE.
assertNull("wrong event:", dms.relinquishRole(N2, DID1));
assertEquals("wrong role for node:", NONE, dms.getRole(N2, DID1));
- assertEquals("wrong role for node:", NONE, dms.getRole(N1, DID1));
assertEquals("wrong number of retired nodes", 2,
dms.roleMap.get(DID1).nodesOfRole(NONE).size());
@@ -201,6 +198,10 @@
assertEquals("wrong number of backup nodes", 1,
dms.roleMap.get(DID1).nodesOfRole(STANDBY).size());
+ //If STANDBY, should drop to NONE
+ assertNull("wrong event:", dms.relinquishRole(N1, DID1));
+ assertEquals("wrong role for node:", NONE, dms.getRole(N1, DID1));
+
//NONE - nothing happens
assertNull("wrong event:", dms.relinquishRole(N1, DID2));
assertEquals("wrong role for node:", NONE, dms.getRole(N1, DID2));
@@ -218,7 +219,7 @@
public void notify(MastershipEvent event) {
assertEquals("wrong event:", Type.MASTER_CHANGED, event.type());
assertEquals("wrong subject", DID1, event.subject());
- assertEquals("wrong subject", N1, event.node());
+ assertEquals("wrong subject", N1, event.roleInfo().master());
addLatch.countDown();
}
};