Improved consistency for BMv2 flow rules handling

Change-Id: I3a4798af3f35f135e8162385a1bf7fc059028307

Improved consistency for BMv2 flow rules handling
Change-Id: I3a4798af3f35f135e8162385a1bf7fc059028307
Carmelo Cascone · Gerrit Code Review
Commit 0b4afd27f3329bcd64820d5b2b61e7e4e1f0f57c 0b4afd27 1 parent ead7bbff
Showing 4 changed files with 56 additions and 37 deletions
drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
protocols/bmv2/api/src/main/java/org/onosproject/bmv2/api/service/Bmv2TableEntryService.java
protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2TableEntryServiceImpl.java
providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
--- a/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
View file @0b4afd2
+++ b/drivers/bmv2/src/main/java/org/onosproject/drivers/bmv2/Bmv2FlowRuleProgrammable.java
View file @0b4afd2
@@ -24,6 +24,7 @@ import org.onosproject.bmv2.api.context.Bmv2DeviceContext;
 import org.onosproject.bmv2.api.context.Bmv2FlowRuleTranslator;
 import org.onosproject.bmv2.api.context.Bmv2FlowRuleTranslatorException;
 import org.onosproject.bmv2.api.context.Bmv2Interpreter;
+import org.onosproject.bmv2.api.context.Bmv2TableModel;
 import org.onosproject.bmv2.api.runtime.Bmv2DeviceAgent;
 import org.onosproject.bmv2.api.runtime.Bmv2FlowRuleWrapper;
 import org.onosproject.bmv2.api.runtime.Bmv2MatchKey;
@@ -112,10 +113,10 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
         List<FlowEntry> entryList = Lists.newArrayList();
-        configuration.tables().forEach(table -> {
+        for (Bmv2TableModel table : configuration.tables()) {
             // For each table in the configuration AND exposed by the interpreter.
             if (!interpreter.tableIdMap().inverse().containsKey(table.name())) {
-                return;
+                continue; // next table
             }
             List<Bmv2ParsedTableEntry> installedEntries;
@@ -123,33 +124,34 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                 installedEntries = deviceAgent.getTableEntries(table.name());
             } catch (Bmv2RuntimeException e) {
                 log.warn("Failed to get table entries of table {} of {}: {}", table.name(), deviceId, e.explain());
-                return;
+                continue; // next table
             }
-            installedEntries.forEach(parsedEntry -> {
+            for (Bmv2ParsedTableEntry parsedEntry : installedEntries) {
-                Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId,
+                Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId, table.name(),
-                                                                               table.name(),
                                                                                parsedEntry.matchKey());
-                ENTRY_LOCKS.compute(entryRef, (key, value) -> {
-                    Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookupEntryReference(entryRef);
+                ENTRY_LOCKS.putIfAbsent(entryRef, true);
+                synchronized (ENTRY_LOCKS.get(entryRef)) {
+
+                    Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
                     if (frWrapper == null) {
                         log.warn("missing reference from table entry service, BUG? " +
                                          "deviceId={}, tableName={}, matchKey={}",
                                  deviceId, table.name(), entryRef.matchKey());
-                        return null;
+                        continue; // next entry
                     }
                     long remoteEntryId = parsedEntry.entryId();
                     long localEntryId = frWrapper.entryId();
                     if (remoteEntryId != localEntryId) {
-                        log.warn("getFlowEntries(): inconsistent entry id! BUG? Updating it... remote={}, local={}",
+                        log.debug("getFlowEntries(): inconsistent entry id! BUG? Updating it... remote={}, local={}",
                                  remoteEntryId, localEntryId);
                         frWrapper = new Bmv2FlowRuleWrapper(frWrapper.rule(), remoteEntryId,
                                                             frWrapper.creationDate());
-                        tableEntryService.bindEntryReference(entryRef, frWrapper);
+                        tableEntryService.bind(entryRef, frWrapper);
                     }
                     long bytes = 0L;
@@ -171,11 +173,9 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                     FlowEntry entry = new DefaultFlowEntry(frWrapper.rule(), ADDED, frWrapper.lifeInSeconds(),
                                                            packets, bytes);
                     entryList.add(entry);
-                    return true;
+                }
-                });
+            }
-
+        }
-            });
-        });
         return Collections.unmodifiableCollection(entryList);
     }
