Maintain OFPortDesc up-to-date

- OFPortDesc cache managed by AbstractOpenFlowSwitch was not always maintained properly.
  reorganized data structure to maintain per OFPortDesc, last known instance

Change-Id: I1b26d7ca284e44bf9744c30374394c581653d78f

drivers/default/src/main/java/org/onosproject/driver/handshaker/DefaultSwitchHandshaker.java

@@ -18,13 +18,8 @@ package org.onosproject.driver.handshaker;
 import org.onosproject.openflow.controller.driver.AbstractOpenFlowSwitch;
 import org.projectfloodlight.openflow.protocol.OFFlowAdd;
 import org.projectfloodlight.openflow.protocol.OFMessage;
-import org.projectfloodlight.openflow.protocol.OFPortDesc;
 import org.projectfloodlight.openflow.protocol.OFVersion;
 
-import java.util.Collections;
-import java.util.List;
-import java.util.stream.Collectors;
-
 /**
  * Default driver to fallback on if no other driver is available.
  */
@@ -53,15 +48,4 @@ public class DefaultSwitchHandshaker extends AbstractOpenFlowSwitch {
     public boolean isDriverHandshakeComplete() {
         return true;
     }
-
-    @Override
-    public List<OFPortDesc> getPorts() {
-        if (this.factory().getVersion() == OFVersion.OF_10) {
-            return Collections.unmodifiableList(features.getPorts());
-        } else {
-            return Collections.unmodifiableList(
-                    ports.stream().flatMap(p -> p.getEntries().stream())
-                            .collect(Collectors.toList()));
-        }
-    }
 }

drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OFOpticalSwitch13.java

@@ -29,8 +29,6 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.stream.Collectors;
-
 import org.onosproject.net.Device;
 import org.onosproject.openflow.controller.OpenFlowOpticalSwitch;
 import org.onosproject.openflow.controller.PortDescPropertyType;
@@ -159,9 +157,7 @@ public class OFOpticalSwitch13 extends AbstractOpenFlowSwitch implements OpenFlo
      */
     @Override
     public List<OFPortDesc> getPorts() {
-        return ImmutableList.copyOf(
+        return super.getPorts();
-                ports.stream().flatMap(p -> p.getEntries().stream())
-                .collect(Collectors.toList()));
     }
 
     @Override

drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OfOpticalSwitchImplLinc13.java

@@ -33,7 +33,6 @@ import org.projectfloodlight.openflow.protocol.OFMessage;
 import org.projectfloodlight.openflow.protocol.OFObject;
 import org.projectfloodlight.openflow.protocol.OFPortDesc;
 import org.projectfloodlight.openflow.protocol.OFPortDescPropOpticalTransport;
-import org.projectfloodlight.openflow.protocol.OFPortDescStatsReply;
 import org.projectfloodlight.openflow.protocol.OFPortOptical;
 import org.projectfloodlight.openflow.protocol.OFStatsReply;
 import org.projectfloodlight.openflow.protocol.OFStatsType;
@@ -43,6 +42,7 @@ import org.projectfloodlight.openflow.protocol.match.Match;
 import org.projectfloodlight.openflow.protocol.match.MatchField;
 import org.projectfloodlight.openflow.protocol.oxm.OFOxmExpOchSigId;
 import org.projectfloodlight.openflow.types.CircuitSignalID;
+import org.projectfloodlight.openflow.types.OFPort;
 import org.projectfloodlight.openflow.types.U8;
 
 import java.io.IOException;
@@ -266,16 +266,9 @@ public class OfOpticalSwitchImplLinc13 extends AbstractOpenFlowSwitch implements
      * @param port given port number
      * @return true if the port is a tap (OCh), false otherwise (OMS port)
      */
