Bugfix in route handler in occasion of route update event

Change-Id: Ia0a0cc7d5522d41cad5f3d992c589c9f5cbad244
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RouteHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RouteHandler.java
index c80bc09..21870ee 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RouteHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RouteHandler.java
@@ -161,25 +161,20 @@
         log.debug("RouteUpdated. populateSubnet {}, {}", allLocations, allPrefixes);
         srManager.defaultRoutingHandler.populateSubnet(allLocations, allPrefixes);
 
-
         Set<ResolvedRoute> toBeRemoved = Sets.difference(oldRoutes, routes).immutableCopy();
         Set<ResolvedRoute> toBeAdded = Sets.difference(routes, oldRoutes).immutableCopy();
 
         toBeRemoved.forEach(route -> {
             srManager.nextHopLocations(route).forEach(oldLocation -> {
-               if (toBeAdded.stream().map(srManager::nextHopLocations)
-                       .flatMap(Set::stream).map(ConnectPoint::deviceId)
-                       .noneMatch(deviceId -> deviceId.equals(oldLocation.deviceId()))) {
-                   IpPrefix prefix = route.prefix();
-                   MacAddress nextHopMac = route.nextHopMac();
-                   VlanId nextHopVlan = route.nextHopVlan();
-
-                   log.debug("RouteUpdated. removeSubnet {}, {}", oldLocation, prefix);
-                   srManager.deviceConfiguration.removeSubnet(oldLocation, prefix);
-                   log.debug("RouteUpdated. revokeRoute {}, {}, {}, {}", oldLocation, prefix, nextHopMac, nextHopVlan);
-                   srManager.defaultRoutingHandler.revokeRoute(oldLocation.deviceId(), prefix,
-                           nextHopMac, nextHopVlan, oldLocation.port());
-               }
+                if (toBeAdded.stream().map(srManager::nextHopLocations)
+                        .flatMap(Set::stream).map(ConnectPoint::deviceId)
+                        .noneMatch(deviceId -> deviceId.equals(oldLocation.deviceId()))) {
+                    IpPrefix prefix = route.prefix();
+                    log.debug("RouteUpdated. removeSubnet {}, {}", oldLocation, prefix);
+                    srManager.deviceConfiguration.removeSubnet(oldLocation, prefix);
+                    // We don't remove the flow on the old location in occasion of two next hops becoming one
+                    // since the populateSubnet will point the old location to the new location via spine.
+                }
             });
         });
 
@@ -197,7 +192,6 @@
                         nextHopMac, nextHopVlan, location.port());
             });
         });
