Small cleanups for vRouter app

Change-Id: Ibee46d3b95ee76dd3547e11d046c4620b3b3306d

Small cleanups for vRouter app
Change-Id: Ibee46d3b95ee76dd3547e11d046c4620b3b3306d
Jonathan Hart · Gerrit Code Review
Commit f04b7d92497e79030499e70da1f970b82f094fc5 f04b7d92 1 parent 8bf6c44d
Showing 5 changed files with 150 additions and 129 deletions
apps/routing/src/main/java/org/onosproject/routing/impl/ControlPlaneRedirectManager.java
apps/routing/src/main/java/org/onosproject/routing/impl/SingleSwitchFibInstaller.java
apps/routing/src/test/java/org/onosproject/routing/impl/ControlPlaneRedirectManagerTest.java
apps/routing/src/test/java/org/onosproject/routing/impl/SingleSwitchFibInstallerTest.java
core/api/src/main/java/org/onosproject/net/flowobjective/DefaultObjectiveContext.java
--- a/apps/routing/src/main/java/org/onosproject/routing/impl/ControlPlaneRedirectManager.java
View file @f04b7d9
+++ b/apps/routing/src/main/java/org/onosproject/routing/impl/ControlPlaneRedirectManager.java
View file @f04b7d9
@@ -16,16 +16,8 @@
 package org.onosproject.routing.impl;
-import static com.google.common.base.Preconditions.checkState;
+import com.google.common.collect.ImmutableSortedSet;
-import static org.slf4j.LoggerFactory.getLogger;
+import com.google.common.collect.Maps;
-
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
-
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -69,8 +61,15 @@ import org.onosproject.routing.RoutingService;
 import org.onosproject.routing.config.RouterConfig;
 import org.slf4j.Logger;
-import com.google.common.collect.ImmutableSortedSet;
+import java.util.Collections;
-import com.google.common.collect.Maps;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+
+import static com.google.common.base.Preconditions.checkState;
+import static org.slf4j.LoggerFactory.getLogger;
 /**
  * Manages connectivity between peers redirecting control traffic to a routing
@@ -183,7 +182,7 @@ public class ControlPlaneRedirectManager {
         updateOspfForwarding(intf, true);
     }
     /**
-     * Install or removes the basic forwarding flows for each interface
+     * Installs or removes the basic forwarding flows for each interface
      * based on the flag used.
      *
      * @param intf the Interface on which event is received
@@ -266,9 +265,9 @@ public class ControlPlaneRedirectManager {
     }
     /**
-     * Install or removes ospf forwarding rules.
+     * Installs or removes OSPF forwarding rules.
      *
-     * @param intf the Interface on which event is received
+     * @param intf the interface on which event is received
      * @param install true to create an add objective, false to create a remove
      *            objective
      **/
@@ -293,7 +292,7 @@ public class ControlPlaneRedirectManager {
             cpNextId = modifyNextObjective(deviceId, controlPlanePort,
                                            intf.vlan(), false, install);
         }
-        log.debug("ospf flows intf:{} nextid:{}", intf, cpNextId);
+        log.debug("OSPF flows intf:{} nextid:{}", intf, cpNextId);
         flowObjectiveService.forward(controlPlaneConnectPoint.deviceId(),
                 buildForwardingObjective(toSelector, null, cpNextId, install ? ospfEnabled : install));
     }
