Timeout ForwardingRulesManager workOrder

- Before this patch is an FRM request was submitted and no answer was
received by a remote controller, that would indefinitively stuck the
caller, clearly a bad practice. This patch is introducing a default
timeout of 30 second (very bad scenario) and a log.error is being
raised if that happens.
- Cleaned workMonitor cache on workStatus coming back or timeout.
- Make timeout tunable via system properties

Change-Id: I59ac402b30101e3cf1cf1b1beacf03fbc9add6c9
Signed-off-by: Giovanni Meo <gmeo@cisco.com>

diff --git a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/FlowEntryDistributionOrderFutureTask.java b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/FlowEntryDistributionOrderFutureTask.java
index 1ebc300c75ac80c05a5ae6e8dc8b88366468f1c6..a51409fc2d241b61f2180b20841a942cf5e2ca36 100644 (file)
--- a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/FlowEntryDistributionOrderFutureTask.java
+++ b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/FlowEntryDistributionOrderFutureTask.java
@@ -36,6 +36,10 @@ final class FlowEntryDistributionOrderFutureTask implements Future<Status> {
     private CountDownLatch waitingLatch;
     private Status retStatus;
     private static final Logger logger = LoggerFactory.getLogger(FlowEntryDistributionOrderFutureTask.class);
+    // Don't wait forever to program, rather timeout if there are issues, and
+    // log an error
+    private long timeout;
+    private static final Long DEFAULTTIMEOUT = 30000L;
 
     /**
      * @param order
@@ -49,6 +53,14 @@ final class FlowEntryDistributionOrderFutureTask implements Future<Status> {
         this.waitingLatch = new CountDownLatch(1);
         // No return status yet!
         this.retStatus = new Status(StatusCode.UNDEFINED);
+        // Set the timeout
+        String strTimeout = System.getProperty("FlowEntryDistributionOrderFutureTask.timeout",
+                                               DEFAULTTIMEOUT.toString());
+        try {
+            timeout = Long.parseLong(strTimeout);
+        } catch (Exception e) {
+            timeout = DEFAULTTIMEOUT;
+        }
     }
 
     @Override
@@ -58,6 +70,7 @@ final class FlowEntryDistributionOrderFutureTask implements Future<Status> {
 
     @Override
     public Status get() throws InterruptedException, ExecutionException {
+        boolean didFinish = false;
         logger.trace("Getting status for order {}", this.order);
         // If i'm done lets return the status as many times as caller wants
         if (this.waitingLatch.getCount() == 0L) {
@@ -67,16 +80,22 @@ final class FlowEntryDistributionOrderFutureTask implements Future<Status> {
 
         logger.trace("Start waiting for status to come back");
         // Wait till someone signal that we are done
-        this.waitingLatch.await();
+        didFinish = this.waitingLatch.await(this.timeout, TimeUnit.MILLISECONDS);
 
-        logger.trace("Waiting for the status is over, returning it");
-        // Return the known status
-        return retStatus;
+        if (didFinish) {
+            logger.trace("Waiting for the status of order {} is over, returning it", this.order);
+            // Return the known status
+            return retStatus;
+        } else {
+            logger.error("Timing out, the workStatus for order {} has not come back in time!", this.order);
+            return new Status(StatusCode.TIMEOUT);
+        }
     }
 
     @Override
     public Status get(long timeout, TimeUnit unit) throws InterruptedException,
             ExecutionException, TimeoutException {
+        boolean didFinish = false;
         logger.trace("Getting status for order {}", this.order);
         // If i'm done lets return the status as many times as caller wants
         if (this.waitingLatch.getCount() == 0L) {
@@ -86,11 +105,18 @@ final class FlowEntryDistributionOrderFutureTask implements Future<Status> {
 
         logger.trace("Start waiting for status to come back");
         // Wait till someone signal that we are done
-        this.waitingLatch.await(timeout, unit);
+        didFinish = this.waitingLatch.await(timeout, unit);
 
-        logger.trace("Waiting for the status is over, returning it");
-        // Return the known status, could also be null if didn't return
-        return retStatus;
+        if (didFinish) {
+            logger.trace("Waiting for the status is over, returning it");
+            // Return the known status, could also be null if didn't return
+            return retStatus;
+        } else {
+            // No need to bark here as long as this routine could indeed
+            // timeout
+            logger.trace("Timing out, the workStatus for order {} has not come back in time!", this.order);
+            return new Status(StatusCode.TIMEOUT);
+        }
     }
 
     @Override
@@ -123,4 +149,12 @@ final class FlowEntryDistributionOrderFutureTask implements Future<Status> {
         this.waitingLatch.countDown();
         logger.trace("Unlocked the Future");
     }
+
+    /**
+     * Getter for the workOrder for which the order is waiting for
+     * @return the order
+     */
+    public FlowEntryDistributionOrder getOrder() {
+        return order;
+    }
 }

