Reworked the dependency on default drivers. Driver manager must NOT depend on these.

Instead, it is ok for the flow manager to have a dependency on the drivers and go fully active/dormant when the default drivers arrive/depart. Removed inclusion of the onos-drivers bundle as part of the onos-openflow app as this caused an unwanted dependency. Change-Id: I614290277d1621c8243c0c19e5d79273f2168016

Reworked the dependency on default drivers. Driver manager must NOT depend on these.
Instead, it is ok for the flow manager to have a dependency on the drivers and go fully active/dormant when the default drivers arrive/depart. Removed inclusion of the onos-drivers bundle as part of the onos-openflow app as this caused an unwanted dependency. Change-Id: I614290277d1621c8243c0c19e5d79273f2168016
Thomas Vachuska · Gerrit Code Review
Commit 866b46a5bee9ee4a26413f9bd21a665a7eed9886 866b46a5 1 parent 3ccb9eb1
Showing 6 changed files with 61 additions and 35 deletions
core/api/src/main/java/org/onosproject/net/driver/DefaultDriverProviderService.java
core/net/src/main/java/org/onosproject/net/driver/impl/DriverManager.java
core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
core/net/src/main/java/org/onosproject/net/packet/impl/PacketManager.java
drivers/src/main/java/org/onosproject/driver/pipeline/DefaultDrivers.java
providers/openflow/app/features.xml
--- a/core/api/src/main/java/org/onosproject/net/driver/DefaultDriverProviderService.java
View file @866b46a
+++ b/core/api/src/main/java/org/onosproject/net/driver/DefaultDriverProviderService.java
View file @866b46a
@@ -17,7 +17,7 @@
 package org.onosproject.net.driver;
 /**
- * Service capable of providing a set of default drivers.
+ * Service representing availability of default drivers.
  */
