Various BMv2 bugfixes

Change-Id: Ia5a2a1c86b8a90ad68ddb92980377f6308e200d2

@@ -64,6 +64,8 @@ import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -87,7 +89,7 @@ public abstract class AbstractUpgradableFabricApp {
     private static final int NUM_SPINES = 3;
     private static final int FLOW_PRIORITY = 100;
 
-    private static final int CLEANUP_SLEEP = 1000;
+    private static final int CLEANUP_SLEEP = 2000;
 
     protected final Logger log = getLogger(getClass());
 
@@ -137,10 +139,11 @@ public abstract class AbstractUpgradableFabricApp {
     private Set<DeviceId> spineSwitches;
 
     private Map<DeviceId, List<FlowRule>> deviceFlowRules;
+    private Map<DeviceId, Bmv2DeviceContext> previousContexts;
     private Map<DeviceId, Boolean> contextFlags;
     private Map<DeviceId, Boolean> ruleFlags;
 
-    private ConcurrentMap<DeviceId, Boolean> deployLocks = Maps.newConcurrentMap();
+    private ConcurrentMap<DeviceId, Lock> deviceLocks = Maps.newConcurrentMap();
 
     /**
      * Creates a new BMv2 fabric app.
@@ -270,7 +273,7 @@ public abstract class AbstractUpgradableFabricApp {
     public abstract List<FlowRule> generateSpineRules(DeviceId deviceId, Collection<Host> dstHosts, Topology topology)
             throws FlowRuleGeneratorException;
 
-    private void deployRoutine() {
+    private void deployAllDevices() {
         if (otherAppFound && otherApp.appActive) {
             log.info("Deactivating other app...");
             appService.deactivate(otherApp.appId);
@@ -297,9 +300,10 @@ public abstract class AbstractUpgradableFabricApp {
         DeviceId deviceId = device.id();
 
         // Synchronize executions over the same device.
-        deployLocks.putIfAbsent(deviceId, new Boolean(true));
-        synchronized (deployLocks.get(deviceId)) {
+        Lock lock = deviceLocks.computeIfAbsent(deviceId, k -> new ReentrantLock());
+        lock.lock();
 
+        try {
             // Set context if not already done.
             if (!contextFlags.getOrDefault(deviceId, false)) {
                 log.info("Setting context to {} for {}...", configurationName, deviceId);
@@ -321,6 +325,8 @@ public abstract class AbstractUpgradableFabricApp {
                     ruleFlags.put(deviceId, true);
                 }
             }
+        } finally {
+            lock.unlock();
         }
     }
 
@@ -433,10 +439,9 @@ public abstract class AbstractUpgradableFabricApp {
         // Avoid other executions to modify the generated flow rules.
         flowRuleGenerated = true;
 
-        log.info("DONE! Generated {} flow rules for {} devices...", newFlowRules.size(), spines.size() + leafs.size());
+        log.info("Generated {} flow rules for {} devices", newFlowRules.size(), spines.size() + leafs.size());
 
-        // Deploy configuration.
-        spawnTask(this::deployRoutine);
+        spawnTask(this::deployAllDevices);
     }
 
     /**

-/Users/carmelo/workspace/onos-p4-dev/p4src/build/ecmp.json
\ No newline at end of file

@@ -105,7 +105,7 @@ public class WcmpFabricApp extends AbstractUpgradableFabricApp {
             }
             return true;
         } catch (Bmv2RuntimeException e) {
-            log.error("Unable to init device {}: {}", deviceId, e.explain());
+            log.debug("Exception while initializing device {}: {}", deviceId, e.explain());
             return false;
         }
     }

-/Users/carmelo/workspace/onos-p4-dev/p4src/build/wcmp.json
\ No newline at end of file

@@ -18,6 +18,7 @@ package org.onosproject.drivers.bmv2;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
+import org.onlab.osgi.ServiceNotFoundException;
 import org.onlab.packet.ChassisId;
 import org.onosproject.bmv2.api.runtime.Bmv2DeviceAgent;
 import org.onosproject.bmv2.api.runtime.Bmv2RuntimeException;
@@ -54,12 +55,13 @@ public class Bmv2DeviceDescriptionDiscovery extends AbstractHandlerBehaviour imp
     private Bmv2Controller controller;
 
     private boolean init() {
+        try {
             controller = handler().get(Bmv2Controller.class);
-        if (controller == null) {
-            log.warn("Failed to get a BMv2 controller");
+            return true;
+        } catch (ServiceNotFoundException e) {
+            log.warn(e.getMessage());
             return false;
         }
-        return true;
     }
 
     @Override

@@ -19,6 +19,7 @@ package org.onosproject.drivers.bmv2;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import org.apache.commons.lang3.tuple.Pair;
+import org.onlab.osgi.ServiceNotFoundException;
 import org.onosproject.bmv2.api.context.Bmv2Configuration;
 import org.onosproject.bmv2.api.context.Bmv2DeviceContext;
 import org.onosproject.bmv2.api.context.Bmv2FlowRuleTranslator;
@@ -70,22 +71,15 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
     private Bmv2DeviceContextService contextService;
 
     private boolean init() {
+        try {
             controller = handler().get(Bmv2Controller.class);
             tableEntryService = handler().get(Bmv2TableEntryService.class);
             contextService = handler().get(Bmv2DeviceContextService.class);
-        if (controller == null) {
-            log.warn("Failed to get a BMv2 controller");
-            return false;
-        }
-        if (tableEntryService == null) {
-            log.warn("Failed to get a BMv2 table entry service");
-            return false;
-        }
-        if (contextService == null) {
-            log.warn("Failed to get a BMv2 device context service");
+            return true;
+        } catch (ServiceNotFoundException e) {
+            log.warn(e.getMessage());
             return false;
         }
-        return true;
     }
 
     @Override
@@ -140,9 +134,14 @@ public class Bmv2FlowRuleProgrammable extends AbstractHandlerBehaviour implement
                     Bmv2FlowRuleWrapper frWrapper = tableEntryService.lookup(entryRef);
 
                     if (frWrapper == null) {
-                        log.warn("missing reference from table entry service, BUG? " +
+                        log.debug("Missing reference from table entry service. Deleting it. BUG? " +
                                          "deviceId={}, tableName={}, matchKey={}",
                                  deviceId, table.name(), entryRef.matchKey());
+                        try {
+                            doRemove(deviceAgent, table.name(), parsedEntry.entryId(), parsedEntry.matchKey());
+                        } catch (Bmv2RuntimeException e) {
+                            log.warn("Unable to remove inconsistent flow rule: {}", e.explain());
+                        }
                         continue; // next entry
                     }
 

@@ -16,6 +16,7 @@
 
 package org.onosproject.drivers.bmv2;
 
+import org.onlab.osgi.ServiceNotFoundException;
 import org.onlab.util.ImmutableByteSequence;
 import org.onosproject.bmv2.api.runtime.Bmv2DeviceAgent;
 import org.onosproject.bmv2.api.runtime.Bmv2RuntimeException;
@@ -78,9 +79,11 @@ public class Bmv2PacketProgrammable extends AbstractHandlerBehaviour implements
 
         DeviceId deviceId = handler().data().deviceId();
 
-        Bmv2Controller controller = handler().get(Bmv2Controller.class);
-        if (controller == null) {
-            log.error("Failed to get BMv2 controller");
+        Bmv2Controller controller;
+        try {
+            controller = handler().get(Bmv2Controller.class);
+        } catch (ServiceNotFoundException e) {
+            log.warn(e.getMessage());
             return;
         }
 

@@ -189,17 +189,18 @@ public class Bmv2DeviceProvider extends AbstractDeviceProvider {
                                 (!Objects.equals(thisDescription, lastDescription) ||
                                         !Objects.equals(thisDescription.annotations(), lastDescription.annotations()));
                 if (descriptionChanged || !deviceService.isAvailable(did)) {
-                    if (contextService.getContext(did) == null) {
+                    if (deviceService.getDevice(did) == null) {
                         // Device is a first timer.
                         log.info("Setting DEFAULT context for {}", did);
+                        // It is important to do this before connecting the device so other
+                        // services won't find a null context.
                         contextService.setContext(did, contextService.defaultContext());
-                    } else {
+                    }
                     resetDeviceState(did);
                     initPortCounters(did);
                     providerService.deviceConnected(did, thisDescription);
                     updatePortsAndStats(did);
                 }
-                }
                 return thisDescription;
             } else {
                 log.warn("Unable to get device description for {}", did);
@@ -272,7 +273,7 @@ public class Bmv2DeviceProvider extends AbstractDeviceProvider {
         if (deviceService.isAvailable(did)) {
             providerService.deviceDisconnected(did);
         }
-        activeDevices.put(did, null);
+        activeDevices.remove(did);
     }
 
     /**
@@ -333,7 +334,7 @@ public class Bmv2DeviceProvider extends AbstractDeviceProvider {
     }
 
     /**
-     * Task that periodically trigger device probes to check for device status and update port informations.
+     * Task that periodically trigger device probes to check for device status and update port information.
      */
     private class DevicePoller implements TimerTask {