Fix for flow equality bug which can cause:
* multiple flows with the same match to be simultaneously present in the flow
store, and
* similar but different rule in switch and store, resulting in flows stuck in
PENDING_ADD state.
Fixes ONOS-2426.
Ported from onos-1.2 branch.
Change-Id: I4b4e444c3a6dba7e4d3278e9469069e2dbdb9b67
diff --git a/core/api/src/main/java/org/onosproject/net/flow/DefaultFlowRule.java b/core/api/src/main/java/org/onosproject/net/flow/DefaultFlowRule.java
index 293f12f..44a4d36 100644
--- a/core/api/src/main/java/org/onosproject/net/flow/DefaultFlowRule.java
+++ b/core/api/src/main/java/org/onosproject/net/flow/DefaultFlowRule.java
@@ -249,6 +249,13 @@
}
@Override
+ public boolean exactMatch(FlowRule rule) {
+ return this.equals(rule) &&
+ Objects.equals(this.id, rule.id()) &&
+ Objects.equals(this.treatment, rule.treatment());
+ }
+
+ @Override
public String toString() {
return toStringHelper(this)
.add("id", Long.toHexString(id.value()))
@@ -370,7 +377,7 @@
}
private int hash() {
- return Objects.hash(deviceId, selector, treatment, tableId);
+ return Objects.hash(deviceId, priority, selector, tableId);
}
}
diff --git a/core/api/src/main/java/org/onosproject/net/flow/FlowRule.java b/core/api/src/main/java/org/onosproject/net/flow/FlowRule.java
index 397149b..e446a9f 100644
--- a/core/api/src/main/java/org/onosproject/net/flow/FlowRule.java
+++ b/core/api/src/main/java/org/onosproject/net/flow/FlowRule.java
@@ -138,6 +138,34 @@
int tableId();
/**
+ * {@inheritDoc}
+ *
+ * Equality for flow rules only considers 'match equality'. This means that
+ * two flow rules with the same match conditions will be equal, regardless
+ * of the treatment or other characteristics of the flow.
+ *
+ * @param obj the reference object with which to compare.
+ * @return {@code true} if this object is the same as the obj
+ * argument; {@code false} otherwise.
+ */
+ boolean equals(Object obj);
+
+ /**
+ * Returns whether this flow rule is an exact match to the flow rule given
+ * in the argument.
+ * <p>
+ * Exact match means that deviceId, priority, selector,
+ * tableId, flowId and treatment are equal. Note that this differs from
+ * the notion of object equality for flow rules, which does not consider the
+ * flowId or treatment when testing equality.
+ * </p>
+ *
+ * @param rule other rule to match against
+ * @return true if the rules are an exact match, otherwise false
+ */
+ boolean exactMatch(FlowRule rule);
+
+ /**
* A flowrule builder.
*/
interface Builder {
diff --git a/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java b/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
index 6aeeb56..550406f 100644
--- a/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
+++ b/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
@@ -423,6 +423,11 @@
}
@Override
+ public boolean exactMatch(FlowRule rule) {
+ return this.equals(rule);
+ }
+
+ @Override
public int tableId() {
return tableId;
}
diff --git a/core/net/src/main/java/org/onosproject/net/flow/impl/FlowRuleManager.java b/core/net/src/main/java/org/onosproject/net/flow/impl/FlowRuleManager.java
index b35002d..488d35c 100644
--- a/core/net/src/main/java/org/onosproject/net/flow/impl/FlowRuleManager.java
+++ b/core/net/src/main/java/org/onosproject/net/flow/impl/FlowRuleManager.java
@@ -387,12 +387,22 @@
@Override
public void pushFlowMetrics(DeviceId deviceId, Iterable<FlowEntry> flowEntries) {
- Set<FlowEntry> storedRules = Sets.newHashSet(store.getFlowEntries(deviceId));
+ Map<FlowEntry, FlowEntry> storedRules = Maps.newHashMap();
+ store.getFlowEntries(deviceId).forEach(f -> storedRules.put(f, f));
+
for (FlowEntry rule : flowEntries) {
try {
- if (storedRules.remove(rule)) {
- // we both have the rule, let's update some info then.
- flowAdded(rule);
+ FlowEntry storedRule = storedRules.remove(rule);
+ if (storedRule != null) {
+ if (storedRule.exactMatch(rule)) {
+ // we both have the rule, let's update some info then.
+ flowAdded(rule);
+ } else {
+ // the two rules are not an exact match - remove the
+ // switch's rule and install our rule
+ extraneousFlow(rule);
+ flowMissing(storedRule);
+ }
} else {
// the device has a rule the store does not have
if (!allowExtraneousRules) {
@@ -404,7 +414,7 @@
continue;
}
}
- for (FlowEntry rule : storedRules) {
+ for (FlowEntry rule : storedRules.keySet()) {
try {
// there are rules in the store that aren't on the switch
log.debug("Adding rule in store, but not on switch {}", rule);
diff --git a/web/api/src/test/java/org/onosproject/rest/FlowsResourceTest.java b/web/api/src/test/java/org/onosproject/rest/FlowsResourceTest.java
index 192c417..9cb63e7 100644
--- a/web/api/src/test/java/org/onosproject/rest/FlowsResourceTest.java
+++ b/web/api/src/test/java/org/onosproject/rest/FlowsResourceTest.java
@@ -15,14 +15,12 @@
*/
package org.onosproject.rest;
-import java.io.InputStream;
-import java.net.HttpURLConnection;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Set;
-
-import javax.ws.rs.core.MediaType;
-
+import com.eclipsesource.json.JsonArray;
+import com.eclipsesource.json.JsonObject;
+import com.google.common.collect.ImmutableSet;
+import com.sun.jersey.api.client.ClientResponse;
+import com.sun.jersey.api.client.UniformInterfaceException;
+import com.sun.jersey.api.client.WebResource;
import org.hamcrest.Description;
import org.hamcrest.Matchers;
import org.hamcrest.TypeSafeMatcher;
@@ -49,6 +47,7 @@
import org.onosproject.net.flow.DefaultTrafficTreatment;
import org.onosproject.net.flow.FlowEntry;
import org.onosproject.net.flow.FlowId;
+import org.onosproject.net.flow.FlowRule;
import org.onosproject.net.flow.FlowRuleExtPayLoad;
import org.onosproject.net.flow.FlowRuleService;
import org.onosproject.net.flow.TrafficSelector;
@@ -57,12 +56,12 @@
import org.onosproject.net.flow.instructions.Instruction;
import org.onosproject.net.flow.instructions.Instructions;
-import com.eclipsesource.json.JsonArray;
-import com.eclipsesource.json.JsonObject;
-import com.google.common.collect.ImmutableSet;
-import com.sun.jersey.api.client.ClientResponse;
-import com.sun.jersey.api.client.UniformInterfaceException;
-import com.sun.jersey.api.client.WebResource;
+import javax.ws.rs.core.MediaType;
+import java.io.InputStream;
+import java.net.HttpURLConnection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Set;
import static org.easymock.EasyMock.anyObject;
import static org.easymock.EasyMock.anyShort;
@@ -209,6 +208,11 @@
}
@Override
+ public boolean exactMatch(FlowRule rule) {
+ return false;
+ }
+
+ @Override
public FlowRuleExtPayLoad payLoad() {
return null;
}