Minor cleanups and renaming in vRouter

Change-Id: I9f5334a0d428f77789880bb9caef5a5d12084f1c

Minor cleanups and renaming in vRouter
Change-Id: I9f5334a0d428f77789880bb9caef5a5d12084f1c
Jonathan Hart · Gerrit Code Review
Commit f8035d3817b728d287d3cc3ad8b33091d7396974 f8035d38 1 parent d177f8f5
Showing 3 changed files with 23 additions and 37 deletions
apps/routing/src/main/java/org/onosproject/routing/impl/ControlPlaneRedirectManager.java
apps/routing/src/main/java/org/onosproject/routing/impl/SingleSwitchFibInstaller.java
drivers/default/src/main/java/org/onosproject/driver/pipeline/SoftRouterPipeline.java
--- a/apps/routing/src/main/java/org/onosproject/routing/impl/ControlPlaneRedirectManager.java
View file @f8035d3
+++ b/apps/routing/src/main/java/org/onosproject/routing/impl/ControlPlaneRedirectManager.java
View file @f8035d3
@@ -133,10 +133,8 @@ public class ControlPlaneRedirectManager {
         hostService.addListener(hostListener);
         interfaceService.addListener(interfaceListener);
-        updateConfig(true);
+        readConfig();
-        applicationService.registerDeactivateHook(this.appId, () -> {
+        applicationService.registerDeactivateHook(this.appId, () -> provisionDevice(false));
-            this.updateConfig(false);
-        });
     }
     @Deactivate
