modified SimpleFlowRuleStore not to use MultiMap

Change-Id: Ie9adb127f1acb4d919951c75513e689fbd80596d

modified SimpleFlowRuleStore not to use MultiMap
Change-Id: Ie9adb127f1acb4d919951c75513e689fbd80596d
Yuta HIGUCHI
Commit 605347c288b4dd8962a9b4c72c9c3523553f52d0 605347c2 1 parent 32cc1c00
Showing 2 changed files with 125 additions and 58 deletions
core/net/src/test/java/org/onlab/onos/net/flow/impl/FlowRuleManagerTest.java
core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleFlowRuleStore.java
--- a/core/net/src/test/java/org/onlab/onos/net/flow/impl/FlowRuleManagerTest.java
View file @605347c
+++ b/core/net/src/test/java/org/onlab/onos/net/flow/impl/FlowRuleManagerTest.java
View file @605347c
@@ -8,7 +8,9 @@ import static org.onlab.onos.net.flow.FlowRuleEvent.Type.RULE_UPDATED;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
@@ -54,6 +56,7 @@ import org.onlab.onos.net.provider.ProviderId;
 import org.onlab.onos.store.trivial.impl.SimpleFlowRuleStore;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
@@ -166,16 +169,17 @@ public class FlowRuleManagerTest {
     }
+    // TODO: If preserving iteration order is a requirement, redo FlowRuleStore.
     //backing store is sensitive to the order of additions/removals
-    private boolean validateState(FlowEntryState... state) {
+    private boolean validateState(Map<FlowRule, FlowEntryState> expected) {
+        Map<FlowRule, FlowEntryState> expectedToCheck = new HashMap<>(expected);
         Iterable<FlowEntry> rules = service.getFlowEntries(DID);
-        int i = 0;
         for (FlowEntry f : rules) {
-            if (f.state() != state[i]) {
+            assertTrue("Unexpected FlowRule " + f, expectedToCheck.containsKey(f));
-                return false;
+            assertEquals("FlowEntry" + f, expectedToCheck.get(f), f.state());
-            }
+            expectedToCheck.remove(f);
-            i++;
         }
+        assertEquals(Collections.emptySet(), expectedToCheck.entrySet());
         return true;
     }
@@ -191,8 +195,10 @@ public class FlowRuleManagerTest {
         mgr.applyFlowRules(r1, r2, r3);
         assertEquals("3 rules should exist", 3, flowCount());
         assertTrue("Entries should be pending add.",
-                   validateState(FlowEntryState.PENDING_ADD, FlowEntryState.PENDING_ADD,
+                   validateState(ImmutableMap.of(
-                                 FlowEntryState.PENDING_ADD));
+                                   r1, FlowEntryState.PENDING_ADD,
+                                   r2, FlowEntryState.PENDING_ADD,
+                                   r3, FlowEntryState.PENDING_ADD)));
     }
     @Test
@@ -213,8 +219,10 @@ public class FlowRuleManagerTest {
         validateEvents();
         assertEquals("3 rule should exist", 3, flowCount());
         assertTrue("Entries should be pending remove.",
-                   validateState(FlowEntryState.PENDING_REMOVE, FlowEntryState.PENDING_REMOVE,
+                   validateState(ImmutableMap.of(
-                                 FlowEntryState.ADDED));
+                                 f1, FlowEntryState.PENDING_REMOVE,
+                                 f2, FlowEntryState.PENDING_REMOVE,
+                                 f3, FlowEntryState.ADDED)));
         mgr.removeFlowRules(f1);
         assertEquals("3 rule should still exist", 3, flowCount());
@@ -263,8 +271,10 @@ public class FlowRuleManagerTest {
         providerService.pushFlowMetrics(DID, Lists.newArrayList(fe1, fe2));
         assertTrue("Entries should be added.",
-                validateState(FlowEntryState.ADDED, FlowEntryState.ADDED,
+                validateState(ImmutableMap.of(
-                        FlowEntryState.PENDING_ADD));
+                        f1, FlowEntryState.ADDED,
+                        f2, FlowEntryState.ADDED,
+                        f3, FlowEntryState.PENDING_ADD)));
         validateEvents(RULE_ADDED, RULE_ADDED);
     }
@@ -336,7 +346,9 @@ public class FlowRuleManagerTest {
         //only check that we are in pending remove. Events and actual remove state will
         // be set by flowRemoved call.
-        validateState(FlowEntryState.PENDING_REMOVE, FlowEntryState.PENDING_REMOVE);
+        validateState(ImmutableMap.of(
+                    f1, FlowEntryState.PENDING_REMOVE,
+                    f2, FlowEntryState.PENDING_REMOVE));
     }
     @Test
