Take down edge ports on a leaf switch when all uplinks are gone

- Bug fix for case when all uplinks are gone, but dual-homed host continues to send packets to switch;
  We now administratively take down the host port to force the host to use the other leaf in the pair.
- Restructured SR manager by creating a LinkHandler
- fixed/added some log messages

Change-Id: I3722cd364dc8798b16891519bec165627e92bd87

apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java

@@ -161,8 +161,8 @@ public class DefaultRoutingHandler {
     //  Route path handling
     //////////////////////////////////////
 
-    /* The following three methods represent the three major ways in routing
+    /* The following three methods represent the three major ways in which
-     * is triggered in the network
+     * route-path handling is triggered in the network
      *      a) due to configuration change
      *      b) due to route-added event
      *      c) due to change in the topology
@@ -416,11 +416,11 @@ public class DefaultRoutingHandler {
                 routeChanges = computeRouteChange();
 
                 // deal with linkUp of a seen-before link
-                if (linkUp != null && srManager.isSeenLink(linkUp)) {
+                if (linkUp != null && srManager.linkHandler.isSeenLink(linkUp)) {
-                    if (!srManager.isBidirectional(linkUp)) {
+                    if (!srManager.linkHandler.isBidirectional(linkUp)) {
                         log.warn("Not a bidirectional link yet .. not "
                                 + "processing link {}", linkUp);
-                        srManager.updateSeenLink(linkUp, true);
+                        srManager.linkHandler.updateSeenLink(linkUp, true);
                         populationStatus = Status.ABORTED;
                         return;
                     }
@@ -436,7 +436,7 @@ public class DefaultRoutingHandler {
                 // now that we are past the check for a previously seen link
                 // it is safe to update the store for the linkUp
                 if (linkUp != null) {
-                    srManager.updateSeenLink(linkUp, true);
+                    srManager.linkHandler.updateSeenLink(linkUp, true);
                 }
 
                 //deal with switchDown

apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/EcmpShortestPathGraph.java

@@ -74,7 +74,7 @@ public class EcmpShortestPathGraph {
             currDistance = distanceQueue.poll();
 
             for (Link link : srManager.linkService.getDeviceEgressLinks(sw)) {
-                if (srManager.avoidLink(link)) {
+                if (srManager.linkHandler.avoidLink(link)) {
                     continue;
                 }
                 DeviceId reachedDevice = link.dst().deviceId();

apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/HostHandler.java

@@ -42,6 +42,7 @@ import org.slf4j.LoggerFactory;
 
 import com.google.common.collect.Sets;
 
+import java.util.HashSet;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Executors;
@@ -88,6 +89,14 @@ public class HostHandler {
 
     private void processHostAdded(Host host) {
         host.locations().forEach(location -> processHostAddedAtLocation(host, location));
+        // ensure dual-homed host locations have viable uplinks
+        if (host.locations().size() > 1) {
+            host.locations().forEach(loc -> {
+                if (srManager.mastershipService.isLocalMaster(loc.deviceId())) {
+                    srManager.linkHandler.checkUplinksForDualHomedHosts(loc);
+                }
+            });
+        }
     }
 
     void processHostAddedAtLocation(Host host, HostLocation location) {
@@ -276,6 +285,15 @@ public class HostHandler {
                         hostVlanId, ip, false)
             );
         });
+
+        // ensure dual-homed host locations have viable uplinks
+        if (newLocations.size() > prevLocations.size()) {
+            newLocations.forEach(loc -> {
+                if (srManager.mastershipService.isLocalMaster(loc.deviceId())) {
+                    srManager.linkHandler.checkUplinksForDualHomedHosts(loc);
+                }
+            });
+        }
     }
 
     void processHostUpdatedEvent(HostEvent event) {
@@ -640,4 +658,23 @@ public class HostHandler {
                 });
             }));
     }
+
+    /**
+     * Returns the set of portnumbers on the given device that are part of the
+     * locations for dual-homed hosts.
+     *
+     * @param deviceId the given deviceId
+     * @return set of port numbers on given device that are dual-homed host
+     *         locations. May be empty if no dual homed hosts are connected to
+     *         the given device
+     */
+    Set<PortNumber> getDualHomedHostPorts(DeviceId deviceId) {
+        Set<PortNumber> dualHomedLocations = new HashSet<>();
+        srManager.hostService.getConnectedHosts(deviceId).stream()
+            .filter(host -> host.locations().size() == 2)
+            .forEach(host -> host.locations().stream()
+                     .filter(loc -> loc.deviceId().equals(deviceId))
+                        .forEach(loc -> dualHomedLocations.add(loc.port())));
+        return dualHomedLocations;
+    }
 }

apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/LinkHandler.java

