role reassignment tweaks

Change-Id: Ie6d412787330e67a13e605a34f0824cf70882f85

@@ -33,7 +33,7 @@ public interface MastershipService {
     /**
      * Abandons mastership of the specified device on the local node thus
      * forcing selection of a new master. If the local node is not a master
-     * for this device, no action will be taken.
+     * for this device, no master selection will occur.
      *
      * @param deviceId the identifier of the device
      */

@@ -66,12 +66,25 @@ public interface MastershipStore extends Store<MastershipEvent, MastershipStoreD
     MastershipTerm getTermFor(DeviceId deviceId);
 
     /**
-     * Revokes a controller instance's mastership over a device and hands
-     * over mastership to another controller instance.
+     * Sets a controller instance's mastership role to STANDBY for a device.
+     * If the role is MASTER, another controller instance will be selected
+     * as a candidate master.
      *
      * @param nodeId   the controller instance identifier
-     * @param deviceId device to revoke mastership for
+     * @param deviceId device to revoke mastership role for
      * @return a mastership event
      */
-    MastershipEvent unsetMaster(NodeId nodeId, DeviceId deviceId);
+    MastershipEvent setStandby(NodeId nodeId, DeviceId deviceId);
+
+    /**
+     * Allows a controller instance to give up its current role for a device.
+     * If the role is MASTER, another controller instance will be selected
+     * as a candidate master.
+     *
+     * @param nodeId   the controller instance identifier
+     * @param deviceId device to revoke mastership role for
+     * @return a mastership event
+     */
+    MastershipEvent relinquishRole(NodeId nodeId, DeviceId deviceId);
+
 }

