CORD-1583 More bug fixes in dual-ToR scenarios

 - reentrant lock was not being used correctly
 - fixHashGroup in group handler was not updating global store correctly
 - linkUp was not being noted in seenLinks if configuration came after switches connected
 - serialization error in global objective store due to missing kryo for Sets
 - damaged routepath computation was not taking pair-devs into account
 - switch failures were leading to improper ecmpSpg graph updates, and missed hash-group changes
 - implemented more next-objective verification as group sub-system can go out-of-sync with objective-store

Change-Id: If3cfdd715e9b69820894b49def31f75ceb748863
diff --git a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index e9517f1..50b0cd2 100644
--- a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -64,6 +64,7 @@
     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;
@@ -148,7 +149,7 @@
    public boolean isRoutingStable() {
        long last = (long) (lastRoutingChange.getMillis() / 1000.0);
        long now = (long) (DateTime.now().getMillis() / 1000.0);
-       log.debug("Routing stable since {}s", now - last);
+       log.trace("Routing stable since {}s", now - last);
        return (now - last) > STABLITY_THRESHOLD;
    }
 
@@ -364,6 +365,8 @@
             return;
         }
         lastRoutingChange = DateTime.now();
+        executorService.schedule(new UpdateMaps(), UPDATE_INTERVAL,
+                                 TimeUnit.SECONDS);
         statusLock.lock();
         try {
 
@@ -402,34 +405,23 @@
                 // comparing all routes of existing ECMP SPG to new ECMP SPG
                 routeChanges = computeRouteChange();
 
-                if (routeChanges != null) {
-                    // deal with linkUp of a seen-before link
-                    if (linkUp != null && srManager.isSeenLink(linkUp)) {
-                        if (!srManager.isBidirectional(linkUp)) {
-                            log.warn("Not a bidirectional link yet .. not "
-                                    + "processing link {}", linkUp);
-                            srManager.updateSeenLink(linkUp, true);
-                            populationStatus = Status.ABORTED;
-                            return;
-                        }
-                        // link previously seen before
-                        // do hash-bucket changes instead of a re-route
-                        processHashGroupChange(routeChanges, false, null);
-                        // clear out routesChanges so a re-route is not attempted
-                        routeChanges = ImmutableSet.of();
+                // deal with linkUp of a seen-before link
+                if (linkUp != null && srManager.isSeenLink(linkUp)) {
+                    if (!srManager.isBidirectional(linkUp)) {
+                        log.warn("Not a bidirectional link yet .. not "
+                                + "processing link {}", linkUp);
+                        srManager.updateSeenLink(linkUp, true);
+                        populationStatus = Status.ABORTED;
+                        return;
                     }
-
-                    //deal with switchDown
-                    if (switchDown != null) {
-                        processHashGroupChange(routeChanges, true, switchDown);
-                        // clear out routesChanges so a re-route is not attempted
-                        routeChanges = ImmutableSet.of();
-                    }
-
-                    // for a linkUp of a never-seen-before link
-                    // let it fall through to a reroute of the routeChanges
-
+                    // link previously seen before
+                    // do hash-bucket changes instead of a re-route
+                    processHashGroupChange(routeChanges, false, null);
+                    // clear out routesChanges so a re-route is not attempted
+                    routeChanges = ImmutableSet.of();
                 }
+                // for a linkUp of a never-seen-before link
+                // let it fall through to a reroute of the routeChanges
 
                 // now that we are past the check for a previously seen link
                 // it is safe to update the store for the linkUp
@@ -437,6 +429,12 @@
                     srManager.updateSeenLink(linkUp, true);
                 }
 
+                //deal with switchDown
+                if (switchDown != null) {
+                    processHashGroupChange(routeChanges, true, switchDown);
+                    // clear out routesChanges so a re-route is not attempted
+                    routeChanges = ImmutableSet.of();
+                }
             } else {
                 // link has gone down
                 // Compare existing ECMP SPG only with the link that went down
@@ -452,7 +450,6 @@
             if (routeChanges == null) {
                 log.info("Optimized routing failed... opting for full reroute");
                 populationStatus = Status.ABORTED;
-                statusLock.unlock();
                 populateAllRoutingRules();
                 return;
             }
@@ -482,6 +479,7 @@
         }
     }
 