diff --git a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManager.java b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManager.java
index a6cdf2c466ab579986234f2a5a4c884fe8e7fd33..9c2afe42be0860df310516ff2f5b85ec1dd0261f 100644 (file)
--- a/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManager.java
+++ b/opendaylight/forwardingrulesmanager/implementation/src/main/java/org/opendaylight/controller/forwardingrulesmanager/internal/ForwardingRulesManager.java
@@ -214,7 +214,8 @@ public class ForwardingRulesManager implements
      * @return a Future object for monitoring the progress of the result, or
      *         null in case the processing should take place locally
      */
-    private Future<Status> distributeWorkOrder(FlowEntryInstall e, FlowEntryInstall u, UpdateType t) {
+    private FlowEntryDistributionOrderFutureTask distributeWorkOrder(FlowEntryInstall e, FlowEntryInstall u,
+            UpdateType t) {
         // A null entry it's an unexpected condition, anyway it's safe to keep
         // the handling local
         if (e == null) {
@@ -544,11 +545,17 @@ public class ForwardingRulesManager implements
      *         contain the unique id assigned to this request
      */
     private Status modifyEntryInternal(FlowEntryInstall currentEntries, FlowEntryInstall newEntries, boolean async) {
-        Future<Status> futureStatus = distributeWorkOrder(currentEntries, newEntries, UpdateType.CHANGED);
+        FlowEntryDistributionOrderFutureTask futureStatus =
+                distributeWorkOrder(currentEntries, newEntries, UpdateType.CHANGED);
         if (futureStatus != null) {
             Status retStatus = new Status(StatusCode.UNDEFINED);
             try {
                 retStatus = futureStatus.get();
+                if (retStatus.getCode()
+                        .equals(StatusCode.TIMEOUT)) {
+                    // A timeout happened, lets cleanup the workMonitor
+                    workMonitor.remove(futureStatus.getOrder());
+                }
             } catch (InterruptedException e) {
                 log.error("", e);
             } catch (ExecutionException e) {
@@ -656,11 +663,16 @@ public class ForwardingRulesManager implements
      *         contain the unique id assigned to this request
      */
     private Status removeEntryInternal(FlowEntryInstall entry, boolean async) {
-        Future<Status> futureStatus = distributeWorkOrder(entry, null, UpdateType.REMOVED);
+        FlowEntryDistributionOrderFutureTask futureStatus = distributeWorkOrder(entry, null, UpdateType.REMOVED);
         if (futureStatus != null) {
             Status retStatus = new Status(StatusCode.UNDEFINED);
             try {
                 retStatus = futureStatus.get();
+                if (retStatus.getCode()
+                        .equals(StatusCode.TIMEOUT)) {
+                    // A timeout happened, lets cleanup the workMonitor
+                    workMonitor.remove(futureStatus.getOrder());
+                }
             } catch (InterruptedException e) {
                 log.error("", e);
             } catch (ExecutionException e) {
@@ -704,11 +716,16 @@ public class ForwardingRulesManager implements
      *         contain the unique id assigned to this request
      */
     private Status addEntriesInternal(FlowEntryInstall entry, boolean async) {
-        Future<Status> futureStatus = distributeWorkOrder(entry, null, UpdateType.ADDED);
+        FlowEntryDistributionOrderFutureTask futureStatus = distributeWorkOrder(entry, null, UpdateType.ADDED);
         if (futureStatus != null) {
             Status retStatus = new Status(StatusCode.UNDEFINED);
             try {
                 retStatus = futureStatus.get();
+                if (retStatus.getCode()
+                        .equals(StatusCode.TIMEOUT)) {
+                    // A timeout happened, lets cleanup the workMonitor
+                    workMonitor.remove(futureStatus.getOrder());
+                }
             } catch (InterruptedException e) {
                 log.error("", e);
             } catch (ExecutionException e) {
@@ -3171,7 +3188,7 @@ public class ForwardingRulesManager implements
              */
             if (fe.getRequestorController()
                     .equals(clusterContainerService.getMyAddress())) {
-                FlowEntryDistributionOrderFutureTask fet = workMonitor.get(fe);
+                FlowEntryDistributionOrderFutureTask fet = workMonitor.remove(fe);
                 if (fet != null) {
                     logsync.trace("workStatus response is for us {}", fe);
                     // Signal we got the status