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/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();