Jonathan Hart

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
......@@ -249,6 +249,13 @@ public class DefaultFlowRule implements FlowRule {
}
@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 @@ public class DefaultFlowRule implements FlowRule {
}
private int hash() {
return Objects.hash(deviceId, selector, treatment, tableId);
return Objects.hash(deviceId, priority, selector, tableId);
}
}
......
......@@ -138,6 +138,34 @@ public interface FlowRule {
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 {
......
......@@ -423,6 +423,11 @@ public class IntentTestsMocks {
}
@Override
public boolean exactMatch(FlowRule rule) {
return this.equals(rule);
}
@Override
public int tableId() {
return tableId;
}
......
......@@ -387,12 +387,22 @@ public class FlowRuleManager
@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 @@ public class FlowRuleManager
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);
......
......@@ -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.DefaultTrafficSelector;
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.criteria.Criterion;
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 @@ public class FlowsResourceTest extends ResourceTest {
}
@Override
public boolean exactMatch(FlowRule rule) {
return false;
}
@Override
public FlowRuleExtPayLoad payLoad() {
return null;
}
......