홍길동 / onos

Go to a project
Toggle navigation Toggle navigation pinning
Jonathan Hart
31582d1b 1 parent 936c49d1

Cleanup and javadocs for SDN-IP code 

 plus refactor a unit test that started failing

Change-Id: Ib9f0f8eefc2ba7a9798d8f01b537dae18dd2920c
Expand all
Inline Side-by-side
Showing 4 changed files with 83 additions and 78 deletions
package org.onlab.onos.sdnip;
import java.util.List;
import org.onlab.onos.ApplicationId;
import org.onlab.onos.net.ConnectPoint;
import org.onlab.onos.net.flow.DefaultTrafficSelector;
......@@ -8,6 +10,7 @@ import org.onlab.onos.net.flow.TrafficSelector;
import org.onlab.onos.net.flow.TrafficTreatment;
import org.onlab.onos.net.intent.IntentService;
import org.onlab.onos.net.intent.PointToPointIntent;
import org.onlab.onos.sdnip.bgp.BgpConstants;
import org.onlab.onos.sdnip.config.BgpPeer;
import org.onlab.onos.sdnip.config.BgpSpeaker;
import org.onlab.onos.sdnip.config.Interface;
......@@ -20,8 +23,6 @@ import org.onlab.packet.IpPrefix;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.List;
/**
* Manages the connectivity requirements between peers.
*/
......@@ -30,37 +31,44 @@ public class PeerConnectivityManager {
private static final Logger log = LoggerFactory.getLogger(
PeerConnectivityManager.class);
// TODO these shouldn't be defined here
private static final short BGP_PORT = 179;
private static final int IPV4_BIT_LENGTH = 32;
private final SdnIpConfigService configInfoService;
private final SdnIpConfigService configService;
private final InterfaceService interfaceService;
private final IntentService intentService;
private final ApplicationId appId;
/**
* Creates a new PeerConnectivityManager.
*
* @param appId the application ID
* @param configService the SDN-IP config service
* @param interfaceService the interface service
* @param intentService the intent service
*/
public PeerConnectivityManager(ApplicationId appId,
SdnIpConfigService configInfoService,
SdnIpConfigService configService,
InterfaceService interfaceService,
IntentService intentService) {
this.appId = appId;
this.configInfoService = configInfoService;
this.configService = configService;
this.interfaceService = interfaceService;
this.intentService = intentService;
}
/**
* Starts the peer connectivity manager.
*/
public void start() {
// TODO are any of these errors?
if (interfaceService.getInterfaces().isEmpty()) {
log.warn("The interface in configuration file is empty. "
+ "Thus, the SDN-IP application can not be started.");
} else if (configInfoService.getBgpPeers().isEmpty()) {
} else if (configService.getBgpPeers().isEmpty()) {
log.warn("The BGP peer in configuration file is empty."
+ "Thus, the SDN-IP application can not be started.");
} else if (configInfoService.getBgpSpeakers() == null) {
} else if (configService.getBgpSpeakers() == null) {
log.error("The BGP speaker in configuration file is empty. "
+ "Thus, the SDN-IP application can not be started.");
......@@ -79,7 +87,7 @@ public class PeerConnectivityManager {
* for paths from all peers to each BGP speaker.
*/
private void setupBgpPaths() {
for (BgpSpeaker bgpSpeaker : configInfoService.getBgpSpeakers()
for (BgpSpeaker bgpSpeaker : configService.getBgpSpeakers()
.values()) {
log.debug("Start to set up BGP paths for BGP speaker: {}",
bgpSpeaker);
......@@ -88,7 +96,7 @@ public class PeerConnectivityManager {
List<InterfaceAddress> interfaceAddresses =
bgpSpeaker.interfaceAddresses();
for (BgpPeer bgpPeer : configInfoService.getBgpPeers().values()) {
for (BgpPeer bgpPeer : configService.getBgpPeers().values()) {
log.debug("Start to set up BGP paths between BGP speaker: {} "
+ "to BGP peer: {}", bgpSpeaker, bgpPeer);
......@@ -121,16 +129,14 @@ public class PeerConnectivityManager {
// install intent for BGP path from BGPd to BGP peer matching
// destination TCP port 179
// TODO: The usage of PacketMatchBuilder will be improved, then we
// only need to new the PacketMatchBuilder once.
// By then, the code here will be improved accordingly.
TrafficSelector selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPProtocol(IPv4.PROTOCOL_TCP)
.matchIPSrc(IpPrefix.valueOf(bgpdAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPDst(IpPrefix.valueOf(bgpdPeerAddress.toInt(), IPV4_BIT_LENGTH))
.matchTcpDst(BGP_PORT)
.matchIPSrc(IpPrefix.valueOf(bgpdAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchIPDst(IpPrefix.valueOf(bgpdPeerAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchTcpDst((short) BgpConstants.BGP_PORT)
.build();
TrafficTreatment treatment = DefaultTrafficTreatment.builder()
......@@ -149,9 +155,11 @@ public class PeerConnectivityManager {
selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPProtocol(IPv4.PROTOCOL_TCP)
.matchIPSrc(IpPrefix.valueOf(bgpdAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPDst(IpPrefix.valueOf(bgpdPeerAddress.toInt(), IPV4_BIT_LENGTH))
.matchTcpSrc(BGP_PORT)
.matchIPSrc(IpPrefix.valueOf(bgpdAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchIPDst(IpPrefix.valueOf(bgpdPeerAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchTcpSrc((short) BgpConstants.BGP_PORT)
.build();
PointToPointIntent intentMatchSrcTcpPort =
......@@ -167,9 +175,11 @@ public class PeerConnectivityManager {
selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPProtocol(IPv4.PROTOCOL_TCP)
.matchIPSrc(IpPrefix.valueOf(bgpdPeerAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPDst(IpPrefix.valueOf(bgpdAddress.toInt(), IPV4_BIT_LENGTH))
.matchTcpDst(BGP_PORT)
.matchIPSrc(IpPrefix.valueOf(bgpdPeerAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchIPDst(IpPrefix.valueOf(bgpdAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchTcpDst((short) BgpConstants.BGP_PORT)
.build();
PointToPointIntent reversedIntentMatchDstTcpPort =
......@@ -185,9 +195,11 @@ public class PeerConnectivityManager {
selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPProtocol(IPv4.PROTOCOL_TCP)
.matchIPSrc(IpPrefix.valueOf(bgpdPeerAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPDst(IpPrefix.valueOf(bgpdAddress.toInt(), IPV4_BIT_LENGTH))
.matchTcpSrc(BGP_PORT)
.matchIPSrc(IpPrefix.valueOf(bgpdPeerAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchIPDst(IpPrefix.valueOf(bgpdAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchTcpSrc((short) BgpConstants.BGP_PORT)
.build();
PointToPointIntent reversedIntentMatchSrcTcpPort =
......@@ -211,7 +223,7 @@ public class PeerConnectivityManager {
* for paths from all peers to each BGP speaker.
*/
private void setupIcmpPaths() {
for (BgpSpeaker bgpSpeaker : configInfoService.getBgpSpeakers()
for (BgpSpeaker bgpSpeaker : configService.getBgpSpeakers()
.values()) {
log.debug("Start to set up ICMP paths for BGP speaker: {}",
bgpSpeaker);
......@@ -219,7 +231,7 @@ public class PeerConnectivityManager {
List<InterfaceAddress> interfaceAddresses = bgpSpeaker
.interfaceAddresses();
for (BgpPeer bgpPeer : configInfoService.getBgpPeers().values()) {
for (BgpPeer bgpPeer : configService.getBgpPeers().values()) {
Interface peerInterface = interfaceService.getInterface(
bgpPeer.connectPoint());
......@@ -253,8 +265,10 @@ public class PeerConnectivityManager {
TrafficSelector selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPProtocol(IPv4.PROTOCOL_ICMP)
.matchIPSrc(IpPrefix.valueOf(bgpdAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPDst(IpPrefix.valueOf(bgpdPeerAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPSrc(IpPrefix.valueOf(bgpdAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchIPDst(IpPrefix.valueOf(bgpdPeerAddress.toInt(),
IpAddress.MAX_INET_MASK))
.build();
TrafficTreatment treatment = DefaultTrafficTreatment.builder()
......@@ -271,8 +285,10 @@ public class PeerConnectivityManager {
selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPProtocol(IPv4.PROTOCOL_ICMP)
.matchIPSrc(IpPrefix.valueOf(bgpdPeerAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPDst(IpPrefix.valueOf(bgpdAddress.toInt(), IPV4_BIT_LENGTH))
.matchIPSrc(IpPrefix.valueOf(bgpdPeerAddress.toInt(),
IpAddress.MAX_INET_MASK))
.matchIPDst(IpPrefix.valueOf(bgpdAddress.toInt(),
IpAddress.MAX_INET_MASK))
.build();
PointToPointIntent reversedIntent =
......
......@@ -55,8 +55,6 @@ import com.googlecode.concurrenttrees.radixinverted.InvertedRadixTree;
/**
* This class processes BGP route update, translates each update into a intent
* and submits the intent.
* <p/>
* TODO: Make it thread-safe.
*/
public class Router implements RouteListener {
......@@ -69,14 +67,13 @@ public class Router implements RouteListener {
// Stores all incoming route updates in a queue.
private BlockingQueue<RouteUpdate> routeUpdates;
// The Ip4Address is the next hop address of each route update.
// The IpAddress is the next hop address of each route update.
private SetMultimap<IpAddress, RouteEntry> routesWaitingOnArp;
private ConcurrentHashMap<IpPrefix, MultiPointToSinglePointIntent> pushedRouteIntents;
private IntentService intentService;
//private IProxyArpService proxyArp;
private HostService hostService;
private SdnIpConfigService configInfoService;
private SdnIpConfigService configService;
private InterfaceService interfaceService;
private ExecutorService bgpUpdatesExecutor;
......@@ -98,18 +95,19 @@ public class Router implements RouteListener {
/**
* Class constructor.
*
* @param appId the application ID
* @param intentService the intent service
* @param hostService the host service
* @param configInfoService the configuration service
* @param configService the configuration service
* @param interfaceService the interface service
*/
public Router(ApplicationId appId, IntentService intentService,
HostService hostService, SdnIpConfigService configInfoService,
HostService hostService, SdnIpConfigService configService,
InterfaceService interfaceService) {
this.appId = appId;
this.intentService = intentService;
this.hostService = hostService;
this.configInfoService = configInfoService;
this.configService = configService;
this.interfaceService = interfaceService;
bgpRoutes = new ConcurrentInvertedRadixTree<>(
......@@ -172,7 +170,7 @@ public class Router implements RouteListener {
@Override
public void update(RouteUpdate routeUpdate) {
log.debug("Received new route Update: {}", routeUpdate);
log.debug("Received new route update: {}", routeUpdate);
try {
routeUpdates.put(routeUpdate);
......@@ -498,9 +496,11 @@ public class Router implements RouteListener {
private void executeRouteAdd(RouteEntry routeEntry) {
log.debug("Executing route add: {}", routeEntry);
// Monitor the IP address so we'll get notified of updates to the MAC
// address.
hostService.startMonitoringIp(routeEntry.nextHop());
// See if we know the MAC address of the next hop
//MacAddress nextHopMacAddress =
//proxyArp.getMacAddress(routeEntry.getNextHop());
MacAddress nextHopMacAddress = null;
Set<Host> hosts = hostService.getHostsByIp(
routeEntry.nextHop().toPrefix());
......@@ -511,9 +511,6 @@ public class Router implements RouteListener {
if (nextHopMacAddress == null) {
routesWaitingOnArp.put(routeEntry.nextHop(), routeEntry);
//proxyArp.sendArpRequest(routeEntry.getNextHop(), this, true);
// TODO maybe just do this for every prefix anyway
hostService.startMonitoringIp(routeEntry.nextHop());
return;
}
......@@ -536,11 +533,11 @@ public class Router implements RouteListener {
// Find the attachment point (egress interface) of the next hop
Interface egressInterface;
if (configInfoService.getBgpPeers().containsKey(nextHopIpAddress)) {
if (configService.getBgpPeers().containsKey(nextHopIpAddress)) {
// Route to a peer
log.debug("Route to peer {}", nextHopIpAddress);
BgpPeer peer =
configInfoService.getBgpPeers().get(nextHopIpAddress);
configService.getBgpPeers().get(nextHopIpAddress);
egressInterface =
interfaceService.getInterface(peer.connectPoint());
} else {
......@@ -593,17 +590,12 @@ public class Router implements RouteListener {
}
// Match the destination IP prefix at the first hop
//PacketMatchBuilder builder = new PacketMatchBuilder();
//builder.setEtherType(Ethernet.TYPE_IPV4).setDstIpNet(prefix);
//PacketMatch packetMatch = builder.build();
TrafficSelector selector = DefaultTrafficSelector.builder()
.matchEthType(Ethernet.TYPE_IPV4)
.matchIPDst(prefix)
.build();
// Rewrite the destination MAC address
//ModifyDstMacAction modifyDstMacAction =
//new ModifyDstMacAction(nextHopMacAddress);
TrafficTreatment treatment = DefaultTrafficTreatment.builder()
.setEthDst(nextHopMacAddress)
.build();
......@@ -635,10 +627,6 @@ public class Router implements RouteListener {
log.debug("Processing route delete: {}", routeEntry);
IpPrefix prefix = routeEntry.prefix();
// TODO check the change of logic here - remove doesn't check that
// the route entry was what we expected (and we can't do this
// concurrently)
if (bgpRoutes.remove(RouteEntry.createBinaryString(prefix))) {
//
// Only delete flows if an entry was actually removed from the
......@@ -680,17 +668,19 @@ public class Router implements RouteListener {
}
/**
* This method handles the prefixes which are waiting for ARP replies for
* MAC addresses of next hops.
* Signals the Router that the MAC to IP mapping has potentially been
* updated. This has the effect of updating the MAC address for any
* installed prefixes if it has changed, as well as installing any pending
* prefixes that were waiting for MAC resolution.
*
* @param ipAddress next hop router IP address, for which we sent ARP
* request out
* @param macAddress MAC address which is relative to the ipAddress
* @param ipAddress the IP address that an event was received for
* @param macAddress the most recently known MAC address for the IP address
*/
//@Override
// TODO change name
public void arpResponse(IpAddress ipAddress, MacAddress macAddress) {
log.debug("Received ARP response: {} => {}", ipAddress, macAddress);
private void updateMac(IpAddress ipAddress, MacAddress macAddress) {
log.debug("Received updated MAC info: {} => {}", ipAddress, macAddress);
// TODO here we should check whether the next hop for any of our
// installed prefixes has changed, not just prefixes pending installation.
// We synchronize on this to prevent changes to the radix tree
// while we're pushing intents. If the tree changes, the
......@@ -708,8 +698,6 @@ public class Router implements RouteListener {
bgpRoutes.getValueForExactKey(binaryString);
if (foundRouteEntry != null &&
foundRouteEntry.nextHop().equals(routeEntry.nextHop())) {
log.debug("Pushing prefix {} next hop {}",
routeEntry.prefix(), routeEntry.nextHop());
// We only push prefix flows if the prefix is still in the
// radix tree and the next hop is the same as our
// update.
......@@ -717,9 +705,8 @@ public class Router implements RouteListener {
// for the ARP, or the next hop could have changed.
addRouteIntentToNextHop(prefix, ipAddress, macAddress);
} else {
log.debug("Received ARP response, but {}/{} is no longer in"
+ " the radix tree", routeEntry.prefix(),
routeEntry.nextHop());
log.debug("{} has been revoked before the MAC was resolved",
routeEntry);
}
}
}
......@@ -769,7 +756,7 @@ public class Router implements RouteListener {
event.type() == HostEvent.Type.HOST_UPDATED) {
Host host = event.subject();
for (IpPrefix ip : host.ipAddresses()) {
arpResponse(ip.toIpAddress(), host.mac());
updateMac(ip.toIpAddress(), host.mac());
}
}
}
......
......@@ -26,7 +26,7 @@ import org.slf4j.Logger;
@Service
public class SdnIp implements SdnIpService {
private static final String SDN_ID_APP = "org.onlab.onos.sdnip";
private static final String SDN_IP_APP = "org.onlab.onos.sdnip";
private final Logger log = getLogger(getClass());
......@@ -53,8 +53,10 @@ public class SdnIp implements SdnIpService {
InterfaceService interfaceService = new HostToInterfaceAdaptor(hostService);
ApplicationId appId = coreService.registerApplication(SDN_ID_APP);
peerConnectivity = new PeerConnectivityManager(appId, config, interfaceService, intentService);
ApplicationId appId = coreService.registerApplication(SDN_IP_APP);
peerConnectivity = new PeerConnectivityManager(appId, config,
interfaceService, intentService);
peerConnectivity.start();
router = new Router(appId, intentService, hostService, config, interfaceService);
......