CORD-1719 Cleanup old flows properly if the switch moves to a inexistent location

Change-Id: I8ffe970aaa9cec9ac3d4c266e460538bfd07c4fc
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/HostHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/HostHandler.java
index f8428a7..5029b30 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/HostHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/HostHandler.java
@@ -50,6 +50,8 @@
  * Handles host-related events.
  */
 public class HostHandler {
+    private static final String NOT_MASTER = "Current instance is not the master of {}. Ignore.";
+
     private static final Logger log = LoggerFactory.getLogger(HostHandler.class);
     protected final SegmentRoutingManager srManager;
     private HostService hostService;
@@ -85,6 +87,11 @@
     void processHostAddedAtLocation(Host host, HostLocation location) {
         checkArgument(host.locations().contains(location), "{} is not a location of {}", location, host);
 
+        if (!isMasterOf(location)) {
+            log.debug(NOT_MASTER, location);
+            return;
+        }
+
         MacAddress mac = host.mac();
         VlanId vlanId = host.vlan();
         Set<HostLocation> locations = host.locations();
@@ -108,7 +115,7 @@
         Set<IpAddress> ips = host.ipAddresses();
         log.info("Host {}/{} is removed from {}", hostMac, hostVlanId, locations);
 
-        locations.forEach(location -> {
+        locations.stream().filter(this::isMasterOf).forEach(location -> {
             processBridgingRule(location.deviceId(), location.port(), hostMac, hostVlanId, true);
             ips.forEach(ip ->
                 processRoutingRule(location.deviceId(), location.port(), hostMac, hostVlanId, ip, true)
@@ -143,7 +150,8 @@
                 .collect(Collectors.toSet());
 
         // For each old location
-        Sets.difference(prevLocations, newLocations).forEach(prevLocation -> {
+        Sets.difference(prevLocations, newLocations).stream().filter(this::isMasterOf)
+                .forEach(prevLocation -> {
             // Remove routing rules for old IPs
             Sets.difference(prevIps, newIps).forEach(ip ->
                     processRoutingRule(prevLocation.deviceId(), prevLocation.port(), hostMac, hostVlanId,
@@ -203,7 +211,8 @@
         });
 
         // For each new location, add all new IPs.
-        Sets.difference(newLocations, prevLocations).forEach(newLocation -> {
+        Sets.difference(newLocations, prevLocations).stream().filter(this::isMasterOf)
+                .forEach(newLocation -> {
             processBridgingRule(newLocation.deviceId(), newLocation.port(), hostMac, hostVlanId, false);
             newIps.forEach(ip ->
                     processRoutingRule(newLocation.deviceId(), newLocation.port(), hostMac, hostVlanId,
@@ -212,7 +221,8 @@
         });
 
         // For each unchanged location, add new IPs and remove old IPs.
-        Sets.intersection(newLocations, prevLocations).forEach(unchangedLocation -> {
+        Sets.intersection(newLocations, prevLocations).stream().filter(this::isMasterOf)
+                .forEach(unchangedLocation -> {
             Sets.difference(prevIps, newIps).forEach(ip ->
                     processRoutingRule(unchangedLocation.deviceId(), unchangedLocation.port(), hostMac,
                             hostVlanId, ip, true)
@@ -233,7 +243,7 @@
         Set<IpAddress> newIps = event.subject().ipAddresses();
         log.info("Host {}/{} is updated", mac, vlanId);
 
-        locations.forEach(location -> {
+        locations.stream().filter(this::isMasterOf).forEach(location -> {
             Sets.difference(prevIps, newIps).forEach(ip ->
                     processRoutingRule(location.deviceId(), location.port(), mac, vlanId, ip, true)
             );
@@ -373,4 +383,15 @@
             srManager.routingRulePopulator.populateRoute(deviceId, ip.toIpPrefix(), mac, vlanId, port);
         }
     }
+
+    /**
+     * Determine if current instance is the master of given connect point.
+     *
+     * @param cp connect point
+     * @return true if current instance is the master of given connect point
+     */
+    private boolean isMasterOf(ConnectPoint cp) {
+        log.debug(NOT_MASTER, cp);
+        return srManager.mastershipService.isLocalMaster(cp.deviceId());
+    }
 }
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 9f6ecd4..0e23a26 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -1478,12 +1478,6 @@
     private class InternalHostListener implements HostListener {
         @Override
         public void event(HostEvent event) {
-            // Do not proceed without mastership
-            DeviceId deviceId = event.subject().location().deviceId();
-            if (!mastershipService.isLocalMaster(deviceId)) {
-                return;
-            }
-
             switch (event.type()) {
                 case HOST_ADDED:
                     hostHandler.processHostAddedEvent(event);
diff --git a/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java b/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java
index ca3378c..314fef6 100644
--- a/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java
+++ b/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java
@@ -18,6 +18,7 @@
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
@@ -28,6 +29,7 @@
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.VlanId;
 import org.onosproject.core.DefaultApplicationId;
+import org.onosproject.mastership.MastershipServiceAdapter;
 import org.onosproject.net.intf.Interface;
 import org.onosproject.net.intf.InterfaceServiceAdapter;
 import org.onosproject.net.ConnectPoint;
@@ -95,8 +97,10 @@
     // Device
     private static final DeviceId DEV1 = DeviceId.deviceId("of:0000000000000001");
     private static final DeviceId DEV2 = DeviceId.deviceId("of:0000000000000002");
-    private static final DeviceId DEV3 = DeviceId.deviceId("of:000000000000003");
-    private static final DeviceId DEV4 = DeviceId.deviceId("of:000000000000004");
+    private static final DeviceId DEV3 = DeviceId.deviceId("of:0000000000000003");
+    private static final DeviceId DEV4 = DeviceId.deviceId("of:0000000000000004");
+    private static final DeviceId DEV5 = DeviceId.deviceId("of:0000000000000005");
+    private static final DeviceId DEV6 = DeviceId.deviceId("of:0000000000000006");
     // Port
     private static final PortNumber P1 = PortNumber.portNumber(1);
     private static final PortNumber P2 = PortNumber.portNumber(2);
@@ -120,6 +124,11 @@
     private static final HostLocation HOST_LOC41 = new HostLocation(CP41, 0);
     private static final ConnectPoint CP39 = new ConnectPoint(DEV3, P9);
     private static final ConnectPoint CP49 = new ConnectPoint(DEV4, P9);
+    // Conenct Point for mastership test
+    private static final ConnectPoint CP51 = new ConnectPoint(DEV5, P1);
+    private static final HostLocation HOST_LOC51 = new HostLocation(CP51, 0);
+    private static final ConnectPoint CP61 = new ConnectPoint(DEV6, P1);
+    private static final HostLocation HOST_LOC61 = new HostLocation(CP61, 0);
     // Interface VLAN
     private static final VlanId INTF_VLAN_UNTAGGED = VlanId.vlanId((short) 10);
     private static final Set<VlanId> INTF_VLAN_TAGGED = Sets.newHashSet(VlanId.vlanId((short) 20));
@@ -146,6 +155,7 @@
         srManager.flowObjectiveService = new MockFlowObjectiveService();
         srManager.routingRulePopulator = new MockRoutingRulePopulator();
         srManager.interfaceService = new MockInterfaceService();
+        srManager.mastershipService = new MockMastershipService();
         srManager.hostService = new MockHostService();
         srManager.cfgService = new MockNetworkConfigRegistry();
 
@@ -389,6 +399,92 @@
     }
 
     @Test
+    public void testHostMoveToInvalidLocation() throws Exception {
+        Host host1 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+                Sets.newHashSet(HOST_LOC11), Sets.newHashSet(HOST_IP11), false);
+        Host host2 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+                Sets.newHashSet(HOST_LOC51), Sets.newHashSet(HOST_IP11), false);
+
+        // Add a host
+        // Expect: add one new routing rule, one new bridging rule
+        hostHandler.processHostAddedEvent(new HostEvent(HostEvent.Type.HOST_ADDED, host1));
+        assertEquals(1, routingTable.size());
+        assertNotNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())));
+        assertNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP21.toIpPrefix())));
+        assertEquals(1, bridgingTable.size());
+        assertNotNull(bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)));
+        assertNull(bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)));
+
+        // Move the host to an invalid location
+        // Expect: Old flow is removed. New flow is not created
+        hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host2, host1));
+        assertEquals(0, routingTable.size());
+        assertEquals(0, bridgingTable.size());
+
+        // Move the host to a valid location
+        // Expect: add one new routing rule, one new bridging rule
+        hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host1, host2));
+        assertEquals(1, routingTable.size());
+        assertNotNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())));
+        assertNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP21.toIpPrefix())));
+        assertEquals(1, bridgingTable.size());
+        assertNotNull(bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)));
+        assertNull(bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)));
+    }
+
+    @Test
+    public void testDualHomedHostMoveToInvalidLocation() throws Exception {
+        Host host1 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+                Sets.newHashSet(HOST_LOC11, HOST_LOC21), Sets.newHashSet(HOST_IP11), false);
+        Host host2 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+                Sets.newHashSet(HOST_LOC11, HOST_LOC51), Sets.newHashSet(HOST_IP11), false);
+        Host host3 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+                Sets.newHashSet(HOST_LOC61, HOST_LOC51), Sets.newHashSet(HOST_IP11), false);
+
+        // Add a host
+        // Expect: add two new routing rules, two new bridging rules
+        hostHandler.processHostAddedEvent(new HostEvent(HostEvent.Type.HOST_ADDED, host1));
+        assertEquals(2, routingTable.size());
+        assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+        assertEquals(P1, routingTable.get(new RoutingTableKey(DEV2, HOST_IP11.toIpPrefix())).portNumber);
+        assertEquals(2, bridgingTable.size());
+        assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+        assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+
+        // Move first host location to an invalid location
+        // Expect: One routing and one bridging flow
+        hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host2, host1));
+        assertEquals(1, routingTable.size());
+        assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+        assertEquals(1, bridgingTable.size());
+        assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+
+        // Move second host location to an invalid location
+        // Expect: No routing or bridging rule
+        hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host3, host2));
+        assertEquals(0, routingTable.size());
+        assertEquals(0, bridgingTable.size());
+
+        // Move second host location back to a valid location
+        // Expect: One routing and one bridging flow
+        hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host2, host3));
+        assertEquals(1, routingTable.size());
+        assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+        assertEquals(1, bridgingTable.size());
+        assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+
+        // Move first host location back to a valid location
+        // Expect: Two routing and two bridging flow
+        hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host1, host2));
+        assertEquals(2, routingTable.size());
+        assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+        assertEquals(P1, routingTable.get(new RoutingTableKey(DEV2, HOST_IP11.toIpPrefix())).portNumber);
+        assertEquals(2, bridgingTable.size());
+        assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+        assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+    }
+
+    @Test
     public void testDualHomingSingleLocationFail() throws Exception {
         Host host1 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
                 Sets.newHashSet(HOST_LOC31, HOST_LOC41), Sets.newHashSet(HOST_IP11, HOST_IP12), false);
@@ -618,6 +714,14 @@
         }
     }
 
+    class MockMastershipService extends MastershipServiceAdapter {
+        @Override
+        public boolean isLocalMaster(DeviceId deviceId) {
+            Set<DeviceId> localDevices = ImmutableSet.of(DEV1, DEV2, DEV3, DEV4);
+            return localDevices.contains(deviceId);
+        }
+    }
+
     class MockNetworkConfigRegistry extends NetworkConfigRegistryAdapter {
         @Override
         public <S, C extends Config<S>> C getConfig(S subject, Class<C> configClass) {