Move EdgeService back to Link/Device services rather than TopologyService.

I think the EdgeService has to use one or the other, because the TopologyService is not in sync with the Link/Device services. The problem with using the TopologyService is that it does not handle Port events, only Device and Link, so it is not suitable for building an inventory of edge ports. Change-Id: If31d6d7013985da3e03f8c83f2f2eb2957deffe1

Move EdgeService back to Link/Device services rather than TopologyService.
I think the EdgeService has to use one or the other, because the TopologyService is not in sync with the Link/Device services. The problem with using the TopologyService is that it does not handle Port events, only Device and Link, so it is not suitable for building an inventory of edge ports. Change-Id: If31d6d7013985da3e03f8c83f2f2eb2957deffe1
Jonathan Hart · Gerrit Code Review
Commit 6ff6ffe4c046c18ead5eb2cedc78d96b8599ba04 6ff6ffe4 1 parent a32f6da9
Showing 4 changed files with 123 additions and 88 deletions
cli/src/main/java/org/onosproject/cli/net/EdgePortsListCommand.java
cli/src/main/resources/OSGI-INF/blueprint/shell-config.xml
core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
core/net/src/test/java/org/onosproject/net/edgeservice/impl/EdgeManagerTest.java
--- a/cli/src/main/java/org/onosproject/cli/net/EdgePortsListCommand.java
View file @6ff6ffe
+++ b/cli/src/main/java/org/onosproject/cli/net/EdgePortsListCommand.java
View file @6ff6ffe
@@ -18,8 +18,14 @@ package org.onosproject.cli.net;
 import org.apache.karaf.shell.commands.Argument;
 import org.apache.karaf.shell.commands.Command;
 import org.onosproject.cli.AbstractShellCommand;
+import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.edge.EdgePortService;
+import org.onosproject.utils.Comparators;
+import java.util.Collections;
+import java.util.List;
+
+import static com.google.common.collect.Lists.newArrayList;
 import static org.onosproject.net.DeviceId.deviceId;
 /**
@@ -35,16 +41,24 @@ public class EdgePortsListCommand extends AbstractShellCommand {
             required = false, multiValued = false)
     String uri = null;
-
-
     @Override
     protected void execute() {
         EdgePortService service = get(EdgePortService.class);
         if (uri == null) {
-            service.getEdgePoints().forEach(e -> print(FMT, e.deviceId(), e.port()));
+            printEdgePoints(service.getEdgePoints());
         } else {
-            service.getEdgePoints(deviceId(uri)).forEach(e -> print(FMT, e.deviceId(), e.port()));
+            printEdgePoints(service.getEdgePoints(deviceId(uri)));
+        }
     }
+
+    private void printEdgePoints(Iterable<ConnectPoint> edgePoints) {
+        sort(edgePoints).forEach(e -> print(FMT, e.deviceId(), e.port()));
+    }
+
+    private static List<ConnectPoint> sort(Iterable<ConnectPoint> connectPoints) {
+        List<ConnectPoint> edgePoints = newArrayList(connectPoints);
+        Collections.sort(edgePoints, Comparators.CONNECT_POINT_COMPARATOR);
+        return edgePoints;
     }
 }
--- a/cli/src/main/resources/OSGI-INF/blueprint/shell-config.xml
View file @6ff6ffe
+++ b/cli/src/main/resources/OSGI-INF/blueprint/shell-config.xml
View file @6ff6ffe
@@ -229,6 +229,7 @@
             <action class="org.onosproject.cli.net.EdgePortsListCommand"/>
             <completers>
                 <ref component-id="deviceIdCompleter"/>
+                <null/>
             </completers>
         </command>
--- a/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
View file @6ff6ffe
+++ b/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
View file @6ff6ffe
@@ -31,6 +31,7 @@ import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.Link.Type;
 import org.onosproject.net.device.DeviceEvent;
+import org.onosproject.net.device.DeviceListener;
 import org.onosproject.net.device.DeviceService;
 import org.onosproject.net.edge.EdgePortEvent;
 import org.onosproject.net.edge.EdgePortListener;
@@ -38,25 +39,29 @@ import org.onosproject.net.edge.EdgePortService;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
 import org.onosproject.net.flow.TrafficTreatment;
 import org.onosproject.net.link.LinkEvent;
+import org.onosproject.net.link.LinkListener;
+import org.onosproject.net.link.LinkService;
 import org.onosproject.net.packet.DefaultOutboundPacket;
 import org.onosproject.net.packet.OutboundPacket;
 import org.onosproject.net.packet.PacketService;
-import org.onosproject.net.topology.Topology;
-import org.onosproject.net.topology.TopologyEvent;
-import org.onosproject.net.topology.TopologyListener;
-import org.onosproject.net.topology.TopologyService;
 import org.slf4j.Logger;
 import java.nio.ByteBuffer;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import static org.onosproject.net.device.DeviceEvent.Type.*;
+
+import static org.onosproject.net.device.DeviceEvent.Type.DEVICE_ADDED;
+import static org.onosproject.net.device.DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED;
+import static org.onosproject.net.device.DeviceEvent.Type.DEVICE_REMOVED;
+import static org.onosproject.net.device.DeviceEvent.Type.PORT_STATS_UPDATED;
+import static org.onosproject.net.device.DeviceEvent.Type.PORT_UPDATED;
 import static org.onosproject.net.edge.EdgePortEvent.Type.EDGE_PORT_ADDED;
 import static org.onosproject.net.edge.EdgePortEvent.Type.EDGE_PORT_REMOVED;
-import static org.slf4j.LoggerFactory.getLogger;
 import static org.onosproject.security.AppGuard.checkPermission;
-import static org.onosproject.security.AppPermission.Type.*;
+import static org.onosproject.security.AppPermission.Type.PACKET_WRITE;
+import static org.onosproject.security.AppPermission.Type.TOPOLOGY_READ;
+import static org.slf4j.LoggerFactory.getLogger;
 /**
  * This is an implementation of the edge net service.
@@ -69,14 +74,11 @@ public class EdgeManager
     private final Logger log = getLogger(getClass());
-    private Topology topology;
+    // Set of edge ConnectPoints per Device.
-
-    /**
-     * Set of edge ConnectPoints per Device.
-     */
     private final Map<DeviceId, Set<ConnectPoint>> connectionPoints = Maps.newConcurrentMap();
