diff options
Diffstat (limited to 'framework/src/onos/providers/lldp')
3 files changed, 259 insertions, 93 deletions
diff --git a/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LLDPLinkProvider.java b/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LLDPLinkProvider.java index a840f856..98442033 100644 --- a/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LLDPLinkProvider.java +++ b/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LLDPLinkProvider.java @@ -27,6 +27,7 @@ import org.apache.felix.scr.annotations.Reference; import org.apache.felix.scr.annotations.ReferenceCardinality; import org.onlab.packet.Ethernet; import org.onosproject.cfg.ComponentConfigService; +import org.onosproject.cluster.ClusterService; import org.onosproject.core.ApplicationId; import org.onosproject.core.CoreService; import org.onosproject.mastership.MastershipEvent; @@ -37,6 +38,7 @@ import org.onosproject.net.Device; import org.onosproject.net.DeviceId; import org.onosproject.net.LinkKey; import org.onosproject.net.Port; +import org.onosproject.net.config.NetworkConfigRegistry; import org.onosproject.net.device.DeviceEvent; import org.onosproject.net.device.DeviceListener; import org.onosproject.net.device.DeviceService; @@ -60,6 +62,7 @@ import java.io.IOException; import java.util.Dictionary; import java.util.EnumSet; import java.util.Map; +import java.util.Optional; import java.util.Properties; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ScheduledExecutorService; @@ -86,6 +89,10 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { "Settings: enabled={}, useBDDP={}, probeRate={}, " + "staleLinkAge={}, lldpSuppression={}"; + // When a Device/Port has this annotation, do not send out LLDP/BDDP + public static final String NO_LLDP = "no-lldp"; + + private final Logger log = getLogger(getClass()); @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) @@ -109,6 +116,12 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) protected ComponentConfigService cfgService; + @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) + protected ClusterService clusterService; + + @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) + protected NetworkConfigRegistry cfgRegistry; + private LinkProviderService providerService; private ScheduledExecutorService executor; @@ -207,7 +220,7 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { newLldpSuppression = isNullOrEmpty(s) ? DEFAULT_LLDP_SUPPRESSION_CONFIG : s; } catch (NumberFormatException e) { - log.warn(e.getMessage()); + log.warn("Component configuration had invalid values", e); newEnabled = enabled; newUseBddp = useBDDP; newProbeRate = probeRate; @@ -227,6 +240,14 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { enable(); } else if (wasEnabled && !enabled) { disable(); + } else { + // reflect changes in suppression rules to discovery helpers + // FIXME: After migrating to Network Configuration Subsystem, + // it should be possible to update only changed subset + if (enabled) { + // update all discovery helper state + loadDevices(); + } } log.info(FORMAT, enabled, useBDDP, probeRate, staleLinkAge, lldpSuppression); @@ -277,31 +298,96 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { * Loads available devices and registers their ports to be probed. */ private void loadDevices() { - for (Device device : deviceService.getAvailableDevices()) { - if (rules.isSuppressed(device)) { - log.debug("LinkDiscovery from {} disabled by configuration", device.id()); - continue; - } - LinkDiscovery ld = new LinkDiscovery(device, context); - discoverers.put(device.id(), ld); - addPorts(ld, device.id()); + deviceService.getAvailableDevices() + .forEach(d -> updateDevice(d) + .ifPresent(ld -> updatePorts(ld, d.id()))); + } + + /** + * Updates discovery helper for specified device. + * + * Adds and starts a discovery helper for specified device if enabled, + * calls {@link #removeDevice(DeviceId)} otherwise. + * + * @param device device to add + * @return discovery helper if discovery is enabled for the device + */ + private Optional<LinkDiscovery> updateDevice(Device device) { + if (rules.isSuppressed(device)) { + log.trace("LinkDiscovery from {} disabled by configuration", device.id()); + removeDevice(device.id()); + return Optional.empty(); + } + LinkDiscovery ld = discoverers.computeIfAbsent(device.id(), + did -> new LinkDiscovery(device, context)); + if (ld.isStopped()) { + ld.start(); } + return Optional.of(ld); } /** - * Adds ports of the specified device to the specified discovery helper. + * Removes after stopping discovery helper for specified device. + * @param deviceId device to remove */ - private void addPorts(LinkDiscovery discoverer, DeviceId deviceId) { - for (Port p : deviceService.getPorts(deviceId)) { - if (rules.isSuppressed(p)) { - continue; - } - if (!p.number().isLogical()) { - discoverer.addPort(p); - } + private void removeDevice(final DeviceId deviceId) { + discoverers.computeIfPresent(deviceId, (did, ld) -> { + ld.stop(); + providerService.linksVanished(deviceId); + return null; + }); + + } + + /** + * Updates ports of the specified device to the specified discovery helper. + */ + private void updatePorts(LinkDiscovery discoverer, DeviceId deviceId) { + deviceService.getPorts(deviceId).forEach(p -> updatePort(discoverer, p)); + } + + /** + * Updates discovery helper state of the specified port. + * + * Adds a port to the discovery helper if up and discovery is enabled, + * or calls {@link #removePort(Port)} otherwise. + */ + private void updatePort(LinkDiscovery discoverer, Port port) { + if (rules.isSuppressed(port)) { + log.trace("LinkDiscovery from {} disabled by configuration", port); + removePort(port); + return; + } + + // check if enabled and turn off discovery? + if (!port.isEnabled()) { + removePort(port); + return; + } + + if (!port.number().isLogical()) { + discoverer.addPort(port); } } + /** + * Removes a port from the specified discovery helper. + * @param port the port + */ + private void removePort(Port port) { + if (port.element() instanceof Device) { + Device d = (Device) port.element(); + LinkDiscovery ld = discoverers.get(d.id()); + if (ld != null) { + ld.removePort(port.number()); + } + + ConnectPoint point = new ConnectPoint(d.id(), port.number()); + providerService.linksVanished(point); + } else { + log.warn("Attempted to remove non-Device port", port); + } + } /** * Loads LLDP suppression rules. @@ -317,7 +403,7 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { // default rule to suppress ROADM to maintain compatibility rules = new SuppressionRules(ImmutableSet.of(), EnumSet.of(Device.Type.ROADM), - ImmutableMap.of()); + ImmutableMap.of(NO_LLDP, SuppressionRules.ANY_VALUE)); } // should refresh discoverers when we need dynamic reconfiguration @@ -367,10 +453,9 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { log.debug("Device {} doesn't exist, or isn't there yet", deviceId); return; } - if (rules.isSuppressed(device)) { - return; + if (clusterService.getLocalNode().id().equals(event.roleInfo().master())) { + updateDevice(device).ifPresent(ld -> updatePorts(ld, device.id())); } - discoverers.computeIfAbsent(deviceId, k -> new LinkDiscovery(device, context)); } } @@ -381,7 +466,6 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { private class InternalDeviceListener implements DeviceListener { @Override public void event(DeviceEvent event) { - LinkDiscovery ld; Device device = event.subject(); Port port = event.port(); if (device == null) { @@ -393,73 +477,33 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { switch (event.type()) { case DEVICE_ADDED: case DEVICE_UPDATED: - synchronized (discoverers) { - ld = discoverers.get(deviceId); - if (ld == null) { - if (rules != null && rules.isSuppressed(device)) { - log.debug("LinkDiscovery from {} disabled by configuration", device.id()); - return; - } - log.debug("Device added ({}) {}", event.type(), deviceId); - discoverers.put(deviceId, new LinkDiscovery(device, context)); - } else { - if (ld.isStopped()) { - log.debug("Device restarted ({}) {}", event.type(), deviceId); - ld.start(); - } - } - } + updateDevice(device).ifPresent(ld -> updatePorts(ld, deviceId)); break; case PORT_ADDED: case PORT_UPDATED: if (port.isEnabled()) { - ld = discoverers.get(deviceId); - if (ld == null) { - return; - } - if (rules.isSuppressed(port)) { - log.debug("LinkDiscovery from {}@{} disabled by configuration", - port.number(), device.id()); - return; - } - if (!port.number().isLogical()) { - log.debug("Port added {}", port); - ld.addPort(port); - } + updateDevice(device).ifPresent(ld -> updatePort(ld, port)); } else { log.debug("Port down {}", port); - ConnectPoint point = new ConnectPoint(deviceId, port.number()); - providerService.linksVanished(point); + removePort(port); } break; case PORT_REMOVED: log.debug("Port removed {}", port); - ConnectPoint point = new ConnectPoint(deviceId, port.number()); - providerService.linksVanished(point); - + removePort(port); break; case DEVICE_REMOVED: case DEVICE_SUSPENDED: log.debug("Device removed {}", deviceId); - ld = discoverers.get(deviceId); - if (ld == null) { - return; - } - ld.stop(); - providerService.linksVanished(deviceId); + removeDevice(deviceId); break; case DEVICE_AVAILABILITY_CHANGED: - ld = discoverers.get(deviceId); - if (ld == null) { - return; - } if (deviceService.isAvailable(deviceId)) { log.debug("Device up {}", deviceId); - ld.start(); + updateDevice(device); } else { - providerService.linksVanished(deviceId); log.debug("Device down {}", deviceId); - ld.stop(); + removeDevice(deviceId); } break; case PORT_STATS_UPDATED: @@ -508,17 +552,7 @@ public class LLDPLinkProvider extends AbstractProvider implements LinkProvider { } // check what deviceService sees, to see if we are missing anything try { - for (Device dev : deviceService.getDevices()) { - if (rules.isSuppressed(dev)) { - continue; - } - DeviceId did = dev.id(); - synchronized (discoverers) { - LinkDiscovery ld = discoverers - .computeIfAbsent(did, k -> new LinkDiscovery(dev, context)); - addPorts(ld, did); - } - } + loadDevices(); } catch (Exception e) { // Catch all exceptions to avoid task being suppressed log.error("Exception thrown during synchronization process", e); diff --git a/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LinkDiscovery.java b/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LinkDiscovery.java index 8cdfd50f..7dc9aed0 100644 --- a/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LinkDiscovery.java +++ b/framework/src/onos/providers/lldp/src/main/java/org/onosproject/provider/lldp/impl/LinkDiscovery.java @@ -61,7 +61,7 @@ class LinkDiscovery implements TimerTask { private final ONOSLLDP lldpPacket; private final Ethernet ethPacket; - private Ethernet bddpEth; + private final Ethernet bddpEth; private Timeout timeout; private volatile boolean isStopped; @@ -126,7 +126,7 @@ class LinkDiscovery implements TimerTask { } /** - * Add physical port port to discovery process. + * Add physical port to discovery process. * Send out initial LLDP and label it as slow port. * * @param port the port @@ -141,6 +141,14 @@ class LinkDiscovery implements TimerTask { } /** + * removed physical port from discovery process. + * @param port the port number + */ + void removePort(PortNumber port) { + ports.remove(port.toLong()); + } + + /** * Handles an incoming LLDP packet. Creates link in topology and adds the * link for staleness tracking. * diff --git a/framework/src/onos/providers/lldp/src/test/java/org/onosproject/provider/lldp/impl/LLDPLinkProviderTest.java b/framework/src/onos/providers/lldp/src/test/java/org/onosproject/provider/lldp/impl/LLDPLinkProviderTest.java index b4b7b7b6..6070b857 100644 --- a/framework/src/onos/providers/lldp/src/test/java/org/onosproject/provider/lldp/impl/LLDPLinkProviderTest.java +++ b/framework/src/onos/providers/lldp/src/test/java/org/onosproject/provider/lldp/impl/LLDPLinkProviderTest.java @@ -16,6 +16,7 @@ package org.onosproject.provider.lldp.impl; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import org.junit.After; @@ -32,7 +33,9 @@ import org.onosproject.core.CoreService; import org.onosproject.core.DefaultApplicationId; import org.onosproject.mastership.MastershipListener; import org.onosproject.mastership.MastershipService; +import org.onosproject.net.Annotations; import org.onosproject.net.ConnectPoint; +import org.onosproject.net.DefaultAnnotations; import org.onosproject.net.DefaultDevice; import org.onosproject.net.DefaultPort; import org.onosproject.net.Device; @@ -73,6 +76,7 @@ public class LLDPLinkProviderTest { private static final DeviceId DID1 = DeviceId.deviceId("of:0000000000000001"); private static final DeviceId DID2 = DeviceId.deviceId("of:0000000000000002"); + private static final DeviceId DID3 = DeviceId.deviceId("of:0000000000000003"); private static Port pd1; private static Port pd2; @@ -133,10 +137,35 @@ public class LLDPLinkProviderTest { deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID1)); deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_REMOVED, DID1)); - assertTrue("Discoverer is not gone", provider.discoverers.get(DID1).isStopped()); + final LinkDiscovery linkDiscovery = provider.discoverers.get(DID1); + if (linkDiscovery != null) { + // If LinkDiscovery helper is there after DEVICE_REMOVED, + // it should be stopped + assertTrue("Discoverer is not stopped", linkDiscovery.isStopped()); + } assertTrue("Device is not gone.", vanishedDpid(DID1)); } + /** + * Checks that links on a reconfigured switch are properly removed. + */ + @Test + public void switchSuppressed() { + // add device to stub DeviceService + deviceService.putDevice(device(DID3)); + deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID3)); + + assertFalse("Device not added", provider.discoverers.isEmpty()); + + // update device in stub DeviceService with suppression config + deviceService.putDevice(device(DID3, DefaultAnnotations.builder() + .set(LLDPLinkProvider.NO_LLDP, "true") + .build())); + deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_UPDATED, DID3)); + + assertTrue("Links on suppressed Device was expected to vanish.", vanishedDpid(DID3)); + } + @Test public void portUp() { deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID1)); @@ -152,27 +181,101 @@ public class LLDPLinkProviderTest { deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID1)); deviceListener.event(portEvent(DeviceEvent.Type.PORT_ADDED, DID1, port(DID1, 1, false))); - - assertFalse("Port added to discoverer", provider.discoverers.get(DID1).containsPort(1L)); assertTrue("Port is not gone.", vanishedPort(1L)); } @Test + public void portRemoved() { + deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID1)); + deviceListener.event(portEvent(DeviceEvent.Type.PORT_ADDED, DID1, port(DID1, 3, true))); + deviceListener.event(portEvent(DeviceEvent.Type.PORT_REMOVED, DID1, port(DID1, 3, true))); + + assertTrue("Port is not gone.", vanishedPort(3L)); + assertFalse("Port was not removed from discoverer", + provider.discoverers.get(DID1).containsPort(3L)); + } + + /** + * Checks that discovery on reconfigured switch are properly restarted. + */ + @Test + public void portSuppressedByDeviceConfig() { + + /// When Device is configured with suppression:ON, Port also is same + + // add device in stub DeviceService with suppression configured + deviceService.putDevice(device(DID3, DefaultAnnotations.builder() + .set(LLDPLinkProvider.NO_LLDP, "true") + .build())); + deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID3)); + + // non-suppressed port added to suppressed device + final long portno3 = 3L; + deviceService.putPorts(DID3, port(DID3, portno3, true)); + deviceListener.event(portEvent(DeviceEvent.Type.PORT_ADDED, DID3, port(DID3, portno3, true))); + + // discovery on device is expected to be stopped + LinkDiscovery linkDiscovery = provider.discoverers.get(DID3); + if (linkDiscovery != null) { + assertTrue("Discovery expected to be stopped", linkDiscovery.isStopped()); + } + + /// When Device is reconfigured without suppression:OFF, + /// Port should be included for discovery + + // update device in stub DeviceService without suppression configured + deviceService.putDevice(device(DID3)); + // update the Port in stub DeviceService. (Port has reference to Device) + deviceService.putPorts(DID3, port(DID3, portno3, true)); + deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_UPDATED, DID3)); + + // discovery should come back on + assertFalse("Discoverer is expected to start", provider.discoverers.get(DID3).isStopped()); + assertTrue("Discoverer should contain the port there", provider.discoverers.get(DID3).containsPort(portno3)); + } + + /** + * Checks that discovery on reconfigured port are properly restarted. + */ + @Test + public void portSuppressedByPortConfig() { + // add device in stub DeviceService without suppression configured + deviceService.putDevice(device(DID3)); + deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID3)); + + // suppressed port added to non-suppressed device + final long portno3 = 3L; + final Port port3 = port(DID3, portno3, true, + DefaultAnnotations.builder() + .set(LLDPLinkProvider.NO_LLDP, "true") + .build()); + deviceService.putPorts(DID3, port3); + deviceListener.event(portEvent(DeviceEvent.Type.PORT_ADDED, DID3, port3)); + + // discovery helper should be there turned on + assertFalse("Discoverer is expected to start", provider.discoverers.get(DID3).isStopped()); + assertFalse("Discoverer should not contain the port there", + provider.discoverers.get(DID3).containsPort(portno3)); + } + + @Test public void portUnknown() { deviceListener.event(deviceEvent(DeviceEvent.Type.DEVICE_ADDED, DID1)); - deviceListener.event(portEvent(DeviceEvent.Type.PORT_ADDED, DID2, port(DID2, 1, false))); + // Note: DID3 hasn't been added to TestDeviceService, but only port is added + deviceListener.event(portEvent(DeviceEvent.Type.PORT_ADDED, DID3, port(DID3, 1, false))); assertNull("DeviceId exists", - provider.discoverers.get(DID2)); + provider.discoverers.get(DID3)); } @Test public void unknownPktCtx() { - PacketContext pktCtx = new TestPacketContext(deviceService.getDevice(DID2)); + // Note: DID3 hasn't been added to TestDeviceService + PacketContext pktCtx = new TestPacketContext(device(DID3)); testProcessor.process(pktCtx); assertFalse("Context should still be free", pktCtx.isHandled()); @@ -206,6 +309,16 @@ public class LLDPLinkProviderTest { } + private DefaultDevice device(DeviceId did) { + return new DefaultDevice(ProviderId.NONE, did, Device.Type.SWITCH, + "TESTMF", "TESTHW", "TESTSW", "TESTSN", new ChassisId()); + } + + private DefaultDevice device(DeviceId did, Annotations annotations) { + return new DefaultDevice(ProviderId.NONE, did, Device.Type.SWITCH, + "TESTMF", "TESTHW", "TESTSW", "TESTSN", new ChassisId(), annotations); + } + @SuppressWarnings(value = { "unused" }) private DeviceEvent portEvent(DeviceEvent.Type type, DeviceId did, PortNumber port) { return new DeviceEvent(type, deviceService.getDevice(did), @@ -221,6 +334,10 @@ public class LLDPLinkProviderTest { PortNumber.portNumber(port), enabled); } + private Port port(DeviceId did, long port, boolean enabled, Annotations annotations) { + return new DefaultPort(deviceService.getDevice(did), + PortNumber.portNumber(port), enabled, annotations); + } private boolean vanishedDpid(DeviceId... dids) { for (int i = 0; i < dids.length; i++) { @@ -384,10 +501,9 @@ public class LLDPLinkProviderTest { private class TestDeviceService extends DeviceServiceAdapter { - private Map<DeviceId, Device> devices = new HashMap<>(); + private final Map<DeviceId, Device> devices = new HashMap<>(); private final ArrayListMultimap<DeviceId, Port> ports = ArrayListMultimap.create(); - public TestDeviceService() { Device d1 = new DefaultDevice(ProviderId.NONE, DID1, Device.Type.SWITCH, "TESTMF", "TESTHW", "TESTSW", "TESTSN", new ChassisId()); @@ -395,7 +511,6 @@ public class LLDPLinkProviderTest { "TESTMF", "TESTHW", "TESTSW", "TESTSN", new ChassisId()); devices.put(DID1, d1); devices.put(DID2, d2); - pd1 = new DefaultPort(d1, PortNumber.portNumber(1), true); pd2 = new DefaultPort(d1, PortNumber.portNumber(2), true); pd3 = new DefaultPort(d2, PortNumber.portNumber(1), true); @@ -405,6 +520,15 @@ public class LLDPLinkProviderTest { ports.putAll(DID2, Lists.newArrayList(pd3, pd4)); } + private void putDevice(Device device) { + DeviceId deviceId = device.id(); + devices.put(deviceId, device); + } + + private void putPorts(DeviceId did, Port...ports) { + this.ports.putAll(did, Lists.newArrayList(ports)); + } + @Override public int getDeviceCount() { return devices.values().size(); @@ -412,7 +536,7 @@ public class LLDPLinkProviderTest { @Override public Iterable<Device> getDevices() { - return Collections.emptyList(); + return ImmutableList.copyOf(devices.values()); } @Override |