@@ -0,0 +1,561 @@
+/*
+ * 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;
+
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.onosproject.net.Device;
+import org.onosproject.net.DeviceId;
+import org.onosproject.net.HostLocation;
+import org.onosproject.net.Link;
+import org.onosproject.net.PortNumber;
+import org.onosproject.net.link.LinkService;
+import org.onosproject.segmentrouting.config.DeviceConfigNotFoundException;
+import org.onosproject.segmentrouting.config.DeviceConfiguration;
+import org.onosproject.segmentrouting.grouphandler.DefaultGroupHandler;
+import org.onosproject.store.service.EventuallyConsistentMap;
+import org.onosproject.store.service.EventuallyConsistentMapBuilder;
+import org.onosproject.store.service.WallClockTimestamp;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.Sets;
+
+public class LinkHandler {
+    private static final Logger log = LoggerFactory.getLogger(LinkHandler.class);
+    protected final SegmentRoutingManager srManager;
+    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
+    // 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.
+    private Map<Link, Boolean> seenLinks = new ConcurrentHashMap<>();
+
+    private EventuallyConsistentMap<DeviceId, Set<PortNumber>> downedPortStore = null;
+
+    /**
+     * Constructs the LinkHandler.
+     *
+     * @param srManager Segment Routing manager
+     */
+    LinkHandler(SegmentRoutingManager srManager) {
+        this.srManager = srManager;
+        linkService = srManager.linkService;
+        log.debug("Creating EC map downedportstore");
+        EventuallyConsistentMapBuilder<DeviceId, Set<PortNumber>> downedPortsMapBuilder
+                = srManager.storageService.eventuallyConsistentMapBuilder();
+        downedPortStore = downedPortsMapBuilder.withName("downedportstore")
+                .withSerializer(srManager.createSerializer())
+                .withTimestampProvider((k, v) -> new WallClockTimestamp())
+                .build();
+        log.trace("Current size {}", downedPortStore.size());
+    }
+
+    /**
+     * Constructs the LinkHandler for unit-testing.
+     *
+     * @param srManager SegmentRoutingManager
+     * @param linkService LinkService
+     */
+    LinkHandler(SegmentRoutingManager srManager, LinkService linkService) {
+        this.srManager = srManager;
+        this.linkService = linkService;
+    }
+
+    /**
+     * Preprocessing of added link before being sent for route-path handling.
+     * Also performs post processing of link.
+     *
+     * @param link the link to be processed
+     */
+    void processLinkAdded(Link link) {
+        log.info("** LINK ADDED {}", link.toString());
+        if (!isLinkValid(link)) {
+            return;
+        }
+        if (!srManager.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,
+        // as in a multi-instance setup, instances can initiate groups for any
+        // device.
+        DefaultGroupHandler groupHandler = srManager.groupHandlerMap
+                .get(link.src().deviceId());
+        if (groupHandler != null) {
+            groupHandler.portUpForLink(link);
+        } else {
+            // XXX revisit/cleanup
+            Device device = srManager.deviceService.getDevice(link.src().deviceId());
+            if (device != null) {
+                log.warn("processLinkAdded: Link Added "
+                        + "Notification without Device Added "
+                        + "event, still handling it");
+                srManager.processDeviceAdded(device);
+                groupHandler = srManager.groupHandlerMap.get(link.src().deviceId());
+                groupHandler.portUpForLink(link);
+            }
+        }
+
+        /*
+         // process link only if it is bidirectional
+         if (!isBidirectional(link)) {
+            log.debug("Link not bidirectional.. waiting for other direction " +
+                      "src {} --> dst {} ", link.dst(), link.src());
+            // note that if we are not processing for routing, it should at least
+            // be considered a seen-link
+            updateSeenLink(link, true); return;
+          }
+         //TODO ensure that rehash is still done correctly even if link is not processed for
+         //rerouting - perhaps rehash in both directions when it ultimately becomes bidi?
+         */
+
+        log.debug("Starting optimized route-path processing for added link "
+                + "{} --> {}", link.src(), link.dst());
+        boolean seenBefore = isSeenLink(link);
+        // seenLink updates will be done after route-path changes
+        srManager.defaultRoutingHandler
+                .populateRoutingRulesForLinkStatusChange(null, link, null);
+
+        if (srManager.mastershipService.isLocalMaster(link.src().deviceId())) {
+            // handle edge-ports for dual-homed hosts
+            updateDualHomedHostPorts(link, true);
+
+            // 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
+            // for the link that has come up.
+            if (!seenBefore && isParallelLink(link)) {
+                // if link seen first time, we need to ensure hash-groups have
+                // all ports
+                log.debug("Attempting retryHash for paralled first-time link {}",
+                          link);
+                groupHandler.retryHash(link, false, true);
+            } else {
+                // seen before-link
+                if (isParallelLink(link)) {
+                    log.debug("Attempting retryHash for paralled seen-before "
+                            + "link {}", link);
+                    groupHandler.retryHash(link, false, false);
+                }
+            }
+        }
+
+        srManager.mcastHandler.init();
+    }
+
+    /**
+     * Preprocessing of removed link before being sent for route-path handling.
+     * Also performs post processing of link.
+     *
+     * @param link the link to be processed
+     */
+    void processLinkRemoved(Link link) {
+        log.info("** LINK REMOVED {}", link.toString());
+        if (!isLinkValid(link)) {
+            return;
+        }
+        // when removing links, update seen links first, before doing route-path
+        // changes
+        updateSeenLink(link, false);
+
+        // device availability check helps to ensure that multiple link-removed
+        // events are actually treated as a single switch removed event.
+        // purgeSeenLink is necessary so we do rerouting (instead of rehashing)
+        // when switch comes back.
+        if (link.src().elementId() instanceof DeviceId
+                && !srManager.deviceService.isAvailable(link.src().deviceId())) {
+            purgeSeenLink(link);
+            return;
+        }
+        if (link.dst().elementId() instanceof DeviceId
+                && !srManager.deviceService.isAvailable(link.dst().deviceId())) {
+            purgeSeenLink(link);
+            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
+                .populateRoutingRulesForLinkStatusChange(link, null, null);
+
+        // update local groupHandler stores
+        DefaultGroupHandler groupHandler = srManager.groupHandlerMap
+                .get(link.src().deviceId());
+        if (groupHandler != null) {
+            if (srManager.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,
+                          (srManager.mastershipService.isLocalMaster(link.src()
+                                  .deviceId())) ? "not parallel"
+                                                : "not master");
+            }
+            // ensure local stores are updated
+            groupHandler.portDown(link.src().port());
+        } else {
+            log.warn("group handler not found for dev:{} when removing link: {}",
+                     link.src().deviceId(), link);
+        }
+
+        srManager.mcastHandler.processLinkDown(link);
+    }
+
+    /**
+     * Checks validity of link. Examples of invalid links include
+     * indirect-links, links between ports on the same switch, and more.
+     *
+     * @param link the link to be processed
+     * @return true if valid link
+     */
+    private boolean isLinkValid(Link link) {
+        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
+            // until the LLDP is received and the link is marked as DIRECT.
+            log.info("Ignore link {}->{}. Link type is {} instead of DIRECT.",
+                     link.src(), link.dst(), link.type());
+            return false;
+        }
+        DeviceId srcId = link.src().deviceId();
+        DeviceId dstId = link.dst().deviceId();
+        if (srcId.equals(dstId)) {
+            log.warn("Links between ports on the same switch are not "
+                    + "allowed .. ignoring link {}", link);
+            return false;
+        }
+        DeviceConfiguration devConfig = srManager.deviceConfiguration;
+        try {
+            if (!devConfig.isEdgeDevice(srcId)
+                    && !devConfig.isEdgeDevice(dstId)) {
+                // ignore links between spines
+                // XXX revisit when handling multi-stage fabrics
+                log.warn("Links between spines not allowed...ignoring "
+                        + "link {}", link);
+                return false;
+            }
+            if (devConfig.isEdgeDevice(srcId)
+                    && devConfig.isEdgeDevice(dstId)) {
+                // ignore links between leaves if they are not pair-links
+                // XXX revisit if removing pair-link config or allowing more than
+                // one pair-link
+                if (devConfig.getPairDeviceId(srcId).equals(dstId)
+                        && devConfig.getPairLocalPort(srcId)
+                                .equals(link.src().port())
+                        && devConfig.getPairLocalPort(dstId)
+                                .equals(link.dst().port())) {
+                    // found pair link - allow it
+                    return true;
+                } else {
+                    log.warn("Links between leaves other than pair-links are "
+                            + "not allowed...ignoring link {}", link);
+                    return false;
+                }
+            }
+        } catch (DeviceConfigNotFoundException e) {
+            // We still want to count the links in seenLinks even though there
+            // is no config. So we let it return true
+            log.warn("Could not check validity of link {} as subtending devices "
+                    + "are not yet configured", link);
+        }
+        return true;
+    }
+
+    /**
+     * Administratively enables or disables edge ports if the link that was
+     * added or removed was the only uplink port from an edge device. Only edge
+     * ports that belong to dual-homed hosts are considered.
+     *
+     * @param link the link to be processed
+     * @param added true if link was added, false if link was removed
+     */
+    private void updateDualHomedHostPorts(Link link, boolean added) {
+        if (!lastUplink(link)) {
+            return;
+        }
+        if (added) {
+            // re-enable previously disabled ports on this dev
+            Set<PortNumber> p = downedPortStore.remove(link.src().deviceId());
+            if (p != null) {
+                log.warn("Link src {} -->dst {} added is the first uplink, "
+                        + "enabling dual homed ports: {}", link.src().deviceId(),
+                     link.dst().deviceId(), (p.isEmpty()) ? "no ports" : p);
+                p.forEach(pnum -> srManager.deviceAdminService
+                        .changePortState(link.src().deviceId(), pnum, true));
+            }
+        } else {
+            // find dual homed hosts on this dev to disable
+            Set<PortNumber> dhp = srManager.hostHandler
+                    .getDualHomedHostPorts(link.src().deviceId());
+            log.warn("Link src {} -->dst {} removed was the last uplink, "
+                    + "disabling  dual homed ports:  {}", link.src().deviceId(),
+                     link.dst().deviceId(), (dhp.isEmpty()) ? "no ports" : dhp);
+            dhp.forEach(pnum -> srManager.deviceAdminService
+                        .changePortState(link.src().deviceId(), pnum, false));
+            if (!dhp.isEmpty()) {
+                // update global store
+                Set<PortNumber> p = downedPortStore.get(link.src().deviceId());
+                if (p == null) {
+                    p = dhp;
+                } else {
+                    p.addAll(dhp);
+                }
+                downedPortStore.put(link.src().deviceId(), p);
+            }
+        }
+    }
+
+    /**
+     * Returns true if given link is the last active uplink from src-device of
+     * link. An uplink is defined as a unidirectional link with src as
+     * edgeRouter and dst as non-edgeRouter.
+     *
+     * @param link
+     * @return true if given link is-the-first/was-the-last uplink from the src
+     *         device
+     */
+    private boolean lastUplink(Link link) {
+        DeviceConfiguration devConfig = srManager.deviceConfiguration;
+        try {
+            if (!devConfig.isEdgeDevice(link.src().deviceId())) {
+                return false;
+            }
+            Set<Link> devLinks = srManager.linkService
+                    .getDeviceLinks(link.src().deviceId());
+            boolean foundOtherUplink = false;
+            for (Link l : devLinks) {
+                if (devConfig.isEdgeDevice(l.dst().deviceId()) || l.equals(link)
+                        || l.state() == Link.State.INACTIVE) {
+                    continue;
+                }
+                foundOtherUplink = true;
+                break;
+            }
+            if (!foundOtherUplink) {
+                return true;
+            }
+        } catch (DeviceConfigNotFoundException e) {
+            log.warn("Unable to determine if link is last uplink"
+                    + e.getMessage());
+        }
+        return false;
+    }
+
+    /**
+     * Returns true if this controller instance has seen this link before. The
+     * link may not be currently up, but as long as the link had been seen
+     * before this method will return true. The one exception is when the link
+     * was indeed seen before, but this controller instance was forced to forget
+     * it by a call to purgeSeenLink method.
+     *
+     * @param link the infrastructure link being queried
+     * @return true if this controller instance has seen this link before
+     */
+    boolean isSeenLink(Link link) {
+        return seenLinks.containsKey(link);
+    }
+
+    /**
+     * Updates the seen link store. Updates can be for links that are currently
+     * available or not.
+     *
+     * @param link the link to update in the seen-link local store
+     * @param up the status of the link, true if up, false if down
+     */
+    void updateSeenLink(Link link, boolean up) {
+        seenLinks.put(link, up);
+    }
+
+    /**
+     * Returns the status of a seen-link (up or down). If the link has not been
+     * seen-before, a null object is returned.
+     *
+     * @param link the infrastructure link being queried
+     * @return null if the link was not seen-before; true if the seen-link is
+     *         up; false if the seen-link is down
+     */
+    private Boolean isSeenLinkUp(Link link) {
+        return seenLinks.get(link);
+    }
+
+    /**
+     * Makes this controller instance forget a previously seen before link.
+     *
+     * @param link the infrastructure link to purge
+     */
+    private void purgeSeenLink(Link link) {
+        seenLinks.remove(link);
+    }
+
+    /**
+     * Returns the status of a link as parallel link. A parallel link is defined
+     * as a link which has common src and dst switches as another seen-link that
+     * is currently enabled. It is not necessary for the link being queried to
+     * be a seen-link.
+     *
+     * @param link the infrastructure link being queried
+     * @return true if a seen-link exists that is up, and shares the same src
+     *         and dst switches as the link being queried
+     */
+    private boolean isParallelLink(Link link) {
+        for (Entry<Link, Boolean> seen : seenLinks.entrySet()) {
+            Link seenLink = seen.getKey();
+            if (seenLink.equals(link)) {
+                continue;
+            }
+            if (seenLink.src().deviceId().equals(link.src().deviceId())
+                    && seenLink.dst().deviceId().equals(link.dst().deviceId())
+                    && seen.getValue()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Returns true if the link being queried is a bidirectional link. A bidi
+     * link is defined as a link, whose reverse link - ie. the link in the
+     * reverse direction - has been seen-before and is up. It is not necessary
+     * for the link being queried to be a seen-link.
+     *
+     * @param link the infrastructure link being queried
+     * @return true if another unidirectional link exists in the reverse
+     *         direction, has been seen-before and is up
+     */
+    boolean isBidirectional(Link link) {
+        Link reverseLink = linkService.getLink(link.dst(), link.src());
+        if (reverseLink == null) {
+            return false;
+        }
+        Boolean result = isSeenLinkUp(reverseLink);
+        if (result == null) {
+            return false;
+        }
+        return result.booleanValue();
+    }
+
+    /**
+     * Determines if the given link should be avoided in routing calculations by
+     * policy or design.
+     *
+     * @param link the infrastructure link being queried
+     * @return true if link should be avoided
+     */
+    boolean avoidLink(Link link) {
+        // XXX currently only avoids all pair-links. In the future can be
+        // extended to avoid any generic link
+        DeviceId src = link.src().deviceId();
+        PortNumber srcPort = link.src().port();
+        DeviceConfiguration devConfig = srManager.deviceConfiguration;
+        if (devConfig == null || !devConfig.isConfigured(src)) {
+            log.warn("Device {} not configured..cannot avoid link {}", src,
+                     link);
+            return false;
+        }
+        DeviceId pairDev;
+        PortNumber pairLocalPort, pairRemotePort = null;
+        try {
+            pairDev = devConfig.getPairDeviceId(src);
+            pairLocalPort = devConfig.getPairLocalPort(src);
+            if (pairDev != null) {
+                pairRemotePort = devConfig
+                        .getPairLocalPort(pairDev);
+            }
+        } catch (DeviceConfigNotFoundException e) {
+            log.warn("Pair dev for dev {} not configured..cannot avoid link {}",
+                     src, link);
+            return false;
+        }
+
+        return srcPort.equals(pairLocalPort)
+                && link.dst().deviceId().equals(pairDev)
+                && link.dst().port().equals(pairRemotePort);
+    }
+
+    /**
+     * Cleans up internal LinkHandler stores.
+     *
+     * @param device the device that has been removed
+     */
+    void processDeviceRemoved(Device device) {
+        seenLinks.keySet()
+                .removeIf(key -> key.src().deviceId().equals(device.id())
+                        || key.dst().deviceId().equals(device.id()));
+    }
+
+    /**
+     * Administratively disables the host location switchport if the edge device
+     * has no viable uplinks.
+     *
+     * @param loc one of the locations of the dual-homed host
+     */
+    void checkUplinksForDualHomedHosts(HostLocation loc) {
+        try {
+            for (Link l : srManager.linkService.getDeviceLinks(loc.deviceId())) {
+                if (srManager.deviceConfiguration.isEdgeDevice(l.dst().deviceId())
+                        || l.state() == Link.State.INACTIVE) {
+                    continue;
+                }
+                // found valid uplink - so, nothing to do
+                return;
+            }
+        } catch (DeviceConfigNotFoundException e) {
+            log.warn("Could not check for valid uplinks due to missing device"
+                    + "config " + e.getMessage());
+            return;
+        }
+        log.warn("Dual homed host location {} has no valid uplinks; "
+                + "disabling  dual homed port", loc);
+        srManager.deviceAdminService.changePortState(loc.deviceId(), loc.port(),
+                                                     false);
+        Set<PortNumber> p = downedPortStore.get(loc.deviceId());
+        if (p == null) {
+            p = Sets.newHashSet(loc.port());
+        } else {
+            p.add(loc.port());
+        }
+        downedPortStore.put(loc.deviceId(), p);
+    }
+
+}

apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java

@@ -60,6 +60,7 @@ import org.onosproject.net.config.NetworkConfigRegistry;
 import org.onosproject.net.config.basics.InterfaceConfig;
 import org.onosproject.net.config.basics.McastConfig;
 import org.onosproject.net.config.basics.SubjectFactories;
+import org.onosproject.net.device.DeviceAdminService;
 import org.onosproject.net.device.DeviceEvent;
 import org.onosproject.net.device.DeviceListener;
 import org.onosproject.net.device.DeviceService;
@@ -119,7 +120,6 @@ import java.util.Dictionary;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -166,6 +166,9 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     DeviceService deviceService;
 
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    DeviceAdminService deviceAdminService;
+
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     public FlowObjectiveService flowObjectiveService;
 
@@ -212,9 +215,10 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     private InternalDeviceListener deviceListener = null;
     private AppConfigHandler appCfgHandler = null;
     XConnectHandler xConnectHandler = null;
-    private McastHandler mcastHandler = null;
+    McastHandler mcastHandler = null;
-    private HostHandler hostHandler = null;
+    HostHandler hostHandler = null;
     private RouteHandler routeHandler = null;
+    LinkHandler linkHandler = null;
     private SegmentRoutingNeighbourDispatcher neighbourHandler = null;
     private L2TunnelHandler l2TunnelHandler = null;
     private InternalEventHandler eventHandler = new InternalEventHandler();
@@ -230,7 +234,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     private static ScheduledFuture<?> eventHandlerFuture = null;
     @SuppressWarnings("rawtypes")
     private ConcurrentLinkedQueue<Event> eventQueue = new ConcurrentLinkedQueue<>();
-    private Map<DeviceId, DefaultGroupHandler> groupHandlerMap =
+    Map<DeviceId, DefaultGroupHandler> groupHandlerMap =
             new ConcurrentHashMap<>();
     /**
      * Per device next objective ID store with (device id + destination set) as key.
@@ -251,16 +255,6 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     private EventuallyConsistentMap<PortNextObjectiveStoreKey, Integer>
             portNextObjStore = null;
 
-    // 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
-    // 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.
-    private Map<Link, Boolean> seenLinks = new ConcurrentHashMap<>();
-
     private EventuallyConsistentMap<String, Tunnel> tunnelStore = null;
     private EventuallyConsistentMap<String, Policy> policyStore = null;
 
@@ -412,6 +406,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         xConnectHandler = new XConnectHandler(this);
         mcastHandler = new McastHandler(this);
         hostHandler = new HostHandler(this);
+        linkHandler = new LinkHandler(this);
         routeHandler = new RouteHandler(this);
         neighbourHandler = new SegmentRoutingNeighbourDispatcher(this);
         l2TunnelHandler = new L2TunnelHandler(this);
@@ -437,7 +432,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         log.info("Started");
     }
 
-    private KryoNamespace.Builder createSerializer() {
+    KryoNamespace.Builder createSerializer() {
         return new KryoNamespace.Builder()
                 .register(KryoNamespaces.API)
                 .register(DestinationSetNextObjectiveStoreKey.class,
@@ -921,135 +916,6 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         return defaultRoutingHandler;
     }
 
-    /**
-     * Returns true if this controller instance has seen this link before. The
-     * link may not be currently up, but as long as the link had been seen before
-     * this method will return true. The one exception is when the link was
-     * indeed seen before, but this controller instance was forced to forget it
-     * by a call to purgeSeenLink method.
-     *
-     * @param link the infrastructure link being queried
-     * @return true if this controller instance has seen this link before
-     */
-    boolean isSeenLink(Link link) {
-        return seenLinks.containsKey(link);
-    }
-
-    /**
-     * Updates the seen link store. Updates can be for links that are currently
-     * available or not.
-     *
-     * @param link the link to update in the seen-link local store
-     * @param up the status of the link, true if up, false if down
-     */
-    void updateSeenLink(Link link, boolean up) {
-        seenLinks.put(link, up);
-    }
-
-    /**
-     * Returns the status of a seen-link (up or down). If the link has not
-     * been seen-before, a null object is returned.
-     *
-     * @param link the infrastructure link being queried
-     * @return null if the link was not seen-before;
-     *         true if the seen-link is up;
-     *         false if the seen-link is down
-     */
-    private Boolean isSeenLinkUp(Link link) {
-        return seenLinks.get(link);
-    }
-
-    /**
-     * Makes this controller instance forget a previously seen before link.
-     *
-     * @param link the infrastructure link to purge
-     */
-    private void purgeSeenLink(Link link) {
-        seenLinks.remove(link);
-    }
-
-    /**
-     * Returns the status of a link as parallel link. A parallel link
-     * is defined as a link which has common src and dst switches as another
-     * seen-link that is currently enabled. It is not necessary for the link being
-     * queried to be a seen-link.
-     *
-     *  @param link the infrastructure link being queried
-     *  @return true if a seen-link exists that is up, and shares the
-     *          same src and dst switches as the link being queried
-     */
-    private boolean isParallelLink(Link link) {
-        for (Entry<Link, Boolean> seen : seenLinks.entrySet()) {
-            Link seenLink = seen.getKey();
-           if (seenLink.equals(link)) {
-               continue;
-           }
-           if (seenLink.src().deviceId().equals(link.src().deviceId()) &&
-                   seenLink.dst().deviceId().equals(link.dst().deviceId()) &&
-                   seen.getValue()) {
-               return true;
-           }
-        }
-        return false;
-    }
-
-    /**
-     * Returns true if the link being queried is a bidirectional link. A bidi
-     * link is defined as a link, whose reverse link - ie. the link in the reverse
-     * direction - has been seen-before and is up. It is not necessary for the link
-     * being queried to be a seen-link.
-     *
-     * @param link the infrastructure link being queried
-     * @return true if another unidirectional link exists in the reverse direction,
-     *              has been seen-before and is up
-     */
-    boolean isBidirectional(Link link) {
-        Link reverseLink = linkService.getLink(link.dst(), link.src());
-        if (reverseLink == null) {
-            return false;
-        }
-        Boolean result = isSeenLinkUp(reverseLink);
-        if (result == null) {
-            return false;
-        }
-        return result.booleanValue();
-    }
-
-    /**
-     * Determines if the given link should be avoided in routing calculations
-     * by policy or design.
-     *
-     * @param link the infrastructure link being queried
-     * @return true if link should be avoided
-     */
-    boolean avoidLink(Link link) {
-        // XXX currently only avoids all pair-links. In the future can be
-        // extended to avoid any generic link
-        DeviceId src = link.src().deviceId();
-        PortNumber srcPort = link.src().port();
-        if (deviceConfiguration == null || !deviceConfiguration.isConfigured(src)) {
-            log.warn("Device {} not configured..cannot avoid link {}", src, link);
-            return false;
-        }
-        DeviceId pairDev;
-        PortNumber pairLocalPort, pairRemotePort = null;
-        try {
-            pairDev = deviceConfiguration.getPairDeviceId(src);
-            pairLocalPort = deviceConfiguration.getPairLocalPort(src);
-            if (pairDev != null) {
-                pairRemotePort = deviceConfiguration.getPairLocalPort(pairDev);
-            }
-        } catch (DeviceConfigNotFoundException e) {
-            log.warn("Pair dev for dev {} not configured..cannot avoid link {}",
-                     src, link);
-            return false;
-        }
-
-        return srcPort.equals(pairLocalPort) &&
-                link.dst().deviceId().equals(pairDev) &&
-                link.dst().port().equals(pairRemotePort);
-    }
-
     private class InternalPacketProcessor implements PacketProcessor {
         @Override
         public void process(PacketContext context) {
@@ -1174,28 +1040,9 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                         // Note: do not update seenLinks here, otherwise every
                         // link, even one seen for the first time, will be appear
                         // to be a previously seen link
-                        processLinkAdded((Link) event.subject());
+                        linkHandler.processLinkAdded((Link) event.subject());
                     } else if (event.type() == LinkEvent.Type.LINK_REMOVED) {
-                        Link linkRemoved = (Link) event.subject();
+                        linkHandler.processLinkRemoved((Link) event.subject());
-                        if (linkRemoved.type() == Link.Type.DIRECT) {
-                            updateSeenLink(linkRemoved, false);
-                        }
-                        // device availability check helps to ensure that
-                        // multiple link-removed events are actually treated as a
-                        // single switch removed event. purgeSeenLink is necessary
-                        // so we do rerouting (instead of rehashing) when switch
-                        // comes back.
-                        if (linkRemoved.src().elementId() instanceof DeviceId &&
-                                !deviceService.isAvailable(linkRemoved.src().deviceId())) {
-                            purgeSeenLink(linkRemoved);
-                            continue;
-                        }
-                        if (linkRemoved.dst().elementId() instanceof DeviceId &&
-                                !deviceService.isAvailable(linkRemoved.dst().deviceId())) {
-                            purgeSeenLink(linkRemoved);
-                            continue;
-                        }
-                        processLinkRemoved((Link) event.subject());
                     } else if (event.type() == DeviceEvent.Type.DEVICE_ADDED ||
                             event.type() == DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED ||
                             event.type() == DeviceEvent.Type.DEVICE_UPDATED) {
@@ -1238,164 +1085,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         }
     }
 
-    private void processLinkAdded(Link link) {
+    void processDeviceAdded(Device device) {
-        log.info("** LINK ADDED {}", link.toString());
-        if (!isLinkValid(link)) {
-            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,
-        //as in a multi-instance setup, instances can initiate groups for any device.
-        DefaultGroupHandler groupHandler = groupHandlerMap.get(link.src()
-                .deviceId());
-        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 "
-                        + "Notification without Device Added "
-                        + "event, still handling it");
-                processDeviceAdded(device);
-                groupHandler = groupHandlerMap.get(link.src()
-                                                   .deviceId());
-                groupHandler.portUpForLink(link);
-            }
-        }
-
-        /*// process link only if it is bidirectional
-        if (!isBidirectional(link)) {
-            log.debug("Link not bidirectional.. waiting for other direction "
-                    + "src {} --> dst {} ", link.dst(), link.src());
-            // note that if we are not processing for routing, it should at least
-            // be considered a seen-link
-            updateSeenLink(link, true);
-            return;
-        }
-         TO DO this ensure that rehash is still done correctly even if link is
-         not processed for rerouting - perhaps rehash in both directions when
-         it ultimately becomes bidi?
-        */
-
-        log.debug("Starting optimized route population process for link "
-                + "{} --> {}", link.src(), link.dst());
-        boolean seenBefore = isSeenLink(link);
-        defaultRoutingHandler.populateRoutingRulesForLinkStatusChange(null, link, null);
-
-        // 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
-        // for the link that has come up.
-        if (groupHandler != null && mastershipService.isLocalMaster(link.src().deviceId())) {
-            if (!seenBefore && isParallelLink(link)) {
-                // if link seen first time, we need to ensure hash-groups have all ports
-                log.debug("Attempting retryHash for paralled first-time link {}", link);
-                groupHandler.retryHash(link, false, true);
-            } else {
-                //seen before-link
-                if (isParallelLink(link)) {
-                    log.debug("Attempting retryHash for paralled seen-before "
-                            + "link {}", link);
-                    groupHandler.retryHash(link, false, false);
-                }
-            }
-        }
-
-        mcastHandler.init();
-    }
-
-    private void processLinkRemoved(Link link) {
-        log.info("** LINK REMOVED {}", link.toString());
-        if (!isLinkValid(link)) {
-            return;
-        }
-        defaultRoutingHandler.populateRoutingRulesForLinkStatusChange(link, null, null);
-
-        // update local groupHandler stores
-        DefaultGroupHandler groupHandler = groupHandlerMap.get(link.src().deviceId());
-        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());
-        } else {
-            log.warn("group handler not found for dev:{} when removing link: {}",
-                     link.src().deviceId(), link);
-        }
-
-        mcastHandler.processLinkDown(link);
-        l2TunnelHandler.processLinkDown(link);
-    }
-
-    private boolean isLinkValid(Link link) {
-        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
-            // until the LLDP is received and the link is marked as DIRECT.
-            log.info("Ignore link {}->{}. Link type is {} instead of DIRECT.",
-                     link.src(), link.dst(), link.type());
-            return false;
-        }
-        DeviceId srcId = link.src().deviceId();
-        DeviceId dstId = link.dst().deviceId();
-        if (srcId.equals(dstId)) {
-            log.warn("Links between ports on the same switch are not "
-                    + "allowed .. ignoring link {}", link);
-            return false;
-        }
-        try {
-            if (!deviceConfiguration.isEdgeDevice(srcId)
-                    && !deviceConfiguration.isEdgeDevice(dstId)) {
-                // ignore links between spines
-                // XXX revisit when handling multi-stage fabrics
-                log.warn("Links between spines not allowed...ignoring "
-                        + "link {}", link);
-                return false;
-            }
-            if (deviceConfiguration.isEdgeDevice(srcId)
-                    && deviceConfiguration.isEdgeDevice(dstId)) {
-                // ignore links between leaves if they are not pair-links
-                // XXX revisit if removing pair-link config or allowing more than
-                // one pair-link
-                if (deviceConfiguration.getPairDeviceId(srcId).equals(dstId)
-                        && deviceConfiguration.getPairLocalPort(srcId)
-                                .equals(link.src().port())
-                        && deviceConfiguration.getPairLocalPort(dstId)
-                                .equals(link.dst().port())) {
-                    // found pair link - allow it
-                    return true;
-                } else {
-                    log.warn("Links between leaves other than pair-links are "
-                            + "not allowed...ignoring link {}", link);
-                    return false;
-                }
-            }
-        } catch (DeviceConfigNotFoundException e) {
-            // We still want to count the links in seenLinks even though there
-            // is no config. So we let it return true
-            log.warn("Could not check validity of link {} as subtending devices "
-                    + "are not yet configured", link);
-        }
-        return true;
-    }
-
-    private void processDeviceAdded(Device device) {
         log.info("** DEVICE ADDED with ID {}", device.id());
 
         // NOTE: Punt ARP/NDP even when the device is not configured.
@@ -1461,9 +1151,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         portNextObjStore.entrySet().stream()
                 .filter(entry -> entry.getKey().deviceId().equals(device.id()))
                 .forEach(entry -> portNextObjStore.remove(entry.getKey()));
-
+        linkHandler.processDeviceRemoved(device);
-        seenLinks.keySet().removeIf(key -> key.src().deviceId().equals(device.id()) ||
-                key.dst().deviceId().equals(device.id()));
 
         DefaultGroupHandler gh = groupHandlerMap.remove(device.id());
         if (gh != null) {

apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingNeighbourDispatcher.java

@@ -42,7 +42,8 @@ public class SegmentRoutingNeighbourDispatcher implements NeighbourMessageHandle
 
     @Override
     public void handleMessage(NeighbourMessageContext context, HostService hostService) {
-        log.trace("Received a {} packet {}", context.protocol(), context.packet());
+        log.trace("Received {} packet on {}: {}", context.protocol(),
+                  context.inPort(), context.packet());
         switch (context.protocol()) {
             case ARP:
                 if (this.manager.arpHandler != null) {

apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java

@@ -212,6 +212,7 @@ public class HostHandlerTest {
         srManager.cfgService = mockNetworkConfigRegistry;
         mockLocationProbingService = new MockLocationProbingService();
         srManager.probingService = mockLocationProbingService;
+        srManager.linkHandler = new MockLinkHandler(srManager);
 
         hostHandler = new HostHandler(srManager);
 

apps/segmentrouting/src/test/java/org/onosproject/segmentrouting/MockLinkHandler.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright 2017-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;
+
+import org.onosproject.net.HostLocation;
+
+/**
+ * Mocks the LinkHandler in SR.
+ *
+ */
+public class MockLinkHandler extends LinkHandler {
+
+    MockLinkHandler(SegmentRoutingManager srManager) {
+        super(srManager, null);
+    }
+
+    @Override
+    void checkUplinksForDualHomedHosts(HostLocation loc) {
+        // currently does nothing - can be extended to be a useful mock when
+        // implementing unit tests for link handling
+    }
+}

protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java

@@ -380,8 +380,9 @@ class OFChannelHandler extends ChannelInboundHandlerAdapter
                     h.portDescReplies.add((OFPortDescStatsReply) m);
                 }
                 //h.portDescReply = (OFPortDescStatsReply) m; // temp store
-                log.info("Received port desc reply for switch at {}",
+                log.debug("Received port desc reply for switch at {}: {}",
-                        h.getSwitchInfoString());
+                         h.getSwitchInfoString(),
+                         ((OFPortDescStatsReply) m).getEntries());
                 try {
                     h.sendHandshakeSetConfig();
                 } catch (IOException e) {
@@ -780,6 +781,9 @@ class OFChannelHandler extends ChannelInboundHandlerAdapter
             void processOFStatisticsReply(OFChannelHandler h,
                     OFStatsReply m) {
                 if (m.getStatsType().equals(OFStatsType.PORT_DESC)) {
+                    log.debug("Received port desc message from {}: {}",
+                             h.sw.getDpid(),
+                             ((OFPortDescStatsReply) m).getEntries());
                     h.sw.setPortDescReply((OFPortDescStatsReply) m);
                 }
                 h.dispatchMessage(m);