From a2f4842e4a89095348f4110ace27c05eaa61f1db Mon Sep 17 00:00:00 2001
From: Kanzhe Jiang <kanzhe.jiang@bigswitch.com>
Date: Sun, 29 Apr 2012 10:06:22 -0700
Subject: [PATCH] Revert "Update device AP when necessary"
This reverts commit 0ae432f42f47f3d72fcc3034a97c335d1326b5e0.
Conflicts:
src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
Conflicts:
src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
---
.../internal/DeviceManagerImpl.java | 246 +++++++++---------
.../internal/DeviceManagerImplTest.java | 16 +-
2 files changed, 130 insertions(+), 132 deletions(-)
diff --git a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
index 5e9053102..7f58f5b81 100755
--- a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
+++ b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java
@@ -304,22 +304,8 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
/**
* Overall update, delete and clear methods for the set of maps
* @param d the device to update
- * @return true if successfully update the device
*/
- private synchronized boolean updateMaps(Device d, Date lastTopoChangeTime) {
- /**
- * If the device was last seen before the most recent topology change,
- * the device might be outdated.
- * Throw it away.
- */
- if (lastTopoChangeTime.after(d.getLastSeen())) {
- if (log.isDebugEnabled()) {
- log.debug("Skip device update {}, which is seen before last topo change, {}",
- d, lastTopoChangeTime);
- }
- return false;
- }
-
+ private synchronized void updateMaps(Device d) {
// Update dataLayerAddressDeviceMap
updateDataLayerAddressDeviceMap(d);
// Update ipv4AddressDeviceMap
@@ -328,7 +314,6 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
updateSwitchDeviceMap(d);
// Update switchPortDeviceMap
updateSwitchPortDeviceMap(d);
- return true;
}
/**
@@ -437,7 +422,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
if (na == null) { // this particular address is not in the device
na = new DeviceNetworkAddress(nwAddr, lastSeen);
dCopy.addNetworkAddress(na);
- updateMaps(dCopy, lastTopoChangeTime);
+ updateMaps(dCopy);
}
}
@@ -456,8 +441,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
if (na != null) {
delFromIpv4AddressDeviceMap(nwAddr, d);
dCopy.removeNetworkAddress(na);
- // Always update§
- updateMaps(dCopy, new Date(0));
+ updateMaps(dCopy);
removeNetworkAddressFromStorage(d.getDlAddrString(), na);
}
}
@@ -505,8 +489,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
dCopy.addAttachmentPoint(dap);
// Now add this updated device to the maps, which will replace
// the old copy
- updateMaps(dCopy, lastTopoChangeTime);
- writeAttachmentPointToStorage(dCopy, dap, lastSeen);
+ updateMaps(dCopy);
return true;
}
@@ -550,9 +533,9 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
delFromSwitchPortDeviceMap(swPort, d);
// Remove the original device from the Switch map
delFromSwitchDeviceMap(sw, d);
- // Now always add this updated device to the maps, which will replace
+ // Now add this updated device to the maps, which will replace
// the old copy
- updateMaps(dCopy, new Date(0));
+ updateMaps(dCopy);
if (log.isDebugEnabled()) {
log.debug("Remove AP {} post {} prev {} for Device {}",
new Object[] {dap, dCopy.getAttachmentPoints().size(),
@@ -632,7 +615,6 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
} // End of DevMgrMap class definition
private DevMgrMaps devMgrMaps;
- private Date lastTopoChangeTime;
public DevMgrMaps getDevMgrMaps() {
return devMgrMaps;
@@ -717,7 +699,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
@Override
public String toString() {
- String updateString = device.getDlAddrString();
+ String updateString = null;
switch (updateType) {
case ADDRESS_ADDED:
@@ -805,11 +787,8 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
evHistAttachmtPt(mac, swPort, EvAction.ADDED, "New device");
device.addNetworkAddress(nwAddr, currentDate);
device.setVlanId(vlan);
- // Update the maps - insert the device in the maps only if
- // it was learned after the most recent topology change
- if (!devMgrMaps.updateMaps(device, lastTopoChangeTime)) {
- return;
- }
+ // Update the maps - insert the device in the maps
+ devMgrMaps.updateMaps(device);
writeDeviceToStorage(device, currentDate);
updateStatus(device, true);
}
@@ -918,17 +897,17 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
Short vlan = match.getDataLayerVirtualLan();
if (vlan < 0) vlan = null;
int nwSrc = getSrcNwAddr(eth, dlAddr);
- Device srcDevice = devMgrMaps.getDeviceByDataLayerAddr(dlAddr);
+ Device device = devMgrMaps.getDeviceByDataLayerAddr(dlAddr);
if (log.isTraceEnabled()) {
long dstAddr = Ethernet.toLong(match.getDataLayerDestination());
- Device dstDevice = devMgrMaps.getDeviceByDataLayerAddr(dstAddr);
- if (srcDevice != null)
- log.trace(" Src.AttachmentPts: {}", srcDevice.getAttachmentPointsMap().keySet());
- if (dstDevice != null)
- log.trace(" Dst.AttachmentPts: {}", dstDevice.getAttachmentPointsMap().keySet());
+ Device dstDev = devMgrMaps.getDeviceByDataLayerAddr(dstAddr);
+ if (device != null)
+ log.trace(" Src.AttachmentPts: {}", device.getAttachmentPointsMap().keySet());
+ if (dstDev != null)
+ log.trace(" Dst.AttachmentPts: {}", dstDev.getAttachmentPointsMap().keySet());
}
Date currentDate = new Date();
- if (srcDevice != null) {
+ if (device != null) {
// Write lock is expensive, check if we have an update first
boolean newAttachmentPoint = false;
boolean newNetworkAddress = false;
@@ -943,14 +922,14 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
DeviceNetworkAddress networkAddress = null;
// Copy-replace of device would be too expensive here
- srcDevice.setLastSeen(currentDate);
- updateDevice = srcDevice.shouldWriteLastSeenToStorage();
+ device.setLastSeen(currentDate);
+ updateDevice = device.shouldWriteLastSeenToStorage();
if (isGratArp(eth)) {
clearAttachmentPoints = true;
}
- attachmentPoint = srcDevice.getAttachmentPoint(switchPort);
+ attachmentPoint = device.getAttachmentPoint(switchPort);
if (attachmentPoint != null) {
updateAttachmentPointLastSeen = true;
} else {
@@ -966,7 +945,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
}
if (nwSrc != 0) {
- networkAddress = srcDevice.getNetworkAddress(nwSrc);
+ networkAddress = device.getNetworkAddress(nwSrc);
if (networkAddress != null) {
updateNetworkAddressLastSeen = true;
} else if (eth != null && (eth.getPayload() instanceof ARP)) {
@@ -994,7 +973,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
Device deviceByNwaddr = this.getDeviceByIPv4Address(nwSrc);
if ((deviceByNwaddr != null) &&
(deviceByNwaddr.getDataLayerAddressAsLong() !=
- srcDevice.getDataLayerAddressAsLong())) {
+ device.getDataLayerAddressAsLong())) {
updateNeworkAddressMap = true;
Device dCopy = new Device(deviceByNwaddr);
DeviceNetworkAddress naOld = dCopy.getNetworkAddress(nwSrc);
@@ -1002,20 +981,20 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
dCopy.getNetworkAddressesMap();
if (namap.containsKey(nwSrc)) namap.remove(nwSrc);
dCopy.setNetworkAddresses(namap.values());
- this.devMgrMaps.updateMaps(dCopy, new Date(0));
+ this.devMgrMaps.updateMaps(dCopy);
if (naOld !=null)
removeNetworkAddressFromStorage(dCopy.getDlAddrString(), naOld);
}
}
- if ((vlan == null && srcDevice.getVlanId() != null) ||
- (vlan != null && !vlan.equals(srcDevice.getVlanId()))) {
+ if ((vlan == null && device.getVlanId() != null) ||
+ (vlan != null && !vlan.equals(device.getVlanId()))) {
updateDeviceVlan = true;
}
if (newAttachmentPoint || newNetworkAddress || updateDeviceVlan ||
updateNeworkAddressMap) {
- Device nd = new Device(srcDevice);
+ Device nd = new Device(device);
try {
// Check if we have seen this attachmentPoint recently,
@@ -1027,10 +1006,10 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
} else {
if (log.isDebugEnabled()) {
log.debug("Learned new AP for device {} at {}",
- nd, attachmentPoint);
+ HexString.toHexString(nd.getDataLayerAddressAsLong()),
+ attachmentPoint);
}
nd.addAttachmentPoint(attachmentPoint);
- updateDevice = true;
evHistAttachmtPt(nd.getDataLayerAddressAsLong(),
attachmentPoint.getSwitchPort(),
EvAction.ADDED,
@@ -1039,11 +1018,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
}
if (clearAttachmentPoints) {
- if (log.isDebugEnabled()) {
- log.debug("Grat ARP, clear all APs for device {}", nd);
- }
nd.clearAttachmentPoints();
- updateDevice = true;
evHistAttachmtPt(nd, 0L, (short)(-1),
EvAction.CLEARED, "Grat. ARP from pkt-in");
}
@@ -1051,7 +1026,6 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
if (newNetworkAddress) {
// add the address
nd.addNetworkAddress(networkAddress);
- updateDevice = true;
if (log.isTraceEnabled()) {
log.trace("Device {} added IP {}",
new Object[] {nd, IPv4.fromIPv4Address(nwSrc)});
@@ -1060,8 +1034,9 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
if (updateDeviceVlan) {
nd.setVlanId(vlan);
- updateDevice = true;
+ writeDeviceToStorage(nd, currentDate);
}
+
} catch (APBlockedException e) {
assert(attachmentPoint == null);
ret = Command.STOP;
@@ -1071,13 +1046,8 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
ForwardingBase.blockHost(floodlightProvider, switchPort,
dlAddr, ForwardingBase.FLOWMOD_DEFAULT_HARD_TIMEOUT);
}
-
// Update the maps
- if (!devMgrMaps.updateMaps(nd, lastTopoChangeTime)) {
- log.debug("Failed to update devMgrMap for device {}, lastSeen is before topoChange {}",
- nd, lastTopoChangeTime);
- return ret;
- }
+ devMgrMaps.updateMaps(nd);
// publish the update after devMgrMaps is updated.
if (newNetworkAddress) {
updateAddress(nd, networkAddress, true);
@@ -1085,28 +1055,29 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
if (updateDeviceVlan) {
updateVlan(nd);
}
- srcDevice = nd;
+ device = nd;
}
- if (updateDevice) {
- writeDeviceToStorage(srcDevice, currentDate);
- } else {
- if (updateAttachmentPointLastSeen) {
- attachmentPoint.setLastSeen(currentDate);
- if (attachmentPoint.shouldWriteLastSeenToStorage())
- writeAttachmentPointToStorage(
- srcDevice, attachmentPoint, currentDate);
- }
+ if (updateAttachmentPointLastSeen) {
+ attachmentPoint.setLastSeen(currentDate);
+ if (attachmentPoint.shouldWriteLastSeenToStorage())
+ writeAttachmentPointToStorage(
+ device, attachmentPoint, currentDate);
+ }
- if (updateNetworkAddressLastSeen || newNetworkAddress) {
- if (updateNetworkAddressLastSeen)
- networkAddress.setLastSeen(currentDate);
- if (newNetworkAddress ||
- networkAddress.shouldWriteLastSeenToStorage())
- writeNetworkAddressToStorage(
- srcDevice, networkAddress, currentDate);
- }
+ if (updateNetworkAddressLastSeen || newNetworkAddress) {
+ if (updateNetworkAddressLastSeen)
+ networkAddress.setLastSeen(currentDate);
+ if (newNetworkAddress ||
+ networkAddress.shouldWriteLastSeenToStorage())
+ writeNetworkAddressToStorage(
+ device, networkAddress, currentDate);
+ }
+
+ if (updateDevice) {
+ writeDeviceToStorage(device, currentDate);
}
+
} else { // device is null
if (log.isTraceEnabled())
log.trace(" new device");
@@ -1665,6 +1636,8 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
// The long here is the SwitchCluster ID
Map<Long, DeviceAttachmentPoint> tempAPMap =
new HashMap<Long, DeviceAttachmentPoint>();
+ Map<Long, DeviceAttachmentPoint> tempOldAPMap =
+ new HashMap<Long, DeviceAttachmentPoint>();
// Get only the latest DAPs into a map
for (DeviceAttachmentPoint dap : d.getAttachmentPoints()) {
@@ -1686,32 +1659,43 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
// They are removed after deleting from storage.
if (isNewer(dap, existingDap)) {
tempAPMap.put(clusterId, dap);
+ tempOldAPMap.put(clusterId, existingDap);
+ } else {
+ tempOldAPMap.put(clusterId, dap);
}
} else {
tempAPMap.put(clusterId, dap);
}
}
}
- /**
- * Also make sure the attachmentPoints are in non-blocked state
- */
- d.setAttachmentPoints(tempAPMap.values());
- for (DeviceAttachmentPoint dap: tempAPMap.values()) {
- boolean isblocked = dap.isBlocked();
- dap.resetConflictState();
- // only write to DB if the blocking state is reset.
- if (isblocked) {
- writeAttachmentPointToStorage(d, dap, dap.getLastSeen());
+
+ synchronized (d) {
+ /**
+ * Since the update below is happening on a copy of the device it
+ * should not impact packetIn processing time due to lock contention
+ *
+ * Also make sure the attachmentPoints are in non-blocked state
+ */
+ for (DeviceAttachmentPoint dap: tempAPMap.values()) {
+ if (log.isDebugEnabled()) {
+ log.debug("Reset AP {} for device {}", dap, d);
+ }
+ //dap.setBlocked(false);
+ dap.resetConflictState();
+ }
+ d.setAttachmentPoints(tempAPMap.values());
+ for (DeviceAttachmentPoint dap : tempOldAPMap.values()) {
+ if (log.isDebugEnabled()) {
+ log.debug("Reset Old AP {} for device {}", dap, d);
+ }
+ //dap.setBlocked(false);
+ dap.resetConflictState();
+ d.addOldAttachmentPoint(dap);
+ }
+
+ if (log.isDebugEnabled()) {
+ log.debug("After cleanup, device {}", d);
}
- }
- for (DeviceAttachmentPoint dap : d.getOldAttachmentPoints()) {
- // Delete from memory after storage,
- // otherwise an exception will
- // leave stale attachment points on storage.
- removeAttachmentPointFromStorage(d.getDlAddrString(),
- HexString.toHexString(dap.getSwitchPort().getSw().getId()),
- dap.getSwitchPort().getPort().toString());
- d.removeOldAttachmentPoint(dap);
}
}
@@ -1901,7 +1885,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
continue;
}
}
- devMgrMaps.updateMaps(d, lastTopoChangeTime);
+ devMgrMaps.updateMaps(d);
}
}
@@ -2058,7 +2042,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
Date agedBoundary = ageBoundaryDifference(currentDate, expire);
if (ap.getLastSeen().before(agedBoundary)) {
if (log.isDebugEnabled()) {
- log.debug("Remove aged AP {} from device {}", ap, HexString.toHexString(dlAddr));
+ log.debug("Remove aged AP {} from device {}", ap, device);
}
devMgrMaps.delDevAttachmentPoint(dlAddr, ap.getSwitchPort());
evHistAttachmtPt(device.getDataLayerAddressAsLong(),
@@ -2073,7 +2057,7 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
/**
* Age device entry based on an expiry associated with the device.
*/
- protected void removeAgedDevices(Date currentDate) {
+ private void removeAgedDevices(Date currentDate) {
Date deviceAgeBoundary = ageBoundaryDifference(currentDate,
DEVICE_MAX_AGE);
@@ -2139,28 +2123,45 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
readPortChannelConfigFromStorage();
}
- try {
- // serialize the devMgrMaps updates
- synchronized (devMgrMaps) {
- Collection<Device> devices = devMgrMaps.getDevices();
- for (Device d : devices) {
- try {
- Device dCopy = new Device(d);
- cleanupAttachmentPoints(dCopy);
-
- // Update the maps with the new device copy
- devMgrMaps.updateMaps(dCopy, new Date(0));
- } catch (ConcurrentModificationException e) {
- log.error("DeviceUpdateWorker ConcurrentModificationException, ", e);
- } catch (NullPointerException e) {
- log.error("DeviceUpdateWorker NPE, ", e);
- }
- }
- lastTopoChangeTime = new Date();
- if (log.isDebugEnabled()) {
- log.debug("Set lastTopoChangeTime {}", lastTopoChangeTime);
- }
- }
+ try {
+ log.debug("DeviceUpdateWorker: cleaning up attachment points.");
+ for (IOFSwitch sw : devMgrMaps.getSwitches()) {
+ // If the set of devices connected to the switches we
+ // re-iterate a max of 3 times - WHY? TODO
+ int maxIter = 3;
+ while (maxIter > 0) {
+ try {
+ for (Device d: devMgrMaps.getDevicesOnASwitch(sw)) {
+ Device dCopy = new Device(d);
+ cleanupAttachmentPoints(dCopy);
+ for (DeviceAttachmentPoint dap :
+ dCopy.getOldAttachmentPoints()) {
+ // Don't remove conflict attachment points
+ // with recent activities
+ if (dap.isInConflict() && !updatePortChannel)
+ continue;
+ // Delete from memory after storage,
+ // otherwise an exception will
+ // leave stale attachment points on storage.
+ log.debug("Remove AP {} from storage for device {}", dap, dCopy.getDlAddrString());
+ removeAttachmentPointFromStorage(dCopy.getDlAddrString(),
+ HexString.toHexString(dap.getSwitchPort().getSw().getId()),
+ dap.getSwitchPort().getPort().toString());
+ dCopy.removeOldAttachmentPoint(dap);
+ }
+ // Update the maps with the new device copy
+ devMgrMaps.updateMaps(dCopy);
+ }
+ break;
+ } catch (ConcurrentModificationException e) {
+ maxIter--;
+ } catch (NullPointerException e) { }
+ }
+ if (maxIter == 0) {
+ log.warn("Device attachment point clean up " +
+ "attempted three times and failed.");
+ }
+ }
} catch (StorageException e) {
log.error("DeviceUpdateWorker had a storage exception, " +
"Floodlight exiting");
@@ -2297,7 +2298,6 @@ public class DeviceManagerImpl implements IDeviceManagerService, IOFMessageListe
this.updates = new LinkedList<Update>();
this.devMgrMaps = new DevMgrMaps();
this.lock = new ReentrantReadWriteLock();
- this.lastTopoChangeTime = new Date(0);
this.evHistDevMgrAttachPt =
new EventHistory<EventHistoryAttachmentPoint>("Attachment-Point");
diff --git a/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java b/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
index 44a87c0d5..8d241604e 100644
--- a/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
+++ b/src/test/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImplTest.java
@@ -297,10 +297,11 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
expect(mockTopology.isInternal(1L, (short)1)).andReturn(false);
deviceManager.setTopology(mockTopology);
- // reduce the aging period to a few milliseconds
+ // reduce the aging period to a few seconds
+ DeviceManagerImpl.DEVICE_AGING_TIMER = 2;
DeviceManagerImpl.DEVICE_AP_MAX_AGE = 1;
DeviceManagerImpl.DEVICE_NA_MAX_AGE = 1;
- DeviceManagerImpl.DEVICE_MAX_AGE = 2;
+ DeviceManagerImpl.DEVICE_MAX_AGE = 3;
Date currentDate = new Date();
@@ -334,11 +335,10 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
assertEquals(2, rdevice.getNetworkAddresses().size());
// Sleep to make sure the aging thread has run
- Thread.sleep(Math.max(DeviceManagerImpl.DEVICE_NA_MAX_AGE, DeviceManagerImpl.DEVICE_AP_MAX_AGE)*1000);
- deviceManager.removeAgedDevices(new Date());
+ Thread.sleep((DeviceManagerImpl.DEVICE_AGING_TIMER + DeviceManagerImpl.DEVICE_AGING_TIMER_INTERVAL)*1000);
rdevice = deviceManager.getDeviceByDataLayerAddress(dataLayerSource);
-
+ assertEquals(0, rdevice.getAttachmentPoints().size());
assertEquals(0, rdevice.getNetworkAddresses().size());
// Make sure the device's AP and NA were removed from storage
@@ -347,10 +347,8 @@ public class DeviceManagerImplTest extends FloodlightTestCase {
assertEquals(0, rdevice.getAttachmentPoints().size());
assertEquals(0, rdevice.getNetworkAddresses().size());
- // Sleep a bit longer seconds to allow device age
- Thread.sleep((DeviceManagerImpl.DEVICE_MAX_AGE -
- Math.max(DeviceManagerImpl.DEVICE_NA_MAX_AGE, DeviceManagerImpl.DEVICE_AP_MAX_AGE))*1000);
- deviceManager.removeAgedDevices(new Date());
+ // Sleep 4 more seconds to allow device aging thread to run
+ Thread.sleep(DeviceManagerImpl.DEVICE_MAX_AGE*1000);
assertNull(deviceManager.getDeviceByDataLayerAddress(dataLayerSource));
--
GitLab