@@ -226,19 +226,16 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                 bmv2Entry = translator.translate(rule, context);
             } catch (Bmv2FlowRuleTranslatorException e) {
                 log.warn("Unable to translate flow rule: {} - {}", e.getMessage(), rule);
-                continue;
+                continue; // next rule
             }
             String tableName = bmv2Entry.tableName();
             Bmv2TableEntryReference entryRef = new Bmv2TableEntryReference(deviceId, tableName, bmv2Entry.matchKey());
-            /*
+            ENTRY_LOCKS.putIfAbsent(entryRef, true);
-            From here on threads are synchronized over entryKey, i.e. serialize operations
+            synchronized (ENTRY_LOCKS.get(entryRef)) {
-            over the same matchKey of a specific table and device.
-             */
-            ENTRY_LOCKS.compute(entryRef, (key, value) -> {
                 // Get from store
-                Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookupEntryReference(entryRef);
+                Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
                 try {
                     if (operation == Operation.APPLY) {
                         // Apply entry
@@ -269,15 +266,14 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                 } catch (Bmv2RuntimeException e) {
                     log.warn("Unable to {} flow rule: {}", operation.name(), e.explain());
                 }
-                // Update binding in table entry service.
+
+                // Update entryRef binding in table entry service.
                 if (frWrapper != null) {
-                    tableEntryService.bindEntryReference(entryRef, frWrapper);
+                    tableEntryService.bind(entryRef, frWrapper);
-                    return true;
                 } else {
-                    tableEntryService.unbindEntryReference(entryRef);
+                    tableEntryService.unbind(entryRef);
-                    return null;
                 }
-            });
+            }
         }
         return processedFlowRules;