@@ -331,7 +330,7 @@ public class ControlPlaneRedirectManager {
         nextObjBuilder.withMeta(metabuilder.build());
         nextObjBuilder.addTreatment(ttBuilder.build());
-        log.debug("Submited next objective {} in device {} for port/vlan {}/{}",
+        log.debug("Submitted next objective {} in device {} for port/vlan {}/{}",
                 nextId, deviceId, portNumber, vlanId);
         if (install) {
              flowObjectiveService.next(deviceId, nextObjBuilder.add());
@@ -385,7 +384,6 @@ public class ControlPlaneRedirectManager {
                         log.info("Device connected {}", event.subject().id());
                         updateDevice();
                     }
-
                     break;
                 case DEVICE_UPDATED:
                 case DEVICE_REMOVED:
--- a/apps/routing/src/main/java/org/onosproject/routing/impl/SingleSwitchFibInstaller.java
View file @f04b7d9
+++ b/apps/routing/src/main/java/org/onosproject/routing/impl/SingleSwitchFibInstaller.java
View file @f04b7d9
@@ -60,9 +60,8 @@ import org.onosproject.net.flowobjective.FilteringObjective;
 import org.onosproject.net.flowobjective.FlowObjectiveService;
 import org.onosproject.net.flowobjective.ForwardingObjective;
 import org.onosproject.net.flowobjective.NextObjective;
-import org.onosproject.net.flowobjective.Objective;
 import org.onosproject.net.flowobjective.ObjectiveContext;
-import org.onosproject.net.flowobjective.ObjectiveError;
+import org.onosproject.net.flowobjective.DefaultObjectiveContext;
 import org.onosproject.routing.FibEntry;
 import org.onosproject.routing.FibListener;
 import org.onosproject.routing.FibUpdate;
@@ -442,43 +441,15 @@ public class SingleSwitchFibInstaller {
     private void sendFilteringObjective(boolean install, FilteringObjective.Builder fob,
                                         Interface intf) {
-        if (install) {
-            flowObjectiveService.filter(
-                deviceId,
-                fob.add(new ObjectiveContext() {
-                    @Override
-                    public void onSuccess(Objective objective) {
-                        log.info("Successfully installed interface based "
-                                + "filtering objectives for intf {}", intf);
-                    }
-                    @Override
+        ObjectiveContext context = new DefaultObjectiveContext(
-                    public void onError(Objective objective,
+                (objective) -> log.info("Installed filter for interface {}", intf),
-                                        ObjectiveError error) {
+                (objective, error) ->
-                        log.error("Failed to install interface filters for intf {}: {}",
+                        log.error("Failed to install filter for interface {}: {}", intf, error));
-                                  intf, error);
-                        // TODO something more than just logging
-                    }
-            }));
-        } else {
-            flowObjectiveService.filter(
-                deviceId,
-                fob.remove(new ObjectiveContext() {
-                    @Override
-                    public void onSuccess(Objective objective) {
-                        log.info("Successfully removed interface based "
-                                   + "filtering objectives for intf {}", intf);
-                    }
-                    @Override
+        FilteringObjective filter = install ? fob.add(context) : fob.remove(context);
-                    public void onError(Objective objective,
+
-                                        ObjectiveError error) {
+        flowObjectiveService.filter(deviceId, filter);
-                        log.error("Failed to install interface filters for intf {}: {}",
-                                  intf, error);
-                            // TODO something more than just logging
-                    }
-            }));
-        }
     }
     private class InternalFibListener implements FibListener {
--- a/apps/routing/src/test/java/org/onosproject/routing/impl/ControlPlaneRedirectManagerTest.java
View file @f04b7d9
+++ b/apps/routing/src/test/java/org/onosproject/routing/impl/ControlPlaneRedirectManagerTest.java
View file @f04b7d9
@@ -15,19 +15,7 @@
  */
 package org.onosproject.routing.impl;
-import static org.easymock.EasyMock.createMock;
+import com.google.common.collect.Sets;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.expectLastCall;
-import static org.easymock.EasyMock.replay;
-import static org.easymock.EasyMock.verify;
-import static org.slf4j.LoggerFactory.getLogger;
-
-import java.util.ArrayList;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-
 import org.easymock.EasyMock;
 import org.junit.Before;
 import org.junit.Test;
@@ -70,23 +58,29 @@ import org.onosproject.net.flowobjective.DefaultNextObjective;
 import org.onosproject.net.flowobjective.FlowObjectiveService;
 import org.onosproject.net.flowobjective.ForwardingObjective;
 import org.onosproject.net.flowobjective.NextObjective;
-import org.onosproject.net.host.HostListener;
 import org.onosproject.net.host.HostService;
-import org.onosproject.net.host.HostServiceAdapter;
 import org.onosproject.net.host.InterfaceIpAddress;
-import org.onosproject.net.intent.AbstractIntentTest;
 import org.onosproject.routing.RoutingService;
 import org.onosproject.routing.config.RouterConfig;
-import org.slf4j.Logger;
-import com.google.common.collect.Sets;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.createNiceMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.expectLastCall;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
 /**
  * UnitTests for ControlPlaneRedirectManager.
  */
-public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
+public class ControlPlaneRedirectManagerTest {
-    private final Logger log = getLogger(getClass());
     private DeviceService deviceService;
     private FlowObjectiveService flowObjectiveService;
     private NetworkConfigService networkConfigService;
@@ -97,39 +91,34 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
     private InterfaceService interfaceService;
     private static final ApplicationId APPID = TestApplicationId.create("org.onosproject.cpredirect");
-    /**
+    private static final DeviceId DEVICE_ID = DeviceId.deviceId("of:0000000000000001");
-     * Interface Configuration.
+
-     *
+    private ConnectPoint controlPlaneConnectPoint = new ConnectPoint(DEVICE_ID,
-     **/
-    private ConnectPoint controlPlaneConnectPoint = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"),
             PortNumber.portNumber(1));
-    private static final ConnectPoint SW1_ETH1 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"),
+
+    private static final ConnectPoint SW1_ETH1 = new ConnectPoint(DEVICE_ID,
             PortNumber.portNumber(1));
-    private static final ConnectPoint SW1_ETH2 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"),
+    private static final ConnectPoint SW1_ETH2 = new ConnectPoint(DEVICE_ID,
             PortNumber.portNumber(2));
-    private static final ConnectPoint SW1_ETH3 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"),
+    private static final ConnectPoint SW1_ETH3 = new ConnectPoint(DEVICE_ID,
             PortNumber.portNumber(3));
-    protected HostService hostService;
-    private ControlPlaneRedirectManager controlPlaneRedirectManager = new ControlPlaneRedirectManager();;
+    private ControlPlaneRedirectManager controlPlaneRedirectManager = new ControlPlaneRedirectManager();
     private RouterConfig routerConfig = new TestRouterConfig();
     private NetworkConfigListener networkConfigListener;
     private DeviceListener deviceListener;
     private MastershipService mastershipService = new InternalMastershipServiceTest();
-    private HostListener hostListener;
     private InterfaceListener interfaceListener;
-    @Override
+
     @Before
     public void setUp() {
         networkConfigListener = createMock(NetworkConfigListener.class);
-        hostService = new TestHostService();
         deviceService = new TestDeviceService();
         deviceListener = createMock(DeviceListener.class);
-        hostListener = createMock(HostListener.class);
+
         interfaceListener = createMock(InterfaceListener.class);
-        hostService.addListener(hostListener);
         deviceService.addListener(deviceListener);
         setUpInterfaceService();
         interfaceService = new InternalInterfaceService();
@@ -143,7 +132,7 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
         controlPlaneRedirectManager.networkConfigService = networkConfigService;
         controlPlaneRedirectManager.interfaceService = interfaceService;
         controlPlaneRedirectManager.deviceService = deviceService;
-        controlPlaneRedirectManager.hostService = hostService;
+        controlPlaneRedirectManager.hostService = createNiceMock(HostService.class);
         controlPlaneRedirectManager.mastershipService = mastershipService;
         controlPlaneRedirectManager.activate();
         verify(flowObjectiveService);
@@ -154,7 +143,7 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
      */
     @Test
     public void testAddDevice() {
-        ConnectPoint sw1eth4 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"), PortNumber.portNumber(4));
+        ConnectPoint sw1eth4 = new ConnectPoint(DEVICE_ID, PortNumber.portNumber(4));
         Set<InterfaceIpAddress> interfaceIpAddresses4 = Sets.newHashSet();
         interfaceIpAddresses4
                 .add(new InterfaceIpAddress(IpAddress.valueOf("192.168.40.101"), IpPrefix.valueOf("192.168.40.0/24")));
@@ -173,7 +162,7 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
      */
     @Test
     public void testUpdateNetworkConfig() {
-        ConnectPoint sw1eth4 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"), PortNumber.portNumber(4));
+        ConnectPoint sw1eth4 = new ConnectPoint(DEVICE_ID, PortNumber.portNumber(4));
         Set<InterfaceIpAddress> interfaceIpAddresses4 = Sets.newHashSet();
         interfaceIpAddresses4
                 .add(new InterfaceIpAddress(IpAddress.valueOf("192.168.40.101"), IpPrefix.valueOf("192.168.40.0/24")));
@@ -194,7 +183,7 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
      */
     @Test
     public void testAddInterface() {
-        ConnectPoint sw1eth4 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"), PortNumber.portNumber(4));
+        ConnectPoint sw1eth4 = new ConnectPoint(DEVICE_ID, PortNumber.portNumber(4));
         Set<InterfaceIpAddress> interfaceIpAddresses4 = Sets.newHashSet();
         interfaceIpAddresses4
                 .add(new InterfaceIpAddress(IpAddress.valueOf("192.168.40.101"), IpPrefix.valueOf("192.168.40.0/24")));
@@ -208,14 +197,13 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
         setUpInterfaceConfiguration(sw1Eth4, true);
         replay(flowObjectiveService);
-        interfaceListener.event(new InterfaceEvent(
+        interfaceListener.event(new InterfaceEvent(InterfaceEvent.Type.INTERFACE_ADDED, sw1Eth4, 500L));
-                org.onosproject.incubator.net.intf.InterfaceEvent.Type.INTERFACE_ADDED, sw1Eth4, 500L));
         verify(flowObjectiveService);
     }
     @Test
     public void testRemoveInterface() {
-        ConnectPoint sw1eth4 = new ConnectPoint(DeviceId.deviceId("of:0000000000000001"), PortNumber.portNumber(4));
+        ConnectPoint sw1eth4 = new ConnectPoint(DEVICE_ID, PortNumber.portNumber(4));
         Set<InterfaceIpAddress> interfaceIpAddresses4 = Sets.newHashSet();
         interfaceIpAddresses4
                 .add(new InterfaceIpAddress(IpAddress.valueOf("192.168.40.101"), IpPrefix.valueOf("192.168.40.0/24")));
@@ -227,8 +215,7 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
         setUpInterfaceConfiguration(sw1Eth4, false);
         replay(flowObjectiveService);
-        interfaceListener.event(new InterfaceEvent(
+        interfaceListener.event(new InterfaceEvent(InterfaceEvent.Type.INTERFACE_REMOVED, sw1Eth4, 500L));
-                org.onosproject.incubator.net.intf.InterfaceEvent.Type.INTERFACE_REMOVED, sw1Eth4, 500L));
         verify(flowObjectiveService);
     }
@@ -407,15 +394,6 @@ public class ControlPlaneRedirectManagerTest extends AbstractIntentTest {
     }
-    private class TestHostService extends HostServiceAdapter {
-
-        @Override
-        public void addListener(HostListener listener) {
-            ControlPlaneRedirectManagerTest.this.hostListener = listener;
-        }
-
-    }
-
     private class TestRouterConfig extends RouterConfig {
         @Override
--- a/apps/routing/src/test/java/org/onosproject/routing/impl/SingleSwitchFibInstallerTest.java
View file @f04b7d9
+++ b/apps/routing/src/test/java/org/onosproject/routing/impl/SingleSwitchFibInstallerTest.java
View file @f04b7d9
@@ -15,20 +15,8 @@
  */
 package org.onosproject.routing.impl;
-import static org.easymock.EasyMock.anyObject;
+import com.google.common.collect.ImmutableSet;
-import  static org.easymock.EasyMock.createMock;
+import com.google.common.collect.Sets;
-import static org.easymock.EasyMock.expect;
-import static org.easymock.EasyMock.replay;
-import static org.easymock.EasyMock.reset;
-import static org.easymock.EasyMock.verify;
-
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Dictionary;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
-
 import org.easymock.EasyMock;
 import org.junit.Before;
 import org.junit.Test;
@@ -40,11 +28,10 @@ import org.onlab.packet.IpPrefix;
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.VlanId;
 import org.onosproject.TestApplicationId;
+import org.onosproject.cfg.ComponentConfigService;
 import org.onosproject.core.ApplicationId;
 import org.onosproject.core.CoreService;
 import org.onosproject.core.CoreServiceAdapter;
-import org.osgi.service.component.ComponentContext;
-import org.onosproject.cfg.ComponentConfigService;
 import org.onosproject.incubator.net.intf.Interface;
 import org.onosproject.incubator.net.intf.InterfaceListener;
 import org.onosproject.incubator.net.intf.InterfaceService;
@@ -73,8 +60,21 @@ import org.onosproject.routing.FibUpdate;
 import org.onosproject.routing.RoutingService;
 import org.onosproject.routing.RoutingServiceAdapter;
 import org.onosproject.routing.config.RouterConfig;
-import com.google.common.collect.ImmutableSet;
+import org.osgi.service.component.ComponentContext;
-import com.google.common.collect.Sets;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Dictionary;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.reset;
+import static org.easymock.EasyMock.verify;
 /**
  * Unit tests for SingleSwitchFibInstaller.
@@ -198,7 +198,7 @@ public class SingleSwitchFibInstallerTest extends AbstractIntentTest {
     /*
      * Sets up NetworkConfigService.
-    */
+     */
     private void setUpNetworkConfigService() {
         ApplicationId routerAppId = coreService.registerApplication(RoutingService.ROUTER_APP_ID);
         expect(networkConfigService.getConfig(routerAppId, RoutingService.ROUTER_CONFIG_CLASS)).
@@ -423,13 +423,12 @@ public class SingleSwitchFibInstallerTest extends AbstractIntentTest {
         verify(flowObjectiveService);
     }
-     /**
+    /**
      * Tests deleting a FIB entry.
      *
      * We verify that the flowObjectiveService records the correct state and that the
      * correct flow is withdrawn from the flowObjectiveService.
      */
-
     @Test
     public void testFibDelete() {
         // Firstly add a route
--- a/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultObjectiveContext.java 0 → 100644
View file @f04b7d9
+++ b/core/api/src/main/java/org/onosproject/net/flowobjective/DefaultObjectiveContext.java 0 → 100644
View file @f04b7d9
+/*
+ * Copyright 2016 Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.net.flowobjective;
+
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
+
+/**
+ * Implementation of objective context that delegates calls to provided
+ * consumers.
+ */
+public class DefaultObjectiveContext implements ObjectiveContext {
+
+    private final Consumer<Objective> onSuccess;
+    private final BiConsumer<Objective, ObjectiveError> onError;
+
+    /**
+     * Creates a new objective context using the given success and error
+     * consumers.
+     *
+     * @param onSuccess consumer to be called on success
+     * @param onError consumer to be called on error
+     */
+    public DefaultObjectiveContext(Consumer<Objective> onSuccess,
+                                   BiConsumer<Objective, ObjectiveError> onError) {
+        this.onSuccess = onSuccess;
+        this.onError = onError;
+    }
+
+    /**
+     * Creates a new objective context using the given success consumer.
+     *
+     * @param onSuccess consumer to be called on success
+     */
+    public DefaultObjectiveContext(Consumer<Objective> onSuccess) {
+        this(onSuccess, null);
+    }
+
+    /**
+     * Creates a new objective context using the given error consumer.
+     *
+     * @param onError consumer to be called on error
+     */
+    public DefaultObjectiveContext(BiConsumer<Objective, ObjectiveError> onError) {
+        this(null, onError);
+    }
+
+    @Override
+    public void onSuccess(Objective objective) {
+        if (onSuccess != null) {
+            onSuccess.accept(objective);
+        }
+    }
+
+    @Override
+    public void onError(Objective objective, ObjectiveError error) {
+        if (onError != null) {
+            onError.accept(objective, error);
+        }
+    }
+}