Bug 6513 Remove FD from registry immediately

 - remove FlowDescriptor from DeviceFlowRegistry immeadiately
   instead of marking it as to-be-removed
 - updated tests

Change-Id: I269a510fa67b6d04181039515aaefda90b53b827
Signed-off-by: Andrej Leitner <andrej.leitner@pantheon.tech>

diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java
index 8823319f10ceeff12f235cc6caccc0119efd796b..e43433f4fb5692f4cbf0f94f4c630fd0b27bb3d3 100644 (file)
--- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java
+++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java
@@ -15,12 +15,10 @@ import java.util.List;
 import java.util.Map;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.inventory.rev130819.FlowCapableNode;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.inventory.rev130819.FlowId;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.Node;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeKey;
-import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 
 /**
- * Created by Martin Bobak <mbobak@cisco.com> on 8.4.2015.
+ * Registry for mapping composite-key of flow ({@link FlowRegistryKey}) from device view
+ * to flow descriptor ({@link FlowDescriptor}) as the identifier of the same flow in data store.
  */
 public interface DeviceFlowRegistry extends AutoCloseable {
 
@@ -32,12 +30,10 @@ public interface DeviceFlowRegistry extends AutoCloseable {
 
     FlowId storeIfNecessary(FlowRegistryKey flowRegistryKey);
 
-    void markToBeremoved(FlowRegistryKey flowRegistryKey);
+    void removeDescriptor(FlowRegistryKey flowRegistryKey);
 
     void update(FlowRegistryKey newFlowRegistryKey,FlowDescriptor flowDescriptor);
 
-    void removeMarked();
-
     Map<FlowRegistryKey, FlowDescriptor> getAllFlowDescriptors();
 
     @Override

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java
index c44f1e3e9c0cb6cb7c08f25adb58fd843414f54d..6d30a9da53d040553f3717b57f73f9dbdce148ee 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java
@@ -11,25 +11,19 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
 import com.google.common.collect.BiMap;
 import com.google.common.collect.HashBiMap;
-import com.google.common.collect.Maps;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
-import com.romix.scala.collection.concurrent.TrieMap;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
-import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.ReadOnlyTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
@@ -47,18 +41,12 @@ import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-/**
- * Created by Martin Bobak <mbobak@cisco.com> on 8.4.2015.
- */
 public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
     private static final Logger LOG = LoggerFactory.getLogger(DeviceFlowRegistryImpl.class);
     private static final String ALIEN_SYSTEM_FLOW_ID = "#UF$TABLE*";
     private static final AtomicInteger UNACCOUNTED_FLOWS_COUNTER = new AtomicInteger(0);
 
-
     private final BiMap<FlowRegistryKey, FlowDescriptor> flowRegistry = HashBiMap.create();
-    @GuardedBy("marks")
-    private final Collection<FlowRegistryKey> marks = new HashSet<>();
     private final DataBroker dataBroker;
     private final KeyedInstanceIdentifier<Node, NodeKey> instanceIdentifier;
     private final List<ListenableFuture<List<Optional<FlowCapableNode>>>> lastFillFutures = new ArrayList<>();
@@ -76,7 +64,6 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
         }
     };
 
-
     public DeviceFlowRegistryImpl(final DataBroker dataBroker, final KeyedInstanceIdentifier<Node, NodeKey> instanceIdentifier) {
         this.dataBroker = dataBroker;
         this.instanceIdentifier = instanceIdentifier;
@@ -173,13 +160,12 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
         return flowDescriptor;
     }
 
