Using replace instead install when there is already an intent

Fixes the problem of flows being left on the data plane

Change-Id: Iec3db8b460123f2744a57d8c08d14c8effe9ec34

@@ -50,7 +50,6 @@ public interface IntentInstaller<T extends Intent> {
      * @return flow rule operations to complete the replace
      * @throws IntentException if issues are encountered while uninstalling the intent
      */
-    @Deprecated
     List<FlowRuleBatchOperation> replace(T oldIntent, T newIntent);
 
 }

@@ -49,7 +49,7 @@ class InstallCoordinating implements IntentUpdate {
         try {
             //FIXME we orphan flow rules that are currently on the data plane
             // ... should either reuse them or remove them
-            FlowRuleOperations flowRules = intentManager.coordinate(pending);
+            FlowRuleOperations flowRules = intentManager.coordinate(current, pending);
             return Optional.of(new Installing(intentManager, pending, flowRules));
         } catch (IntentException e) {
             log.warn("Unable to generate a FlowRuleOperations from intent {} due to:", pending.intent().id(), e);

@@ -16,7 +16,6 @@
 package org.onosproject.net.intent.impl;
 
 import org.onosproject.net.flow.FlowRuleOperations;
-import org.onosproject.net.intent.Intent;
 import org.onosproject.net.intent.IntentData;
 import org.onosproject.net.intent.IntentException;
 import org.slf4j.Logger;
@@ -49,9 +48,6 @@ class Installing implements IntentUpdate {
     public Optional<IntentUpdate> execute() {
         try {
             intentManager.flowRuleService.apply(flowRules); // FIXME we need to provide a context
-            for (Intent installable: pending.installables()) {
-                intentManager.trackerService.addTrackedResources(pending.key(), installable.resources());
-            }
             return Optional.of(new Installed(pending));
         // What kinds of exceptions are thrown by FlowRuleService.apply()?
         // Is IntentException a correct exception abstraction?

@@ -66,8 +66,10 @@ import java.util.concurrent.Future;
 import java.util.stream.Collectors;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static java.util.concurrent.Executors.newFixedThreadPool;
 import static java.util.concurrent.Executors.newSingleThreadExecutor;
+import static org.onlab.util.Tools.isNullOrEmpty;
 import static org.onlab.util.Tools.namedThreads;
 import static org.onosproject.net.intent.IntentState.*;
 import static org.slf4j.LoggerFactory.getLogger;
@@ -282,13 +284,42 @@ public class IntentManager
 
     //TODO javadoc
     //FIXME
-    FlowRuleOperations coordinate(IntentData pending) {
-        List<Intent> installables = pending.installables();
-        List<List<FlowRuleBatchOperation>> plans = new ArrayList<>(installables.size());
-        for (Intent installable : installables) {
-            registerSubclassInstallerIfNeeded(installable);
+    FlowRuleOperations coordinate(IntentData current, IntentData pending) {
+        List<Intent> oldInstallables = (current != null) ? current.installables() : null;
+        List<Intent> newInstallables = pending.installables();
+
+        checkState(isNullOrEmpty(oldInstallables) ||
+                   oldInstallables.size() == newInstallables.size(),
+                   "Old and New Intent must have equivalent installable intents.");
+
+        List<List<FlowRuleBatchOperation>> plans = new ArrayList<>();
+        for (int i = 0; i < newInstallables.size(); i++) {
+            Intent newInstallable = newInstallables.get(i);
+            registerSubclassInstallerIfNeeded(newInstallable);
             //TODO consider migrating installers to FlowRuleOperations
-            plans.add(getInstaller(installable).install(installable));
+            /* FIXME
+               - we need to do another pass on this method about that doesn't
+               require the length of installables to be equal, and also doesn't
+               depend on ordering
+               - we should also reconsider when to start/stop tracking resources
+             */
+            if (isNullOrEmpty(oldInstallables)) {
+                plans.add(getInstaller(newInstallable).install(newInstallable));
+            } else {
+                Intent oldInstallable = oldInstallables.get(i);
+                checkState(oldInstallable.getClass().equals(newInstallable.getClass()),
+                           "Installable Intent type mismatch.");
+                trackerService.removeTrackedResources(pending.key(), oldInstallable.resources());
+                plans.add(getInstaller(newInstallable).replace(oldInstallable, newInstallable));
+            }
+            trackerService.addTrackedResources(pending.key(), newInstallable.resources());
+//            } catch (IntentException e) {
+//                log.warn("Unable to update intent {} due to:", oldIntent.id(), e);
+//                //FIXME... we failed. need to uninstall (if same) or revert (if different)
+//                trackerService.removeTrackedResources(newIntent.id(), newInstallable.resources());
+//                exception = e;
+//                batches = uninstallIntent(oldIntent, oldInstallables);
+//            }
         }
 
         return merge(plans).build(new FlowRuleOperationsContext() { // TODO move this out
@@ -321,6 +352,7 @@ public class IntentManager
         List<List<FlowRuleBatchOperation>> plans = new ArrayList<>();
         for (Intent installable : installables) {
             plans.add(getInstaller(installable).uninstall(installable));
+            trackerService.removeTrackedResources(pending.key(), installable.resources());
         }
 
         return merge(plans).build(new FlowRuleOperationsContext() {
@@ -494,7 +526,7 @@ public class IntentManager
             case INSTALL_REQ:
                 return new InstallRequest(this, intentData, Optional.ofNullable(current));
             case WITHDRAW_REQ:
-                if (current == null) {
+                if (current == null || isNullOrEmpty(current.installables())) {
                     return new Withdrawn(current, WITHDRAWN);
                 } else {
                     return new WithdrawRequest(this, intentData, current);

@@ -16,6 +16,7 @@
 package org.onosproject.net.intent.impl;
 
 import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Lists;
 import com.google.common.collect.SetMultimap;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
@@ -112,10 +113,13 @@ public class LinkCollectionIntentInstaller
     }
 
     @Override
-    public List<FlowRuleBatchOperation> replace(LinkCollectionIntent intent,
+    public List<FlowRuleBatchOperation> replace(LinkCollectionIntent oldIntent,
                                                 LinkCollectionIntent newIntent) {
-        // FIXME: implement
-        return null;
+        // FIXME: implement this in a more intelligent/less brute force way
+        List<FlowRuleBatchOperation> batches = Lists.newArrayList();
+        batches.addAll(uninstall(oldIntent));
+        batches.addAll(install(newIntent));
+        return batches;
     }
 
     /**

@@ -47,6 +47,7 @@ class WithdrawCoordinating implements IntentUpdate {
     @Override
     public Optional<IntentUpdate> execute() {
         try {
+            // Note: current.installables() are not null or empty due to createIntentUpdate check
             FlowRuleOperations flowRules = intentManager.uninstallCoordinate(current, pending);
             pending.setInstallables(current.installables());
             return Optional.of(new Withdrawing(intentManager, pending, flowRules));

@@ -16,7 +16,6 @@
 package org.onosproject.net.intent.impl;
 
 import org.onosproject.net.flow.FlowRuleOperations;
-import org.onosproject.net.intent.Intent;
 import org.onosproject.net.intent.IntentData;
 
 import java.util.Optional;
@@ -43,9 +42,6 @@ class Withdrawing implements IntentUpdate {
     @Override
     public Optional<IntentUpdate> execute() {
         intentManager.flowRuleService.apply(flowRules);
-        for (Intent installable: pending.installables()) {
-            intentManager.trackerService.removeTrackedResources(pending.key(), installable.resources());
-        }
         return Optional.of(new Withdrawn(pending));
     }
 }

@@ -34,6 +34,7 @@ import java.nio.file.SimpleFileVisitor;
 import java.nio.file.StandardCopyOption;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.ThreadFactory;
 
@@ -84,6 +85,16 @@ public abstract class Tools {
     }
 
     /**
+     * Returns true if the collection is null or is empty.
+     *
+     * @param collection collection to test
+     * @return true if null or empty; false otherwise
+     */
+    public static boolean isNullOrEmpty(Collection collection) {
+        return collection == null || collection.isEmpty();
+    }
+
+    /**
      * Converts a string from hex to long.
      *
      * @param string hex number in string form; sans 0x