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.

Change-Id: I4b4e444c3a6dba7e4d3278e9469069e2dbdb9b67

@@ -362,6 +362,13 @@ public class DefaultFlowRule implements FlowRule {
     }
     }
 
 
     @Override
     @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() {
     public String toString() {
         return toStringHelper(this)
         return toStringHelper(this)
                 .add("id", Long.toHexString(id.value()))
                 .add("id", Long.toHexString(id.value()))
@@ -483,7 +490,7 @@ public class DefaultFlowRule implements FlowRule {
         }
         }
 
 
         private int hash() {
         private int hash() {
-            return Objects.hash(deviceId, selector, treatment, tableId);
+            return Objects.hash(deviceId, priority, selector, tableId);
         }
         }
 
 
     }
     }

@@ -136,6 +136,34 @@ public interface FlowRule {
     int tableId();
     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.
      * A flowrule builder.
      */
      */
     interface Builder {
     interface Builder {

@@ -426,6 +426,11 @@ public class IntentTestsMocks {
         }
         }
 
 
         @Override
         @Override
+        public boolean exactMatch(FlowRule rule) {
+            return this.equals(rule);
+        }
+
+        @Override
         public int tableId() {
         public int tableId() {
             return tableId;
             return tableId;
         }
         }

@@ -417,12 +417,22 @@ public class FlowRuleManager
 
 
         @Override
         @Override
         public void pushFlowMetrics(DeviceId deviceId, Iterable<FlowEntry> flowEntries) {
         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) {
             for (FlowEntry rule : flowEntries) {
                 try {
                 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 {
                     } else {
                         // the device has a rule the store does not have
                         // the device has a rule the store does not have
                         if (!allowExtraneousRules) {
                         if (!allowExtraneousRules) {
@@ -434,7 +444,7 @@ public class FlowRuleManager
                     continue;
                     continue;
                 }
                 }
             }
             }
-            for (FlowEntry rule : storedRules) {
+            for (FlowEntry rule : storedRules.keySet()) {
                 try {
                 try {
                     // there are rules in the store that aren't on the switch
                     // there are rules in the store that aren't on the switch
                     log.debug("Adding rule in store, but not on switch {}", rule);
                     log.debug("Adding rule in store, but not on switch {}", rule);

@@ -15,10 +15,11 @@
  */
  */
 package org.onosproject.rest;
 package org.onosproject.rest;
 
 
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Set;
-
+import com.eclipsesource.json.JsonArray;
+import com.eclipsesource.json.JsonObject;
+import com.google.common.collect.ImmutableSet;
+import com.sun.jersey.api.client.UniformInterfaceException;
+import com.sun.jersey.api.client.WebResource;
 import org.hamcrest.Description;
 import org.hamcrest.Description;
 import org.hamcrest.TypeSafeMatcher;
 import org.hamcrest.TypeSafeMatcher;
 import org.junit.After;
 import org.junit.After;
@@ -40,6 +41,7 @@ import org.onosproject.net.flow.DefaultTrafficSelector;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
 import org.onosproject.net.flow.DefaultTrafficTreatment;
 import org.onosproject.net.flow.FlowEntry;
 import org.onosproject.net.flow.FlowEntry;
 import org.onosproject.net.flow.FlowId;
 import org.onosproject.net.flow.FlowId;
+import org.onosproject.net.flow.FlowRule;
 import org.onosproject.net.flow.FlowRuleExtPayLoad;
 import org.onosproject.net.flow.FlowRuleExtPayLoad;
 import org.onosproject.net.flow.FlowRuleService;
 import org.onosproject.net.flow.FlowRuleService;
 import org.onosproject.net.flow.TrafficSelector;
 import org.onosproject.net.flow.TrafficSelector;
@@ -48,11 +50,9 @@ import org.onosproject.net.flow.criteria.Criterion;
 import org.onosproject.net.flow.instructions.Instruction;
 import org.onosproject.net.flow.instructions.Instruction;
 import org.onosproject.net.flow.instructions.Instructions;
 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.UniformInterfaceException;
-import com.sun.jersey.api.client.WebResource;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Set;
 
 
 import static org.easymock.EasyMock.anyObject;
 import static org.easymock.EasyMock.anyObject;
 import static org.easymock.EasyMock.createMock;
 import static org.easymock.EasyMock.createMock;
@@ -194,6 +194,11 @@ public class FlowsResourceTest extends ResourceTest {
         }
         }
 
 
         @Override
         @Override
+        public boolean exactMatch(FlowRule rule) {
+            return false;
+        }
+
+        @Override
         public FlowRuleExtPayLoad payLoad() {
         public FlowRuleExtPayLoad payLoad() {
             return null;
             return null;
         }
         }