In this commit: CORD-799
Bug fix for host-update to not remove and add the same IP addr
NPE fix in ofdpa3pipeline
Removing unused code in ofdpa2pipeline
Ability to add or revoke port filters for port-updates
Retry filters retry for a longer time
Bug fix for suppress ports to not suppress filters
Filters now sent only by master instance
Removing the MPLS BOS=0 rules for now until inconsitent hardware behavior is fixed
Change-Id: I8b4ee4af6de263531e0696af86e65f1c502f5f85
diff --git a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index 4893684..bee1150 100644
--- a/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -27,6 +27,7 @@
import org.onosproject.net.Device;
import org.onosproject.net.DeviceId;
import org.onosproject.net.Link;
+import org.onosproject.net.PortNumber;
import org.onosproject.segmentrouting.config.DeviceConfigNotFoundException;
import org.onosproject.segmentrouting.config.DeviceConfiguration;
import org.slf4j.Logger;
@@ -52,8 +53,9 @@
* routing rule population.
*/
public class DefaultRoutingHandler {
- private static final int MAX_CONSTANT_RETRY_ATTEMPTS = 4;
- private static final int RETRY_INTERVAL_MS = 500;
+ private static final int MAX_CONSTANT_RETRY_ATTEMPTS = 5;
+ private static final int RETRY_INTERVAL_MS = 250;
+ private static final int RETRY_INTERVAL_SCALE = 1;
private static final String ECMPSPG_MISSING = "ECMP shortest path graph not found";
private static Logger log = LoggerFactory.getLogger(DefaultRoutingHandler.class);
@@ -634,7 +636,9 @@
}
/**
- * Populates filtering rules for permitting Router DstMac and VLAN.
+ * Populates filtering rules for port, and punting rules
+ * for gateway IPs, loopback IPs and arp/ndp traffic.
+ * Should only be called by the master instance for this device/port.
*
* @param deviceId Switch ID to set the rules
*/
@@ -655,6 +659,28 @@
}
/**
+ * Populates filtering rules for a port that has been enabled.
+ * Should only be called by the master instance for this device/port.
+ *
+ * @param devId device identifier
+ * @param portnum port identifier
+ */
+ public void populateSinglePortFilteringRules(DeviceId devId, PortNumber portnum) {
+ rulePopulator.populateSinglePortFilters(devId, portnum);
+ }
+
+ /**
+ * Revokes filtering rules for a port that has been disabled.
+ * Should only be called by the master instance for this device/port.
+ *
+ * @param devId device identifier
+ * @param portnum port identifier
+ */
+ public void revokeSinglePortFilteringRules(DeviceId devId, PortNumber portnum) {
+ rulePopulator.revokeSinglePortFilters(devId, portnum);
+ }
+
+ /**
* Start the flow rule population process if it was never started. The
* process finishes successfully when all flow rules are set and stops with
* ABORTED status when any groups required for flows is not set yet.
@@ -743,21 +769,20 @@
/**
* Utility class used to temporarily store information about the ports on a
* device processed for filtering objectives.
- *
*/
public final class PortFilterInfo {
- int disabledPorts = 0, suppressedPorts = 0, filteredPorts = 0;
+ int disabledPorts = 0, errorPorts = 0, filteredPorts = 0;
- public PortFilterInfo(int disabledPorts, int suppressedPorts,
+ public PortFilterInfo(int disabledPorts, int errorPorts,
int filteredPorts) {
this.disabledPorts = disabledPorts;
this.filteredPorts = filteredPorts;
- this.suppressedPorts = suppressedPorts;
+ this.errorPorts = errorPorts;
}
@Override
public int hashCode() {
- return Objects.hash(disabledPorts, filteredPorts, suppressedPorts);
+ return Objects.hash(disabledPorts, filteredPorts, errorPorts);
}
@Override
@@ -771,14 +796,14 @@
PortFilterInfo other = (PortFilterInfo) obj;
return ((disabledPorts == other.disabledPorts) &&
(filteredPorts == other.filteredPorts) &&
- (suppressedPorts == other.suppressedPorts));
+ (errorPorts == other.errorPorts));
}
@Override
public String toString() {
MoreObjects.ToStringHelper helper = toStringHelper(this)
.add("disabledPorts", disabledPorts)
- .add("suppressedPorts", suppressedPorts)
+ .add("errorPorts", errorPorts)
.add("filteredPorts", filteredPorts);
return helper.toString();
}
@@ -809,7 +834,10 @@
log.debug("dev:{} prevRun:{} thisRun:{} sameResult:{}", devId, prevRun,
thisRun, sameResult);
if (thisRun == null || !sameResult || (sameResult && --constantAttempts > 0)) {
- executorService.schedule(this, RETRY_INTERVAL_MS, TimeUnit.MILLISECONDS);
+ // exponentially increasing intervals for retries
+ executorService.schedule(this,
+ RETRY_INTERVAL_MS * (int) Math.pow(counter, RETRY_INTERVAL_SCALE),
+ TimeUnit.MILLISECONDS);
if (!sameResult) {
constantAttempts = MAX_CONSTANT_RETRY_ATTEMPTS; //reset
}
diff --git a/src/main/java/org/onosproject/segmentrouting/HostHandler.java b/src/main/java/org/onosproject/segmentrouting/HostHandler.java
index cf49c52..62ab1ce 100644
--- a/src/main/java/org/onosproject/segmentrouting/HostHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/HostHandler.java
@@ -39,6 +39,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.Sets;
import java.util.Set;
/**
@@ -83,7 +84,7 @@
DeviceId deviceId = location.deviceId();
PortNumber port = location.port();
Set<IpAddress> ips = host.ipAddresses();
- log.debug("Host {}/{} is added at {}:{}", mac, vlanId, deviceId, port);
+ log.info("Host {}/{} is added at {}:{}", mac, vlanId, deviceId, port);
if (accepted(host)) {
// Populate bridging table entry
@@ -122,7 +123,7 @@
DeviceId deviceId = location.deviceId();
PortNumber port = location.port();
Set<IpAddress> ips = host.ipAddresses();
- log.debug("Host {}/{} is removed from {}:{}", mac, vlanId, deviceId, port);
+ log.info("Host {}/{} is removed from {}:{}", mac, vlanId, deviceId, port);
if (accepted(host)) {
// Revoke bridging table entry
@@ -159,7 +160,7 @@
DeviceId newDeviceId = newLocation.deviceId();
PortNumber newPort = newLocation.port();
Set<IpAddress> newIps = event.subject().ipAddresses();
- log.debug("Host {}/{} is moved from {}:{} to {}:{}",
+ log.info("Host {}/{} is moved from {}:{} to {}:{}",
mac, vlanId, prevDeviceId, prevPort, newDeviceId, newPort);
if (accepted(event.prevSubject())) {
@@ -220,12 +221,13 @@
DeviceId newDeviceId = newLocation.deviceId();
PortNumber newPort = newLocation.port();
Set<IpAddress> newIps = event.subject().ipAddresses();
- log.debug("Host {}/{} is updated", mac, vlanId);
+ log.info("Host {}/{} is updated", mac, vlanId);
if (accepted(event.prevSubject())) {
// Revoke previous IP table entry
- prevIps.forEach(ip -> {
+ Sets.difference(prevIps, newIps).forEach(ip -> {
if (srManager.deviceConfiguration.inSameSubnet(prevLocation, ip)) {
+ log.info("revoking previous IP rule:{}", ip);
srManager.routingRulePopulator.revokeRoute(
prevDeviceId, ip.toIpPrefix(), mac, prevPort);
}
@@ -234,8 +236,9 @@
if (accepted(event.subject())) {
// Populate new IP table entry
- newIps.forEach(ip -> {
+ Sets.difference(newIps, prevIps).forEach(ip -> {
if (srManager.deviceConfiguration.inSameSubnet(newLocation, ip)) {
+ log.info("populating new IP rule:{}", ip);
srManager.routingRulePopulator.populateRoute(
newDeviceId, ip.toIpPrefix(), mac, newPort);
}
diff --git a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
index 758e4cc..050bb97 100644
--- a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -509,11 +509,11 @@
fwdObjs.addAll(fwdObjsMpls);
// Generates the transit rules used by the MPLS Pwaas. For now it is
// the only case !BoS supported.
- fwdObjsMpls = handleMpls(targetSwId, destSwId, nextHops, segmentId, routerIp, false);
+ /*fwdObjsMpls = handleMpls(targetSwId, destSwId, nextHops, segmentId, routerIp, false);
if (fwdObjsMpls.isEmpty()) {
return false;
}
- fwdObjs.addAll(fwdObjsMpls);
+ fwdObjs.addAll(fwdObjsMpls);*/
for (ForwardingObjective fwdObj : fwdObjs) {
log.debug("Sending MPLS fwd obj {} for SID {}-> next {} in sw: {}",
@@ -595,9 +595,11 @@
* 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).
+ * Note that the vlan assignment and filter programming should only be done by
+ * the master for a switch. This method is typically called at deviceAdd and
+ * programs filters only for the enabled ports of the device. For port-updates,
+ * that enable/disable ports after device add, singlePortFilter methods should
+ * be called.
*
* @param deviceId the switch dpid for the router
* @return PortFilterInfo information about the processed ports
@@ -606,73 +608,117 @@
log.debug("Installing per-port filtering objective for untagged "
+ "packets in device {}", deviceId);
- MacAddress deviceMac;
- try {
- deviceMac = config.getDeviceMac(deviceId);
- } catch (DeviceConfigNotFoundException e) {
- log.warn(e.getMessage() + " Aborting populateRouterMacVlanFilters.");
- return null;
- }
-
List<Port> devPorts = srManager.deviceService.getPorts(deviceId);
if (devPorts == null || devPorts.isEmpty()) {
log.warn("Device {} ports not available. Unable to add MacVlan filters",
deviceId);
return null;
}
- int disabledPorts = 0, suppressedPorts = 0, filteredPorts = 0;
+ int disabledPorts = 0, errorPorts = 0, filteredPorts = 0;
for (Port port : devPorts) {
- ConnectPoint connectPoint = new ConnectPoint(deviceId, port.number());
- // TODO: Handles dynamic port events when we are ready for dynamic config
if (!port.isEnabled()) {
disabledPorts++;
continue;
}
-
- boolean isSuppressed = false;
- SegmentRoutingAppConfig appConfig = srManager.cfgService
- .getConfig(srManager.appId, SegmentRoutingAppConfig.class);
- if (appConfig != null && appConfig.suppressSubnet().contains(connectPoint)) {
- isSuppressed = true;
- suppressedPorts++;
- continue;
+ if (populateSinglePortFilters(deviceId, port.number())) {
+ filteredPorts++;
+ } else {
+ errorPorts++;
}
-
- Ip4Prefix portSubnet = config.getPortIPv4Subnet(deviceId, port.number());
- VlanId assignedVlan = (portSubnet == null || isSuppressed)
- ? VlanId.vlanId(SegmentRoutingManager.ASSIGNED_VLAN_NO_SUBNET)
- : srManager.getSubnetAssignedVlanId(deviceId, portSubnet);
-
- if (assignedVlan == null) {
- log.warn("Assigned vlan is null for {} in {} - Aborting populateRouterMacVlanFilters.",
- port.number(), deviceId);
- return null;
- }
-
- FilteringObjective.Builder fob = DefaultFilteringObjective.builder();
- fob.withKey(Criteria.matchInPort(port.number()))
- .addCondition(Criteria.matchEthDst(deviceMac))
- .addCondition(Criteria.matchVlanId(VlanId.NONE))
- .withPriority(SegmentRoutingService.DEFAULT_PRIORITY);
- // vlan assignment is valid only if this instance is master
- if (srManager.mastershipService.isLocalMaster(deviceId)) {
- TrafficTreatment tt = DefaultTrafficTreatment.builder()
- .pushVlan().setVlanId(assignedVlan).build();
- fob.withMeta(tt);
- }
- fob.permit().fromApp(srManager.appId);
- log.debug("Sending filtering objective for dev/port:{}/{}", deviceId, port);
- filteredPorts++;
- ObjectiveContext context = new DefaultObjectiveContext(
- (objective) -> log.debug("Filter for {} populated", connectPoint),
- (objective, error) ->
- log.warn("Failed to populate filter for {}: {}", connectPoint, error));
- srManager.flowObjectiveService.filter(deviceId, fob.add(context));
}
- log.info("Filtering on dev:{}, disabledPorts:{}, suppressedPorts:{}, filteredPorts:{}",
- deviceId, disabledPorts, suppressedPorts, filteredPorts);
+ log.info("Filtering on dev:{}, disabledPorts:{}, errorPorts:{}, filteredPorts:{}",
+ deviceId, disabledPorts, errorPorts, filteredPorts);
return srManager.defaultRoutingHandler.new PortFilterInfo(disabledPorts,
- suppressedPorts, filteredPorts);
+ errorPorts, filteredPorts);
+ }
+
+ /**
+ * Creates filtering objectives for a single port. Should only be called
+ * by the master for a switch.
+ *
+ * @param deviceId device identifier
+ * @param portnum port identifier for port to be filtered
+ * @return true if no errors occurred during the build of the filtering objective
+ */
+ public boolean populateSinglePortFilters(DeviceId deviceId, PortNumber portnum) {
+ FilteringObjective.Builder fob = buildFilteringObjective(deviceId, portnum);
+ if (fob == null) {
+ // error encountered during build
+ return false;
+ }
+ log.info("Sending filtering objectives for dev/port:{}/{}", deviceId, portnum);
+ ObjectiveContext context = new DefaultObjectiveContext(
+ (objective) -> log.debug("Filter for {}/{} populated", deviceId, portnum),
+ (objective, error) ->
+ log.warn("Failed to populate filter for {}/{}: {}",
+ deviceId, portnum, error));
+ srManager.flowObjectiveService.filter(deviceId, fob.add(context));
+ return true;
+ }
+
+ /**
+ * Removes filtering objectives for a single port. Should only be called
+ * by the master for a switch.
+ *
+ * @param deviceId device identifier
+ * @param portnum port identifier
+ */
+ public void revokeSinglePortFilters(DeviceId deviceId, PortNumber portnum) {
+ FilteringObjective.Builder fob = buildFilteringObjective(deviceId, portnum);
+ if (fob == null) {
+ // error encountered during build
+ return;
+ }
+ log.info("Removing filtering objectives for dev/port:{}/{}", deviceId, portnum);
+ ObjectiveContext context = new DefaultObjectiveContext(
+ (objective) -> log.debug("Filter for {}/{} removed", deviceId, portnum),
+ (objective, error) ->
+ log.warn("Failed to remove filter for {}/{}: {}",
+ deviceId, portnum, error));
+ srManager.flowObjectiveService.filter(deviceId, fob.remove(context));
+ return;
+ }
+
+
+ private FilteringObjective.Builder buildFilteringObjective(DeviceId deviceId,
+ PortNumber portnum) {
+ MacAddress deviceMac;
+ try {
+ deviceMac = config.getDeviceMac(deviceId);
+ } catch (DeviceConfigNotFoundException e) {
+ log.warn(e.getMessage() + " Processing SinglePortFilters aborted");
+ return null;
+ }
+ // suppressed ports still have filtering rules pushed by SR using default vlan
+ ConnectPoint connectPoint = new ConnectPoint(deviceId, portnum);
+ boolean isSuppressed = false;
+ SegmentRoutingAppConfig appConfig = srManager.cfgService
+ .getConfig(srManager.appId, SegmentRoutingAppConfig.class);
+ if (appConfig != null && appConfig.suppressSubnet().contains(connectPoint)) {
+ isSuppressed = true;
+ }
+
+ Ip4Prefix portSubnet = config.getPortIPv4Subnet(deviceId, portnum);
+ VlanId assignedVlan = (portSubnet == null || isSuppressed)
+ ? VlanId.vlanId(SegmentRoutingManager.ASSIGNED_VLAN_NO_SUBNET)
+ : srManager.getSubnetAssignedVlanId(deviceId, portSubnet);
+
+ if (assignedVlan == null) {
+ log.warn("Assigned vlan is null for {} in {} - Processing "
+ + "SinglePortFilters aborted", portnum, deviceId);
+ return null;
+ }
+
+ FilteringObjective.Builder fob = DefaultFilteringObjective.builder();
+ fob.withKey(Criteria.matchInPort(portnum))
+ .addCondition(Criteria.matchEthDst(deviceMac))
+ .addCondition(Criteria.matchVlanId(VlanId.NONE))
+ .withPriority(SegmentRoutingService.DEFAULT_PRIORITY);
+ TrafficTreatment tt = DefaultTrafficTreatment.builder()
+ .pushVlan().setVlanId(assignedVlan).build();
+ fob.withMeta(tt);
+ fob.permit().fromApp(srManager.appId);
+ return fob;
}
/**
diff --git a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 9c761e7..d7896a1 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -499,7 +499,9 @@
public void rerouteNetwork() {
cfgListener.configureNetwork();
for (Device device : deviceService.getDevices()) {
- defaultRoutingHandler.populatePortAddressingRules(device.id());
+ if (mastershipService.isLocalMaster(device.id())) {
+ defaultRoutingHandler.populatePortAddressingRules(device.id());
+ }
}
defaultRoutingHandler.startPopulationProcess();
}
@@ -952,13 +954,9 @@
deviceId);
groupHandlerMap.put(deviceId, 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(deviceId);
if (mastershipService.isLocalMaster(deviceId)) {
+ defaultRoutingHandler.populatePortAddressingRules(deviceId);
hostHandler.init(deviceId);
xConnectHandler.init(deviceId);
cordConfigHandler.init(deviceId);
@@ -1004,19 +1002,32 @@
+ "dev: {} port: {}", device.id(), port.number());
return;
}
- /* XXX create method for single port filtering rules which are needed
- for both switch-to-switch ports and edge ports
- if (defaultRoutingHandler != null) {
- defaultRoutingHandler.populatePortAddressingRules(
- ((Device) event.subject()).id());
- }*/
+
+ if (!mastershipService.isLocalMaster(device.id())) {
+ log.debug("Not master for dev:{} .. not handling port updated event"
+ + "for port {}", device.id(), port.number());
+ return;
+ }
+
+ // first we handle filtering rules associated with the port
+ if (port.isEnabled()) {
+ log.info("Switchport {}/{} enabled..programming filters",
+ device.id(), port.number());
+ defaultRoutingHandler.populateSinglePortFilteringRules(device.id(),
+ port.number());
+ } else {
+ log.info("Switchport {}/{} disabled..removing filters",
+ device.id(), port.number());
+ defaultRoutingHandler.revokeSinglePortFilteringRules(device.id(),
+ port.number());
+ }
// portUpdated calls are for ports that have gone down or up. For switch
// to switch ports, link-events should take care of any re-routing or
// group editing necessary for port up/down. Here we only process edge ports
// that are already configured.
- Ip4Prefix configuredSubnet = deviceConfiguration.getPortIPv4Subnet(device.id(),
- port.number());
+ Ip4Prefix configuredSubnet = deviceConfiguration.getPortIPv4Subnet(device.id(),
+ port.number());
if (configuredSubnet == null) {
log.debug("Not handling port updated event for unconfigured port "
+ "dev/port: {}/{}", device.id(), port.number());
@@ -1037,8 +1048,7 @@
DefaultGroupHandler groupHandler = groupHandlerMap.get(device.id());
if (groupHandler != null) {
- groupHandler.processEdgePort(port.number(), subnet, portUp,
- mastershipService.isLocalMaster(device.id()));
+ groupHandler.processEdgePort(port.number(), subnet, portUp);
} else {
log.warn("Group handler not found for dev:{}. Not handling edge port"
+ " {} event for port:{}", device.id(),
@@ -1196,8 +1206,8 @@
/////////////////////////////////////////////////////////////////
// XXX Neighbour hacking, temporary workaround will be //
- // removed as soon as possible, when the bridging will //
- // be implemented. For now, it's fine to leave this //
+ // removed as soon as possible, when bridging based //
+ // control plane redirect is implemented. //
/////////////////////////////////////////////////////////////////
// XXX Neighbour hacking. To store upstream connect
diff --git a/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java b/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
index 6ceeb4b..bd0db54 100644
--- a/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
+++ b/src/main/java/org/onosproject/segmentrouting/config/DeviceConfiguration.java
@@ -456,7 +456,7 @@
.collect(Collectors.toSet());
if (subnets.isEmpty()) {
- log.info(NO_SUBNET, connectPoint);
+ log.debug(NO_SUBNET, connectPoint);
return Collections.emptySet();
} else if (subnets.size() > 2) {
log.warn(TOO_MANY_SUBNET, connectPoint);
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
index f301a78..0e8f220 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
@@ -360,17 +360,13 @@
/**
* Adds or removes a port that has been configured with a subnet to a broadcast group
* for bridging. Note that this does not create the broadcast group itself.
+ * Should only be called by the master instance for this device/port.
*
* @param port the port on this device that needs to be added/removed to a bcast group
* @param subnet the subnet corresponding to the broadcast group
* @param portUp true if port is enabled, false if disabled
- * @param isMaster true if local instance is the master
*/
- public void processEdgePort(PortNumber port, Ip4Prefix subnet,
- boolean portUp, boolean isMaster) {
- if (!isMaster) {
- return;
- }
+ public void processEdgePort(PortNumber port, Ip4Prefix subnet, boolean portUp) {
//get the next id for the subnet and edit it.
Integer nextId = getSubnetNextObjectiveId(subnet);
if (nextId == -1) {