+
     /**
      * Processes a set a route-path changes by reprogramming routing rules and
      * creating new hash-groups or editing them if necessary. This method also
@@ -533,8 +531,6 @@
             return false; //abort routing and fail fast
         }
 
-        //XXX should we do hashgroupchanges here?
-
         // update ecmpSPG for all edge-pairs
         for (EdgePair ep : edgePairs) {
             currentEcmpSpgMap.put(ep.dev1, updatedEcmpSpgMap.get(ep.dev1));
@@ -847,41 +843,51 @@
     private void processHashGroupChange(Set<ArrayList<DeviceId>> routeChanges,
                                         boolean linkOrSwitchFailed,
                                         DeviceId failedSwitch) {
+        Set<ArrayList<DeviceId>> changedRoutes = new HashSet<>();
+        // first, ensure each routeChanges entry has two elements
         for (ArrayList<DeviceId> route : routeChanges) {
-            DeviceId targetSw = route.get(0);
-            boolean success;
-            DeviceId dstSw = null;
-            if (route.size() > 1) {
-                dstSw = route.get(1);
+            if (route.size() == 1) {
+                // route-path changes are from everyone else to this switch
+                DeviceId dstSw = route.get(0);
+                srManager.deviceService.getAvailableDevices().forEach(sw -> {
+                    if (!sw.id().equals(dstSw)) {
+                        changedRoutes.add(Lists.newArrayList(sw.id(), dstSw));
+                    }
+                });
+            } else {
+                changedRoutes.add(route);
             }
+        }
 
+        for (ArrayList<DeviceId> route : changedRoutes) {
+            DeviceId targetSw = route.get(0);
+            DeviceId dstSw = route.get(1);
             if (linkOrSwitchFailed) {
-                fixHashGroupsForRoute(route, true);
+                boolean success = fixHashGroupsForRoute(route, true);
                 // it's possible that we cannot fix hash groups for a route
                 // if the target switch has failed. Nevertheless the ecmp graph
                 // for the impacted switch must still be updated.
-                if (failedSwitch != null && targetSw.equals(failedSwitch)
-                        && dstSw != null) {
+                if (!success && failedSwitch != null && targetSw.equals(failedSwitch)) {
                     currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
                     currentEcmpSpgMap.remove(targetSw);
-                    log.debug("Updating ECMPspg for dst:{} removing failed "
+                    log.debug("Updating ECMPspg for dst:{} removing failed switch "
                             + "target:{}", dstSw, targetSw);
-                    return;
+                    continue;
                 }
                 //linkfailed - update both sides
-                currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
-                if (dstSw != null) {
-                    currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
-                }
-                log.debug("Updating ECMPspg for dst:{} and target:{}", dstSw, targetSw);
-            } else {
-                success = fixHashGroupsForRoute(route, false);
                 if (success) {
                     currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
-                    if (dstSw != null) {
-                        currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
-                    }
-                    log.debug("Updating ECMPspg for target:{} and dst:{}",
+                    currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
+                    log.debug("Updating ECMPspg for dst:{} and target:{} for linkdown",
+                              dstSw, targetSw);
+                }
+            } else {
+                //linkup of seen before link
+                boolean success = fixHashGroupsForRoute(route, false);
+                if (success) {
+                    currentEcmpSpgMap.put(targetSw, updatedEcmpSpgMap.get(targetSw));
+                    currentEcmpSpgMap.put(dstSw, updatedEcmpSpgMap.get(dstSw));
+                    log.debug("Updating ECMPspg for target:{} and dst:{} for linkup",
                               targetSw, dstSw);
                 }
             }
@@ -908,48 +914,10 @@
             return false;
         }
         DeviceId destSw = route.get(1);
-        log.debug("Processing fixHashGroupsForRoute: Target {} -> Dest {}",
+        log.debug("* processing fixHashGroupsForRoute: Target {} -> Dest {}",
                   targetSw, destSw);
-        boolean targetIsEdge = false;
-        try {
-            targetIsEdge = srManager.deviceConfiguration.isEdgeDevice(targetSw);
-        } catch (DeviceConfigNotFoundException e) {
-            log.warn(e.getMessage() + "Cannot determine if targetIsEdge {}.. "
-                    + "continuing fixHash", targetSw);
-        }
-
         // figure out the new next hops at the targetSw towards the destSw
-        Set<DeviceId> nextHops = new HashSet<>();
-        EcmpShortestPathGraph ecmpSpg = updatedEcmpSpgMap.get(destSw);
-        HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
-                ecmpSpg.getAllLearnedSwitchesAndVia();
-        for (Integer itrIdx : switchVia.keySet()) {
-            HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> swViaMap =
-                    switchVia.get(itrIdx);
-            for (DeviceId target : swViaMap.keySet()) {
-                if (target.equals(targetSw)) {
-                    // found the iteration where targetSw is reached- get nextHops
-                    if (!targetIsEdge && itrIdx > 1) {
-                        // optimization for spines to not use other leaves to get
-                        // to a leaf to avoid loops
-                        log.debug("Avoiding {} hop path for non-edge targetSw:{}"
-                                + " --> dstSw:{}", itrIdx, targetSw, destSw);
-                        break;
-                    }
-                    for (ArrayList<DeviceId> via : swViaMap.get(target)) {
-                        if (via.isEmpty()) {
-                            // the dstSw is the next-hop from the targetSw
-                            nextHops.add(destSw);
-                        } else {
-                            // first elem is next-hop in each ECMP path
-                            nextHops.add(via.get(0));
-                        }
-                    }
-                    break;
-                }
-            }
-        }
-
+        Set<DeviceId> nextHops = getNextHops(targetSw, destSw);
         // call group handler to change hash group at targetSw
         DefaultGroupHandler grpHandler = srManager.getGroupHandler(targetSw);
         if (grpHandler == null) {
@@ -1010,7 +978,7 @@
      * @param deviceId the device for which graphs need to be purged
      */
     protected void purgeEcmpGraph(DeviceId deviceId) {
-        currentEcmpSpgMap.remove(deviceId);
+        currentEcmpSpgMap.remove(deviceId); // XXX reconsider
         if (updatedEcmpSpgMap != null) {
             updatedEcmpSpgMap.remove(deviceId);
         }
@@ -1030,64 +998,64 @@
      *         affected, or null if no previous ecmp spg was found for comparison
      */
     private Set<ArrayList<DeviceId>> computeDamagedRoutes(Link linkFail) {
-
         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.isLocalMaster(sw.id())) {
-                log.debug("No mastership for {} .. skipping route optimization",
-                          sw.id());
+            DeviceId retId = shouldHandleRouting(sw.id());
+            if (retId == null) {
                 continue;
             }
-            EcmpShortestPathGraph ecmpSpg = currentEcmpSpgMap.get(sw.id());
-            if (ecmpSpg == null) {
-                log.warn("No existing ECMP graph for switch {}. Aborting optimized"
-                        + " rerouting and opting for full-reroute", sw.id());
-                return null;
-            }
-            if (log.isDebugEnabled()) {
-                log.debug("Root switch: {}", sw.id());
-                log.debug("  Current/Existing SPG: {}", ecmpSpg);
-                log.debug("       New/Updated SPG: {}", updatedEcmpSpgMap.get(sw.id()));
-            }
-            HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>> switchVia =
-                    ecmpSpg.getAllLearnedSwitchesAndVia();
-            for (Integer itrIdx : switchVia.keySet()) {
-                log.trace("Current/Exiting SPG Iterindex# {}", itrIdx);
-                HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> swViaMap =
-                        switchVia.get(itrIdx);
-                for (DeviceId targetSw : swViaMap.keySet()) {
-                    DeviceId rootSw = sw.id();
-                    if (log.isTraceEnabled()) {
-                        log.trace("TargetSwitch {} --> RootSwitch {}", targetSw, rootSw);
+            Set<DeviceId> devicesToProcess = Sets.newHashSet(retId, sw.id());
+            for (DeviceId rootSw : devicesToProcess) {
+                EcmpShortestPathGraph ecmpSpg = currentEcmpSpgMap.get(rootSw);
+                if (ecmpSpg == null) {
+                    log.warn("No existing ECMP graph for switch {}. Aborting optimized"
+                            + " rerouting and opting for full-reroute", rootSw);
+                    return null;
+                }
+                if (log.isDebugEnabled()) {
+                    log.debug("Root switch: {}", rootSw);
+                    log.debug("  Current/Existing SPG: {}", ecmpSpg);
+                    log.debug("       New/Updated SPG: {}", updatedEcmpSpgMap.get(rootSw));
+                }
+                HashMap<Integer, HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>>>
+                    switchVia = ecmpSpg.getAllLearnedSwitchesAndVia();
+                // figure out if the broken link affected any route-paths in this graph
+                for (Integer itrIdx : switchVia.keySet()) {
+                    log.trace("Current/Exiting SPG Iterindex# {}", itrIdx);
+                    HashMap<DeviceId, ArrayList<ArrayList<DeviceId>>> swViaMap =
+                            switchVia.get(itrIdx);
+                    for (DeviceId targetSw : swViaMap.keySet()) {
+                        log.trace("TargetSwitch {} --> RootSwitch {}",
+                                  targetSw, rootSw);
                         for (ArrayList<DeviceId> via : swViaMap.get(targetSw)) {
                             log.trace(" Via:");
                             via.forEach(e -> log.trace("  {}", e));
                         }
-                    }
-                    Set<ArrayList<DeviceId>> subLinks =
-                            computeLinks(targetSw, rootSw, swViaMap);
-                    for (ArrayList<DeviceId> alink: subLinks) {
-                        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, rootSw);
-                            ArrayList<DeviceId> aRoute = new ArrayList<>();
-                            aRoute.add(targetSw); // switch with rules to populate
-                            aRoute.add(rootSw); // towards this destination
-                            routes.add(aRoute);
-                            break;
+                        Set<ArrayList<DeviceId>> subLinks =
+                                computeLinks(targetSw, rootSw, swViaMap);
+                        for (ArrayList<DeviceId> alink: subLinks) {
+                            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, rootSw);
+                                ArrayList<DeviceId> aRoute = new ArrayList<>();
+                                aRoute.add(targetSw); // switch with rules to populate
+                                aRoute.add(rootSw); // towards this destination
+                                routes.add(aRoute);
+                                break;
+                            }
                         }
                     }
                 }
+
             }
 
         }
-
         return routes;
     }
 
