CORD-48 Fixing filtering rule bug in multi-instance operation.

Change-Id: Ie91b2efcebff1bdb14357b6720ff8214a712cdc2

CORD-48 Fixing filtering rule bug in multi-instance operation.
Change-Id: Ie91b2efcebff1bdb14357b6720ff8214a712cdc2
Saurav Das · Gerrit Code Review
Commit 837e0bb9b082073b94ce152869d06cc829cef12a 837e0bb9 1 parent dc5d8bd7
Showing 5 changed files with 54 additions and 28 deletions
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java
View file @837e0bb
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java
View file @837e0bb
@@ -35,7 +35,8 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import java.nio.ByteBuffer;
-import java.util.List;
+import java.util.Set;
+
 import static com.google.common.base.Preconditions.checkNotNull;
 public class ArpHandler {
@@ -112,7 +113,7 @@ public class ArpHandler {
     private boolean isArpReqForRouter(DeviceId deviceId, ARP arpRequest) {
-        List<Ip4Address> gatewayIpAddresses = config.getPortIPs(deviceId);
+        Set<Ip4Address> gatewayIpAddresses = config.getPortIPs(deviceId);
         if (gatewayIpAddresses != null) {
             Ip4Address targetProtocolAddress = Ip4Address.valueOf(arpRequest
                     .getTargetProtocolAddress());
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
View file @837e0bb
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
View file @837e0bb
@@ -312,14 +312,14 @@ public class DeviceConfiguration implements DeviceProperties {
      * on those ports.
      *
      * @param deviceId device identifier
-     * @return list of ip addresses configured on the ports or null if not found
+     * @return immutable set of ip addresses configured on the ports or null if not found
      */
-    public List<Ip4Address> getPortIPs(DeviceId deviceId) {
+    public Set<Ip4Address> getPortIPs(DeviceId deviceId) {
         SegmentRouterInfo srinfo = deviceConfigMap.get(deviceId);
         if (srinfo != null) {
             log.trace("getSubnetGatewayIps for device{} is {}", deviceId,
                       srinfo.gatewayIps.values());
-            return new ArrayList<>(srinfo.gatewayIps.values());
+            return ImmutableSet.copyOf(srinfo.gatewayIps.values());
         }
         return null;
     }
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
View file @837e0bb
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
View file @837e0bb
@@ -32,7 +32,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import java.nio.ByteBuffer;
-import java.util.List;
+import java.util.Set;
 import static com.google.common.base.Preconditions.checkNotNull;
@@ -70,7 +70,7 @@ public class IcmpHandler {
         DeviceId deviceId = connectPoint.deviceId();
         Ip4Address destinationAddress =
                 Ip4Address.valueOf(ipv4.getDestinationAddress());
-        List<Ip4Address> gatewayIpAddresses = config.getPortIPs(deviceId);
+        Set<Ip4Address> gatewayIpAddresses = config.getPortIPs(deviceId);
         Ip4Address routerIp = config.getRouterIp(deviceId);
         IpPrefix routerIpPrefix = IpPrefix.valueOf(routerIp, IpPrefix.MAX_INET_MASK_LENGTH);
         Ip4Address routerIpAddress = routerIpPrefix.getIp4Prefix().address();
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
View file @837e0bb
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
View file @837e0bb
@@ -45,6 +45,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
@@ -361,6 +362,10 @@ public class RoutingRulePopulator {
      * dstMac corresponding to the router's MAC address. For those pipelines
      * that need to internally assign vlans to untagged packets, this method
      * provides per-subnet vlan-ids as metadata.
+     * <p>
+     * Note that the vlan assignment is only done by the master-instance for a switch.
+     * However we send the filtering objective from slave-instances as well, so
+     * that drivers can obtain other information (like Router MAC and IP).
      *
      * @param deviceId  the switch dpid for the router
      */
@@ -373,16 +378,16 @@ public class RoutingRulePopulator {
                 VlanId assignedVlan = (portSubnet == null)
                         ? VlanId.vlanId(SegmentRoutingManager.ASSIGNED_VLAN_NO_SUBNET)
                         : srManager.getSubnetAssignedVlanId(deviceId, portSubnet);
-                TrafficTreatment tt = DefaultTrafficTreatment.builder()
-                        .pushVlan().setVlanId(assignedVlan).build();
                 FilteringObjective.Builder fob = DefaultFilteringObjective.builder();
                 fob.withKey(Criteria.matchInPort(port.number()))
                 .addCondition(Criteria.matchEthDst(config.getDeviceMac(deviceId)))
-                .addCondition(Criteria.matchVlanId(VlanId.NONE))
+                .addCondition(Criteria.matchVlanId(VlanId.NONE));
-                .setMeta(tt)
+                // vlan assignment is valid only if this instance is master
-                .addCondition(Criteria.matchIPDst(IpPrefix.valueOf(
+                if (srManager.mastershipService.isLocalMaster(deviceId)) {
-                                                      config.getRouterIp(deviceId),
+                    TrafficTreatment tt = DefaultTrafficTreatment.builder()
-                                                      IpPrefix.MAX_INET_MASK_LENGTH)));
+                            .pushVlan().setVlanId(assignedVlan).build();
+                    fob.setMeta(tt);
+                }
                 fob.permit().fromApp(srManager.appId);
                 srManager.flowObjectiveService.
                 filter(deviceId, fob.add(new SRObjectiveContext(deviceId,
@@ -393,16 +398,22 @@ public class RoutingRulePopulator {
     /**
      * Creates a forwarding objective to punt all IP packets, destined to the
-     * router's port IP addresses, to the controller. Note that it the input
+     * router's port IP addresses, to the controller. Note that the input
      * port should not be matched on, as these packets can come from any input.
+     * Furthermore, these are applied only by the master instance.
      *
      * @param deviceId the switch dpid for the router
      */
     public void populateRouterIpPunts(DeviceId deviceId) {
+        if (!srManager.mastershipService.isLocalMaster(deviceId)) {
+            log.debug("Not installing port-IP punts - not the master for dev:{} ",
+                      deviceId);
+            return;
+        }
         ForwardingObjective.Builder puntIp = DefaultForwardingObjective.builder();
-
+        Set<Ip4Address> allIps = new HashSet<Ip4Address>(config.getPortIPs(deviceId));
-        List<Ip4Address> gws = config.getPortIPs(deviceId);
+        allIps.add(config.getRouterIp(deviceId));
-        for (Ip4Address ipaddr : gws) {
+        for (Ip4Address ipaddr : allIps) {
             TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
             TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
             selector.matchEthType(Ethernet.TYPE_IPV4);
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
View file @837e0bb
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
View file @837e0bb
@@ -533,6 +533,8 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                             event.type() == DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED ||
                             event.type() == DeviceEvent.Type.DEVICE_UPDATED) {
                         if (deviceService.isAvailable(((Device) event.subject()).id())) {
+                            log.info("Processing device event {} for available device {}",
+                                     event.type(), ((Device) event.subject()).id());
                             processDeviceAdded((Device) event.subject());
                         }
                     } else if (event.type() == DeviceEvent.Type.PORT_REMOVED) {
@@ -594,11 +596,12 @@ public class SegmentRoutingManager implements SegmentRoutingService {
     private void processDeviceAdded(Device device) {
         log.debug("A new device with ID {} was added", device.id());
-        //Irrespective whether the local is a MASTER or not for this device,
+        // Irrespective of whether the local is a MASTER or not for this device,
-        //create group handler instance and push default TTP flow rules.
+        // we need to create a SR-group-handler instance. This is because in a
-        //Because in a multi-instance setup, instances can initiate
+        // multi-instance setup, any instance can initiate forwarding/next-objectives
-        //groups for any devices. Also the default TTP rules are needed
+        // for any switch (even if this instance is a SLAVE or not even connected
-        //to be pushed before inserting any IP table entries for any device
+        // to the switch). To handle this, a default-group-handler instance is necessary
+        // per switch.
         DefaultGroupHandler groupHandler = DefaultGroupHandler.
                 createGroupHandler(device.id(),
                                    appId,
@@ -608,6 +611,11 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                                    nsNextObjStore,
                                    subnetNextObjStore);
         groupHandlerMap.put(device.id(), groupHandler);
+
+        // Also, in some cases, drivers may need extra
+        // information to process rules (eg. Router IP/MAC); and so, we send
+        // port addressing rules to the driver as well irrespective of whether
+        // this instance is the master or not.
         defaultRoutingHandler.populatePortAddressingRules(device.id());
         if (mastershipService.isLocalMaster(device.id())) {
@@ -646,11 +654,12 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                                               tunnelHandler, policyStore);
             for (Device device : deviceService.getDevices()) {
-                //Irrespective whether the local is a MASTER or not for this device,
+                // Irrespective of whether the local is a MASTER or not for this device,
-                //create group handler instance and push default TTP flow rules.
+                // we need to create a SR-group-handler instance. This is because in a
-                //Because in a multi-instance setup, instances can initiate
+                // multi-instance setup, any instance can initiate forwarding/next-objectives
-                //groups for any devices. Also the default TTP rules are needed
+                // for any switch (even if this instance is a SLAVE or not even connected
-                //to be pushed before inserting any IP table entries for any device
+                // to the switch). To handle this, a default-group-handler instance is necessary
+                // per switch.
                 DefaultGroupHandler groupHandler = DefaultGroupHandler
                         .createGroupHandler(device.id(), appId,
                                             deviceConfiguration, linkService,
@@ -658,6 +667,11 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                                             nsNextObjStore,
                                             subnetNextObjStore);
                 groupHandlerMap.put(device.id(), groupHandler);
+
+                // Also, in some cases, drivers may need extra
+                // information to process rules (eg. Router IP/MAC); and so, we send
+                // port addressing rules to the driver as well, irrespective of whether
+                // this instance is the master or not.
                 defaultRoutingHandler.populatePortAddressingRules(device.id());
                 if (mastershipService.isLocalMaster(device.id())) {