@@ -151,10 +149,8 @@ public class ControlPlaneRedirectManager {
      * Installs or removes interface configuration
      * based on the flag used on activate or deactivate.
      *
-     * @param operation true on activate application, false on deactivate
-     *            the application
      **/
-    private void updateConfig(boolean operation) {
+    private void readConfig() {
         ApplicationId routingAppId =
                 coreService.registerApplication(RoutingService.ROUTER_APP_ID);
@@ -170,17 +166,16 @@ public class ControlPlaneRedirectManager {
         ospfEnabled = config.getOspfEnabled();
         interfaces = config.getInterfaces();
-        updateDevice(operation);
+        provisionDevice(true);
     }
     /**
      * Installs or removes interface configuration for each interface
      * based on the flag used on activate or deactivate.
      *
-     * @param operation true on activate application, false on deactivate
+     * @param install true to install flows, false to remove them
-     *            the application
      **/
-    private void updateDevice(boolean operation) {
+    private void provisionDevice(boolean install) {
         if (controlPlaneConnectPoint != null &&
                 deviceService.isAvailable(controlPlaneConnectPoint.deviceId())) {
             DeviceId deviceId = controlPlaneConnectPoint.deviceId();
@@ -188,21 +183,17 @@ public class ControlPlaneRedirectManager {
             interfaceService.getInterfaces().stream()
                     .filter(intf -> intf.connectPoint().deviceId().equals(deviceId))
                     .filter(intf -> interfaces.isEmpty() || interfaces.contains(intf.name()))
-                    .forEach(operation ? this::provisionInterface : this::removeInterface);
+                    .forEach(intf -> provisionInterface(intf, install));
             log.info("Set up interfaces on {}", controlPlaneConnectPoint.deviceId());
         }
     }
-    private void removeInterface(Interface intf) {
+    private void provisionInterface(Interface intf, boolean install) {
-        modifyBasicInterfaceForwarding(intf, false);
+        updateInterfaceForwarding(intf, install);
-        updateOspfForwarding(intf, false);
+        updateOspfForwarding(intf, install);
     }
-    private void provisionInterface(Interface intf) {
-        modifyBasicInterfaceForwarding(intf, true);
-        updateOspfForwarding(intf, true);
-    }
     /**
      * Installs or removes the basic forwarding flows for each interface
      * based on the flag used.
@@ -211,7 +202,7 @@ public class ControlPlaneRedirectManager {
      * @param install true to create an add objective, false to create a remove
      *            objective
      **/
-    private void modifyBasicInterfaceForwarding(Interface intf, boolean install) {
+    private void updateInterfaceForwarding(Interface intf, boolean install) {
         log.debug("Adding interface objectives for {}", intf);
         DeviceId deviceId = controlPlaneConnectPoint.deviceId();
@@ -361,6 +352,7 @@ public class ControlPlaneRedirectManager {
         }
         return nextId;
     }
+
     /**
      * Builds a forwarding objective from the given selector, treatment and nextId.
      *
@@ -404,7 +396,7 @@ public class ControlPlaneRedirectManager {
                 case DEVICE_AVAILABILITY_CHANGED:
                     if (deviceService.isAvailable(event.subject().id())) {
                         log.info("Device connected {}", event.subject().id());
-                        updateDevice(true);
+                        provisionDevice(true);
                     }
                     break;
                 case DEVICE_UPDATED:
@@ -431,7 +423,7 @@ public class ControlPlaneRedirectManager {
                 switch (event.type()) {
                 case CONFIG_ADDED:
                 case CONFIG_UPDATED:
-                    updateConfig(true);
+                    readConfig();
                     break;
                 case CONFIG_REGISTERED:
                 case CONFIG_UNREGISTERED:
@@ -494,7 +486,6 @@ public class ControlPlaneRedirectManager {
                 return;
             }
-            Set<Integer> nextIds = peerNextId.get(peer);
             checkState(peerNextId.get(peer) != null,
                     "Peer nextId should not be null");
             checkState(peerNextId.get(peer).size() == 2,
@@ -593,12 +584,12 @@ public class ControlPlaneRedirectManager {
      * Update the flows comparing previous event and current event.
      *
      * @param prevIntf the previous interface event
-     * @param intf the current occured update envent
+     * @param intf the current occurred update event
      **/
     private void updateInterface(Interface prevIntf, Interface intf) {
         if (!prevIntf.vlan().equals(intf.vlan()) || !prevIntf.mac().equals(intf)) {
-            removeInterface(prevIntf);
+            provisionInterface(prevIntf, false);
-            provisionInterface(intf);
+            provisionInterface(intf, true);
         } else {
             List<InterfaceIpAddress> removeIps =
                     prevIntf.ipAddressesList().stream()
@@ -609,10 +600,10 @@ public class ControlPlaneRedirectManager {
                     .filter(cur -> !prevIntf.ipAddressesList().contains(cur))
                     .collect(Collectors.toList());
             // removing flows with match parameters present in previous subject
-            modifyBasicInterfaceForwarding(new Interface(prevIntf.name(), prevIntf.connectPoint(),
+            updateInterfaceForwarding(new Interface(prevIntf.name(), prevIntf.connectPoint(),
                     removeIps, prevIntf.mac(), prevIntf.vlan()), false);
             // adding flows with match parameters present in event subject
-            modifyBasicInterfaceForwarding(new Interface(intf.name(), intf.connectPoint(),
+            updateInterfaceForwarding(new Interface(intf.name(), intf.connectPoint(),
                     addIps, intf.mac(), intf.vlan()), true);
         }
     }
@@ -626,7 +617,7 @@ public class ControlPlaneRedirectManager {
                 switch (event.type()) {
                 case INTERFACE_ADDED:
                     if (intf != null && !intf.connectPoint().equals(controlPlaneConnectPoint)) {
-                        provisionInterface(intf);
+                        provisionInterface(intf, true);
                     }
                     break;
                 case INTERFACE_UPDATED:
@@ -636,7 +627,7 @@ public class ControlPlaneRedirectManager {
                     break;
                 case INTERFACE_REMOVED:
                     if (intf != null && !intf.connectPoint().equals(controlPlaneConnectPoint)) {
-                        removeInterface(intf);
+                        provisionInterface(intf, false);
                     }
                     break;
                 default:
--- a/apps/routing/src/main/java/org/onosproject/routing/impl/SingleSwitchFibInstaller.java
View file @f8035d3
+++ b/apps/routing/src/main/java/org/onosproject/routing/impl/SingleSwitchFibInstaller.java
View file @f8035d3
@@ -178,9 +178,7 @@ public class SingleSwitchFibInstaller {
         updateConfig();
-        applicationService.registerDeactivateHook(vrouterAppId, () -> {
+        applicationService.registerDeactivateHook(vrouterAppId, () -> cleanUp());
-            this.cleanUp();
-        });
         log.info("Started");
     }
@@ -192,8 +190,6 @@ public class SingleSwitchFibInstaller {
         interfaceService.removeListener(internalInterfaceList);
         networkConfigService.removeListener(configListener);
-        //processIntfFilters(false, configService.getInterfaces()); //TODO necessary?
-
         componentConfigService.unregisterProperties(getClass(), false);
         log.info("Stopped");
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/SoftRouterPipeline.java
View file @f8035d3
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/SoftRouterPipeline.java
View file @f8035d3
@@ -261,7 +261,7 @@ public class SoftRouterPipeline extends AbstractHandlerBehaviour implements Pipe
             }
         }
-        log.debug("adding Port/VLAN/MAC filtering rules in filter table: {}/{}/{}",
+        log.debug("Modifying Port/VLAN/MAC filtering rules in filter table: {}/{}/{}",
                   p.port(), v.vlanId(), e.mac());
         TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
         TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
@@ -287,7 +287,6 @@ public class SoftRouterPipeline extends AbstractHandlerBehaviour implements Pipe
                 .fromApp(applicationId)
                 .makePermanent()
                 .forTable(FILTER_TABLE).build();
-        ops =  ops.add(rule);
         ops = install ? ops.add(rule) : ops.remove(rule);
         // apply filtering flow rules