Changes include:

      bug fix for host IP and MAC flows not being generated sometimes in multi-controller scenarios
      bug fix for filtering objectives not being sent sometimes when ports become available later
      npe fixes in ofdpa driver for cases where selectors or treatments may not be available
      new cli command to manually trigger routing and rule population
      portstats option on cli to display only those ports with non-zero stats
      group cli command tab completion displays choices in lower case (similar to flows)
      segment routing cli commands now start with sr-

Change-Id: Idcd641882d180acbd304e5560ed3483b5a943f96

@@ -32,6 +32,9 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
@@ -53,6 +56,8 @@ public class DefaultRoutingHandler {
     private DeviceConfiguration config;
     private final Lock statusLock = new ReentrantLock();
     private volatile Status populationStatus;
+    private static final int MAX_RETRY_ATTEMPTS = 5;
+    private ScheduledExecutorService executorService = Executors.newScheduledThreadPool(1);
 
     /**
      * Represents the default routing population status.
@@ -515,9 +520,17 @@ public class DefaultRoutingHandler {
      * @param deviceId Switch ID to set the rules
      */
     public void populatePortAddressingRules(DeviceId deviceId) {
-        rulePopulator.populateRouterMacVlanFilters(deviceId);
         rulePopulator.populateXConnectVlanFilters(deviceId);
         rulePopulator.populateRouterIpPunts(deviceId);
+
+        // Although device is added, sometimes device store does not have the
+        // 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);
+        }
     }
 
     /**
@@ -568,4 +581,25 @@ public class DefaultRoutingHandler {
             updatedEcmpSpgMap.remove(deviceId);
         }
     }
+
+    private class RetryFilters implements Runnable {
+        int attempts = MAX_RETRY_ATTEMPTS;
+        DeviceId devId;
+
+        public RetryFilters(DeviceId deviceId) {
+            devId = deviceId;
+        }
+
+        @Override
+        public void run() {
+            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);
+            }
+        }
+
+    }
+
 }

@@ -62,18 +62,28 @@ public class HostHandler {
         flowObjectiveService = srManager.flowObjectiveService;
     }
 
-    protected void readInitialHosts() {
+    protected void readInitialHosts(DeviceId devId) {
         hostService.getHosts().forEach(host -> {
+            DeviceId deviceId = host.location().deviceId();
+            if (!deviceId.equals(devId)) {
+                // not an attached host to this device
+                return;
+            }
             MacAddress mac = host.mac();
             VlanId vlanId = host.vlan();
-            DeviceId deviceId = host.location().deviceId();
             PortNumber port = host.location().port();
             Set<IpAddress> ips = host.ipAddresses();
-            log.debug("Host {}/{} is added at {}:{}", mac, vlanId, deviceId, port);
+            log.debug("Attached Host {}/{} is added at {}:{}", mac, vlanId,
+                      deviceId, port);
 
             // Populate bridging table entry
             ForwardingObjective.Builder fob =
                     getForwardingObjectiveBuilder(deviceId, mac, vlanId, port);
+            if (fob == null) {
+                log.warn("Aborting host bridging & routing table entries due "
+                         + "to error for dev:{} host:{}", deviceId, host);
+                return;
+            }
             ObjectiveContext context = new DefaultObjectiveContext(
                     (objective) -> log.debug("Host rule for {} populated", host),
                     (objective, error) ->
@@ -127,6 +137,10 @@ public class HostHandler {
         int portNextObjId = srManager.getPortNextObjectiveId(deviceId, outport,
                 tbuilder.build(),
                 meta);
+        if (portNextObjId == -1) {
+            // warning log will come from getPortNextObjective method
+            return null;
+        }
 
         return DefaultForwardingObjective.builder()
                 .withFlag(ForwardingObjective.Flag.SPECIFIC)

@@ -114,6 +114,11 @@ public class RoutingRulePopulator {
             log.warn(e.getMessage() + " Aborting populateIpRuleForHost.");
             return;
         }
+        if (fwdBuilder == null) {
+            log.warn("Aborting host routing table entries due "
+                    + "to error for dev:{} host:{}", deviceId, hostIp);
+            return;
+        }
         ObjectiveContext context = new DefaultObjectiveContext(
                 (objective) -> log.debug("IP rule for host {} populated", hostIp),
                 (objective, error) ->
@@ -191,7 +196,10 @@ public class RoutingRulePopulator {
                                     .matchVlanId(outvlan).build();
         int portNextObjId = srManager.getPortNextObjectiveId(deviceId, outPort,
                                                              treatment, meta);
-
+        if (portNextObjId == -1) {
+            // warning log will come from getPortNextObjective method
+            return null;
+        }
         return DefaultForwardingObjective.builder()
                 .withSelector(selector)
                 .nextStep(portNextObjId)
@@ -464,7 +472,7 @@ public class RoutingRulePopulator {
      *
      * @param deviceId  the switch dpid for the router
      */
