Changes to ARP, Forwarding, and OnosDeviceManager to make reactive forwarding more reliable
diff --git a/src/main/java/net/floodlightcontroller/util/MACAddress.java b/src/main/java/net/floodlightcontroller/util/MACAddress.java
index b143bda..fc144bb 100644
--- a/src/main/java/net/floodlightcontroller/util/MACAddress.java
+++ b/src/main/java/net/floodlightcontroller/util/MACAddress.java
@@ -8,6 +8,7 @@
import org.codehaus.jackson.map.annotate.JsonDeserialize;
import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.openflow.util.HexString;
/**
* The class representing MAC address.
@@ -167,13 +168,6 @@
@Override
public String toString() {
- StringBuilder builder = new StringBuilder();
- for (byte b: address) {
- if (builder.length() > 0) {
- builder.append(":");
- }
- builder.append(String.format("%02X", b & 0xFF));
- }
- return builder.toString();
+ return HexString.toHexString(address);
}
}
diff --git a/src/main/java/net/onrc/onos/ofcontroller/core/internal/DeviceStorageImpl.java b/src/main/java/net/onrc/onos/ofcontroller/core/internal/DeviceStorageImpl.java
index a71b762..ba4a3b5 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/core/internal/DeviceStorageImpl.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/core/internal/DeviceStorageImpl.java
@@ -2,6 +2,7 @@
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Iterator;
import java.util.List;
import net.floodlightcontroller.devicemanager.IDevice;
@@ -325,9 +326,10 @@
*/
@Override
public void addOnosDevice(OnosDevice onosDevice) {
-
String macAddress = HexString.toHexString(onosDevice.getMacAddress().toBytes());
+ log.debug("addOnosDevice: {}", onosDevice);
+
try {
IDeviceObject device = ope.searchDevice(macAddress);
@@ -339,7 +341,6 @@
}
// Check if the device has the IP address, add it if it doesn't
- // TODO multiple IP addresses
if (onosDevice.getIpv4Address() != null) {
boolean hasIpAddress = false;
for (IIpv4Address ipv4Address : device.getIpv4Addresses()) {
@@ -360,8 +361,25 @@
}
// Check if the device has the attachment point, add it if not
+ // TODO single attachment point for now, extend to multiple later
String switchDpid = HexString.toHexString(onosDevice.getSwitchDPID());
boolean hasAttachmentPoint = false;
+ Iterator<IPortObject> it = device.getAttachedPorts().iterator();
+ if (it.hasNext()) {
+ IPortObject existingPort = it.next();
+ if (existingPort != null) {
+ ISwitchObject existingSwitch = existingPort.getSwitch();
+ if (!existingSwitch.getDPID().equals(switchDpid) ||
+ existingPort.getNumber() != onosDevice.getSwitchPort()) {
+ existingPort.removeDevice(device);
+ }
+ else {
+ hasAttachmentPoint = true;
+ }
+ }
+ }
+
+ /*
for (IPortObject portObject : device.getAttachedPorts()) {
ISwitchObject switchObject = portObject.getSwitch();
if (switchObject.getDPID().equals(switchDpid)
@@ -370,10 +388,13 @@
break;
}
}
+ */
if (!hasAttachmentPoint) {
IPortObject portObject = ope.searchPort(switchDpid, onosDevice.getSwitchPort());
- portObject.setDevice(device);
+ if (portObject != null) {
+ portObject.setDevice(device);
+ }
}
ope.commit();
diff --git a/src/main/java/net/onrc/onos/ofcontroller/devicemanager/OnosDeviceManager.java b/src/main/java/net/onrc/onos/ofcontroller/devicemanager/OnosDeviceManager.java
index a776eaa..f75a2b0 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/devicemanager/OnosDeviceManager.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/devicemanager/OnosDeviceManager.java
@@ -35,7 +35,7 @@
private IFloodlightProviderService floodlightProvider;
- public class OnosDeviceUpdate implements IUpdate {
+ private class OnosDeviceUpdate implements IUpdate {
private OnosDevice device;
public OnosDeviceUpdate(OnosDevice device) {
@@ -55,8 +55,9 @@
@Override
public boolean isCallbackOrderingPrereq(OFType type, String name) {
- // TODO Auto-generated method stub
- return false;
+ // We want link discovery to consume LLDP first otherwise we'll
+ // end up reading bad device info from LLDP packets
+ return type == OFType.PACKET_IN && "linkdiscovery".equals(name);
}
@Override
diff --git a/src/main/java/net/onrc/onos/ofcontroller/forwarding/Forwarding.java b/src/main/java/net/onrc/onos/ofcontroller/forwarding/Forwarding.java
index 4b2b087..02972e2 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/forwarding/Forwarding.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/forwarding/Forwarding.java
@@ -249,13 +249,17 @@
destinationMac);
if (deviceObject == null) {
- log.debug("No device entry found for {}", destinationMac);
+ log.debug("No device entry found for {} - broadcasting packet",
+ destinationMac);
+ handleBroadcast(sw, pi, eth);
return;
}
Iterator<IPortObject> ports = deviceObject.getAttachedPorts().iterator();
if (!ports.hasNext()) {
- log.debug("No attachment point found for device {}", destinationMac);
+ log.debug("No attachment point found for device {} - broadcasting packet",
+ destinationMac);
+ handleBroadcast(sw, pi, eth);
return;
}
IPortObject portObject = ports.next();
diff --git a/src/main/java/net/onrc/onos/ofcontroller/proxyarp/ProxyArpManager.java b/src/main/java/net/onrc/onos/ofcontroller/proxyarp/ProxyArpManager.java
index a63d2a8..32b2e9c 100644
--- a/src/main/java/net/onrc/onos/ofcontroller/proxyarp/ProxyArpManager.java
+++ b/src/main/java/net/onrc/onos/ofcontroller/proxyarp/ProxyArpManager.java
@@ -63,7 +63,7 @@
private final long ARP_TIMER_PERIOD = 100; //ms
- private static final int ARP_REQUEST_TIMEOUT = 500; //ms
+ private static final int ARP_REQUEST_TIMEOUT = 2000; //ms
private IFloodlightProviderService floodlightProvider;
private ITopologyService topology;
@@ -77,7 +77,7 @@
private short vlan;
private static final short NO_VLAN = 0;
- private ArpCache arpCache;
+ //private ArpCache arpCache;
private SetMultimap<InetAddress, ArpRequest> arpRequests;
@@ -165,7 +165,7 @@
this.configService = context.getServiceImpl(IConfigInfoService.class);
this.restApi = context.getServiceImpl(IRestApiService.class);
- arpCache = new ArpCache();
+ //arpCache = new ArpCache();
arpRequests = Multimaps.synchronizedSetMultimap(
HashMultimap.<InetAddress, ArpRequest>create());
@@ -207,10 +207,7 @@
//Have to synchronize externally on the Multimap while using an iterator,
//even though it's a synchronizedMultimap
- synchronized (arpRequests) {
- //log.debug("Current have {} outstanding requests",
- //arpRequests.size());
-
+ synchronized (arpRequests) {
Iterator<Map.Entry<InetAddress, ArpRequest>> it
= arpRequests.entries().iterator();
@@ -222,16 +219,15 @@
log.debug("Cleaning expired ARP request for {}",
entry.getKey().getHostAddress());
- //if he ARP Request is expired and then delete the device
- /*
+ // If the ARP request is expired and then delete the device
+ // TODO check whether this is OK from this thread
IDeviceObject targetDevice =
deviceStorage.getDeviceByIP(InetAddresses.coerceToInteger(entry.getKey()));
- if(targetDevice!=null)
- {deviceStorage.removeDevice(targetDevice);
- log.debug("RemoveDevice: {} due to no have not recieve the ARP reply", targetDevice.toString());
+ if (targetDevice != null) {
+ deviceStorage.removeDevice(targetDevice);
+ log.debug("RemoveDevice: {} due to no have not recieve the ARP reply", targetDevice.toString());
}
- */
it.remove();
@@ -289,17 +285,11 @@
if (eth.getEtherType() == Ethernet.TYPE_ARP){
ARP arp = (ARP) eth.getPayload();
if (arp.getOpCode() == ARP.OP_REQUEST) {
- log.debug("receive ARP request");
- //TODO check what the DeviceManager does about propagating
- //or swallowing ARPs. We want to go after DeviceManager in the
- //chain but we really need it to CONTINUE ARP packets so we can
- //get them.
handleArpRequest(sw, pi, arp, eth);
}
else if (arp.getOpCode() == ARP.OP_REPLY) {
- log.debug("receive ARP reply");
- handleArpReply(sw, pi, arp);
- sendToOtherNodesReply(eth, pi);
+ handleArpReply(sw, pi, arp);
+ sendToOtherNodesReply(eth, pi);
}
// Stop ARP packets here
@@ -339,20 +329,30 @@
}
//MACAddress macAddress = arpCache.lookup(target);
-
- IDeviceObject targetDevice =
- deviceStorage.getDeviceByIP(InetAddresses.coerceToInteger(target));
- log.debug("targetDevice: {}", targetDevice);
arpRequests.put(target, new ArpRequest(
new HostArpRequester(arp, sw.getId(), pi.getInPort()), false));
- if (targetDevice != null) {
- // Even the device in our database is not null, we do not reply to the request directly, but to check whether the device is still valid
+ IDeviceObject targetDevice =
+ deviceStorage.getDeviceByIP(InetAddresses.coerceToInteger(target));
+
+ if (targetDevice == null) {
+ if (log.isTraceEnabled()) {
+ log.trace("No device info found for {} - broadcasting",
+ target.getHostAddress());
+ }
+
+ // We don't know the device so broadcast the request out
+ sendToOtherNodes(eth, sw.getId(), pi);
+ }
+ else {
+ // Even if the device exists in our database, we do not reply to
+ // the request directly, but check whether the device is still valid
MACAddress macAddress = MACAddress.valueOf(targetDevice.getMACAddress());
if (log.isTraceEnabled()) {
- log.trace("The target Device Record in DB is: {} => {} from ARP request host at {}/{}", new Object [] {
+ log.trace("The target Device Record in DB is: {} => {} from ARP request host at {}/{}",
+ new Object [] {
inetAddressToString(arp.getTargetProtocolAddress()),
macAddress.toString(),
HexString.toHexString(sw.getId()), pi.getInPort()});
@@ -361,38 +361,38 @@
// sendArpReply(arp, sw.getId(), pi.getInPort(), macAddress);
log.trace("Checking the device info from DB is still valid or not");
- Iterable<IPortObject> outPorts=targetDevice.getAttachedPorts();
+ Iterable<IPortObject> outPorts = targetDevice.getAttachedPorts();
- if(!outPorts.iterator().hasNext()){
- log.debug("outPort : null");
+ if (!outPorts.iterator().hasNext()){
+ if (log.isTraceEnabled()) {
+ log.trace("Device {} exists but is not connected to any ports" +
+ " - broadcasting", macAddress);
+ }
+
sendToOtherNodes(eth, sw.getId(), pi);
- }else{
-
+ }
+ else {
for (IPortObject portObject : outPorts) {
- long outSwitch=0;
- short outPort=0;
-
+ long outSwitch = 0;
+ short outPort = 0;
if (!portObject.getLinkedPorts().iterator().hasNext()) {
- outPort=portObject.getNumber();
- log.debug("outPort:{} ", outPort);
- }
+ outPort = portObject.getNumber();
+ }
ISwitchObject outSwitchObject = portObject.getSwitch();
- outSwitch= HexString.toLong(outSwitchObject.getDPID());
- log.debug("outSwitch.DPID:{}; outPort: {}", outSwitchObject.getDPID(), outPort );
- sendToOtherNodes( eth, pi, outSwitch, outPort);
+ outSwitch = HexString.toLong(outSwitchObject.getDPID());
+
+ if (log.isTraceEnabled()) {
+ log.trace("Probing device {} on port {}/{}",
+ new Object[] {macAddress,
+ HexString.toHexString(outSwitch), outPort});
+ }
+ sendToOtherNodes(eth, pi, outSwitch, outPort);
}
}
-
- }else {
- log.debug("The Device info in DB is {} for IP {}", targetDevice, inetAddressToString(arp.getTargetProtocolAddress()));
-
- // We don't know the device so broadcast the request out
- sendToOtherNodes(eth, sw.getId(), pi);
}
-
}
private void handleArpReply(IOFSwitch sw, OFPacketIn pi, ARP arp){
@@ -534,22 +534,22 @@
datagrid.sendArpRequest(ArpMessage.newRequest(targetAddress, eth.serialize(),
-1L, (short)-1, inSwitchId, pi.getInPort()));
}
+
//hazelcast to other ONOS instances to send the ARP packet out on outPort of outSwitch
private void sendToOtherNodes(Ethernet eth, OFPacketIn pi, long outSwitch, short outPort) {
ARP arp = (ARP) eth.getPayload();
if (log.isTraceEnabled()) {
- log.trace("Sending ARP request for {} to other ONOS instances with outSwitch {} ",
- inetAddressToString(arp.getTargetProtocolAddress()), String.valueOf(outSwitch));
-
+ log.trace("Sending ARP request for {} to other ONOS instances with outSwitch {} ",
+ inetAddressToString(arp.getTargetProtocolAddress()), String.valueOf(outSwitch));
}
InetAddress targetAddress;
try {
- targetAddress = InetAddress.getByAddress(arp.getTargetProtocolAddress());
+ targetAddress = InetAddress.getByAddress(arp.getTargetProtocolAddress());
} catch (UnknownHostException e) {
- log.error("Unknown host", e);
- return;
+ log.error("Unknown host", e);
+ return;
}
datagrid.sendArpRequest(ArpMessage.newRequest(targetAddress, eth.serialize(), outSwitch, outPort));
@@ -557,25 +557,26 @@
}
+
private void sendToOtherNodesReply(Ethernet eth, OFPacketIn pi) {
ARP arp = (ARP) eth.getPayload();
if (log.isTraceEnabled()) {
- log.trace("Sending ARP reply for {} to other ONOS instances",
- inetAddressToString(arp.getSenderProtocolAddress()));
+ log.trace("Sending ARP reply for {} to other ONOS instances",
+ inetAddressToString(arp.getSenderProtocolAddress()));
}
InetAddress targetAddress;
MACAddress mac = new MACAddress(arp.getSenderHardwareAddress());
try {
- targetAddress = InetAddress.getByAddress(arp.getSenderProtocolAddress());
+ targetAddress = InetAddress.getByAddress(arp.getSenderProtocolAddress());
} catch (UnknownHostException e) {
- log.error("Unknown host", e);
- return;
+ log.error("Unknown host", e);
+ return;
}
- datagrid.sendArpRequest(ArpMessage.newReply(targetAddress,mac));
+ datagrid.sendArpRequest(ArpMessage.newReply(targetAddress, mac));
//datagrid.sendArpReply(ArpMessage.newRequest(targetAddress, eth.serialize()));
}
@@ -678,7 +679,9 @@
}
}
- log.debug("Broadcast ARP request to: {}", switchPorts);
+ if (log.isTraceEnabled()) {
+ log.trace("Broadcast ARP request to: {}", switchPorts);
+ }
}
private void sendArpRequestOutPort(byte[] arpRequest, long dpid, short port) {
@@ -793,7 +796,8 @@
@Override
public MACAddress getMacAddress(InetAddress ipAddress) {
- return arpCache.lookup(ipAddress);
+ //return arpCache.lookup(ipAddress);
+ return null;
}
@Override
@@ -809,7 +813,8 @@
@Override
public List<String> getMappings() {
- return arpCache.getMappings();
+ //return arpCache.getMappings();
+ return new ArrayList<String>();
}
/*