Out of Memory when static Routes are configured with unresolved next hop 97/697/1
authorChi-Vien Ly <chivly@cisco.com>
Thu, 25 Jul 2013 05:51:03 +0000 (22:51 -0700)
committerChi-Vien Ly <chivly@cisco.com>
Thu, 25 Jul 2013 05:51:03 +0000 (22:51 -0700)
Root cause: This problem happens when there are staticRoutes configured but their
next hop IP are unresolved (ie. not responding to ARP Request).
StaticRouteImpl.java spawns a thread requesting hostTracker to discover the host,
this thread is waiting forever due to a bug in the thread (HostTrackerCallable)
that handles the discovery which never returns since the host is not responding.

Solution:
- Instead of creating threads dynamically, we create a threadpool of 2 and reuse
  the threads.
- Fix the bug in HostTrackerCallable to sleep(2000) and returns.

Change-Id: Ic04d1dbaf17d5cd6672646050d71abb9885695e9
Signed-off-by: Chi-Vien Ly <chivly@cisco.com>
opendaylight/forwarding/staticrouting/src/main/java/org/opendaylight/controller/forwarding/staticrouting/internal/StaticRoutingImplementation.java
opendaylight/hosttracker/implementation/src/main/java/org/opendaylight/controller/hosttracker/internal/HostTracker.java
opendaylight/hosttracker/implementation/src/main/java/org/opendaylight/controller/hosttracker/internal/HostTrackerCallable.java

index 4afd4fb..bccdea4 100644 (file)
@@ -24,8 +24,11 @@ import java.util.Map;
 import java.util.Set;
 import java.util.Timer;
 import java.util.TimerTask;
+import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -55,9 +58,6 @@ import org.slf4j.LoggerFactory;
 
 /**
  * Static Routing feature provides the bridge between SDN and Non-SDN networks.
- *
- *
- *
  */
 public class StaticRoutingImplementation implements IfNewHostNotify,
         IForwardingStaticRouting, IObjectReader, IConfigurationContainerAware,
@@ -75,6 +75,7 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
     private IClusterContainerServices clusterContainerService = null;
     private Set<IStaticRoutingAware> staticRoutingAware = Collections
             .synchronizedSet(new HashSet<IStaticRoutingAware>());
+    private ExecutorService executor;
 
     void setStaticRoutingAware(IStaticRoutingAware s) {
         if (this.staticRoutingAware != null) {
@@ -235,43 +236,53 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         }
     }
 
-    private class NotifyStaticRouteThread extends Thread {
+    private class NotifyStaticRouteWorker implements Callable<Object> {
         private StaticRoute staticRoute;
         private boolean added;
 
-        public NotifyStaticRouteThread(StaticRoute s, boolean update) {
+        public NotifyStaticRouteWorker(StaticRoute s, boolean update) {
             this.staticRoute = s;
             this.added = update;
         }
 
-        public void run() {
+        @Override
+        public Object call() throws Exception {
             if (!added
                     || (staticRoute.getType() == StaticRoute.NextHopType.SWITCHPORT)) {
                 notifyStaticRouteUpdate(staticRoute, added);
             } else {
-                HostNodeConnector host = hostTracker.hostQuery(staticRoute
-                        .getNextHopAddress());
+                InetAddress nh = staticRoute.getNextHopAddress();
+                HostNodeConnector host = hostTracker.hostQuery(nh);
                 if (host == null) {
-                    Future<HostNodeConnector> future = hostTracker
-                            .discoverHost(staticRoute.getNextHopAddress());
+                    log.debug("Next hop {}  is not present, try to discover it", nh.getHostAddress());
+                    Future<HostNodeConnector> future = hostTracker.discoverHost(nh);
                     if (future != null) {
                         try {
                             host = future.get();
                         } catch (Exception e) {
-                            log.error("",e);
+                            log.error("", e);
                         }
                     }
                 }
                 if (host != null) {
+                    log.debug("Next hop {} is found", nh.getHostAddress());
                     staticRoute.setHost(host);
                     notifyStaticRouteUpdate(staticRoute, added);
+                } else {
+                    log.debug("Next hop {}  is still not present, try again later", nh.getHostAddress());
                 }
             }
+            return null;
         }
     }
 
     private void checkAndUpdateListeners(StaticRoute staticRoute, boolean added) {
-        new NotifyStaticRouteThread(staticRoute, added).start();
+        NotifyStaticRouteWorker worker = new NotifyStaticRouteWorker(staticRoute, added);
+        try {
+            executor.submit(worker);
+        } catch (Exception e) {
+            log.error("got Exception ", e);
+        }
     }
 
     private void notifyHostUpdate(HostNodeConnector host, boolean added) {
@@ -440,7 +451,7 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         //staticRoutes = new ConcurrentHashMap<String, StaticRoute>();
         allocateCaches();
         retrieveCaches();
-
+        this.executor = Executors.newFixedThreadPool(1);
         if (staticRouteConfigs.isEmpty())
             loadConfiguration();
 
@@ -494,10 +505,12 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
      *
      */
     void stop() {
+        executor.shutdown();
     }
 
     @Override
     public Status saveConfiguration() {
         return this.saveConfig();
     }
+
 }
index 6b7bac0..5aa8f10 100644 (file)
@@ -97,7 +97,7 @@ public class HostTracker implements IfIptoHost, IfHostListener, ISwitchManagerAw
     private Timer timer;
     private Timer arp_refresh_timer;
     private String containerName = null;
-
+    private ExecutorService executor;
     private static class ARPPending {
         protected InetAddress hostIP;
         protected short sent_count;
@@ -158,7 +158,7 @@ public class HostTracker implements IfIptoHost, IfHostListener, ISwitchManagerAw
 
         timer = new Timer();
         timer.schedule(new OutStandingARPHandler(), 4000, 4000);
-
+        executor = Executors.newFixedThreadPool(2);
         /* ARP Refresh Timer to go off every 5 seconds to implement ARP aging */
         arp_refresh_timer = new Timer();
         arp_refresh_timer.schedule(new ARPRefreshHandler(), 5000, 5000);
@@ -296,7 +296,6 @@ public class HostTracker implements IfIptoHost, IfHostListener, ISwitchManagerAw
 
     @Override
     public Future<HostNodeConnector> discoverHost(InetAddress networkAddress) {
-        ExecutorService executor = Executors.newFixedThreadPool(1);
         if (executor == null) {
             logger.error("discoverHost: Null executor");
             return null;
@@ -1349,6 +1348,7 @@ public class HostTracker implements IfIptoHost, IfHostListener, ISwitchManagerAw
      *
      */
     void stop() {
+        executor.shutdown();
     }
 
     @Override
index 06311a5..3033082 100644 (file)
@@ -42,7 +42,7 @@ public class HostTrackerCallable implements Callable<HostNodeConnector> {
         if (h != null)
             return h;
         hostTracker.setCallableOnPendingARP(trackedHost, this);
-        latch.await();
+        Thread.sleep(2000); // wait 2sec to see if the host responds
         return hostTracker.hostQuery(trackedHost);
     }
 

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.