-
     }
 
     void processRouteRemoved(RouteEvent event) {
diff --git a/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java b/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java
index 775e5ee..843ba6b 100644
--- a/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java
+++ b/apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java
@@ -49,12 +49,18 @@
 import java.util.Map;
 import java.util.Set;
 
+import static org.easymock.EasyMock.reset;
 import static org.junit.Assert.*;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expectLastCall;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
 
 /**
  * Unit test for {@link RouteHandler}.
  */
 public class RouteHandlerTest {
+    private SegmentRoutingManager srManager;
     private RouteHandler routeHandler;
     private HostService hostService;
 
@@ -134,9 +140,9 @@
         mockNetworkConfigRegistry.applyConfig(dev2Config);
 
         // Initialize Segment Routing Manager
-        SegmentRoutingManager srManager = new MockSegmentRoutingManager(NEXT_TABLE);
+        srManager = new MockSegmentRoutingManager(NEXT_TABLE);
         srManager.cfgService = new NetworkConfigRegistryAdapter();
-        srManager.deviceConfiguration = new DeviceConfiguration(srManager);
+        srManager.deviceConfiguration = createMock(DeviceConfiguration.class);
         srManager.flowObjectiveService = new MockFlowObjectiveService(BRIDGING_TABLE, NEXT_TABLE);
         srManager.routingRulePopulator = new MockRoutingRulePopulator(srManager, ROUTING_TABLE);
         srManager.defaultRoutingHandler = new MockDefaultRoutingHandler(srManager, SUBNET_TABLE);
@@ -180,6 +186,11 @@
 
     @Test
     public void processRouteAdded() throws Exception {
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.addSubnet(CP1, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_ADDED, RR1, Sets.newHashSet(RR1));
         routeHandler.processRouteAdded(re);
 
@@ -191,17 +202,28 @@
 
         assertEquals(1, SUBNET_TABLE.size());
         assertTrue(SUBNET_TABLE.get(CP1).contains(P1));
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void processRouteUpdated() throws Exception {
         processRouteAdded();
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.removeSubnet(CP1, P1);
+        expectLastCall().once();
+        srManager.deviceConfiguration.addSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_UPDATED, RR2, RR1, Sets.newHashSet(RR2),
                 Sets.newHashSet(RR1));
         routeHandler.processRouteUpdated(re);
 
-        assertEquals(1, ROUTING_TABLE.size());
+        // Note: We shouldn't remove the old nexthop during the occasion of route update
+        //       since the populate subnet will take care of it and point it to an ECMP group
+        assertEquals(2, ROUTING_TABLE.size());
         MockRoutingTableValue rtv2 = ROUTING_TABLE.get(new MockRoutingTableKey(CP2.deviceId(), P1));
         assertEquals(M2, rtv2.macAddress);
         assertEquals(V2, rtv2.vlanId);
@@ -209,21 +231,37 @@
 
         assertEquals(1, SUBNET_TABLE.size());
         assertTrue(SUBNET_TABLE.get(CP2).contains(P1));
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void processRouteRemoved() throws Exception {
         processRouteAdded();
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.removeSubnet(CP1, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_REMOVED, RR1, Sets.newHashSet(RR1));
         routeHandler.processRouteRemoved(re);
 
         assertEquals(0, ROUTING_TABLE.size());
         assertEquals(0, SUBNET_TABLE.size());
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void testTwoSingleHomedAdded() throws Exception {
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.addSubnet(CP1, P1);
+        expectLastCall().once();
+        srManager.deviceConfiguration.addSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_ADDED, RR1, Sets.newHashSet(RR1, RR2));
         routeHandler.processRouteAdded(re);
 
@@ -240,10 +278,19 @@
         assertEquals(2, SUBNET_TABLE.size());
         assertTrue(SUBNET_TABLE.get(CP1).contains(P1));
         assertTrue(SUBNET_TABLE.get(CP2).contains(P1));
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void testOneDualHomedAdded() throws Exception {
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.addSubnet(CP1, P1);
+        expectLastCall().once();
+        srManager.deviceConfiguration.addSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_ADDED, RR3, Sets.newHashSet(RR3));
         routeHandler.processRouteAdded(re);
 
@@ -260,12 +307,19 @@
         assertEquals(2, SUBNET_TABLE.size());
         assertTrue(SUBNET_TABLE.get(CP1).contains(P1));
         assertTrue(SUBNET_TABLE.get(CP2).contains(P1));
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void testOneSingleHomedToTwoSingleHomed() throws Exception {
         processRouteAdded();
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.addSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ALTERNATIVE_ROUTES_CHANGED, RR1, null,
                 Sets.newHashSet(RR1, RR2), Sets.newHashSet(RR1));
         routeHandler.processAlternativeRoutesChanged(re);
@@ -283,17 +337,26 @@
         assertEquals(2, SUBNET_TABLE.size());
         assertTrue(SUBNET_TABLE.get(CP1).contains(P1));
         assertTrue(SUBNET_TABLE.get(CP2).contains(P1));
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void testTwoSingleHomedToOneSingleHomed() throws Exception {
         testTwoSingleHomedAdded();
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.removeSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ALTERNATIVE_ROUTES_CHANGED, RR1, null,
                 Sets.newHashSet(RR1), Sets.newHashSet(RR1, RR2));
         routeHandler.processAlternativeRoutesChanged(re);
 
-        assertEquals(1, ROUTING_TABLE.size());
+        // Note: We shouldn't remove the old nexthop during the occasion of route update
+        //       since the populate subnet will take care of it and point it to an ECMP group
+        assertEquals(2, ROUTING_TABLE.size());
         MockRoutingTableValue rtv1 = ROUTING_TABLE.get(new MockRoutingTableKey(CP1.deviceId(), P1));
         assertEquals(M1, rtv1.macAddress);
         assertEquals(V1, rtv1.vlanId);
@@ -301,8 +364,12 @@
 
         assertEquals(1, SUBNET_TABLE.size());
         assertTrue(SUBNET_TABLE.get(CP1).contains(P1));
+
+        verify(srManager.deviceConfiguration);
     }
 
+    // TODO Add test cases for two single homed next hop at same location
+
     @Test
     public void testDualHomedSingleLocationFail() throws Exception {
         testOneDualHomedAdded();
@@ -332,11 +399,20 @@
 
         hostService = new MockHostService(HOSTS_ONE_FAIL);
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.removeSubnet(CP1, P1);
+        expectLastCall().once();
+        srManager.deviceConfiguration.removeSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_REMOVED, RR3, Sets.newHashSet(RR3));
         routeHandler.processRouteRemoved(re);
 
         assertEquals(0, ROUTING_TABLE.size());
         assertEquals(0, SUBNET_TABLE.size());
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
@@ -345,21 +421,39 @@
 
         hostService = new MockHostService(HOSTS_BOTH_FAIL);
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.removeSubnet(CP1, P1);
+        expectLastCall().once();
+        srManager.deviceConfiguration.removeSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_REMOVED, RR1, Sets.newHashSet(RR1, RR2));
         routeHandler.processRouteRemoved(re);
 
         assertEquals(0, ROUTING_TABLE.size());
         assertEquals(0, SUBNET_TABLE.size());
+
+        verify(srManager.deviceConfiguration);
     }
 
     @Test
     public void testOneDualHomeRemoved() throws Exception {
         testOneDualHomedAdded();
 
+        reset(srManager.deviceConfiguration);
+        srManager.deviceConfiguration.removeSubnet(CP1, P1);
+        expectLastCall().once();
+        srManager.deviceConfiguration.removeSubnet(CP2, P1);
+        expectLastCall().once();
+        replay(srManager.deviceConfiguration);
+
         RouteEvent re = new RouteEvent(RouteEvent.Type.ROUTE_REMOVED, RR3, Sets.newHashSet(RR3));
         routeHandler.processRouteRemoved(re);
 
         assertEquals(0, ROUTING_TABLE.size());
         assertEquals(0, SUBNET_TABLE.size());
+
+        verify(srManager.deviceConfiguration);
     }
 }
\ No newline at end of file