From c5061230cdddcfef56d1845334f3c8d75116d6e7 Mon Sep 17 00:00:00 2001
From: Gregor Maier <gregor.maier@bigswitch.com>
Date: Fri, 9 Nov 2012 15:23:29 -0800
Subject: [PATCH] Several improvements and bug fixes for Device Manager:
BSC-2837: When returning early from learnDeviceByEntity: process
deleteQueue
BSC-2836: Do not learn entities on internal ports
BSC-2834: Some device updates get queued before check for concurrent
modification
BSC-2824: Remove potential incorrect Corrupted Device Index exception
---
.../internal/DeviceManagerImpl.java | 133 ++++++++-------
.../devicemanager/internal/Entity.java | 6 +
.../internal/DeviceManagerImplTest.java | 160 ++++++++++++++++--
3 files changed, 222 insertions(+), 77 deletions(-)
diff --git a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
index 6c6aa4cef..db04452f0 100755
--- a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
+++ b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
@@ -1052,7 +1052,8 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
entityClass = entityClassifier.classifyEntity(entity);
if (entityClass == null) {
// could not classify entity. No device
- return null;
+ device = null;
+ break;
}
ClassState classState = getClassState(entityClass);
@@ -1066,17 +1067,36 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
// use resulting device key to look up the device in the
// device map, and use the referenced Device below.
device = deviceMap.get(deviceKey);
- if (device == null)
- throw new IllegalStateException("Corrupted device index");
+ if (device == null) {
+ // This can happen due to concurrent modification
+ if (logger.isDebugEnabled()) {
+ logger.debug("No device for deviceKey {} while "
+ + "while processing entity {}",
+ deviceKey, entity);
+ }
+ }
} else {
// If the secondary index does not contain the entity,
// create a new Device object containing the entity, and
- // generate a new device ID. However, we first check if
+ // generate a new device ID if the the entity is on an
+ // attachment point port. Otherwise ignore.
+ if (entity.hasSwitchPort() &&
+ !this.isValidAttachmentPoint(entity.getSwitchDPID(),
+ entity.getSwitchPort())) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Not learning new device on internal"
+ + " link: {}", entity);
+ }
+ device = null;
+ break;
+ }
+ // Before we create the new device also check if
// the entity is allowed (e.g., for spoofing protection)
if (!isEntityAllowed(entity, entityClass)) {
logger.info("PacketIn is not allowed {} {}",
entityClass.getName(), entity);
- return null;
+ device = null;
+ break;
}
synchronized (deviceKeyLock) {
deviceKey = Long.valueOf(deviceKeyCounter++);
@@ -1113,59 +1133,36 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
device.getEntityClass().getName(), entity);
return null;
}
+ // If this is an internal port we don't learn the new entity
+ // and don't update indexes. We only learn on attachment point
+ // ports.
+ if (entity.hasSwitchPort() &&
+ !this.isValidAttachmentPoint(entity.getSwitchDPID(),
+ entity.getSwitchPort())) {
+ break;
+ }
int entityindex = -1;
if ((entityindex = device.entityIndex(entity)) >= 0) {
+ // Entity already exists
// update timestamp on the found entity
Date lastSeen = entity.getLastSeenTimestamp();
- if (lastSeen == null) lastSeen = new Date();
- device.entities[entityindex].setLastSeenTimestamp(lastSeen);
- if (device.entities[entityindex].getSwitchDPID() != null &&
- device.entities[entityindex].getSwitchPort() != null) {
- long sw = device.entities[entityindex].getSwitchDPID();
- short port = device.entities[entityindex].getSwitchPort().shortValue();
-
- boolean moved =
- device.updateAttachmentPoint(sw,
- port,
- lastSeen.getTime());
-
- if (moved) {
- sendDeviceMovedNotification(device);
- if (logger.isTraceEnabled()) {
- logger.trace("Device moved: attachment points {}," +
- "entities {}", device.attachmentPoints,
- device.entities);
- }
- } else {
- if (logger.isTraceEnabled()) {
- logger.trace("Device attachment point NOT updated: " +
- "attachment points {}," +
- "entities {}", device.attachmentPoints,
- device.entities);
- }
- }
+ if (lastSeen == null) {
+ lastSeen = new Date();
+ entity.setLastSeenTimestamp(lastSeen);
}
- break;
+ device.entities[entityindex].setLastSeenTimestamp(lastSeen);
+ // we break the loop after the else block and after checking
+ // for new AP
} else {
- boolean moved = false;
+ // New entity for this devce
// compute the insertion point for the entity.
// see Arrays.binarySearch()
entityindex = -(entityindex + 1);
Device newDevice = allocateDevice(device, entity, entityindex);
- if (entity.getSwitchDPID() != null && entity.getSwitchPort() != null) {
- moved = newDevice.updateAttachmentPoint(entity.getSwitchDPID(),
- entity.getSwitchPort().shortValue(),
- entity.getLastSeenTimestamp().getTime());
- }
-
+
// generate updates
EnumSet<DeviceField> changedFields =
findChangedFields(device, entity);
- if (changedFields.size() > 0)
- deviceUpdates =
- updateUpdates(deviceUpdates,
- new DeviceUpdate(newDevice, CHANGE,
- changedFields));
// update the device map with a replace call
boolean res = deviceMap.replace(deviceKey, device, newDevice);
@@ -1176,7 +1173,6 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
continue;
device = newDevice;
-
// update indices
if (!updateIndices(device, deviceKey)) {
continue;
@@ -1184,36 +1180,45 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
updateSecondaryIndices(entity,
device.getEntityClass(),
deviceKey);
-
+
+ if (changedFields.size() > 0) {
+ deviceUpdates =
+ updateUpdates(deviceUpdates,
+ new DeviceUpdate(newDevice, CHANGE,
+ changedFields));
+ }
+ // we break the loop after checking for changed AP
+ }
+ // Update attachment point (will only be hit if the device
+ // already existed and no concurrent modification
+ if (entity.hasSwitchPort()) {
+ boolean moved =
+ device.updateAttachmentPoint(entity.getSwitchDPID(),
+ entity.getSwitchPort().shortValue(),
+ entity.getLastSeenTimestamp().getTime());
if (moved) {
sendDeviceMovedNotification(device);
- if (logger.isDebugEnabled()) {
- logger.debug("Device moved: attachment points {}," +
+ if (logger.isTraceEnabled()) {
+ logger.trace("Device moved: attachment points {}," +
"entities {}", device.attachmentPoints,
device.entities);
}
} else {
- if (logger.isDebugEnabled()) {
- logger.debug("Device attachment point updated: " +
+ if (logger.isTraceEnabled()) {
+ logger.trace("Device attachment point updated: " +
"attachment points {}," +
"entities {}", device.attachmentPoints,
device.entities);
}
}
- break;
}
+ break;
}
if (deleteQueue != null) {
for (Long l : deleteQueue) {
Device dev = deviceMap.get(l);
this.deleteDevice(dev);
-
-
- // generate new device update
- deviceUpdates =
- updateUpdates(deviceUpdates,
- new DeviceUpdate(dev, DELETE, null));
}
}
@@ -1482,9 +1487,9 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
for (Entity e : toRemove) {
changedFields.addAll(findChangedFields(newDevice, e));
}
+ DeviceUpdate update = null;
if (changedFields.size() > 0)
- deviceUpdates.add(new DeviceUpdate(d, CHANGE,
- changedFields));
+ update = new DeviceUpdate(d, CHANGE, changedFields);
if (!deviceMap.replace(newDevice.getDeviceKey(),
d,
@@ -1496,15 +1501,19 @@ IFlowReconcileListener, IInfoProvider, IHAListener {
if (null != d)
continue;
}
+ if (update != null)
+ deviceUpdates.add(update);
} else {
- deviceUpdates.add(new DeviceUpdate(d, DELETE, null));
- if (!deviceMap.remove(d.getDeviceKey(), d))
+ DeviceUpdate update = new DeviceUpdate(d, DELETE, null);
+ if (!deviceMap.remove(d.getDeviceKey(), d)) {
// concurrent modification; try again
// need to use device that is the map now for the next
// iteration
d = deviceMap.get(d.getDeviceKey());
if (null != d)
continue;
+ }
+ deviceUpdates.add(update);
}
processUpdates(deviceUpdates);
break;
diff --git a/src/main/java/net/floodlightcontroller/devicemanager/internal/Entity.java b/src/main/java/net/floodlightcontroller/devicemanager/internal/Entity.java
index 36c5471fb..5949a19e2 100644
--- a/src/main/java/net/floodlightcontroller/devicemanager/internal/Entity.java
+++ b/src/main/java/net/floodlightcontroller/devicemanager/internal/Entity.java
@@ -24,6 +24,7 @@ import net.floodlightcontroller.core.web.serializers.MACSerializer;
import net.floodlightcontroller.core.web.serializers.DPIDSerializer;
import net.floodlightcontroller.packet.IPv4;
+import org.codehaus.jackson.annotate.JsonIgnore;
import org.codehaus.jackson.map.annotate.JsonSerialize;
import org.openflow.util.HexString;
@@ -145,6 +146,11 @@ public class Entity implements Comparable<Entity> {
public Integer getSwitchPort() {
return switchPort;
}
+
+ @JsonIgnore
+ public boolean hasSwitchPort() {
+ return (switchDPID != null && switchPort != null);
+ }
public Date getLastSeenTimestamp() {
return lastSeenTimestamp;
diff --git a/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java b/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
index 855a23ffd..5ac0a9c3b 100644
--- a/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
+++ b/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
@@ -567,11 +567,131 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
assertArrayEquals(new Integer[] { 1 }, ips);
verify(mockListener);
}
+
+ private void verifyEntityArray(Entity[] expected, Device d) {
+ Arrays.sort(expected);
+ assertArrayEquals(expected, d.entities);
+ }
+
+ @Test
+ public void testNoLearningOnInternalPorts() throws Exception {
+ IDeviceListener mockListener =
+ createMock(IDeviceListener.class);
+
+ deviceManager.addListener(mockListener);
+
+ ITopologyService mockTopology = createMock(ITopologyService.class);
+ expect(mockTopology.getL2DomainId(1L)).
+ andReturn(1L).anyTimes();
+ expect(mockTopology.getL2DomainId(2L)).
+ andReturn(1L).anyTimes();
+ expect(mockTopology.getL2DomainId(3L)).
+ andReturn(1L).anyTimes();
+ expect(mockTopology.getL2DomainId(4L)).
+ andReturn(1L).anyTimes();
+ expect(mockTopology.isBroadcastDomainPort(anyLong(), anyShort()))
+ .andReturn(false).anyTimes();
+ expect(mockTopology.isInSameBroadcastDomain(anyLong(), anyShort(),
+ anyLong(), anyShort()))
+ .andReturn(false).anyTimes();
+
+ expect(mockTopology.isAttachmentPointPort(or(eq(1L), eq(3L)), anyShort()))
+ .andReturn(true).anyTimes();
+ // Switches 2 and 4 have only internal ports
+ expect(mockTopology.isAttachmentPointPort(or(eq(2L), eq(4L)), anyShort()))
+ .andReturn(false).anyTimes();
+
+ expect(mockTopology.isConsistent(1L, (short)1, 3L, (short)1))
+ .andReturn(false).once();
+
+ Date topologyUpdateTime = new Date();
+ expect(mockTopology.getLastUpdateTime()).andReturn(topologyUpdateTime).
+ anyTimes();
+
+ replay(mockTopology);
+
+ deviceManager.topology = mockTopology;
+
+ Calendar c = Calendar.getInstance();
+ Entity entity1 = new Entity(1L, null, 1, 1L, 1, c.getTime());
+ c.add(Calendar.SECOND, 1);
+ Entity entity2 = new Entity(1L, null, 2, 2L, 1, c.getTime());
+ c.add(Calendar.SECOND, 1);
+ Entity entity3 = new Entity(1L, null, 3, 3L, 1, c.getTime());
+ c.add(Calendar.SECOND, 1);
+ Entity entity4 = new Entity(1L, null, 4, 4L, 1, c.getTime());
+ IDevice d;
+ SwitchPort[] aps;
+ Integer[] ips;
+
+ mockListener.deviceAdded(isA(IDevice.class));
+ expectLastCall().once();
+ replay(mockListener);
+
+ // cannot learn device internal ports
+ d = deviceManager.learnDeviceByEntity(entity2);
+ assertNull(d);
+ d = deviceManager.learnDeviceByEntity(entity4);
+ assertNull(d);
+
+ d = deviceManager.learnDeviceByEntity(entity1);
+ assertEquals(1, deviceManager.getAllDevices().size());
+ aps = d.getAttachmentPoints();
+ assertArrayEquals(new SwitchPort[] { new SwitchPort(1L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity1 } , (Device)d);
+ ips = d.getIPv4Addresses();
+ assertArrayEquals(new Integer[] { 1 }, ips);
+ verify(mockListener);
+
+ reset(mockListener);
+ replay(mockListener);
+
+ // don't learn
+ d = deviceManager.learnDeviceByEntity(entity2);
+ assertEquals(1, deviceManager.getAllDevices().size());
+ aps = d.getAttachmentPoints();
+ assertArrayEquals(new SwitchPort[] { new SwitchPort(1L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity1 } , (Device)d);
+ ips = d.getIPv4Addresses();
+ assertArrayEquals(new Integer[] { 1 }, ips);
+ verify(mockListener);
+
+ reset(mockListener);
+ mockListener.deviceMoved(isA(IDevice.class));
+ mockListener.deviceIPV4AddrChanged(isA(IDevice.class));
+ replay(mockListener);
+
+ // learn
+ d = deviceManager.learnDeviceByEntity(entity3);
+ assertEquals(1, deviceManager.getAllDevices().size());
+ aps = d.getAttachmentPoints();
+ assertArrayEquals(new SwitchPort[] { new SwitchPort(3L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity1, entity3 } , (Device)d);
+ ips = d.getIPv4Addresses();
+ Arrays.sort(ips);
+ assertArrayEquals(new Integer[] { 1, 3 }, ips);
+ verify(mockListener);
+
+ reset(mockListener);
+ replay(mockListener);
+
+ // don't learn
+ d = deviceManager.learnDeviceByEntity(entity4);
+ assertEquals(1, deviceManager.getAllDevices().size());
+ aps = d.getAttachmentPoints();
+ assertArrayEquals(new SwitchPort[] { new SwitchPort(3L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity1, entity3 } , (Device)d);
+ ips = d.getIPv4Addresses();
+ Arrays.sort(ips);
+ assertArrayEquals(new Integer[] { 1, 3 }, ips);
+ verify(mockListener);
+ }
+
@Test
public void testAttachmentPointSuppression() throws Exception {
IDeviceListener mockListener =
- createStrictMock(IDeviceListener.class);
+ createMock(IDeviceListener.class);
deviceManager.addListener(mockListener);
@@ -584,15 +704,16 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
andReturn(10L).anyTimes();
expect(mockTopology.getL2DomainId(50L)).
andReturn(10L).anyTimes();
- expect(mockTopology.isBroadcastDomainPort(anyLong(), anyShort())).
- andReturn(false).anyTimes();
+ expect(mockTopology.isBroadcastDomainPort(anyLong(), anyShort()))
+ .andReturn(false).anyTimes();
expect(mockTopology.isInSameBroadcastDomain(anyLong(), anyShort(),
- anyLong(), anyShort())).andReturn(false).anyTimes();
+ anyLong(), anyShort()))
+ .andReturn(false).anyTimes();
- expect(mockTopology.isAttachmentPointPort(anyLong(),
- anyShort())).andReturn(true).anyTimes();
- expect(mockTopology.isConsistent(5L, (short)1, 50L, (short)1)).
- andReturn(false).anyTimes();
+ expect(mockTopology.isAttachmentPointPort(anyLong(), anyShort()))
+ .andReturn(true).anyTimes();
+ expect(mockTopology.isConsistent(5L, (short)1, 50L, (short)1))
+ .andReturn(false).anyTimes();
Date topologyUpdateTime = new Date();
expect(mockTopology.getLastUpdateTime()).andReturn(topologyUpdateTime).
@@ -606,10 +727,10 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
deviceManager.addSuppressAPs(10L, (short)1);
Calendar c = Calendar.getInstance();
- Entity entity1 = new Entity(1L, null, 1, 1L, 1, c.getTime());
Entity entity0 = new Entity(1L, null, null, null, null, c.getTime());
+ Entity entity1 = new Entity(1L, null, 1, 1L, 1, c.getTime());
c.add(Calendar.SECOND, 1);
- Entity entity2 = new Entity(1L, null, null, 5L, 1, c.getTime());
+ Entity entity2 = new Entity(1L, null, 1, 5L, 1, c.getTime());
c.add(Calendar.SECOND, 1);
Entity entity3 = new Entity(1L, null, null, 10L, 1, c.getTime());
c.add(Calendar.SECOND, 1);
@@ -621,39 +742,47 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
mockListener.deviceAdded(isA(IDevice.class));
replay(mockListener);
+
+ // cannot learn device on suppressed AP
+ d = deviceManager.learnDeviceByEntity(entity1);
+ assertNull(d);
- deviceManager.learnDeviceByEntity(entity1);
- d = deviceManager.learnDeviceByEntity(entity0);
+ deviceManager.learnDeviceByEntity(entity0);
+ d = deviceManager.learnDeviceByEntity(entity1);
assertEquals(1, deviceManager.getAllDevices().size());
aps = d.getAttachmentPoints();
assertEquals(aps.length, 0);
+ // entities from suppressed switchPorts will not be learned
+ verifyEntityArray(new Entity[] { entity0} , (Device)d);
ips = d.getIPv4Addresses();
- assertArrayEquals(new Integer[] { 1 }, ips);
+ assertArrayEquals(new Integer[] { }, ips);
verify(mockListener);
reset(mockListener);
mockListener.deviceMoved((isA(IDevice.class)));
+ mockListener.deviceIPV4AddrChanged((isA(IDevice.class)));
replay(mockListener);
d = deviceManager.learnDeviceByEntity(entity2);
assertEquals(1, deviceManager.getAllDevices().size());
aps = d.getAttachmentPoints();
assertArrayEquals(new SwitchPort[] { new SwitchPort(5L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity0, entity2} , (Device)d);
ips = d.getIPv4Addresses();
assertArrayEquals(new Integer[] { 1 }, ips);
verify(mockListener);
reset(mockListener);
- mockListener.deviceMoved((isA(IDevice.class)));
replay(mockListener);
d = deviceManager.learnDeviceByEntity(entity3);
assertEquals(1, deviceManager.getAllDevices().size());
aps = d.getAttachmentPoints();
assertArrayEquals(new SwitchPort[] { new SwitchPort(5L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity0, entity2} , (Device)d);
ips = d.getIPv4Addresses();
assertArrayEquals(new Integer[] { 1 }, ips);
- //verify(mockListener); // There is no device movement here; no not needed.
+ verify(mockListener);
reset(mockListener);
mockListener.deviceMoved((isA(IDevice.class)));
@@ -664,6 +793,7 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
aps = d.getAttachmentPoints();
assertArrayEquals(new SwitchPort[] { new SwitchPort(5L, 1),
new SwitchPort(50L, 1) }, aps);
+ verifyEntityArray(new Entity[] { entity0, entity2, entity4} , (Device)d);
ips = d.getIPv4Addresses();
assertArrayEquals(new Integer[] { 1 }, ips);
verify(mockListener);
--
GitLab