@@ -360,7 +372,9 @@ public class FlowRuleManagerTest {
                 Lists.newArrayList(fbe1, fbe2));
         Future<CompletedBatchOperation> future = mgr.applyBatch(fbo);
         assertTrue("Entries in wrong state",
-                   validateState(FlowEntryState.PENDING_REMOVE, FlowEntryState.PENDING_ADD));
+                   validateState(ImmutableMap.of(
+                               f1, FlowEntryState.PENDING_REMOVE,
+                               f2, FlowEntryState.PENDING_ADD)));
         CompletedBatchOperation completed = null;
         try {
             completed = future.get();
@@ -381,9 +395,18 @@ public class FlowRuleManagerTest {
         mgr.applyFlowRules(f1);
+        assertTrue("Entries in wrong state",
+                validateState(ImmutableMap.of(
+                            f1, FlowEntryState.PENDING_ADD)));
+
         FlowEntry fe1 = new DefaultFlowEntry(f1);
         providerService.pushFlowMetrics(DID, Collections.<FlowEntry>singletonList(fe1));
+        assertTrue("Entries in wrong state",
+                validateState(ImmutableMap.of(
+                            f1, FlowEntryState.ADDED)));
+
+
         FlowRuleBatchEntry fbe1 = new FlowRuleBatchEntry(
                 FlowRuleBatchEntry.FlowRuleOperation.REMOVE, f1);
@@ -403,8 +426,9 @@ public class FlowRuleManagerTest {
          * state.
          */
         assertTrue("Entries in wrong state",
-                   validateState(FlowEntryState.PENDING_REMOVE,
+                   validateState(ImmutableMap.of(
-                                 FlowEntryState.PENDING_ADD));
+                               f2, FlowEntryState.PENDING_REMOVE,
+                               f1, FlowEntryState.PENDING_ADD)));
     }
--- a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleFlowRuleStore.java
View file @605347c
+++ b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleFlowRuleStore.java
View file @605347c
@@ -2,9 +2,13 @@ package org.onlab.onos.store.trivial.impl;
 import static org.onlab.onos.net.flow.FlowRuleEvent.Type.RULE_REMOVED;
 import static org.slf4j.LoggerFactory.getLogger;
+import static org.apache.commons.lang3.concurrent.ConcurrentUtils.createIfAbsentUnchecked;
+import static java.util.Collections.unmodifiableCollection;
-import java.util.Collection;
+import java.util.HashSet;
-import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
@@ -15,18 +19,16 @@ import org.onlab.onos.net.DeviceId;
 import org.onlab.onos.net.flow.DefaultFlowEntry;
 import org.onlab.onos.net.flow.FlowEntry;
 import org.onlab.onos.net.flow.FlowEntry.FlowEntryState;
+import org.onlab.onos.net.flow.FlowId;
 import org.onlab.onos.net.flow.FlowRule;
 import org.onlab.onos.net.flow.FlowRuleEvent;
 import org.onlab.onos.net.flow.FlowRuleEvent.Type;
 import org.onlab.onos.net.flow.FlowRuleStore;
 import org.onlab.onos.net.flow.FlowRuleStoreDelegate;
 import org.onlab.onos.store.AbstractStore;
+import org.onlab.util.NewConcurrentHashMap;
 import org.slf4j.Logger;
-import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Multimap;
-
 /**
  * Manages inventory of flow rules using trivial in-memory implementation.
  */
@@ -38,12 +40,11 @@ public class SimpleFlowRuleStore
     private final Logger log = getLogger(getClass());
-    // store entries as a pile of rules, no info about device tables
-    private final Multimap<DeviceId, FlowEntry> flowEntries =
-            ArrayListMultimap.<DeviceId, FlowEntry>create();
-    private final Multimap<Short, FlowRule> flowEntriesById =
+    // inner Map is Device flow table
-            ArrayListMultimap.<Short, FlowRule>create();
+    // Assumption: FlowId cannot have synonyms
+    private final ConcurrentMap<DeviceId, ConcurrentMap<FlowId, FlowEntry>>
+            flowEntries = new ConcurrentHashMap<>();
     @Activate
     public void activate() {
@@ -52,88 +53,130 @@ public class SimpleFlowRuleStore
     @Deactivate
     public void deactivate() {
+        flowEntries.clear();
         log.info("Stopped");
     }
     @Override
     public int getFlowRuleCount() {
-        return flowEntries.size();
+        int sum = 0;
+        for (ConcurrentMap<FlowId, FlowEntry> ft : flowEntries.values()) {
+            sum += ft.size();
+        }
+        return sum;
     }
-    @Override
+    private static NewConcurrentHashMap<FlowId, FlowEntry> lazyEmptyFlowTable() {
-    public synchronized FlowEntry getFlowEntry(FlowRule rule) {
+        return NewConcurrentHashMap.<FlowId, FlowEntry>ifNeeded();
-        for (FlowEntry f : flowEntries.get(rule.deviceId())) {
-            if (f.equals(rule)) {
-                return f;
     }
+
+    /**
+     * Returns the flow table for specified device.
+     *
+     * @param deviceId identifier of the device
+     * @return Map representing Flow Table of given device.
+     */
+    private ConcurrentMap<FlowId, FlowEntry> getFlowTable(DeviceId deviceId) {
+        return createIfAbsentUnchecked(flowEntries,
+                                       deviceId, lazyEmptyFlowTable());
     }
-        return null;
+
+    private FlowEntry getFlowEntry(DeviceId deviceId, FlowId flowId) {
+        return getFlowTable(deviceId).get(flowId);
     }
     @Override
-    public synchronized Iterable<FlowEntry> getFlowEntries(DeviceId deviceId) {
+    public FlowEntry getFlowEntry(FlowRule rule) {
-        Collection<FlowEntry> rules = flowEntries.get(deviceId);
+        return getFlowEntry(rule.deviceId(), rule.id());
-        if (rules == null) {
-            return Collections.emptyList();
     }
-        return ImmutableSet.copyOf(rules);
+
+    @Override
+    public Iterable<FlowEntry> getFlowEntries(DeviceId deviceId) {
+        return unmodifiableCollection(getFlowTable(deviceId).values());
     }
     @Override
-    public synchronized Iterable<FlowRule> getFlowRulesByAppId(ApplicationId appId) {
+    public Iterable<FlowRule> getFlowRulesByAppId(ApplicationId appId) {
-        Collection<FlowRule> rules = flowEntriesById.get(appId.id());
+
-        if (rules == null) {
+        Set<FlowRule> rules = new HashSet<>();
-            return Collections.emptyList();
+        for (DeviceId did : flowEntries.keySet()) {
+            ConcurrentMap<FlowId, FlowEntry> ft = getFlowTable(did);
+            for (FlowEntry fe : ft.values()) {
+                if (fe.appId() == appId.id()) {
+                    rules.add(fe);
                 }
-        return ImmutableSet.copyOf(rules);
+            }
+        }
+        return rules;
     }
     @Override
-    public synchronized void storeFlowRule(FlowRule rule) {
+    public void storeFlowRule(FlowRule rule) {
+        final boolean added = storeFlowRuleInternal(rule);
+    }
+
+    private boolean storeFlowRuleInternal(FlowRule rule) {
         FlowEntry f = new DefaultFlowEntry(rule);
-        DeviceId did = f.deviceId();
+        final DeviceId did = f.deviceId();
-        if (!flowEntries.containsEntry(did, f)) {
+        final FlowId fid = f.id();
-            flowEntries.put(did, f);
+        FlowEntry existing = getFlowTable(did).putIfAbsent(fid, f);
-            flowEntriesById.put(rule.appId(), f);
+        if (existing != null) {
+            // was already there? ignore
+            return false;
         }
+        // new flow rule added
+        // TODO: notify through delegate about remote event?
+        return true;
     }
     @Override
-    public synchronized void deleteFlowRule(FlowRule rule) {
+    public void deleteFlowRule(FlowRule rule) {
-        FlowEntry entry = getFlowEntry(rule);
+
+        FlowEntry entry = getFlowEntry(rule.deviceId(), rule.id());
         if (entry == null) {
-            //log.warn("Cannot find rule {}", rule);
+            log.warn("Cannot find rule {}", rule);
+            System.err.println("Cannot find rule " + rule);
             return;
         }
+        synchronized (entry) {
             entry.setState(FlowEntryState.PENDING_REMOVE);
         }
+    }
     @Override
-    public synchronized FlowRuleEvent addOrUpdateFlowRule(FlowEntry rule) {
+    public FlowRuleEvent addOrUpdateFlowRule(FlowEntry rule) {
-        DeviceId did = rule.deviceId();
-
         // check if this new rule is an update to an existing entry
-        FlowEntry stored = getFlowEntry(rule);
+        FlowEntry stored = getFlowEntry(rule.deviceId(), rule.id());
         if (stored != null) {
+            synchronized (stored) {
                 stored.setBytes(rule.bytes());
                 stored.setLife(rule.life());
                 stored.setPackets(rule.packets());
                 if (stored.state() == FlowEntryState.PENDING_ADD) {
                     stored.setState(FlowEntryState.ADDED);
+                    // TODO: Do we need to change `rule` state?
                     return new FlowRuleEvent(Type.RULE_ADDED, rule);
                 }
                 return new FlowRuleEvent(Type.RULE_UPDATED, rule);
             }
+        }
+
+        // should not reach here
+        // storeFlowRule was expected to be called
+        log.error("FlowRule was not found in store {} to update", rule);
         //flowEntries.put(did, rule);
         return null;
     }
     @Override
-    public synchronized FlowRuleEvent removeFlowRule(FlowEntry rule) {
+    public FlowRuleEvent removeFlowRule(FlowEntry rule) {
         // This is where one could mark a rule as removed and still keep it in the store.
-        if (flowEntries.remove(rule.deviceId(), rule)) {
+        final DeviceId did = rule.deviceId();
+
+        ConcurrentMap<FlowId, FlowEntry> ft = getFlowTable(did);
+        if (ft.remove(rule.id(), rule)) {
             return new FlowRuleEvent(RULE_REMOVED, rule);
         } else {
             return null;