Implemented CORD-1843 to avoid race condition when updating ECMPspgs.
In this commit:
- a new mechanism in DefaultRoutingHandler to update route-path maps in all instances,
for the entire topology, after every route event has been processesed.
- fixed a race condition in LinkHandler
- avoids retrying flows in the ofdpa3 driver as this issue has been fixed in the switch
- a new CLI command to check internal link state
Change-Id: I307d0a96cc46569294d15d042b3bcb1fde735f1b
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index 04e34bf..879abc0 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -52,7 +52,6 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
-
import static com.google.common.base.MoreObjects.toStringHelper;
import static com.google.common.base.Preconditions.checkNotNull;
import static java.util.concurrent.Executors.newScheduledThreadPool;
@@ -67,7 +66,6 @@
private static final int RETRY_INTERVAL_MS = 250;
private static final int RETRY_INTERVAL_SCALE = 1;
private static final long STABLITY_THRESHOLD = 10; //secs
- private static final int UPDATE_INTERVAL = 5; //secs
private static Logger log = LoggerFactory.getLogger(DefaultRoutingHandler.class);
private SegmentRoutingManager srManager;
@@ -261,8 +259,17 @@
rulePopulator.resetCounter();
log.info("Starting to populate routing rules for added routes, subnets={}, cpts={}",
subnets, cpts);
- // Take snapshots of the topology
- updatedEcmpSpgMap = new HashMap<>();
+ // In principle an update to a subnet/prefix should not require a
+ // new ECMPspg calculation as it is not a topology event. As a
+ // result, we use the current/existing ECMPspg in the updated map
+ // used by the redoRouting method.
+ currentEcmpSpgMap.entrySet().forEach(entry -> {
+ updatedEcmpSpgMap.put(entry.getKey(), entry.getValue());
+ if (log.isDebugEnabled()) {
+ log.debug("Root switch: {}", entry.getKey());
+ log.debug(" Current/Existing SPG: {}", entry.getValue());
+ }
+ });
Set<EdgePair> edgePairs = new HashSet<>();
Set<ArrayList<DeviceId>> routeChanges = new HashSet<>();
boolean handleRouting = false;
@@ -281,9 +288,13 @@
return;
}
for (ConnectPoint cp : cpts) {
- EcmpShortestPathGraph ecmpSpgUpdated =
+ if (updatedEcmpSpgMap.get(cp.deviceId()) == null) {
+ EcmpShortestPathGraph ecmpSpgUpdated =
new EcmpShortestPathGraph(cp.deviceId(), srManager);
- updatedEcmpSpgMap.put(cp.deviceId(), ecmpSpgUpdated);
+ updatedEcmpSpgMap.put(cp.deviceId(), ecmpSpgUpdated);
+ log.warn("populateSubnet: no updated graph for dev:{}"
+ + " ... creating", cp.deviceId());
+ }
DeviceId retId = shouldHandleRouting(cp.deviceId());
if (retId == null) {
continue;
@@ -293,9 +304,13 @@
} else {
// single connect point
DeviceId dstSw = cpts.iterator().next().deviceId();
- EcmpShortestPathGraph ecmpSpgUpdated =
+ if (updatedEcmpSpgMap.get(dstSw) == null) {
+ EcmpShortestPathGraph ecmpSpgUpdated =
new EcmpShortestPathGraph(dstSw, srManager);
- updatedEcmpSpgMap.put(dstSw, ecmpSpgUpdated);
+ updatedEcmpSpgMap.put(dstSw, ecmpSpgUpdated);
+ log.warn("populateSubnet: no updated graph for dev:{}"
+ + " ... creating", dstSw);
+ }
if (srManager.mastershipService.isLocalMaster(dstSw)) {
handleRouting = true;
}
@@ -375,14 +390,12 @@
return;
}
lastRoutingChange = DateTime.now();
- executorService.schedule(new UpdateMaps(), UPDATE_INTERVAL,
- TimeUnit.SECONDS);
statusLock.lock();
try {
if (populationStatus == Status.STARTED) {
log.warn("Previous rule population is not finished. Cannot"
- + " proceeed with routingRules for Link Status change");
+ + " proceeed with routingRules for Topology change");
return;
}
@@ -402,13 +415,14 @@
}
}
- log.info("Starting to populate routing rules from link status change");
+ log.info("Starting to populate routing rules from Topology change");
Set<ArrayList<DeviceId>> routeChanges;
log.debug("populateRoutingRulesForLinkStatusChange: "
+ "populationStatus is STARTED");
populationStatus = Status.STARTED;
- rulePopulator.resetCounter();
+ rulePopulator.resetCounter(); //XXX maybe useful to have a rehash ctr
+ boolean hashGroupsChanged = false;
// try optimized re-routing
if (linkDown == null) {
// either a linkUp or a switchDown - compute all route changes by
@@ -429,6 +443,7 @@
processHashGroupChange(routeChanges, false, null);
// clear out routesChanges so a re-route is not attempted
routeChanges = ImmutableSet.of();
+ hashGroupsChanged = true;
}
// for a linkUp of a never-seen-before link
// let it fall through to a reroute of the routeChanges
@@ -444,6 +459,7 @@
processHashGroupChange(routeChanges, true, switchDown);
// clear out routesChanges so a re-route is not attempted
routeChanges = ImmutableSet.of();
+ hashGroupsChanged = true;
}
} else {
// link has gone down
@@ -453,19 +469,29 @@
processHashGroupChange(routeChanges, true, null);
// clear out routesChanges so a re-route is not attempted
routeChanges = ImmutableSet.of();
+ hashGroupsChanged = true;
}
}
// do full re-routing if optimized routing returns null routeChanges
if (routeChanges == null) {
- log.info("Optimized routing failed... opting for full reroute");
+ log.warn("Optimized routing failed... opting for full reroute");
populationStatus = Status.ABORTED;
populateAllRoutingRules();
return;
}
if (routeChanges.isEmpty()) {
- log.info("No re-route attempted for the link status change");
+ if (hashGroupsChanged) {
+ log.info("Hash-groups changed for link status change");
+ } else {
+ log.info("No re-route or re-hash attempted for the link"
+ + " status change");
+ updatedEcmpSpgMap.keySet().forEach(devId -> {
+ currentEcmpSpgMap.put(devId, updatedEcmpSpgMap.get(devId));
+ log.debug("Updating ECMPspg for remaining dev:{}", devId);
+ });
+ }
log.debug("populateRoutingRulesForLinkStatusChange: populationStatus is SUCCEEDED");
populationStatus = Status.SUCCEEDED;
return;
@@ -489,7 +515,6 @@
}
}
-
/**
* Processes a set a route-path changes by reprogramming routing rules and
* creating new hash-groups or editing them if necessary. This method also
@@ -537,7 +562,9 @@
}
// whatever is left in changedRoutes is now processed for individual dsts.
- if (!redoRoutingIndividualDests(subnets, changedRoutes)) {
+ Set<DeviceId> updatedDevices = Sets.newHashSet();
+ if (!redoRoutingIndividualDests(subnets, changedRoutes,
+ updatedDevices)) {
return false; //abort routing and fail fast
}
@@ -547,6 +574,15 @@
currentEcmpSpgMap.put(ep.dev2, updatedEcmpSpgMap.get(ep.dev2));
log.debug("Updating ECMPspg for edge-pair:{}-{}", ep.dev1, ep.dev2);
}
+
+ // here is where we update all devices not touched by this instance
+ updatedEcmpSpgMap.keySet().stream()
+ .filter(devId -> !edgePairs.stream().anyMatch(ep -> ep.includes(devId)))
+ .filter(devId -> !updatedDevices.contains(devId))
+ .forEach(devId -> {
+ currentEcmpSpgMap.put(devId, updatedEcmpSpgMap.get(devId));
+ log.debug("Updating ECMPspg for remaining dev:{}", devId);
+ });
return true;
}
@@ -621,9 +657,14 @@
: subnets;
ipDev1 = (ipDev1 == null) ? Sets.newHashSet() : ipDev1;
ipDev2 = (ipDev2 == null) ? Sets.newHashSet() : ipDev2;
+ Set<DeviceId> nhDev1 = perDstNextHops.get(ep.dev1);
+ Set<DeviceId> nhDev2 = perDstNextHops.get(ep.dev2);
// handle routing to subnets common to edge-pair
- // only if the targetSw is not part of the edge-pair
- if (!ep.includes(targetSw)) {
+ // only if the targetSw is not part of the edge-pair and there
+ // exists a next hop to at least one of the devices in the edge-pair
+ if (!ep.includes(targetSw)
+ && ((nhDev1 != null && !nhDev1.isEmpty())
+ || (nhDev2 != null && !nhDev2.isEmpty()))) {
if (!populateEcmpRoutingRulePartial(
targetSw,
ep.dev1, ep.dev2,
@@ -632,11 +673,13 @@
return false; // abort everything and fail fast
}
}
- // handle routing to subnets that only belong to dev1
+ // handle routing to subnets that only belong to dev1 only if
+ // a next-hop exists from the target to dev1
Set<IpPrefix> onlyDev1Subnets = Sets.difference(ipDev1, ipDev2);
- if (!onlyDev1Subnets.isEmpty() && perDstNextHops.get(ep.dev1) != null) {
+ if (!onlyDev1Subnets.isEmpty()
+ && nhDev1 != null && !nhDev1.isEmpty()) {
Map<DeviceId, Set<DeviceId>> onlyDev1NextHops = new HashMap<>();
- onlyDev1NextHops.put(ep.dev1, perDstNextHops.get(ep.dev1));
+ onlyDev1NextHops.put(ep.dev1, nhDev1);
if (!populateEcmpRoutingRulePartial(
targetSw,
ep.dev1, null,
@@ -645,11 +688,13 @@
return false; // abort everything and fail fast
}
}
- // handle routing to subnets that only belong to dev2
+ // handle routing to subnets that only belong to dev2 only if
+ // a next-hop exists from the target to dev2
Set<IpPrefix> onlyDev2Subnets = Sets.difference(ipDev2, ipDev1);
- if (!onlyDev2Subnets.isEmpty() && perDstNextHops.get(ep.dev2) != null) {
+ if (!onlyDev2Subnets.isEmpty()
+ && nhDev2 != null && !nhDev2.isEmpty()) {
Map<DeviceId, Set<DeviceId>> onlyDev2NextHops = new HashMap<>();
- onlyDev2NextHops.put(ep.dev2, perDstNextHops.get(ep.dev2));
+ onlyDev2NextHops.put(ep.dev2, nhDev2);
if (!populateEcmpRoutingRulePartial(
targetSw,
ep.dev2, null,
@@ -680,7 +725,8 @@
* @return true if successful
*/
private boolean redoRoutingIndividualDests(Set<IpPrefix> subnets,
- Set<ArrayList<DeviceId>> changedRoutes) {
+ Set<ArrayList<DeviceId>> changedRoutes,
+ Set<DeviceId> updatedDevices) {
// aggregate route-path changes for each dst device
HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> routesBydevice =
new HashMap<>();
@@ -726,6 +772,7 @@
//is updated here, without any flows being pushed.
currentEcmpSpgMap.put(impactedDstDevice,
updatedEcmpSpgMap.get(impactedDstDevice));
+ updatedDevices.add(impactedDstDevice);
log.debug("Updating ECMPspg for impacted dev:{}", impactedDstDevice);
}
return true;
@@ -873,7 +920,8 @@
changedRoutes.add(route);
}
}
-
+ boolean someFailed = false;
+ Set<DeviceId> updatedDevices = Sets.newHashSet();
for (ArrayList<DeviceId> route : changedRoutes) {
DeviceId targetSw = route.get(0);
DeviceId dstSw = route.get(1);
@@ -887,14 +935,20 @@
currentEcmpSpgMap.remove(targetSw);
log.debug("Updating ECMPspg for dst:{} removing failed switch "
+ "target:{}", dstSw, targetSw);
+ updatedDevices.add(targetSw);
+ updatedDevices.add(dstSw);
continue;
}
//linkfailed - update both sides
if (success) {
currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
- log.debug("Updating ECMPspg for dst:{} and target:{} for linkdown",
- dstSw, targetSw);
+ log.debug("Updating ECMPspg for dst:{} and target:{} for linkdown"
+ + " or switchdown", dstSw, targetSw);
+ updatedDevices.add(targetSw);
+ updatedDevices.add(dstSw);
+ } else {
+ someFailed = true;
}
} else {
//linkup of seen before link
@@ -904,9 +958,22 @@
currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
log.debug("Updating ECMPspg for target:{} and dst:{} for linkup",
targetSw, dstSw);
+ updatedDevices.add(targetSw);
+ updatedDevices.add(dstSw);
+ } else {
+ someFailed = true;
}
}
}
+ if (!someFailed) {
+ // here is where we update all devices not touched by this instance
+ updatedEcmpSpgMap.keySet().stream()
+ .filter(devId -> !updatedDevices.contains(devId))
+ .forEach(devId -> {
+ currentEcmpSpgMap.put(devId, updatedEcmpSpgMap.get(devId));
+ log.debug("Updating ECMPspg for remaining dev:{}", devId);
+ });
+ }
}
/**
@@ -1027,9 +1094,21 @@
* @param deviceId the device for which graphs need to be purged
*/
protected void purgeEcmpGraph(DeviceId deviceId) {
- currentEcmpSpgMap.remove(deviceId); // XXX reconsider
- if (updatedEcmpSpgMap != null) {
- updatedEcmpSpgMap.remove(deviceId);
+ statusLock.lock();
+ try {
+
+ if (populationStatus == Status.STARTED) {
+ log.warn("Previous rule population is not finished. Cannot"
+ + " proceeed with purgeEcmpGraph for {}", deviceId);
+ return;
+ }
+ log.debug("Updating ECMPspg for unavailable dev:{}", deviceId);
+ currentEcmpSpgMap.remove(deviceId);
+ if (updatedEcmpSpgMap != null) {
+ updatedEcmpSpgMap.remove(deviceId);
+ }
+ } finally {
+ statusLock.unlock();
}
}
@@ -1424,35 +1503,6 @@
}
}
- /**
- * Updates the currentEcmpSpgGraph for all devices.
- */
- private void updateEcmpSpgMaps() {
- for (Device sw : srManager.deviceService.getDevices()) {
- EcmpShortestPathGraph ecmpSpgUpdated =
- new EcmpShortestPathGraph(sw.id(), srManager);
- currentEcmpSpgMap.put(sw.id(), ecmpSpgUpdated);
- }
- }
-
- /**
- * Ensures routing is stable before updating all ECMP SPG graphs.
- *
- * TODO: CORD-1843 will ensure maps are updated faster, potentially while
- * topology/routing is still unstable
- */
- private final class UpdateMaps implements Runnable {
- @Override
- public void run() {
- if (isRoutingStable()) {
- updateEcmpSpgMaps();
- } else {
- executorService.schedule(new UpdateMaps(), UPDATE_INTERVAL,
- TimeUnit.SECONDS);
- }
- }
- }
-
//////////////////////////////////////
// Filtering rule creation
//////////////////////////////////////
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/LinkHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/LinkHandler.java
index ef9fb38..2961e50 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/LinkHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/LinkHandler.java
@@ -20,6 +20,8 @@
import java.util.Map.Entry;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+
import org.onosproject.net.Device;
import org.onosproject.net.DeviceId;
import org.onosproject.net.HostLocation;
@@ -35,6 +37,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
public class LinkHandler {
@@ -43,15 +46,11 @@
protected LinkService linkService;
// Local store for all links seen and their present status, used for
- // optimized routing. The existence of the link in the keys is enough to
- // know
+ // optimized routing. The existence of the link in the keys is enough to know
// if the link has been "seen-before" by this instance of the controller.
// The boolean value indicates if the link is currently up or not.
- // XXX Currently the optimized routing logic depends on "forgetting" a link
- // when a switch goes down, but "remembering" it when only the link goes
- // down.
- // Consider changing this logic so we can use the Link Service instead of
- // a local cache.
+ // Currently the optimized routing logic depends on "forgetting" a link
+ // when a switch goes down, but "remembering" it when only the link goes down.
private Map<Link, Boolean> seenLinks = new ConcurrentHashMap<>();
private EventuallyConsistentMap<DeviceId, Set<PortNumber>> downedPortStore = null;
@@ -106,8 +105,7 @@
}
// Irrespective of whether the local is a MASTER or not for this device,
- // create group handler instance and push default TTP flow rules if
- // needed,
+ // create group handler instance and push default TTP flow rules if needed,
// as in a multi-instance setup, instances can initiate groups for any
// device.
DefaultGroupHandler groupHandler = srManager.groupHandlerMap
@@ -151,12 +149,9 @@
// handle edge-ports for dual-homed hosts
updateDualHomedHostPorts(link, true);
- // It's possible that linkUp causes no route-path change as ECMP
- // graph does
+ // It's possible that linkUp causes no route-path change as ECMP graph does
// not change if the link is a parallel link (same src-dst as
- // another link.
- // However we still need to update ECMP hash groups to include new
- // buckets
+ // another link). However we still need to update ECMP hash groups to include new buckets
// for the link that has come up.
if (groupHandler != null) {
if (!seenBefore && isParallelLink(link)) {
@@ -193,6 +188,10 @@
// when removing links, update seen links first, before doing route-path
// changes
updateSeenLink(link, false);
+ // handle edge-ports for dual-homed hosts
+ if (srManager.mastershipService.isLocalMaster(link.src().deviceId())) {
+ updateDualHomedHostPorts(link, false);
+ }
// device availability check helps to ensure that multiple link-removed
// events are actually treated as a single switch removed event.
@@ -209,11 +208,6 @@
return;
}
- // handle edge-ports for dual-homed hosts
- if (srManager.mastershipService.isLocalMaster(link.src().deviceId())) {
- updateDualHomedHostPorts(link, false);
- }
-
log.debug("Starting optimized route-path processing for removed link "
+ "{} --> {}", link.src(), link.dst());
srManager.defaultRoutingHandler
@@ -313,7 +307,7 @@
* @param added true if link was added, false if link was removed
*/
private void updateDualHomedHostPorts(Link link, boolean added) {
- if (!lastUplink(link)) {
+ if (!onlyUplink(link)) {
return;
}
if (added) {
@@ -349,7 +343,7 @@
}
/**
- * Returns true if given link is the last active uplink from src-device of
+ * Returns true if given link is the only active uplink from src-device of
* link. An uplink is defined as a unidirectional link with src as
* edgeRouter and dst as non-edgeRouter.
*
@@ -357,28 +351,35 @@
* @return true if given link is-the-first/was-the-last uplink from the src
* device
*/
- private boolean lastUplink(Link link) {
+ private boolean onlyUplink(Link link) {
DeviceConfiguration devConfig = srManager.deviceConfiguration;
try {
- if (!devConfig.isEdgeDevice(link.src().deviceId())) {
+ if (!devConfig.isEdgeDevice(link.src().deviceId())
+ || devConfig.isEdgeDevice(link.dst().deviceId())) {
return false;
}
- Set<Link> devLinks = srManager.linkService
- .getDeviceLinks(link.src().deviceId());
- boolean foundOtherUplink = false;
+ // note that using linkservice here would cause race conditions as
+ // more links can show up while the app is still processing the first one
+ Set<Link> devLinks = seenLinks.entrySet().stream()
+ .filter(entry -> entry.getKey().src().deviceId()
+ .equals(link.src().deviceId()))
+ .filter(entry -> entry.getValue())
+ .filter(entry -> !entry.getKey().equals(link))
+ .map(entry -> entry.getKey())
+ .collect(Collectors.toSet());
+
for (Link l : devLinks) {
- if (devConfig.isEdgeDevice(l.dst().deviceId()) || l.equals(link)
- || l.state() == Link.State.INACTIVE) {
+ if (devConfig.isEdgeDevice(l.dst().deviceId())) {
continue;
}
- foundOtherUplink = true;
- break;
+ log.debug("Link {} is not the only active uplink. Found another"
+ + "link {}", link, l);
+ return false;
}
- if (!foundOtherUplink) {
- return true;
- }
+ log.debug("Link {} is the only uplink", link);
+ return true;
} catch (DeviceConfigNotFoundException e) {
- log.warn("Unable to determine if link is last uplink"
+ log.warn("Unable to determine if link is only uplink"
+ e.getMessage());
}
return false;
@@ -560,4 +561,12 @@
downedPortStore.put(loc.deviceId(), p);
}
+ ImmutableMap<Link, Boolean> getSeenLinks() {
+ return ImmutableMap.copyOf(seenLinks);
+ }
+
+ ImmutableMap<DeviceId, Set<PortNumber>> getDownedPorts() {
+ return ImmutableMap.copyOf(downedPortStore.entrySet());
+ }
+
}
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 4b86d13..8e3b55e 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -576,6 +576,16 @@
}
}
+ @Override
+ public ImmutableMap<Link, Boolean> getSeenLinks() {
+ return linkHandler.getSeenLinks();
+ }
+
+ @Override
+ public ImmutableMap<DeviceId, Set<PortNumber>> getDownedPortState() {
+ return linkHandler.getDownedPorts();
+ }
+
/**
* Extracts the application ID from the manager.
*
@@ -1093,12 +1103,12 @@
if (gh != null) {
gh.shutdown();
}
- defaultRoutingHandler.purgeEcmpGraph(device.id());
// Note that a switch going down is associated with all of its links
// going down as well, but it is treated as a single switch down event
// while the link-downs are ignored.
defaultRoutingHandler
.populateRoutingRulesForLinkStatusChange(null, null, device.id());
+ defaultRoutingHandler.purgeEcmpGraph(device.id());
mcastHandler.removeDevice(device.id());
xConnectHandler.removeDevice(device.id());
}
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingService.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingService.java
index 3668ba2..3f5a3b8 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingService.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingService.java
@@ -17,6 +17,8 @@
import org.onlab.packet.IpPrefix;
import org.onosproject.net.DeviceId;
+import org.onosproject.net.Link;
+import org.onosproject.net.PortNumber;
import org.onosproject.segmentrouting.grouphandler.NextNeighbors;
import org.onosproject.segmentrouting.storekey.DestinationSetNextObjectiveStoreKey;
@@ -143,4 +145,20 @@
* @param id the device identifier
*/
void verifyGroups(DeviceId id);
+
+ /**
+ * Returns the internal link state as seen by this instance of the
+ * controller.
+ *
+ * @return the internal link state
+ */
+ ImmutableMap<Link, Boolean> getSeenLinks();
+
+ /**
+ * Returns the ports administratively disabled by the controller.
+ *
+ * @return a map of devices and port numbers for administratively disabled
+ * ports. Does not include ports manually disabled by the operator.
+ */
+ ImmutableMap<DeviceId, Set<PortNumber>> getDownedPortState();
}
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/cli/LinkStateCommand.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/cli/LinkStateCommand.java
new file mode 100644
index 0000000..ded4ae6
--- /dev/null
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/cli/LinkStateCommand.java
@@ -0,0 +1,92 @@
+/*
+ * Copyright 2018-present Open Networking Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.segmentrouting.cli;
+
+import java.util.Comparator;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.karaf.shell.commands.Command;
+import org.onosproject.cli.AbstractShellCommand;
+import org.onosproject.net.DeviceId;
+import org.onosproject.net.Link;
+import org.onosproject.net.PortNumber;
+import org.onosproject.segmentrouting.SegmentRoutingService;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+
+
+/**
+ * Command to read the current state of the DestinationSetNextObjectiveStore.
+ *
+ */
+@Command(scope = "onos", name = "sr-link-state", description = "Displays the current internal link state "
+ + "noted by this instance of the controller")
+public class LinkStateCommand extends AbstractShellCommand {
+ private static final String FORMAT_MAPPING = " %s";
+
+ @Override
+ protected void execute() {
+ SegmentRoutingService srService = AbstractShellCommand
+ .get(SegmentRoutingService.class);
+ printLinkState(srService.getSeenLinks(),
+ srService.getDownedPortState());
+ }
+
+ private void printLinkState(ImmutableMap<Link, Boolean> seenLinks,
+ ImmutableMap<DeviceId, Set<PortNumber>> downedPortState) {
+ List<Link> a = Lists.newArrayList();
+ a.addAll(seenLinks.keySet());
+ a.sort(new CompLinks());
+
+ StringBuilder slbldr = new StringBuilder();
+ slbldr.append("\n Seen Links: ");
+ for (int i = 0; i < a.size(); i++) {
+ slbldr.append("\n "
+ + (seenLinks.get(a.get(i)) == Boolean.TRUE ? " up : "
+ : "down : "));
+ slbldr.append(a.get(i).src() + " --> " + a.get(i).dst());
+ }
+ print(FORMAT_MAPPING, slbldr.toString());
+
+ StringBuilder dpbldr = new StringBuilder();
+ dpbldr.append("\n\n Administratively Disabled Ports: ");
+ downedPortState.entrySet().forEach(entry -> dpbldr
+ .append("\n " + entry.getKey() + entry.getValue()));
+ print(FORMAT_MAPPING, dpbldr.toString());
+ }
+
+ static class CompLinks implements Comparator<Link> {
+
+ @Override
+ public int compare(Link o1, Link o2) {
+ int res = o1.src().deviceId().toString()
+ .compareTo(o2.src().deviceId().toString());
+ if (res < 0) {
+ return -1;
+ } else if (res > 0) {
+ return +1;
+ }
+ if (o1.src().port().toLong() < o2.src().port().toLong()) {
+ return -1;
+ }
+ return +1;
+ }
+
+ }
+
+}
diff --git a/apps/segmentrouting/src/main/resources/OSGI-INF/blueprint/shell-config.xml b/apps/segmentrouting/src/main/resources/OSGI-INF/blueprint/shell-config.xml
index da4ebe3..b322360 100644
--- a/apps/segmentrouting/src/main/resources/OSGI-INF/blueprint/shell-config.xml
+++ b/apps/segmentrouting/src/main/resources/OSGI-INF/blueprint/shell-config.xml
@@ -35,10 +35,10 @@
<command>
<action class="org.onosproject.segmentrouting.cli.TunnelRemoveCommand"/>
</command>
- -->
<command>
<action class="org.onosproject.segmentrouting.cli.RerouteNetworkCommand"/>
</command>
+ -->
<command>
<action class="org.onosproject.segmentrouting.cli.DeviceSubnetListCommand"/>
</command>
@@ -54,6 +54,9 @@
<ref component-id="deviceIdCompleter"/>
</completers>
</command>
+ <command>
+ <action class="org.onosproject.segmentrouting.cli.LinkStateCommand"/>
+ </command>
</command-bundle>
<bean id="deviceIdCompleter" class="org.onosproject.cli.net.DeviceIdCompleter"/>
diff --git a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
index c623bf2..b877ef8 100644
--- a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
+++ b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
@@ -300,7 +300,7 @@
}
}
if (queued) {
- log.info("Queued forwarding objective {} for nextId {} meant for device {}",
+ log.debug("Queued forwarding objective {} for nextId {} meant for device {}",
fwd.id(), fwd.nextId(), deviceId);
}
return queued;
@@ -328,7 +328,7 @@
}
}
if (queued) {
- log.info("Queued next objective {} with operation {} meant for device {}",
+ log.debug("Queued next objective {} with operation {} meant for device {}",
next.id(), next.op(), deviceId);
}
return queued;
diff --git a/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java b/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
index f820feb..1335087 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/group/impl/DistributedGroupStore.java
@@ -435,7 +435,7 @@
// Check if a group is existing with the same key
Group existingGroup = getGroup(groupDesc.deviceId(), groupDesc.appCookie());
if (existingGroup != null) {
- log.info("Group already exists with the same key {} in dev:{} with id:0x{}",
+ log.debug("Group already exists with the same key {} in dev:{} with id:0x{}",
groupDesc.appCookie(), groupDesc.deviceId(),
Integer.toHexString(existingGroup.id().id()));
return;
diff --git a/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2Pipeline.java b/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2Pipeline.java
index 430d424..0c4a852 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2Pipeline.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa2Pipeline.java
@@ -223,6 +223,16 @@
return true;
}
+ /**
+ * Determines whether this driver should continue to retry flows that point
+ * to empty groups. See CORD-554.
+ *
+ * @return true if the driver should retry flows
+ */
+ protected boolean shouldRetry() {
+ return true;
+ }
+
//////////////////////////////////////
// Flow Objectives
//////////////////////////////////////
@@ -1237,11 +1247,14 @@
return Collections.emptySet();
}
tb.deferred().group(group.id());
- // check if group is empty
+ // retrying flows may be necessary due to bug CORD-554
if (gkeys.size() == 1 && gkeys.get(0).size() == 1) {
- log.warn("Found empty group 0x{} in dev:{} .. will retry fwd:{}",
- Integer.toHexString(group.id().id()), deviceId, fwd.id());
- emptyGroup = true;
+ if (shouldRetry()) {
+ log.warn("Found empty group 0x{} in dev:{} .. will retry fwd:{}",
+ Integer.toHexString(group.id().id()), deviceId,
+ fwd.id());
+ emptyGroup = true;
+ }
}
} else {
log.warn("Cannot find group for nextId:{} in dev:{}. Aborting fwd:{}",
@@ -1283,7 +1296,7 @@
);
log.debug("Default rule 0.0.0.0/0 is being installed two rules");
}
- // XXX retrying flows may be necessary due to bug CORD-554
+
if (emptyGroup) {
executorService.schedule(new RetryFlows(fwd, flowRuleCollection),
RETRY_MS, TimeUnit.MILLISECONDS);
diff --git a/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa3Pipeline.java b/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa3Pipeline.java
index edf8c38..dee4fff 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa3Pipeline.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/ofdpa/Ofdpa3Pipeline.java
@@ -88,6 +88,11 @@
}
@Override
+ protected boolean shouldRetry() {
+ return false;
+ }
+
+ @Override
protected void processFilter(FilteringObjective filteringObjective,
boolean install,
ApplicationId applicationId) {