@@ -82,7 +82,7 @@ implements MastershipService, MastershipAdminService {
         if (role.equals(MastershipRole.MASTER)) {
             event = store.setMaster(nodeId, deviceId);
         } else {
-            event = store.unsetMaster(nodeId, deviceId);
+            event = store.setStandby(nodeId, deviceId);
         }
 
         if (event != null) {
@@ -98,13 +98,10 @@ implements MastershipService, MastershipAdminService {
 
     @Override
     public void relinquishMastership(DeviceId deviceId) {
-        MastershipRole role = getLocalRole(deviceId);
-        if (!role.equals(MastershipRole.MASTER)) {
-            return;
-        }
-
-        MastershipEvent event = store.unsetMaster(
+        MastershipEvent event = null;
+            event = store.relinquishRole(
                     clusterService.getLocalNode().id(), deviceId);
+
         if (event != null) {
             post(event);
         }

@@ -142,7 +142,7 @@ public class DeviceManager
 
     // Applies the specified role to the device; ignores NONE
     private void applyRole(DeviceId deviceId, MastershipRole newRole) {
-        if (newRole != MastershipRole.NONE) {
+        if (newRole.equals(MastershipRole.NONE)) {
             Device device = store.getDevice(deviceId);
             DeviceProvider provider = getProvider(device.providerId());
             if (provider != null) {
@@ -196,11 +196,8 @@ public class DeviceManager
             DeviceEvent event = store.createOrUpdateDevice(provider().id(),
                     deviceId, deviceDescription);
 
-            // If there was a change of any kind, trigger role selection
-            // process.
             if (event != null) {
                 log.info("Device {} connected", deviceId);
-                //mastershipService.requestRoleFor(deviceId);
                 provider().roleChanged(event.subject(),
                         mastershipService.requestRoleFor(deviceId));
                 post(event);
@@ -212,11 +209,11 @@ public class DeviceManager
             checkNotNull(deviceId, DEVICE_ID_NULL);
             checkValidity();
             DeviceEvent event = store.markOffline(deviceId);
+            //we're no longer capable of being master or a candidate.
+            mastershipService.relinquishMastership(deviceId);
 
-            //we're no longer capable of mastership.
             if (event != null) {
                 log.info("Device {} disconnected", deviceId);
-                mastershipService.relinquishMastership(deviceId);
                 post(event);
             }
         }
@@ -267,17 +264,23 @@ public class DeviceManager
     }
 
     // Intercepts mastership events
-    private class InternalMastershipListener
-    implements MastershipListener {
+    private class InternalMastershipListener implements MastershipListener {
+
         @Override
         public void event(MastershipEvent event) {
+            DeviceId did = event.subject();
+            if (isAvailable(did)) {
                 if (event.master().equals(clusterService.getLocalNode().id())) {
-                MastershipTerm term = mastershipService.requestTermService()
-                        .getMastershipTerm(event.subject());
-                clockService.setMastershipTerm(event.subject(), term);
-                applyRole(event.subject(), MastershipRole.MASTER);
+                    MastershipTerm term = termService.getMastershipTerm(did);
+                    clockService.setMastershipTerm(did, term);
+                    applyRole(did, MastershipRole.MASTER);
+                } else {
+                    applyRole(did, MastershipRole.STANDBY);
+                }
             } else {
-                applyRole(event.subject(), MastershipRole.STANDBY);
+                //device dead to node, give up
+                mastershipService.relinquishMastership(did);
+                applyRole(did, MastershipRole.STANDBY);
             }
         }
     }

 package org.onlab.onos.net.device.impl;
 
 import com.google.common.collect.Sets;
+
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Ignore;
@@ -258,7 +259,8 @@ public class DeviceManagerTest {
         }
     }
 
-    private static class TestMastershipService extends MastershipServiceAdapter {
+    private static class TestMastershipService
+            extends MastershipServiceAdapter {
         @Override
         public MastershipRole getLocalRole(DeviceId deviceId) {
             return MastershipRole.MASTER;

@@ -5,7 +5,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.onlab.onos.net.MastershipRole.*;
 
-import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
@@ -26,13 +26,7 @@ import org.onlab.onos.cluster.MastershipEvent.Type;
 import org.onlab.onos.cluster.MastershipStoreDelegate;
 import org.onlab.onos.cluster.MastershipTerm;
 import org.onlab.onos.cluster.NodeId;
-import org.onlab.onos.net.Device;
 import org.onlab.onos.net.DeviceId;
-import org.onlab.onos.net.MastershipRole;
-import org.onlab.onos.net.Port;
-import org.onlab.onos.net.PortNumber;
-import org.onlab.onos.net.device.DeviceListener;
-import org.onlab.onos.net.device.DeviceService;
 import org.onlab.onos.store.common.StoreManager;
 import org.onlab.onos.store.common.StoreService;
 import org.onlab.onos.store.common.TestStoreManager;
@@ -87,7 +81,6 @@ public class DistributedMastershipStoreTest {
 
         dms = new TestDistributedMastershipStore(storeMgr, serializationMgr);
         dms.clusterService = new TestClusterService();
-        dms.deviceService = new TestDeviceService();
         dms.activate();
 
         testStore = (TestDistributedMastershipStore) dms;
@@ -105,14 +98,14 @@ public class DistributedMastershipStoreTest {
     @Test
     public void getRole() {
         assertEquals("wrong role:", NONE, dms.getRole(N1, DID1));
-        testStore.put(DID1, N1, true, true, true);
+        testStore.put(DID1, N1, true, false, true);
         assertEquals("wrong role:", MASTER, dms.getRole(N1, DID1));
         assertEquals("wrong role:", STANDBY, dms.getRole(N2, DID1));
     }
 
     @Test
     public void getMaster() {
-        assertTrue("wrong store state:", dms.rawMasters.isEmpty());
+        assertTrue("wrong store state:", dms.masters.isEmpty());
 
         testStore.put(DID1, N1, true, false, false);
         assertEquals("wrong master:", N1, dms.getMaster(DID1));
@@ -121,7 +114,7 @@ public class DistributedMastershipStoreTest {
 
     @Test
     public void getDevices() {
-        assertTrue("wrong store state:", dms.rawMasters.isEmpty());
+        assertTrue("wrong store state:", dms.masters.isEmpty());
 
         testStore.put(DID1, N1, true, false, false);
         testStore.put(DID2, N1, true, false, false);
@@ -139,20 +132,17 @@ public class DistributedMastershipStoreTest {
         //if already MASTER, nothing should happen
         testStore.put(DID2, N1, true, false, false);
         assertEquals("wrong role for MASTER:", MASTER, dms.requestRole(DID2));
-        assertTrue("wrong state for store:",
-                dms.backups.isEmpty() & dms.rawTerms.isEmpty());
 
         //populate maps with DID1, N1 thru NONE case
         assertEquals("wrong role for NONE:", MASTER, dms.requestRole(DID1));
-        assertTrue("wrong state for store:",
-                !dms.backups.isEmpty() & !dms.rawTerms.isEmpty());
+        assertTrue("wrong state for store:", !dms.terms.isEmpty());
         assertEquals("wrong term",
                 MastershipTerm.of(N1, 0), dms.getTermFor(DID1));
 
         //CN2 now local. DID2 has N1 as MASTER so N2 is STANDBY
         testStore.setCurrent(CN2);
         assertEquals("wrong role for STANDBY:", STANDBY, dms.requestRole(DID2));
-        assertEquals("wrong number of entries:", 2, dms.rawTerms.size());
+        assertEquals("wrong number of entries:", 2, dms.terms.size());
 
         //change term and requestRole() again; should persist
         testStore.increment(DID2);
@@ -181,35 +171,42 @@ public class DistributedMastershipStoreTest {
     }
 
     @Test
-    public void unsetMaster() {
+    public void relinquishRole() {
         //populate maps with DID1, N1 as MASTER thru NONE case
         testStore.setCurrent(CN1);
         assertEquals("wrong role for NONE:", MASTER, dms.requestRole(DID1));
         //no backup, no new MASTER/event
-        assertNull("wrong event:", dms.unsetMaster(N1, DID1));
+        assertNull("wrong event:", dms.relinquishRole(N1, DID1));
 
         dms.requestRole(DID1);
-        ((TestDeviceService) dms.deviceService).active.add(DID1);
 
         //add backup CN2, get it elected MASTER by relinquishing
         testStore.setCurrent(CN2);
-        dms.requestRole(DID1);
-        assertEquals("wrong event:", Type.MASTER_CHANGED, dms.unsetMaster(N1, DID1).type());
+        assertEquals("wrong role for NONE:", STANDBY, dms.requestRole(DID1));
+        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.unsetMaster(N1, DID1));
+        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.unusable.size());
+
+        //bring nodes back
+        assertEquals("wrong role for NONE:", MASTER, dms.requestRole(DID1));
+        testStore.setCurrent(CN1);
+        assertEquals("wrong role for NONE:", STANDBY, dms.requestRole(DID1));
+        assertEquals("wrong number of backup nodes", 1, dms.standbys.size());
+
         //NONE - nothing happens
-        assertNull("wrong event:", dms.unsetMaster(N1, DID2));
+        assertNull("wrong event:", dms.relinquishRole(N1, DID2));
         assertEquals("wrong role for node:", NONE, dms.getRole(N1, DID2));
 
-        //for a device that turned off (not active) - status to NONE
-        ((TestDeviceService) dms.deviceService).active.clear();
-        assertNull("extraneous event:", dms.unsetMaster(N2, DID1));
-        assertEquals("wrong role", NONE, dms.getRole(N2, DID1));
-
     }
 
     @Ignore("Ignore until Delegate spec. is clear.")
@@ -244,36 +241,55 @@ public class DistributedMastershipStoreTest {
 
         //helper to populate master/backup structures
         public void put(DeviceId dev, NodeId node,
-                boolean store, boolean backup, boolean term) {
-            if (store) {
-                dms.rawMasters.put(serialize(dev), serialize(node));
+                boolean master, boolean backup, boolean term) {
+            byte [] n = serialize(node);
+            byte [] d = serialize(dev);
+
+            if (master) {
+                dms.masters.put(d, n);
+                dms.unusable.put(d, n);
+                dms.standbys.remove(d, n);
             }
             if (backup) {
-                dms.backups.put(serialize(node), (byte) 0);
+                dms.standbys.put(d, n);
+                dms.masters.remove(d, n);
+                dms.unusable.remove(d, n);
             }
             if (term) {
-                dms.rawTerms.put(serialize(dev), 0);
+                dms.terms.put(d, 0);
+            }
+        }
+
+        public void dump() {
+            System.out.println("standbys");
+            for (Map.Entry<byte [], byte []> e : standbys.entrySet()) {
+                System.out.println(deserialize(e.getKey()) + ":" + deserialize(e.getValue()));
+            }
+            System.out.println("unusable");
+            for (Map.Entry<byte [], byte []> e : unusable.entrySet()) {
+                System.out.println(deserialize(e.getKey()) + ":" + deserialize(e.getValue()));
             }
         }
 
         //clears structures
         public void reset(boolean store, boolean backup, boolean term) {
             if (store) {
-                dms.rawMasters.clear();
+                dms.masters.clear();
+                dms.unusable.clear();
             }
             if (backup) {
-                dms.backups.clear();
+                dms.standbys.clear();
             }
             if (term) {
-                dms.rawTerms.clear();
+                dms.terms.clear();
             }
         }
 
         //increment term for a device
         public void increment(DeviceId dev) {
-            Integer t = dms.rawTerms.get(serialize(dev));
+            Integer t = dms.terms.get(serialize(dev));
             if (t != null) {
-                dms.rawTerms.put(serialize(dev), ++t);
+                dms.terms.put(serialize(dev), ++t);
             }
         }
 
@@ -317,52 +333,4 @@ public class DistributedMastershipStoreTest {
 
     }
 
-    private class TestDeviceService implements DeviceService {
-
-        Set<DeviceId> active = Sets.newHashSet();
-
-        @Override
-        public int getDeviceCount() {
-            return 0;
-        }
-
-        @Override
-        public Iterable<Device> getDevices() {
-            return null;
-        }
-
-        @Override
-        public Device getDevice(DeviceId deviceId) {
-            return null;
-        }
-
-        @Override
-        public MastershipRole getRole(DeviceId deviceId) {
-            return null;
-        }
-
-        @Override
-        public List<Port> getPorts(DeviceId deviceId) {
-            return null;
-        }
-
-        @Override
-        public Port getPort(DeviceId deviceId, PortNumber portNumber) {
-            return null;
-        }
-
-        @Override
-        public boolean isAvailable(DeviceId deviceId) {
-            return active.contains(deviceId);
-        }
-
-        @Override
-        public void addListener(DeviceListener listener) {
-        }
-
-        @Override
-        public void removeListener(DeviceListener listener) {
-        }
-
-    }
 }

@@ -174,7 +174,7 @@ public class SimpleMastershipStore
     }
 
     @Override
-    public MastershipEvent unsetMaster(NodeId nodeId, DeviceId deviceId) {
+    public MastershipEvent setStandby(NodeId nodeId, DeviceId deviceId) {
         MastershipRole role = getRole(nodeId, deviceId);
         synchronized (this) {
             switch (role) {
@@ -214,4 +214,9 @@ public class SimpleMastershipStore
         return backup;
     }
 
+    @Override
+    public MastershipEvent relinquishRole(NodeId nodeId, DeviceId deviceId) {
+        return setStandby(nodeId, deviceId);
+    }
+
 }

@@ -129,22 +129,22 @@ public class SimpleMastershipStoreTest {
     public void unsetMaster() {
         //NONE - record backup but take no other action
         put(DID1, N1, false, false);
-        sms.unsetMaster(N1, DID1);
+        sms.setStandby(N1, DID1);
         assertTrue("not backed up", sms.backups.contains(N1));
         sms.termMap.clear();
-        sms.unsetMaster(N1, DID1);
+        sms.setStandby(N1, DID1);
         assertTrue("term not set", sms.termMap.containsKey(DID1));
 
         //no backup, MASTER
         put(DID1, N1, true, true);
-        assertNull("wrong event", sms.unsetMaster(N1, DID1));
+        assertNull("wrong event", sms.setStandby(N1, DID1));
         assertNull("wrong node", sms.masterMap.get(DID1));
 
         //backup, switch
         sms.masterMap.clear();
         put(DID1, N1, true, true);
         put(DID2, N2, true, true);
-        assertEquals("wrong event", MASTER_CHANGED, sms.unsetMaster(N1, DID1).type());
+        assertEquals("wrong event", MASTER_CHANGED, sms.setStandby(N1, DID1).type());
     }
 
     //helper to populate master/backup structures

@@ -981,11 +981,13 @@ class OFChannelHandler extends IdleStateAwareChannelHandler {
                 // switch was a duplicate-dpid, calling the method below would clear
                 // all state for the original switch (with the same dpid),
                 // which we obviously don't want.
+                log.info("{}:removal called");
                 sw.removeConnectedSwitch();
             } else {
                 // A duplicate was disconnected on this ChannelHandler,
                 // this is the same switch reconnecting, but the original state was
                 // not cleaned up - XXX check liveness of original ChannelHandler
+                log.info("{}:duplicate found");
                 duplicateDpidFound = Boolean.FALSE;
             }
         } else {

@@ -307,9 +307,11 @@ public class OpenFlowControllerImpl implements OpenFlowController {
             connectedSwitches.remove(dpid);
             OpenFlowSwitch sw = activeMasterSwitches.remove(dpid);
             if (sw == null) {
+                log.warn("sw was null for {}", dpid);
                 sw = activeEqualSwitches.remove(dpid);
             }
             for (OpenFlowSwitchListener l : ofSwitchListener) {
+                log.warn("removal for {}", dpid);
                 l.switchRemoved(dpid);
             }
         }