From f83c8cfdf6a8101acb96e7121d22b2605496755e Mon Sep 17 00:00:00 2001 From: Yuta HIGUCHI Date: Sun, 17 Dec 2017 14:33:49 -0800 Subject: [PATCH] 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 --- .../handshaker/DefaultSwitchHandshaker.java | 16 ----- .../optical/handshaker/OFOpticalSwitch13.java | 6 +- .../handshaker/OfOpticalSwitchImplLinc13.java | 15 ++--- .../handshaker/OplinkSwitchHandshaker.java | 6 +- .../driver/AbstractOpenFlowSwitch.java | 59 +++++++++++++++++-- .../device/impl/OpenFlowDeviceProvider.java | 5 +- 6 files changed, 62 insertions(+), 45 deletions(-) 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 86a87e3d97..0b1dc2be42 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 @@ 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 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 123ab9b1cd..172e82e60e 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.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 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 3691cfd40b..da7597cb98 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.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) { - 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 @@ 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 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 16808d13b9..6e315cb8ce 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.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 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 323e03a876..0ec80712a1 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,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 portDescs = new ConcurrentHashMap<>(); + + @Deprecated // in 1.13.0 protected List 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 allPorts) { + Map ports = new ConcurrentHashMap<>(allPorts.size()); + allPorts.forEach(pd -> ports.put(pd.getPortNo(), pd)); + // replace all + this.portDescs = ports; + } + + protected Map portDescs() { + return portDescs; + } + + // only called once during handshake WAIT_DESCRIPTION_STAT_REPLY @Override public void setPortDescReplies(List 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 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 b303308238..bac8b19d11 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 @@ public class OpenFlowDeviceProvider extends AbstractProvider implements DevicePr * @return list of portdescriptions */ private List buildPortDescriptions(OpenFlowSwitch sw) { - final List portDescs = new ArrayList<>(sw.getPorts().size()); + List ofPorts = sw.getPorts(); + final List 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)