avoid transient changes to MastershipStore from being posted as events

Change-Id: Id033cf50f865e44439138f5b3814ebaedb832b73

@@ -372,4 +372,8 @@ public class OpticalConfigProvider extends AbstractProvider implements DevicePro
         // TODO Auto-generated method stub.
         // TODO Auto-generated method stub.
     }
     }
 
 
+    @Override
+    public boolean isReachable(Device device) {
+        return false;
+    }
 }
 }

@@ -47,4 +47,11 @@ public interface DeviceProvider extends Provider {
      */
      */
     void roleChanged(Device device, MastershipRole newRole);
     void roleChanged(Device device, MastershipRole newRole);
 
 
+    /**
+     * Checks the reachability (connectivity) of a device from this provider.
+     *
+     * @param device device to check
+     * @return true if reachable, false otherwise
+     */
+    boolean isReachable(Device device);
 }
 }

@@ -97,10 +97,20 @@ implements MastershipService, MastershipAdminService {
         checkNotNull(role, ROLE_NULL);
         checkNotNull(role, ROLE_NULL);
 
 
         MastershipEvent event = null;
         MastershipEvent event = null;
-        if (role.equals(MastershipRole.MASTER)) {
-            event = store.setMaster(nodeId, deviceId);
-        } else {
-            event = store.setStandby(nodeId, deviceId);
+
+        switch (role) {
+            case MASTER:
+                event = store.setMaster(nodeId, deviceId);
+                break;
+            case STANDBY:
+                event = store.setStandby(nodeId, deviceId);
+                break;
+            case NONE:
+                event = store.relinquishRole(nodeId, deviceId);
+                break;
+            default:
+                log.info("Unknown role; ignoring");
+                return;
         }
         }
 
 
         if (event != null) {
         if (event != null) {
@@ -259,6 +269,10 @@ implements MastershipService, MastershipAdminService {
 
 
         @Override
         @Override
         public void notify(MastershipEvent event) {
         public void notify(MastershipEvent event) {
+            if (clusterService.getLocalNode().id().equals(event.roleInfo().master())) {
+                log.info("ignoring locally-generated event {}", event);
+               // return;
+            }
             log.info("dispatching mastership event {}", event);
             log.info("dispatching mastership event {}", event);
             eventDispatcher.post(event);
             eventDispatcher.post(event);
         }
         }

@@ -159,32 +159,37 @@ public class DeviceManager
 
 
     // Applies the specified role to the device; ignores NONE
     // Applies the specified role to the device; ignores NONE
     private void applyRole(DeviceId deviceId, MastershipRole newRole) {
     private void applyRole(DeviceId deviceId, MastershipRole newRole) {
-        if (!newRole.equals(MastershipRole.NONE)) {
-            Device device = store.getDevice(deviceId);
-            // FIXME: Device might not be there yet. (eventual consistent)
-            if (device == null) {
-                return;
-            }
-            DeviceProvider provider = getProvider(device.providerId());
-            if (provider != null) {
-                provider.roleChanged(device, newRole);
+        if (newRole.equals(MastershipRole.NONE)) {
+            return;
+        }
 
 
-                // only trigger event when request was sent to provider
-                // TODO: consider removing this from Device event type?
-                post(new DeviceEvent(DEVICE_MASTERSHIP_CHANGED, device));
+        Device device = store.getDevice(deviceId);
+        // FIXME: Device might not be there yet. (eventual consistent)
+        if (device == null) {
+            return;
+        }
+
+        DeviceProvider provider = getProvider(device.providerId());
+        if (provider != null) {
+            provider.roleChanged(device, newRole);
+            // only trigger event when request was sent to provider
+            // TODO: consider removing this from Device event type?
+            post(new DeviceEvent(DEVICE_MASTERSHIP_CHANGED, device));
+
+            if (newRole.equals(MastershipRole.MASTER)) {
+                provider.triggerProbe(device);
             }
             }
         }
         }
     }
     }
 
 
-    // Queries a device for port information.
-    private void queryPortInfo(DeviceId deviceId) {
-        Device device = store.getDevice(deviceId);
+    // Check a device for control channel connectivity.
+    private boolean isReachable(Device device) {
         // FIXME: Device might not be there yet. (eventual consistent)
         // FIXME: Device might not be there yet. (eventual consistent)
         if (device == null) {
         if (device == null) {
-            return;
+            return false;
         }
         }
         DeviceProvider provider = getProvider(device.providerId());
         DeviceProvider provider = getProvider(device.providerId());
-        provider.triggerProbe(device);
+        return provider.isReachable(device);
     }
     }
 
 
     @Override
     @Override
@@ -236,7 +241,6 @@ public class DeviceManager
             log.info("Device {} connected", deviceId);
             log.info("Device {} connected", deviceId);
             // check my Role
             // check my Role
             MastershipRole role = mastershipService.requestRoleFor(deviceId);
             MastershipRole role = mastershipService.requestRoleFor(deviceId);
-            log.info("requestedRole, became {} for {}", role, deviceId);
             if (role != MastershipRole.MASTER) {
             if (role != MastershipRole.MASTER) {
                 // TODO: Do we need to explicitly tell the Provider that
                 // TODO: Do we need to explicitly tell the Provider that
                 // this instance is no longer the MASTER? probably not
                 // this instance is no longer the MASTER? probably not
@@ -405,14 +409,16 @@ public class DeviceManager
                 // only set the new term if I am the master
                 // only set the new term if I am the master
                 deviceClockProviderService.setMastershipTerm(did, term);
                 deviceClockProviderService.setMastershipTerm(did, term);
 
 
-                // FIXME: we should check that the device is connected on our end.
-                // currently, this is not straight forward as the actual switch
-                // implementation is hidden from the registry. Maybe we can ask the
-                // provider.
                 // if the device is null here, we are the first master to claim the
                 // if the device is null here, we are the first master to claim the
                 // device. No worries, the DeviceManager will create one soon.
                 // device. No worries, the DeviceManager will create one soon.
                 Device device = getDevice(did);
                 Device device = getDevice(did);
                 if ((device != null) && !isAvailable(did)) {
                 if ((device != null) && !isAvailable(did)) {
+                    if (!isReachable(device)) {
+                        log.warn("Device {} has disconnected after this event", did);
+                        mastershipService.relinquishMastership(did);
+                        applyRole(did, MastershipRole.STANDBY);
+                        return;
+                    }
                     //flag the device as online. Is there a better way to do this?
                     //flag the device as online. Is there a better way to do this?
                     DeviceEvent devEvent = store.createOrUpdateDevice(device.providerId(), did,
                     DeviceEvent devEvent = store.createOrUpdateDevice(device.providerId(), did,
                             new DefaultDeviceDescription(
                             new DefaultDeviceDescription(
@@ -422,9 +428,11 @@ public class DeviceManager
                     post(devEvent);
                     post(devEvent);
                 }
                 }
                 applyRole(did, MastershipRole.MASTER);
                 applyRole(did, MastershipRole.MASTER);
-                // re-collect device information to fix potential staleness
-                queryPortInfo(did);
             } else if (event.roleInfo().backups().contains(myNodeId)) {
             } else if (event.roleInfo().backups().contains(myNodeId)) {
+                if (!isReachable(getDevice(did))) {
+                    log.warn("Device {} has disconnected after this event", did);
+                    mastershipService.relinquishMastership(did);
+                }
                 applyRole(did, MastershipRole.STANDBY);
                 applyRole(did, MastershipRole.STANDBY);
             }
             }
         }
         }

@@ -278,6 +278,11 @@ public class DeviceManagerTest {
             deviceReceived = device;
             deviceReceived = device;
             roleReceived = newRole;
             roleReceived = newRole;
         }
         }
+
+        @Override
+        public boolean isReachable(Device device) {
+            return false;
+        }
     }
     }
 
 
     private static class TestListener implements DeviceListener {
     private static class TestListener implements DeviceListener {

@@ -272,6 +272,10 @@ implements MastershipStore {
             switch (role) {
             switch (role) {
                 case MASTER:
                 case MASTER:
                     event = reelect(nodeId, deviceId, rv);
                     event = reelect(nodeId, deviceId, rv);
+                    if (event != null) {
+                        Integer term = terms.get(deviceId);
+                        terms.put(deviceId, ++term);
+                    }
                     //fall through to reinforce relinquishment
                     //fall through to reinforce relinquishment
                 case STANDBY:
                 case STANDBY:
                     //fall through to reinforce relinquishment
                     //fall through to reinforce relinquishment
@@ -304,15 +308,11 @@ implements MastershipStore {
         if (backup == null) {
         if (backup == null) {
             log.info("{} giving up and going to NONE for {}", current, deviceId);
             log.info("{} giving up and going to NONE for {}", current, deviceId);
             rv.remove(MASTER, current);
             rv.remove(MASTER, current);
-            roleMap.put(deviceId, rv);
             return null;
             return null;
         } else {
         } else {
             log.info("{} trying to pass mastership for {} to {}", current, deviceId, backup);
             log.info("{} trying to pass mastership for {} to {}", current, deviceId, backup);
             rv.replace(current, backup, MASTER);
             rv.replace(current, backup, MASTER);
             rv.reassign(backup, STANDBY, NONE);
             rv.reassign(backup, STANDBY, NONE);
-            roleMap.put(deviceId, rv);
-            Integer term = terms.get(deviceId);
-            terms.put(deviceId, ++term);
             return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
             return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
         }
         }
     }
     }
@@ -366,7 +366,7 @@ implements MastershipStore {
 
 
         @Override
         @Override
         public void entryUpdated(EntryEvent<DeviceId, RoleValue> event) {
         public void entryUpdated(EntryEvent<DeviceId, RoleValue> event) {
-
+            // this subsumes entryAdded event
             notifyDelegate(new MastershipEvent(
             notifyDelegate(new MastershipEvent(
                     MASTER_CHANGED, event.getKey(), event.getValue().roleInfo()));
                     MASTER_CHANGED, event.getKey(), event.getValue().roleInfo()));
         }
         }

@@ -103,22 +103,31 @@ public class OpenFlowDeviceProvider extends AbstractProvider implements DevicePr
         LOG.info("Stopped");
         LOG.info("Stopped");
     }
     }
 
 
-    @Override
-    public void triggerProbe(Device device) {
-        LOG.info("Triggering probe on device {}", device.id());
 
 
-        // 1. check device liveness
+    @Override
+    public boolean isReachable(Device device) {
         // FIXME if possible, we might want this to be part of
         // FIXME if possible, we might want this to be part of
         // OpenFlowSwitch interface so the driver interface isn't misused.
         // OpenFlowSwitch interface so the driver interface isn't misused.
         OpenFlowSwitch sw = controller.getSwitch(dpid(device.id().uri()));
         OpenFlowSwitch sw = controller.getSwitch(dpid(device.id().uri()));
-        if (sw == null ||
-            !((OpenFlowSwitchDriver) sw).isConnected()) {
-            LOG.error("Failed to probe device {} on sw={}", device, sw);
-            providerService.deviceDisconnected(device.id());
-            return;
+        if (sw == null || !((OpenFlowSwitchDriver) sw).isConnected()) {
+            return false;
         }
         }
+        return true;
+        //return checkChannel(device, sw);
+    }
+
+    @Override
+    public void triggerProbe(Device device) {
+        LOG.info("Triggering probe on device {}", device.id());
+
+        OpenFlowSwitch sw = controller.getSwitch(dpid(device.id().uri()));
+        //if (!checkChannel(device, sw)) {
+          //  LOG.error("Failed to probe device {} on sw={}", device, sw);
+        //  providerService.deviceDisconnected(device.id());
+            //return;
+        //}
 
 
-        // 2. Prompt an update of port information. Do we have an XID for this?
+        // Prompt an update of port information. We can use any XID for this.
         OFFactory fact = sw.factory();
         OFFactory fact = sw.factory();
         switch (fact.getVersion()) {
         switch (fact.getVersion()) {
             case OF_10:
             case OF_10:
@@ -132,6 +141,16 @@ public class OpenFlowDeviceProvider extends AbstractProvider implements DevicePr
         }
         }
     }
     }
 
 
+    // Checks if the OF channel is connected.
+    //private boolean checkChannel(Device device, OpenFlowSwitch sw) {
+        // FIXME if possible, we might want this to be part of
+        // OpenFlowSwitch interface so the driver interface isn't misused.
+    //    if (sw == null || !((OpenFlowSwitchDriver) sw).isConnected()) {
+      //      return false;
+  //      }
+    //    return true;
+   // }
+
     @Override
     @Override
     public void roleChanged(Device device, MastershipRole newRole) {
     public void roleChanged(Device device, MastershipRole newRole) {
         switch (newRole) {
         switch (newRole) {

@@ -226,4 +226,9 @@ class ConfigProvider implements DeviceProvider, LinkProvider, HostProvider {
     public ProviderId id() {
     public ProviderId id() {
         return PID;
         return PID;
     }
     }
+
+    @Override
+    public boolean isReachable(Device device) {
+        return false;
+    }
 }
 }