홍길동 / onos

Go to a project
Toggle navigation Toggle navigation pinning
HIGUCHI Yuta
Committed by Gerrit Code Review
9a9edf87 1 parent d25a30a1

ONOS-3206 refactoring LLDP link provider. 

- refactoring to unify LinkDiscovery helper instantiation
- Support suppression rule update at runtime

Change-Id: I2a6db6e82fcb90ee5635f0ac09564efd55276ebf
Inline Side-by-side
Showing 3 changed files with 255 additions and 89 deletions
......@@ -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()) {
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.debug("LinkDiscovery from {} disabled by configuration", device.id());
continue;
log.trace("LinkDiscovery from {} disabled by configuration", device.id());
removeDevice(device.id());
return Optional.empty();
}
LinkDiscovery ld = new LinkDiscovery(device, context);
discoverers.put(device.id(), ld);
addPorts(ld, device.id());
LinkDiscovery ld = discoverers.computeIfAbsent(device.id(),
did -> new LinkDiscovery(device, context));
if (ld.isStopped()) {
ld.start();
}
return Optional.of(ld);
}
/**
* Removes after stopping discovery helper for specified device.
* @param deviceId device to remove
*/
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));
}
/**
* Adds ports of the specified device to the specified discovery helper.
* 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 addPorts(LinkDiscovery discoverer, DeviceId deviceId) {
for (Port p : deviceService.getPorts(deviceId)) {
if (rules.isSuppressed(p)) {
continue;
private void updatePort(LinkDiscovery discoverer, Port port) {
if (rules.isSuppressed(port)) {
log.trace("LinkDiscovery from {} disabled by configuration", port);
removePort(port);
return;
}
if (!p.number().isLogical()) {
discoverer.addPort(p);
// 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);
......
......@@ -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.
*
......
......@@ -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
......