SimpleLinkStore with annotation and multi-provider support

Change-Id: I98a35c4497363c6115fd0c61d140dfe7790e6cee

+package org.onlab.onos.net;
+
+public final class AnnotationsUtil {
+
+    public static boolean isEqual(Annotations lhs, Annotations rhs) {
+        if (lhs == rhs) {
+            return true;
+        }
+        if (lhs == null || rhs == null) {
+            return false;
+        }
+
+        if (!lhs.keys().equals(rhs.keys())) {
+            return false;
+        }
+
+        for (String key : lhs.keys()) {
+            if (!lhs.value(key).equals(rhs.value(key))) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    // not to be instantiated
+    private AnnotationsUtil() {}
+}

 package org.onlab.onos.net.link;
 
 import org.onlab.onos.net.ConnectPoint;
+import org.onlab.onos.net.Description;
 import org.onlab.onos.net.Link;
 
 /**
  * Describes an infrastructure link.
  */
-public interface LinkDescription {
+public interface LinkDescription extends Description {
 
     /**
      * Returns the link source.

@@ -11,7 +11,7 @@ import org.apache.felix.scr.annotations.Deactivate;
 import org.apache.felix.scr.annotations.Reference;
 import org.apache.felix.scr.annotations.ReferenceCardinality;
 import org.apache.felix.scr.annotations.Service;
-import org.onlab.onos.net.Annotations;
+import org.onlab.onos.net.AnnotationsUtil;
 import org.onlab.onos.net.DefaultAnnotations;
 import org.onlab.onos.net.DefaultDevice;
 import org.onlab.onos.net.DefaultPort;
@@ -196,7 +196,7 @@ public class GossipDeviceStore
         // We allow only certain attributes to trigger update
         if (!Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
             !Objects.equals(oldDevice.swVersion(), newDevice.swVersion()) ||
-            !isAnnotationsEqual(oldDevice.annotations(), newDevice.annotations())) {
+            !AnnotationsUtil.isEqual(oldDevice.annotations(), newDevice.annotations())) {
 
             boolean replaced = devices.replace(newDevice.id(), oldDevice, newDevice);
             if (!replaced) {
@@ -327,7 +327,7 @@ public class GossipDeviceStore
                                    Port newPort,
                                    Map<PortNumber, Port> ports) {
         if (oldPort.isEnabled() != newPort.isEnabled() ||
-            !isAnnotationsEqual(oldPort.annotations(), newPort.annotations())) {
+            !AnnotationsUtil.isEqual(oldPort.annotations(), newPort.annotations())) {
 
             ports.put(oldPort.number(), newPort);
             return new DeviceEvent(PORT_UPDATED, device, newPort);
@@ -438,32 +438,11 @@ public class GossipDeviceStore
 
     @Override
     public DeviceEvent removeDevice(DeviceId deviceId) {
-        synchronized (this) {
         Device device = devices.remove(deviceId);
+        // FIXME: should we be removing deviceDescs also?
         return device == null ? null :
             new DeviceEvent(DEVICE_REMOVED, device, null);
     }
-    }
-
-    private static boolean isAnnotationsEqual(Annotations lhs, Annotations rhs) {
-        if (lhs == rhs) {
-            return true;
-        }
-        if (lhs == null || rhs == null) {
-            return false;
-        }
-
-        if (!lhs.keys().equals(rhs.keys())) {
-            return false;
-        }
-
-        for (String key : lhs.keys()) {
-            if (!lhs.value(key).equals(rhs.value(key))) {
-                return false;
-            }
-        }
-        return true;
-    }
 
     /**
      * Returns a Device, merging description given from multiple Providers.

@@ -9,7 +9,7 @@ import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
 import org.apache.felix.scr.annotations.Service;
-import org.onlab.onos.net.Annotations;
+import org.onlab.onos.net.AnnotationsUtil;
 import org.onlab.onos.net.DefaultAnnotations;
 import org.onlab.onos.net.DefaultDevice;
 import org.onlab.onos.net.DefaultPort;
@@ -28,6 +28,7 @@ import org.onlab.onos.net.device.DeviceStoreDelegate;
 import org.onlab.onos.net.device.PortDescription;
 import org.onlab.onos.net.provider.ProviderId;
 import org.onlab.onos.store.AbstractStore;
+import org.onlab.util.NewConcurrentHashMap;
 import org.slf4j.Logger;
 
 import java.util.ArrayList;
@@ -109,8 +110,7 @@ public class SimpleDeviceStore
     public synchronized DeviceEvent createOrUpdateDevice(ProviderId providerId, DeviceId deviceId,
                                      DeviceDescription deviceDescription) {
         ConcurrentMap<ProviderId, DeviceDescriptions> providerDescs
-            = createIfAbsentUnchecked(deviceDescs, deviceId,
-                    new InitConcurrentHashMap<ProviderId, DeviceDescriptions>());
+            = getDeviceDescriptions(deviceId);
 
         Device oldDevice = devices.get(deviceId);
 
@@ -151,7 +151,7 @@ public class SimpleDeviceStore
         // We allow only certain attributes to trigger update
         if (!Objects.equals(oldDevice.hwVersion(), newDevice.hwVersion()) ||
             !Objects.equals(oldDevice.swVersion(), newDevice.swVersion()) ||
-            !isAnnotationsEqual(oldDevice.annotations(), newDevice.annotations())) {
+            !AnnotationsUtil.isEqual(oldDevice.annotations(), newDevice.annotations())) {
 
             synchronized (this) {
                 devices.replace(newDevice.id(), oldDevice, newDevice);
@@ -238,7 +238,7 @@ public class SimpleDeviceStore
                                    Port newPort,
                                    ConcurrentMap<PortNumber, Port> ports) {
         if (oldPort.isEnabled() != newPort.isEnabled() ||
-            !isAnnotationsEqual(oldPort.annotations(), newPort.annotations())) {
+            !AnnotationsUtil.isEqual(oldPort.annotations(), newPort.annotations())) {
 
             ports.put(oldPort.number(), newPort);
             return new DeviceEvent(PORT_UPDATED, device, newPort);
@@ -264,11 +264,17 @@ public class SimpleDeviceStore
         return events;
     }
 
+    private ConcurrentMap<ProviderId, DeviceDescriptions> getDeviceDescriptions(
+            DeviceId deviceId) {
+        return createIfAbsentUnchecked(deviceDescs, deviceId,
+                NewConcurrentHashMap.<ProviderId, DeviceDescriptions>ifNeeded());
+    }
+
     // Gets the map of ports for the specified device; if one does not already
     // exist, it creates and registers a new one.
     private ConcurrentMap<PortNumber, Port> getPortMap(DeviceId deviceId) {
         return createIfAbsentUnchecked(devicePorts, deviceId,
-                new InitConcurrentHashMap<PortNumber, Port>());
+                NewConcurrentHashMap.<PortNumber, Port>ifNeeded());
     }
 
     @Override
@@ -325,31 +331,12 @@ public class SimpleDeviceStore
     public DeviceEvent removeDevice(DeviceId deviceId) {
         synchronized (this) {
             Device device = devices.remove(deviceId);
+            // FIXME: should we be removing deviceDescs also?
             return device == null ? null :
                     new DeviceEvent(DEVICE_REMOVED, device, null);
         }
     }
 
-    private static boolean isAnnotationsEqual(Annotations lhs, Annotations rhs) {
-        if (lhs == rhs) {
-            return true;
-        }
-        if (lhs == null || rhs == null) {
-            return false;
-        }
-
-        if (!lhs.keys().equals(rhs.keys())) {
-            return false;
-        }
-
-        for (String key : lhs.keys()) {
-            if (!lhs.value(key).equals(rhs.value(key))) {
-                return false;
-            }
-        }
-        return true;
-    }
-
     /**
      * Returns a Device, merging description given from multiple Providers.
      *
@@ -445,15 +432,6 @@ public class SimpleDeviceStore
         return fallBackPrimary;
     }
 
-    // TODO: can be made generic
-    private static final class InitConcurrentHashMap<K, V> implements
-            ConcurrentInitializer<ConcurrentMap<K, V>> {
-        @Override
-        public ConcurrentMap<K, V> get() throws ConcurrentException {
-            return new ConcurrentHashMap<>();
-        }
-    }
-
     public static final class InitDeviceDescs
         implements ConcurrentInitializer<DeviceDescriptions> {
         private final DeviceDescription deviceDesc;

@@ -126,6 +126,7 @@ public class SimpleDeviceStoreTest {
         assertEquals(SN, device.serialNumber());
     }
 
+    // TODO slice this out somewhere
     /**
      * Verifies that Annotations created by merging {@code annotations} is
      * equal to actual Annotations.
@@ -133,7 +134,7 @@ public class SimpleDeviceStoreTest {
      * @param actual Annotations to check
      * @param annotations
      */
-    private static void assertAnnotationsEquals(Annotations actual, SparseAnnotations... annotations) {
+    public static void assertAnnotationsEquals(Annotations actual, SparseAnnotations... annotations) {
         DefaultAnnotations expected = DefaultAnnotations.builder().build();
         for (SparseAnnotations a : annotations) {
             expected = DefaultAnnotations.merge(expected, a);
@@ -347,6 +348,7 @@ public class SimpleDeviceStoreTest {
         assertFalse("Port is disabled", event.port().isEnabled());
 
     }
+
     @Test
     public final void testUpdatePortStatusAncillary() {
         putDeviceAncillary(DID1, SW1);

@@ -4,7 +4,9 @@ import static org.junit.Assert.*;
 import static org.onlab.onos.net.DeviceId.deviceId;
 import static org.onlab.onos.net.Link.Type.*;
 import static org.onlab.onos.net.link.LinkEvent.Type.*;
+import static org.onlab.onos.store.trivial.impl.SimpleDeviceStoreTest.assertAnnotationsEquals;
 
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
@@ -18,10 +20,12 @@ import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Test;
 import org.onlab.onos.net.ConnectPoint;
+import org.onlab.onos.net.DefaultAnnotations;
 import org.onlab.onos.net.DeviceId;
 import org.onlab.onos.net.Link;
 import org.onlab.onos.net.LinkKey;
 import org.onlab.onos.net.PortNumber;
+import org.onlab.onos.net.SparseAnnotations;
 import org.onlab.onos.net.Link.Type;
 import org.onlab.onos.net.link.DefaultLinkDescription;
 import org.onlab.onos.net.link.LinkEvent;
@@ -37,6 +41,7 @@ import com.google.common.collect.Iterables;
 public class SimpleLinkStoreTest {
 
     private static final ProviderId PID = new ProviderId("of", "foo");
+    private static final ProviderId PIDA = new ProviderId("of", "bar", true);
     private static final DeviceId DID1 = deviceId("of:foo");
     private static final DeviceId DID2 = deviceId("of:bar");
 
@@ -44,6 +49,23 @@ public class SimpleLinkStoreTest {
     private static final PortNumber P2 = PortNumber.portNumber(2);
     private static final PortNumber P3 = PortNumber.portNumber(3);
 
+    private static final SparseAnnotations A1 = DefaultAnnotations.builder()
+            .set("A1", "a1")
+            .set("B1", "b1")
+            .build();
+    private static final SparseAnnotations A1_2 = DefaultAnnotations.builder()
+            .remove("A1")
+            .set("B3", "b3")
+            .build();
+    private static final SparseAnnotations A2 = DefaultAnnotations.builder()
+            .set("A2", "a2")
+            .set("B2", "b2")
+            .build();
+    private static final SparseAnnotations A2_2 = DefaultAnnotations.builder()
+            .remove("A2")
+            .set("B4", "b4")
+            .build();
+
 
     private SimpleLinkStore simpleLinkStore;
     private LinkStore linkStore;
@@ -270,6 +292,59 @@ public class SimpleLinkStoreTest {
     }
 
     @Test
+    public final void testCreateOrUpdateLinkAncillary() {
+        ConnectPoint src = new ConnectPoint(DID1, P1);
+        ConnectPoint dst = new ConnectPoint(DID2, P2);
+
+        // add Ancillary link
+        LinkEvent event = linkStore.createOrUpdateLink(PIDA,
+                    new DefaultLinkDescription(src, dst, INDIRECT, A1));
+
+        assertNull("Ancillary only link is ignored", event);
+
+        // add Primary link
+        LinkEvent event2 = linkStore.createOrUpdateLink(PID,
+                new DefaultLinkDescription(src, dst, INDIRECT, A2));
+
+        assertLink(DID1, P1, DID2, P2, INDIRECT, event2.subject());
+        assertAnnotationsEquals(event2.subject().annotations(), A2, A1);
+        assertEquals(LINK_ADDED, event2.type());
+
+        // update link type
+        LinkEvent event3 = linkStore.createOrUpdateLink(PID,
+                new DefaultLinkDescription(src, dst, DIRECT, A2));
+        assertLink(DID1, P1, DID2, P2, DIRECT, event3.subject());
+        assertAnnotationsEquals(event3.subject().annotations(), A2, A1);
+        assertEquals(LINK_UPDATED, event3.type());
+
+
+        // no change
+        LinkEvent event4 = linkStore.createOrUpdateLink(PID,
+                new DefaultLinkDescription(src, dst, DIRECT));
+        assertNull("No change event expected", event4);
+
+        // update link annotation (Primary)
+        LinkEvent event5 = linkStore.createOrUpdateLink(PID,
+                new DefaultLinkDescription(src, dst, DIRECT, A2_2));
+        assertLink(DID1, P1, DID2, P2, DIRECT, event5.subject());
+        assertAnnotationsEquals(event5.subject().annotations(), A2, A2_2, A1);
+        assertEquals(LINK_UPDATED, event5.type());
+
+        // update link annotation (Ancillary)
+        LinkEvent event6 = linkStore.createOrUpdateLink(PIDA,
+                new DefaultLinkDescription(src, dst, DIRECT, A1_2));
+        assertLink(DID1, P1, DID2, P2, DIRECT, event6.subject());
+        assertAnnotationsEquals(event6.subject().annotations(), A2, A2_2, A1, A1_2);
+        assertEquals(LINK_UPDATED, event6.type());
+
+        // update link type (Ancillary) : ignored
+        LinkEvent event7 = linkStore.createOrUpdateLink(PIDA,
+                new DefaultLinkDescription(src, dst, EDGE));
+        assertNull("Ancillary change other than annotation is ignored", event7);
+    }
+
+
+    @Test
     public final void testRemoveLink() {
         final ConnectPoint d1P1 = new ConnectPoint(DID1, P1);
         final ConnectPoint d2P2 = new ConnectPoint(DID2, P2);
@@ -291,6 +366,30 @@ public class SimpleLinkStoreTest {
         assertLink(linkId2, DIRECT, linkStore.getLink(d2P2, d1P1));
     }
 
+    @Test
+    public final void testAncillaryOnlyNotVisible() {
+        ConnectPoint src = new ConnectPoint(DID1, P1);
+        ConnectPoint dst = new ConnectPoint(DID2, P2);
+
+        // add Ancillary link
+        linkStore.createOrUpdateLink(PIDA,
+                    new DefaultLinkDescription(src, dst, INDIRECT, A1));
+
+        // Ancillary only link should not be visible
+        assertEquals(0, linkStore.getLinkCount());
+
+        assertTrue(Iterables.isEmpty(linkStore.getLinks()));
+
+        assertNull(linkStore.getLink(src, dst));
+
+        assertEquals(Collections.emptySet(), linkStore.getIngressLinks(dst));
+
+        assertEquals(Collections.emptySet(), linkStore.getEgressLinks(src));
+
+        assertEquals(Collections.emptySet(), linkStore.getDeviceEgressLinks(DID1));
+        assertEquals(Collections.emptySet(), linkStore.getDeviceIngressLinks(DID2));
+    }
+
     // If Delegates should be called only on remote events,
     // then Simple* should never call them, thus not test required.
     @Ignore("Ignore until Delegate spec. is clear.")

+package org.onlab.util;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+import org.apache.commons.lang3.concurrent.ConcurrentException;
+import org.apache.commons.lang3.concurrent.ConcurrentInitializer;
+
+/**
+ * Creates an instance of new ConcurrentHashMap on each {@link #get()} call.
+ * <p>
+ * To be used with
+ * {@link org.apache.commons.lang3.concurrent.ConcurrentUtils#createIfAbsent()
+ *  ConcurrentUtils#createIfAbsent}
+ *
+ * @param <K> ConcurrentHashMap key type
+ * @param <V> ConcurrentHashMap value type
+ */
+public final class NewConcurrentHashMap<K, V>
+    implements  ConcurrentInitializer<ConcurrentMap<K, V>> {
+
+    public static final NewConcurrentHashMap<?, ?> INSTANCE = new NewConcurrentHashMap<>();
+
+    @SuppressWarnings("unchecked")
+    public static <K, V> NewConcurrentHashMap<K, V> ifNeeded() {
+        return (NewConcurrentHashMap<K, V>) INSTANCE;
+    }
+
+    @Override
+    public ConcurrentMap<K, V> get() throws ConcurrentException {
+        return new ConcurrentHashMap<>();
+    }
+}