Fix duplicate host event
Change-Id: I632a1482e7b1b768ce7a0243cc8b6b398b9825b7
diff --git a/core/common/src/test/java/org/onosproject/store/trivial/SimpleHostStore.java b/core/common/src/test/java/org/onosproject/store/trivial/SimpleHostStore.java
index 72ec98c..a56daab 100644
--- a/core/common/src/test/java/org/onosproject/store/trivial/SimpleHostStore.java
+++ b/core/common/src/test/java/org/onosproject/store/trivial/SimpleHostStore.java
@@ -88,10 +88,14 @@
boolean replaceIps) {
//TODO We need a way to detect conflicting changes and abort update.
StoredHost host = hosts.get(hostId);
+ HostEvent hostEvent;
if (host == null) {
- return createHost(providerId, hostId, hostDescription);
+ hostEvent = createHost(providerId, hostId, hostDescription);
+ } else {
+ hostEvent = updateHost(providerId, host, hostDescription, replaceIps);
}
- return updateHost(providerId, host, hostDescription, replaceIps);
+ notifyDelegate(hostEvent);
+ return hostEvent;
}
// creates a new host and sends HOST_ADDED
@@ -153,7 +157,9 @@
Host host = hosts.remove(hostId);
if (host != null) {
locations.remove((host.location()), host);
- return new HostEvent(HOST_REMOVED, host);
+ HostEvent hostEvent = new HostEvent(HOST_REMOVED, host);
+ notifyDelegate(hostEvent);
+ return hostEvent;
}
return null;
}
diff --git a/core/net/src/main/java/org/onosproject/net/host/impl/HostManager.java b/core/net/src/main/java/org/onosproject/net/host/impl/HostManager.java
index 1473f33..f48b836 100644
--- a/core/net/src/main/java/org/onosproject/net/host/impl/HostManager.java
+++ b/core/net/src/main/java/org/onosproject/net/host/impl/HostManager.java
@@ -192,10 +192,7 @@
@Override
public void removeHost(HostId hostId) {
checkNotNull(hostId, HOST_ID_NULL);
- HostEvent event = store.removeHost(hostId);
- if (event != null) {
- post(event);
- }
+ store.removeHost(hostId);
}
// Personalized host provider service issued to the supplied provider.
@@ -211,11 +208,8 @@
checkNotNull(hostId, HOST_ID_NULL);
checkValidity();
hostDescription = validateHost(hostDescription, hostId);
- HostEvent event = store.createOrUpdateHost(provider().id(), hostId,
+ store.createOrUpdateHost(provider().id(), hostId,
hostDescription, replaceIps);
- if (event != null) {
- post(event);
- }
}
// returns a HostDescription made from the union of the BasicHostConfig
@@ -231,20 +225,14 @@
public void hostVanished(HostId hostId) {
checkNotNull(hostId, HOST_ID_NULL);
checkValidity();
- HostEvent event = store.removeHost(hostId);
- if (event != null) {
- post(event);
- }
+ store.removeHost(hostId);
}
@Override
public void removeIpFromHost(HostId hostId, IpAddress ipAddress) {
checkNotNull(hostId, HOST_ID_NULL);
checkValidity();
- HostEvent event = store.removeIp(hostId, ipAddress);
- if (event != null) {
- post(event);
- }
+ store.removeIp(hostId, ipAddress);
}
}
diff --git a/core/net/src/test/java/org/onosproject/net/host/impl/HostManagerTest.java b/core/net/src/test/java/org/onosproject/net/host/impl/HostManagerTest.java
index 92c6c93..c5a6cab 100644
--- a/core/net/src/test/java/org/onosproject/net/host/impl/HostManagerTest.java
+++ b/core/net/src/test/java/org/onosproject/net/host/impl/HostManagerTest.java
@@ -20,6 +20,7 @@
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
+import org.onlab.junit.TestTools;
import org.onlab.packet.IpAddress;
import org.onlab.packet.MacAddress;
import org.onlab.packet.VlanId;
@@ -129,13 +130,15 @@
}
private void validateEvents(Enum... types) {
- int i = 0;
- assertEquals("wrong events received", types.length, listener.events.size());
- for (Event event : listener.events) {
- assertEquals("incorrect event type", types[i], event.type());
- i++;
- }
- listener.events.clear();
+ TestTools.assertAfter(100, () -> {
+ int i = 0;
+ assertEquals("wrong events received", types.length, listener.events.size());
+ for (Event event : listener.events) {
+ assertEquals("incorrect event type", types[i], event.type());
+ i++;
+ }
+ listener.events.clear();
+ });
}
@Test
diff --git a/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java b/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java
index f9c9689..7066b21 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/host/impl/ECHostStore.java
@@ -28,9 +28,10 @@
import java.util.Collection;
import java.util.HashSet;
+import java.util.Map;
import java.util.Objects;
import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Predicate;
import java.util.stream.Collectors;
@@ -56,7 +57,6 @@
import org.onosproject.net.host.HostEvent;
import org.onosproject.net.host.HostStore;
import org.onosproject.net.host.HostStoreDelegate;
-import org.onosproject.net.host.HostEvent.Type;
import org.onosproject.net.provider.ProviderId;
import org.onosproject.store.AbstractStore;
import org.onosproject.store.serializers.KryoNamespaces;
@@ -67,10 +67,7 @@
import org.onosproject.store.service.StorageService;
import org.slf4j.Logger;
-import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Multimaps;
-import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
/**
@@ -90,13 +87,11 @@
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected LogicalClockService clockService;
- // Hosts tracked by their location
- private final SetMultimap<ConnectPoint, Host> locations =
- Multimaps.synchronizedSetMultimap(
- HashMultimap.<ConnectPoint, Host>create());
-
private EventuallyConsistentMap<HostId, DefaultHost> hosts;
+ private final ConcurrentHashMap<HostId, ConnectPoint> locations =
+ new ConcurrentHashMap<>();
+
private EventuallyConsistentMapListener<HostId, DefaultHost> hostLocationTracker =
new HostLocationTracker();
@@ -125,6 +120,7 @@
log.info("Stopped");
}
+ // TODO No longer need to return HostEvent
@Override
public HostEvent createOrUpdateHost(ProviderId providerId,
HostId hostId,
@@ -133,18 +129,7 @@
// TODO: We need a way to detect conflicting changes and abort update.
// (BOC) Compute might do this for us.
- final AtomicReference<Type> eventType = new AtomicReference<>();
- final AtomicReference<DefaultHost> oldHost = new AtomicReference<>();
- DefaultHost host = hosts.compute(hostId, (id, existingHost) -> {
- if (existingHost != null) {
- oldHost.set(existingHost);
- checkState(Objects.equals(hostDescription.hwAddress(), existingHost.mac()),
- "Existing and new MAC addresses differ.");
- checkState(Objects.equals(hostDescription.vlan(), existingHost.vlan()),
- "Existing and new VLANs differ.");
- }
-
- // TODO do we ever want the existing location?
+ hosts.compute(hostId, (id, existingHost) -> {
HostLocation location = hostDescription.location();
final Set<IpAddress> addresses;
@@ -163,15 +148,6 @@
annotations = hostDescription.annotations();
}
- if (existingHost == null) {
- eventType.set(HOST_ADDED);
- } else if (!Objects.equals(existingHost.location(), hostDescription.location())) {
- eventType.set(HOST_MOVED);
- } else if (!existingHost.ipAddresses().containsAll(hostDescription.ipAddress()) ||
- !hostDescription.annotations().keys().isEmpty()) {
- eventType.set(HOST_UPDATED);
- } // else, eventType == null; this means we don't send an event
-
return new DefaultHost(providerId,
hostId,
hostDescription.hwAddress(),
@@ -181,24 +157,20 @@
annotations);
});
- if (oldHost.get() != null) {
- DefaultHost old = oldHost.get();
- locations.remove(old.location(), old);
- }
- locations.put(host.location(), host);
-
- return eventType.get() != null ? new HostEvent(eventType.get(), host) : null;
+ return null;
}
+ // TODO No longer need to return HostEvent
@Override
public HostEvent removeHost(HostId hostId) {
- Host host = hosts.remove(hostId);
- return host != null ? new HostEvent(HOST_REMOVED, host) : null;
+ hosts.remove(hostId);
+ return null;
}
+ // TODO No longer need to return HostEvent
@Override
public HostEvent removeIp(HostId hostId, IpAddress ipAddress) {
- DefaultHost host = hosts.compute(hostId, (id, existingHost) -> {
+ hosts.compute(hostId, (id, existingHost) -> {
if (existingHost != null) {
checkState(Objects.equals(hostId.mac(), existingHost.mac()),
"Existing and new MAC addresses differ.");
@@ -222,7 +194,7 @@
}
return null;
});
- return host != null ? new HostEvent(HOST_UPDATED, host) : null;
+ return null;
}
@Override
@@ -257,22 +229,19 @@
@Override
public Set<Host> getConnectedHosts(ConnectPoint connectPoint) {
- synchronized (locations) {
- return ImmutableSet.copyOf(locations.get(connectPoint));
- }
+ Set<Host> filtered = hosts.entrySet().stream()
+ .filter(entry -> entry.getValue().location().equals(connectPoint))
+ .map(Map.Entry::getValue)
+ .collect(Collectors.toSet());
+ return ImmutableSet.copyOf(filtered);
}
@Override
public Set<Host> getConnectedHosts(DeviceId deviceId) {
- Set<Host> filtered;
- synchronized (locations) {
- filtered = locations
- .entries()
- .stream()
- .filter(entry -> entry.getKey().deviceId().equals(deviceId))
- .map(entry -> entry.getValue())
- .collect(Collectors.toSet());
- }
+ Set<Host> filtered = hosts.entrySet().stream()
+ .filter(entry -> entry.getValue().location().deviceId().equals(deviceId))
+ .map(Map.Entry::getValue)
+ .collect(Collectors.toSet());
return ImmutableSet.copyOf(filtered);
}
@@ -285,13 +254,18 @@
public void event(EventuallyConsistentMapEvent<HostId, DefaultHost> event) {
DefaultHost host = checkNotNull(event.value());
if (event.type() == PUT) {
- boolean isNew = locations.put(host.location(), host);
- notifyDelegate(new HostEvent(isNew ? HOST_ADDED : HOST_UPDATED, host));
+ ConnectPoint prevLocation = locations.put(host.id(), host.location());
+ if (prevLocation == null) {
+ notifyDelegate(new HostEvent(HOST_ADDED, host));
+ } else if (!Objects.equals(prevLocation, host.location())) {
+ notifyDelegate(new HostEvent(HOST_MOVED, host));
+ } else {
+ notifyDelegate(new HostEvent(HOST_UPDATED, host));
+ }
} else if (event.type() == REMOVE) {
- if (locations.remove(host.location(), host)) {
+ if (locations.remove(host.id()) != null) {
notifyDelegate(new HostEvent(HOST_REMOVED, host));
}
-
}
}
}