@@ -1407,6 +1375,35 @@
         }
     }
 
+    /**
+     * 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/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index e00d865..39b901b 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -1106,10 +1106,6 @@
 
     private void processLinkAdded(Link link) {
         log.info("** LINK ADDED {}", link.toString());
-        if (!deviceConfiguration.isConfigured(link.src().deviceId())) {
-            log.warn("Source device of this link is not configured..not processing");
-            return;
-        }
         if (link.type() != Link.Type.DIRECT) {
             // NOTE: A DIRECT link might be transiently marked as INDIRECT
             //       if BDDP is received before LLDP. We can safely ignore that
@@ -1118,6 +1114,13 @@
                     link.src(), link.dst(), link.type());
             return;
         }
+        if (!deviceConfiguration.isConfigured(link.src().deviceId())) {
+            updateSeenLink(link, true);
+            // XXX revisit - what about devicePortMap
+            log.warn("Source device of this link is not configured.. "
+                    + "not processing further");
+            return;
+        }
 
         //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,
@@ -1127,6 +1130,7 @@
         if (groupHandler != null) {
             groupHandler.portUpForLink(link);
         } else {
+            // XXX revisit/cleanup
             Device device = deviceService.getDevice(link.src().deviceId());
             if (device != null) {
                 log.warn("processLinkAdded: Link Added "
@@ -1189,7 +1193,12 @@
         if (groupHandler != null) {
             if (mastershipService.isLocalMaster(link.src().deviceId()) &&
                     isParallelLink(link)) {
+                log.debug("* retrying hash for parallel link removed:{}", link);
                 groupHandler.retryHash(link, true, false);
+            } else {
+                log.debug("Not attempting retry-hash for link removed: {} .. {}", link,
+                          (mastershipService.isLocalMaster(link.src().deviceId()))
+                                  ? "not parallel" : "not master");
             }
             // ensure local stores are updated
             groupHandler.portDown(link.src().port());
diff --git a/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java b/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
index d0dfa5e..11a8425 100644
--- a/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
+++ b/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
@@ -267,7 +267,6 @@
     public MacAddress getDeviceMac(DeviceId deviceId) throws DeviceConfigNotFoundException {
         SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
         if (srinfo != null) {
-            log.trace("getDeviceMac for device{} is {}", deviceId, srinfo.mac);
             return srinfo.mac;
         } else {
             String message = "getDeviceMac fails for device: " + deviceId + ".";
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
index 6f06d60..1a9a80e 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
@@ -15,9 +15,9 @@
  */
 package org.onosproject.segmentrouting.grouphandler;
 