-    private final TopologyListener topologyListener = new InnerTopologyListener();
+    private final DeviceListener deviceListener = new InnerDeviceListener();
+    private final LinkListener linkListener = new InnerLinkListener();
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected PacketService packetService;
@@ -85,19 +87,21 @@ public class EdgeManager
     protected DeviceService deviceService;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    protected TopologyService topologyService;
+    protected LinkService linkService;
     @Activate
     public void activate() {
         eventDispatcher.addSink(EdgePortEvent.class, listenerRegistry);
-        topologyService.addListener(topologyListener);
+        deviceService.addListener(deviceListener);
+        linkService.addListener(linkListener);
         loadAllEdgePorts();
         log.info("Started");
     }
     @Deactivate
     public void deactivate() {
-        topologyService.removeListener(topologyListener);
+        deviceService.removeListener(deviceListener);
+        linkService.removeListener(linkListener);
         eventDispatcher.removeSink(EdgePortEvent.class);
         log.info("Stopped");
     }
@@ -105,7 +109,8 @@ public class EdgeManager
     @Override
     public boolean isEdgePoint(ConnectPoint point) {
         checkPermission(TOPOLOGY_READ);
-        return !topologyService.isInfrastructure(topologyService.currentTopology(), point);
+        Set<ConnectPoint> connectPoints = connectionPoints.get(point.deviceId());
+        return connectPoints != null && connectPoints.contains(point);
     }
     @Override
@@ -148,26 +153,25 @@ public class EdgeManager
         return new DefaultOutboundPacket(point.deviceId(), builder.build(), data);
     }
