Changed ProxyArpManager to make use of EdgeManager.

Change-Id: I05193146490aba6736c1815bf0d9022df8628973

Changed ProxyArpManager to make use of EdgeManager.
Change-Id: I05193146490aba6736c1815bf0d9022df8628973
Aaron Kruglikov · Gerrit Code Review
Commit d8123835629026a2db88d40f60be596aa7791b8b d8123835 1 parent aeeb33ef
Showing 3 changed files with 76 additions and 163 deletions
core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java
core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java
--- a/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
View file @d812383
+++ b/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
View file @d812383
@@ -174,6 +174,8 @@ public class EdgeManager implements EdgePortService {
                     }
                 });
             } else {
+                //FIXME special case of preexisting edgeport & no triggerless events could cause this to never hit and
+                //never discover an edgeport that should have been discovered.
                 loadAllEdgePorts();
             }
         }
@@ -198,6 +200,7 @@ public class EdgeManager implements EdgePortService {
     // Processes a device event by adding or removing its end-points in our cache.
     private void processDeviceEvent(DeviceEvent event) {
+        //FIXME handle the case where a device is suspended, this may or may not come up
         DeviceEvent.Type type = event.type();
         DeviceId id = event.subject().id();
--- a/core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java
View file @d812383
+++ b/core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java
View file @d812383
@@ -15,9 +15,6 @@
  */
 package org.onosproject.net.proxyarp.impl;
-import com.google.common.collect.HashMultimap;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Multimap;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -38,22 +35,15 @@ import org.onlab.packet.ndp.NeighborDiscoveryOptions;
 import org.onlab.packet.ndp.NeighborSolicitation;
 import org.onosproject.core.Permission;
 import org.onosproject.net.ConnectPoint;
-import org.onosproject.net.Device;
 import org.onosproject.net.Host;
 import org.onosproject.net.HostId;
-import org.onosproject.net.Link;
-import org.onosproject.net.Port;
-import org.onosproject.net.PortNumber;
-import org.onosproject.net.device.DeviceEvent;
-import org.onosproject.net.device.DeviceListener;
 import org.onosproject.net.device.DeviceService;
+import org.onosproject.net.edge.EdgePortService;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
 import org.onosproject.net.flow.TrafficTreatment;
 import org.onosproject.net.host.HostService;
 import org.onosproject.net.host.InterfaceIpAddress;
 import org.onosproject.net.host.PortAddresses;
-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.InboundPacket;
@@ -64,14 +54,13 @@ import org.slf4j.Logger;
 import java.nio.ByteBuffer;
 import java.util.HashSet;
-import java.util.List;
-import java.util.Map.Entry;
 import java.util.Set;
+import java.util.stream.Collectors;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
-import static org.slf4j.LoggerFactory.getLogger;
 import static org.onosproject.security.AppGuard.checkPermission;
+import static org.slf4j.LoggerFactory.getLogger;
 @Component(immediate = true)
@@ -87,6 +76,9 @@ public class ProxyArpManager implements ProxyArpService {
     private static final String NOT_ARP_REPLY = "ARP is not a reply.";
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    protected EdgePortService edgeService;
+
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected HostService hostService;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
@@ -98,29 +90,18 @@ public class ProxyArpManager implements ProxyArpService {
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected DeviceService deviceService;
-    private final Multimap<Device, PortNumber> internalPorts = HashMultimap.create();
-    private final Multimap<Device, PortNumber> externalPorts = HashMultimap.create();
-
-    private final DeviceListener deviceListener = new InternalDeviceListener();
-    private final InternalLinkListener linkListener = new InternalLinkListener();
-
     /**
      * Listens to both device service and link service to determine
      * whether a port is internal or external.
      */
     @Activate
     public void activate() {
-        deviceService.addListener(deviceListener);
-        linkService.addListener(linkListener);
-        determinePortLocations();
         log.info("Started");
     }
     @Deactivate
     public void deactivate() {
-        deviceService.removeListener(deviceListener);
-        linkService.removeListener(linkListener);
         log.info("Stopped");
     }
@@ -303,6 +284,7 @@ public class ProxyArpManager implements ProxyArpService {
         Ethernet ndpReply = buildNdpReply(targetAddress, dst.mac(), eth);
         sendTo(ndpReply, inPort);
     }
+    //TODO checkpoint
     /**
      * Outputs the given packet out the given port.
@@ -311,8 +293,7 @@ public class ProxyArpManager implements ProxyArpService {
      * @param outPort the port to send it out
      */
     private void sendTo(Ethernet packet, ConnectPoint outPort) {
-        if (internalPorts.containsEntry(
+        if (!edgeService.isEdgePoint(outPort)) {
-                deviceService.getDevice(outPort.deviceId()), outPort.port())) {
             // Sanity check to make sure we don't send the packet out an
             // internal port and create a loop (could happen due to
             // misconfiguration).
@@ -332,13 +313,10 @@ public class ProxyArpManager implements ProxyArpService {
      * @return a set of PortAddresses describing ports in the subnet
      */
     private Set<PortAddresses> findPortsInSubnet(IpAddress target) {
-        Set<PortAddresses> result = new HashSet<PortAddresses>();
+        Set<PortAddresses> result = new HashSet<>();
         for (PortAddresses addresses : hostService.getAddressBindings()) {
-            for (InterfaceIpAddress ia : addresses.ipAddresses()) {
+            result.addAll(addresses.ipAddresses().stream().filter(ia -> ia.subnetAddress().contains(target)).
-                if (ia.subnetAddress().contains(target)) {
+                    map(ia -> addresses).collect(Collectors.toList()));
-                    result.add(addresses);
-                }
-            }
         }
         return result;
     }
@@ -438,58 +416,17 @@ public class ProxyArpManager implements ProxyArpService {
         TrafficTreatment.Builder builder = null;
         ByteBuffer buf = ByteBuffer.wrap(request.serialize());
-        synchronized (externalPorts) {
+        for (ConnectPoint connectPoint : edgeService.getEdgePoints()) {
-            for (Entry<Device, PortNumber> entry : externalPorts.entries()) {
+            if (isOutsidePort(connectPoint) || connectPoint.equals(inPort)) {
-                ConnectPoint cp = new ConnectPoint(entry.getKey().id(), entry.getValue());
-                if (isOutsidePort(cp) || cp.equals(inPort)) {
                 continue;
             }
             builder = DefaultTrafficTreatment.builder();
-                builder.setOutput(entry.getValue());
+            builder.setOutput(connectPoint.port());
-                packetService.emit(new DefaultOutboundPacket(entry.getKey().id(),
+            packetService.emit(new DefaultOutboundPacket(connectPoint.deviceId(),
                     builder.build(), buf));
         }
-        }
-    }
-    /**
-     * Determines the location of all known ports in the system.
-     */
-    private void determinePortLocations() {
-        Iterable<Device> devices = deviceService.getDevices();
-        Iterable<Link> links = null;
-        List<PortNumber> ports = null;
-        for (Device d : devices) {
-            ports = buildPortNumberList(deviceService.getPorts(d.id()));
-            links = linkService.getLinks();
-            for (Link l : links) {
-                // for each link, mark the concerned ports as internal
-                // and the remaining ports are therefore external.
-                if (l.src().deviceId().equals(d.id())
-                        && ports.contains(l.src().port())) {
-                    ports.remove(l.src().port());
-                    internalPorts.put(d, l.src().port());
-                }
-                if (l.dst().deviceId().equals(d.id())
-                        && ports.contains(l.dst().port())) {
-                    ports.remove(l.dst().port());
-                    internalPorts.put(d, l.dst().port());
-                }
-            }
-            synchronized (externalPorts) {
-                externalPorts.putAll(d, ports);
-            }
-        }
-
-    }
-
-    private List<PortNumber> buildPortNumberList(List<Port> ports) {
-        List<PortNumber> portNumbers = Lists.newLinkedList();
-        for (Port p : ports) {
-            portNumbers.add(p.number());
-        }
-        return portNumbers;
     }
     /**
@@ -531,84 +468,4 @@ public class ProxyArpManager implements ProxyArpService {
         eth.setPayload(ipv6);
         return eth;
     }
-
-    public class InternalLinkListener implements LinkListener {
-
-        @Override
-        public void event(LinkEvent event) {
-            Link link = event.subject();
-            Device src = deviceService.getDevice(link.src().deviceId());
-            Device dst = deviceService.getDevice(link.dst().deviceId());
-            switch (event.type()) {
-                case LINK_ADDED:
-                    synchronized (externalPorts) {
-                        externalPorts.remove(src, link.src().port());
-                        externalPorts.remove(dst, link.dst().port());
-                        internalPorts.put(src, link.src().port());
-                        internalPorts.put(dst, link.dst().port());
-                    }
-
-                    break;
-                case LINK_REMOVED:
-                    synchronized (externalPorts) {
-                        externalPorts.put(src, link.src().port());
-                        externalPorts.put(dst, link.dst().port());
-                        internalPorts.remove(src, link.src().port());
-                        internalPorts.remove(dst, link.dst().port());
-                    }
-
-                    break;
-                case LINK_UPDATED:
-                    // don't care about links being updated.
-                    break;
-                default:
-                    break;
-            }
-
-        }
-
-    }
-
-    public class InternalDeviceListener implements DeviceListener {
-
-        @Override
-        public void event(DeviceEvent event) {
-            Device device = event.subject();
-            switch (event.type()) {
-                case DEVICE_ADDED:
-                case DEVICE_AVAILABILITY_CHANGED:
-                case DEVICE_SUSPENDED:
-                case DEVICE_UPDATED:
-                 // nothing to do in these cases; handled when links get reported
-                    break;
-                case DEVICE_REMOVED:
-                    synchronized (externalPorts) {
-                        externalPorts.removeAll(device);
-                        internalPorts.removeAll(device);
-                    }
-                    break;
-                case PORT_ADDED:
-                case PORT_UPDATED:
-                    synchronized (externalPorts) {
-                        if (event.port().isEnabled()) {
-                            externalPorts.put(device, event.port().number());
-                            internalPorts.remove(device, event.port().number());
-                        }
-                    }
-                    break;
-                case PORT_REMOVED:
-                    synchronized (externalPorts) {
-                        externalPorts.remove(device, event.port().number());
-                        internalPorts.remove(device, event.port().number());
-                    }
-                    break;
-                default:
-                    break;
-
-            }
-
-        }
-
-    }
-
 }
--- a/core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java
View file @d812383
+++ b/core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java
View file @d812383
@@ -15,6 +15,7 @@
  */
 package org.onosproject.net.proxyarp.impl;
+import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import org.junit.Before;
 import org.junit.Test;
@@ -36,6 +37,7 @@ import org.onosproject.net.Port;
 import org.onosproject.net.PortNumber;
 import org.onosproject.net.device.DeviceListener;
 import org.onosproject.net.device.DeviceService;
+import org.onosproject.net.edgeservice.impl.EdgeManager;
 import org.onosproject.net.flow.instructions.Instruction;
 import org.onosproject.net.flow.instructions.Instructions.OutputInstruction;
 import org.onosproject.net.host.HostService;
@@ -53,8 +55,14 @@ import java.util.Comparator;
 import java.util.List;
 import java.util.Set;
-import static org.easymock.EasyMock.*;
+import static org.easymock.EasyMock.anyObject;
-import static org.junit.Assert.*;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 /**
  * Tests for the {@link ProxyArpManager} class.
@@ -85,6 +93,11 @@ public class ProxyArpManagerTest {
     private static final HostLocation LOC2 = new HostLocation(DID2, P1, 123L);
     private static final byte[] ZERO_MAC_ADDRESS = MacAddress.ZERO.toBytes();
+    //Return values used for various functions of the TestPacketService inner class.
+    private boolean isEdgePointReturn;
+    private List<ConnectPoint> getEdgePointsNoArg;
+
+
     private ProxyArpManager proxyArp;
     private TestPacketService packetService;
@@ -98,6 +111,8 @@ public class ProxyArpManagerTest {
         packetService = new TestPacketService();
         proxyArp.packetService = packetService;
+        proxyArp.edgeService = new TestEdgePortService();
+
         // Create a host service mock here. Must be replayed by tests once the
         // expectations have been set up
         hostService = createMock(HostService.class);
@@ -112,7 +127,7 @@ public class ProxyArpManagerTest {
     /**
      * Creates a fake topology to feed into the ARP module.
-     * <p/>
+     * <p>
      * The default topology is a unidirectional ring topology. Each switch has
      * 3 ports. Ports 2 and 3 have the links to neighbor switches, and port 1
      * is free (edge port).
@@ -266,6 +281,9 @@ public class ProxyArpManagerTest {
      */
     @Test
     public void testReplyKnown() {
+        //Set the return value of isEdgePoint from the edgemanager.
+        isEdgePointReturn = true;
+
         Host replyer = new DefaultHost(PID, HID1, MAC1, VLAN1, getLocation(4),
                 Collections.singleton(IP1));
@@ -294,6 +312,8 @@ public class ProxyArpManagerTest {
      */
     @Test
     public void testReplyUnknown() {
+        isEdgePointReturn = true;
+
         Host requestor = new DefaultHost(PID, HID2, MAC2, VLAN1, getLocation(5),
                 Collections.singleton(IP2));
@@ -301,10 +321,16 @@ public class ProxyArpManagerTest {
                 .andReturn(Collections.<Host>emptySet());
         expect(hostService.getHost(HID2)).andReturn(requestor);
+
         replay(hostService);
         Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC2, null, IP2, IP1);
+        //Setup the set of edge ports to be used in the reply method
+        getEdgePointsNoArg = Lists.newLinkedList();
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("5"), PortNumber.portNumber(1)));
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("4"), PortNumber.portNumber(1)));
+
         proxyArp.reply(arpRequest, getLocation(6));
         verifyFlood(arpRequest);
@@ -318,6 +344,7 @@ public class ProxyArpManagerTest {
      */
     @Test
     public void testReplyDifferentVlan() {
+
         Host replyer = new DefaultHost(PID, HID1, MAC1, VLAN2, getLocation(4),
                 Collections.singleton(IP1));
@@ -332,6 +359,10 @@ public class ProxyArpManagerTest {
         Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC2, null, IP2, IP1);
+        //Setup for flood test
+        getEdgePointsNoArg = Lists.newLinkedList();
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("5"), PortNumber.portNumber(1)));
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("4"), PortNumber.portNumber(1)));
         proxyArp.reply(arpRequest, getLocation(6));
         verifyFlood(arpRequest);
@@ -352,7 +383,7 @@ public class ProxyArpManagerTest {
         replay(hostService);
         Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC2, null, theirIp, ourFirstIp);
-
+        isEdgePointReturn = true;
         proxyArp.reply(arpRequest, LOC1);
         assertEquals(1, packetService.packets.size());
@@ -402,8 +433,10 @@ public class ProxyArpManagerTest {
         // This is a request from something inside our network (like a BGP
         // daemon) to an external host.
         Ethernet arpRequest = buildArp(ARP.OP_REQUEST, ourMac, null, ourIp, theirIp);
-        proxyArp.reply(arpRequest, getLocation(5));
+        //Ensure the packet is allowed through (it is not to an internal port)
+        isEdgePointReturn = true;
+        proxyArp.reply(arpRequest, getLocation(5));
         assertEquals(1, packetService.packets.size());
         verifyPacketOut(arpRequest, getLocation(1), packetService.packets.get(0));
@@ -448,6 +481,13 @@ public class ProxyArpManagerTest {
         Ethernet arpRequest = buildArp(ARP.OP_REPLY, MAC2, MAC1, IP2, IP1);
+        //populate the list of edges when so that when forward hits flood in the manager it contains the values
+        //that should continue on
+        getEdgePointsNoArg = Lists.newLinkedList();
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("3"), PortNumber.portNumber(1)));
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("5"), PortNumber.portNumber(1)));
+        getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("4"), PortNumber.portNumber(1)));
+
         proxyArp.forward(arpRequest, getLocation(6));
         verifyFlood(arpRequest);
@@ -572,4 +612,17 @@ public class ProxyArpManagerTest {
         }
     }
+
+    class TestEdgePortService extends EdgeManager {
+
+        @Override
+        public boolean isEdgePoint(ConnectPoint connectPoint) {
+            return isEdgePointReturn;
+        }
+
+        @Override
+        public Iterable<ConnectPoint> getEdgePoints() {
+            return getEdgePointsNoArg;
+        }
+    }
 }