-
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 
 import org.apache.commons.lang3.RandomUtils;
@@ -236,9 +236,9 @@
      *                  not seen-before
      */
     public void retryHash(Link link, boolean linkDown, boolean firstTime) {
-        MacAddress dstMac;
+        MacAddress neighborMac;
         try {
-            dstMac = deviceConfig.getDeviceMac(link.dst().deviceId());
+            neighborMac = deviceConfig.getDeviceMac(link.dst().deviceId());
         } catch (DeviceConfigNotFoundException e) {
             log.warn(e.getMessage() + " Aborting retryHash.");
             return;
@@ -264,124 +264,153 @@
             int nextId = nextHops.nextId();
             Set<DeviceId> dstSet = nextHops.getDstForNextHop(link.dst().deviceId());
             if (!linkDown) {
-                dstSet.forEach(dst -> {
-                    int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
-                    addToHashedNextObjective(link.src().port(), dstMac,
-                                             edgeLabel, nextId);
-                });
-
+                List<PortLabel> pl = Lists.newArrayList();
                 if (firstTime) {
                     // some links may have come up before the next-objective was created
                     // we take this opportunity to ensure other ports to same next-hop-dst
                     // are part of the hash group (see CORD-1180). Duplicate additions
                     // to the same hash group are avoided by the driver.
                     for (PortNumber p : devicePortMap.get(link.dst().deviceId())) {
-                        if (p.equals(link.src().port())) {
-                            continue;
-                        }
                         dstSet.forEach(dst -> {
                             int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
-                            addToHashedNextObjective(p, dstMac, edgeLabel, nextId);
+                            pl.add(new PortLabel(p, edgeLabel));
                         });
                     }
+                    addToHashedNextObjective(pl, neighborMac, nextId);
+                } else {
+                    // handle only the port that came up
+                    dstSet.forEach(dst -> {
+                        int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
+                        pl.add(new PortLabel(link.src().port(), edgeLabel));
+                    });
+                    addToHashedNextObjective(pl, neighborMac, nextId);
                 }
             } else {
+                // linkdown
+                List<PortLabel> pl = Lists.newArrayList();
                 dstSet.forEach(dst -> {
                     int edgeLabel = dsKey.destinationSet().getEdgeLabel(dst);
-                    removeFromHashedNextObjective(link.src().port(), dstMac,
-                                                  edgeLabel, nextId);
+                    pl.add(new PortLabel(link.src().port(), edgeLabel));
                 });
+                removeFromHashedNextObjective(pl, neighborMac, nextId);
             }
         }
     }
 
     /**
-     * Makes a call to the FlowObjective service to add a single bucket to
-     * a hashed group.
+     * Utility class for associating output ports and the corresponding MPLS
+     * labels to push. In dual-homing, there are different labels to push
+     * corresponding to the destination switches in an edge-pair. If both
+     * destinations are reachable via the same spine, then the output-port to
+     * the spine will be associated with two labels i.e. there will be two
+     * PortLabel objects for the same port but with different labels.
+     */
+    private class PortLabel {
+        PortNumber port;
+        int edgeLabel;
+
+        PortLabel(PortNumber port, int edgeLabel) {
+            this.port = port;
+            this.edgeLabel = edgeLabel;
+        }
+
+        @Override
+        public String toString() {
+            return port.toString() + "/" + String.valueOf(edgeLabel);
+        }
+    }
+
+    /**
+     * Makes a call to the FlowObjective service to add buckets to
+     * a hashed group. User must ensure that all the ports & labels are meant
+     * same neighbor (ie. dstMac).
      *
-     * @param outport port to add to hash group
+     * @param portLables a collection of port & label combinations to add
+     *                   to the hash group identified by the nextId
      * @param dstMac destination mac address of next-hop
-     * @param edgeLabel the label to use in the bucket
-     * @param nextId id for next-objective to which the bucket will be added
+     * @param nextId id for next-objective to which buckets will be added
      *
      */
