fixes related to getRole() assumptions

Change-Id: Icf19d95714dc217200eed021a495d9a78440ca8e

fixes related to getRole() assumptions
Change-Id: Icf19d95714dc217200eed021a495d9a78440ca8e
Ayaka Koshibe
Commit a7384a83d8fc7b0db1799a31dbb9a11e78dd8314 a7384a83 1 parent 44521ecc
Showing 5 changed files with 36 additions and 59 deletions
core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java
core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java
core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java
core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java
--- a/core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java
View file @a7384a8
+++ b/core/api/src/main/java/org/onlab/onos/mastership/MastershipEvent.java
View file @a7384a8
 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 @@ public class MastershipEvent extends AbstractEvent<MastershipEvent.Type, DeviceI
     }
     /**
-     * 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
--- a/core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java
View file @a7384a8
+++ b/core/net/src/main/java/org/onlab/onos/cluster/impl/MastershipManager.java
View file @a7384a8
@@ -227,10 +227,14 @@ implements MastershipService, MastershipAdminService {
             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;
         }
--- a/core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java
View file @a7384a8
+++ b/core/store/dist/src/main/java/org/onlab/onos/store/flow/impl/ReplicaInfoManager.java
View file @a7384a8
@@ -86,7 +86,7 @@ public class ReplicaInfoManager implements ReplicaInfoService {
             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)));
         }
     }
--- a/core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
View file @a7384a8
+++ b/core/store/hz/cluster/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
View file @a7384a8
@@ -91,23 +91,14 @@ implements MastershipStore {
     @Override
     public MastershipRole getRole(NodeId nodeId, DeviceId deviceId) {
-        NodeId current = getNode(MASTER, deviceId);
+        final RoleValue roleInfo = getRoleValue(deviceId);
-        if (current == null) {
+        if (roleInfo.contains(MASTER, nodeId)) {
-            if (isRole(STANDBY, nodeId, deviceId)) {
+            return MASTER;
-                //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;
         }
+        if (roleInfo.contains(STANDBY, nodeId)) {
+            return STANDBY;
         }
+        return NONE;
     }
     @Override
@@ -124,10 +115,11 @@ implements MastershipStore {
                     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 @@ implements MastershipStore {
                     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 @@ implements MastershipStore {
             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
+                    //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);
-                    roleMap.put(deviceId, rv);
                         updateTerm(deviceId);
                         role = MastershipRole.MASTER;
+                    } else {
+                        rv.add(STANDBY, local);
+                        rv.reassign(local, NONE, STANDBY);
+                        role = MastershipRole.STANDBY;
+                    }
+                    roleMap.put(deviceId, rv);
                     break;
                 default:
                     log.warn("unknown Mastership Role {}", role);
@@ -315,7 +308,10 @@ implements MastershipStore {
         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 @@ implements MastershipStore {
         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);
--- a/core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java
View file @a7384a8
+++ b/core/store/hz/cluster/src/test/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStoreTest.java
View file @a7384a8
@@ -97,6 +97,7 @@ public class DistributedMastershipStoreTest {
         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 @@ public class DistributedMastershipStoreTest {
         //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 @@ public class DistributedMastershipStoreTest {
         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 @@ public class DistributedMastershipStoreTest {
         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 class DistributedMastershipStoreTest {
             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();
             }
         };