-    private class InnerTopologyListener implements TopologyListener {
+    private class InnerLinkListener implements LinkListener {
+        @Override
+        public void event(LinkEvent event) {
+            processLinkEvent(event);
+        }
+    }
+    private class InnerDeviceListener implements DeviceListener {
         @Override
-        public void event(TopologyEvent event) {
+        public void event(DeviceEvent event) {
-            log.trace("Processing TopologyEvent {} caused by {}",
+            if (event.type() == PORT_STATS_UPDATED) {
-                      event.subject(), event.reasons());
+                return;
-            topology = event.subject();
-            event.reasons().forEach(reason -> {
-                if (reason instanceof DeviceEvent) {
-                    processDeviceEvent((DeviceEvent) reason);
-                } else if (reason instanceof LinkEvent) {
-                    processLinkEvent((LinkEvent) reason);
             }
-            });
+            processDeviceEvent(event);
         }
     }
     // Initial loading of the edge port cache.
     private void loadAllEdgePorts() {
-        topology = topologyService.currentTopology();
         deviceService.getAvailableDevices().forEach(d -> deviceService.getPorts(d.id())
                 .forEach(p -> addEdgePort(new ConnectPoint(d.id(), p.number()))));
     }
@@ -177,8 +181,7 @@ public class EdgeManager
         // negative Link event can result in increase of edge ports
         boolean addEdgePort = event.type() == LinkEvent.Type.LINK_REMOVED;
-        // but if the Link is an Edge type,
+        // but if the Link is an Edge type, it will be the opposite
-        // it will be the opposite
         if (event.subject().type() == Type.EDGE) {
             addEdgePort = !addEdgePort;
         }
@@ -198,8 +201,6 @@ public class EdgeManager
         DeviceEvent.Type type = event.type();
         DeviceId id = event.subject().id();
-        // FIXME there's still chance that Topology and Device Service
-        // view is out-of-sync
         if (type == DEVICE_ADDED ||
                 type == DEVICE_AVAILABILITY_CHANGED && deviceService.isAvailable(id)) {
             // When device is added or becomes available, add all its ports
@@ -223,8 +224,11 @@ public class EdgeManager
     }
     private boolean isEdgePort(ConnectPoint point) {
-        return !topologyService.isInfrastructure(topology, point) &&
+        // Logical ports are not counted as edge ports nor are infrastructure
-               !point.port().isLogical();
+        // ports. Ports that have only edge links are considered edge ports.
+        return !point.port().isLogical() &&
+                linkService.getLinks(point).stream()
+                        .allMatch(link -> link.type() == Type.EDGE);
     }
     // Adds the specified connection point to the edge points if needed.
--- a/core/net/src/test/java/org/onosproject/net/edgeservice/impl/EdgeManagerTest.java
View file @6ff6ffe
+++ b/core/net/src/test/java/org/onosproject/net/edgeservice/impl/EdgeManagerTest.java
View file @6ff6ffe
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
-
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -29,6 +28,7 @@ import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.DefaultPort;
 import org.onosproject.net.Device;
 import org.onosproject.net.DeviceId;
+import org.onosproject.net.Link;
 import org.onosproject.net.NetTestTools;
 import org.onosproject.net.Port;
 import org.onosproject.net.PortNumber;
@@ -38,26 +38,32 @@ import org.onosproject.net.device.DeviceServiceAdapter;
 import org.onosproject.net.edge.EdgePortEvent;
 import org.onosproject.net.edge.EdgePortListener;
 import org.onosproject.net.link.LinkEvent;
+import org.onosproject.net.link.LinkListener;
+import org.onosproject.net.link.LinkServiceAdapter;
 import org.onosproject.net.packet.OutboundPacket;
 import org.onosproject.net.packet.PacketServiceAdapter;
-import org.onosproject.net.topology.Topology;
 import org.onosproject.net.topology.TopologyEvent;
 import org.onosproject.net.topology.TopologyEvent.Type;
-import org.onosproject.net.topology.TopologyListener;
-import org.onosproject.net.topology.TopologyServiceAdapter;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import static org.hamcrest.Matchers.*;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
-import static org.junit.Assert.*;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.onosproject.net.NetTestTools.injectEventDispatcher;
-import static org.onosproject.net.device.DeviceEvent.Type.*;
+import static org.onosproject.net.device.DeviceEvent.Type.DEVICE_ADDED;
+import static org.onosproject.net.device.DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED;
+import static org.onosproject.net.device.DeviceEvent.Type.DEVICE_REMOVED;
 import static org.onosproject.net.edge.EdgePortEvent.Type.EDGE_PORT_ADDED;
 import static org.onosproject.net.edge.EdgePortEvent.Type.EDGE_PORT_REMOVED;
 import static org.onosproject.net.link.LinkEvent.Type.LINK_ADDED;
@@ -78,23 +84,23 @@ public class EdgeManagerTest {
     private Set<OutboundPacket> packets = Sets.newConcurrentHashSet();
     private final EdgePortListener testListener = new TestListener(events);
     private TestDeviceManager testDeviceManager;
-    private TestTopologyManager testTopologyManager;
+    private TestLinkService testLinkService;
     @Before
     public void setUp() {
         mgr = new EdgeManager();
         injectEventDispatcher(mgr, new TestEventDispatcher());
-        testTopologyManager = new TestTopologyManager(infrastructurePorts);
-        mgr.topologyService = testTopologyManager;
         testDeviceManager = new TestDeviceManager(devices);
         mgr.deviceService = testDeviceManager;
+
+        testLinkService = new TestLinkService();
+        mgr.linkService = testLinkService;
+
         mgr.packetService = new TestPacketManager();
         mgr.activate();
         mgr.addListener(testListener);
-
     }
-
     @After
     public void tearDown() {
         mgr.removeListener(testListener);
@@ -110,13 +116,12 @@ public class EdgeManagerTest {
         assertEquals("Unexpected number of ports", numDevices * numPorts, infrastructurePorts.size());
-        assertFalse("no ports expected", mgr.getEdgePoints().iterator().hasNext());
-
         assertFalse("Expected isEdge to return false",
                     mgr.isEdgePoint(NetTestTools.connectPoint(Integer.toString(1), 1)));
         removeInfraPort(NetTestTools.connectPoint(Integer.toString(1), 1));
-        assertTrue("Expected isEdge to return false",
+        postTopologyEvent(new LinkEvent(LINK_REMOVED, NetTestTools.link(Integer.toString(1), 1, "b", 2)));
+        assertTrue("Expected isEdge to return true",
                    mgr.isEdgePoint(NetTestTools.connectPoint(Integer.toString(1), 1)));
     }
@@ -190,17 +195,25 @@ public class EdgeManagerTest {
         int numDevices = 10;
         int numInfraPorts = 5;
         totalPorts = 10;
+
         defaultPopulator(numDevices, numInfraPorts);
+        events.clear();
+
         //Test response to device added events
-        referenceDevice = NetTestTools.device("1");
+
+        referenceDevice = NetTestTools.device("11");
+        devices.put(referenceDevice.id(), referenceDevice);
+        for (int port = 1; port <= numInfraPorts; port++) {
+            infrastructurePorts.add(NetTestTools.connectPoint("11", port));
+        }
         event = new DeviceEvent(DEVICE_ADDED, referenceDevice,
                                 new DefaultPort(referenceDevice, PortNumber.portNumber(1), true));
         postTopologyEvent(event);
         //Check that ports were populated correctly
         assertTrue("Unexpected number of new ports added",
-                   mgr.deviceService.getPorts(NetTestTools.did("1")).size() == 10);
+                   mgr.deviceService.getPorts(NetTestTools.did("11")).size() == 10);
         //Check that of the ten ports the half that are infrastructure ports aren't added
         assertEquals("Unexpected number of new edge ports added", (totalPorts - numInfraPorts), events.size());
@@ -227,7 +240,7 @@ public class EdgeManagerTest {
         for (; pointIterator.hasNext(); count++) {
             pointIterator.next();
         }
-        assertEquals("Unexpected number of edge points", totalPorts - numInfraPorts, count);
+        assertEquals("Unexpected number of edge points", (numDevices + 1) * numInfraPorts, count);
         //Testing device removal
         events.clear();
         event = (new DeviceEvent(DEVICE_REMOVED, referenceDevice,
@@ -271,10 +284,11 @@ public class EdgeManagerTest {
         //Ensure that the deviceManager shows the device as unavailable
         removeDevice(referenceDevice);
-        /*This variable copies the behavior of the topology by returning ports attached to an unavailable device
+        // This variable copies the behavior of the topology by returning ports
-        //this behavior is necessary for the following event to execute properly, if these statements are removed
+        // attached to an unavailable device this behavior is necessary for the
-        no events will be generated since no ports will be provided in getPorts() to EdgeManager.
+        // following event to execute properly, if these statements are removed
-        */
+        // no events will be generated since no ports will be provided in
+        // getPorts() to EdgeManager.
         alwaysReturnPorts = true;
         postTopologyEvent(event);
         alwaysReturnPorts = false;
@@ -330,7 +344,6 @@ public class EdgeManagerTest {
         }
     }
-
     @Test
     public void testEmit() {
         byte[] arr = new byte[10];
@@ -345,7 +358,7 @@ public class EdgeManagerTest {
         }
         for (int i = 0; i < numDevices; i++) {
             referenceDevice = NetTestTools.device(Integer.toString(i));
-            postTopologyEvent(new DeviceEvent(DEVICE_ADDED, referenceDevice,
+            testDeviceManager.listener.event(new DeviceEvent(DEVICE_ADDED, referenceDevice,
                                                              new DefaultPort(referenceDevice,
                                                                              PortNumber.portNumber(1),
                                                                              true)));
@@ -392,7 +405,13 @@ public class EdgeManagerTest {
      * @param event Event
      */
     private void postTopologyEvent(Event event) {
-        testTopologyManager.listener.event(topologyEventOf(event));
+        if (event instanceof DeviceEvent) {
+            testDeviceManager.listener.event((DeviceEvent) event);
+        }
+        if (event instanceof LinkEvent) {
+            testLinkService.listener.event((LinkEvent) event);
+        }
+        //testTopologyManager.listener.event(topologyEventOf(event));
     }
@@ -406,7 +425,9 @@ public class EdgeManagerTest {
             String str = Integer.toString(device);
             Device deviceToAdd = NetTestTools.device(str);
             devices.put(deviceToAdd.id(), deviceToAdd);
+            testDeviceManager.listener.event(new DeviceEvent(DEVICE_ADDED, deviceToAdd));
             for (int port = 1; port <= numInfraPorts; port++) {
+                testLinkService.listener.event(new LinkEvent(LINK_ADDED, NetTestTools.link(str, port, "other", 1)));
                 infrastructurePorts.add(NetTestTools.connectPoint(str, port));
             }
         }
@@ -436,30 +457,6 @@ public class EdgeManagerTest {
         infrastructurePorts.remove(port);
     }
-    private class TestTopologyManager extends TopologyServiceAdapter {
-        private TopologyListener listener;
-        private Set<ConnectPoint> infrastructurePorts;
-
-        public TestTopologyManager(Set<ConnectPoint> infrastructurePorts) {
-            this.infrastructurePorts = infrastructurePorts;
-        }
-
-        @Override
-        public boolean isInfrastructure(Topology topology, ConnectPoint connectPoint) {
-            return infrastructurePorts.contains(connectPoint);
-        }
-
-        @Override
-        public void addListener(TopologyListener listener) {
-            this.listener = listener;
-        }
-
-        @Override
-        public void removeListener(TopologyListener listener) {
-            this.listener = null;
-        }
-    }
-
     private class TestDeviceManager extends DeviceServiceAdapter {
         private DeviceListener listener;
@@ -510,6 +507,25 @@ public class EdgeManagerTest {
         }
     }
+    private class TestLinkService extends LinkServiceAdapter {
+
+        private LinkListener listener;
+
+        @Override
+        public Set<Link> getLinks(ConnectPoint connectPoint) {
+            if (infrastructurePorts.contains(connectPoint)) {
+                return Collections.singleton(NetTestTools.link("1", 1, "2", 1));
+            } else {
+                return Collections.emptySet();
+            }
+        }
+
+        @Override
+        public void addListener(LinkListener listener) {
+            this.listener = listener;
+        }
+    }
+
     private class TestPacketManager extends PacketServiceAdapter {
         @Override
         public void emit(OutboundPacket packet) {