Bug 6110: Fixed bugs in statistics manager due to race condition.
[openflowplugin.git] / openflowplugin-impl / src / main / java / org / opendaylight / openflowplugin / impl / registry / flow / DeviceFlowRegistryImpl.java
index 0f051f7230588392a5c1aa3068b3a80654bbab0e..01f7178e2f3cb656f92dd08ec4d3e0c55ab64a0d 100644 (file)
@@ -9,23 +9,22 @@ package org.opendaylight.openflowplugin.impl.registry.flow;
 
 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.concurrent.ConcurrentMap;
+import java.util.Objects;
 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;
@@ -43,17 +42,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 ConcurrentMap<FlowRegistryKey, FlowDescriptor> flowRegistry = new TrieMap<>();
-    @GuardedBy("marks")
-    private final Collection<FlowRegistryKey> marks = new HashSet<>();
+    private final BiMap<FlowRegistryKey, FlowDescriptor> flowRegistry = Maps.synchronizedBiMap(HashBiMap.create());
     private final DataBroker dataBroker;
     private final KeyedInstanceIdentifier<Node, NodeKey> instanceIdentifier;
     private final List<ListenableFuture<List<Optional<FlowCapableNode>>>> lastFillFutures = new ArrayList<>();
@@ -71,7 +65,6 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
         }
     };
 
-
     public DeviceFlowRegistryImpl(final DataBroker dataBroker, final KeyedInstanceIdentifier<Node, NodeKey> instanceIdentifier) {
         this.dataBroker = dataBroker;
         this.instanceIdentifier = instanceIdentifier;
@@ -79,7 +72,7 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
 
     @Override
     public ListenableFuture<List<Optional<FlowCapableNode>>> fill() {
-        LOG.debug("Filling flow registry with flows for node: {}", instanceIdentifier);
+        LOG.debug("Filling flow registry with flows for node: {}", instanceIdentifier.getKey().getId().getValue());
 
         // Prepare path for read transaction
         // TODO: Read only Tables, and not entire FlowCapableNode (fix Yang model)
@@ -125,8 +118,14 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
             @Override
             public void onSuccess(Optional<FlowCapableNode> result) {
                 result.asSet().stream()
+                        .filter(Objects::nonNull)
+                        .filter(flowCapableNode -> Objects.nonNull(flowCapableNode.getTable()))
                         .flatMap(flowCapableNode -> flowCapableNode.getTable().stream())
+                        .filter(Objects::nonNull)
+                        .filter(table -> Objects.nonNull(table.getFlow()))
                         .flatMap(table -> table.getFlow().stream())
+                        .filter(Objects::nonNull)
+                        .filter(flow -> Objects.nonNull(flow.getId()))
                         .forEach(flowConsumer);
 
                 // After we are done with reading from datastore, close the transaction
@@ -145,10 +144,13 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
 
     @Override
     public FlowDescriptor retrieveIdForFlow(final FlowRegistryKey flowRegistryKey) {
-        LOG.trace("Retrieving flowDescriptor for flow hash: {}", flowRegistryKey.hashCode());
+        LOG.trace("Retrieving flow descriptor for flow hash : {}", flowRegistryKey.hashCode());
         FlowDescriptor flowDescriptor = flowRegistry.get(flowRegistryKey);
+        // Get FlowDescriptor from flow registry
         if(flowDescriptor == null){
-            LOG.info("Triggered the loop");
+            if (LOG.isTraceEnabled()) {
+                LOG.trace("Failed to retrieve flow descriptor for flow hash : {}, trying with custom equals method", flowRegistryKey.hashCode());
+            }
             for(Map.Entry<FlowRegistryKey, FlowDescriptor> fd : flowRegistry.entrySet()) {
                 if (fd.getKey().equals(flowRegistryKey)) {
                     flowDescriptor = fd.getValue();
@@ -156,25 +158,41 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
                 }
             }
         }
-        // Get FlowDescriptor from flow registry
         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());
-        flowRegistry.put(flowRegistryKey, flowDescriptor);
+        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.warn("Flow with flowId {} already exists in table {}", flowDescriptor.getFlowId().getValue(),
+                    flowDescriptor.getTableKey().getId());
+          final FlowId newFlowId = createAlienFlowId(flowDescriptor.getTableKey().getId());
+          final FlowDescriptor newFlowDescriptor = FlowDescriptorFactory.
+            create(flowDescriptor.getTableKey().getId(), newFlowId);
+          flowRegistry.put(flowRegistryKey, newFlowDescriptor);
+        }
+    }
+
+    @Override
+    public void update(final FlowRegistryKey newFlowRegistryKey, final FlowDescriptor flowDescriptor) {
+        LOG.trace("Updating the entry with hash: {}", newFlowRegistryKey.hashCode());
+        flowRegistry.forcePut(newFlowRegistryKey, flowDescriptor);
     }
 
     @Override
     public FlowId storeIfNecessary(final FlowRegistryKey flowRegistryKey) {
-        LOG.trace("Trying to retrieve flowDescriptor for flow hash: {}", flowRegistryKey.hashCode());
+        LOG.trace("Trying to retrieve flow ID for flow hash : {}", flowRegistryKey.hashCode());
 
         // First, try to get FlowDescriptor from flow registry
         FlowDescriptor flowDescriptor = retrieveIdForFlow(flowRegistryKey);
 
         // We was not able to retrieve FlowDescriptor, so we will at least try to generate it
         if (flowDescriptor == null) {
+            LOG.trace("Flow descriptor for flow hash : {} not found, generating alien flow ID", flowRegistryKey.hashCode());
             final short tableId = flowRegistryKey.getTableId();
             final FlowId alienFlowId = createAlienFlowId(tableId);
             flowDescriptor = FlowDescriptorFactory.create(tableId, alienFlowId);
@@ -188,24 +206,9 @@ 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 flowDescriptor for flow hash : {}", flowRegistryKey.hashCode());
-                flowRegistry.remove(flowRegistryKey);
-            }
-
-            marks.clear();
-        }
+    public void removeDescriptor(final FlowRegistryKey flowRegistryKey) {
+        LOG.trace("Removing flow descriptor for flow hash : {}", flowRegistryKey.hashCode());
+        flowRegistry.remove(flowRegistryKey);
     }
 
     @Override
@@ -225,7 +228,6 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
         }
 
         flowRegistry.clear();
-        marks.clear();
     }
 
     @VisibleForTesting
@@ -233,4 +235,4 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry {
         final String alienId = ALIEN_SYSTEM_FLOW_ID + tableId + '-' + UNACCOUNTED_FLOWS_COUNTER.incrementAndGet();
         return new FlowId(alienId);
     }
-}
+}
\ No newline at end of file