CORD-637 Fixing intermittent filtering objective failures by repeatedly trying
till results are consistent. Also fixed some typos and made some logs clearer.
Change-Id: If829b02ac6dc2f8ada455b5290c718d29a6d7988
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
index 01ffd6e..ba918c6 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/DefaultRoutingHandler.java
@@ -15,6 +15,7 @@
*/
package org.onosproject.segmentrouting;
+import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
@@ -37,6 +38,7 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
@@ -44,13 +46,15 @@
import java.util.concurrent.locks.ReentrantLock;
import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.MoreObjects.toStringHelper;
/**
* Default routing handler that is responsible for route computing and
* routing rule population.
*/
public class DefaultRoutingHandler {
- private static final int MAX_RETRY_ATTEMPTS = 25;
+ private static final int MAX_CONSTANT_RETRY_ATTEMPTS = 4;
+ private static final int RETRY_INTERVAL_MS = 500;
private static final String ECMPSPG_MISSING = "ECMP shortest path graph not found";
private static Logger log = LoggerFactory.getLogger(DefaultRoutingHandler.class);
@@ -62,7 +66,7 @@
private final Lock statusLock = new ReentrantLock();
private volatile Status populationStatus;
private ScheduledExecutorService executorService
- = newScheduledThreadPool(1, groupedThreads("RoutingHandler", "retry-%d", log));
+ = newScheduledThreadPool(1, groupedThreads("retryftr", "retry-%d", log));
/**
* Represents the default routing population status.
@@ -612,10 +616,12 @@
// ports for this device yet. It results in missing filtering rules in the
// switch. We will attempt it a few times. If it still does not work,
// user can manually repopulate using CLI command sr-reroute-network
- boolean success = rulePopulator.populateRouterMacVlanFilters(deviceId);
- if (!success) {
- executorService.schedule(new RetryFilters(deviceId), 200, TimeUnit.MILLISECONDS);
+ PortFilterInfo firstRun = rulePopulator.populateRouterMacVlanFilters(deviceId);
+ if (firstRun == null) {
+ firstRun = new PortFilterInfo(0, 0, 0);
}
+ executorService.schedule(new RetryFilters(deviceId, firstRun),
+ RETRY_INTERVAL_MS, TimeUnit.MILLISECONDS);
}
/**
@@ -704,26 +710,82 @@
this.populateRoutingRulesForLinkStatusChange(null);
}
- private final class RetryFilters implements Runnable {
- int attempts = MAX_RETRY_ATTEMPTS;
- DeviceId devId;
+ /**
+ * 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;
- private RetryFilters(DeviceId deviceId) {
+ public PortFilterInfo(int disabledPorts, int suppressedPorts,
+ int filteredPorts) {
+ this.disabledPorts = disabledPorts;
+ this.filteredPorts = filteredPorts;
+ this.suppressedPorts = suppressedPorts;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(disabledPorts, filteredPorts, suppressedPorts);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if ((obj == null) || (!(obj instanceof PortFilterInfo))) {
+ return false;
+ }
+ PortFilterInfo other = (PortFilterInfo) obj;
+ return ((disabledPorts == other.disabledPorts) &&
+ (filteredPorts == other.filteredPorts) &&
+ (suppressedPorts == other.suppressedPorts));
+ }
+
+ @Override
+ public String toString() {
+ MoreObjects.ToStringHelper helper = toStringHelper(this)
+ .add("disabledPorts", disabledPorts)
+ .add("suppressedPorts", suppressedPorts)
+ .add("filteredPorts", filteredPorts);
+ return helper.toString();
+ }
+ }
+
+ /**
+ * RetryFilters populates filtering objectives for a device and keeps retrying
+ * till the number of ports filtered are constant for a predefined number
+ * of attempts.
+ */
+ protected final class RetryFilters implements Runnable {
+ int constantAttempts = MAX_CONSTANT_RETRY_ATTEMPTS;
+ DeviceId devId;
+ int counter;
+ PortFilterInfo prevRun;
+
+ private RetryFilters(DeviceId deviceId, PortFilterInfo previousRun) {
devId = deviceId;
+ prevRun = previousRun;
+ counter = 0;
}
@Override
public void run() {
- log.info("RETRY FILTER ATTEMPT# {} for dev:{}",
- MAX_RETRY_ATTEMPTS - attempts, devId);
- boolean success = rulePopulator.populateRouterMacVlanFilters(devId);
- if (!success && --attempts > 0) {
- executorService.schedule(this, 200, TimeUnit.MILLISECONDS);
- } else if (attempts == 0) {
- log.error("Unable to populate MacVlan filters in dev:{}", devId);
+ log.info("RETRY FILTER ATTEMPT {} ** dev:{}", ++counter, devId);
+ PortFilterInfo thisRun = rulePopulator.populateRouterMacVlanFilters(devId);
+ boolean sameResult = prevRun.equals(thisRun);
+ 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);
+ if (!sameResult) {
+ constantAttempts = MAX_CONSTANT_RETRY_ATTEMPTS; //reset
+ }
}
+ prevRun = (thisRun == null) ? prevRun : thisRun;
}
-
}
}
diff --git a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/EcmpShortestPathGraph.java b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/EcmpShortestPathGraph.java
index 5e7e2ae..f5d8ed1 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/EcmpShortestPathGraph.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/EcmpShortestPathGraph.java
@@ -31,7 +31,7 @@
/**
* This class creates bandwidth constrained breadth first tree and returns paths
- * from root Device to leaf Devices which satisfies the bandwidth condition. If
+ * from root Device to leaf Devices (target devices) which satisfies the bandwidth condition. If
* bandwidth parameter is not specified, the normal breadth first tree will be
* calculated. The paths are snapshot paths at the point of the class
* instantiation.
@@ -302,15 +302,15 @@
}
/**
- * Return the complete info of the computed ECMP paths for each Device
+ * Returns the complete info of the computed ECMP paths for each target device
* learned in multiple iterations from the root Device. The computed info
* returned is per iteration (Integer key of outer HashMap). In each
- * iteration, for each device as root (DeviceId key of inner HashMap),
+ * iteration, for the target devices reached (DeviceId key of inner HashMap),
* the ECMP paths are detailed (2D array).
*
- * @return the hash table of Devices learned in multiple Dijkstra
+ * @return the hash table of target Devices learned in multiple Dijkstra
* iterations and corresponding ECMP paths in terms of Devices to
- * be traversed to it from the root Device
+ * be traversed (via) from the root Device to the target Device
*/
public HashMap<Integer, HashMap<DeviceId,
ArrayList<ArrayList<DeviceId>>>> getAllLearnedSwitchesAndVia() {
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 e12fbcc..2f07c05 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -26,6 +26,7 @@
import org.onosproject.net.ConnectPoint;
import org.onosproject.net.flowobjective.DefaultObjectiveContext;
import org.onosproject.net.flowobjective.ObjectiveContext;
+import org.onosproject.segmentrouting.DefaultRoutingHandler.PortFilterInfo;
import org.onosproject.segmentrouting.config.DeviceConfigNotFoundException;
import org.onosproject.segmentrouting.config.DeviceConfiguration;
import org.onosproject.segmentrouting.config.SegmentRoutingAppConfig;
@@ -535,9 +536,9 @@
* that drivers can obtain other information (like Router MAC and IP).
*
* @param deviceId the switch dpid for the router
- * @return true if operation succeeds
+ * @return PortFilterInfo information about the processed ports
*/
- public boolean populateRouterMacVlanFilters(DeviceId deviceId) {
+ public PortFilterInfo populateRouterMacVlanFilters(DeviceId deviceId) {
log.debug("Installing per-port filtering objective for untagged "
+ "packets in device {}", deviceId);
@@ -546,14 +547,14 @@
deviceMac = config.getDeviceMac(deviceId);
} catch (DeviceConfigNotFoundException e) {
log.warn(e.getMessage() + " Aborting populateRouterMacVlanFilters.");
- return false;
+ return null;
}
List<Port> devPorts = srManager.deviceService.getPorts(deviceId);
if (devPorts != null && devPorts.size() == 0) {
log.warn("Device {} ports not available. Unable to add MacVlan filters",
deviceId);
- return false;
+ return null;
}
int disabledPorts = 0, suppressedPorts = 0, filteredPorts = 0;
for (Port port : devPorts) {
@@ -596,13 +597,8 @@
}
log.info("Filtering on dev:{}, disabledPorts:{}, suppressedPorts:{}, filteredPorts:{}",
deviceId, disabledPorts, suppressedPorts, filteredPorts);
- // XXX With this check, there is a chance that not all the ports that
- // should be filtered actually get filtered as long as one of them does.
- // Note there is no PORT_UPDATED event that makes the port go from disabled
- // to enabled state, because the ports comes enabled from the switch.
- // Check ONOS core, where the port becoming available and being declared
- // enabled is possibly not atomic.
- return (filteredPorts > 0) ? true : false;
+ return srManager.defaultRoutingHandler.new PortFilterInfo(disabledPorts,
+ suppressedPorts, filteredPorts);
}
/**
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 ca41a0f..dd2f5e8 100644
--- a/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/apps/segmentrouting/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -742,14 +742,17 @@
processDeviceRemoved((Device) event.subject());
}
} else if (event.type() == DeviceEvent.Type.PORT_ADDED) {
- // XXX typically these calls come when device is added
- // so port filtering rules are handled there, and it
- // represents all ports on the device, enabled or not.
+ // typically these calls come when device is added first time
+ // so port filtering rules are handled at the device_added event.
+ // port added calls represent all ports on the device,
+ // enabled or not.
log.debug("** PORT ADDED {}/{} -> {}",
- event.subject(),
- ((DeviceEvent) event).port(),
- event.type());
+ ((DeviceEvent) event).subject().id(),
+ ((DeviceEvent) event).port().number(),
+ event.type());
} else if (event.type() == DeviceEvent.Type.PORT_UPDATED) {
+ // these calls happen for every subsequent event
+ // ports enabled, disabled, switch goes away, comes back
log.info("** PORT UPDATED {}/{} -> {}",
event.subject(),
((DeviceEvent) event).port(),
diff --git a/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java b/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java
index f2fcdab..42701c0 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/pipeline/Ofdpa2Pipeline.java
@@ -207,7 +207,7 @@
rules.stream()
.filter(Objects::nonNull)
.forEach(flowOpsBuilder::add);
- log.info("Applying a flow rule to sw:{}", deviceId);
+ log.debug("Applying a add fwd-obj {} to sw:{}", fwd.id(), deviceId);
break;
case REMOVE:
rules.stream()
@@ -710,12 +710,14 @@
* the flow rule
*/
protected Collection<FlowRule> processVersatile(ForwardingObjective fwd) {
- log.info("Processing versatile forwarding objective");
+ log.info("Processing versatile forwarding objective:{} in dev:{}",
+ fwd.id(), deviceId);
EthTypeCriterion ethType =
(EthTypeCriterion) fwd.selector().getCriterion(Criterion.Type.ETH_TYPE);
if (ethType == null) {
- log.error("Versatile forwarding objective must include ethType");
+ log.error("Versatile forwarding objective:{} must include ethType",
+ fwd.id());
fail(fwd, ObjectiveError.BADPARAMS);
return Collections.emptySet();
}