@@ -287,7 +283,7 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
         try {
             return agent.addTableEntry(entry);
         } catch (Bmv2RuntimeException e) {
-            if (e.getCode() != TABLE_DUPLICATE_ENTRY) {
+            if (e.getCode().equals(TABLE_DUPLICATE_ENTRY)) {
                 forceRemove(agent, entry.tableName(), entry.matchKey());
                 return agent.addTableEntry(entry);
             } else {
@@ -301,7 +297,7 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
         try {
             agent.deleteTableEntry(tableName, entryId);
         } catch (Bmv2RuntimeException e) {
-            if (e.getCode() == TABLE_INVALID_HANDLE || e.getCode() == TABLE_EXPIRED_HANDLE) {
+            if (e.getCode().equals(TABLE_INVALID_HANDLE) || e.getCode().equals(TABLE_EXPIRED_HANDLE)) {
                 // entry is not there with the declared ID, try with a forced remove.
                 forceRemove(agent, tableName, matchKey);
             } else {
--- a/protocols/bmv2/api/src/main/java/org/onosproject/bmv2/api/service/Bmv2TableEntryService.java
View file @0b4afd2
+++ b/protocols/bmv2/api/src/main/java/org/onosproject/bmv2/api/service/Bmv2TableEntryService.java
View file @0b4afd2
@@ -21,6 +21,7 @@ import com.google.common.annotations.Beta;
 import org.onosproject.bmv2.api.context.Bmv2FlowRuleTranslator;
 import org.onosproject.bmv2.api.runtime.Bmv2FlowRuleWrapper;
 import org.onosproject.bmv2.api.runtime.Bmv2TableEntryReference;
+import org.onosproject.net.DeviceId;
 /**
  * A service for managing BMv2 table entries.
@@ -41,7 +42,7 @@ public interface Bmv2TableEntryService {
      * @param entryRef a table entry reference
      * @param rule     a BMv2 flow rule wrapper
      */
-    void bindEntryReference(Bmv2TableEntryReference entryRef, Bmv2FlowRuleWrapper rule);
+    void bind(Bmv2TableEntryReference entryRef, Bmv2FlowRuleWrapper rule);
     /**
      * Returns the ONOS flow rule associated with the given BMv2 table entry reference, or null if there's no such a
@@ -50,12 +51,19 @@ public interface Bmv2TableEntryService {
      * @param entryRef a table entry reference
      * @return a BMv2 flow rule wrapper
      */
-    Bmv2FlowRuleWrapper lookupEntryReference(Bmv2TableEntryReference entryRef);
+    Bmv2FlowRuleWrapper lookup(Bmv2TableEntryReference entryRef);
     /**
      * Removes any flow rule previously bound with a given BMv2 table entry reference.
      *
      * @param entryRef a table entry reference
      */
-    void unbindEntryReference(Bmv2TableEntryReference entryRef);
+    void unbind(Bmv2TableEntryReference entryRef);
+
+    /**
+     * Removes all bindings for a given device.
+     *
+     * @param deviceId a device ID
+     */
+    void unbindAll(DeviceId deviceId);
 }
--- a/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2TableEntryServiceImpl.java
View file @0b4afd2
+++ b/protocols/bmv2/ctl/src/main/java/org/onosproject/bmv2/ctl/Bmv2TableEntryServiceImpl.java
View file @0b4afd2
@@ -34,6 +34,7 @@ import org.onosproject.bmv2.api.runtime.Bmv2ValidMatchParam;
 import org.onosproject.bmv2.api.service.Bmv2Controller;
 import org.onosproject.bmv2.api.service.Bmv2DeviceContextService;
 import org.onosproject.bmv2.api.service.Bmv2TableEntryService;
+import org.onosproject.net.DeviceId;
 import org.onosproject.store.serializers.KryoNamespaces;
 import org.onosproject.store.service.EventuallyConsistentMap;
 import org.onosproject.store.service.StorageService;
@@ -99,21 +100,29 @@ public class Bmv2TableEntryServiceImpl implements Bmv2TableEntryService {
     }
     @Override
-    public Bmv2FlowRuleWrapper lookupEntryReference(Bmv2TableEntryReference entryRef) {
+    public Bmv2FlowRuleWrapper lookup(Bmv2TableEntryReference entryRef) {
         checkNotNull(entryRef, "table entry reference cannot be null");
         return flowRules.get(entryRef);
     }
     @Override
-    public void bindEntryReference(Bmv2TableEntryReference entryRef, Bmv2FlowRuleWrapper rule) {
+    public void bind(Bmv2TableEntryReference entryRef, Bmv2FlowRuleWrapper rule) {
         checkNotNull(entryRef, "table entry reference cannot be null");
         checkNotNull(rule, "bmv2 flow rule cannot be null");
         flowRules.put(entryRef, rule);
     }
     @Override
-    public void unbindEntryReference(Bmv2TableEntryReference entryRef) {
+    public void unbind(Bmv2TableEntryReference entryRef) {
         checkNotNull(entryRef, "table entry reference cannot be null");
         flowRules.remove(entryRef);
     }
+
+    @Override
+    public void unbindAll(DeviceId deviceId) {
+        flowRules.keySet()
+                .stream()
+                .filter(entryRef -> entryRef.deviceId().equals(deviceId))
+                .forEach(flowRules::remove);
+    }
 }
--- a/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
View file @0b4afd2
+++ b/providers/bmv2/device/src/main/java/org/onosproject/provider/bmv2/device/impl/Bmv2DeviceProvider.java
View file @0b4afd2
@@ -29,6 +29,7 @@ import org.onosproject.bmv2.api.runtime.Bmv2RuntimeException;
 import org.onosproject.bmv2.api.service.Bmv2Controller;
 import org.onosproject.bmv2.api.service.Bmv2DeviceContextService;
 import org.onosproject.bmv2.api.service.Bmv2DeviceListener;
+import org.onosproject.bmv2.api.service.Bmv2TableEntryService;
 import org.onosproject.common.net.AbstractDeviceProvider;
 import org.onosproject.core.ApplicationId;
 import org.onosproject.core.CoreService;
@@ -107,6 +108,9 @@ public class Bmv2DeviceProvider extends AbstractDeviceProvider {
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected Bmv2DeviceContextService contextService;
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    protected Bmv2TableEntryService tableEntryService;
+
     private ApplicationId appId;
     /**
@@ -228,6 +232,8 @@ public class Bmv2DeviceProvider extends AbstractDeviceProvider {
     private void resetDeviceState(DeviceId did) {
         try {
             controller.getAgent(did).resetState();
+            // Tables emptied. Reset all bindings.
+            tableEntryService.unbindAll(did);
         } catch (Bmv2RuntimeException e) {
             log.warn("Unable to reset {}: {}", did, e.toString());
         }