refactoring intent framework installers

installers should return work rather than futures Change-Id: I664c9ef43279139eaf3bbc3cfc84b258b480eaab

refactoring intent framework installers
installers should return work rather than futures Change-Id: I664c9ef43279139eaf3bbc3cfc84b258b480eaab
Brian O'Connor
Commit f2dbde576bf3c91a54c2a96c8a4675090aa64092 f2dbde57 1 parent 6bf91bcb
Showing 5 changed files with 81 additions and 74 deletions
core/api/src/main/java/org/onlab/onos/net/intent/IntentInstaller.java
core/api/src/test/java/org/onlab/onos/net/intent/IntentServiceTest.java
core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
core/net/src/main/java/org/onlab/onos/net/intent/impl/LinkCollectionIntentInstaller.java
core/net/src/main/java/org/onlab/onos/net/intent/impl/PathIntentInstaller.java
--- a/core/api/src/main/java/org/onlab/onos/net/intent/IntentInstaller.java
View file @f2dbde5
+++ b/core/api/src/main/java/org/onlab/onos/net/intent/IntentInstaller.java
View file @f2dbde5
 package org.onlab.onos.net.intent;
-import java.util.concurrent.Future;
+import java.util.List;
-import org.onlab.onos.net.flow.CompletedBatchOperation;
+import org.onlab.onos.net.flow.FlowRuleBatchOperation;
 /**
  * Abstraction of entity capable of installing intents to the environment.
@@ -14,7 +14,7 @@ public interface IntentInstaller<T extends InstallableIntent> {
      * @param intent intent to be installed
      * @throws IntentException if issues are encountered while installing the intent
      */
-    Future<CompletedBatchOperation> install(T intent);
+    List<FlowRuleBatchOperation> install(T intent);
     /**
      * Uninstalls the specified intent from the environment.
@@ -22,5 +22,5 @@ public interface IntentInstaller<T extends InstallableIntent> {
      * @param intent intent to be uninstalled
      * @throws IntentException if issues are encountered while uninstalling the intent
      */
-    Future<CompletedBatchOperation> uninstall(T intent);
+    List<FlowRuleBatchOperation> uninstall(T intent);
 }