-public interface DefaultDriverProviderService extends DriverProvider {
+public interface DefaultDriverProviderService {
 }
--- a/core/net/src/main/java/org/onosproject/net/driver/impl/DriverManager.java
View file @866b46a
+++ b/core/net/src/main/java/org/onosproject/net/driver/impl/DriverManager.java
View file @866b46a
@@ -31,7 +31,6 @@ import org.onosproject.net.driver.Behaviour;
 import org.onosproject.net.driver.DefaultDriverData;
 import org.onosproject.net.driver.DefaultDriverHandler;
 import org.onosproject.net.driver.DefaultDriverProvider;
-import org.onosproject.net.driver.DefaultDriverProviderService;
 import org.onosproject.net.driver.Driver;
 import org.onosproject.net.driver.DriverAdminService;
 import org.onosproject.net.driver.DriverHandler;
@@ -63,21 +62,16 @@ public class DriverManager extends DefaultDriverProvider implements DriverAdminS
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected DeviceService deviceService;
-    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    protected DefaultDriverProviderService defaultDriverService;
-
     private Set<DriverProvider> providers = Sets.newConcurrentHashSet();
     private Map<String, Driver> driverByKey = Maps.newConcurrentMap();
     @Activate
     protected void activate() {
-        registerProvider(defaultDriverService);
         log.info("Started");
     }
     @Deactivate
     protected void deactivate() {
-        unregisterProvider(defaultDriverService);
         log.info("Stopped");
     }
--- a/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
View file @866b46a
+++ b/core/net/src/main/java/org/onosproject/net/flowobjective/impl/FlowObjectiveManager.java
View file @866b46a
@@ -27,7 +27,6 @@ import org.onlab.osgi.DefaultServiceDirectory;
 import org.onlab.osgi.ServiceDirectory;
 import org.onlab.util.ItemNotFoundException;
 import org.onosproject.cluster.ClusterService;
-import org.onosproject.cluster.NodeId;
 import org.onosproject.mastership.MastershipEvent;
 import org.onosproject.mastership.MastershipListener;
 import org.onosproject.mastership.MastershipService;
@@ -37,6 +36,7 @@ import org.onosproject.net.behaviour.PipelinerContext;
 import org.onosproject.net.device.DeviceEvent;
 import org.onosproject.net.device.DeviceListener;
 import org.onosproject.net.device.DeviceService;
+import org.onosproject.net.driver.DefaultDriverProviderService;
 import org.onosproject.net.driver.DriverHandler;
 import org.onosproject.net.driver.DriverService;
 import org.onosproject.net.flow.FlowRuleService;
@@ -56,8 +56,8 @@ import org.slf4j.LoggerFactory;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
+import static java.util.concurrent.Executors.newFixedThreadPool;
 import static org.onlab.util.Tools.groupedThreads;
 /**
@@ -96,6 +96,12 @@ public class FlowObjectiveManager implements FlowObjectiveService {
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     protected FlowObjectiveStore flowObjectiveStore;
+    // Note: This must remain an optional dependency to allow re-install of default drivers.
+    // Note: For now disabled until we can move to OPTIONAL_UNARY dependency
+    // @Reference(cardinality = ReferenceCardinality.OPTIONAL_UNARY, policy = ReferencePolicy.DYNAMIC)
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    protected DefaultDriverProviderService defaultDriverService;
+
     private final FlowObjectiveStoreDelegate delegate = new InternalStoreDelegate();
     private final Map<DeviceId, DriverHandler> driverHandlers = Maps.newConcurrentMap();
@@ -107,20 +113,14 @@ public class FlowObjectiveManager implements FlowObjectiveService {
     protected ServiceDirectory serviceDirectory = new DefaultServiceDirectory();
-    private NodeId localNode;
+    private Map<Integer, Set<PendingNext>> pendingForwards = Maps.newConcurrentMap();
-
-    private Map<Integer, Set<PendingNext>> pendingForwards =
-            Maps.newConcurrentMap();
     private ExecutorService executorService;
     @Activate
     protected void activate() {
-        executorService = Executors.newFixedThreadPool(
+        executorService = newFixedThreadPool(4, groupedThreads("onos/objective-installer", "%d"));
-                4, groupedThreads("onos/objective-installer", "%d"));
-
         flowObjectiveStore.setDelegate(delegate);
-        localNode = clusterService.getLocalNode().id();
         mastershipService.addListener(mastershipListener);
         deviceService.addListener(deviceListener);
         deviceService.getDevices().forEach(device -> setupPipelineHandler(device.id()));
@@ -132,10 +132,39 @@ public class FlowObjectiveManager implements FlowObjectiveService {
         flowObjectiveStore.unsetDelegate(delegate);
         mastershipService.removeListener(mastershipListener);
         deviceService.removeListener(deviceListener);
+        executorService.shutdown();
+        pipeliners.clear();
+        driverHandlers.clear();
         log.info("Stopped");
     }
     /**
+     * Hook for binding the optional default driver providers.
+     *
+     * @param service arriving default driver provider service
+     */
+    // Note: For now disabled until we can move to OPTIONAL_UNARY dependency
+    protected void xbindDefaultDriverService(DefaultDriverProviderService service) {
+        log.info("Detected default drivers... going active");
+        defaultDriverService = service;
+        deviceService.getDevices().forEach(device -> setupPipelineHandler(device.id()));
+    }
+
+    /**
+     * Hook for unbinding the optional default driver providers.
+     *
+     * @param service departing default driver provider service
+     */
+    // Note: For now disabled until we can move to OPTIONAL_UNARY dependency
+    protected void xunbindDefaultDriverService(DefaultDriverProviderService service) {
+        log.info("Lost default drivers... going dormant");
+        defaultDriverService = null;
+        pipeliners.clear();
+        driverHandlers.clear();
+    }
+
+
+    /**
      * Task that passes the flow objective down to the driver. The task will
      * make a few attempts to find the appropriate driver, then eventually give
      * up and report an error if no suitable driver could be found.
@@ -187,13 +216,10 @@ public class FlowObjectiveManager implements FlowObjectiveService {
     }
     @Override
-    public void forward(DeviceId deviceId,
+    public void forward(DeviceId deviceId, ForwardingObjective forwardingObjective) {
-                                   ForwardingObjective forwardingObjective) {
-
         if (queueObjective(deviceId, forwardingObjective)) {
             return;
         }
-
         executorService.submit(new ObjectiveInstaller(deviceId, forwardingObjective));
     }
@@ -212,7 +238,7 @@ public class FlowObjectiveManager implements FlowObjectiveService {
                 flowObjectiveStore.getNextGroup(fwd.nextId()) == null) {
             log.trace("Queuing forwarding objective for nextId {}", fwd.nextId());
             if (pendingForwards.putIfAbsent(fwd.nextId(),
-                                Sets.newHashSet(new PendingNext(deviceId, fwd))) != null) {
+                                            Sets.newHashSet(new PendingNext(deviceId, fwd))) != null) {
                 Set<PendingNext> pending = pendingForwards.get(fwd.nextId());
                 pending.add(new PendingNext(deviceId, fwd));
             }
@@ -228,6 +254,11 @@ public class FlowObjectiveManager implements FlowObjectiveService {
     }
     private void setupPipelineHandler(DeviceId deviceId) {
+        if (defaultDriverService == null) {
+            // We're not ready to go to work yet.
+            return;
+        }
+
         // Attempt to lookup the handler in the cache
         DriverHandler handler = driverHandlers.get(deviceId);
         if (handler == null) {
--- a/core/net/src/main/java/org/onosproject/net/packet/impl/PacketManager.java
View file @866b46a
+++ b/core/net/src/main/java/org/onosproject/net/packet/impl/PacketManager.java
View file @866b46a
@@ -78,9 +78,6 @@ public class PacketManager
     private CoreService coreService;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
-    private FlowObjectiveService objectiveService;
-
-    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     private DeviceService deviceService;
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
@@ -89,6 +86,9 @@ public class PacketManager
     @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
     private PacketStore store;
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    private FlowObjectiveService objectiveService;
+
     private final DeviceListener deviceListener = new InternalDeviceListener();
     private final Map<Integer, PacketProcessor> processors = new ConcurrentHashMap<>();
@@ -100,6 +100,7 @@ public class PacketManager
         appId = coreService.getAppId(CoreService.CORE_APP_NAME);
         store.setDelegate(delegate);
         deviceService.addListener(deviceListener);
+        // TODO: Should we request packets for all existing devices? I believe we should.
         log.info("Started");
     }
@@ -250,8 +251,7 @@ public class PacketManager
     /**
      * Internal callback from the packet store.
      */
-    private class InternalStoreDelegate
+    private class InternalStoreDelegate implements PacketStoreDelegate {
-            implements PacketStoreDelegate {
         @Override
         public void notify(PacketEvent event) {
             localEmit(event.subject());
--- a/drivers/src/main/java/org/onosproject/driver/pipeline/DefaultDrivers.java
View file @866b46a
+++ b/drivers/src/main/java/org/onosproject/driver/pipeline/DefaultDrivers.java
View file @866b46a
@@ -18,9 +18,11 @@ package org.onosproject.driver.pipeline;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
+import org.apache.felix.scr.annotations.Reference;
+import org.apache.felix.scr.annotations.ReferenceCardinality;
 import org.apache.felix.scr.annotations.Service;
 import org.onosproject.net.driver.DefaultDriverProviderService;
-import org.onosproject.net.driver.Driver;
+import org.onosproject.net.driver.DriverAdminService;
 import org.onosproject.net.driver.DriverProvider;
 import org.onosproject.net.driver.XmlDriverLoader;
 import org.slf4j.Logger;
@@ -28,7 +30,6 @@ import org.slf4j.LoggerFactory;
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.Set;
 /**
  * Bootstrap for built in drivers.
@@ -43,12 +44,16 @@ public class DefaultDrivers implements DefaultDriverProviderService {
     private DriverProvider provider;
+    @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+    protected DriverAdminService driverAdminService;
+
     @Activate
     protected void activate() {
         ClassLoader classLoader = getClass().getClassLoader();
         try {
             InputStream stream = classLoader.getResourceAsStream(DRIVERS_XML);
             provider = new XmlDriverLoader(classLoader).loadDrivers(stream);
+            driverAdminService.registerProvider(provider);
         } catch (IOException e) {
             log.error("Unable to load default drivers", e);
         }
@@ -57,11 +62,8 @@ public class DefaultDrivers implements DefaultDriverProviderService {
     @Deactivate
     protected void deactivate() {
+        driverAdminService.unregisterProvider(provider);
         log.info("Stopped");
     }
-    @Override
-    public Set<Driver> getDrivers() {
-        return provider.getDrivers();
-    }
 }
--- a/providers/openflow/app/features.xml
View file @866b46a
+++ b/providers/openflow/app/features.xml
View file @866b46a
@@ -23,7 +23,6 @@
         <bundle>mvn:${project.groupId}/onos-of-api/${project.version}</bundle>
         <bundle>mvn:${project.groupId}/onos-of-drivers/${project.version}</bundle>
         <bundle>mvn:${project.groupId}/onos-of-ctl/${project.version}</bundle>
-        <bundle>mvn:${project.groupId}/onos-drivers/${project.version}</bundle>
         <bundle>mvn:${project.groupId}/onos-lldp-provider/${project.version}</bundle>
         <bundle>mvn:${project.groupId}/onos-host-provider/${project.version}</bundle>