-    public void populateRouterMacVlanFilters(DeviceId deviceId) {
+    public boolean populateRouterMacVlanFilters(DeviceId deviceId) {
         log.debug("Installing per-port filtering objective for untagged "
                 + "packets in device {}", deviceId);
 
@@ -473,10 +481,17 @@ public class RoutingRulePopulator {
             deviceMac = config.getDeviceMac(deviceId);
         } catch (DeviceConfigNotFoundException e) {
             log.warn(e.getMessage() + " Aborting populateRouterMacVlanFilters.");
-            return;
+            return false;
+        }
+
+        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;
         }
 
-        for (Port port : srManager.deviceService.getPorts(deviceId)) {
+        for (Port port : devPorts) {
             ConnectPoint connectPoint = new ConnectPoint(deviceId, port.number());
             // TODO: Handles dynamic port events when we are ready for dynamic config
             if (!srManager.deviceConfiguration.suppressSubnet().contains(connectPoint) &&
@@ -498,6 +513,7 @@ public class RoutingRulePopulator {
                     fob.withMeta(tt);
                 }
                 fob.permit().fromApp(srManager.appId);
+                log.debug("Sending filtering objective for dev/port:{}/{}", deviceId, port);
                 ObjectiveContext context = new DefaultObjectiveContext(
                         (objective) -> log.debug("Filter for {} populated", connectPoint),
                         (objective, error) ->
@@ -505,6 +521,7 @@ public class RoutingRulePopulator {
                 srManager.flowObjectiveService.filter(deviceId, fob.add(context));
             }
         }
+        return true;
     }
 
     /**

@@ -434,6 +434,15 @@ public class SegmentRoutingManager implements SegmentRoutingService {
         return policyHandler.getPolicies();
     }
 
+    @Override
+    public void rerouteNetwork() {
+        cfgListener.configureNetwork();
+        for (Device device : deviceService.getDevices()) {
+            defaultRoutingHandler.populatePortAddressingRules(device.id());
+        }
+        defaultRoutingHandler.startPopulationProcess();
+    }
+
     /**
      * Returns the tunnel object with the tunnel ID.
      *
@@ -567,7 +576,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
      * @param portNum port number on device for which NextObjective is queried
      * @param treatment the actions to apply on the packets (should include outport)
      * @param meta metadata passed into the creation of a Next Objective if necessary
-     * @return next objective ID or -1 if it was not found
+     * @return next objective ID or -1 if an error occurred during retrieval or creation
      */
     public int getPortNextObjectiveId(DeviceId deviceId, PortNumber portNum,
                                       TrafficTreatment treatment,
@@ -801,9 +810,9 @@ public class SegmentRoutingManager implements SegmentRoutingService {
             // port addressing rules to the driver as well irrespective of whether
             // this instance is the master or not.
             defaultRoutingHandler.populatePortAddressingRules(device.id());
-            hostHandler.readInitialHosts();
         }
         if (mastershipService.isLocalMaster(device.id())) {
+            hostHandler.readInitialHosts(device.id());
             DefaultGroupHandler groupHandler = groupHandlerMap.get(device.id());
             groupHandler.createGroupsFromSubnetConfig();
             routingRulePopulator.populateSubnetBroadcastRule(device.id());
@@ -897,6 +906,7 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                 // 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.
+                log.debug("Current groupHandlerMap devs: {}", groupHandlerMap.keySet());
                 if (groupHandlerMap.get(device.id()) == null) {
                     DefaultGroupHandler groupHandler;
                     try {
@@ -911,6 +921,8 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                         log.warn(e.getMessage() + " Aborting configureNetwork.");
                         return;
                     }
+                    log.debug("updating groupHandlerMap with new config for "
+                            + "device: {}", device.id());
                     groupHandlerMap.put(device.id(), groupHandler);
 
                     // Also, in some cases, drivers may need extra
@@ -918,9 +930,9 @@ public class SegmentRoutingManager implements SegmentRoutingService {
                     // port addressing rules to the driver as well, irrespective of whether
                     // this instance is the master or not.
                     defaultRoutingHandler.populatePortAddressingRules(device.id());
-                    hostHandler.readInitialHosts();
                 }
                 if (mastershipService.isLocalMaster(device.id())) {
+                    hostHandler.readInitialHosts(device.id());
                     DefaultGroupHandler groupHandler = groupHandlerMap.get(device.id());
                     groupHandler.createGroupsFromSubnetConfig();
                     routingRulePopulator.populateSubnetBroadcastRule(device.id());

@@ -103,4 +103,10 @@ public interface SegmentRoutingService {
      * SUCCESS if it is removed successfully
      */
     PolicyHandler.Result removePolicy(Policy policy);
+
+    /**
+     * Use current state of the network to repopulate forwarding rules.
+     *
+     */
+    void rerouteNetwork();
 }

@@ -26,7 +26,7 @@ import org.onosproject.segmentrouting.TunnelPolicy;
 /**
  * Command to add a new policy.
  */
-@Command(scope = "onos", name = "srpolicy-add",
+@Command(scope = "onos", name = "sr-policy-add",
         description = "Create a new policy")
 public class PolicyAddCommand extends AbstractShellCommand {
 

@@ -24,7 +24,7 @@ import org.onosproject.segmentrouting.TunnelPolicy;
 /**
  * Command to show the list of policies.
  */
-@Command(scope = "onos", name = "srpolicy-list",
+@Command(scope = "onos", name = "sr-policy-list",
         description = "Lists all policies")
 public class PolicyListCommand extends AbstractShellCommand {
 

@@ -26,7 +26,7 @@ import org.onosproject.segmentrouting.TunnelPolicy;
 /**
  * Command to remove a policy.
  */
-@Command(scope = "onos", name = "srpolicy-remove",
+@Command(scope = "onos", name = "sr-policy-remove",
         description = "Remove a policy")
 public class PolicyRemoveCommand extends AbstractShellCommand {
 

+package org.onosproject.segmentrouting.cli;
+
+
+import org.apache.karaf.shell.commands.Command;
+import org.onosproject.cli.AbstractShellCommand;
+import org.onosproject.segmentrouting.SegmentRoutingService;
+
+/**
+ * Command to manually trigger routing and rule-population in the network.
+ *
+ */
+@Command(scope = "onos", name = "sr-reroute-network",
+        description = "Repopulate routing rules given current network state")
+public class RerouteNetworkCommand extends AbstractShellCommand {
+
+    @Override
+    protected void execute() {
+        SegmentRoutingService srService =
+                AbstractShellCommand.get(SegmentRoutingService.class);
+        srService.rerouteNetwork();
+    }
+
+}

@@ -31,7 +31,7 @@ import java.util.StringTokenizer;
 /**
  * Command to add a new tunnel.
  */
-@Command(scope = "onos", name = "srtunnel-add",
+@Command(scope = "onos", name = "sr-tunnel-add",
         description = "Create a new tunnel")
 public class TunnelAddCommand extends AbstractShellCommand {
 

@@ -23,7 +23,7 @@ import org.onosproject.segmentrouting.Tunnel;
 /**
  * Command to show the list of tunnels.
  */
-@Command(scope = "onos", name = "srtunnel-list",
+@Command(scope = "onos", name = "sr-tunnel-list",
         description = "Lists all tunnels")
 public class TunnelListCommand extends AbstractShellCommand {
 

@@ -28,7 +28,7 @@ import org.onosproject.segmentrouting.TunnelHandler;
 /**
  * Command to remove a tunnel.
  */
-@Command(scope = "onos", name = "srtunnel-remove",
+@Command(scope = "onos", name = "sr-tunnel-remove",
         description = "Remove a tunnel")
 public class TunnelRemoveCommand extends AbstractShellCommand {
 

@@ -101,8 +101,8 @@ public class DeviceConfiguration implements DeviceProperties {
             info.mac = config.routerMac();
             info.isEdge = config.isEdgeRouter();
             info.adjacencySids = config.adjacencySids();
-
             deviceConfigMap.put(info.deviceId, info);
+            log.info("Read device config for device: {}", info.deviceId);
             allSegmentIds.add(info.nodeSid);
         });
 

@@ -197,6 +197,10 @@ public class DefaultGroupHandler {
 
         log.info("* LinkUP: Device {} linkUp at local port {} to neighbor {}", deviceId,
                  newLink.src().port(), newLink.dst().deviceId());
+        // ensure local state is updated even if linkup is aborted later on
+        addNeighborAtPort(newLink.dst().deviceId(),
+                          newLink.src().port());
+
         MacAddress dstMac;
         try {
             dstMac = deviceConfig.getDeviceMac(newLink.dst().deviceId());
@@ -205,8 +209,6 @@ public class DefaultGroupHandler {
             return;
         }
 
-        addNeighborAtPort(newLink.dst().deviceId(),
-                          newLink.src().port());
         /*if (devicePortMap.get(newLink.dst().deviceId()) == null) {
             // New Neighbor
             newNeighbor(newLink);

@@ -34,6 +34,9 @@
         <command>
             <action class="org.onosproject.segmentrouting.cli.TunnelRemoveCommand"/>
         </command>
+        <command>
+            <action class="org.onosproject.segmentrouting.cli.RerouteNetworkCommand"/>
+        </command>
     </command-bundle>
 </blueprint>
 

@@ -36,6 +36,10 @@ import org.onosproject.net.device.PortStatistics;
         description = "Lists statistics of all ports in the system")
 public class DevicePortStatsCommand extends DevicesListCommand {
 
+    @Option(name = "-nz", aliases = "--nonzero", description = "Show only non-zero portstats",
+            required = false, multiValued = false)
+    private boolean nonzero = false;
+
     @Option(name = "-d", aliases = "--delta", description = "Show Delta Port Statistics,"
             + "only for the last polling interval",
             required = false, multiValued = false)
@@ -100,6 +104,9 @@ public class DevicePortStatsCommand extends DevicesListCommand {
             if (portNumber != null && stat.port() != portNumber) {
                 continue;
             }
+            if (nonzero && stat.isZero()) {
+                continue;
+            }
             print(FORMAT, stat.port(), stat.packetsReceived(), stat.packetsSent(), stat.bytesReceived(),
                     stat.bytesSent(), stat.packetsRxDropped(), stat.packetsTxDropped(), stat.durationSec());
         }
@@ -118,6 +125,9 @@ public class DevicePortStatsCommand extends DevicesListCommand {
             if (portNumber != null && stat.port() != portNumber) {
                 continue;
             }
+            if (nonzero && stat.isZero()) {
+                continue;
+            }
             float duration = ((float) stat.durationSec()) +
                     (((float) stat.durationNano()) / TimeUnit.SECONDS.toNanos(1));
             float rateRx = stat.bytesReceived() * 8 / duration;
@@ -154,6 +164,9 @@ public class DevicePortStatsCommand extends DevicesListCommand {
             if (portNumber != null && stat.port() != portNumber) {
                 continue;
             }
+            if (nonzero && stat.isZero()) {
+                continue;
+            }
             float duration = ((float) stat.durationSec()) +
                     (((float) stat.durationNano()) / TimeUnit.SECONDS.toNanos(1));
             float rateRx = stat.bytesReceived() * 8 / duration;

@@ -29,7 +29,7 @@ public class GroupStatusCompleter extends AbstractChoicesCompleter {
     protected List<String> choices() {
         List<String> strings = Lists.newArrayList();
         for (Group.GroupState groupState : Group.GroupState.values()) {
-            strings.add(groupState.toString());
+            strings.add(groupState.toString().toLowerCase());
         }
         strings.add(GroupsListCommand.ANY);
         return strings;

@@ -142,6 +142,16 @@ public final class DefaultPortStatistics implements PortStatistics {
     }
 
     @Override
+    public boolean isZero() {
+        return  bytesReceived() == 0 &&
+                bytesSent() == 0 &&
+                packetsReceived() == 0 &&
+                packetsRxDropped() == 0 &&
+                packetsSent() == 0 &&
+                packetsTxDropped() == 0;
+    }
+
+    @Override
     public String toString() {
         StringBuilder sb = new StringBuilder("device: " + deviceId + ", ");
 
@@ -343,4 +353,5 @@ public final class DefaultPortStatistics implements PortStatistics {
         }
 
     }
+
 }

@@ -97,4 +97,11 @@ public interface PortStatistics {
      */
     long durationNano();
 
+    /**
+     * Returns true if all the port stats are zero, excluding TxErrors and RxErrors.
+     *
+     * @return boolean true if all port stats are zero
+     */
+    boolean isZero();
+
 }

@@ -292,9 +292,11 @@ public class Ofdpa2Pipeline extends AbstractHandlerBehaviour implements Pipeline
         } else {
             log.warn("No key defined in filtering objective from app: {}. Not"
                     + "processing filtering objective", applicationId);
-            fail(filt, ObjectiveError.UNKNOWN);
+            fail(filt, ObjectiveError.BADPARAMS);
             return;
         }
+        log.debug("Received filtering objective for dev/port: {}/{}", deviceId,
+                 portCriterion.port());
         // convert filtering conditions for switch-intfs into flowrules
         FlowRuleOperations.Builder ops = FlowRuleOperations.builder();
         for (Criterion criterion : filt.conditions()) {
@@ -333,7 +335,9 @@ public class Ofdpa2Pipeline extends AbstractHandlerBehaviour implements Pipeline
         }
 
         if (ethCriterion == null || ethCriterion.mac().equals(MacAddress.NONE)) {
-            log.debug("filtering objective missing dstMac, cannot program TMAC table");
+            log.warn("filtering objective missing dstMac, cannot program TMAC table");
+            fail(filt, ObjectiveError.BADPARAMS);
+            return;
         } else {
             for (FlowRule tmacRule : processEthDstFilter(portCriterion, ethCriterion,
                                                          vidCriterion, assignedVlan,
@@ -345,8 +349,10 @@ public class Ofdpa2Pipeline extends AbstractHandlerBehaviour implements Pipeline
         }
 
         if (ethCriterion == null || vidCriterion == null) {
-            log.debug("filtering objective missing dstMac or VLAN, "
+            log.warn("filtering objective missing dstMac or VLAN, "
                     + "cannot program VLAN Table");
+            fail(filt, ObjectiveError.BADPARAMS);
+            return;
         } else {
             /*
              * NOTE: Separate vlan filtering rules and assignment rules
@@ -1079,17 +1085,26 @@ public class Ofdpa2Pipeline extends AbstractHandlerBehaviour implements Pipeline
     }
 
     protected static VlanId readVlanFromSelector(TrafficSelector selector) {
+        if (selector == null) {
+            return null;
+        }
         Criterion criterion = selector.getCriterion(Criterion.Type.VLAN_VID);
         return (criterion == null)
                 ? null : ((VlanIdCriterion) criterion).vlanId();
     }
 
     protected static IpPrefix readIpDstFromSelector(TrafficSelector selector) {
+        if (selector == null) {
+            return null;
+        }
         Criterion criterion = selector.getCriterion(Criterion.Type.IPV4_DST);
         return (criterion == null) ? null : ((IPCriterion) criterion).ip();
     }
 
     private static VlanId readVlanFromTreatment(TrafficTreatment treatment) {
+        if (treatment == null) {
+            return null;
+        }
         for (Instruction i : treatment.allInstructions()) {
             if (i instanceof ModVlanIdInstruction) {
                 return ((ModVlanIdInstruction) i).vlanId();