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 72243bb..f1a63fc 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 long RETRY_INTERVAL_MS = 250L;
     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 = Instant.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 3242866..b346173 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -640,6 +640,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.
      *
@@ -1157,12 +1167,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 4ef6079..3269657 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.pwaas.DefaultL2Tunnel;
 import org.onosproject.segmentrouting.pwaas.DefaultL2TunnelPolicy;
@@ -189,4 +191,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 4e00481..56b6f43 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>
@@ -66,6 +66,9 @@
         <command>
             <action class="org.onosproject.segmentrouting.cli.PseudowireAddCommand"/>
         </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 ff9f2c0..661d3e9 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
@@ -252,6 +252,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
     //////////////////////////////////////
@@ -1276,11 +1286,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:{}",
@@ -1322,7 +1335,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 cbd6fb7..0059b64 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
@@ -89,6 +89,11 @@
     }
 
     @Override
+    protected boolean shouldRetry() {
+        return false;
+    }
+
+    @Override
     protected void processFilter(FilteringObjective filteringObjective,
                                  boolean install,
                                  ApplicationId applicationId) {