Using replace instead install when there is already an intent
Fixes the problem of flows being left on the data plane
Change-Id: Iec3db8b460123f2744a57d8c08d14c8effe9ec34
diff --git a/core/api/src/main/java/org/onosproject/net/intent/IntentInstaller.java b/core/api/src/main/java/org/onosproject/net/intent/IntentInstaller.java
index b260080..8a9577f 100644
--- a/core/api/src/main/java/org/onosproject/net/intent/IntentInstaller.java
+++ b/core/api/src/main/java/org/onosproject/net/intent/IntentInstaller.java
@@ -50,7 +50,6 @@
* @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);
}
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinating.java b/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinating.java
index abf8886..6145ad2 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinating.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinating.java
@@ -49,7 +49,7 @@
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);
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/Installing.java b/core/net/src/main/java/org/onosproject/net/intent/impl/Installing.java
index 3827f6d..122f224 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/Installing.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/Installing.java
@@ -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 @@
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?
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java b/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java
index fb772fd..e28457a 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java
@@ -66,8 +66,10 @@
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 @@
//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 @@
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 @@
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);
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/LinkCollectionIntentInstaller.java b/core/net/src/main/java/org/onosproject/net/intent/impl/LinkCollectionIntentInstaller.java
index 816fc12..abc08d1 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/LinkCollectionIntentInstaller.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/LinkCollectionIntentInstaller.java
@@ -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 @@
}
@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;
}
/**
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/WithdrawCoordinating.java b/core/net/src/main/java/org/onosproject/net/intent/impl/WithdrawCoordinating.java
index bd94ed6..aa5e993 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/WithdrawCoordinating.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/WithdrawCoordinating.java
@@ -47,6 +47,7 @@
@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));
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/Withdrawing.java b/core/net/src/main/java/org/onosproject/net/intent/impl/Withdrawing.java
index a797c49..06a0ad3 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/Withdrawing.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/Withdrawing.java
@@ -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 @@
@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));
}
}
diff --git a/utils/misc/src/main/java/org/onlab/util/Tools.java b/utils/misc/src/main/java/org/onlab/util/Tools.java
index c8b2b74..5e28fcd 100644
--- a/utils/misc/src/main/java/org/onlab/util/Tools.java
+++ b/utils/misc/src/main/java/org/onlab/util/Tools.java
@@ -34,6 +34,7 @@
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 @@
}
/**
+ * 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