From: Giovanni Meo Date: Thu, 22 Aug 2013 20:50:57 +0000 (+0200) Subject: On ARPHandler restart Threads are left dangling, very visible in the IT tests X-Git-Tag: releasepom-0.1.0~161^2 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=de67f4a01a2c0ee5a2ac5af51202277765617b2a On ARPHandler restart Threads are left dangling, very visible in the IT tests - Also added debug logs to trace how the ARP Handler is doing event syncs Change-Id: I183c1eeb69e737eedd4d7b785cf974e8abd19b75 Signed-off-by: Giovanni Meo --- diff --git a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPEvent.java b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPEvent.java index fb92e8df85..f676a79d49 100644 --- a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPEvent.java +++ b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPEvent.java @@ -56,4 +56,21 @@ public abstract class ARPEvent implements Serializable{ public InetAddress getTargetIP() { return tIP; } + + /* + * (non-Javadoc) + * + * @see java.lang.Object#toString() + */ + @Override + public String toString() { + StringBuilder builder = new StringBuilder(); + builder.append("ARPEvent ["); + if (tIP != null) { + builder.append("tIP=") + .append(tIP); + } + builder.append("]"); + return builder.toString(); + } } diff --git a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPReply.java b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPReply.java index 4ca3e42c7c..e4388c598f 100644 --- a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPReply.java +++ b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPReply.java @@ -13,6 +13,7 @@ import java.net.InetAddress; import java.util.Arrays; import org.opendaylight.controller.sal.core.NodeConnector; +import org.opendaylight.controller.sal.utils.HexEncode; /* * ARP Reply event wrapper */ @@ -92,4 +93,36 @@ public class ARPReply extends ARPEvent { public NodeConnector getPort() { return port; } + + /* + * (non-Javadoc) + * + * @see java.lang.Object#toString() + */ + @Override + public String toString() { + StringBuilder builder = new StringBuilder(); + builder.append("ARPReply ["); + if (port != null) { + builder.append("port=") + .append(port) + .append(", "); + } + if (tMac != null) { + builder.append("tMac=") + .append(HexEncode.bytesToHexString(tMac)) + .append(", "); + } + if (sMac != null) { + builder.append("sMac=") + .append(HexEncode.bytesToHexString(sMac)) + .append(", "); + } + if (sIP != null) { + builder.append("sIP=") + .append(sIP); + } + builder.append("]"); + return builder.toString(); + } } diff --git a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPRequest.java b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPRequest.java index 7f88a25e31..39cd4f7131 100644 --- a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPRequest.java +++ b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/ARPRequest.java @@ -82,4 +82,26 @@ public class ARPRequest extends ARPEvent { public HostNodeConnector getHost() { return host; } + + /* + * (non-Javadoc) + * + * @see java.lang.Object#toString() + */ + @Override + public String toString() { + StringBuilder builder = new StringBuilder(); + builder.append("ARPRequest ["); + if (subnet != null) { + builder.append("subnet=") + .append(subnet) + .append(", "); + } + if (host != null) { + builder.append("host=") + .append(host); + } + builder.append("]"); + return builder.toString(); + } } diff --git a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/internal/ArpHandler.java b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/internal/ArpHandler.java index f3b22c75d8..627ab26739 100644 --- a/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/internal/ArpHandler.java +++ b/opendaylight/arphandler/src/main/java/org/opendaylight/controller/arphandler/internal/ArpHandler.java @@ -76,7 +76,8 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA private ConcurrentMap countDownTimers; private Timer periodicTimer; private BlockingQueue ARPCacheEvents = new LinkedBlockingQueue(); - Thread cacheEventHandler; + private Thread cacheEventHandler; + private boolean stopping = false; /* * A cluster allocated cache. Used for synchronizing ARP request/reply * events across all cluster controllers. To raise an event, we put() a specific @@ -224,7 +225,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA try { requestor = new HostNodeConnector(sourceMAC, sourceIP, p, subnet.getVlan()); } catch (ConstructionException e) { - log.debug("Received ARP packet with invalid MAC: {}", sourceMAC); + log.debug("Received ARP packet with invalid MAC: {}", HexEncode.bytesToHexString(sourceMAC)); return; } /* @@ -268,7 +269,8 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA && (NetUtils.isBroadcastMACAddr(targetMAC) || Arrays.equals(targetMAC, getControllerMAC()))) { if (connectionManager.isLocal(p.getNode())){ if (log.isTraceEnabled()){ - log.trace("Received local ARP req. for default gateway. Replying with controller MAC: {}", getControllerMAC()); + log.trace("Received local ARP req. for default gateway. Replying with controller MAC: {}", + HexEncode.bytesToHexString(getControllerMAC())); } sendARPReply(p, getControllerMAC(), targetIP, pkt.getSenderHardwareAddress(), sourceIP); } else { @@ -297,7 +299,6 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA //Raise a bcast request event, all controllers need to send one log.trace("Sending a bcast ARP request for {}", targetIP); arpRequestReplyEvent.put(new ARPRequest(targetIP, subnet), false); - } else { /* * Target host known (across the cluster), send ARP REPLY make sure that targetMAC @@ -337,6 +338,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA * the targetIP as the target Network Address */ protected void sendBcastARPRequest(InetAddress targetIP, Subnet subnet) { + log.trace("sendBcatARPRequest targetIP:{} subnet:{}", targetIP, subnet); Set nodeConnectors; if (subnet.isFlatLayer2()) { nodeConnectors = new HashSet(); @@ -348,11 +350,11 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA } for (NodeConnector p : nodeConnectors) { - //fiter out any non-local or internal ports if (! connectionManager.isLocal(p.getNode()) || topologyManager.isInternal(p)) { continue; } + log.trace("Sending toward nodeConnector:{}", p); ARP arp = new ARP(); byte[] senderIP = subnet.getNetworkAddress().getAddress(); byte[] targetIPByte = targetIP.getAddress(); @@ -393,7 +395,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA * The sender MAC is the controller's MAC */ protected void sendUcastARPRequest(HostNodeConnector host, Subnet subnet) { - + log.trace("sendUcastARPRequest host:{} subnet:{}", host, subnet); NodeConnector outPort = host.getnodeConnector(); if (outPort == null) { log.error("Failed sending UcastARP because cannot extract output port from Host: {}", host); @@ -426,6 +428,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA this.dataPacketService.transmitDataPacket(destPkt); } + @Override public void find(InetAddress networkAddress) { log.trace("Received find IP {}", networkAddress); @@ -445,6 +448,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA /* * Probe the host by sending a unicast ARP Request to the host */ + @Override public void probe(HostNodeConnector host) { log.trace("Received probe host {}", host); @@ -562,6 +566,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA * */ void destroy() { + cacheEventHandler.interrupt(); } /** @@ -571,9 +576,9 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA * */ void start() { + stopping = false; startPeriodicTimer(); cacheEventHandler.start(); - } /** @@ -586,6 +591,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA } void stopping() { + stopping = true; cancelPeriodicTimer(); } @@ -666,15 +672,25 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA } private void generateAndSendReply(InetAddress sourceIP, byte[] sourceMAC) { + if (log.isTraceEnabled()) { + log.trace("generateAndSendReply called with params sourceIP:{} sourceMAC:{}", sourceIP, + HexEncode.bytesToHexString(sourceMAC)); + } Set hosts = arpRequestors.remove(sourceIP); if ((hosts == null) || hosts.isEmpty()) { + log.trace("Bailing out no requestors Hosts"); return; } countDownTimers.remove(sourceIP); for (HostNodeConnector host : hosts) { - log.trace("Sending ARP Reply with src {}/{}, target {}/{}", - new Object[] { sourceMAC, sourceIP, host.getDataLayerAddressBytes(), host.getNetworkAddress() }); - + if (log.isTraceEnabled()) { + log.trace("Sending ARP Reply with src {}/{}, target {}/{}", + new Object[] { + HexEncode.bytesToHexString(sourceMAC), + sourceIP, + HexEncode.bytesToHexString(host.getDataLayerAddressBytes()), + host.getNetworkAddress() }); + } if (connectionManager.isLocal(host.getnodeconnectorNode())){ sendARPReply(host.getnodeConnector(), sourceMAC, @@ -682,6 +698,12 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA host.getDataLayerAddressBytes(), host.getNetworkAddress()); } else { + /* + * In the remote event a requestor moved to another + * controller it may turn out it now we need to send + * the ARP reply from a different controller, this + * cover the case + */ arpRequestReplyEvent.put( new ARPReply( host.getnodeConnector(), @@ -696,6 +718,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA @Override public void entryUpdated(ARPEvent key, Boolean new_value, String cacheName, boolean originLocal) { + log.trace("Got and entryUpdated for cacheName {} key {} isNew {}", cacheName, key, new_value); enqueueARPCacheEvent(key, new_value); } @@ -713,6 +736,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA ARPCacheEvent cacheEvent = new ARPCacheEvent(event, new_value); if (!ARPCacheEvents.contains(cacheEvent)) { this.ARPCacheEvents.add(cacheEvent); + log.trace("Enqueued {}", event); } } catch (Exception e) { log.debug("enqueueARPCacheEvent caught Interrupt Exception for event {}", event); @@ -725,7 +749,7 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA private class ARPCacheEventHandler implements Runnable { @Override public void run() { - while (true) { + while (!stopping) { try { ARPCacheEvent ev = ARPCacheEvents.take(); ARPEvent event = ev.getEvent(); @@ -733,20 +757,23 @@ public class ArpHandler implements IHostFinder, IListenDataPacket, ICacheUpdateA ARPRequest req = (ARPRequest) event; // If broadcast request if (req.getHost() == null) { + log.trace("Trigger and ARP Broadcast Request upon receipt of {}", req); sendBcastARPRequest(req.getTargetIP(), req.getSubnet()); //If unicast and local, send reply } else if (connectionManager.isLocal(req.getHost().getnodeconnectorNode())) { + log.trace("ARPCacheEventHandler - sendUcatARPRequest upon receipt of {}", req); sendUcastARPRequest(req.getHost(), req.getSubnet()); } } else if (event instanceof ARPReply) { ARPReply rep = (ARPReply) event; // New reply received by controller, notify all awaiting requestors across the cluster if (ev.isNewReply()) { + log.trace("Trigger a generateAndSendReply in response to {}", rep); generateAndSendReply(rep.getTargetIP(), rep.getTargetMac()); - // Otherwise, a specific reply. If local, send out. } else if (connectionManager.isLocal(rep.getPort().getNode())) { + log.trace("ARPCacheEventHandler - sendUcatARPReply locally in response to {}", rep); sendARPReply(rep.getPort(), rep.getSourceMac(), rep.getSourceIP(),