Gather topology element info to TopologyImpl
- Moved all the self-contained topology elements (*Event) to
TopologyImpl. (ONOS-1651)
Now {Switch, Port, Link, Host}Impl is just a handler attached to
TopologyImpl.
- BugFix: TopologyManager.addHost(HostEvent)
HostEvent could be pushed to reorder queue multiple times,
if multiple attachment point was given.
- BugFix: TopologyManager.{addLink, removePort}
Properly handle if Host attachment point was removed as side-effect.
- BugFix: Copy HostEvent#lastSeenTime
- BugFix: Event instance notified to listeners (api*Events) should be
the event which was/will be in the replica. (TopologyManager)
- Added/Modified debug log in TopologyManager so that log will be in
same format for each event type.
"{Added, Update, Removed} <Self-contained>"
- Removed backdoor method and use TestUtils instead.
Change-Id: If053d6f11f39574a188e7a52cb6194114f8afe5d
diff --git a/src/main/java/net/onrc/onos/core/topology/HostEvent.java b/src/main/java/net/onrc/onos/core/topology/HostEvent.java
index 4c8f78d..c35764e 100644
--- a/src/main/java/net/onrc/onos/core/topology/HostEvent.java
+++ b/src/main/java/net/onrc/onos/core/topology/HostEvent.java
@@ -59,6 +59,7 @@
super(original);
this.mac = original.mac;
this.attachmentPoints = new ArrayList<>(original.attachmentPoints);
+ this.lastSeenTime = original.lastSeenTime;
}
diff --git a/src/main/java/net/onrc/onos/core/topology/HostImpl.java b/src/main/java/net/onrc/onos/core/topology/HostImpl.java
index 469d335..d0964a1 100644
--- a/src/main/java/net/onrc/onos/core/topology/HostImpl.java
+++ b/src/main/java/net/onrc/onos/core/topology/HostImpl.java
@@ -9,50 +9,29 @@
import net.onrc.onos.core.util.SwitchPort;
/**
- * Host Object stored in In-memory Topology.
+ * Handler to Host object stored in In-memory Topology snapshot.
+ * <p/>
*/
public class HostImpl extends TopologyObject implements Host {
- //////////////////////////////////////////////////////
- /// Topology element attributes
- /// - any changes made here needs to be replicated.
- //////////////////////////////////////////////////////
- private HostEvent deviceObj;
+ private final MACAddress id;
/**
- * Creates a Host object based on {@link HostEvent}.
- *
- * @param topology Topology instance this object belongs to
- * @param scHost self contained {@link HostEvent}
- */
- public HostImpl(Topology topology, HostEvent scHost) {
- super(topology);
- Validate.notNull(scHost);
-
- // TODO should we assume deviceObj is already frozen before this call
- // or expect attribute update will happen after .
- if (scHost.isFrozen()) {
- this.deviceObj = scHost;
- } else {
- this.deviceObj = new HostEvent(scHost);
- this.deviceObj.freeze();
- }
- }
-
- /**
- * Creates a Host object with empty attributes.
+ * Creates a Host handler object.
*
* @param topology Topology instance this object belongs to
* @param mac MAC address of the host
*/
- public HostImpl(Topology topology, MACAddress mac) {
- this(topology, new HostEvent(mac).freeze());
+ HostImpl(TopologyInternal topology, MACAddress mac) {
+ super(topology);
+ Validate.notNull(mac);
+ this.id = mac;
}
@Override
public MACAddress getMacAddress() {
- return this.deviceObj.getMac();
+ return id;
}
@Override
@@ -60,7 +39,7 @@
List<Port> ports = new ArrayList<>();
topology.acquireReadLock();
try {
- for (SwitchPort swp : this.deviceObj.getAttachmentPoints()) {
+ for (SwitchPort swp : getHostEvent().getAttachmentPoints()) {
Port p = this.topology.getPort(swp);
if (p != null) {
ports.add(p);
@@ -74,7 +53,7 @@
@Override
public long getLastSeenTime() {
- return deviceObj.getLastSeenTime();
+ return this.topology.getHostEvent(id).getLastSeenTime();
}
@Override
@@ -82,45 +61,16 @@
return getMacAddress().toString();
}
- // TODO we may no longer need this. confirm and delete later.
- void setLastSeenTime(long lastSeenTime) {
- // XXX Following will make this instance thread unsafe. Need to use AtomicRef.
- HostEvent updated = new HostEvent(this.deviceObj);
- updated.setLastSeenTime(lastSeenTime);
- updated.freeze();
- this.deviceObj = updated;
- }
-
/**
- * Only {@link TopologyManager} should use this method.
+ * Gets the current HostEvent.
*
- * @param port the port that the device is attached to
+ * @return HostEvent
*/
- void addAttachmentPoint(Port port) {
- // XXX Following will make this instance thread unsafe. Need to use AtomicRef.
- HostEvent updated = new HostEvent(this.deviceObj);
- updated.removeAttachmentPoint(port.asSwitchPort());
- updated.addAttachmentPoint(port.asSwitchPort());
- updated.freeze();
- this.deviceObj = updated;
+ private HostEvent getHostEvent() {
+ return this.topology.getHostEvent(id);
}
/**
- * Only {@link TopologyManager} should use this method.
- *
- * @param port the port that the device is attached to
- */
- boolean removeAttachmentPoint(Port port) {
- // XXX Following will make this instance thread unsafe. Need to use AtomicRef.
- HostEvent updated = new HostEvent(this.deviceObj);
- final boolean result = updated.removeAttachmentPoint(port.asSwitchPort());
- updated.freeze();
- this.deviceObj = updated;
- return result;
- }
-
-
- /**
* Returns the type of topology object.
*
* @return the type of the topology object
diff --git a/src/main/java/net/onrc/onos/core/topology/LinkImpl.java b/src/main/java/net/onrc/onos/core/topology/LinkImpl.java
index 5ad0194..8379279 100644
--- a/src/main/java/net/onrc/onos/core/topology/LinkImpl.java
+++ b/src/main/java/net/onrc/onos/core/topology/LinkImpl.java
@@ -7,63 +7,36 @@
import org.apache.commons.lang.Validate;
/**
- * Link Object stored in In-memory Topology.
+ * Handler to Link object stored in In-memory Topology snapshot.
* <p/>
- * TODO REMOVE following design memo: This object itself may hold the DBObject,
- * but this Object itself will not issue any read/write to the DataStore.
*/
public class LinkImpl extends TopologyObject implements Link {
- //////////////////////////////////////////////////////
- /// Topology element attributes
- /// - any changes made here needs to be replicated.
- //////////////////////////////////////////////////////
- private LinkEvent linkObj;
+ private final LinkTuple id;
/**
- * Creates a Link object based on {@link LinkEvent}.
+ * Creates a Link handler object.
*
* @param topology Topology instance this object belongs to
- * @param scPort self contained {@link LinkEvent}
+ * @param linkTuple Link identifier
*/
- public LinkImpl(Topology topology, LinkEvent scPort) {
+ LinkImpl(TopologyInternal topology, LinkTuple linkTuple) {
super(topology);
- Validate.notNull(scPort);
-
- // TODO should we assume linkObj is already frozen before this call
- // or expect attribute update will happen after .
- if (scPort.isFrozen()) {
- this.linkObj = scPort;
- } else {
- this.linkObj = new LinkEvent(scPort);
- this.linkObj.freeze();
- }
- }
-
- /**
- * Creates a Link object with empty attributes.
- *
- * @param topology Topology instance this object belongs to
- * @param srcPort source port
- * @param dstPort destination port
- */
- public LinkImpl(Topology topology, Port srcPort, Port dstPort) {
- this(topology,
- new LinkEvent(srcPort.asSwitchPort(),
- dstPort.asSwitchPort()).freeze());
+ Validate.notNull(linkTuple);
+ this.id = linkTuple;
}
@Override
public LinkTuple getLinkTuple() {
- return linkObj.getLinkTuple();
+ return id;
}
@Override
public Switch getSrcSwitch() {
topology.acquireReadLock();
try {
- return topology.getSwitch(linkObj.getSrc().getDpid());
+ return topology.getSwitch(id.getSrc().getDpid());
} finally {
topology.releaseReadLock();
}
@@ -73,7 +46,7 @@
public Port getSrcPort() {
topology.acquireReadLock();
try {
- return topology.getPort(linkObj.getSrc());
+ return topology.getPort(id.getSrc());
} finally {
topology.releaseReadLock();
}
@@ -83,7 +56,7 @@
public Switch getDstSwitch() {
topology.acquireReadLock();
try {
- return topology.getSwitch(linkObj.getDst().getDpid());
+ return topology.getSwitch(id.getDst().getDpid());
} finally {
topology.releaseReadLock();
}
@@ -93,7 +66,7 @@
public Port getDstPort() {
topology.acquireReadLock();
try {
- return topology.getPort(linkObj.getDst());
+ return topology.getPort(id.getDst());
} finally {
topology.releaseReadLock();
}
@@ -107,38 +80,12 @@
@Override
public Double getCapacity() {
- return this.linkObj.getCapacity();
+ return this.topology.getLinkEvent(id).getCapacity();
}
- void setCapacity(Double capacity) {
- if (this.linkObj.isFrozen()) {
- this.linkObj = new LinkEvent(this.linkObj);
- this.linkObj.setCapacity(capacity);
- this.linkObj.freeze();
- } else {
- this.linkObj.setCapacity(capacity);
- }
- }
-
- // XXX actually replaces everything
- void replaceStringAttributes(LinkEvent updated) {
- Validate.isTrue(this.linkObj.getSrc().equals(updated.getSrc()),
- "Wrong LinkEvent given.");
- Validate.isTrue(this.linkObj.getDst().equals(updated.getDst()),
- "Wrong LinkEvent given.");
-
- // XXX simply replacing whole self-contained object for now
- if (updated.isFrozen()) {
- this.linkObj = updated;
- } else {
- this.linkObj = new LinkEvent(updated).freeze();
- }
- }
-
-
@Override
public String getStringAttribute(String attr) {
- return linkObj.getStringAttribute(attr);
+ return this.topology.getLinkEvent(id).getStringAttribute(attr);
}
@Override
@@ -153,7 +100,7 @@
@Override
public Map<String, String> getAllStringAttributes() {
- return linkObj.getAllStringAttributes();
+ return this.topology.getLinkEvent(id).getAllStringAttributes();
}
@Override
diff --git a/src/main/java/net/onrc/onos/core/topology/Port.java b/src/main/java/net/onrc/onos/core/topology/Port.java
index 4758b16..50feb99 100644
--- a/src/main/java/net/onrc/onos/core/topology/Port.java
+++ b/src/main/java/net/onrc/onos/core/topology/Port.java
@@ -120,11 +120,10 @@
*/
public Collection<Link> getIncomingLinks();
- // XXX Iterable or Collection?
/**
* Gets all the devices attached to this port.
*
* @return {@link Host}s attached to this port
*/
- public Iterable<Host> getHosts();
+ public Collection<Host> getHosts();
}
diff --git a/src/main/java/net/onrc/onos/core/topology/PortImpl.java b/src/main/java/net/onrc/onos/core/topology/PortImpl.java
index 935b3c5..4e0da5e 100644
--- a/src/main/java/net/onrc/onos/core/topology/PortImpl.java
+++ b/src/main/java/net/onrc/onos/core/topology/PortImpl.java
@@ -10,61 +10,35 @@
import net.onrc.onos.core.util.SwitchPort;
/**
- * Port Object stored in In-memory Topology.
+ * Handler to Port object stored in In-memory Topology snapshot.
* <p/>
- * TODO REMOVE following design memo: This object itself may hold the DBObject,
- * but this Object itself will not issue any read/write to the DataStore.
*/
public class PortImpl extends TopologyObject implements Port {
- //////////////////////////////////////////////////////
- /// Topology element attributes
- /// - any changes made here needs to be replicated.
- //////////////////////////////////////////////////////
- private PortEvent portObj;
+ private final SwitchPort id;
/**
- * Creates a Port object based on {@link PortEvent}.
- *
- * @param topology Topology instance this object belongs to
- * @param scPort self contained {@link PortEvent}
- */
- public PortImpl(Topology topology, PortEvent scPort) {
- super(topology);
- Validate.notNull(scPort);
-
- // TODO should we assume portObj is already frozen before this call
- // or expect attribute update will happen after .
- if (scPort.isFrozen()) {
- this.portObj = scPort;
- } else {
- this.portObj = new PortEvent(scPort);
- this.portObj.freeze();
- }
- }
-
- /**
- * Creates a Port object with empty attributes.
+ * Creates a Port handler object.
*
* @param topology Topology instance this object belongs to
* @param switchPort SwitchPort
*/
- public PortImpl(Topology topology, SwitchPort switchPort) {
- this(topology, new PortEvent(switchPort).freeze());
+ PortImpl(TopologyInternal topology, SwitchPort switchPort) {
+ super(topology);
+ Validate.notNull(switchPort);
+ this.id = switchPort;
}
/**
- * Creates a Port object with empty attributes.
+ * Creates a Port handler object.
*
* @param topology Topology instance this object belongs to
* @param dpid DPID
* @param number PortNumber
*/
- public PortImpl(Topology topology, Dpid dpid, PortNumber number) {
+ PortImpl(TopologyInternal topology, Dpid dpid, PortNumber number) {
this(topology, new SwitchPort(dpid, number));
- Validate.notNull(dpid);
- Validate.notNull(number);
}
@Override
@@ -79,7 +53,7 @@
@Override
public SwitchPort asSwitchPort() {
- return portObj.getSwitchPort();
+ return id;
}
@Override
@@ -87,12 +61,6 @@
return getStringAttribute(PortEvent.DESCRIPTION, "");
}
- void setDescription(String description) {
-// portObj.createStringAttribute(attr, value);
- // TODO implement using attributes
- throw new UnsupportedOperationException("Not implemented yet");
- }
-
@Override
public Long getHardwareAddress() {
// TODO implement using attributes?
@@ -170,7 +138,7 @@
}
@Override
- public Iterable<Host> getHosts() {
+ public Collection<Host> getHosts() {
topology.acquireReadLock();
try {
return topology.getHosts(this.asSwitchPort());
@@ -179,21 +147,9 @@
}
}
- void replaceStringAttributes(PortEvent updated) {
- Validate.isTrue(this.asSwitchPort().equals(updated.getSwitchPort()),
- "Wrong PortEvent given.");
-
- // XXX simply replacing whole self-contained object for now
- if (updated.isFrozen()) {
- this.portObj = updated;
- } else {
- this.portObj = new PortEvent(updated).freeze();
- }
- }
-
@Override
public String getStringAttribute(String attr) {
- return portObj.getStringAttribute(attr);
+ return this.topology.getPortEvent(id).getStringAttribute(attr);
}
@Override
@@ -208,7 +164,7 @@
@Override
public Map<String, String> getAllStringAttributes() {
- return portObj.getAllStringAttributes();
+ return this.topology.getPortEvent(id).getAllStringAttributes();
}
@Override
diff --git a/src/main/java/net/onrc/onos/core/topology/SwitchImpl.java b/src/main/java/net/onrc/onos/core/topology/SwitchImpl.java
index b0486b0..6be8b13 100644
--- a/src/main/java/net/onrc/onos/core/topology/SwitchImpl.java
+++ b/src/main/java/net/onrc/onos/core/topology/SwitchImpl.java
@@ -14,53 +14,30 @@
import org.apache.commons.lang.Validate;
/**
- * Switch Object stored in In-memory Topology.
+ * Handler to Switch object stored in In-memory Topology snapshot.
* <p/>
- * TODO REMOVE following design memo: This object itself may hold the DBObject,
- * but this Object itself will not issue any read/write to the DataStore.
+ *
*/
public class SwitchImpl extends TopologyObject implements Switch {
- //////////////////////////////////////////////////////
- /// Topology element attributes
- /// - any changes made here needs to be replicated.
- //////////////////////////////////////////////////////
- private SwitchEvent switchObj;
+ private final Dpid id;
/**
- * Creates a Switch object with empty attributes.
+ * Creates a Switch handler object.
*
* @param topology Topology instance this object belongs to
* @param dpid DPID
*/
- public SwitchImpl(Topology topology, Dpid dpid) {
- this(topology, new SwitchEvent(dpid).freeze());
- }
-
- /**
- * Creates a Switch object based on {@link SwitchEvent}.
- *
- * @param topology Topology instance this object belongs to
- * @param scSw self contained {@link SwitchEvent}
- */
- public SwitchImpl(Topology topology, SwitchEvent scSw) {
+ SwitchImpl(TopologyInternal topology, Dpid dpid) {
super(topology);
- Validate.notNull(scSw);
-
- // TODO should we assume switchObj is already frozen before this call
- // or expect attribute update will happen after .
- if (scSw.isFrozen()) {
- this.switchObj = scSw;
- } else {
- this.switchObj = new SwitchEvent(scSw);
- this.switchObj.freeze();
- }
+ Validate.notNull(dpid);
+ this.id = dpid;
}
@Override
public Dpid getDpid() {
- return switchObj.getDpid();
+ return this.id;
}
@Override
@@ -121,18 +98,6 @@
return hosts;
}
- void replaceStringAttributes(SwitchEvent updated) {
- Validate.isTrue(this.getDpid().equals(updated.getDpid()),
- "Wrong SwitchEvent given.");
-
- // XXX simply replacing whole self-contained object for now
- if (updated.isFrozen()) {
- this.switchObj = updated;
- } else {
- this.switchObj = new SwitchEvent(updated).freeze();
- }
- }
-
@Override
public Iterable<Link> getOutgoingLinks() {
LinkedList<Link> links = new LinkedList<Link>();
@@ -159,7 +124,7 @@
@Override
public String getStringAttribute(String attr) {
- return this.switchObj.getStringAttribute(attr);
+ return this.topology.getSwitchEvent(getDpid()).getStringAttribute(attr);
}
@Override
@@ -174,7 +139,7 @@
@Override
public Map<String, String> getAllStringAttributes() {
- return this.switchObj.getAllStringAttributes();
+ return this.topology.getSwitchEvent(getDpid()).getAllStringAttributes();
}
@Override
diff --git a/src/main/java/net/onrc/onos/core/topology/TopologyImpl.java b/src/main/java/net/onrc/onos/core/topology/TopologyImpl.java
index decab6b..d79d204 100644
--- a/src/main/java/net/onrc/onos/core/topology/TopologyImpl.java
+++ b/src/main/java/net/onrc/onos/core/topology/TopologyImpl.java
@@ -6,6 +6,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.locks.Lock;
@@ -16,6 +17,7 @@
import net.floodlightcontroller.util.MACAddress;
import net.onrc.onos.core.util.Dpid;
+import net.onrc.onos.core.util.LinkTuple;
import net.onrc.onos.core.util.PortNumber;
import net.onrc.onos.core.util.SwitchPort;
@@ -26,90 +28,113 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;
-public class TopologyImpl implements Topology {
- @SuppressWarnings("unused")
+
+/**
+ * Class to represent an instance of Topology Snapshot.
+ */
+public class TopologyImpl implements Topology, TopologyInternal {
+
private static final Logger log = LoggerFactory.getLogger(TopologyImpl.class);
+ // TODO Revisit Map types after implementing CoW/lock-free
+
// DPID -> Switch
- private final ConcurrentMap<Dpid, Switch> switches;
- // XXX may need to be careful when shallow copying.
- private final ConcurrentMap<Dpid, ConcurrentMap<PortNumber, Port>> ports;
+ private final ConcurrentMap<Dpid, SwitchEvent> switches;
+ private final ConcurrentMap<Dpid, ConcurrentMap<PortNumber, PortEvent>> ports;
// Index from Port to Host
- private final Multimap<SwitchPort, Host> hosts;
- private final ConcurrentMap<MACAddress, Host> mac2Host;
+ private final Multimap<SwitchPort, HostEvent> hosts;
+ private final ConcurrentMap<MACAddress, HostEvent> mac2Host;
// SwitchPort -> (type -> Link)
- private final ConcurrentMap<SwitchPort, ConcurrentMap<String, Link>> outgoingLinks;
- private final ConcurrentMap<SwitchPort, ConcurrentMap<String, Link>> incomingLinks;
+ private final ConcurrentMap<SwitchPort, ConcurrentMap<String, LinkEvent>> outgoingLinks;
+ private final ConcurrentMap<SwitchPort, ConcurrentMap<String, LinkEvent>> incomingLinks;
private ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
private Lock readLock = readWriteLock.readLock();
// TODO use the write lock after refactor
private Lock writeLock = readWriteLock.writeLock();
+ /**
+ * Create an empty Topology.
+ */
public TopologyImpl() {
// TODO: Does these object need to be stored in Concurrent Collection?
switches = new ConcurrentHashMap<>();
ports = new ConcurrentHashMap<>();
hosts = Multimaps.synchronizedMultimap(
- HashMultimap.<SwitchPort, Host>create());
+ HashMultimap.<SwitchPort, HostEvent>create());
mac2Host = new ConcurrentHashMap<>();
outgoingLinks = new ConcurrentHashMap<>();
incomingLinks = new ConcurrentHashMap<>();
}
+ /**
+ * Create a shallow copy of given Topology.
+ *
+ * @param original Topology
+ */
+ public TopologyImpl(TopologyImpl original) {
+ original.acquireReadLock();
+ try {
+ this.switches = new ConcurrentHashMap<>(original.switches);
+
+ // shallow copy Map in Map
+ this.ports = new ConcurrentHashMap<>(original.ports.size());
+ for (Entry<Dpid, ConcurrentMap<PortNumber, PortEvent>> entry
+ : original.ports.entrySet()) {
+ this.ports.put(entry.getKey(), new ConcurrentHashMap<>(entry.getValue()));
+ }
+
+ this.hosts = Multimaps.synchronizedMultimap(
+ HashMultimap.<SwitchPort, HostEvent>create(original.hosts));
+ this.mac2Host = new ConcurrentHashMap<>(original.mac2Host);
+
+ // shallow copy Map in Map
+ this.outgoingLinks = new ConcurrentHashMap<>(original.outgoingLinks.size());
+ for (Entry<SwitchPort, ConcurrentMap<String, LinkEvent>> entry
+ : original.outgoingLinks.entrySet()) {
+ this.outgoingLinks.put(entry.getKey(), new ConcurrentHashMap<>(entry.getValue()));
+ }
+
+ // shallow copy Map in Map
+ this.incomingLinks = new ConcurrentHashMap<>(original.incomingLinks.size());
+ for (Entry<SwitchPort, ConcurrentMap<String, LinkEvent>> entry
+ : original.incomingLinks.entrySet()) {
+ this.incomingLinks.put(entry.getKey(), new ConcurrentHashMap<>(entry.getValue()));
+ }
+ } finally {
+ original.releaseReadLock();
+ }
+ }
+
@Override
public Switch getSwitch(Dpid dpid) {
- // TODO Check if it is safe to directly return this Object.
- return switches.get(dpid);
- }
-
- // Only add switch.
- protected void putSwitch(Switch sw) {
- switches.put(sw.getDpid(), sw);
- ports.putIfAbsent(sw.getDpid(), new ConcurrentHashMap<PortNumber, Port>());
- }
-
- // XXX Will remove ports in snapshot as side-effect.
- protected void removeSwitch(Dpid dpid) {
- switches.remove(dpid);
- ports.remove(dpid);
- }
-
- // This method is expected to be serialized by writeLock.
- protected void putPort(Port port) {
- ConcurrentMap<PortNumber, Port> portMap = ports.get(port.getDpid());
- if (portMap == null) {
- portMap = new ConcurrentHashMap<>();
- ConcurrentMap<PortNumber, Port> existing =
- ports.putIfAbsent(port.getDpid(), portMap);
- if (existing != null) {
- // port map was added concurrently, using theirs
- portMap = existing;
- }
- }
- portMap.put(port.getNumber(), port);
- }
-
- protected void removePort(Port port) {
- ConcurrentMap<PortNumber, Port> portMap = ports.get(port.getDpid());
- if (portMap != null) {
- portMap.remove(port.getNumber());
+ final SwitchEvent sw = switches.get(dpid);
+ if (sw != null) {
+ return new SwitchImpl(this, dpid);
+ } else {
+ return null;
}
}
@Override
public Iterable<Switch> getSwitches() {
- // TODO Check if it is safe to directly return this Object.
- return Collections.unmodifiableCollection(switches.values());
+ List<Switch> list = new ArrayList<>(switches.size());
+ for (SwitchEvent elm : switches.values()) {
+ list.add(new SwitchImpl(this, elm.getDpid()));
+ }
+ return list;
}
@Override
public Port getPort(Dpid dpid, PortNumber number) {
- ConcurrentMap<PortNumber, Port> portMap = ports.get(dpid);
+ ConcurrentMap<PortNumber, PortEvent> portMap = ports.get(dpid);
if (portMap != null) {
- return portMap.get(number);
+ final PortEvent port = portMap.get(number);
+ if (port != null) {
+ return new PortImpl(this, port.getSwitchPort());
+ }
}
return null;
}
@@ -121,11 +146,15 @@
@Override
public Collection<Port> getPorts(Dpid dpid) {
- ConcurrentMap<PortNumber, Port> portMap = ports.get(dpid);
+ ConcurrentMap<PortNumber, PortEvent> portMap = ports.get(dpid);
if (portMap == null) {
return Collections.emptyList();
}
- return Collections.unmodifiableCollection(portMap.values());
+ List<Port> list = new ArrayList<>(portMap.size());
+ for (PortEvent elm : portMap.values()) {
+ list.add(new PortImpl(this, elm.getSwitchPort()));
+ }
+ return list;
}
@Override
@@ -135,32 +164,33 @@
@Override
public Link getOutgoingLink(SwitchPort port) {
- Map<String, Link> links = outgoingLinks.get(port);
+ Map<String, LinkEvent> links = outgoingLinks.get(port);
return getPacketLinkIfExists(links);
}
// TODO remove when we no longer need packet fall back behavior
/**
- * Gets the "packet" link if such exists, if not return whatever found.
+ * Gets the "packet" link if such exists,
+ * if not return whichever link is found first.
*
* @param links Collection of links to search from
* @return Link instance found or null if no link exists
*/
- private Link getPacketLinkIfExists(Map<String, Link> links) {
+ private Link getPacketLinkIfExists(Map<String, LinkEvent> links) {
if (links == null) {
return null;
}
- Link link = links.get(TopologyElement.TYPE_PACKET_LAYER);
+ LinkEvent link = links.get(TopologyElement.TYPE_PACKET_LAYER);
if (link != null) {
// return packet link
- return link;
+ return new LinkImpl(this, link.getLinkTuple());
} else {
// return whatever found
- Iterator<Link> it = links.values().iterator();
+ Iterator<LinkEvent> it = links.values().iterator();
if (it.hasNext()) {
- return it.next();
+ return new LinkImpl(this, it.next().getLinkTuple());
}
}
return null;
@@ -173,13 +203,38 @@
@Override
public Link getOutgoingLink(SwitchPort port, String type) {
- Map<String, Link> links = outgoingLinks.get(port);
- return links.get(type);
+ Map<String, LinkEvent> links = outgoingLinks.get(port);
+ final LinkEvent link = links.get(type);
+ if (link != null) {
+ return new LinkImpl(this, link.getLinkTuple());
+ }
+ return null;
}
@Override
public Collection<Link> getOutgoingLinks(SwitchPort port) {
- return Collections.unmodifiableCollection(outgoingLinks.get(port).values());
+ ConcurrentMap<String, LinkEvent> typeMap = outgoingLinks.get(port);
+ if (typeMap == null) {
+ return Collections.emptyList();
+ }
+ return toLinkImpls(typeMap.values());
+ }
+
+ /**
+ * Converts collection of LinkEvent to collection of LinkImpls.
+ *
+ * @param links collection of LinkEvent
+ * @return collection of LinkImpls
+ */
+ private Collection<Link> toLinkImpls(final Collection<LinkEvent> links) {
+ if (links == null) {
+ return Collections.emptyList();
+ }
+ List<Link> list = new ArrayList<>(links.size());
+ for (LinkEvent elm : links) {
+ list.add(new LinkImpl(this, elm.getLinkTuple()));
+ }
+ return list;
}
@Override
@@ -189,7 +244,7 @@
@Override
public Link getIncomingLink(SwitchPort port) {
- Map<String, Link> links = incomingLinks.get(port);
+ Map<String, LinkEvent> links = incomingLinks.get(port);
return getPacketLinkIfExists(links);
}
@@ -200,13 +255,21 @@
@Override
public Link getIncomingLink(SwitchPort port, String type) {
- Map<String, Link> links = incomingLinks.get(port);
- return links.get(type);
+ Map<String, LinkEvent> links = incomingLinks.get(port);
+ final LinkEvent link = links.get(type);
+ if (link != null) {
+ return new LinkImpl(this, link.getLinkTuple());
+ }
+ return null;
}
@Override
public Collection<Link> getIncomingLinks(SwitchPort port) {
- return Collections.unmodifiableCollection(incomingLinks.get(port).values());
+ ConcurrentMap<String, LinkEvent> typeMap = incomingLinks.get(port);
+ if (typeMap == null) {
+ return Collections.emptyList();
+ }
+ return toLinkImpls(typeMap.values());
}
@Override
@@ -248,94 +311,300 @@
public Iterable<Link> getLinks() {
List<Link> links = new ArrayList<>();
- for (Map<String, Link> portLinks : outgoingLinks.values()) {
- links.addAll(portLinks.values());
+ for (Map<String, LinkEvent> portLinks : outgoingLinks.values()) {
+ if (portLinks == null) {
+ continue;
+ }
+ for (LinkEvent elm : portLinks.values()) {
+ links.add(new LinkImpl(this, elm.getLinkTuple()));
+ }
}
return links;
}
- @GuardedBy("topology.writeLock")
- protected void putLink(Link link) {
- putLinkMap(outgoingLinks, link.getSrcPort().asSwitchPort(), link);
- putLinkMap(incomingLinks, link.getDstPort().asSwitchPort(), link);
+ @Override
+ public Host getHostByMac(MACAddress address) {
+ HostEvent host = mac2Host.get(address);
+ if (host != null) {
+ return new HostImpl(this, address);
+ }
+ return null;
+ }
+
+ @Override
+ public Iterable<Host> getHosts() {
+ return toHostImpls(mac2Host.values());
+ }
+
+ /**
+ * Converts collection of HostEvent to collection of HostImpl.
+ *
+ * @param events collection of HostEvent
+ * @return collection of HostImpl
+ */
+ private List<Host> toHostImpls(Collection<HostEvent> events) {
+ if (events == null) {
+ return Collections.emptyList();
+ }
+ List<Host> list = new ArrayList<>(events.size());
+ for (HostEvent elm : events) {
+ list.add(new HostImpl(this, elm.getMac()));
+ }
+ return list;
+ }
+
+ @Override
+ public Collection<Host> getHosts(SwitchPort port) {
+ return toHostImpls(hosts.get(port));
+ }
+
+ @Override
+ public SwitchEvent getSwitchEvent(final Dpid dpid) {
+ return this.switches.get(dpid);
+ }
+
+ @Override
+ public PortEvent getPortEvent(final SwitchPort port) {
+ ConcurrentMap<PortNumber, PortEvent> portMap = this.ports.get(port.getDpid());
+ if (portMap != null) {
+ return portMap.get(port.getPortNumber());
+ }
+ return null;
+ }
+
+ @Override
+ public LinkEvent getLinkEvent(final LinkTuple linkId) {
+ ConcurrentMap<String, LinkEvent> links = this.outgoingLinks.get(linkId.getSrc());
+ if (links == null) {
+ return null;
+ }
+
+ // TODO Should we look for Packet link first?
+ // Not unless invariant is broken.
+
+ for (LinkEvent link : links.values()) {
+ if (link.getDst().equals(linkId.getDst())) {
+ return link;
+ }
+ }
+ return null;
+ }
+
+ @Override
+ public LinkEvent getLinkEvent(final LinkTuple linkId, final String type) {
+ ConcurrentMap<String, LinkEvent> links = this.outgoingLinks.get(linkId.getSrc());
+ if (links == null) {
+ return null;
+ }
+ return links.get(type);
+ }
+
+ @Override
+ public Collection<LinkEvent> getLinkEvents(final LinkTuple linkId) {
+ ConcurrentMap<String, LinkEvent> links = this.outgoingLinks.get(linkId.getSrc());
+ if (links == null) {
+ return Collections.emptyList();
+ }
+
+ // unless invariant is broken, this should contain at most 1 element.
+ return Collections.unmodifiableCollection(links.values());
+ }
+
+ @Override
+ public HostEvent getHostEvent(final MACAddress mac) {
+ return this.mac2Host.get(mac);
+ }
+
+ /**
+ * Puts a SwitchEvent.
+ *
+ * @param sw Switch to add. (Will be frozen if not already)
+ */
+ @GuardedBy("writeLock")
+ protected void putSwitch(SwitchEvent sw) {
+ // TODO isFrozen check once we implement CoW/lock-free
+ switches.put(sw.getDpid(), sw.freeze());
+ ports.putIfAbsent(sw.getDpid(), new ConcurrentHashMap<PortNumber, PortEvent>());
+ }
+
+ /**
+ * Removes a SwitchEvent from this snapshot.
+ * <p/>
+ * Will also remove ports, if it has not been removed already.
+ *
+ * @param dpid Switch DPID
+ */
+ @GuardedBy("writeLock")
+ protected void removeSwitch(Dpid dpid) {
+ // TODO isFrozen check once we implement CoW/lock-free
+ switches.remove(dpid);
+ ConcurrentMap<PortNumber, PortEvent> removedPorts = ports.remove(dpid);
+ if (removedPorts != null && !removedPorts.isEmpty()) {
+ log.warn("Some ports were removed as side-effect of #removeSwitch({})", dpid);
+ }
+ }
+
+ /**
+ * Puts a PortEvent.
+ *
+ * @param port Port to add. (Will be frozen if not already)
+ */
+ @GuardedBy("writeLock")
+ protected void putPort(PortEvent port) {
+
+ ConcurrentMap<PortNumber, PortEvent> portMap = ports.get(port.getDpid());
+ if (portMap == null) {
+ portMap = new ConcurrentHashMap<>();
+ ConcurrentMap<PortNumber, PortEvent> existing
+ = ports.putIfAbsent(port.getDpid(), portMap);
+ if (existing != null) {
+ // port map was added concurrently, using theirs
+ portMap = existing;
+ }
+ }
+ portMap.put(port.getPortNumber(), port.freeze());
+ }
+
+ /**
+ * Removes a PortEvent from this snapshot.
+ *
+ * @param port SwitchPort to remove
+ */
+ @GuardedBy("writeLock")
+ protected void removePort(SwitchPort port) {
+ removePort(port.getDpid(), port.getPortNumber());
+ }
+
+ /**
+ * Removes a PortEvent from this snapshot.
+ * <p/>
+ * Will also remove ports, if it has not been removed already.
+ *
+ * @param dpid Switch DPID
+ * @param number PortNumber
+ */
+ @GuardedBy("writeLock")
+ protected void removePort(Dpid dpid, PortNumber number) {
+ // TODO sanity check Host attachment point.
+ ConcurrentMap<PortNumber, PortEvent> portMap = ports.get(dpid);
+ if (portMap != null) {
+ portMap.remove(number);
+ }
+ }
+
+ /**
+ * Puts a LinkEvent.
+ *
+ * @param link LinkEvent
+ */
+ @GuardedBy("writeLock")
+ protected void putLink(LinkEvent link) {
+ // TODO Do sanity check?
+ // - There cannot be 2 links in same direction between a port pair.
+ putLinkMap(outgoingLinks, link.getSrc(), link);
+ putLinkMap(incomingLinks, link.getDst(), link);
}
/**
* Helper method to update outgoingLinks, incomingLinks.
*
- * @param linkMap outgoingLinks or incomingLinks
- * @param port Map key
+ * @param linkMap outgoingLinks or incomingLinks to update
+ * @param port {@code linkMap} key to update
* @param link Link to add
*/
- @GuardedBy("topology.writeLock")
- private void putLinkMap(ConcurrentMap<SwitchPort, ConcurrentMap<String, Link>> linkMap,
- SwitchPort port, Link link) {
- ConcurrentMap<String, Link> portLinks = new ConcurrentHashMap<String, Link>(3);
- portLinks.put(link.getType(), link);
- Map<String, Link> existing = linkMap.putIfAbsent(
+ @GuardedBy("writeLock")
+ private void putLinkMap(ConcurrentMap<SwitchPort, ConcurrentMap<String, LinkEvent>> linkMap,
+ SwitchPort port, LinkEvent link) {
+
+ ConcurrentMap<String, LinkEvent> linksOnPort = linkMap.get(port);
+ if (linksOnPort == null) {
+ linksOnPort = new ConcurrentHashMap<>(4);
+ ConcurrentMap<String, LinkEvent> existing
+ = linkMap.putIfAbsent(
port,
- portLinks);
- if (existing != null) {
- // no conditional update here
- existing.put(link.getType(), link);
+ linksOnPort);
+
+ if (existing != null) {
+ linksOnPort = existing;
+ }
}
+ linksOnPort.put(link.getType(), link);
}
- @GuardedBy("topology.writeLock")
- protected void removeLink(Link link) {
- ConcurrentMap<String, Link> portLinks = outgoingLinks.get(link.getSrcPort().asSwitchPort());
+ /**
+ * Removes a LinkEvent from this snapshot.
+ *
+ * @param link Link to remove
+ * @param type type of link to remove
+ */
+ @GuardedBy("writeLock")
+ protected void removeLink(LinkTuple link, String type) {
+ ConcurrentMap<String, LinkEvent> portLinks
+ = outgoingLinks.get(link.getSrc());
if (portLinks != null) {
// no conditional update here
- portLinks.remove(link.getType());
+ portLinks.remove(type);
}
- portLinks = incomingLinks.get(link.getDstPort().asSwitchPort());
+ portLinks
+ = incomingLinks.get(link.getDst());
if (portLinks != null) {
// no conditional update here
- portLinks.remove(link.getType());
+ portLinks.remove(type);
}
}
- @Override
- public Host getHostByMac(MACAddress address) {
- return mac2Host.get(address);
- }
-
- @Override
- public Iterable<Host> getHosts() {
- return Collections.unmodifiableCollection(mac2Host.values());
- }
-
- @Override
- public Collection<Host> getHosts(SwitchPort port) {
- return Collections.unmodifiableCollection(hosts.get(port));
- }
-
- // This method is expected to be serialized by writeLock.
- // XXX new or updated device
- protected void putHost(Host host) {
- // assuming Host is immutable
- Host oldHost = mac2Host.get(host.getMacAddress());
- if (oldHost != null) {
- // remove old attachment point
- removeHost(oldHost);
+ /**
+ * Removes a LinkEvent from this snapshot.
+ *
+ * @param link Link to remove
+ */
+ @GuardedBy("writeLock")
+ protected void removeLink(LinkTuple link) {
+ Collection<LinkEvent> links = getLinkEvents(link);
+ for (LinkEvent l : links) {
+ removeLink(link, l.getType());
}
+ }
+
+ /**
+ * Puts a HostEvent.
+ * <p/>
+ * Removes attachment points for previous HostEvent and update
+ * them with new HostEvent
+ *
+ * @param host HostEvent
+ */
+ @GuardedBy("writeLock")
+ protected void putHost(HostEvent host) {
+ // Host cannot be simply put() to replace instance since it has mobility.
+ // Simply remove -> put for now.
+
+ // remove old attachment points
+ removeHost(host.getMac());
+
// add new attachment points
- for (Port port : host.getAttachmentPoints()) {
- // TODO Won't need remove() if we define Host equality to reflect
- // all of it's fields.
- hosts.remove(port.asSwitchPort(), host);
- hosts.put(port.asSwitchPort(), host);
+ for (SwitchPort port : host.getAttachmentPoints()) {
+ hosts.put(port, host);
}
- mac2Host.put(host.getMacAddress(), host);
+ mac2Host.put(host.getMac(), host);
}
- protected void removeHost(Host host) {
- for (Port port : host.getAttachmentPoints()) {
- hosts.remove(port.asSwitchPort(), host);
+ /**
+ * Removes a HostEvent from this snapshot.
+ *
+ * @param mac MACAddress of the Host to remove
+ */
+ @GuardedBy("writeLock")
+ protected void removeHost(MACAddress mac) {
+ HostEvent host = mac2Host.remove(mac);
+ if (host != null) {
+ for (SwitchPort port : host.getAttachmentPoints()) {
+ hosts.remove(port, host);
+ }
}
- mac2Host.remove(host.getMacAddress());
}
+
@Override
public void acquireReadLock() {
readLock.lock();
diff --git a/src/main/java/net/onrc/onos/core/topology/TopologyInternal.java b/src/main/java/net/onrc/onos/core/topology/TopologyInternal.java
new file mode 100644
index 0000000..10b008e
--- /dev/null
+++ b/src/main/java/net/onrc/onos/core/topology/TopologyInternal.java
@@ -0,0 +1,64 @@
+package net.onrc.onos.core.topology;
+
+import java.util.Collection;
+
+import net.floodlightcontroller.util.MACAddress;
+import net.onrc.onos.core.util.Dpid;
+import net.onrc.onos.core.util.LinkTuple;
+import net.onrc.onos.core.util.SwitchPort;
+
+/**
+ * Interface to reference internal self-contained elements.
+ */
+public interface TopologyInternal extends Topology {
+
+ /**
+ * Gets a SwitchEvent.
+ *
+ * @param dpid Switch DPID
+ * @return the SwitchEvent for the Switch DPID if found, otherwise null
+ */
+ public SwitchEvent getSwitchEvent(Dpid dpid);
+
+ /**
+ * Gets a PortEvent.
+ *
+ * @param port Port identifier
+ * @return the PortEvent for the Port identifier if found, otherwise null
+ */
+ public PortEvent getPortEvent(SwitchPort port);
+
+ /**
+ * Gets a LinkEvent.
+ *
+ * @param linkId Link identifier
+ * @return the LinkEvent for the Link identifier if found, otherwise null
+ */
+ public LinkEvent getLinkEvent(LinkTuple linkId);
+
+ /**
+ * Gets a LinkEvent.
+ *
+ * @param linkId Link identifier
+ * @param type type
+ * @return the LinkEvent for the Link identifier and type if found, otherwise null
+ */
+ public LinkEvent getLinkEvent(final LinkTuple linkId, final String type);
+
+ /**
+ * Gets a LinkEvent.
+ *
+ * @param linkId Link identifier
+ * @return Collection of LinkEvent
+ */
+ public Collection<LinkEvent> getLinkEvents(LinkTuple linkId);
+
+ /**
+ * Gets a HostEvent.
+ *
+ * @param mac MACAddress of the host
+ * @return the HostEvent for the MACAddress if found, otherwise null
+ */
+ public HostEvent getHostEvent(MACAddress mac);
+
+}
diff --git a/src/main/java/net/onrc/onos/core/topology/TopologyManager.java b/src/main/java/net/onrc/onos/core/topology/TopologyManager.java
index bdc6d67..7e7f2b8 100644
--- a/src/main/java/net/onrc/onos/core/topology/TopologyManager.java
+++ b/src/main/java/net/onrc/onos/core/topology/TopologyManager.java
@@ -2,13 +2,16 @@
import java.nio.ByteBuffer;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.TreeSet;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.LinkedBlockingQueue;
@@ -153,7 +156,7 @@
/**
* Event handler class.
*/
- private class EventHandler extends Thread implements
+ class EventHandler extends Thread implements
IEventChannelListener<byte[], TopologyEvent> {
private BlockingQueue<EventEntry<TopologyEvent>> topologyEvents =
new LinkedBlockingQueue<EventEntry<TopologyEvent>>();
@@ -243,6 +246,8 @@
//
// Extract the events
//
+ // FIXME Following event squashing logic based only on ID
+ // potentially lose attribute change.
switch (event.eventType()) {
case ENTRY_ADD:
log.debug("Topology event ENTRY_ADD: {}", topologyEvent);
@@ -836,6 +841,10 @@
}
}
+ //
+ // Methods to update topology replica
+ //
+
/**
* Adds a switch to the topology replica.
*
@@ -843,16 +852,15 @@
*/
@GuardedBy("topology.writeLock")
private void addSwitch(SwitchEvent switchEvent) {
- Switch sw = topology.getSwitch(switchEvent.getDpid());
- if (sw == null) {
- sw = new SwitchImpl(topology, switchEvent);
- topology.putSwitch(sw);
- } else {
- // TODO: Update the switch attributes
- log.debug("Update switch attributes");
- SwitchImpl impl = (SwitchImpl) sw;
- impl.replaceStringAttributes(switchEvent);
+ if (log.isDebugEnabled()) {
+ SwitchEvent sw = topology.getSwitchEvent(switchEvent.getDpid());
+ if (sw != null) {
+ log.debug("Update {}", switchEvent);
+ } else {
+ log.debug("Added {}", switchEvent);
+ }
}
+ topology.putSwitch(switchEvent.freeze());
apiAddedSwitchEvents.add(switchEvent);
}
@@ -865,8 +873,10 @@
*/
@GuardedBy("topology.writeLock")
private void removeSwitch(SwitchEvent switchEvent) {
- Switch sw = topology.getSwitch(switchEvent.getDpid());
- if (sw == null) {
+ final Dpid dpid = switchEvent.getDpid();
+
+ SwitchEvent swInTopo = topology.getSwitchEvent(dpid);
+ if (swInTopo == null) {
log.warn("Switch {} already removed, ignoring", switchEvent);
return;
}
@@ -875,19 +885,19 @@
// Remove all Ports on the Switch
//
ArrayList<PortEvent> portsToRemove = new ArrayList<>();
- for (Port port : sw.getPorts()) {
+ for (Port port : topology.getPorts(dpid)) {
log.warn("Port {} on Switch {} should be removed prior to removing Switch. Removing Port now.",
port, switchEvent);
- PortEvent portEvent = new PortEvent(port.getDpid(),
- port.getNumber());
+ PortEvent portEvent = new PortEvent(port.asSwitchPort());
portsToRemove.add(portEvent);
}
for (PortEvent portEvent : portsToRemove) {
removePort(portEvent);
}
- topology.removeSwitch(switchEvent.getDpid());
- apiRemovedSwitchEvents.add(switchEvent);
+ log.debug("Removed {}", swInTopo);
+ topology.removeSwitch(dpid);
+ apiRemovedSwitchEvents.add(swInTopo);
}
/**
@@ -906,100 +916,101 @@
return;
}
-
- Port port = sw.getPort(portEvent.getPortNumber());
- if (port == null) {
- port = new PortImpl(topology, portEvent);
- topology.putPort(port);
- } else {
- // TODO: Update the port attributes
- log.debug("Update port attributes");
- PortImpl impl = (PortImpl) port;
- impl.replaceStringAttributes(portEvent);
+ if (log.isDebugEnabled()) {
+ PortEvent port = topology.getPortEvent(portEvent.getSwitchPort());
+ if (port != null) {
+ log.debug("Update {}", portEvent);
+ } else {
+ log.debug("Added {}", portEvent);
+ }
}
+ topology.putPort(portEvent.freeze());
apiAddedPortEvents.add(portEvent);
}
/**
* Removes a port from the topology replica.
* <p/>
- * It will call {@link #removeHost(HostEvent)} for each hosts on this port
+ * It will remove attachment points from each hosts on this port
* and call {@link #removeLink(LinkEvent)} for each links on this port.
*
* @param portEvent the PortEvent with the port to remove.
*/
@GuardedBy("topology.writeLock")
private void removePort(PortEvent portEvent) {
- Switch sw = topology.getSwitch(portEvent.getDpid());
+ SwitchEvent sw = topology.getSwitchEvent(portEvent.getDpid());
if (sw == null) {
log.warn("Parent Switch for Port {} already removed, ignoring",
portEvent);
return;
}
- Port port = sw.getPort(portEvent.getPortNumber());
- if (port == null) {
+ final SwitchPort switchPort = portEvent.getSwitchPort();
+ PortEvent portInTopo = topology.getPortEvent(switchPort);
+ if (portInTopo == null) {
log.warn("Port {} already removed, ignoring", portEvent);
return;
}
//
- // Remove all Hosts attached to the Port
+ // Remove all Host attachment points bound to this Port
//
- ArrayList<HostEvent> hostsToRemove = new ArrayList<>();
- for (Host host : port.getHosts()) {
- log.debug("Removing Host {} on Port {}", host, portEvent);
- HostEvent hostEvent = new HostEvent(host.getMacAddress());
- SwitchPort switchPort = new SwitchPort(port.getSwitch().getDpid(),
- port.getNumber());
- hostEvent.addAttachmentPoint(switchPort);
- hostsToRemove.add(hostEvent);
+ List<HostEvent> hostsToUpdate = new ArrayList<>();
+ for (Host host : topology.getHosts(switchPort)) {
+ log.debug("Removing Host {} on Port {}", host, portInTopo);
+ HostEvent hostEvent = topology.getHostEvent(host.getMacAddress());
+ hostsToUpdate.add(hostEvent);
}
- for (HostEvent hostEvent : hostsToRemove) {
- removeHost(hostEvent);
+ for (HostEvent hostEvent : hostsToUpdate) {
+ HostEvent newHostEvent = new HostEvent(hostEvent);
+ newHostEvent.removeAttachmentPoint(switchPort);
+ newHostEvent.freeze();
+
+ // TODO should this event be fired inside #addHost?
+ if (newHostEvent.getAttachmentPoints().isEmpty()) {
+ // No more attachment point left -> remove Host
+ removeHost(hostEvent);
+ } else {
+ // Update Host
+ addHost(newHostEvent);
+ }
}
//
// Remove all Links connected to the Port
//
Set<Link> links = new HashSet<>();
- links.add(port.getOutgoingLink());
- links.add(port.getIncomingLink());
- ArrayList<LinkEvent> linksToRemove = new ArrayList<>();
+ links.addAll(topology.getOutgoingLinks(switchPort));
+ links.addAll(topology.getIncomingLinks(switchPort));
for (Link link : links) {
if (link == null) {
continue;
}
- log.debug("Removing Link {} on Port {}", link, portEvent);
- LinkEvent linkEvent = new LinkEvent(link.getSrcSwitch().getDpid(),
- link.getSrcPort().getNumber(),
- link.getDstSwitch().getDpid(),
- link.getDstPort().getNumber());
- linksToRemove.add(linkEvent);
- }
- for (LinkEvent linkEvent : linksToRemove) {
- removeLink(linkEvent);
+ LinkEvent linkEvent = topology.getLinkEvent(link.getLinkTuple());
+ if (linkEvent != null) {
+ log.debug("Removing Link {} on Port {}", link, portInTopo);
+ removeLink(linkEvent);
+ }
}
- // Remove the Port from the Switch
- topology.removePort(port);
+ // Remove the Port from Topology
+ log.debug("Removed {}", portInTopo);
+ topology.removePort(switchPort);
- apiRemovedPortEvents.add(portEvent);
+ apiRemovedPortEvents.add(portInTopo);
}
/**
* Adds a link to the topology replica.
* <p/>
- * It will call {@link #removeHost(HostEvent)} for each hosts on both ports.
+ * It will remove attachment points from each hosts using the same ports.
*
* @param linkEvent the LinkEvent with the link to add.
*/
@GuardedBy("topology.writeLock")
private void addLink(LinkEvent linkEvent) {
- Port srcPort = topology.getPort(linkEvent.getSrc().getDpid(),
- linkEvent.getSrc().getPortNumber());
- Port dstPort = topology.getPort(linkEvent.getDst().getDpid(),
- linkEvent.getDst().getPortNumber());
+ PortEvent srcPort = topology.getPortEvent(linkEvent.getSrc());
+ PortEvent dstPort = topology.getPortEvent(linkEvent.getDst());
if ((srcPort == null) || (dstPort == null)) {
log.debug("{} reordered because {} port is null", linkEvent,
(srcPort == null) ? "src" : "dst");
@@ -1011,47 +1022,64 @@
return;
}
- // Get the Link instance from the Destination Port Incoming Link
- // XXX domain knowledge: Link is discovered by LLDP,
- // thus incoming link is likely to be more up-to-date
+ // XXX domain knowledge: Sanity check: Port cannot have both Link and Host
// FIXME potentially local replica may not be up-to-date yet due to HZ delay.
// may need to manage local truth and use them instead.
- Link link = dstPort.getIncomingLink();
- assert (link == srcPort.getOutgoingLink());
- if (link == null) {
- link = new LinkImpl(topology, linkEvent);
- topology.putLink(link);
+ if (topology.getLinkEvent(linkEvent.getLinkTuple()) == null) {
+ // Only check for existing Host when adding new Link.
- // Remove all Hosts attached to the Ports
- ArrayList<HostEvent> hostsToRemove = new ArrayList<>();
- ArrayList<Port> ports = new ArrayList<>();
- ports.add(srcPort);
- ports.add(dstPort);
- for (Port port : ports) {
- for (Host host : port.getHosts()) {
+ // Remove all Hosts attached to the ports on both ends
+
+ Set<HostEvent> hostsToUpdate = new TreeSet<>(new Comparator<HostEvent>() {
+ // comparison only using ID(=MAC)
+ @Override
+ public int compare(HostEvent o1, HostEvent o2) {
+ return Long.compare(o1.getMac().toLong(), o2.getMac().toLong());
+ }
+ });
+
+ List<SwitchPort> portsToCheck = Arrays.asList(
+ srcPort.getSwitchPort(),
+ dstPort.getSwitchPort());
+
+ // Enumerate Host which needs to be updated by this Link add event
+ for (SwitchPort port : portsToCheck) {
+ for (Host host : topology.getHosts(port)) {
log.error("Host {} on Port {} should have been removed prior to adding Link {}",
host, port, linkEvent);
- // FIXME must get Host info from topology, when we add attrs.
- HostEvent hostEvent =
- new HostEvent(host.getMacAddress());
- SwitchPort switchPort =
- new SwitchPort(port.getSwitch().getDpid(),
- port.getNumber());
- // adding attachment port which needs to be removed
- hostEvent.addAttachmentPoint(switchPort);
- hostsToRemove.add(hostEvent);
+
+ HostEvent hostEvent = topology.getHostEvent(host.getMacAddress());
+ hostsToUpdate.add(hostEvent);
}
}
- for (HostEvent hostEvent : hostsToRemove) {
- removeHost(hostEvent);
+ // remove attachment point from them.
+ for (HostEvent hostEvent : hostsToUpdate) {
+ // remove port from attachment point and update
+ HostEvent newHostEvent = new HostEvent(hostEvent);
+ newHostEvent.removeAttachmentPoint(srcPort.getSwitchPort());
+ newHostEvent.removeAttachmentPoint(dstPort.getSwitchPort());
+ newHostEvent.freeze();
+
+ // TODO should this event be fired inside #addHost?
+ if (newHostEvent.getAttachmentPoints().isEmpty()) {
+ // No more attachment point left -> remove Host
+ removeHost(hostEvent);
+ } else {
+ // Update Host
+ addHost(newHostEvent);
+ }
}
- } else {
- // TODO: Update the link attributes
- log.debug("Update link attributes");
- LinkImpl impl = (LinkImpl) link;
- impl.replaceStringAttributes(linkEvent);
}
+ if (log.isDebugEnabled()) {
+ LinkEvent link = topology.getLinkEvent(linkEvent.getLinkTuple());
+ if (link != null) {
+ log.debug("Update {}", linkEvent);
+ } else {
+ log.debug("Added {}", linkEvent);
+ }
+ }
+ topology.putLink(linkEvent.freeze());
apiAddedLinkEvents.add(linkEvent);
}
@@ -1078,25 +1106,29 @@
return;
}
- //
- // Remove the Link instance from the Destination Port Incoming Link
- // and the Source Port Outgoing Link.
- //
- Link link = dstPort.getIncomingLink();
- if (link == null) {
- log.warn("Link {} already removed on destination Port", linkEvent);
- }
- link = srcPort.getOutgoingLink();
- if (link == null) {
- log.warn("Link {} already removed on src Port", linkEvent);
+ LinkEvent linkInTopo = topology.getLinkEvent(linkEvent.getLinkTuple(),
+ linkEvent.getType());
+ if (linkInTopo == null) {
+ log.warn("Link {} already removed, ignoring", linkEvent);
+ return;
}
- // TODO should we check that we get the same link from each port?
- if (link != null) {
- topology.removeLink(link);
+ if (log.isDebugEnabled()) {
+ // only do sanity check on debug level
+
+ Link linkIn = dstPort.getIncomingLink(linkEvent.getType());
+ if (linkIn == null) {
+ log.warn("Link {} already removed on destination Port", linkEvent);
+ }
+ Link linkOut = srcPort.getOutgoingLink(linkEvent.getType());
+ if (linkOut == null) {
+ log.warn("Link {} already removed on src Port", linkEvent);
+ }
}
- apiRemovedLinkEvents.add(linkEvent);
+ log.debug("Removed {}", linkInTopo);
+ topology.removeLink(linkEvent.getLinkTuple(), linkEvent.getType());
+ apiRemovedLinkEvents.add(linkInTopo);
}
/**
@@ -1104,23 +1136,23 @@
* <p/>
* TODO: Host-related work is incomplete.
* TODO: Eventually, we might need to consider reordering
- * or addLink() and addDevice() events on the same port.
+ * or {@link #addLink(LinkEvent)} and {@link #addHost(HostEvent)} events on the same port.
*
* @param hostEvent the HostEvent with the host to add.
*/
@GuardedBy("topology.writeLock")
private void addHost(HostEvent hostEvent) {
- log.debug("Adding a host to the topology with mac {}", hostEvent.getMac());
- Host host = topology.getHostByMac(hostEvent.getMac());
- if (host == null) {
- log.debug("Existing host was not found in the Topology: Adding new host: mac {}", hostEvent.getMac());
- host = new HostImpl(topology, hostEvent.getMac());
- }
+ // TODO Decide how to handle update scenario.
+ // If the new HostEvent has less attachment point compared to
+ // existing HostEvent, what should the event be?
+ // - AddHostEvent with some attachment point removed? (current behavior)
- HostImpl hostImpl = (HostImpl) host;
+ // create unfrozen copy
+ // for removing attachment points which already has a link
+ HostEvent modifiedHostEvent = new HostEvent(hostEvent);
- // Process each attachment point
+ // Verify each attachment point
boolean attachmentFound = false;
for (SwitchPort swp : hostEvent.getAttachmentPoints()) {
// XXX domain knowledge: Port must exist before Host
@@ -1129,32 +1161,49 @@
// Attached Ports must exist
Port port = topology.getPort(swp.getDpid(), swp.getPortNumber());
if (port == null) {
+ log.debug("{} reordered because port {} was not there", hostEvent, swp);
// Reordered event: delay the event in local cache
ByteBuffer id = hostEvent.getIDasByteBuffer();
reorderedAddedHostEvents.put(id, hostEvent);
- continue;
+ return; // should not continue if re-applying later
}
// Attached Ports must not have Link
if (port.getOutgoingLink() != null ||
port.getIncomingLink() != null) {
- log.warn("Link (Out:{},In:{}) exist on the attachment point, skipping mutation.",
- port.getOutgoingLink(),
- port.getIncomingLink());
+ log.warn("Link (Out:{},In:{}) exist on the attachment point. "
+ + "Ignoring this attachmentpoint ({}) from {}.",
+ port.getOutgoingLink(), port.getIncomingLink(),
+ swp, modifiedHostEvent);
+ // FIXME Should either reject, reorder this HostEvent,
+ // or remove attachment point from given HostEvent
+ // Removing attachment point from given HostEvent for now.
+ modifiedHostEvent.removeAttachmentPoint(swp);
continue;
}
- // Add Host <-> Port attachment
- hostImpl.addAttachmentPoint(port);
attachmentFound = true;
}
- hostImpl.setLastSeenTime(hostEvent.getLastSeenTime());
-
// Update the host in the topology
if (attachmentFound) {
- log.debug("Storing the host info into the Topology: mac {}", hostEvent.getMac());
- topology.putHost(host);
- apiAddedHostEvents.add(hostEvent);
+ if (modifiedHostEvent.getAttachmentPoints().isEmpty()) {
+ log.warn("No valid attachment point left. Ignoring."
+ + "original: {}, modified: {}", hostEvent, modifiedHostEvent);
+ // TODO Should we call #removeHost to trigger remove event?
+ // only if this call is update.
+ return;
+ }
+
+ if (log.isDebugEnabled()) {
+ HostEvent host = topology.getHostEvent(hostEvent.getMac());
+ if (host != null) {
+ log.debug("Update {}", modifiedHostEvent);
+ } else {
+ log.debug("Added {}", modifiedHostEvent);
+ }
+ }
+ topology.putHost(modifiedHostEvent.freeze());
+ apiAddedHostEvents.add(modifiedHostEvent);
}
}
@@ -1167,15 +1216,17 @@
*/
@GuardedBy("topology.writeLock")
private void removeHost(HostEvent hostEvent) {
- log.debug("Removing a host from the topology: mac {}", hostEvent.getMac());
- Host host = topology.getHostByMac(hostEvent.getMac());
- if (host == null) {
+
+ final MACAddress mac = hostEvent.getMac();
+ HostEvent hostInTopo = topology.getHostEvent(mac);
+ if (hostInTopo == null) {
log.warn("Host {} already removed, ignoring", hostEvent);
return;
}
- topology.removeHost(host);
- apiRemovedHostEvents.add(hostEvent);
+ log.debug("Removed {}", hostInTopo);
+ topology.removeHost(mac);
+ apiRemovedHostEvents.add(hostInTopo);
}
/**
@@ -1243,14 +1294,4 @@
return collection;
}
- /**
- * Replaces the internal datastore instance.
- *
- * @param dataStore instance
- *
- * @exclude Backdoor for unit testing purpose only, do not use.
- */
- void debugReplaceDataStore(final TopologyDatastore dataStoreService) {
- this.datastore = dataStoreService;
- }
}
diff --git a/src/main/java/net/onrc/onos/core/topology/TopologyObject.java b/src/main/java/net/onrc/onos/core/topology/TopologyObject.java
index f929926..445c2d8 100644
--- a/src/main/java/net/onrc/onos/core/topology/TopologyObject.java
+++ b/src/main/java/net/onrc/onos/core/topology/TopologyObject.java
@@ -3,28 +3,40 @@
import org.apache.commons.lang.Validate;
-
/**
* Base class for Topology Objects.
*/
public abstract class TopologyObject implements ITopologyElement {
- // XXX This will be a snapshot, thus should be replaceable
/**
- * Topology instance this object belongs to.
+ * Topology snapshot this object belongs to.
*/
- protected final Topology topology;
+ protected volatile TopologyInternal topology;
+
+ // XXX Updater to be used once we implement snapshot update.
+ // Should it be static or not.
+ // static: Low memory consumption, but higher contention on atomic update
+ // non-static: Updater per instance, but less chance of contention
+// private static final AtomicReferenceFieldUpdater<TopologyObject, TopologyImpl>
+// TOPOLOGY_UPDATER =
+// AtomicReferenceFieldUpdater.newUpdater(
+// TopologyObject.class, TopologyImpl.class, "topology");
/**
* Constructor.
*
* @param topology Topology instance this object belongs to
*/
- protected TopologyObject(Topology topology) {
+ protected TopologyObject(TopologyInternal topology) {
Validate.notNull(topology);
this.topology = topology;
}
+ // TODO Add method to replace topology snapshot
+ // - Request TopologyManager for latest TopologyImpl and swap?
+ // - Make caller specify TopologyImpl instance?
+ // -
+
/**
* Returns the type of topology object.