ONOS-2947 Improvements to HostService

- Added ability to replace IPs for existing hosts to HostProviderService
- Moved createOrUpdateHost to use compute() (rather than get/set) in HostStore

Change-Id: I8ac035d010ae65f23158d2237f9fc97c8f811dd4

@@ -330,7 +330,7 @@ public class DhcpManagerTest {
         }
 
         @Override
-        public void hostDetected(HostId hostId, HostDescription hostDescription) {
+        public void hostDetected(HostId hostId, HostDescription hostDescription, boolean replaceIps) {
         }
 
         @Override

@@ -30,7 +30,20 @@ public interface HostProviderService extends ProviderService<HostProvider> {
      * @param hostId          id of the host that been detected
      * @param hostDescription description of host and its location
      */
-    void hostDetected(HostId hostId, HostDescription hostDescription);
+    @Deprecated
+    default void hostDetected(HostId hostId, HostDescription hostDescription) {
+        hostDetected(hostId, hostDescription, false);
+    }
+
+    /**
+     * Notifies the core when a host has been detected on a network along with
+     * information that identifies the host location.
+     *
+     * @param hostId          id of the host that been detected
+     * @param hostDescription description of host and its location
+     * @param replaceIps      replace IP set if true, merge IP set otherwise
+     */
+    void hostDetected(HostId hostId, HostDescription hostDescription, boolean replaceIps);
 
     /**
      * Notifies the core when a host is no longer detected on a network.

@@ -39,12 +39,13 @@ public interface HostStore extends Store<HostEvent, HostStoreDelegate> {
      * @param providerId      provider identification
      * @param hostId          host identification
      * @param hostDescription host description data
+     * @param replaceIps      replace IP set if true, merge IP set otherwise
      * @return appropriate event or null if no change resulted
      */
     HostEvent createOrUpdateHost(ProviderId providerId, HostId hostId,
-                                 HostDescription hostDescription);
+                                 HostDescription hostDescription,
+                                 boolean replaceIps);
 
-    // FIXME: API to remove only IpAddress is missing
     /**
      * Removes the specified host from the inventory.
      *

@@ -84,12 +84,14 @@ public class SimpleHostStore
 
     @Override
     public HostEvent createOrUpdateHost(ProviderId providerId, HostId hostId,
-                                        HostDescription hostDescription) {
+                                        HostDescription hostDescription,
+                                        boolean replaceIps) {
+        //TODO We need a way to detect conflicting changes and abort update.
         StoredHost host = hosts.get(hostId);
         if (host == null) {
             return createHost(providerId, hostId, hostDescription);
         }
-        return updateHost(providerId, host, hostDescription);
+        return updateHost(providerId, host, hostDescription, replaceIps);
     }
 
     // creates a new host and sends HOST_ADDED
@@ -110,7 +112,7 @@ public class SimpleHostStore
 
     // checks for type of update to host, sends appropriate event
     private HostEvent updateHost(ProviderId providerId, StoredHost host,
-                                 HostDescription descr) {
+                                 HostDescription descr, boolean replaceIps) {
         HostEvent event;
         if (!host.location().equals(descr.location())) {
             host.setLocation(descr.location());
@@ -122,8 +124,14 @@ public class SimpleHostStore
             return null;
         }
 
-        Set<IpAddress> addresses = new HashSet<>(host.ipAddresses());
-        addresses.addAll(descr.ipAddress());
+        final Set<IpAddress> addresses;
+        if (replaceIps) {
+            addresses = ImmutableSet.copyOf(descr.ipAddress());
+        } else {
+            addresses = new HashSet<>(host.ipAddresses());
+            addresses.addAll(descr.ipAddress());
+        }
+
         Annotations annotations = merge((DefaultAnnotations) host.annotations(),
                                         descr.annotations());
         StoredHost updated = new StoredHost(providerId, host.id(),

@@ -207,12 +207,12 @@ public class HostManager
         }
 
         @Override
-        public void hostDetected(HostId hostId, HostDescription hostDescription) {
+        public void hostDetected(HostId hostId, HostDescription hostDescription, boolean replaceIps) {
             checkNotNull(hostId, HOST_ID_NULL);
             checkValidity();
             hostDescription = validateHost(hostDescription, hostId);
             HostEvent event = store.createOrUpdateHost(provider().id(), hostId,
-                                                       hostDescription);
+                                                       hostDescription, replaceIps);
             if (event != null) {
                 post(event);
             }

@@ -16,9 +16,11 @@
 package org.onosproject.store.host.impl;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static org.onosproject.net.DefaultAnnotations.merge;
 import static org.onosproject.net.host.HostEvent.Type.HOST_ADDED;
 import static org.onosproject.net.host.HostEvent.Type.HOST_REMOVED;
+import static org.onosproject.net.host.HostEvent.Type.HOST_MOVED;
 import static org.onosproject.net.host.HostEvent.Type.HOST_UPDATED;
 import static org.onosproject.store.service.EventuallyConsistentMapEvent.Type.PUT;
 import static org.onosproject.store.service.EventuallyConsistentMapEvent.Type.REMOVE;
@@ -27,6 +29,7 @@ import static org.slf4j.LoggerFactory.getLogger;
 import java.util.Collection;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -47,6 +50,7 @@ import org.onosproject.net.DefaultHost;
 import org.onosproject.net.DeviceId;
 import org.onosproject.net.Host;
 import org.onosproject.net.HostId;
+import org.onosproject.net.HostLocation;
 import org.onosproject.net.host.HostDescription;
 import org.onosproject.net.host.HostEvent;
 import org.onosproject.net.host.HostStore;
@@ -123,22 +127,67 @@ public class ECHostStore
 
     @Override
     public HostEvent createOrUpdateHost(ProviderId providerId,
-            HostId hostId,
-            HostDescription hostDescription) {
-        DefaultHost currentHost = hosts.get(hostId);
-        if (currentHost == null) {
-            DefaultHost newhost = new DefaultHost(
-                        providerId,
-                        hostId,
-                        hostDescription.hwAddress(),
-                        hostDescription.vlan(),
-                        hostDescription.location(),
-                        ImmutableSet.copyOf(hostDescription.ipAddress()),
-                        hostDescription.annotations());
-            hosts.put(hostId, newhost);
-            return new HostEvent(HOST_ADDED, newhost);
+                                        HostId hostId,
+                                        HostDescription hostDescription,
+                                        boolean replaceIPs) {
+        // 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?
+            HostLocation location = hostDescription.location();
+
+            final Set<IpAddress> addresses;
+            if (existingHost == null || replaceIPs) {
+                addresses = ImmutableSet.copyOf(hostDescription.ipAddress());
+            } else {
+                addresses = Sets.newHashSet(existingHost.ipAddresses());
+                addresses.addAll(hostDescription.ipAddress());
+            }
+
+            final Annotations annotations;
+            if (existingHost != null) {
+                annotations = merge((DefaultAnnotations) existingHost.annotations(),
+                                    hostDescription.annotations());
+            } else {
+                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(),
+                                   hostDescription.vlan(),
+                                   location,
+                                   addresses,
+                                   annotations);
+        });
+
+        if (oldHost.get() != null) {
+            DefaultHost old = oldHost.get();
+            locations.remove(old.location(), old);
         }
-        return updateHost(providerId, hostId, hostDescription, currentHost);
+        locations.put(host.location(), host);
+
+        return eventType.get() != null ? new HostEvent(eventType.get(), host) : null;
     }
 
     @Override
@@ -196,39 +245,6 @@ public class ECHostStore
         return collection.stream().filter(predicate).collect(Collectors.toSet());
     }
 
-    // checks for type of update to host, sends appropriate event
-    private HostEvent updateHost(ProviderId providerId,
-                                 HostId hostId,
-                                 HostDescription descr,
-                                 DefaultHost currentHost) {
-
-        final boolean hostMoved = !currentHost.location().equals(descr.location());
-        if (hostMoved ||
-                !currentHost.ipAddresses().containsAll(descr.ipAddress()) ||
-                !descr.annotations().keys().isEmpty()) {
-
-            Set<IpAddress> addresses = Sets.newHashSet(currentHost.ipAddresses());
-            addresses.addAll(descr.ipAddress());
-            Annotations annotations = merge((DefaultAnnotations) currentHost.annotations(),
-                                            descr.annotations());
-
-            DefaultHost updatedHost = new DefaultHost(providerId, currentHost.id(),
-                                                currentHost.mac(), currentHost.vlan(),
-                                                descr.location(),
-                                                addresses,
-                                                annotations);
-
-            // TODO: We need a way to detect conflicting changes and abort update.
-            hosts.put(hostId, updatedHost);
-            locations.remove(currentHost.location(), currentHost);
-            locations.put(updatedHost.location(), updatedHost);
-
-            HostEvent.Type eventType = hostMoved ? Type.HOST_MOVED : Type.HOST_UPDATED;
-            return new HostEvent(eventType, updatedHost);
-        }
-        return null;
-    }
-
     private class HostLocationTracker implements EventuallyConsistentMapListener<HostId, DefaultHost> {
         @Override
         public void event(EventuallyConsistentMapEvent<HostId, DefaultHost> event) {

@@ -243,7 +243,7 @@ public class HostLocationProviderTest {
         }
 
         @Override
-        public void hostDetected(HostId hostId, HostDescription hostDescription) {
+        public void hostDetected(HostId hostId, HostDescription hostDescription, boolean replaceIps) {
             DeviceId descr = hostDescription.location().deviceId();
             if (added == null) {
                 added = descr;

@@ -143,7 +143,7 @@ public class OvsdbHostProviderTest {
         }
 
         @Override
-        public void hostDetected(HostId hostId, HostDescription hostDescription) {
+        public void hostDetected(HostId hostId, HostDescription hostDescription, boolean replaceIps) {
             DeviceId descr = hostDescription.location().deviceId();
             if (added == null) {
                 added = descr;