-    private boolean isOChPort(long port) {
+    private boolean isOChPort(OFPort port) {
-        for (OFPortDescStatsReply reply : this.ports) {
-            for (OFPortDesc p : reply.getEntries()) {
-                if (p.getPortNo().getPortNumber() == port) {
-                    return true;
-                }
-            }
-        }
 
-        return false;
+        return portDescs().containsKey(port);
     }
 
     /**
@@ -324,7 +317,7 @@ public class OfOpticalSwitchImplLinc13 extends AbstractOpenFlowSwitch implements
             short signalType;
 
             // FIXME: use constants once loxi has full optical extensions
-            if (isOChPort(p.getPortNo().getPortNumber())) {
+            if (isOChPort(p.getPortNo())) {
                 signalType = 5;      // OCH port
             } else {
                 signalType = 2;      // OMS port

drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OplinkSwitchHandshaker.java

@@ -36,8 +36,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.stream.Collectors;
-
 import org.onosproject.net.DefaultAnnotations;
 import org.onosproject.net.Device;
 import org.onosproject.net.device.DefaultPortDescription;
@@ -183,9 +181,7 @@ public class OplinkSwitchHandshaker extends AbstractOpenFlowSwitch implements Op
      */
     @Override
     public List<OFPortDesc> getPorts() {
-        return ImmutableList.copyOf(
+        return super.getPorts();
-                ports.stream().flatMap(p -> p.getEntries().stream())
-                .collect(Collectors.toList()));
     }
 
     @Override

protocols/openflow/api/src/main/java/org/onosproject/openflow/controller/driver/AbstractOpenFlowSwitch.java

@@ -17,6 +17,7 @@
 package org.onosproject.openflow.controller.driver;
 
 import com.google.common.base.MoreObjects;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 
 import org.onosproject.net.Device;
@@ -36,16 +37,22 @@ import org.projectfloodlight.openflow.protocol.OFMeterFeaturesStatsReply;
 import org.projectfloodlight.openflow.protocol.OFNiciraControllerRoleRequest;
 import org.projectfloodlight.openflow.protocol.OFPortDesc;
 import org.projectfloodlight.openflow.protocol.OFPortDescStatsReply;
+import org.projectfloodlight.openflow.protocol.OFPortReason;
 import org.projectfloodlight.openflow.protocol.OFPortStatus;
 import org.projectfloodlight.openflow.protocol.OFRoleReply;
 import org.projectfloodlight.openflow.protocol.OFRoleRequest;
+import org.projectfloodlight.openflow.protocol.OFType;
 import org.projectfloodlight.openflow.protocol.OFVersion;
+import org.projectfloodlight.openflow.types.OFPort;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
@@ -68,9 +75,15 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
     private OpenFlowAgent agent;
     private final AtomicInteger xidCounter = new AtomicInteger(0);
 
-    private OFVersion ofVersion;
     private OFFactory ofFactory;
 
+    // known port descriptions maintained by
+    // (all)    : OFPortStatus
+    // <  OF1.3 : feature reply
+    // >= OF1.3 : multipart stats reply (OFStatsReply:PORT_DESC)
+    private Map<OFPort, OFPortDesc> portDescs = new ConcurrentHashMap<>();
+
+    @Deprecated // in 1.13.0
     protected List<OFPortDescStatsReply> ports = Lists.newCopyOnWriteArrayList();
 
     protected boolean tableFull;
@@ -80,9 +93,12 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
     // TODO this is accessed from multiple threads, but volatile may have performance implications
     protected volatile RoleState role;
 
+    @Deprecated // in 1.13.0 to be made private after deprecation
     protected OFFeaturesReply features;
+    @Deprecated // in 1.13.0 to be made private after deprecation
     protected OFDescStatsReply desc;
 
+    @Deprecated // in 1.13.0 to be made private after deprecation
     protected OFMeterFeaturesStatsReply meterfeatures;
 
     // messagesPendingMastership is used as synchronization variable for
@@ -95,7 +111,6 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
     public void init(Dpid dpid, OFDescStatsReply desc, OFVersion ofv) {
         this.dpid = dpid;
         this.desc = desc;
-        this.ofVersion = ofv;
     }
 
     //************************
@@ -226,7 +241,6 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
 
     @Override
     public final void setOFVersion(OFVersion ofV) {
-        this.ofVersion = ofV;
         this.ofFactory = OFFactories.getFactory(ofV);
     }
 
@@ -238,6 +252,10 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
     @Override
     public void setFeaturesReply(OFFeaturesReply featuresReply) {
         this.features = featuresReply;
+        if (featuresReply.getVersion().compareTo(OFVersion.OF_13) < 0) {
+            // before OF 1.3, feature reply contains OFPortDescs
+            replacePortDescsWith(featuresReply.getPorts());
+        }
     }
 
     @Override
@@ -260,6 +278,16 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
     public final void handleMessage(OFMessage m) {
         if (this.role == RoleState.MASTER || m instanceof OFPortStatus) {
             try {
+                // TODO revisit states other than ports should
+                // also ignore role state.
+                if (m.getType() == OFType.PORT_STATUS) {
+                    OFPortStatus portStatus = (OFPortStatus) m;
+                    if (portStatus.getReason() == OFPortReason.DELETE) {
+                        portDescs.remove(portStatus.getDesc().getPortNo());
+                    } else {
+                        portDescs.put(portStatus.getDesc().getPortNo(), portStatus.getDesc());
+                    }
+                }
                 this.agent.processMessage(dpid, m);
             } catch (Exception e) {
                 log.warn("Unhandled exception processing {}@{}", m, dpid, e);
@@ -323,11 +351,32 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
 
     @Override
     public void setPortDescReply(OFPortDescStatsReply portDescReply) {
+        portDescReply.getEntries().forEach(pd -> portDescs.put(pd.getPortNo(), pd));
+
+        // maintaining only for backward compatibility, to be removed
         this.ports.add(portDescReply);
     }
 
+    protected void replacePortDescsWith(Collection<OFPortDesc> allPorts) {
+        Map<OFPort, OFPortDesc> ports = new ConcurrentHashMap<>(allPorts.size());
+        allPorts.forEach(pd -> ports.put(pd.getPortNo(), pd));
+        // replace all
+        this.portDescs = ports;
+    }
+
+    protected Map<OFPort, OFPortDesc> portDescs() {
+        return portDescs;
+    }
+
+    // only called once during handshake WAIT_DESCRIPTION_STAT_REPLY
     @Override
     public void setPortDescReplies(List<OFPortDescStatsReply> portDescReplies) {
+        replacePortDescsWith(portDescReplies.stream()
+                       .map(OFPortDescStatsReply::getEntries)
+                       .flatMap(List::stream)
+                       .collect(Collectors.toList()));
+
+        // maintaining only for backward compatibility, to be removed
         this.ports.addAll(portDescReplies);
     }
 
@@ -466,9 +515,7 @@ public abstract class AbstractOpenFlowSwitch extends AbstractHandlerBehaviour
 
     @Override
     public List<OFPortDesc> getPorts() {
-        return this.ports.stream()
+        return ImmutableList.copyOf(portDescs.values());
-                  .flatMap(portReply -> portReply.getEntries().stream())
-                  .collect(Collectors.toList());
     }
 
     @Override

providers/openflow/device/src/main/java/org/onosproject/provider/of/device/impl/OpenFlowDeviceProvider.java

@@ -556,12 +556,13 @@ public class OpenFlowDeviceProvider extends AbstractProvider implements DevicePr
          * @return list of portdescriptions
          */
         private List<PortDescription> buildPortDescriptions(OpenFlowSwitch sw) {
-            final List<PortDescription> portDescs = new ArrayList<>(sw.getPorts().size());
+            List<OFPortDesc> ofPorts = sw.getPorts();
+            final List<PortDescription> portDescs = new ArrayList<>(ofPorts.size());
             if (!((Device.Type.ROADM.equals(sw.deviceType())) ||
                     (Device.Type.OTN.equals(sw.deviceType())) ||
                     (Device.Type.OPTICAL_AMPLIFIER.equals(sw.deviceType())))) {
                 // build regular (=non-optical) Device ports
-                sw.getPorts().forEach(port -> portDescs.add(buildPortDescription(port)));
+                ofPorts.forEach(port -> portDescs.add(buildPortDescription(port)));
             }
 
             // TODO handle Optical Device, but plain OF devices(1.4 and later)