From: Giovanni Meo Date: Mon, 23 Sep 2013 20:02:40 +0000 (+0200) Subject: Timeout ForwardingRulesManager workOrder X-Git-Tag: releasepom-0.1.0~23 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=b1a1182db907b19fe986d3cd18146b7842fc58c9 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 --- 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 1ebc300c75..a51409fc2d 100644 --- 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 { 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 { 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 { @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 { 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 { 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 { 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 a6cdf2c466..9c2afe42be 100644 --- 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 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 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 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 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