Refactor the code so the populateSubnet is only called once per nexthop movement
Also make sure FPM and STATIC get processed first in this case
Change-Id: I9235b1c47452dc639ccef488442739bee302adbe
diff --git a/app/src/main/java/org/onosproject/segmentrouting/RouteHandler.java b/app/src/main/java/org/onosproject/segmentrouting/RouteHandler.java
index 7dbbbeb..b99f33f 100644
--- a/app/src/main/java/org/onosproject/segmentrouting/RouteHandler.java
+++ b/app/src/main/java/org/onosproject/segmentrouting/RouteHandler.java
@@ -33,6 +33,7 @@
import org.slf4j.LoggerFactory;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Collection;
import java.util.Objects;
@@ -235,91 +236,81 @@
VlanId hostVlanId = event.subject().vlan();
// map of nextId for prev port in each device
Map<DeviceId, Integer> nextIdMap = new HashMap<>();
+ Set<HostLocation> prevLocations = event.prevSubject().locations();
+ Set<HostLocation> newLocations = event.subject().locations();
+ Set<ConnectPoint> connectPoints = newLocations.stream().map(l -> (ConnectPoint) l).collect(Collectors.toSet());
+ List<Set<IpPrefix>> batchedSubnets =
+ srManager.deviceConfiguration.getBatchedSubnets(event.subject().id());
- affectedRoutes(hostMac, hostVlanId).forEach(affectedRoute -> {
- IpPrefix prefix = affectedRoute.prefix();
- Set<HostLocation> prevLocations = event.prevSubject().locations();
- Set<HostLocation> newLocations = event.subject().locations();
+ batchedSubnets.forEach(subnets -> {
+ log.debug("HostMoved. populateSubnet {}, {}", newLocations, subnets);
+ srManager.defaultRoutingHandler.populateSubnet(connectPoints, subnets);
- Set<ConnectPoint> connectPoints = newLocations.stream()
- .map(l -> (ConnectPoint) l).collect(Collectors.toSet());
- log.debug("HostMoved. populateSubnet {}, {}", newLocations, prefix);
- srManager.defaultRoutingHandler.populateSubnet(connectPoints, Sets.newHashSet(prefix));
+ subnets.forEach(prefix -> {
+ Set<DeviceId> newDeviceIds = newLocations.stream().map(HostLocation::deviceId)
+ .collect(Collectors.toSet());
- Set<DeviceId> newDeviceIds = newLocations.stream().map(HostLocation::deviceId)
- .collect(Collectors.toSet());
+ // Set of deviceIDs of the previous locations where the host was connected
+ // Used to determine if host moved to different connect points
+ // on same device or moved to a different device altogether
+ Set<DeviceId> oldDeviceIds = prevLocations.stream().map(HostLocation::deviceId)
+ .collect(Collectors.toSet());
- // Set of deviceIDs of the previous locations where the host was connected
- // Used to determine if host moved to different connect points
- // on same device or moved to a different device altogether
- Set<DeviceId> oldDeviceIds = prevLocations.stream().map(HostLocation::deviceId)
- .collect(Collectors.toSet());
+ // For each old location
+ Sets.difference(prevLocations, newLocations).forEach(prevLocation -> {
+ //find next Id for each old port, add to map
+ int nextId = srManager.getNextIdForHostPort(prevLocation.deviceId(), hostMac,
+ hostVlanId, prevLocation.port());
+ log.debug("HostMoved. NextId For Host {}, {}, {}, {} {}", prevLocation, prefix,
+ hostMac, hostVlanId, nextId);
- // For each old location
- Sets.difference(prevLocations, newLocations).forEach(prevLocation -> {
- //find next Id for each old port, add to map
- int nextId = srManager.getNextIdForHostPort(prevLocation.deviceId(), hostMac,
- hostVlanId, prevLocation.port());
- log.debug("HostMoved. NextId For Host {}, {}, {}, {} {}", prevLocation, prefix,
- hostMac, hostVlanId, nextId);
+ nextIdMap.put(prevLocation.deviceId(), nextId);
- nextIdMap.put(prevLocation.deviceId(), nextId);
+ // Remove flows for unchanged IPs only when the host moves from a switch to another.
+ // Otherwise, do not remove and let the adding part update the old flow
+ if (newDeviceIds.contains(prevLocation.deviceId())) {
+ return;
+ }
- // Remove flows for unchanged IPs only when the host moves from a switch to another.
- // Otherwise, do not remove and let the adding part update the old flow
- if (newDeviceIds.contains(prevLocation.deviceId())) {
- return;
- }
+ log.debug("HostMoved. removeSubnet {}, {}", prevLocation, prefix);
+ srManager.deviceConfiguration.removeSubnet(prevLocation, prefix);
- log.debug("HostMoved. removeSubnet {}, {}", prevLocation, prefix);
- srManager.deviceConfiguration.removeSubnet(prevLocation, prefix);
+ // Do not remove flow from a device if the route is still reachable via its pair device.
+ // populateSubnet will update the flow to point to its pair device via spine.
+ DeviceId pairDeviceId = srManager.getPairDeviceId(prevLocation.deviceId()).orElse(null);
+ if (newLocations.stream().anyMatch(n -> n.deviceId().equals(pairDeviceId))) {
+ return;
+ }
- // Do not remove flow from a device if the route is still reachable via its pair device.
- // populateSubnet will update the flow to point to its pair device via spine.
- DeviceId pairDeviceId = srManager.getPairDeviceId(prevLocation.deviceId()).orElse(null);
- if (newLocations.stream().anyMatch(n -> n.deviceId().equals(pairDeviceId))) {
- return;
- }
+ log.debug("HostMoved. revokeRoute {}, {}, {}, {}", prevLocation, prefix, hostMac, hostVlanId);
+ srManager.defaultRoutingHandler.revokeRoute(prevLocation.deviceId(), prefix,
+ hostMac, hostVlanId, prevLocation.port(), false);
+ });
- log.debug("HostMoved. revokeRoute {}, {}, {}, {}", prevLocation, prefix, hostMac, hostVlanId);
- srManager.defaultRoutingHandler.revokeRoute(prevLocation.deviceId(), prefix,
- hostMac, hostVlanId, prevLocation.port(), false);
+ // For each new location, add all new IPs.
+ Sets.difference(newLocations, prevLocations).forEach(newLocation -> {
+ log.debug("HostMoved. addSubnet {}, {}", newLocation, prefix);
+ srManager.deviceConfiguration.addSubnet(newLocation, prefix);
+
+ //its a new connect point, not a move from an existing device, populateRoute
+ if (!oldDeviceIds.contains(newLocation.deviceId())) {
+ log.debug("HostMoved. populateRoute {}, {}, {}, {}", newLocation, prefix, hostMac, hostVlanId);
+ srManager.defaultRoutingHandler.populateRoute(newLocation.deviceId(), prefix,
+ hostMac, hostVlanId, newLocation.port(), false);
+ } else {
+ // update same flow to point to new nextObj
+ VlanId vlanId = Optional.ofNullable(srManager.getInternalVlanId(newLocation)).orElse(hostVlanId);
+ //use nextIdMap to send new port details to update the nextId group bucket
+ log.debug("HostMoved. update L3 Ucast Group Bucket {}, {}, {} --> {}",
+ newLocation, hostMac, vlanId, nextIdMap.get(newLocation.deviceId()));
+ srManager.updateMacVlanTreatment(newLocation.deviceId(), hostMac, vlanId,
+ newLocation.port(), nextIdMap.get(newLocation.deviceId()));
+ }
+ });
});
-
- // For each new location, add all new IPs.
- Sets.difference(newLocations, prevLocations).forEach(newLocation -> {
- log.debug("HostMoved. addSubnet {}, {}", newLocation, prefix);
- srManager.deviceConfiguration.addSubnet(newLocation, prefix);
-
- //its a new connect point, not a move from an existing device, populateRoute
- if (!oldDeviceIds.contains(newLocation.deviceId())) {
- log.debug("HostMoved. populateRoute {}, {}, {}, {}", newLocation, prefix, hostMac, hostVlanId);
- srManager.defaultRoutingHandler.populateRoute(newLocation.deviceId(), prefix,
- hostMac, hostVlanId, newLocation.port(), false);
- } else {
- // update same flow to point to new nextObj
- VlanId vlanId = Optional.ofNullable(srManager.getInternalVlanId(newLocation)).orElse(hostVlanId);
- //use nextIdMap to send new port details to update the nextId group bucket
- log.debug("HostMoved. update L3 Ucast Group Bucket {}, {}, {} --> {}",
- newLocation, hostMac, vlanId, nextIdMap.get(newLocation.deviceId()));
- srManager.updateMacVlanTreatment(newLocation.deviceId(), hostMac, vlanId,
- newLocation.port(), nextIdMap.get(newLocation.deviceId()));
- }
- });
-
});
}
- private Set<ResolvedRoute> affectedRoutes(MacAddress mac, VlanId vlanId) {
- return srManager.routeService.getRouteTables().stream()
- .map(routeTableId -> srManager.routeService.getRoutes(routeTableId))
- .flatMap(Collection::stream)
- .map(RouteInfo::allRoutes)
- .flatMap(Collection::stream)
- .filter(resolvedRoute -> mac.equals(resolvedRoute.nextHopMac()) &&
- vlanId.equals(resolvedRoute.nextHopVlan())).collect(Collectors.toSet());
- }
-
private boolean isReady() {
return Objects.nonNull(srManager.deviceConfiguration) &&
Objects.nonNull(srManager.defaultRoutingHandler);
diff --git a/app/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java b/app/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
index f2a971b..81a161f 100644
--- a/app/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
+++ b/app/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
@@ -32,6 +32,7 @@
import org.onlab.packet.VlanId;
import org.onosproject.net.ConnectPoint;
import org.onosproject.net.DeviceId;
+import org.onosproject.net.HostId;
import org.onosproject.net.PortNumber;
import org.onosproject.net.config.ConfigException;
import org.onosproject.net.config.basics.InterfaceConfig;
@@ -53,6 +54,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import static com.google.common.base.Preconditions.checkNotNull;
@@ -512,6 +514,40 @@
}
/**
+ * Returns batches of all subnets reachable via given next hop
+ * <p>
+ * First batch includes FPM and STATIC routes
+ * Second batch includes all other type of routes obtained from routeService, including DHCP routes.
+ *
+ * @param hostId next hop host id
+ * @return list of subnet batches, each batch includes a set of prefixes.
+ */
+ // TODO Querying routeService directly may be expensive. Some kind of reverse lookup cache should be developed.
+ public List<Set<IpPrefix>> getBatchedSubnets(HostId hostId) {
+ Set<IpPrefix> high = Sets.newHashSet();
+ Set<IpPrefix> low = Sets.newHashSet();
+
+ srManager.routeService.getRouteTables().stream()
+ .map(tableId -> srManager.routeService.getResolvedRoutes(tableId))
+ .flatMap(Collection::stream)
+ .forEach(resolvedRoute -> {
+ // Continue if next hop is not what we are looking for
+ if (!Objects.equals(hostId.mac(), resolvedRoute.nextHopMac()) ||
+ !Objects.equals(hostId.vlanId(), resolvedRoute.nextHopVlan())) {
+ return;
+ }
+ // Prioritize STATIC and FPM among others
+ if (resolvedRoute.route().source() == Route.Source.STATIC ||
+ resolvedRoute.route().source() == Route.Source.FPM) {
+ high.add(resolvedRoute.prefix());
+ } else {
+ low.add(resolvedRoute.prefix());
+ }
+ });
+ return Stream.of(high, low).filter(set -> !set.isEmpty()).collect(Collectors.toList());
+ }
+
+ /**
* Returns batches of all subnets reachable on the given device.
* <p>
* First batch includes configured subnets, FPM and STATIC routes
diff --git a/app/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java b/app/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java
index ade5d0c..2d365a2 100644
--- a/app/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java
+++ b/app/src/test/java/org/onosproject/segmentrouting/RouteHandlerTest.java
@@ -18,6 +18,7 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import org.junit.Before;
@@ -407,6 +408,8 @@
ROUTE_STORE.put(P1, Sets.newHashSet(RR3));
reset(srManager.deviceConfiguration);
+ expect(srManager.deviceConfiguration.getBatchedSubnets(H3D.id()))
+ .andReturn(Lists.<Set<IpPrefix>>newArrayList(Sets.newHashSet(P1)));
srManager.deviceConfiguration.removeSubnet(CP2, P1);
expectLastCall().once();
replay(srManager.deviceConfiguration);
@@ -455,6 +458,8 @@
testDualHomedSingleLocationFail();
reset(srManager.deviceConfiguration);
+ expect(srManager.deviceConfiguration.getBatchedSubnets(H3S.id()))
+ .andReturn(Lists.<Set<IpPrefix>>newArrayList(Sets.newHashSet(P1)));
srManager.deviceConfiguration.addSubnet(CP2, P1);
expectLastCall().once();
replay(srManager.deviceConfiguration);