-    private void addToHashedNextObjective(PortNumber outport, MacAddress dstMac,
-            int edgeLabel, Integer nextId) {
-        // Create the new bucket to be updated
-        TrafficTreatment.Builder tBuilder =
-                DefaultTrafficTreatment.builder();
-        tBuilder.setOutput(outport)
-            .setEthDst(dstMac)
-            .setEthSrc(nodeMacAddr);
-        if (edgeLabel != DestinationSet.NO_EDGE_LABEL) {
-            tBuilder.pushMpls()
-                .copyTtlOut()
-                .setMpls(MplsLabel.mplsLabel(edgeLabel));
-        }
+    private void addToHashedNextObjective(Collection<PortLabel> portLabels,
+                                          MacAddress dstMac, Integer nextId) {
         // setup metadata to pass to nextObjective - indicate the vlan on egress
         // if needed by the switch pipeline. Since hashed next-hops are always to
         // other neighboring routers, there is no subnet assigned on those ports.
         TrafficSelector.Builder metabuilder = DefaultTrafficSelector.builder();
         metabuilder.matchVlanId(INTERNAL_VLAN);
-
         NextObjective.Builder nextObjBuilder = DefaultNextObjective.builder()
                 .withId(nextId)
                 .withType(NextObjective.Type.HASHED)
-                .addTreatment(tBuilder.build())
                 .withMeta(metabuilder.build())
                 .fromApp(appId);
-        log.debug("addToHash in device {}: Adding Bucket with port/label {}/{} "
-                + "to nextId {}", deviceId, outport, edgeLabel, nextId);
+        // Create the new buckets to be updated
+        portLabels.forEach(pl -> {
+            TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder();
+            tBuilder.setOutput(pl.port)
+                .setEthDst(dstMac)
+                .setEthSrc(nodeMacAddr);
+            if (pl.edgeLabel != DestinationSet.NO_EDGE_LABEL) {
+                tBuilder.pushMpls()
+                    .copyTtlOut()
+                    .setMpls(MplsLabel.mplsLabel(pl.edgeLabel));
+            }
+            nextObjBuilder.addTreatment(tBuilder.build());
+        });
+
+        log.debug("addToHash in device {}: Adding Bucket with port/label {} "
+                + "to nextId {}", deviceId, portLabels, nextId);
 
         ObjectiveContext context = new DefaultObjectiveContext(
-                (objective) -> log.debug("addToHash addedTo NextObj {} on {}",
-                                         nextId, deviceId),
+                (objective) -> log.debug("addToHash port/label {} addedTo "
+                        + "NextObj {} on {}", portLabels, nextId, deviceId),
                 (objective, error) ->
-                        log.warn("addToHash failed to addTo NextObj {} on {}: {}",
+                        log.warn("addToHash failed to add port/label {} to"
+                                + " NextObj {} on {}: {}", portLabels,
                                  nextId, deviceId, error));
         NextObjective nextObjective = nextObjBuilder.addToExisting(context);
         flowObjectiveService.next(deviceId, nextObjective);
     }
 
     /**
-     * Makes a call to the FlowObjective service to remove a single bucket from
-     * a hashed group.
+     * Makes a call to the FlowObjective service to remove buckets from
+     * a hash group. User must ensure that all the ports & labels are meant
+     * same neighbor (ie. dstMac).
      *
-     * @param port port to remove from hash group
+     * @param portLables a collection of port & label combinations to remove
+     *                   from the hash group identified by the nextId
      * @param dstMac destination mac address of next-hop
-     * @param edgeLabel the label to use in the bucket
-     * @param nextId id for next-objective from which the bucket will be removed
+     * @param nextId id for next-objective from which buckets will be removed
      */
-    private void removeFromHashedNextObjective(PortNumber port, MacAddress dstMac,
-                                               int edgeLabel, Integer nextId) {
-        // Create the bucket to be removed
-        TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment
-                .builder();
-        tBuilder.setOutput(port)
-        .setEthDst(dstMac)
-        .setEthSrc(nodeMacAddr);
-        if (edgeLabel != DestinationSet.NO_EDGE_LABEL) {
-            tBuilder.pushMpls()
-            .copyTtlOut()
-            .setMpls(MplsLabel.mplsLabel(edgeLabel));
-        }
-        log.info("{} in device {}: Removing Bucket with Port {} to next object id {}",
-                 "removeFromHash", deviceId, port, nextId);
+    private void removeFromHashedNextObjective(Collection<PortLabel> portLabels,
+                                               MacAddress dstMac, Integer nextId) {
         NextObjective.Builder nextObjBuilder = DefaultNextObjective
                 .builder()
                 .withType(NextObjective.Type.HASHED) //same as original
                 .withId(nextId)
-                .fromApp(appId)
-                .addTreatment(tBuilder.build());
-        ObjectiveContext context = new DefaultObjectiveContext(
-                (objective) -> log.debug("port {} removedFrom NextObj {} on {}",
-                                         port, nextId, deviceId),
-                (objective, error) ->
-                log.warn("port {} failed to removeFrom NextObj {} on {}: {}",
-                         port, nextId, deviceId, error));
-        NextObjective nextObjective = nextObjBuilder.
-                removeFromExisting(context);
+                .fromApp(appId);
+        // Create the buckets to be removed
+        portLabels.forEach(pl -> {
+            TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder();
+            tBuilder.setOutput(pl.port)
+                .setEthDst(dstMac)
+                .setEthSrc(nodeMacAddr);
+            if (pl.edgeLabel != DestinationSet.NO_EDGE_LABEL) {
+                tBuilder.pushMpls()
+                    .copyTtlOut()
+                    .setMpls(MplsLabel.mplsLabel(pl.edgeLabel));
+            }
+            nextObjBuilder.addTreatment(tBuilder.build());
+        });
+        log.debug("removeFromHash in device {}: Removing Bucket with port/label"
+                + " {} from nextId {}", deviceId, portLabels, nextId);
 
+        ObjectiveContext context = new DefaultObjectiveContext(
+                (objective) -> log.debug("port/label {} removedFrom NextObj"
+                        + " {} on {}", portLabels, nextId, deviceId),
+                (objective, error) ->
+                log.warn("port/label {} failed to removeFrom NextObj {} on "
+                        + "{}: {}", portLabels, nextId, deviceId, error));
+        NextObjective nextObjective = nextObjBuilder.removeFromExisting(context);
         flowObjectiveService.next(deviceId, nextObjective);
     }
 
@@ -405,7 +434,7 @@
         // temporary storage of keys to be updated
         Map<DestinationSetNextObjectiveStoreKey, Set<DeviceId>> tempStore =
                 new HashMap<>();
-        boolean foundNextObjective = false;
+        boolean foundNextObjective = false, success = true;
 
         // retrieve hash-groups meant for destSw, which have destinationSets
         // with different neighbors than the given next-hops
@@ -432,44 +461,17 @@
                         + "hops:{} ..adding {}", targetSw, destSw, nextId,
                         currNeighbors, diff);
             }
-            for (DeviceId neighbor : diff) {
-                MacAddress dstMac;
-                try {
-                    dstMac = deviceConfig.getDeviceMac(neighbor);
-                } catch (DeviceConfigNotFoundException e) {
-                    log.warn(e.getMessage() + " Aborting fixHashGroup for nextId:"
-                            + nextId);
-                    return false;
-                }
-                if (devicePortMap.get(neighbor) == null ||
-                        devicePortMap.get(neighbor).isEmpty()) {
-                    log.warn("No ports found in dev:{} for neighbor:{} .. cannot "
-                            + "fix hash group for nextId: {}",
-                             deviceId, neighbor, nextId);
-                    return false;
-                }
+            boolean suc = updateAllPortsToNextHop(diff, edgeLabel, nextId,
+                                                  revoke);
+            if (suc) {
+                // to update neighbor set with changes made
                 if (revoke) {
-                    for (PortNumber port : devicePortMap.get(neighbor)) {
-                        log.info("fixHashGroup in device {}: Removing Bucket "
-                                + "with Port {} to next object id {}",
-                                deviceId, port, nextId);
-                        removeFromHashedNextObjective(port, dstMac,
-                                                      edgeLabel,
-                                                      nextId);
-                    }
-                    // to update neighbor set with changes made
                     tempStore.put(dskey, Sets.difference(currNeighbors, diff));
                 } else {
-                    for (PortNumber port : devicePortMap.get(neighbor)) {
-                        log.info("fixHashGroup in device {}: Adding Bucket "
-                                + "with Port {} to next object id {}",
-                                deviceId, port, nextId);
-                        addToHashedNextObjective(port, dstMac, edgeLabel, nextId);
-                    }
-                    // to update neighbor set with changes made
                     tempStore.put(dskey, Sets.union(currNeighbors, diff));
                 }
             }
+            success &= suc;
         }
 
         if (!foundNextObjective) {
@@ -480,18 +482,29 @@
 
         // update the dsNextObjectiveStore with new destinationSet to nextId mappings
         for (DestinationSetNextObjectiveStoreKey key : tempStore.keySet()) {
-            NextNeighbors oldHops = dsNextObjStore.get(key);
-            if (oldHops == null) {
+            NextNeighbors currentNextHops = dsNextObjStore.get(key);
+            if (currentNextHops == null) {
+                log.warn("fixHashGroups could not update global store in "
+                        + "device {} .. missing nextNeighbors for key {}",
+                        deviceId, key);
                 continue;
             }
-            Set<DeviceId> newNeighbors = tempStore.get(key);
-            Set<DeviceId> oldNeighbors = ImmutableSet.copyOf(oldHops.nextHops(destSw));
-            oldHops.dstNextHops().put(destSw, newNeighbors);
-            log.debug("Updating nsNextObjStore: oldHops:{} -> newHops:{} :: nextId:{}",
-                      oldNeighbors, newNeighbors, oldHops.nextId());
+            Set<DeviceId> newNeighbors = new HashSet<>();
+            newNeighbors.addAll(tempStore.get(key));
+            Map<DeviceId, Set<DeviceId>> oldDstNextHops =
+                    ImmutableMap.copyOf(currentNextHops.dstNextHops());
+            currentNextHops.dstNextHops().put(destSw, newNeighbors); //local change
+            log.debug("Updating nsNextObjStore target:{} -> dst:{} in key:{} nextId:{}",
+                      targetSw, destSw, key, currentNextHops.nextId());
+            log.debug("Old dstNextHops: {}", oldDstNextHops);
+            log.debug("New dstNextHops: {}", currentNextHops.dstNextHops());
+            // update global store
+            dsNextObjStore.put(key,
+                               new NextNeighbors(currentNextHops.dstNextHops(),
+                                                 currentNextHops.nextId()));
         }
-
-        return true;
+        // even if one fails and others succeed, return false so ECMPspg not updated
+        return success;
     }
 
     /**
@@ -543,7 +556,9 @@
     }
 
     /**
-     * Adds or removes buckets for all ports to a set of neighbor devices.
+     * Adds or removes buckets for all ports to a set of neighbor devices. Caller
+     * needs to ensure that the  given neighbors are all next hops towards the
+     * same destination (represented by the given edgeLabel).
      *
      * @param neighbors set of neighbor device ids
      * @param edgeLabel MPLS label to use in buckets
@@ -556,37 +571,33 @@
     private boolean updateAllPortsToNextHop(Set<DeviceId> neighbors, int edgeLabel,
                                          int nextId, boolean revoke) {
         for (DeviceId neighbor : neighbors) {
-            MacAddress dstMac;
+            MacAddress neighborMac;
             try {
-                dstMac = deviceConfig.getDeviceMac(neighbor);
+                neighborMac = deviceConfig.getDeviceMac(neighbor);
             } catch (DeviceConfigNotFoundException e) {
-                log.warn(e.getMessage() + " Aborting fixHashGroup for nextId:"
-                        + nextId);
+                log.warn(e.getMessage() + " Aborting updateAllPortsToNextHop"
+                        + " for nextId:" + nextId);
                 return false;
             }
-            if (devicePortMap.get(neighbor) == null ||
-                    devicePortMap.get(neighbor).isEmpty()) {
+            Collection<PortNumber> portsToNeighbor = devicePortMap.get(neighbor);
+            if (portsToNeighbor == null || portsToNeighbor.isEmpty()) {
                 log.warn("No ports found in dev:{} for neighbor:{} .. cannot "
-                        + "fix hash group for nextId: {}",
+                        + "updateAllPortsToNextHop for nextId: {}",
                          deviceId, neighbor, nextId);
                 return false;
             }
+            List<PortLabel> pl = Lists.newArrayList();
+            portsToNeighbor.forEach(p -> pl.add(new PortLabel(p, edgeLabel)));
             if (revoke) {
-                for (PortNumber port : devicePortMap.get(neighbor)) {
-                    log.debug("fixHashGroup in device {}: Removing Bucket "
-                            + "with Port {} edgeLabel:{} to next object id {}",
-                            deviceId, port, edgeLabel, nextId);
-                    removeFromHashedNextObjective(port, dstMac,
-                                                  edgeLabel,
-                                                  nextId);
-                }
+                log.debug("updateAllPortsToNextHops in device {}: Removing Bucket(s) "
+                        + "with Port/Label:{} to next object id {}",
+                        deviceId, pl, nextId);
+                removeFromHashedNextObjective(pl, neighborMac, nextId);
             } else {
-                for (PortNumber port : devicePortMap.get(neighbor)) {
-                    log.debug("fixHashGroup in device {}: Adding Bucket "
-                            + "with Port {} edgeLabel: {} to next object id {}",
-                            deviceId, port, edgeLabel, nextId);
-                    addToHashedNextObjective(port, dstMac, edgeLabel, nextId);
-                }
+                log.debug("fixHashGroup in device {}: Adding Bucket(s) "
+                        + "with Port/Label: {} to next object id {}",
+                        deviceId, pl, nextId);
+                addToHashedNextObjective(pl, neighborMac, nextId);
             }
         }
         return true;
@@ -1124,8 +1135,13 @@
 
 
     /**
-     *
-     *
+     * Performs bucket verification operation for all hash groups in this device.
+     * Checks RouteHandler to ensure that routing is stable before attempting
+     * verification. Verification involves creating a nextObjective with
+     * operation VERIFY for existing next objectives in the store, and passing
+     * it to the driver. It is the driver that actually performs the verification
+     * by adding or removing buckets to match the verification next objective
+     * created here.
      */
     protected final class BucketCorrector implements Runnable {
         Integer nextId;
@@ -1152,7 +1168,7 @@
             }
             rh.acquireRoutingLock();
             try {
-                log.debug("running bucket corrector for dev: {}", deviceId);
+                log.trace("running bucket corrector for dev: {}", deviceId);
                 Set<DestinationSetNextObjectiveStoreKey> dsKeySet = dsNextObjStore.entrySet()
                         .stream()
                         .filter(entry -> entry.getKey().deviceId().equals(deviceId))
@@ -1167,7 +1183,7 @@
                     if (nextId != null && nextId != nid) {
                         continue;
                     }
-                    log.debug("bkt-corr: dsNextObjStore for device {}: {}",
+                    log.trace("bkt-corr: dsNextObjStore for device {}: {}",
                               deviceId, dsKey, next);
                     TrafficSelector.Builder metabuilder = DefaultTrafficSelector.builder();
                     metabuilder.matchVlanId(INTERNAL_VLAN);
@@ -1189,7 +1205,7 @@
                                 return;
                             }
                             devicePortMap.get(neighbor).forEach(port -> {
-                                log.debug("verify in device {} nextId {}: bucket with"
+                                log.trace("verify in device {} nextId {}: bucket with"
                                         + " port/label {}/{} to dst {} via {}",
                                         deviceId, nid, port, edgeLabel,
                                         dstDev, neighbor);
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java
index 1a0507b..eaca1aa 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/NextNeighbors.java
@@ -25,27 +25,60 @@
 
 import org.onosproject.net.DeviceId;
 
+/**
+ * Represents the nexthop information associated with a route-path towards a
+ * set of destinations.
+ */
 public class NextNeighbors {
     private final Map<DeviceId, Set<DeviceId>> dstNextHops;
     private final int nextId;
 
+    /**
+     * Constructor.
+     *
+     * @param dstNextHops map of destinations and the next-hops towards each dest
+     * @param nextId id of nextObjective that manifests the next-hop info
+     */
     public NextNeighbors(Map<DeviceId, Set<DeviceId>> dstNextHops, int nextId) {
         this.dstNextHops = dstNextHops;
         this.nextId = nextId;
     }
 
+    /**
+     * Returns a map of destinations and the next-hops towards them.
+     *
+     * @return map of destinations and the next-hops towards them
+     */
     public Map<DeviceId, Set<DeviceId>> dstNextHops() {
         return dstNextHops;
     }
 
+    /**
+     * Set of next-hops towards the given destination.
+     *
+     * @param deviceId the destination
+     * @return set of nexthops towards the destination
+     */
     public Set<DeviceId> nextHops(DeviceId deviceId) {
         return dstNextHops.get(deviceId);
     }
 
+    /**
+     * Return the nextId representing the nextObjective towards the next-hops.
+     *
+     * @return nextId representing the nextObjective towards the next-hops
+     */
     public int nextId() {
         return nextId;
     }
 
+    /**
+     * Checks if the given nextHopId is a valid next hop to any one of the
+     * destinations.
+     *
+     * @param nextHopId the deviceId for the next hop
+     * @return true if given next
+     */
     public boolean containsNextHop(DeviceId nextHopId) {
         for (Set<DeviceId> nextHops : dstNextHops.values()) {
             if (nextHops != null && nextHops.contains(nextHopId)) {
@@ -55,6 +88,14 @@
         return false;
     }
 
+    /**
+     * Returns a set of destinations which have the given nextHopId as one
+     * of the next-hops to that destination.
+     *
+     * @param nextHopId the deviceId for the next hop
+     * @return set of deviceIds that have the given nextHopId as a next-hop
+     *          which could be empty if no destinations were found
+     */
     public Set<DeviceId> getDstForNextHop(DeviceId nextHopId) {
         Set<DeviceId> dstSet = new HashSet<>();
         for (DeviceId dstKey : dstNextHops.keySet()) {