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
(cherry picked from commit f83c8cfdf6a8101acb96e7121d22b2605496755e)
diff --git a/drivers/default/src/main/java/org/onosproject/driver/handshaker/DefaultSwitchHandshaker.java b/drivers/default/src/main/java/org/onosproject/driver/handshaker/DefaultSwitchHandshaker.java
index 86a87e3..0b1dc2b 100644
--- a/drivers/default/src/main/java/org/onosproject/driver/handshaker/DefaultSwitchHandshaker.java
+++ b/drivers/default/src/main/java/org/onosproject/driver/handshaker/DefaultSwitchHandshaker.java
@@ -18,13 +18,8 @@
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 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()));
- }
- }
}
diff --git a/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OFOpticalSwitch13.java b/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OFOpticalSwitch13.java
index 123ab9b..172e82e 100644
--- a/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OFOpticalSwitch13.java
+++ b/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OFOpticalSwitch13.java
@@ -29,8 +29,6 @@
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 @@
*/
@Override
public List<OFPortDesc> getPorts() {
- return ImmutableList.copyOf(
- ports.stream().flatMap(p -> p.getEntries().stream())
- .collect(Collectors.toList()));
+ return super.getPorts();
}
@Override
diff --git a/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OfOpticalSwitchImplLinc13.java b/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OfOpticalSwitchImplLinc13.java
index 3691cfd..da7597c 100644
--- a/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OfOpticalSwitchImplLinc13.java
+++ b/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OfOpticalSwitchImplLinc13.java
@@ -33,7 +33,6 @@
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.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 @@
* @param port given port number
* @return true if the port is a tap (OCh), false otherwise (OMS port)
*/
- private boolean isOChPort(long port) {
- for (OFPortDescStatsReply reply : this.ports) {
- for (OFPortDesc p : reply.getEntries()) {
- if (p.getPortNo().getPortNumber() == port) {
- return true;
- }
- }
- }
+ private boolean isOChPort(OFPort port) {
- return false;
+ return portDescs().containsKey(port);
}
/**
@@ -324,7 +317,7 @@
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
diff --git a/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OplinkSwitchHandshaker.java b/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OplinkSwitchHandshaker.java
index 16808d1..6e315cb 100644
--- a/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OplinkSwitchHandshaker.java
+++ b/drivers/optical/src/main/java/org/onosproject/driver/optical/handshaker/OplinkSwitchHandshaker.java
@@ -36,8 +36,6 @@
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 @@
*/
@Override
public List<OFPortDesc> getPorts() {
- return ImmutableList.copyOf(
- ports.stream().flatMap(p -> p.getEntries().stream())
- .collect(Collectors.toList()));
+ return super.getPorts();
}
@Override
diff --git a/protocols/openflow/api/src/main/java/org/onosproject/openflow/controller/driver/AbstractOpenFlowSwitch.java b/protocols/openflow/api/src/main/java/org/onosproject/openflow/controller/driver/AbstractOpenFlowSwitch.java
index 9865dd1..0ec8071 100644
--- a/protocols/openflow/api/src/main/java/org/onosproject/openflow/controller/driver/AbstractOpenFlowSwitch.java
+++ b/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,17 +37,22 @@
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.ArrayList;
+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;
@@ -69,10 +75,16 @@
private OpenFlowAgent agent;
private final AtomicInteger xidCounter = new AtomicInteger(0);
- private OFVersion ofVersion;
private OFFactory ofFactory;
- protected List<OFPortDescStatsReply> ports = new ArrayList<>();
+ // 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;
@@ -81,9 +93,12 @@
// 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
@@ -96,7 +111,6 @@
public void init(Dpid dpid, OFDescStatsReply desc, OFVersion ofv) {
this.dpid = dpid;
this.desc = desc;
- this.ofVersion = ofv;
}
//************************
@@ -227,7 +241,6 @@
@Override
public final void setOFVersion(OFVersion ofV) {
- this.ofVersion = ofV;
this.ofFactory = OFFactories.getFactory(ofV);
}
@@ -239,6 +252,10 @@
@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
@@ -261,6 +278,16 @@
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);
@@ -324,11 +351,32 @@
@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);
}
@@ -467,9 +515,7 @@
@Override
public List<OFPortDesc> getPorts() {
- return this.ports.stream()
- .flatMap(portReply -> portReply.getEntries().stream())
- .collect(Collectors.toList());
+ return ImmutableList.copyOf(portDescs.values());
}
@Override
diff --git a/providers/openflow/device/src/main/java/org/onosproject/provider/of/device/impl/OpenFlowDeviceProvider.java b/providers/openflow/device/src/main/java/org/onosproject/provider/of/device/impl/OpenFlowDeviceProvider.java
index 51213de..20c9747 100644
--- a/providers/openflow/device/src/main/java/org/onosproject/provider/of/device/impl/OpenFlowDeviceProvider.java
+++ b/providers/openflow/device/src/main/java/org/onosproject/provider/of/device/impl/OpenFlowDeviceProvider.java
@@ -556,12 +556,13 @@
* @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)