Network Graph Refactoring: create a separate NetworkGraphImpl instance
within TopologyManager instead of extending it.
Change-Id: I01126a2004af984b873e46d86fbe521ca1f86ddb
diff --git a/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphImpl.java b/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphImpl.java
index 88cedac..4425a7d 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphImpl.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphImpl.java
@@ -3,6 +3,7 @@
import java.net.InetAddress;
import java.util.Collection;
import java.util.Collections;
+import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Set;
@@ -19,10 +20,10 @@
private static final Logger log = LoggerFactory.getLogger(NetworkGraphImpl.class);
// DPID -> Switch
- protected ConcurrentMap<Long, Switch> switches;
+ private ConcurrentMap<Long, Switch> switches;
- protected ConcurrentMap<InetAddress, Set<Device>> addr2Device;
- protected ConcurrentMap<MACAddress, Device> mac2Device;
+ private ConcurrentMap<InetAddress, Set<Device>> addr2Device;
+ private ConcurrentMap<MACAddress, Device> mac2Device;
public NetworkGraphImpl() {
// TODO: Does these object need to be stored in Concurrent Collection?
@@ -37,6 +38,14 @@
return switches.get(dpid);
}
+ protected void putSwitch(Switch sw) {
+ switches.put(sw.getDpid(), sw);
+ }
+
+ protected void removeSwitch(Long dpid) {
+ switches.remove(dpid);
+ }
+
@Override
public Iterable<Switch> getSwitches() {
// TODO Check if it is safe to directly return this Object.
@@ -106,4 +115,28 @@
public Device getDeviceByMac(MACAddress address) {
return mac2Device.get(address);
}
+
+ protected void putDevice(Device device) {
+ mac2Device.put(device.getMacAddress(), device);
+ for (InetAddress ipAddr : device.getIpAddress()) {
+ Set<Device> devices = addr2Device.get(ipAddr);
+ if (devices == null) {
+ devices = new HashSet<>();
+ addr2Device.put(ipAddr, devices);
+ }
+ devices.add(device);
+ }
+ }
+
+ protected void removeDevice(Device device) {
+ mac2Device.remove(device.getMacAddress());
+ for (InetAddress ipAddr : device.getIpAddress()) {
+ Set<Device> devices = addr2Device.get(ipAddr);
+ if (devices != null) {
+ devices.remove(device);
+ if (devices.isEmpty())
+ addr2Device.remove(ipAddr);
+ }
+ }
+ }
}
diff --git a/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphModule.java b/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphModule.java
index 4b2ef34..6ae4777 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphModule.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/networkgraph/NetworkGraphModule.java
@@ -21,7 +21,7 @@
// This is initialized as a module for now
// private RCNetworkGraphPublisher eventListener;
- private TopologyManager networkGraph;
+ private TopologyManager topologyManager;
//private NetworkGraphDatastore southboundNetworkGraph;
private IDatagridService datagridService;
private IControllerRegistryService registryService;
@@ -64,24 +64,24 @@
registryService = context.getServiceImpl(IControllerRegistryService.class);
networkGraphListeners = new CopyOnWriteArrayList<>();
- networkGraph = new TopologyManager(registryService, networkGraphListeners);
+ topologyManager = new TopologyManager(registryService, networkGraphListeners);
//southboundNetworkGraph = new NetworkGraphDatastore(networkGraph);
}
@Override
public void startUp(FloodlightModuleContext context) {
restApi.addRestletRoutable(new NetworkGraphWebRoutable());
- networkGraph.startup(datagridService);
+ topologyManager.startup(datagridService);
}
@Override
public NetworkGraph getNetworkGraph() {
- return networkGraph;
+ return topologyManager.getNetworkGraph();
}
@Override
public NetworkGraphDiscoveryInterface getNetworkGraphDiscoveryInterface() {
- return networkGraph;
+ return topologyManager;
}
@Override
diff --git a/src/main/java/net/onrc/onos/ofcontroller/networkgraph/TopologyManager.java b/src/main/java/net/onrc/onos/ofcontroller/networkgraph/TopologyManager.java
index d0e215f..4d56fed 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/networkgraph/TopologyManager.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/networkgraph/TopologyManager.java
@@ -39,8 +39,8 @@
* re-ordering. e.g.) Link Add came in, but Switch was not there.
*
*/
-public class TopologyManager extends NetworkGraphImpl implements
- NetworkGraphDiscoveryInterface, NetworkGraphReplicationInterface {
+public class TopologyManager implements NetworkGraphDiscoveryInterface,
+ NetworkGraphReplicationInterface {
private static final Logger log = LoggerFactory
.getLogger(TopologyManager.class);
@@ -50,16 +50,20 @@
private EventHandler eventHandler = new EventHandler();
private final NetworkGraphDatastore datastore;
+ private final NetworkGraphImpl networkGraph = new NetworkGraphImpl();
private final IControllerRegistryService registryService;
private CopyOnWriteArrayList<INetworkGraphListener> networkGraphListeners;
public TopologyManager(IControllerRegistryService registryService, CopyOnWriteArrayList<INetworkGraphListener> networkGraphListeners) {
- super();
datastore = new NetworkGraphDatastore(this);
this.registryService = registryService;
this.networkGraphListeners = networkGraphListeners;
}
+ NetworkGraph getNetworkGraph() {
+ return networkGraph;
+ }
+
/**
* Event handler class.
*/
@@ -368,7 +372,7 @@
}
private void removePortsNotOnEvent(SwitchEvent swEvt) {
- Switch sw = switches.get( swEvt.getDpid() );
+ Switch sw = networkGraph.getSwitch(swEvt.getDpid());
if ( sw != null ) {
Set<Long> port_noOnEvent = new HashSet<>();
for( PortEvent portEvent : swEvt.getPorts()) {
@@ -399,7 +403,7 @@
private boolean prepareForAddPortEvent(PortEvent portEvt) {
// Parent Switch must exist
- if ( getSwitch(portEvt.getDpid()) == null) {
+ if ( networkGraph.getSwitch(portEvt.getDpid()) == null) {
log.warn("Dropping add port event because switch doesn't exist: {}",
portEvt);
return false;
@@ -645,17 +649,11 @@
throw new IllegalArgumentException("Switch cannot be null");
}
- Switch sw = switches.get(swEvt.getDpid());
+ Switch sw = networkGraph.getSwitch(swEvt.getDpid());
if (sw == null) {
- sw = new SwitchImpl(this, swEvt.getDpid());
- Switch existing = switches.putIfAbsent(swEvt.getDpid(), sw);
- if (existing != null) {
- log.warn(
- "Concurrent putSwitch not expected. Continuing updating {}",
- existing);
- sw = existing;
- }
+ sw = new SwitchImpl(networkGraph, swEvt.getDpid());
+ networkGraph.putSwitch(sw);
}
// Update when more attributes are added to Event object
@@ -678,7 +676,7 @@
removePort(portEvt);
}
- Switch sw = switches.get(swEvt.getDpid());
+ Switch sw = networkGraph.getSwitch(swEvt.getDpid());
if (sw == null) {
log.warn("Switch {} already removed, ignoring", swEvt);
@@ -704,19 +702,14 @@
removePortEvent(portEvt);
}
- boolean removed = switches.remove(swEvt.getDpid(), sw);
- if (removed) {
- log.warn(
- "Switch instance was replaced concurrently while removing {}. Something is not right.",
- sw);
- }
+ networkGraph.removeSwitch(swEvt.getDpid());
}
void putPort(PortEvent portEvt) {
if (portEvt == null) {
throw new IllegalArgumentException("Port cannot be null");
}
- Switch sw = switches.get(portEvt.getDpid());
+ Switch sw = networkGraph.getSwitch(portEvt.getDpid());
if (sw == null) {
throw new BrokenInvariantException(String.format(
"Switch with dpid %s did not exist.",
@@ -729,7 +722,7 @@
}
if (port == null) {
- port = new PortImpl(this, sw, portEvt.getNumber());
+ port = new PortImpl(networkGraph, sw, portEvt.getNumber());
}
// TODO update attributes
@@ -743,7 +736,7 @@
throw new IllegalArgumentException("Port cannot be null");
}
- Switch sw = switches.get(portEvt.getDpid());
+ Switch sw = networkGraph.getSwitch(portEvt.getDpid());
if (sw == null) {
log.warn("Parent Switch for Port {} already removed, ignoring", portEvt);
return;
@@ -824,7 +817,7 @@
}
if (link == null) {
- link = new LinkImpl(this, srcPort, dstPort);
+ link = new LinkImpl(networkGraph, srcPort, dstPort);
}
@@ -899,19 +892,19 @@
throw new IllegalArgumentException("Device cannot be null");
}
- Device device = getDeviceByMac(deviceEvt.getMac());
+ Device device = networkGraph.getDeviceByMac(deviceEvt.getMac());
if ( device == null ) {
- device = new DeviceImpl(this, deviceEvt.getMac());
- Device existing = mac2Device.putIfAbsent(deviceEvt.getMac(), device);
- if (existing != null) {
- log.warn(
- "Concurrent putDevice seems to be in action. Continuing updating {}",
- existing);
- device = existing;
- }
+ device = new DeviceImpl(networkGraph, deviceEvt.getMac());
}
DeviceImpl memDevice = getDeviceImpl(device);
+ // for each IP address
+ for( InetAddress ipAddr : deviceEvt.getIpAddresses() ) {
+ memDevice.addIpAddress(ipAddr);
+ }
+
+ networkGraph.putDevice(device);
+
// for each attachment point
for (SwitchPort swp : deviceEvt.getAttachmentPoints() ) {
// Attached Ports must exist
@@ -931,33 +924,6 @@
memPort.addDevice(device);
memDevice.addAttachmentPoint(port);
}
-
- // for each IP address
- for( InetAddress ipAddr : deviceEvt.getIpAddresses() ) {
- // Add Device -> IP
- memDevice.addIpAddress(ipAddr);
-
- // Add IP -> Set<Device>
- boolean updated = false;
- do {
- Set<Device> devices = this.addr2Device.get(ipAddr);
- if ( devices == null ) {
- devices = new HashSet<>();
- Set<Device> existing = this.addr2Device.putIfAbsent(ipAddr, devices);
- if ( existing == null ) {
- // success
- updated = true;
- }
- } else {
- Set<Device> updateDevices = new HashSet<>(devices);
- updateDevices.add(device);
- updated = this.addr2Device.replace(ipAddr, devices, updateDevices);
- }
- if (!updated) {
- log.debug("Collision detected, updating IP to Device mapping retrying.");
- }
- } while( !updated );
- }
}
void removeDevice(DeviceEvent deviceEvt) {
@@ -965,7 +931,7 @@
throw new IllegalArgumentException("Device cannot be null");
}
- Device device = getDeviceByMac(deviceEvt.getMac());
+ Device device = networkGraph.getDeviceByMac(deviceEvt.getMac());
if ( device == null ) {
log.warn("Device {} already removed, ignoring", deviceEvt);
return;
@@ -986,29 +952,7 @@
memPort.removeDevice(device);
memDevice.removeAttachmentPoint(port);
}
-
- // for each IP address
- for( InetAddress ipAddr : deviceEvt.getIpAddresses() ) {
- // Remove Device -> IP
- memDevice.removeIpAddress(ipAddr);
-
- // Remove IP -> Set<Device>
- boolean updated = false;
- do {
- Set<Device> devices = this.addr2Device.get(ipAddr);
- if ( devices == null ) {
- // already empty set, nothing to do
- updated = true;
- } else {
- Set<Device> updateDevices = new HashSet<>(devices);
- updateDevices.remove(device);
- updated = this.addr2Device.replace(ipAddr, devices, updateDevices);
- }
- if (!updated) {
- log.debug("Collision detected, updating IP to Device mapping retrying.");
- }
- } while( !updated );
- }
+ networkGraph.removeDevice(device);
}
private void dispatchPutSwitchEvent(SwitchEvent switchEvent) {
@@ -1069,7 +1013,7 @@
// we might want to include this in NetworkGraph interface
private Port getPort(Long dpid, Long number) {
- Switch sw = getSwitch(dpid);
+ Switch sw = networkGraph.getSwitch(dpid);
if (sw != null) {
return sw.getPort(number);
}
diff --git a/src/test/java/net/onrc/onos/intent/MockNetworkGraph.java b/src/test/java/net/onrc/onos/intent/MockNetworkGraph.java
index cde98f0..ce857de 100644
--- a/src/test/java/net/onrc/onos/intent/MockNetworkGraph.java
+++ b/src/test/java/net/onrc/onos/intent/MockNetworkGraph.java
@@ -9,7 +9,7 @@
public class MockNetworkGraph extends NetworkGraphImpl {
public Switch addSwitch(Long switchId) {
SwitchImpl sw = new SwitchImpl(this, switchId);
- switches.put(sw.getDpid(), sw);
+ this.putSwitch(sw);
return sw;
}