CORD-48 Fixing filtering rule bug in multi-instance operation.
Change-Id: Ie91b2efcebff1bdb14357b6720ff8214a712cdc2
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java
index 55192a9..36563f0 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/ArpHandler.java
@@ -35,7 +35,8 @@
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 @@
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());
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
index 6b51ff3..828c51c 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DeviceConfiguration.java
@@ -312,14 +312,14 @@
* 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;
}
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
index a77ec95..b3916b0 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
@@ -32,7 +32,7 @@
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 @@
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();
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
index 0d7d4c9..d46028e 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -45,6 +45,7 @@
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 @@
* 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 @@
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))
- .setMeta(tt)
- .addCondition(Criteria.matchIPDst(IpPrefix.valueOf(
- config.getRouterIp(deviceId),
- IpPrefix.MAX_INET_MASK_LENGTH)));
+ .addCondition(Criteria.matchVlanId(VlanId.NONE));
+ // vlan assignment is valid only if this instance is master
+ if (srManager.mastershipService.isLocalMaster(deviceId)) {
+ TrafficTreatment tt = DefaultTrafficTreatment.builder()
+ .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 @@
/**
* 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();
-
- List<Ip4Address> gws = config.getPortIPs(deviceId);
- for (Ip4Address ipaddr : gws) {
+ Set<Ip4Address> allIps = new HashSet<Ip4Address>(config.getPortIPs(deviceId));
+ allIps.add(config.getRouterIp(deviceId));
+ for (Ip4Address ipaddr : allIps) {
TrafficSelector.Builder selector = DefaultTrafficSelector.builder();
TrafficTreatment.Builder treatment = DefaultTrafficTreatment.builder();
selector.matchEthType(Ethernet.TYPE_IPV4);
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 012f134..9d60b27 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -533,6 +533,8 @@
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 @@
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,
- //create group handler instance and push default TTP flow rules.
- //Because in a multi-instance setup, instances can initiate
- //groups for any devices. Also the default TTP rules are needed
- //to be pushed before inserting any IP table entries for any device
+ // Irrespective of whether the local is a MASTER or not for this device,
+ // we need to create a SR-group-handler instance. This is because in a
+ // multi-instance setup, any instance can initiate forwarding/next-objectives
+ // for any switch (even if this instance is a SLAVE or not even connected
+ // 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 @@
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 @@
tunnelHandler, policyStore);
for (Device device : deviceService.getDevices()) {
- //Irrespective whether the local is a MASTER or not for this device,
- //create group handler instance and push default TTP flow rules.
- //Because in a multi-instance setup, instances can initiate
- //groups for any devices. Also the default TTP rules are needed
- //to be pushed before inserting any IP table entries for any device
+ // Irrespective of whether the local is a MASTER or not for this device,
+ // we need to create a SR-group-handler instance. This is because in a
+ // multi-instance setup, any instance can initiate forwarding/next-objectives
+ // for any switch (even if this instance is a SLAVE or not even connected
+ // 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 @@
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())) {