-
     @Override
     public void store(final FlowRegistryKey flowRegistryKey, final FlowDescriptor flowDescriptor) {
-        LOG.trace("Storing flowDescriptor with table ID : {} and flow ID : {} for flow hash : {}",
-                flowDescriptor.getTableKey().getId(), flowDescriptor.getFlowId().getValue(), flowRegistryKey.hashCode());
         synchronized (flowRegistryKey) {
             try {
+                LOG.trace("Storing flowDescriptor with table ID : {} and flow ID : {} for flow hash : {}",
+                        flowDescriptor.getTableKey().getId(), flowDescriptor.getFlowId().getValue(), flowRegistryKey.hashCode());
                 flowRegistry.put(flowRegistryKey, flowDescriptor);
             } catch (IllegalArgumentException ex) {
                 LOG.error("Flow with flowId {} already exists in table {}", flowDescriptor.getFlowId().getValue(),
@@ -193,9 +179,9 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
     }
 
     @Override
-    public void update(FlowRegistryKey newFlowRegistryKey,FlowDescriptor flowDescriptor){
-        LOG.trace("Updating the entry with hash: {}", newFlowRegistryKey.hashCode());
+    public void update(final FlowRegistryKey newFlowRegistryKey, final FlowDescriptor flowDescriptor) {
         synchronized (newFlowRegistryKey) {
+            LOG.trace("Updating the entry with hash: {}", newFlowRegistryKey.hashCode());
             flowRegistry.forcePut(newFlowRegistryKey, flowDescriptor);
         }
     }
@@ -223,23 +209,10 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
     }
 
     @Override
-    public void markToBeremoved(final FlowRegistryKey flowRegistryKey) {
-        synchronized (marks) {
-            marks.add(flowRegistryKey);
-        }
-
-        LOG.trace("Flow hash {} was marked for removal.", flowRegistryKey.hashCode());
-    }
-
-    @Override
-    public void removeMarked() {
-        synchronized (marks) {
-            for (FlowRegistryKey flowRegistryKey : marks) {
-                LOG.trace("Removing flow descriptor for flow hash : {}", flowRegistryKey.hashCode());
-                flowRegistry.remove(flowRegistryKey);
-            }
-
-            marks.clear();
+    public void removeDescriptor(final FlowRegistryKey flowRegistryKey) {
+        synchronized (flowRegistryKey) {
+            LOG.trace("Removing flow descriptor for flow hash : {}", flowRegistryKey.hashCode());
+            flowRegistry.remove(flowRegistryKey);
         }
     }
 
@@ -260,7 +233,6 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
         }
 
         flowRegistry.clear();
-        marks.clear();
     }
 
     @VisibleForTesting

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalFlowServiceImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalFlowServiceImpl.java
index b9fa55bb3cafa8c379c412765ca654b29d4c89e1..4531a0756bc5fcd11f55bb3ef5a679822727d41c 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalFlowServiceImpl.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/services/SalFlowServiceImpl.java
@@ -199,7 +199,7 @@ public class SalFlowServiceImpl implements SalFlowService, ItemLifeCycleSource {
                     LOG.debug("flow removed finished without error,");
                 }
                 FlowRegistryKey flowRegistryKey = FlowRegistryKeyFactory.create(input);
-                deviceContext.getDeviceFlowRegistry().markToBeremoved(flowRegistryKey);
+                deviceContext.getDeviceFlowRegistry().removeDescriptor(flowRegistryKey);
                 if (itemLifecycleListener != null) {
                     final FlowDescriptor flowDescriptor =
                             deviceContext.getDeviceFlowRegistry().retrieveIdForFlow(flowRegistryKey);
@@ -239,7 +239,7 @@ public class SalFlowServiceImpl implements SalFlowService, ItemLifeCycleSource {
 
             if (flowRef == null) {
                 // then this is equivalent to a delete
-                deviceFlowRegistry.markToBeremoved(flowRegistryKey);
+                deviceFlowRegistry.removeDescriptor(flowRegistryKey);
 
                 if (itemLifecycleListener != null) {
                     final FlowDescriptor flowDescriptor =

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsGatheringUtils.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsGatheringUtils.java
index 22ea5b560fa0f80f0d93ba2dd7b09a677c80c6ba..9cd9f1003e9c79ad15a7af5276183340a8ffb1fa 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsGatheringUtils.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsGatheringUtils.java
@@ -341,7 +341,6 @@ public final class StatisticsGatheringUtils {
                         txFacade.writeToTransaction(LogicalDatastoreType.OPERATIONAL, iiToTable, table);
                     }
                 }
-                registry.removeMarked();
                 readTx.close();
                 return Futures.immediateFuture(null);
             }

diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java
index 8a89a1cc8465aa6005365b00b54ec5497cf7d239..dafa7ca27e233202e06d2f64b3e591fbd66dead1 100644 (file)
--- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java
+++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java
@@ -112,8 +112,7 @@ public class DeviceFlowRegistryImplTest {
         order.verify(readOnlyTransaction).read(LogicalDatastoreType.OPERATIONAL, path);
         assertTrue(allFlowDescriptors.containsKey(key));
 
-        deviceFlowRegistry.markToBeremoved(key);
-        deviceFlowRegistry.removeMarked();
+        deviceFlowRegistry.removeDescriptor(key);
     }
 
     @Test
@@ -207,31 +206,15 @@ public class DeviceFlowRegistryImplTest {
     }
 
     @Test
-    public void testRemoveMarked() throws Exception {
-        deviceFlowRegistry.markToBeremoved(key);
-        deviceFlowRegistry.removeMarked();
+    public void testRemoveDescriptor() throws Exception {
+        deviceFlowRegistry.removeDescriptor(key);
         Assert.assertEquals(0, deviceFlowRegistry.getAllFlowDescriptors().size());
     }
 
-    @Test
-    public void testRemoveMarkedNegative() throws Exception {
-        final FlowAndStatisticsMapList flowStats = TestFlowHelper.createFlowAndStatisticsMapListBuilder(2).build();
-        FlowRegistryKey key2 = FlowRegistryKeyFactory.create(flowStats);
-        deviceFlowRegistry.markToBeremoved(key2);
-        deviceFlowRegistry.removeMarked();
-        Assert.assertEquals(1, deviceFlowRegistry.getAllFlowDescriptors().size());
-    }
-
     @Test
     public void testClose() throws Exception {
-        deviceFlowRegistry.markToBeremoved(key);
         deviceFlowRegistry.close();
         Assert.assertEquals(0, deviceFlowRegistry.getAllFlowDescriptors().size());
-
-        deviceFlowRegistry.store(key, descriptor);
-        Assert.assertEquals(1, deviceFlowRegistry.getAllFlowDescriptors().size());
-        deviceFlowRegistry.removeMarked();
-        Assert.assertEquals(1, deviceFlowRegistry.getAllFlowDescriptors().size());
     }
 
     @Test