--- a/core/api/src/test/java/org/onlab/onos/net/intent/IntentServiceTest.java
View file @f2dbde5
+++ b/core/api/src/test/java/org/onlab/onos/net/intent/IntentServiceTest.java
View file @f2dbde5
@@ -14,12 +14,11 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
-import java.util.concurrent.Future;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
-import org.onlab.onos.net.flow.CompletedBatchOperation;
+import org.onlab.onos.net.flow.FlowRuleBatchOperation;
 /**
  * Suite of tests for the intent service contract.
@@ -298,7 +297,7 @@ public class IntentServiceTest {
         }
         @Override
-        public Future<CompletedBatchOperation> install(TestInstallableIntent intent) {
+        public List<FlowRuleBatchOperation> install(TestInstallableIntent intent) {
             if (fail) {
                 throw new IntentException("install failed by design");
             }
@@ -306,7 +305,7 @@ public class IntentServiceTest {
         }
         @Override
-        public Future<CompletedBatchOperation> uninstall(TestInstallableIntent intent) {
+        public List<FlowRuleBatchOperation> uninstall(TestInstallableIntent intent) {
             if (fail) {
                 throw new IntentException("remove failed by design");
             }
--- a/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
View file @f2dbde5
+++ b/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
View file @f2dbde5
@@ -34,6 +34,8 @@ import org.apache.felix.scr.annotations.Service;
 import org.onlab.onos.event.AbstractListenerRegistry;
 import org.onlab.onos.event.EventDeliveryService;
 import org.onlab.onos.net.flow.CompletedBatchOperation;
+import org.onlab.onos.net.flow.FlowRuleBatchOperation;
+import org.onlab.onos.net.flow.FlowRuleService;
 import org.onlab.onos.net.intent.InstallableIntent;
 import org.onlab.onos.net.intent.Intent;
 import org.onlab.onos.net.intent.IntentCompiler;
@@ -90,6 +92,9 @@ public class IntentManager
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected EventDeliveryService eventDispatcher;
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    protected FlowRuleService flowRuleService;
+
     @Activate
     public void activate() {
         store.setDelegate(delegate);
@@ -283,7 +288,7 @@ public class IntentManager
         // Indicate that the intent is entering the installing phase.
         store.setState(intent, INSTALLING);
-        List<Future<CompletedBatchOperation>> installFutures = Lists.newArrayList();
+        List<FlowRuleBatchOperation> installWork = Lists.newArrayList();
         try {
             List<InstallableIntent> installables = store.getInstallableIntents(intent.id());
             if (installables != null) {
@@ -291,13 +296,13 @@ public class IntentManager
                     registerSubclassInstallerIfNeeded(installable);
                     trackerService.addTrackedResources(intent.id(),
                                                        installable.requiredLinks());
-                    Future<CompletedBatchOperation> future = getInstaller(installable).install(installable);
+                    List<FlowRuleBatchOperation> batch = getInstaller(installable).install(installable);
-                    installFutures.add(future);
+                    installWork.addAll(batch);
                 }
             }
             // FIXME we have to wait for the installable intents
             //eventDispatcher.post(store.setState(intent, INSTALLED));
-            monitorExecutor.execute(new IntentInstallMonitor(intent, installFutures, INSTALLED));
+            monitorExecutor.execute(new IntentInstallMonitor(intent, installWork, INSTALLED));
         } catch (Exception e) {
             log.warn("Unable to install intent {} due to: {}", intent.id(), e);
             uninstallIntent(intent, RECOMPILING);
@@ -369,16 +374,16 @@ public class IntentManager
      * @param intent intent to be uninstalled
      */
     private void uninstallIntent(Intent intent, IntentState nextState) {
-        List<Future<CompletedBatchOperation>> uninstallFutures = Lists.newArrayList();
+        List<FlowRuleBatchOperation> uninstallWork = Lists.newArrayList();
         try {
             List<InstallableIntent> installables = store.getInstallableIntents(intent.id());
             if (installables != null) {
                 for (InstallableIntent installable : installables) {
-                    Future<CompletedBatchOperation> future = getInstaller(installable).uninstall(installable);
+                    List<FlowRuleBatchOperation> batches = getInstaller(installable).uninstall(installable);
-                    uninstallFutures.add(future);
+                    uninstallWork.addAll(batches);
                 }
             }
-            monitorExecutor.execute(new IntentInstallMonitor(intent, uninstallFutures, nextState));
+            monitorExecutor.execute(new IntentInstallMonitor(intent, uninstallWork, nextState));
         } catch (IntentException e) {
             log.warn("Unable to uninstall intent {} due to: {}", intent.id(), e);
         }
@@ -495,17 +500,27 @@ public class IntentManager
     private class IntentInstallMonitor implements Runnable {
         private final Intent intent;
+        private final List<FlowRuleBatchOperation> work;
         private final List<Future<CompletedBatchOperation>> futures;
         private final IntentState nextState;
         public IntentInstallMonitor(Intent intent,
-                List<Future<CompletedBatchOperation>> futures, IntentState nextState) {
+                List<FlowRuleBatchOperation> work,
+                IntentState nextState) {
             this.intent = intent;
-            this.futures = futures;
+            this.work = work;
+            // TODO how many Futures can be outstanding? one?
+            this.futures = Lists.newLinkedList();
             this.nextState = nextState;
+
+            // TODO need to kick off the first batch sometime, why not now?
+            futures.add(applyNextBatch());
         }
-        private void updateIntent(Intent intent) {
+        /**
+         * Update the intent store with the next status for this intent.
+         */
+        private void updateIntent() {
             if (nextState == RECOMPILING) {
                 executor.execute(new IntentTask(nextState, intent));
             } else if (nextState == INSTALLED || nextState == WITHDRAWN) {
@@ -515,22 +530,55 @@ public class IntentManager
             }
         }
-        @Override
+        /**
-        public void run() {
+         * Apply a list of FlowRules.
+         *
+         * @param rules rules to apply
+         */
+        private Future<CompletedBatchOperation> applyNextBatch() {
+            if (work.isEmpty()) {
+                return null;
+            }
+            FlowRuleBatchOperation batch = work.remove(0);
+            return flowRuleService.applyBatch(batch);
+        }
+
+        /**
+         * Iterate through the pending futures, and remove them when they have completed.
+         */
+        private void processFutures() {
+            List<Future<CompletedBatchOperation>> newFutures = Lists.newArrayList();
             for (Iterator<Future<CompletedBatchOperation>> i = futures.iterator(); i.hasNext();) {
                 Future<CompletedBatchOperation> future = i.next();
                 try {
                     // TODO: we may want to get the future here and go back to the future.
                     CompletedBatchOperation completed = future.get(100, TimeUnit.NANOSECONDS);
-                    // TODO check if future succeeded and if not report fail items
+                    if (completed.isSuccess()) {
+                        Future<CompletedBatchOperation> newFuture = applyNextBatch();
+                        if (newFuture != null) {
+                            // we'll add this later so that we don't get a ConcurrentModException
+                            newFutures.add(newFuture);
+                        }
+                    } else {
+                        // TODO check if future succeeded and if not report fail items
+                        log.warn("Failed items: {}", completed.failedItems());
+                        // TODO revert....
+                        //uninstallIntent(intent, RECOMPILING);
+                    }
                     i.remove();
-
                 } catch (TimeoutException | InterruptedException | ExecutionException te) {
                     log.debug("Intallations of intent {} is still pending", intent);
                 }
             }
+            futures.addAll(newFutures);
+        }
+
+        @Override
+        public void run() {
+            processFutures();
             if (futures.isEmpty()) {
-                updateIntent(intent);
+                // woohoo! we are done!
+                updateIntent();
             } else {
                 // resubmit ourselves if we are not done yet
                 monitorExecutor.submit(this);
--- a/core/net/src/main/java/org/onlab/onos/net/intent/impl/LinkCollectionIntentInstaller.java
View file @f2dbde5
+++ b/core/net/src/main/java/org/onlab/onos/net/intent/impl/LinkCollectionIntentInstaller.java
View file @f2dbde5
@@ -4,7 +4,6 @@ import static org.onlab.onos.net.flow.DefaultTrafficTreatment.builder;
 import static org.slf4j.LoggerFactory.getLogger;
 import java.util.List;
-import java.util.concurrent.Future;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
@@ -16,14 +15,12 @@ import org.onlab.onos.CoreService;
 import org.onlab.onos.net.DeviceId;
 import org.onlab.onos.net.Link;
 import org.onlab.onos.net.PortNumber;
-import org.onlab.onos.net.flow.CompletedBatchOperation;
 import org.onlab.onos.net.flow.DefaultFlowRule;
 import org.onlab.onos.net.flow.DefaultTrafficSelector;
 import org.onlab.onos.net.flow.FlowRule;
 import org.onlab.onos.net.flow.FlowRuleBatchEntry;
 import org.onlab.onos.net.flow.FlowRuleBatchEntry.FlowRuleOperation;
 import org.onlab.onos.net.flow.FlowRuleBatchOperation;
-import org.onlab.onos.net.flow.FlowRuleService;
 import org.onlab.onos.net.flow.TrafficSelector;
 import org.onlab.onos.net.flow.TrafficTreatment;
 import org.onlab.onos.net.intent.IntentExtensionService;
@@ -47,9 +44,6 @@ public class LinkCollectionIntentInstaller implements IntentInstaller<LinkCollec
     protected IntentExtensionService intentManager;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    protected FlowRuleService flowRuleService;
-
-    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected CoreService coreService;
     private ApplicationId appId;
@@ -65,18 +59,8 @@ public class LinkCollectionIntentInstaller implements IntentInstaller<LinkCollec
         intentManager.unregisterInstaller(PathIntent.class);
     }
-    /**
-     * Apply a list of FlowRules.
-     *
-     * @param rules rules to apply
-     */
-    private Future<CompletedBatchOperation> applyBatch(List<FlowRuleBatchEntry> rules) {
-        FlowRuleBatchOperation batch = new FlowRuleBatchOperation(rules);
-        return flowRuleService.applyBatch(batch);
-    }
-
     @Override
-    public Future<CompletedBatchOperation> install(LinkCollectionIntent intent) {
+    public List<FlowRuleBatchOperation> install(LinkCollectionIntent intent) {
         TrafficSelector.Builder builder =
                 DefaultTrafficSelector.builder(intent.selector());
         List<FlowRuleBatchEntry> rules = Lists.newLinkedList();
@@ -92,11 +76,11 @@ public class LinkCollectionIntentInstaller implements IntentInstaller<LinkCollec
                 intent.egressPoint().deviceId(),
                 intent.egressPoint().port()));
-        return applyBatch(rules);
+        return Lists.newArrayList(new FlowRuleBatchOperation(rules));
     }
     @Override
-    public Future<CompletedBatchOperation> uninstall(LinkCollectionIntent intent) {
+    public List<FlowRuleBatchOperation> uninstall(LinkCollectionIntent intent) {
         TrafficSelector.Builder builder =
                 DefaultTrafficSelector.builder(intent.selector());
         List<FlowRuleBatchEntry> rules = Lists.newLinkedList();
@@ -113,7 +97,7 @@ public class LinkCollectionIntentInstaller implements IntentInstaller<LinkCollec
                intent.egressPoint().deviceId(),
                intent.egressPoint().port()));
-        return applyBatch(rules);
+        return Lists.newArrayList(new FlowRuleBatchOperation(rules));
     }
     /**
--- a/core/net/src/main/java/org/onlab/onos/net/intent/impl/PathIntentInstaller.java
View file @f2dbde5
+++ b/core/net/src/main/java/org/onlab/onos/net/intent/impl/PathIntentInstaller.java
View file @f2dbde5
@@ -5,7 +5,6 @@ import static org.slf4j.LoggerFactory.getLogger;
 import java.util.Iterator;
 import java.util.List;
-import java.util.concurrent.Future;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
@@ -16,14 +15,12 @@ import org.onlab.onos.ApplicationId;
 import org.onlab.onos.CoreService;
 import org.onlab.onos.net.ConnectPoint;
 import org.onlab.onos.net.Link;
-import org.onlab.onos.net.flow.CompletedBatchOperation;
 import org.onlab.onos.net.flow.DefaultFlowRule;
 import org.onlab.onos.net.flow.DefaultTrafficSelector;
 import org.onlab.onos.net.flow.FlowRule;
 import org.onlab.onos.net.flow.FlowRuleBatchEntry;
 import org.onlab.onos.net.flow.FlowRuleBatchEntry.FlowRuleOperation;
 import org.onlab.onos.net.flow.FlowRuleBatchOperation;
-import org.onlab.onos.net.flow.FlowRuleService;
 import org.onlab.onos.net.flow.TrafficSelector;
 import org.onlab.onos.net.flow.TrafficTreatment;
 import org.onlab.onos.net.intent.IntentExtensionService;
@@ -45,9 +42,6 @@ public class PathIntentInstaller implements IntentInstaller<PathIntent> {
     protected IntentExtensionService intentManager;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    protected FlowRuleService flowRuleService;
-
-    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected CoreService coreService;
     private ApplicationId appId;
@@ -63,31 +57,14 @@ public class PathIntentInstaller implements IntentInstaller<PathIntent> {
         intentManager.unregisterInstaller(PathIntent.class);
     }
-    /**
-     * Apply a list of FlowRules.
-     *
-     * @param rules rules to apply
-     */
-    private Future<CompletedBatchOperation> applyBatch(List<FlowRuleBatchEntry> rules) {
-        FlowRuleBatchOperation batch = new FlowRuleBatchOperation(rules);
-        Future<CompletedBatchOperation> future = flowRuleService.applyBatch(batch);
-        return future;
-//        try {
-//            //FIXME don't do this here
-//            future.get();
-//        } catch (InterruptedException | ExecutionException e) {
-//            // TODO Auto-generated catch block
-//            e.printStackTrace();
-//        }
-    }
-
     @Override
-    public Future<CompletedBatchOperation> install(PathIntent intent) {
+    public List<FlowRuleBatchOperation> install(PathIntent intent) {
         TrafficSelector.Builder builder =
                 DefaultTrafficSelector.builder(intent.selector());
         Iterator<Link> links = intent.path().links().iterator();
         ConnectPoint prev = links.next().dst();
         List<FlowRuleBatchEntry> rules = Lists.newLinkedList();
+        // TODO Generate multiple batches
         while (links.hasNext()) {
             builder.matchInport(prev.port());
             Link link = links.next();
@@ -100,18 +77,17 @@ public class PathIntentInstaller implements IntentInstaller<PathIntent> {
             rules.add(new FlowRuleBatchEntry(FlowRuleOperation.ADD, rule));
             prev = link.dst();
         }
-
+        return Lists.newArrayList(new FlowRuleBatchOperation(rules));
-        return applyBatch(rules);
     }
     @Override
-    public Future<CompletedBatchOperation> uninstall(PathIntent intent) {
+    public List<FlowRuleBatchOperation> uninstall(PathIntent intent) {
         TrafficSelector.Builder builder =
                 DefaultTrafficSelector.builder(intent.selector());
         Iterator<Link> links = intent.path().links().iterator();
         ConnectPoint prev = links.next().dst();
         List<FlowRuleBatchEntry> rules = Lists.newLinkedList();
-
+        // TODO Generate multiple batches
         while (links.hasNext()) {
             builder.matchInport(prev.port());
             Link link = links.next();
@@ -123,7 +99,7 @@ public class PathIntentInstaller implements IntentInstaller<PathIntent> {
             rules.add(new FlowRuleBatchEntry(FlowRuleOperation.REMOVE, rule));
             prev = link.dst();
         }
-        return applyBatch(rules);
+        return Lists.newArrayList(new FlowRuleBatchOperation(rules));
     }
     // TODO refactor below this line... ----------------------------