ONOS-1438: Segment Routing rule population optimization fixes
Change-Id: I2cad2cd485282b904e035b209530005b93c90ffd
diff --git a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index 14187c0..7e8cb4c 100644
--- a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -184,21 +184,40 @@
private boolean repopulateRoutingRulesForRoutes(Set<ArrayList<DeviceId>> routes) {
rulePopulator.resetCounter();
+ HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> routesBydevice =
+ new HashMap<>();
for (ArrayList<DeviceId> link: routes) {
// When only the source device is defined, reinstall routes to all other devices
if (link.size() == 1) {
log.trace("repopulateRoutingRulesForRoutes: running ECMP graph for device {}", link.get(0));
ECMPShortestPathGraph ecmpSpg = new ECMPShortestPathGraph(link.get(0), srManager);
if (populateEcmpRoutingRules(link.get(0), ecmpSpg)) {
+ log.debug("Populating flow rules from {} to all is successful",
+ link.get(0));
currentEcmpSpgMap.put(link.get(0), ecmpSpg);
} else {
log.warn("Failed to populate the flow rules from {} to all", link.get(0));
return false;
}
} else {
+ ArrayList<ArrayList<DeviceId>> deviceRoutes =
+ routesBydevice.get(link.get(1));
+ if (deviceRoutes == null) {
+ deviceRoutes = new ArrayList<>();
+ routesBydevice.put(link.get(1), deviceRoutes);
+ }
+ deviceRoutes.add(link);
+ }
+ }
+
+ for (DeviceId impactedDevice : routesBydevice.keySet()) {
+ ArrayList<ArrayList<DeviceId>> deviceRoutes =
+ routesBydevice.get(impactedDevice);
+ for (ArrayList<DeviceId> link: deviceRoutes) {
+ log.debug("repopulate RoutingRules For Routes {} -> {}",
+ link.get(0), link.get(1));
DeviceId src = link.get(0);
DeviceId dst = link.get(1);
- log.trace("repopulateRoutingRulesForRoutes: running ECMP graph for device {}", dst);
ECMPShortestPathGraph ecmpSpg = updatedEcmpSpgMap.get(dst);
HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
ecmpSpg.getAllLearnedSwitchesAndVia();
@@ -220,10 +239,18 @@
if (!populateEcmpRoutingRulePartial(targetSw, dst, nextHops)) {
return false;
}
+ log.debug("Populating flow rules from {} to {} is successful",
+ targetSw, dst);
}
}
- currentEcmpSpgMap.put(dst, ecmpSpg);
+ //currentEcmpSpgMap.put(dst, ecmpSpg);
}
+ //Only if all the flows for all impacted routes to a
+ //specific target are pushed successfully, update the
+ //ECMP graph for that target. (Or else the next event
+ //would not see any changes in the ECMP graphs)
+ currentEcmpSpgMap.put(impactedDevice,
+ updatedEcmpSpgMap.get(impactedDevice));
}
return true;
}
@@ -233,13 +260,15 @@
Set<ArrayList<DeviceId>> routes = new HashSet<>();
for (Device sw : srManager.deviceService.getDevices()) {
+ log.debug("Computing the impacted routes for device {} due to link fail",
+ sw.id());
if (srManager.mastershipService.
getLocalRole(sw.id()) != MastershipRole.MASTER) {
continue;
}
ECMPShortestPathGraph ecmpSpg = currentEcmpSpgMap.get(sw.id());
if (ecmpSpg == null) {
- log.error("No existing ECMP path for switch {}", sw.id());
+ log.error("No existing ECMP graph for switch {}", sw.id());
continue;
}
HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
@@ -252,8 +281,12 @@
Set<ArrayList<DeviceId>> subLinks =
computeLinks(targetSw, destSw, swViaMap);
for (ArrayList<DeviceId> alink: subLinks) {
- if (alink.get(0).equals(linkFail.src().deviceId()) &&
- alink.get(1).equals(linkFail.dst().deviceId())) {
+ if ((alink.get(0).equals(linkFail.src().deviceId()) &&
+ alink.get(1).equals(linkFail.dst().deviceId()))
+ ||
+ (alink.get(0).equals(linkFail.dst().deviceId()) &&
+ alink.get(1).equals(linkFail.src().deviceId()))) {
+ log.debug("Impacted route:{}->{}", targetSw, destSw);
ArrayList<DeviceId> aRoute = new ArrayList<>();
aRoute.add(targetSw);
aRoute.add(destSw);
@@ -274,9 +307,12 @@
Set<ArrayList<DeviceId>> routes = new HashSet<>();
for (Device sw : srManager.deviceService.getDevices()) {
+ log.debug("Computing the impacted routes for device {}",
+ sw.id());
if (srManager.mastershipService.
getLocalRole(sw.id()) != MastershipRole.MASTER) {
- log.warn("No mastership for {} and skip route optimization");
+ log.debug("No mastership for {} and skip route optimization",
+ sw.id());
continue;
}
@@ -295,7 +331,7 @@
continue;
}
ECMPShortestPathGraph newEcmpSpg = updatedEcmpSpgMap.get(sw.id());
- currentEcmpSpgMap.put(sw.id(), newEcmpSpg);
+ //currentEcmpSpgMap.put(sw.id(), newEcmpSpg);
HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
ecmpSpg.getAllLearnedSwitchesAndVia();
HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchViaUpdated =
@@ -307,7 +343,8 @@
for (DeviceId srcSw : swViaMapUpdated.keySet()) {
ArrayList<ArrayList<DeviceId>> viaUpdated = swViaMapUpdated.get(srcSw);
ArrayList<ArrayList<DeviceId>> via = getVia(switchVia, srcSw);
- if (via.isEmpty() || !viaUpdated.equals(via)) {
+ if ((via == null) || !viaUpdated.equals(via)) {
+ log.debug("Impacted route:{}->{}", srcSw, sw.id());
ArrayList<DeviceId> route = new ArrayList<>();
route.add(srcSw);
route.add(sw.id());
@@ -318,7 +355,7 @@
}
for (ArrayList<DeviceId> link: routes) {
- log.trace("Link changes - ");
+ log.trace("Route changes - ");
if (link.size() == 1) {
log.trace(" : {} - all", link.get(0));
} else {
@@ -341,7 +378,7 @@
}
}
- return new ArrayList<>();
+ return null;
}
private Set<ArrayList<DeviceId>> computeLinks(DeviceId src,
diff --git a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 23eb668..41cb65b 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -133,7 +133,9 @@
private NetworkConfigManager networkConfigService = new NetworkConfigManager();;
- private static int numOfEvents = 0;
+ private Object threadSchedulerLock = new Object();
+ private static int numOfEventsQueued = 0;
+ private static int numOfEventsExecuted = 0;
private static int numOfHandlerExecution = 0;
private static int numOfHandlerScheduled = 0;
@@ -325,6 +327,7 @@
public void event(LinkEvent event) {
if (event.type() == LinkEvent.Type.LINK_ADDED
|| event.type() == LinkEvent.Type.LINK_REMOVED) {
+ log.debug("Event {} received from Link Service", event.type());
scheduleEventHandlerIfNotScheduled(event);
}
}
@@ -346,6 +349,7 @@
case PORT_REMOVED:
case DEVICE_UPDATED:
case DEVICE_AVAILABILITY_CHANGED:
+ log.debug("Event {} received from Device Service", event.type());
scheduleEventHandlerIfNotScheduled(event);
break;
default:
@@ -355,19 +359,20 @@
private void scheduleEventHandlerIfNotScheduled(Event event) {
- synchronized (eventQueue) {
+ synchronized (threadSchedulerLock) {
eventQueue.add(event);
- numOfEvents++;
- if (eventHandlerFuture == null || eventHandlerFuture.isDone()) {
+ numOfEventsQueued++;
+
+ if ((numOfHandlerScheduled - numOfHandlerExecution) == 0) {
+ //No pending scheduled event handling threads. So start a new one.
eventHandlerFuture = executorService
.schedule(eventHandler, 100, TimeUnit.MILLISECONDS);
numOfHandlerScheduled++;
}
+ log.trace("numOfEventsQueued {}, numOfEventHanlderScheduled {}",
+ numOfEventsQueued,
+ numOfHandlerScheduled);
}
-
- log.trace("numOfEvents {}, numOfEventHanlderScheduled {}", numOfEvents,
- numOfHandlerScheduled);
-
}
private class InternalEventHandler implements Runnable {
@@ -375,32 +380,38 @@
@Override
public void run() {
try {
- synchronized (eventQueue) {
- numOfHandlerExecution++;
- while (!eventQueue.isEmpty()) {
- Event event = eventQueue.poll();
- if (event.type() == LinkEvent.Type.LINK_ADDED) {
- processLinkAdded((Link) event.subject());
- } else if (event.type() == LinkEvent.Type.LINK_REMOVED) {
- processLinkRemoved((Link) event.subject());
- //} else if (event.type() == GroupEvent.Type.GROUP_ADDED) {
- // processGroupAdded((Group) event.subject());
- } else if (event.type() == DeviceEvent.Type.DEVICE_ADDED ||
- event.type() == DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED ||
- event.type() == DeviceEvent.Type.DEVICE_UPDATED) {
- if (deviceService.isAvailable(((Device) event.subject()).id())) {
- processDeviceAdded((Device) event.subject());
- }
- } else if (event.type() == DeviceEvent.Type.PORT_REMOVED) {
- processPortRemoved((Device) event.subject(),
- ((DeviceEvent) event).port());
+ while (true) {
+ Event event = null;
+ synchronized (threadSchedulerLock) {
+ if (!eventQueue.isEmpty()) {
+ event = eventQueue.poll();
+ numOfEventsExecuted++;
} else {
- log.warn("Unhandled event type: {}", event.type());
+ numOfHandlerExecution++;
+ log.debug("numOfHandlerExecution {} numOfEventsExecuted {}",
+ numOfHandlerExecution, numOfEventsExecuted);
+ break;
}
}
+ if (event.type() == LinkEvent.Type.LINK_ADDED) {
+ processLinkAdded((Link) event.subject());
+ } else if (event.type() == LinkEvent.Type.LINK_REMOVED) {
+ processLinkRemoved((Link) event.subject());
+ //} else if (event.type() == GroupEvent.Type.GROUP_ADDED) {
+ // processGroupAdded((Group) event.subject());
+ } else if (event.type() == DeviceEvent.Type.DEVICE_ADDED ||
+ event.type() == DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED ||
+ event.type() == DeviceEvent.Type.DEVICE_UPDATED) {
+ if (deviceService.isAvailable(((Device) event.subject()).id())) {
+ processDeviceAdded((Device) event.subject());
+ }
+ } else if (event.type() == DeviceEvent.Type.PORT_REMOVED) {
+ processPortRemoved((Device) event.subject(),
+ ((DeviceEvent) event).port());
+ } else {
+ log.warn("Unhandled event type: {}", event.type());
+ }
}
- log.debug("numOfHandlerExecution {} numOfEventHanlderScheduled {} numOfEvents {}",
- numOfHandlerExecution, numOfHandlerScheduled, numOfEvents);
} catch (Exception e) {
log.error("SegmentRouting event handler "
+ "thread thrown an exception: {}", e);
@@ -433,9 +444,10 @@
}
}
- //defaultRoutingHandler.populateRoutingRulesForLinkStatusChange(null);
- log.trace("processLinkAdded: re-starting route population process");
- defaultRoutingHandler.startPopulationProcess();
+ log.trace("Starting optimized route population process");
+ defaultRoutingHandler.populateRoutingRulesForLinkStatusChange(null);
+ //log.trace("processLinkAdded: re-starting route population process");
+ //defaultRoutingHandler.startPopulationProcess();
}
private void processLinkRemoved(Link link) {
@@ -444,9 +456,10 @@
if (groupHandler != null) {
groupHandler.portDown(link.src().port());
}
- //defaultRoutingHandler.populateRoutingRulesForLinkStatusChange(link);
- log.trace("processLinkRemoved: re-starting route population process");
- defaultRoutingHandler.startPopulationProcess();
+ log.trace("Starting optimized route population process");
+ defaultRoutingHandler.populateRoutingRulesForLinkStatusChange(link);
+ //log.trace("processLinkRemoved: re-starting route population process");
+ //defaultRoutingHandler.startPopulationProcess();
}
private void processDeviceAdded(Device device) {